-
Notifications
You must be signed in to change notification settings - Fork 812
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
Re-enable proxy_url for alertmanager receivers #4741
Conversation
Not sure why the test failed, I've tried that locally and did not see this error. Rebased the branch to master to retry this, plus added the CHANGELOG.md change which was missing before. |
CHANGELOG.md
Outdated
@@ -10,6 +10,7 @@ | |||
* [CHANGE] Fix incorrectly named `cortex_cache_fetched_keys` and `cortex_cache_hits` metrics. Renamed to `cortex_cache_fetched_keys_total` and `cortex_cache_hits_total` respectively. #4686 | |||
* [CHANGE] Enable Thanos series limiter in store-gateway. #4702 | |||
* [CHANGE] Distributor: Apply `max_fetched_series_per_query` limit for `/series` API. #4683 | |||
* [CHANGE] Re-enable the `proxy_url` option for receiver configuration. #4680 |
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.
In change log, the convention is to put the PR number instead of the issue number. May I ask you to kindly update it? Thanks!
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.
Of course, done!
Signed-off-by: Patrick Bänziger <[email protected]>
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.
The proxy url is used by the roundtripper in alertmanager to create connections. That proxy connection is http, https or socks5 (per go Transport.Proxy). There is no risk that a rogue tenant could access local files here.
This can be re-enabled.
What this PR does:
It re-enables the
proxy_url
option for alertmanager receivers.Originally introduced with #4129 . We believe this might have been an overly cautious fix and propose to re-enable this option again. This seems to be the consensus in the follow-up discussion. #4129 (comment) by @alvinlin123
Which issue(s) this PR fixes:
Fixes #4680
Checklist
Documentation addedCHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]