#2044 reenable proxy_url #2050
#2044 reenable proxy_url #2050
Conversation
|
|
supergicko
force-pushed
the
2044/reenable-proxyurl
branch
from
5588172 to
a63f010
Compare
There was a problem hiding this comment.
Thanks for opening this PR. I prefer to keep it disallowed by default, but I'm open to add an "advanced" configuration option to enable it. The configuration option should be mentioned in the doc at docs/sources/operators-guide/securing/securing-alertmanager.md.
supergicko
force-pushed
the
2044/reenable-proxyurl
branch
from
a63f010 to
fa5c527
Compare
|
updated based on #2044 (comment) |
| @@ -32,7 +32,7 @@ | |||
| * [BUGFIX] Ring: fix bug where instances may appear unhealthy in the hash ring web UI even though they are not. #1933 | |||
| * [BUGFIX] API: gzip is now enforced when identity encoding is explicitly rejected. #1864 | |||
| * [BUGFIX] Fix panic at startup when Mimir is running in monolithic mode and query sharding is enabled. #2036 | |||
|
|
|||
| * [CHANGE] Re-enable the `proxy_url` option for receiver configuration. #2044 | |||
There was a problem hiding this comment.
A couple of comments about this:
- Please link the PR number
#2050instead of the issue one - We're used to group CHANGELOG entries together, so CHANGE first, then FEATURE, then ENHANCEMENT and finally BUGFIX. See our contributing guidelines for more info: https://github.com/grafana/mimir/tree/main/docs/internal/contributing#changelog
- CHANGE is for breaking changes, but this is not a breaking change. ENHANCEMENT looks a better fit.
| @@ -422,15 +422,9 @@ func validateReceiverHTTPConfig(cfg commoncfg.HTTPClientConfig) error { | |||
| if cfg.BearerTokenFile != "" { | |||
| return errPasswordFileNotAllowed | |||
| } | |||
| if cfg.ProxyURL.URL != nil { | |||
| return errProxyURLNotAllowed | |||
There was a problem hiding this comment.
Are we still using errProxyURLNotAllowed? I would have expected that variable to be removed too.
| if cfg.OAuth2 != nil && cfg.OAuth2.ClientSecretFile != "" { | ||
| return errOAuth2SecretFileNotAllowed | ||
| } | ||
| if cfg.OAuth2 != nil && cfg.OAuth2.ProxyURL.URL != nil { |
There was a problem hiding this comment.
Earlier today I opened the PR #2076 to test the firewall works with cfg.ProxyURL.URL. Do think it's feasible to have a similar test on cfg.OAuth2.ProxyURL.URL too?
|
This PR was mostly good. I'm taking over to get it through the finish line. |
|
Superseeded by #2317. |
What this PR does
It re-enables the proxy_url option for alertmanager receivers.
See #2044 and the related cortex PR: cortexproject/cortex#4741
Which issue(s) this PR fixes or relates to
Fixes #2044
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]