From 413c24e62073bf5b7164e68b6ce0908b57fcf3c2 Mon Sep 17 00:00:00 2001 From: Saloni Choudhary <146118978+salonichf5@users.noreply.github.com> Date: Tue, 16 Dec 2025 14:22:25 -0700 Subject: [PATCH] Preserve external controller annotations for deployment and daemonSet (#4468) Problem: Users want that deployments and daemonSet preserve external annotations like how we do for services Solution: Adds a solution to track internal annotations and preserver external annotations. --- internal/controller/provisioner/setter.go | 104 ++++----- .../controller/provisioner/setter_test.go | 198 ++++++++++++++++++ 2 files changed, 253 insertions(+), 49 deletions(-) diff --git a/internal/controller/provisioner/setter.go b/internal/controller/provisioner/setter.go index d529e01af9..1a2cfb9c29 100644 --- a/internal/controller/provisioner/setter.go +++ b/internal/controller/provisioner/setter.go @@ -47,7 +47,7 @@ func deploymentSpecSetter( ) controllerutil.MutateFn { return func() error { deployment.Labels = objectMeta.Labels - deployment.Annotations = objectMeta.Annotations + deployment.Annotations = mergeAnnotations(deployment.Annotations, objectMeta.Annotations) deployment.Spec = spec return nil } @@ -73,7 +73,7 @@ func daemonSetSpecSetter( ) controllerutil.MutateFn { return func() error { daemonSet.Labels = objectMeta.Labels - daemonSet.Annotations = objectMeta.Annotations + daemonSet.Annotations = mergeAnnotations(daemonSet.Annotations, objectMeta.Annotations) daemonSet.Spec = spec return nil } @@ -85,54 +85,8 @@ func serviceSpecSetter( objectMeta metav1.ObjectMeta, ) controllerutil.MutateFn { return func() error { - const managedKeysAnnotation = "gateway.nginx.org/internal-managed-annotation-keys" - - // Track which annotation keys NGF currently manages - currentManagedKeys := make(map[string]bool) - for k := range objectMeta.Annotations { - currentManagedKeys[k] = true - } - - // Get previously managed keys from existing service - var previousManagedKeys map[string]bool - if prevKeysStr, ok := service.Annotations[managedKeysAnnotation]; ok { - previousManagedKeys = make(map[string]bool) - for _, k := range strings.Split(prevKeysStr, ",") { - if k != "" { - previousManagedKeys[k] = true - } - } - } - - // Start with existing annotations (preserves external controller annotations) - mergedAnnotations := make(map[string]string) - for k, v := range service.Annotations { - // Skip the internal tracking annotation - if k == managedKeysAnnotation { - continue - } - // Remove annotations that NGF previously managed but no longer wants - if previousManagedKeys != nil && previousManagedKeys[k] && !currentManagedKeys[k] { - continue // Remove this annotation - } - mergedAnnotations[k] = v - } - - // Apply NGF-managed annotations (take precedence) - maps.Copy(mergedAnnotations, objectMeta.Annotations) - - // Store current managed keys for next reconciliation - if len(currentManagedKeys) > 0 { - var managedKeysList []string - for k := range currentManagedKeys { - managedKeysList = append(managedKeysList, k) - } - slices.Sort(managedKeysList) // Sort for deterministic output - mergedAnnotations[managedKeysAnnotation] = strings.Join(managedKeysList, ",") - } - service.Labels = objectMeta.Labels - service.Annotations = mergedAnnotations + service.Annotations = mergeAnnotations(service.Annotations, objectMeta.Annotations) service.Spec = spec return nil } @@ -210,3 +164,55 @@ func roleBindingSpecSetter( return nil } } + +func mergeAnnotations(existing, desired map[string]string) map[string]string { + const trackingKey = "gateway.nginx.org/internal-managed-annotation-keys" + desiredKeys := make(map[string]struct{}, len(desired)) + for key := range desired { + desiredKeys[key] = struct{}{} + } + + previousKeys := make(map[string]struct{}, len(existing)) + if existing != nil { + if prev, ok := existing[trackingKey]; ok { + for splitKey := range strings.SplitSeq(prev, ",") { + if splitKey != "" { + previousKeys[splitKey] = struct{}{} + } + } + } + } + + annotations := make(map[string]string) + + // Start with existing annotations (preserves external controller annotations) + for key, value := range existing { + if key == trackingKey { + continue + } + + // if this key was previously managed and is no longer desired, drop it + if _, wasManaged := previousKeys[key]; wasManaged { + if _, stillDesired := desiredKeys[key]; !stillDesired { + continue + } + } + + annotations[key] = value + } + + // Apply desired annotations (NGF-managed wins) + maps.Copy(annotations, desired) + + // Store current managed keys + if len(desiredKeys) > 0 { + keys := make([]string, 0, len(desiredKeys)) + for key := range desiredKeys { + keys = append(keys, key) + } + slices.Sort(keys) + annotations[trackingKey] = strings.Join(keys, ",") + } + + return annotations +} diff --git a/internal/controller/provisioner/setter_test.go b/internal/controller/provisioner/setter_test.go index 86c1d04b5f..95c13d0e8c 100644 --- a/internal/controller/provisioner/setter_test.go +++ b/internal/controller/provisioner/setter_test.go @@ -4,6 +4,7 @@ import ( "testing" . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -151,3 +152,200 @@ func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) { }) } } + +func int32Ptr(i int32) *int32 { return &i } + +func TestDeploymentAndDaemonSetSpecSetter(t *testing.T) { + t.Parallel() + + type testCase struct { + existingAnnotations map[string]string + desiredAnnotations map[string]string + expectedAnnotations map[string]string + name string + } + + tests := []testCase{ + { + name: "preserves external annotations while adding NGF annotations", + existingAnnotations: map[string]string{ + "deployment.kubernetes.io/revision": "1", + "field.cattle.io/publicEndpoints": "192.61.0.19", + "field.cattle.io/ports": "80/tcp", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "from-ngf", + }, + expectedAnnotations: map[string]string{ + "deployment.kubernetes.io/revision": "1", + "field.cattle.io/publicEndpoints": "192.61.0.19", + "field.cattle.io/ports": "80/tcp", + "custom.annotation": "from-ngf", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + }, + { + name: "preserves existing NGF-managed annotations when still desired", + existingAnnotations: map[string]string{ + "custom.annotation": "keep-me", + "argocd.argoproj.io/sync-options": "Prune=false", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "keep-me", + }, + expectedAnnotations: map[string]string{ + "custom.annotation": "keep-me", + "argocd.argoproj.io/sync-options": "Prune=false", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + }, + { + name: "removes NGF-managed annotations when no longer desired", + existingAnnotations: map[string]string{ + "custom.annotation": "should-be-removed", + "deployment.kubernetes.io/revision": "2", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + desiredAnnotations: map[string]string{}, + expectedAnnotations: map[string]string{ + "deployment.kubernetes.io/revision": "2", + }, + }, + { + name: "NGF annotations take precedence on conflicts", + existingAnnotations: map[string]string{ + "custom.annotation": "old-value", + "daemonSet.kubernetes.io/revision": "7", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "new-value", + }, + expectedAnnotations: map[string]string{ + "custom.annotation": "new-value", + "daemonSet.kubernetes.io/revision": "7", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + }, + { + name: "creates new deployment with annotations", + existingAnnotations: nil, + desiredAnnotations: map[string]string{ + "custom.annotation": "value", + }, + expectedAnnotations: map[string]string{ + "custom.annotation": "value", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + }, + { + name: "updates tracking annotation when managed keys change", + existingAnnotations: map[string]string{ + "annotation-to-keep": "keep-value", + "annotation-to-remove": "remove-value", + "argocd.argoproj.io/sync-options": "Validate=true", + "gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep,annotation-to-remove", + }, + desiredAnnotations: map[string]string{ + "annotation-to-keep": "updated-keep-value", + }, + expectedAnnotations: map[string]string{ + "annotation-to-keep": "updated-keep-value", + "argocd.argoproj.io/sync-options": "Validate=true", + "gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep", + }, + }, + } + + labels := map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + } + + makeDesiredMeta := func(ann map[string]string) metav1.ObjectMeta { + return metav1.ObjectMeta{ + Labels: labels, + Annotations: ann, + } + } + + podTemplate := corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labels}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "nginx-gateway", + Image: "nginx:1.25", + }}, + }, + } + + runners := []struct { + run func(t *testing.T, tc testCase) + name string + }{ + { + name: "Deployment", + run: func(t *testing.T, tc testCase) { + t.Helper() + g := NewWithT(t) + + existing := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-gateway", + Namespace: "nginx-gateway", + Annotations: tc.existingAnnotations, + }, + } + + spec := appsv1.DeploymentSpec{ + Replicas: int32Ptr(1), + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: podTemplate, + } + + err := deploymentSpecSetter(existing, spec, makeDesiredMeta(tc.desiredAnnotations))() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existing.Annotations).To(Equal(tc.expectedAnnotations)) + g.Expect(existing.Labels).To(Equal(labels)) + g.Expect(existing.Spec).To(Equal(spec)) + }, + }, + { + name: "DaemonSet", + run: func(t *testing.T, tc testCase) { + t.Helper() + g := NewWithT(t) + + existing := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-gateway", + Namespace: "nginx-gateway", + Annotations: tc.existingAnnotations, + }, + } + + spec := appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: podTemplate, + } + + err := daemonSetSpecSetter(existing, spec, makeDesiredMeta(tc.desiredAnnotations))() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existing.Annotations).To(Equal(tc.expectedAnnotations)) + g.Expect(existing.Labels).To(Equal(labels)) + g.Expect(existing.Spec).To(Equal(spec)) + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + for _, r := range runners { + t.Run(r.name, func(t *testing.T) { + r.run(t, tc) + }) + } + }) + } +}