From 0e6da214df7f47908133ceae16309182609841e8 Mon Sep 17 00:00:00 2001 From: Ciara Stacke Date: Wed, 5 Nov 2025 10:42:08 +0000 Subject: [PATCH] Preserve external controller annotations on Services --- internal/controller/provisioner/setter.go | 52 +++++- .../controller/provisioner/setter_test.go | 153 ++++++++++++++++++ 2 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 internal/controller/provisioner/setter_test.go diff --git a/internal/controller/provisioner/setter.go b/internal/controller/provisioner/setter.go index 0ffbf53d49..b7204ade4a 100644 --- a/internal/controller/provisioner/setter.go +++ b/internal/controller/provisioner/setter.go @@ -2,6 +2,8 @@ package provisioner import ( "maps" + "slices" + "strings" appsv1 "k8s.io/api/apps/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" @@ -83,8 +85,56 @@ 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) + for k, v := range objectMeta.Annotations { + mergedAnnotations[k] = v + } + + // 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 = objectMeta.Annotations + service.Annotations = mergedAnnotations service.Spec = spec return nil } diff --git a/internal/controller/provisioner/setter_test.go b/internal/controller/provisioner/setter_test.go new file mode 100644 index 0000000000..86c1d04b5f --- /dev/null +++ b/internal/controller/provisioner/setter_test.go @@ -0,0 +1,153 @@ +package provisioner + +import ( + "testing" + + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) { + t.Parallel() + + tests := []struct { + existingAnnotations map[string]string + desiredAnnotations map[string]string + expectedAnnotations map[string]string + name string + }{ + { + name: "preserves MetalLB annotations while adding NGF annotations", + existingAnnotations: map[string]string{ + "metallb.universe.tf/ip-allocated-from-pool": "production-public-ips", + "metallb.universe.tf/loadBalancerIPs": "192.168.1.100", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "from-gateway-infrastructure", + }, + expectedAnnotations: map[string]string{ + "metallb.universe.tf/ip-allocated-from-pool": "production-public-ips", + "metallb.universe.tf/loadBalancerIPs": "192.168.1.100", + "custom.annotation": "from-gateway-infrastructure", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + }, + { + name: "NGF annotations take precedence on conflicts", + existingAnnotations: map[string]string{ + "custom.annotation": "old-value", + "metallb.universe.tf/address-pool": "staging", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "new-value", + }, + expectedAnnotations: map[string]string{ + "custom.annotation": "new-value", + "metallb.universe.tf/address-pool": "staging", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + }, + { + name: "creates new service 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", + "metallb.universe.tf/ip-allocated-from-pool": "production", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + desiredAnnotations: map[string]string{}, + expectedAnnotations: map[string]string{ + "metallb.universe.tf/ip-allocated-from-pool": "production", + }, + }, + { + name: "preserves cloud provider annotations", + existingAnnotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-type": "nlb", + "service.beta.kubernetes.io/aws-load-balancer-scheme": "internet-facing", + }, + desiredAnnotations: map[string]string{ + "custom.annotation": "from-nginxproxy-patch", + }, + expectedAnnotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-type": "nlb", + "service.beta.kubernetes.io/aws-load-balancer-scheme": "internet-facing", + "custom.annotation": "from-nginxproxy-patch", + "gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation", + }, + }, + { + name: "updates tracking annotation when managed keys change", + existingAnnotations: map[string]string{ + "annotation-to-keep": "value1", + "annotation-to-remove": "value2", + "metallb.universe.tf/address-pool": "production", + "gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep,annotation-to-remove", + }, + desiredAnnotations: map[string]string{ + "annotation-to-keep": "value1", + }, + expectedAnnotations: map[string]string{ + "annotation-to-keep": "value1", + "metallb.universe.tf/address-pool": "production", + "gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + // Create existing service with annotations + existingService := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "default", + Annotations: tt.existingAnnotations, + }, + } + + // Create desired object metadata with NGF-managed annotations + desiredMeta := metav1.ObjectMeta{ + Labels: map[string]string{ + "app": "nginx-gateway", + }, + Annotations: tt.desiredAnnotations, + } + + // Create desired spec + desiredSpec := corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + Protocol: corev1.ProtocolTCP, + }, + }, + } + + // Execute the setter + setter := serviceSpecSetter(existingService, desiredSpec, desiredMeta) + err := setter() + + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(existingService.Annotations).To(Equal(tt.expectedAnnotations)) + g.Expect(existingService.Labels).To(Equal(desiredMeta.Labels)) + g.Expect(existingService.Spec).To(Equal(desiredSpec)) + }) + } +}