From 33863dc1e07adb99bb7085307ed60c06228da719 Mon Sep 17 00:00:00 2001 From: ste Date: Thu, 30 Jan 2025 22:51:35 +0100 Subject: [PATCH 1/4] fix: reciever is a required field chore: NotificationPolicyRoute missing category --- .../grafananotificationpolicy_types.go | 3 +- .../grafananotificationpolicy_types_test.go | 1 + .../grafananotificationpolicyroute_types.go | 1 + ...eatly.org_grafananotificationpolicies.yaml | 3 ++ ...y.org_grafananotificationpolicyroutes.yaml | 5 ++++ ...eatly.org_grafananotificationpolicies.yaml | 3 ++ ...y.org_grafananotificationpolicyroutes.yaml | 5 ++++ deploy/kustomize/base/crds.yaml | 8 ++++++ docs/docs/api.md | 28 +++++++++---------- 9 files changed, 42 insertions(+), 15 deletions(-) diff --git a/api/v1beta1/grafananotificationpolicy_types.go b/api/v1beta1/grafananotificationpolicy_types.go index fcea58679..074f6e607 100644 --- 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 index 8a3d22fb0..18bb48bb6 100644 --- 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 index bb6dd5401..03425a6c3 100644 --- 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 index 7724549fb..1d003d59d 100644 --- 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 index f3b996083..ceecfbc62 100644 --- 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/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicies.yaml b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicies.yaml index 7724549fb..1d003d59d 100644 --- 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/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yaml b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yaml index f3b996083..ceecfbc62 100644 --- a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafananotificationpolicyroutes.yaml +++ b/deploy/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 index a7a10d208..06e42677f 100644 --- 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 index 4326f11e8..9fed40961 100644 --- a/docs/docs/api.md +++ b/docs/docs/api.md @@ -3925,6 +3925,13 @@ Routes for alerts to match against + receiver + string + + receiver
+ + true + continue boolean @@ -3987,13 +3994,6 @@ Routes for alerts to match against provenance
false - - receiver - string - - receiver
- - false repeat_interval string @@ -4341,6 +4341,13 @@ GrafanaNotificationPolicyRouteSpec defines the desired state of GrafanaNotificat + receiver + string + + receiver
+ + true + continue boolean @@ -4403,13 +4410,6 @@ GrafanaNotificationPolicyRouteSpec defines the desired state of GrafanaNotificat provenance
false - - receiver - string - - receiver
- - false repeat_interval string From e656569e269e1ff6e86314d1830c71b47ae6b79e Mon Sep 17 00:00:00 2001 From: ste Date: Sun, 2 Feb 2025 12:54:59 +0100 Subject: [PATCH 2/4] fix: loopDetected condition never applied --- controllers/notificationpolicy_controller.go | 55 +++++++++----------- 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/controllers/notificationpolicy_controller.go b/controllers/notificationpolicy_controller.go index eb96ee065..24f0d4578 100644 --- a/controllers/notificationpolicy_controller.go +++ b/controllers/notificationpolicy_controller.go @@ -116,6 +116,31 @@ func (r *GrafanaNotificationPolicyReconciler) Reconcile(ctx context.Context, req } removeInvalidSpec(¬ificationPolicy.Status.Conditions) + // Assemble routes and check for loops + var mergedRoutes []*v1beta1.GrafanaNotificationPolicyRoute + if notificationPolicy.Spec.Route.RouteSelector != nil || notificationPolicy.Spec.Route.HasRouteSelector() { + mergedRoutes, err = assembleNotificationPolicyRoutes(ctx, r.Client, notificationPolicy) + + if errors.Is(err, ErrLoopDetected) { + meta.SetStatusCondition(¬ificationPolicy.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(¬ificationPolicy.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(¬ificationPolicy.Status.Conditions, conditionNotificationPolicyLoopDetected) + instances, err := GetScopedMatchingInstances(ctx, r.Client, notificationPolicy) if err != nil { setNoMatchingInstancesCondition(¬ificationPolicy.Status.Conditions, notificationPolicy.Generation, err) @@ -132,16 +157,6 @@ func (r *GrafanaNotificationPolicyReconciler) Reconcile(ctx context.Context, req removeNoMatchingInstance(¬ificationPolicy.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(¬ificationPolicy.Status.Conditions, conditionNotificationPolicyLoopDetected) - applyErrors := make(map[string]string) for _, grafana := range instances { // can be removed in go 1.22+ @@ -450,23 +465,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(¬ificationPolicy.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) -} From 502168b3936fc862d0ac2673a71dff5c5477fb0d Mon Sep 17 00:00:00 2001 From: ste Date: Mon, 3 Feb 2025 19:12:11 +0100 Subject: [PATCH 3/4] refactor: Simplify if condition --- controllers/notificationpolicy_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/notificationpolicy_controller.go b/controllers/notificationpolicy_controller.go index 24f0d4578..2395ad350 100644 --- a/controllers/notificationpolicy_controller.go +++ b/controllers/notificationpolicy_controller.go @@ -118,7 +118,7 @@ func (r *GrafanaNotificationPolicyReconciler) Reconcile(ctx context.Context, req // Assemble routes and check for loops var mergedRoutes []*v1beta1.GrafanaNotificationPolicyRoute - if notificationPolicy.Spec.Route.RouteSelector != nil || notificationPolicy.Spec.Route.HasRouteSelector() { + if notificationPolicy.Spec.Route.HasRouteSelector() { mergedRoutes, err = assembleNotificationPolicyRoutes(ctx, r.Client, notificationPolicy) if errors.Is(err, ErrLoopDetected) { From 74fb3afaa60691db6a2f3293b4dc2d348b282c53 Mon Sep 17 00:00:00 2001 From: ste Date: Tue, 4 Feb 2025 09:45:35 +0100 Subject: [PATCH 4/4] fix: sort notification policy status.discoveredRoutes --- controllers/notificationpolicy_controller.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/controllers/notificationpolicy_controller.go b/controllers/notificationpolicy_controller.go index 2395ad350..ba8adcadf 100644 --- 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" @@ -457,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 }