Preserve external controller annotations for deployment and daemonSet (#4468)

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
+}

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)
+				})
+			}
+		})
+	}
+}