docs: add proposal for NotificationPolicyRoute CRD

[msvechla]

As discussed in #1767, this is the proposal for adding a new NotificationPolicyRoute CRD that allows assembling a Notification Policy from individual resources.

Please let me know what you think and thanks for your support!

[Baarsgaard]

I have a few questions/comments.

1. All the reconciliation logic would have to be implemented in the GrafanaNotificationPolicy controller.
   Which is quite different from existing CRDs, but not a problem, there just would not be a reconciliation loop, outside maybe a status updates/finalizers.
   The benefit of a structured config and CR Create/Update validations are probably worth it.

2. I do however not see a reason for the GrafanaNotificationPolicyRoute to contain the common options: instanceSelector or resyncPeriod.
   allowCrossNamespaceImport might still make sense? But I would probably lean towards all or nothing for implementation simplicity.
   allowCrossNamespaceImport could be inherited from GrafanaNotificationPolicy.
   Meaning, a GrafanaNotificationPolicy with allowCrossNamespaceImport: true also imports matching routes from other namespaces.

3. Are you planning on re-using dynamic routes across multiple GrafanaNotificationPolicy CRs, and are there cases where this could not be achieved with the existing instanceSelector, hence the routeSelector?
   An example or two of this would be great.
   Alternatively, I would consider the possibility of re-using the instanceSelector criteria for also finding GrafanaNotificationPolicyRoute, this could be a mistake as then instance is no longer just for filtering instances.

4. It's not shown, but I assume the GrafanaNotificationPolicyRoute is a full route object allowing for further nested routes, but the dynamic assembling is only possible at the top level spec.route.routes?
   routeSelector is only present on GrafanaNotificationPolicy
   It may be necessary to move spec.route.routeSelector to spec.routeSelector to re-use the Route struct for both CRDs

5. I had a point here about the assembling of routes being deterministic and what the tiebreaker is when two priorities are the same.
   but Kubernetes guarantees ordered output according to:
   List objects in deterministic order kubernetes/kubernetes#3812
   kubectl get has random output order kubernetes/kubernetes#4236
   The resulting spec.route.routes should be idempotent if the inputs are the same.

6. Should there be an upper/lower bound on the range of priority?
   1-100 and nil defaults to lowest priority of 100?

[msvechla]

All the reconciliation logic would have to be implemented in the GrafanaNotificationPolicy controller.
Which is quite different from existing CRDs, but not a problem, there just would not be a reconciliation loop, outside maybe a status updates/finalizers.
The benefit of a structured config and CR Create/Update validations are probably worth it.

Thanks, this is good input. I did not look at implementation specifics yet, as I wanted to draft the high-level design first. Definitely makes sense.

allowCrossNamespaceImport could be inherited from GrafanaNotificationPolicy.
Meaning, a GrafanaNotificationPolicy with allowCrossNamespaceImport: true also imports matching routes from other namespaces.

Agreed, I'm also all for simplicity here.

Are you planning on re-using dynamic routes across multiple GrafanaNotificationPolicy CRs, and are there cases where this could not be achieved with the existing instanceSelector, hence the routeSelector?
An example or two of this would be great.
Alternatively, I would consider the possibility of re-using the instanceSelector criteria for also finding GrafanaNotificationPolicyRoute, this could be a mistake as then instance is no longer just for filtering instances.

I don't have such a use-case at the moment and also did not consider it in the design. Imhop we can start with using the instanceSelector and adding further selectors later on if required.

It's not shown, but I assume the GrafanaNotificationPolicyRoute is a full route object allowing for further nested routes, but the dynamic assembling is only possible at the top level spec.route.routes?

Yes correct, that was my idea.

routeSelector is only present on GrafanaNotificationPolicy
It may be necessary to move spec.route.routeSelector to spec.routeSelector to re-use the Route struct for both CRDs

Good point, yes we would have to move it to spec.routeSelector, but if we agree on using the instanceSelector, this can even be discarded entirely for now.

I had a point here about the assembling of routes being deterministic and what the tiebreaker is when two priorities are the same.
but Kubernetes guarantees ordered output according to:
kubernetes/kubernetes#3812
kubernetes/kubernetes#4236
The resulting spec.route.routes should be idempotent if the inputs are the same.

Thanks, noted! Definitely the resulting routes should be idempotent.

Should there be an upper/lower bound on the range of priority?
1-100 and nil defaults to lowest priority of 100?

I'm open for input here, but I think your suggestion makes sense.

So should we agree on using the instanceSelector for selecting possible GrafanaNotificationPolicyRoute resources to merge? Thanks a lot for your input!

[msvechla]

Actually as I'm working on a draft implementation, I think it makes more sense to go with the routeSelector instead of re-using the instanceSelector on GrafanaNotificationPolicyRoute.

My reasoning behind this is the following:

• during the reconcile loop in notificationpolicy_controller.go, we have to fetch all matching GrafanaNotificationPolicyRoutes for the currently reconciled GrafanaNotificationPolicy
• this can be very easily achieved with a routeSelector, which will be a Kubernetes LabelSelector
• if we would go with instanceSelector, we would have to fetch all available GrafanaNotificationPolicyRoutes and then do some filtering afterwards, to see if the instanceSelector matches, which would be both more inefficient and more complex

So I think adding an optional routeSelector to GrafanaNotificationPolicy makes more sense now.

Please let me know your thoughts of course, thanks a lot!

[msvechla]

I updated the feature proposal here with what we discussed.

Also I have a draft PR that has a working implementation of this feature to support the proposal: #1800

Let me know what you think and thanks for your support!