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

feat: add support for notification policies #1610

Merged
merged 6 commits into from
Aug 5, 2024

Conversation

theSuess
Copy link
Member

This PR adds support for Notification Policies.

I've recreated the Route struct to support Kubernetes based validation.

One concern is that only one Notification Policy can be applied to an instance. If a user creates two notification policies which match the same instance, they will flip between each other. I have not yet come up with a clever (and performant) safeguard against this.

@theSuess theSuess force-pushed the feat/notification-policy-cr branch 2 times, most recently from 649b734 to 7eca93e Compare July 15, 2024 12:57
@NissesSenap
Copy link
Collaborator

Sitting in the car reading the PR so not too easy to do research, so just thinking out loud.

I guess the natural solution is to add validation webhook support to the operator, which is a pain.

Could another option be to do something with status on the grafana instance? When the reconciler adds the initial notification policy, write a status. And if a new policy gets added, error out and write a status on the new policy?
I guess similar logic would be needed in the validation webhook, the big difference is that it wouldn't be triggered for each reconciliation.

@theSuess
Copy link
Member Author

That's a good idea! Maybe not in the status field (as IMHO this should be owned by the grafana reconciler) but an annotation like operator.grafana.com/applied-notification-policy: namespace/notification-policy-name/uid could help here

@theSuess theSuess force-pushed the feat/notification-policy-cr branch from 7eca93e to 78f0175 Compare July 16, 2024 08:50
@NissesSenap
Copy link
Collaborator

Could an idea be to use watch https://book.kubebuilder.io/reference/watching-resources/externally-managed to listen for the grafana instances.

So if there are 2 policies for the same instance, and the policy that was created first is deleted. The annotation on grafana will be deleted as part of the delete process. There will be no way for the second policy to be added (except a retry of the reconciler), but if we have a watcher we should be able to check if the annotation has changed and then start to reconcile the "new" policy.

@theSuess theSuess force-pushed the feat/notification-policy-cr branch from 43bde21 to 6e7a1fd Compare July 17, 2024 12:46
@theSuess theSuess force-pushed the feat/notification-policy-cr branch 4 times, most recently from 35c4d9f to b1b2695 Compare July 22, 2024 08:16
@theSuess theSuess force-pushed the feat/notification-policy-cr branch from b1b2695 to 8fe698f Compare July 22, 2024 08:27
@theSuess
Copy link
Member Author

The annotation based locking approach is now implemented. I tried my hand at event watchers but couldn't figure out a good way to trigger reconciliation once the Grafana instance is freed (happy to take input here).

Other than that, this PR is ready for code review from my side

@michelle-douglas
Copy link

Hi Team, Can I get an update on this? I have a customer (Alkami Technology) who is waiting on this and has offered to test it with us if needed. Please advise. Thanks! Michelle

@theSuess theSuess enabled auto-merge August 5, 2024 11:42
@theSuess theSuess added this pull request to the merge queue Aug 5, 2024
Merged via the queue into master with commit 76a4cde Aug 5, 2024
14 checks passed
@theSuess theSuess deleted the feat/notification-policy-cr branch August 5, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants