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

Fix: NotificationPolicy LoopDetected condition and route.receiver validation #1843

Conversation

Baarsgaard
Copy link
Contributor

@Baarsgaard Baarsgaard commented Jan 30, 2025

While working on #1837 on I was unable to provoke the LoopDetected status and discovered that it was removed immediately after being set, the detection itself seems to work without issues.
The above resulted in loops being detected, dynamic routes being ignored and an ApplySucceeded condition on GrafanaNotificationPolicy CRs
This means that previously working NotificationPolicy trees with dynamic routes would be overwritten with only the static routes in the main GrafanaNotificationPolicy, if any.

This changes the behaviour from applying only static routes and ignoring everything dynamic to refusing updates entirely.
LoopDetected is now before fetching instances resulting in it having higher priority.
InvalidSpec is an error and the NoMatchingInstances is not.

receiver is now a required field due to the current implementation always returning 400 BAD REQUEST when trying to apply a route when the receiver is omitted.
Lastly, the grafana-operator category was missing from the GrafanaNotificationPolicyRoute

chore: NotificationPolicyRoute missing category
@github-actions github-actions bot added documentation Issues relating to documentation, missing, non-clear etc. bugfix this PR fixes a bug labels Jan 30, 2025
@Baarsgaard Baarsgaard marked this pull request as ready for review January 31, 2025 09:11
@Baarsgaard Baarsgaard force-pushed the fix_notificationpolicy_conditions_and_validations branch from 1a90230 to e656569 Compare February 2, 2025 11:55
Copy link
Member

@theSuess theSuess 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, I thought this was covered in the E2E tests

left a small comment but good to merge afterward

controllers/notificationpolicy_controller.go Outdated Show resolved Hide resolved
@Baarsgaard Baarsgaard force-pushed the fix_notificationpolicy_conditions_and_validations branch from e5e94e7 to 502168b Compare February 3, 2025 18:12
@Baarsgaard
Copy link
Contributor Author

Baarsgaard commented Feb 4, 2025

I noticed that status.discoveredRoutes is unstable and the list is slightly random when multiple routes match a single selector.
Added sort to prevent unnecessary status changes.

@theSuess theSuess added this pull request to the merge queue Feb 4, 2025
@theSuess theSuess added this to the v5.17.0 milestone Feb 4, 2025
Merged via the queue into grafana:master with commit 4e11eba Feb 4, 2025
15 checks passed
@Baarsgaard Baarsgaard deleted the fix_notificationpolicy_conditions_and_validations branch February 4, 2025 11:59
@msvechla
Copy link
Contributor

msvechla commented Feb 5, 2025

Thanks for catching this from my previous PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix this PR fixes a bug documentation Issues relating to documentation, missing, non-clear etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants