From fa5156cd2e050a74afbbb95923ede6d54aca7da9 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 15 Dec 2025 10:32:43 -0700 Subject: [PATCH 1/4] preserve external controller annotations for deployment and daemonSet --- internal/controller/provisioner/setter.go | 107 +++--- .../controller/provisioner/setter_test.go | 346 ++++++++++++++++++ 2 files changed, 404 insertions(+), 49 deletions(-) diff --git a/internal/controller/provisioner/setter.go b/internal/controller/provisioner/setter.go index d529e01af9..4bd4ab64ad 100644 --- a/internal/controller/provisioner/setter.go +++ b/internal/controller/provisioner/setter.go @@ -40,6 +40,10 @@ func objectSpecSetter(object client.Object) controllerutil.MutateFn { return nil } +const ( + managedKeysAnnotation = "gateway.nginx.org/internal-managed-annotation-keys" +) + func deploymentSpecSetter( deployment *appsv1.Deployment, spec appsv1.DeploymentSpec, @@ -47,7 +51,7 @@ func deploymentSpecSetter( ) controllerutil.MutateFn { return func() error { deployment.Labels = objectMeta.Labels - deployment.Annotations = objectMeta.Annotations + deployment.Annotations = mergeAnnotations(deployment.Annotations, objectMeta.Annotations, managedKeysAnnotation) deployment.Spec = spec return nil } @@ -73,7 +77,7 @@ func daemonSetSpecSetter( ) controllerutil.MutateFn { return func() error { daemonSet.Labels = objectMeta.Labels - daemonSet.Annotations = objectMeta.Annotations + daemonSet.Annotations = mergeAnnotations(daemonSet.Annotations, objectMeta.Annotations, managedKeysAnnotation) daemonSet.Spec = spec return nil } @@ -85,54 +89,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, managedKeysAnnotation) service.Spec = spec return nil } @@ -210,3 +168,54 @@ func roleBindingSpecSetter( return nil } } + +func mergeAnnotations(existing, desired map[string]string, trackingKey string) map[string]string { + 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..a3bf579917 100644 --- a/internal/controller/provisioner/setter_test.go +++ b/internal/controller/provisioner/setter_test.go @@ -4,10 +4,13 @@ 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" ) +func int32Ptr(i int32) *int32 { return &i } + func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) { t.Parallel() @@ -151,3 +154,346 @@ func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) { }) } } + +func TestDeploymentSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { + t.Parallel() + + tests := []struct { + existingAnnotations map[string]string + desiredAnnotations map[string]string + expectedAnnotations map[string]string + name string + }{ + { + 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: "NGF annotations take precedence on conflicts", + existingAnnotations: map[string]string{ + "custom.annotation": "old-value", + "deployment.kubernetes.io/revision": "7", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "new-value", + }, + expectedAnnotations: map[string]string{ + "custom.annotation": "new-value", + "deployment.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: "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: "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", + }, + }, + { + name: "preserves external annotations while adding NGF annotations", + existingAnnotations: map[string]string{ + "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{ + "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", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + existing := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-gateway", + Namespace: "nginx-gateway", + Annotations: tt.existingAnnotations, + }, + } + + desiredMeta := metav1.ObjectMeta{ + Labels: map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + }, + Annotations: tt.desiredAnnotations, + } + + inputSpec := appsv1.DeploymentSpec{ + Replicas: int32Ptr(1), + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "nginx-gateway", + Image: "nginx:1.25", + }}, + }, + }, + } + + setter := deploymentSpecSetter(existing, inputSpec, desiredMeta) + err := setter() + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existing.Annotations).To(Equal(tt.expectedAnnotations)) + g.Expect(existing.Labels).To(Equal(desiredMeta.Labels)) + g.Expect(existing.Spec).To(Equal(inputSpec)) + }) + } +} + +func TestDaemonSetSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { + t.Parallel() + + tests := []struct { + existingAnnotations map[string]string + desiredAnnotations map[string]string + expectedAnnotations map[string]string + name string + }{ + { + 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: "NGF annotations take precedence on conflicts", + existingAnnotations: map[string]string{ + "custom.annotation": "old-value", + "deployment.kubernetes.io/revision": "7", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "new-value", + }, + expectedAnnotations: map[string]string{ + "custom.annotation": "new-value", + "deployment.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: "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: "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", + }, + }, + { + name: "preserves external annotations while adding NGF annotations", + existingAnnotations: map[string]string{ + "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{ + "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", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + existing := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-gateway", + Namespace: "nginx-gateway", + Annotations: tt.existingAnnotations, + }, + } + + desiredMeta := metav1.ObjectMeta{ + Labels: map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + }, + Annotations: tt.desiredAnnotations, + } + + inputSpec := appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "nginx-gateway", + Image: "nginx:1.25", + }}, + }, + }, + } + + setter := daemonSetSpecSetter(existing, inputSpec, desiredMeta) + err := setter() + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existing.Annotations).To(Equal(tt.expectedAnnotations)) + g.Expect(existing.Labels).To(Equal(desiredMeta.Labels)) + g.Expect(existing.Spec).To(Equal(inputSpec)) + }) + } +} From e19e13935e2aea8146ab74838f39049052e7edd4 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 15 Dec 2025 12:27:27 -0700 Subject: [PATCH 2/4] update based on reviews --- internal/controller/provisioner/setter.go | 13 +- .../controller/provisioner/setter_test.go | 337 ++++++------------ 2 files changed, 117 insertions(+), 233 deletions(-) diff --git a/internal/controller/provisioner/setter.go b/internal/controller/provisioner/setter.go index 4bd4ab64ad..d23fdd60cf 100644 --- a/internal/controller/provisioner/setter.go +++ b/internal/controller/provisioner/setter.go @@ -40,10 +40,6 @@ func objectSpecSetter(object client.Object) controllerutil.MutateFn { return nil } -const ( - managedKeysAnnotation = "gateway.nginx.org/internal-managed-annotation-keys" -) - func deploymentSpecSetter( deployment *appsv1.Deployment, spec appsv1.DeploymentSpec, @@ -51,7 +47,7 @@ func deploymentSpecSetter( ) controllerutil.MutateFn { return func() error { deployment.Labels = objectMeta.Labels - deployment.Annotations = mergeAnnotations(deployment.Annotations, objectMeta.Annotations, managedKeysAnnotation) + deployment.Annotations = mergeAnnotations(deployment.Annotations, objectMeta.Annotations) deployment.Spec = spec return nil } @@ -77,7 +73,7 @@ func daemonSetSpecSetter( ) controllerutil.MutateFn { return func() error { daemonSet.Labels = objectMeta.Labels - daemonSet.Annotations = mergeAnnotations(daemonSet.Annotations, objectMeta.Annotations, managedKeysAnnotation) + daemonSet.Annotations = mergeAnnotations(daemonSet.Annotations, objectMeta.Annotations) daemonSet.Spec = spec return nil } @@ -90,7 +86,7 @@ func serviceSpecSetter( ) controllerutil.MutateFn { return func() error { service.Labels = objectMeta.Labels - service.Annotations = mergeAnnotations(service.Annotations, objectMeta.Annotations, managedKeysAnnotation) + service.Annotations = mergeAnnotations(service.Annotations, objectMeta.Annotations) service.Spec = spec return nil } @@ -169,7 +165,8 @@ func roleBindingSpecSetter( } } -func mergeAnnotations(existing, desired map[string]string, trackingKey string) map[string]string { +func mergeAnnotations(existing, desired map[string]string) map[string]string { + trackingKey := "gateway.nginx.org/internal-managed-annotation-keys" desiredKeys := make(map[string]struct{}, len(desired)) for key := range desired { desiredKeys[key] = struct{}{} diff --git a/internal/controller/provisioner/setter_test.go b/internal/controller/provisioner/setter_test.go index a3bf579917..60326c940d 100644 --- a/internal/controller/provisioner/setter_test.go +++ b/internal/controller/provisioner/setter_test.go @@ -9,8 +9,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func int32Ptr(i int32) *int32 { return &i } - func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) { t.Parallel() @@ -155,123 +153,123 @@ func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) { } } -func TestDeploymentSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { - t.Parallel() - - tests := []struct { - existingAnnotations map[string]string - desiredAnnotations map[string]string - expectedAnnotations map[string]string - name string - }{ - { - 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", - }, +var testCasesDeploymentAndDaemonSet = []struct { + existingAnnotations map[string]string + desiredAnnotations map[string]string + expectedAnnotations map[string]string + name string +}{ + { + 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", }, - { - 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", - }, + desiredAnnotations: map[string]string{ + "custom.annotation": "from-ngf", }, - { - name: "NGF annotations take precedence on conflicts", - existingAnnotations: map[string]string{ - "custom.annotation": "old-value", - "deployment.kubernetes.io/revision": "7", - }, - desiredAnnotations: map[string]string{ - "custom.annotation": "new-value", - }, - expectedAnnotations: map[string]string{ - "custom.annotation": "new-value", - "deployment.kubernetes.io/revision": "7", - "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", - }, + 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: "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: "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", }, - { - 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", - }, + desiredAnnotations: map[string]string{ + "custom.annotation": "keep-me", }, - { - 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", - }, + 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: "preserves external annotations while adding NGF annotations", - existingAnnotations: map[string]string{ - "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{ - "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: "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: "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: "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", + }, + }, + { + name: "preserves external annotations while adding NGF annotations", + existingAnnotations: map[string]string{ + "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{ + "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", + }, + }, +} - for _, tt := range tests { +func TestDeploymentSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { + t.Parallel() + + for _, tt := range testCasesDeploymentAndDaemonSet { t.Run(tt.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) @@ -330,120 +328,7 @@ func TestDeploymentSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { func TestDaemonSetSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { t.Parallel() - tests := []struct { - existingAnnotations map[string]string - desiredAnnotations map[string]string - expectedAnnotations map[string]string - name string - }{ - { - 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: "NGF annotations take precedence on conflicts", - existingAnnotations: map[string]string{ - "custom.annotation": "old-value", - "deployment.kubernetes.io/revision": "7", - }, - desiredAnnotations: map[string]string{ - "custom.annotation": "new-value", - }, - expectedAnnotations: map[string]string{ - "custom.annotation": "new-value", - "deployment.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: "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: "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", - }, - }, - { - name: "preserves external annotations while adding NGF annotations", - existingAnnotations: map[string]string{ - "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{ - "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", - }, - }, - } - - for _, tt := range tests { + for _, tt := range testCasesDeploymentAndDaemonSet { t.Run(tt.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) @@ -497,3 +382,5 @@ func TestDaemonSetSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { }) } } + +func int32Ptr(i int32) *int32 { return &i } From bd95e43c3e59f1bbd005a7c2b0d2c9b15c6ff43c Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 15 Dec 2025 12:37:57 -0700 Subject: [PATCH 3/4] update key to constant --- internal/controller/provisioner/setter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/provisioner/setter.go b/internal/controller/provisioner/setter.go index d23fdd60cf..1a2cfb9c29 100644 --- a/internal/controller/provisioner/setter.go +++ b/internal/controller/provisioner/setter.go @@ -166,7 +166,7 @@ func roleBindingSpecSetter( } func mergeAnnotations(existing, desired map[string]string) map[string]string { - trackingKey := "gateway.nginx.org/internal-managed-annotation-keys" + const trackingKey = "gateway.nginx.org/internal-managed-annotation-keys" desiredKeys := make(map[string]struct{}, len(desired)) for key := range desired { desiredKeys[key] = struct{}{} From a26f7d60537e8b256c5ef09b3822cb097a52a5d4 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 15 Dec 2025 16:20:59 -0700 Subject: [PATCH 4/4] improve unit tests --- .../controller/provisioner/setter_test.go | 381 ++++++++---------- 1 file changed, 173 insertions(+), 208 deletions(-) diff --git a/internal/controller/provisioner/setter_test.go b/internal/controller/provisioner/setter_test.go index 60326c940d..95c13d0e8c 100644 --- a/internal/controller/provisioner/setter_test.go +++ b/internal/controller/provisioner/setter_test.go @@ -153,234 +153,199 @@ func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) { } } -var testCasesDeploymentAndDaemonSet = []struct { - existingAnnotations map[string]string - desiredAnnotations map[string]string - expectedAnnotations map[string]string - name string -}{ - { - 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: "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: "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: "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", +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", + }, }, - desiredAnnotations: map[string]string{ - "annotation-to-keep": "updated-keep-value", + { + 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", + }, }, - 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", + { + 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: "preserves external annotations while adding NGF annotations", - existingAnnotations: map[string]string{ - "field.cattle.io/publicEndpoints": "192.61.0.19", - "field.cattle.io/ports": "80/tcp", + { + 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", + }, }, - desiredAnnotations: map[string]string{ - "custom.annotation": "from-ngf", + { + 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", + }, }, - expectedAnnotations: map[string]string{ - "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: "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", + }, }, - }, -} + } -func TestDeploymentSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { - t.Parallel() + labels := map[string]string{ + "app.kubernetes.io/name": "nginx-gateway-fabric", + "app.kubernetes.io/instance": "nginx-gateway", + } - for _, tt := range testCasesDeploymentAndDaemonSet { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) + makeDesiredMeta := func(ann map[string]string) metav1.ObjectMeta { + return metav1.ObjectMeta{ + Labels: labels, + Annotations: ann, + } + } - existing := &appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-gateway", - Namespace: "nginx-gateway", - Annotations: tt.existingAnnotations, - }, - } + podTemplate := corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labels}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "nginx-gateway", + Image: "nginx:1.25", + }}, + }, + } - desiredMeta := metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/name": "nginx-gateway-fabric", - "app.kubernetes.io/instance": "nginx-gateway", - }, - Annotations: tt.desiredAnnotations, - } + 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) - inputSpec := appsv1.DeploymentSpec{ - Replicas: int32Ptr(1), - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "app.kubernetes.io/name": "nginx-gateway-fabric", - "app.kubernetes.io/instance": "nginx-gateway", - }, - }, - Template: corev1.PodTemplateSpec{ + existing := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/name": "nginx-gateway-fabric", - "app.kubernetes.io/instance": "nginx-gateway", - }, - }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Name: "nginx-gateway", - Image: "nginx:1.25", - }}, + Name: "nginx-gateway", + Namespace: "nginx-gateway", + Annotations: tc.existingAnnotations, }, - }, - } + } - setter := deploymentSpecSetter(existing, inputSpec, desiredMeta) - err := setter() + spec := appsv1.DeploymentSpec{ + Replicas: int32Ptr(1), + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: podTemplate, + } - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(existing.Annotations).To(Equal(tt.expectedAnnotations)) - g.Expect(existing.Labels).To(Equal(desiredMeta.Labels)) - g.Expect(existing.Spec).To(Equal(inputSpec)) - }) - } -} - -func TestDaemonSetSpecSetter_PreservesExternalObjectAnnotations(t *testing.T) { - t.Parallel() - - for _, tt := range testCasesDeploymentAndDaemonSet { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - existing := &appsv1.DaemonSet{ - ObjectMeta: metav1.ObjectMeta{ - Name: "nginx-gateway", - Namespace: "nginx-gateway", - Annotations: tt.existingAnnotations, - }, - } - - desiredMeta := metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/name": "nginx-gateway-fabric", - "app.kubernetes.io/instance": "nginx-gateway", - }, - Annotations: tt.desiredAnnotations, - } + 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) - inputSpec := appsv1.DaemonSetSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "app.kubernetes.io/name": "nginx-gateway-fabric", - "app.kubernetes.io/instance": "nginx-gateway", - }, - }, - Template: corev1.PodTemplateSpec{ + existing := &appsv1.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - "app.kubernetes.io/name": "nginx-gateway-fabric", - "app.kubernetes.io/instance": "nginx-gateway", - }, + Name: "nginx-gateway", + Namespace: "nginx-gateway", + Annotations: tc.existingAnnotations, }, - Spec: corev1.PodSpec{ - Containers: []corev1.Container{{ - Name: "nginx-gateway", - Image: "nginx:1.25", - }}, - }, - }, - } + } - setter := daemonSetSpecSetter(existing, inputSpec, desiredMeta) - err := setter() + spec := appsv1.DaemonSetSpec{ + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: podTemplate, + } - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(existing.Annotations).To(Equal(tt.expectedAnnotations)) - g.Expect(existing.Labels).To(Equal(desiredMeta.Labels)) - g.Expect(existing.Spec).To(Equal(inputSpec)) + 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) + }) + } }) } } - -func int32Ptr(i int32) *int32 { return &i }