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

Alerting: Feature toggle to disallow sending alerts externally #87982

Merged
merged 2 commits into from
May 23, 2024

Conversation

stevesg
Copy link
Contributor

@stevesg stevesg commented May 16, 2024

What is this feature?

Adds a feature toggle to disallow sending alerts to external Alertmanager datasources.

  • If a user already has an admin configuration, this is ignored and a warning logged.
  • If a user tries to write an admin configuration to send to external Alertmanagers, it is rejected.

Note that when reading the admin configuration, the persisted configuration is returned. I'm not entirely convinced this is correct, perhaps we should return the effective configuration.

Note this commit does not include frontend changes. We will likely want to modify the Settings page to not show the option to configure external Alertmanagers.

Why do we need this feature?

Site policy may dictate that alerts should be handled only internally, or if they are sent externally, should be done via the new Remote Alertmanager feature.

Note: There is no intention to remove the ability to send externally, this is merely being provided as an option for site administrators who want to disallow it.

Who is this feature for?

Site administrators of Grafana instances.

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone May 16, 2024
@stevesg stevesg changed the title Alerting: Feature toggle to disallow Alerting: Feature toggle to disallow sending alerts externally May 16, 2024
@stevesg stevesg force-pushed the stevesg/disable-send-external-alerts-feature-toggle branch from 0f3ea0e to a900ca0 Compare May 16, 2024 13:32
@stevesg stevesg marked this pull request as ready for review May 16, 2024 18:15
@stevesg stevesg requested review from grafanabot and a team as code owners May 16, 2024 18:15
@stevesg stevesg requested review from jtheory, rwwiv, JacobsonMT, yuri-tceretian and grobinson-grafana and removed request for a team May 16, 2024 18:15
@stevesg stevesg force-pushed the stevesg/disable-send-external-alerts-feature-toggle branch from a900ca0 to 9b622eb Compare May 16, 2024 18:20
@santihernandezc santihernandezc self-requested a review May 21, 2024 14:32
Copy link
Contributor

@santihernandezc santihernandezc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I got the warning log line but the UI still shows the selected option, which is confusing for users unaware of the feature toggle being active.

WARN [05-21|16:21:23] Alertmanager choice in configuration will be ignored due to feature flags logger=ngalert.sender.router org=1 choice=all
Screenshot 2024-05-21 at 16 23 22

The admin config is not modified in the database, which is ok by me, especially if someone wants to disable this feature toggle and get alerts routed to external Alertmanagers without having to re-configure that routing.

I created an issue for hiding the "Send alerts to" options in the front end.

@stevesg stevesg force-pushed the stevesg/disable-send-external-alerts-feature-toggle branch from 9b622eb to 8dbe756 Compare May 23, 2024 07:18
@stevesg stevesg added the no-changelog Skip including change in changelog/release notes label May 23, 2024
@stevesg stevesg force-pushed the stevesg/disable-send-external-alerts-feature-toggle branch from 8dbe756 to 4dae2cd Compare May 23, 2024 11:18
@stevesg stevesg merged commit 8421919 into main May 23, 2024
18 checks passed
@stevesg stevesg deleted the stevesg/disable-send-external-alerts-feature-toggle branch May 23, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/frontend no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants