Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Mar 23, 2025
+43 −6
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ade6d3f
fix: Use patch not update to add/remove applied NotificationPolicy an…
Baarsgaard Mar 13, 2025
4781ef2
chore: Make comments consistent and clearer
Baarsgaard Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions controllers/controller_shared.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
13 changes: 7 additions & 6 deletions controllers/notificationpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
}
}

Expand Down