-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
base: main
Are you sure you want to change the base?
Conversation
pkg/alertmanager/amcfg.go
Outdated
@@ -1724,6 +1724,11 @@ func (hc *httpClientConfig) sanitize(amVersion semver.Version, logger log.Logger | |||
return err | |||
} | |||
|
|||
if hc.ProxyURL != "" { | |||
_, err := url.Parse(hc.ProxyURL) |
There was a problem hiding this comment.
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.
prometheus-operator/pkg/alertmanager/amcfg.go
Lines 1783 to 1785 in 4664990
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 = "" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"))
. 🤔
There was a problem hiding this 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()
@simonpasquier done the above change |
Here is a short version to get you started: simonpasquier@400c2c5 |
…-operator#6466) validate in convertHttpConfig
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.