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

Allow to override Alertmanager receivers firewall settings on a per-tenant basis #4143

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Apr 29, 2021

What this PR does:
In this PR #4085 I've added an experimental support to configure a simple built-in firewall in Alertmanger for outbound connections made by HTTP-based receiver integrations.

To extend it, in this PR I'm proposing to allow to configure it on a per-tenant basis.

Code changes have been designed to limit conflicts with #4135.

Which issue(s) this PR fixes:
N/A

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

…enant basis

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM, good job.

Copy link
Contributor

@stevesg stevesg left a comment

Choose a reason for hiding this comment

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

LGTM.

My only comment: I was considering if we could re-use FirewallConfig in Limits, but I see then you would only be able to override the whole config, not individual elements of the config, without some extra merging logic. Maybe as the Limits grows, we can find a nice pattern for doing this.

@pracucci pracucci merged commit 753f3b8 into cortexproject:master Apr 29, 2021
@pracucci pracucci deleted the allow-to-configure-alertmanager-firewall-on-a-per-tenant-basis branch April 29, 2021 09:55
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.

3 participants