fix(GrafanaNotificationPolicy): use patch instead of update when applying the notification policy annotation to avoid Grafana CR drift (#1898)

* fix: Use patch not update to add/remove applied NotificationPolicy annotation

* chore: Make comments consistent and clearer

Author: Baarsgaard

controllers/controller_shared.go

@@ -344,3 +344,39 @@ func patchFinalizers(ctx context.Context, cl client.Client, cr client.Object) er
 	}
 	return cl.Patch(ctx, cr, client.RawPatch(types.MergePatchType, patch))
 }
+
+func addAnnotation(ctx context.Context, cl client.Client, cr client.Object, key string, value string) error {
+	crAnnotations := cr.GetAnnotations()
+
+	if crAnnotations[key] == value {
+		return nil
+	}
+
+	// Add key to map and create patch
+	crAnnotations[key] = value
+	patch, err := json.Marshal(map[string]interface{}{"metadata": map[string]interface{}{"annotations": crAnnotations}})
+	if err != nil {
+		return err
+	}
+
+	return cl.Patch(ctx, cr, client.RawPatch(types.MergePatchType, patch))
+}
+
+func removeAnnotation(ctx context.Context, cl client.Client, cr client.Object, key string) error {
+	crAnnotations := cr.GetAnnotations()
+	if crAnnotations[key] == "" {
+		return nil
+	}
+
+	// Escape slash '/' according to RFC6901
+	// We could also escape tilde '~', but that is not a valid character in annotation keys.
+	key = strings.ReplaceAll(key, "/", "~1")
+	patch, err := json.Marshal([]interface{}{map[string]interface{}{"op": "remove", "path": "/metadata/annotations/" + key}})
+	if err != nil {
+		return err
+	}
+
+	// MergePatchType only removes map keys when the value is null. JSONPatchType allows removing anything under a path.
+	// Differs from removeFinalizer where we overwrite an array.
+	return cl.Patch(ctx, cr, client.RawPatch(types.JSONPatchType, patch))
+}

controllers/notificationpolicy_controller.go

@@ -285,9 +285,10 @@ func (r *GrafanaNotificationPolicyReconciler) reconcileWithInstance(ctx context.
 	if instance.Annotations == nil {
 		instance.Annotations = make(map[string]string)
 	}
-	instance.Annotations[annotationAppliedNotificationPolicy] = notificationPolicy.NamespacedResource()
-	if err := r.Client.Update(ctx, instance); err != nil {
-		return fmt.Errorf("saving applied policy to instance CR: %w", err)
+
+	err = addAnnotation(ctx, r.Client, instance, annotationAppliedNotificationPolicy, notificationPolicy.NamespacedResource())
+	if err != nil {
+		return fmt.Errorf("saving applied notification policy to Grafana CR: %w", err)
 	}
 	return nil
 }
@@ -313,9 +314,9 @@ func (r *GrafanaNotificationPolicyReconciler) finalize(ctx context.Context, noti
 			return fmt.Errorf("resetting policy tree")
 		}
 
-		delete(grafana.Annotations, annotationAppliedNotificationPolicy)
-		if err := r.Client.Update(ctx, &grafana); err != nil {
-			return fmt.Errorf("removing applied notification policy from Grafana cr: %w", err)
+		err = removeAnnotation(ctx, r.Client, &grafana, annotationAppliedNotificationPolicy)
+		if err != nil {
+			return fmt.Errorf("removing applied notification policy from Grafana CR: %w", err)
 		}
 	}