Skip to content

Disable access to Alertmanager (user, or sending alerts), if it is not running. #3679

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

Merged
merged 8 commits into from
Jan 15, 2021
Merged

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Jan 12, 2021

What this PR does: This PR disables access to user's Alertmanager (by user, or services trying to send alert) until MultitenantAlertmanager has started and synced configuration for each user.

Call to tenant's AM before MultitenantAlertmanager is running may result in loss of configuration for this tenant.

Checklist

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

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
gouthamve
gouthamve previously approved these changes Jan 12, 2021
@gouthamve gouthamve dismissed their stale review January 12, 2021 11:58

Tests are failing

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

Is this enough? In addition, shouldn't we require a check to the backend before storing the fallback? Othwerwise, we could end up in a similar situation in the future. For instance, once we start sharding Alertmanager tenants if a request somehow gets routed to an Alertmanager without the tenant loaded it will override the config.

@pstibrany
Copy link
Contributor Author

Is this enough?

I think yes, for now.

In addition, shouldn't we require a check to the backend before storing the fallback? Othwerwise, we could end up in a similar situation in the future. For instance, once we start sharding Alertmanager tenants if a request somehow gets routed to an Alertmanager without the tenant loaded it will override the config.

That's a valid point, and something keep an eye on when reviewing AM sharding. I think we should also reconsider idea of uploading "empty" config to the same path as user-uploaded config. Eg. we could give empty config ".default" suffix.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 12, 2021
Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

Good catch @pstibrany !

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM modulo changelog suggestion

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany pstibrany merged commit 2b174a5 into cortexproject:master Jan 15, 2021
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.

5 participants