Fix: NotificationPolicy LoopDetected condition and route.receiver v…

…alidation (#1843) * fix: reciever is a required field chore: NotificationPolicyRoute missing category * fix: loopDetected condition never applied * refactor: Simplify if condition * fix: sort notification policy status.discoveredRoutes
grafana · Feb 4, 2025 · 4e11eba · 4e11eba
1 parent c24b47f
commit 4e11eba
Show file tree

Hide file tree

Showing 10 changed files with 70 additions and 45 deletions.
diff --git a/api/v1beta1/grafananotificationpolicy_types.go b/api/v1beta1/grafananotificationpolicy_types.go
@@ -66,7 +66,8 @@ type Route struct {
 	Provenance models.Provenance `json:"provenance,omitempty"`
 
 	// receiver
-	Receiver string `json:"receiver,omitempty"`
+	// +kubebuilder:validation:MinLength=1
+	Receiver string `json:"receiver"`
 
 	// repeat interval
 	RepeatInterval string `json:"repeat_interval,omitempty"`

diff --git a/api/v1beta1/grafananotificationpolicy_types_test.go b/api/v1beta1/grafananotificationpolicy_types_test.go
@@ -31,6 +31,7 @@ func newNotificationPolicy(name string, editable *bool) *GrafanaNotificationPoli
 			},
 			Route: &Route{
 				Continue:          false,
+				Receiver:          "grafana-default-email",
 				GroupBy:           []string{"group_name", "alert_name"},
 				MuteTimeIntervals: []string{},
 				Routes:            []*Route{},

diff --git a/api/v1beta1/grafananotificationpolicyroute_types.go b/api/v1beta1/grafananotificationpolicyroute_types.go
@@ -38,6 +38,7 @@ type GrafanaNotificationPolicyRouteSpec struct {
 //+kubebuilder:subresource:status
 
 // GrafanaNotificationPolicyRoute is the Schema for the grafananotificationpolicyroutes API
+// +kubebuilder:resource:categories={grafana-operator}
 type GrafanaNotificationPolicyRoute struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

diff --git a/config/crd/bases/grafana.integreatly.org_grafananotificationpolicies.yaml b/config/crd/bases/grafana.integreatly.org_grafananotificationpolicies.yaml
@@ -183,6 +183,7 @@ spec:
                     type: string
                   receiver:
                     description: receiver
+                    minLength: 1
                     type: string
                   repeat_interval:
                     description: repeat interval
@@ -238,6 +239,8 @@ spec:
                   routes:
                     description: routes, mutually exclusive with RouteSelector
                     x-kubernetes-preserve-unknown-fields: true
+                required:
+                - receiver
                 type: object
             required:
             - instanceSelector

diff --git a/config/crd/bases/grafana.integreatly.org_grafananotificationpolicyroutes.yaml b/config/crd/bases/grafana.integreatly.org_grafananotificationpolicyroutes.yaml
@@ -8,6 +8,8 @@ metadata:
 spec:
   group: grafana.integreatly.org
   names:
+    categories:
+    - grafana-operator
     kind: GrafanaNotificationPolicyRoute
     listKind: GrafanaNotificationPolicyRouteList
     plural: grafananotificationpolicyroutes
@@ -102,6 +104,7 @@ spec:
                 type: string
               receiver:
                 description: receiver
+                minLength: 1
                 type: string
               repeat_interval:
                 description: repeat interval
@@ -157,6 +160,8 @@ spec:
               routes:
                 description: routes, mutually exclusive with RouteSelector
                 x-kubernetes-preserve-unknown-fields: true
+            required:
+            - receiver
             type: object
           status:
             description: The most recent observed state of a Grafana resource

diff --git a/controllers/notificationpolicy_controller.go b/controllers/notificationpolicy_controller.go
@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"slices"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -116,6 +117,31 @@ func (r *GrafanaNotificationPolicyReconciler) Reconcile(ctx context.Context, req
 	}
 	removeInvalidSpec(&notificationPolicy.Status.Conditions)
 
+	// Assemble routes and check for loops
+	var mergedRoutes []*v1beta1.GrafanaNotificationPolicyRoute
+	if notificationPolicy.Spec.Route.HasRouteSelector() {
+		mergedRoutes, err = assembleNotificationPolicyRoutes(ctx, r.Client, notificationPolicy)
+
+		if errors.Is(err, ErrLoopDetected) {
+			meta.SetStatusCondition(&notificationPolicy.Status.Conditions, metav1.Condition{
+				Type:               conditionNotificationPolicyLoopDetected,
+				Status:             metav1.ConditionTrue,
+				ObservedGeneration: notificationPolicy.Generation,
+				Reason:             "LoopDetected",
+				Message:            fmt.Sprintf("Loop detected in notification policy routes: %s", err.Error()),
+			})
+			meta.RemoveStatusCondition(&notificationPolicy.Status.Conditions, conditionNotificationPolicySynchronized)
+			return ctrl.Result{}, fmt.Errorf("failed to assemble notification policy routes: %w", err)
+		}
+
+		if err != nil {
+			r.Recorder.Event(notificationPolicy, corev1.EventTypeWarning, "AssemblyFailed", fmt.Sprintf("Failed to assemble GrafanaNotificationPolicy using routeSelectors: %v", err))
+			return ctrl.Result{}, fmt.Errorf("failed to assemble GrafanaNotificationPolicy using routeSelectors: %w", err)
+		}
+	}
+
+	meta.RemoveStatusCondition(&notificationPolicy.Status.Conditions, conditionNotificationPolicyLoopDetected)
+
 	instances, err := GetScopedMatchingInstances(ctx, r.Client, notificationPolicy)
 	if err != nil {
 		setNoMatchingInstancesCondition(&notificationPolicy.Status.Conditions, notificationPolicy.Generation, err)
@@ -132,16 +158,6 @@ func (r *GrafanaNotificationPolicyReconciler) Reconcile(ctx context.Context, req
 	removeNoMatchingInstance(&notificationPolicy.Status.Conditions)
 	log.Info("found matching Grafana instances for notificationPolicy", "count", len(instances))
 
-	var mergedRoutes []*v1beta1.GrafanaNotificationPolicyRoute
-
-	if notificationPolicy.Spec.Route.RouteSelector != nil || notificationPolicy.Spec.Route.HasRouteSelector() {
-		mergedRoutes, err = assembleNotificationPolicyRoutes(ctx, r.Client, notificationPolicy)
-		if err := r.handleAssembleError(err, notificationPolicy); err != nil {
-			return ctrl.Result{}, err
-		}
-	}
-	meta.RemoveStatusCondition(&notificationPolicy.Status.Conditions, conditionNotificationPolicyLoopDetected)
-
 	applyErrors := make(map[string]string)
 	for _, grafana := range instances {
 		// can be removed in go 1.22+
@@ -442,6 +458,8 @@ func statusDiscoveredRoutes(routes []*v1beta1.GrafanaNotificationPolicyRoute) []
 	for i, route := range routes {
 		discoveredRoutes[i] = fmt.Sprintf("%s/%s", route.Namespace, route.Name)
 	}
+	// Reduce status updates by ensuring order of routes
+	slices.Sort(discoveredRoutes)
 
 	return discoveredRoutes
 }
@@ -450,23 +468,3 @@ func statusDiscoveredRoutes(routes []*v1beta1.GrafanaNotificationPolicyRoute) []
 func setInvalidSpecMutuallyExclusive(conditions *[]metav1.Condition, generation int64) {
 	setInvalidSpec(conditions, generation, "FieldsMutuallyExclusive", "RouteSelector and Routes are mutually exclusive")
 }
-
-// handleAssembleError handles errors during notification policy assembly
-func (r *GrafanaNotificationPolicyReconciler) handleAssembleError(err error, notificationPolicy *grafanav1beta1.GrafanaNotificationPolicy) error {
-	if err == nil {
-		return nil
-	}
-
-	if errors.Is(err, ErrLoopDetected) {
-		meta.SetStatusCondition(&notificationPolicy.Status.Conditions, metav1.Condition{
-			Type:               conditionNotificationPolicyLoopDetected,
-			Status:             metav1.ConditionTrue,
-			ObservedGeneration: notificationPolicy.Generation,
-			Reason:             "LoopDetected",
-			Message:            fmt.Sprintf("Loop detected in notification policy routes: %s", err.Error()),
-		})
-		return nil
-	}
-	r.Recorder.Event(notificationPolicy, corev1.EventTypeWarning, "AssemblyFailed", fmt.Sprintf("Failed to assemble GrafanaNotificationPolicy using routeSelectors: %v", err))
-	return fmt.Errorf("failed to assemble GrafanaNotificationPolicy using routeSelectors: %w", err)
-}
diff --git a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicies.yaml b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicies.yaml
@@ -183,6 +183,7 @@ spec:
                     type: string
                   receiver:
                     description: receiver
+                    minLength: 1
                     type: string
                   repeat_interval:
                     description: repeat interval
@@ -238,6 +239,8 @@ spec:
                   routes:
                     description: routes, mutually exclusive with RouteSelector
                     x-kubernetes-preserve-unknown-fields: true
+                required:
+                - receiver
                 type: object
             required:
             - instanceSelector

diff --git a/...y/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yaml b/...y/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yaml
@@ -8,6 +8,8 @@ metadata:
 spec:
   group: grafana.integreatly.org
   names:
+    categories:
+    - grafana-operator
     kind: GrafanaNotificationPolicyRoute
     listKind: GrafanaNotificationPolicyRouteList
     plural: grafananotificationpolicyroutes
@@ -102,6 +104,7 @@ spec:
                 type: string
               receiver:
                 description: receiver
+                minLength: 1
                 type: string
               repeat_interval:
                 description: repeat interval
@@ -157,6 +160,8 @@ spec:
               routes:
                 description: routes, mutually exclusive with RouteSelector
                 x-kubernetes-preserve-unknown-fields: true
+            required:
+            - receiver
             type: object
           status:
             description: The most recent observed state of a Grafana resource

diff --git a/deploy/kustomize/base/crds.yaml b/deploy/kustomize/base/crds.yaml
@@ -2157,6 +2157,7 @@ spec:
                     type: string
                   receiver:
                     description: receiver
+                    minLength: 1
                     type: string
                   repeat_interval:
                     description: repeat interval
@@ -2212,6 +2213,8 @@ spec:
                   routes:
                     description: routes, mutually exclusive with RouteSelector
                     x-kubernetes-preserve-unknown-fields: true
+                required:
+                - receiver
                 type: object
             required:
             - instanceSelector
@@ -2311,6 +2314,8 @@ metadata:
 spec:
   group: grafana.integreatly.org
   names:
+    categories:
+    - grafana-operator
     kind: GrafanaNotificationPolicyRoute
     listKind: GrafanaNotificationPolicyRouteList
     plural: grafananotificationpolicyroutes
@@ -2405,6 +2410,7 @@ spec:
                 type: string
               receiver:
                 description: receiver
+                minLength: 1
                 type: string
               repeat_interval:
                 description: repeat interval
@@ -2460,6 +2466,8 @@ spec:
               routes:
                 description: routes, mutually exclusive with RouteSelector
                 x-kubernetes-preserve-unknown-fields: true
+            required:
+            - receiver
             type: object
           status:
             description: The most recent observed state of a Grafana resource

diff --git a/docs/docs/api.md b/docs/docs/api.md
@@ -3925,6 +3925,13 @@ Routes for alerts to match against
         </tr>
     </thead>
     <tbody><tr>
+        <td><b>receiver</b></td>
+        <td>string</td>
+        <td>
+          receiver<br/>
+        </td>
+        <td>true</td>
+      </tr><tr>
         <td><b>continue</b></td>
         <td>boolean</td>
         <td>
@@ -3987,13 +3994,6 @@ Routes for alerts to match against
           provenance<br/>
         </td>
         <td>false</td>
-      </tr><tr>
-        <td><b>receiver</b></td>
-        <td>string</td>
-        <td>
-          receiver<br/>
-        </td>
-        <td>false</td>
       </tr><tr>
         <td><b>repeat_interval</b></td>
         <td>string</td>
@@ -4341,6 +4341,13 @@ GrafanaNotificationPolicyRouteSpec defines the desired state of GrafanaNotificat
         </tr>
     </thead>
     <tbody><tr>
+        <td><b>receiver</b></td>
+        <td>string</td>
+        <td>
+          receiver<br/>
+        </td>
+        <td>true</td>
+      </tr><tr>
         <td><b>continue</b></td>
         <td>boolean</td>
         <td>
@@ -4403,13 +4410,6 @@ GrafanaNotificationPolicyRouteSpec defines the desired state of GrafanaNotificat
           provenance<br/>
         </td>
         <td>false</td>
-      </tr><tr>
-        <td><b>receiver</b></td>
-        <td>string</td>
-        <td>
-          receiver<br/>
-        </td>
-        <td>false</td>
       </tr><tr>
         <td><b>repeat_interval</b></td>
         <td>string</td>