Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat:validate proxyURL for alertManagerCfg (#6466) #6481

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yp969803
Copy link
Contributor

@yp969803 yp969803 commented Apr 5, 2024

Description

fixes: #6466
added validation for proxyURL for alertManagerCfg

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Verification

written unit test

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

validate proxyURL for alertManagerCfg

@yp969803 yp969803 requested a review from a team as a code owner April 5, 2024 17:30
@@ -1724,6 +1724,11 @@ func (hc *httpClientConfig) sanitize(amVersion semver.Version, logger log.Logger
return err
}

if hc.ProxyURL != "" {
_, err := url.Parse(hc.ProxyURL)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taking the idea of validation on proxyurl obj of oauth2 type would be better.

msg := "'proxy_url' set in 'oauth2' but supported in Alertmanager >= 0.25.0 only - dropping field from provided config"
level.Warn(logger).Log("msg", msg, "current_version", amVersion.String())
o.ProxyURL = ""

cc @simonpasquier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @afzal442 don't follow what you mean 😄
The code snippet you shared is only about log message.
Could you clarify?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I see. I didn't look into that much. Nice caught. Parsing was missing. I think only one extra condition to be met here is !amVersion.GTE(semver.MustParse("0.25.0")). 🤔

Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need also to add a check to convertHTTPConfig()

@yp969803
Copy link
Contributor Author

yp969803 commented May 4, 2024

@simonpasquier done the above change

@simonpasquier
Copy link
Contributor

Sorry reading again the code, I suggest that you add the validation here and here.

@simonpasquier
Copy link
Contributor

Here is a short version to get you started: simonpasquier@400c2c5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add proxyURL validation for the AlertmanagerConfig CRD.
4 participants