Dynamic Notification Policy Routes

[msvechla]

See #1789 for the related feature proposal.

This implementation is still work in progress for now and should support the feature proposal.

Updates on GrafanaNotificationPolicyRoutes are tracked via watches. I first tried to accomplish this via ownerReferences (see commits), but this is not supported cross-namespace.

---

Example of how the sample policies in hack/kind/resources/default/grafana-notification-policy.yaml are rendered:

Status updates on the GrafanaNotificationPolicy

❯ kubectl get grafananotificationpolicy grafananotificationpolicy-sample -o jsonpath="{.status}" |jq

{
  "conditions": [
    {
      "lastTransitionTime": "2024-12-17T15:52:31Z",
      "message": "Notification Policy was successfully applied to 1 instances",
      "observedGeneration": 9,
      "reason": "ApplySuccessful",
      "status": "True",
      "type": "NotificationPolicySynchronized"
    }
  ],
  "discoveredRoutes": [
    "grafana-crds/dynamic-c (priority: 1)",
    "default/dynamic-d (priority: 2)",
    "default/dynamic-e (priority: nil)"
  ]
}

Events emitted for the merged routes:

❯ kubectl get events

LAST SEEN   TYPE     REASON   OBJECT                                     MESSAGE
20m         Normal   Merged   grafananotificationpolicyroute/dynamic-c   Route merged into NotificationPolicy default/grafananotificationpolicy-sample
3s          Normal   Merged   grafananotificationpolicyroute/dynamic-d   Route merged into NotificationPolicy default/grafananotificationpolicy-sample
3s          Normal   Merged   grafananotificationpolicyroute/dynamic-e   Route merged into NotificationPolicy default/grafananotificationpolicy-sample

[github-actions]

This PR hasn't been updated for a while, marking as stale

[msvechla]

I have updated the draft PR with the latest discussions from #1789:

• RouteSelector has been moved to the Route object, allowing to dynamically inject routes on all levels now
• Logic to detect reference loops has been implemented
• Tests have been added accordingly
• Priority logic has been removed

There are no checks for RouteSelector and Routes being mutually exclusive so far.
Unfortunately I think we can not implement this validation on the OpenAPI level, as the Routes attribute uses // +kubebuilder:validation:Schemaless, which disables all validation.

Another idea would be to solve this by implementing a ValidationWebhook. @theSuess do you have any thoughts on this?

Results of updates Samples

Assembled hack/kind/resources/default/grafana-notification-policy.yaml:

❮ kubectl get grafananotificationpolicy grafananotificationpolicy-sample -o jsonpath="{.status}" |jq

{
  "conditions": [
    {
      "lastTransitionTime": "2025-01-21T09:48:19Z",
      "message": "Notification Policy was successfully applied to 1 instances",
      "observedGeneration": 1,
      "reason": "ApplySuccessful",
      "status": "True",
      "type": "NotificationPolicySynchronized"
    }
  ],
  "discoveredRoutes": [
    "default/dynamic-e",
    "grafana-crds/dynamic-c",
    "default/dynamic-d"
  ]
}

❯ kubectl get events

LAST SEEN   TYPE     REASON   OBJECT                                     MESSAGE
20m         Normal   Merged   grafananotificationpolicyroute/dynamic-d   Route merged into NotificationPolicy default/grafananotificationpolicy-sample
20m         Normal   Merged   grafananotificationpolicyroute/dynamic-d   Route merged into NotificationPolicy default/grafananotificationpolicy-sample
20m         Normal   Merged   grafananotificationpolicyroute/dynamic-e   Route merged into NotificationPolicy default/grafananotificationpolicy-sample
20m         Normal   Merged   grafananotificationpolicyroute/dynamic-e   Route merged into NotificationPolicy default/grafananotificationpolicy-sample

[msvechla]

I rebased on the latest master branch.

Additionally I looked into the ValidationWebhook for ensuring that routeSelector and routes are mutually exlusive.

While the implementation of the logic is quite straight-forward, setting up webhooks is a bit more complex, especially bringing this to the Helm chart.

I can certainly do this if you think this is the way forward, we can find another way to do the validation, or maybe skip this validation for now.

Any thoughts?

[theSuess]

Sorry for the deleted comment, missed the part about api level validation rules in your previous note.

We had some discussions around Validation Webhooks before and came to the conclusion that these are too finicky to get right.

If the validation rules don't work, I'd just go for a status condition in the respective object when both values are set which tells the user that the dynamic matching takes priority and the routes field is ignored

[msvechla]

Unfortunately, as far as I can see, the validation expressions will not work due to the +kubebuilder:validation:Schemaless on the Routes object. I can see if I can find a workaround.

Alternatively, I will go with the hint in the status condition as you suggested 👍

[msvechla]

I hope I caught everything now, if not let me know!

Thanks a lot for your support! I learned a lot and this was a really nice experience!

[Baarsgaard]

The synchronize condition is specifically used when talking about API calls to a Grafana instance.
For other errors, the plan is to re-use or create new shared conditions to use.
Or potentially look into publishing events as you have done here 😉.
Hence, putting it right after the apply/synchronize loop makes the most sense.
a simpler defer is generally better also.

This also allows you to remove the applyError[globalApplyError] which could confuse users that have a Grafana instance named global(Guilty)

As a side note, dissimilar to other projects' use of conditions, Grafana-Operator removes old conditions immediately rather than setting the status False.
This avoids multiple conditions where you have to look for the ones that are True

[msvechla]

@Baarsgaard I tried to adapt this in the latest commit 00ebd92

• reverted changes that moved building synchronized condition to defer
• added a specific error type for loop detected errors
• set condition for specific assembly errors and emit events otherwise

Thanks for your explanation. If the synchronized condition was built specifically for Grafana API errors, then of course it makes sense to adapt this.

[Baarsgaard]

Huge fan of the changes to the Reconcile function, it's a lot simpler now :D

[theSuess]

Thanks again for all the work on this! I've pushed a small cleanup commit and queued this for merging. Looks like the next release will be full of cool new features!