Skip to content

Commit ab00eed

Browse files
ciarams87ciarams87
authored
Preserve external controller annotations on Services (#4182)
1 parent 3562cc0 commit ab00eed

File tree

2 files changed

+204
-1
lines changed

2 files changed

+204
-1
lines changed

‎internal/controller/provisioner/setter.go‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package provisioner
22

33
import (
44
"maps"
5+
"slices"
6+
"strings"
57

68
appsv1 "k8s.io/api/apps/v1"
79
autoscalingv2 "k8s.io/api/autoscaling/v2"
@@ -83,8 +85,56 @@ func serviceSpecSetter(
8385
objectMeta metav1.ObjectMeta,
8486
) controllerutil.MutateFn {
8587
return func() error {
88+
const managedKeysAnnotation = "gateway.nginx.org/internal-managed-annotation-keys"
89+
90+
// Track which annotation keys NGF currently manages
91+
currentManagedKeys := make(map[string]bool)
92+
for k := range objectMeta.Annotations {
93+
currentManagedKeys[k] = true
94+
}
95+
96+
// Get previously managed keys from existing service
97+
var previousManagedKeys map[string]bool
98+
if prevKeysStr, ok := service.Annotations[managedKeysAnnotation]; ok {
99+
previousManagedKeys = make(map[string]bool)
100+
for _, k := range strings.Split(prevKeysStr, ",") {
101+
if k != "" {
102+
previousManagedKeys[k] = true
103+
}
104+
}
105+
}
106+
107+
// Start with existing annotations (preserves external controller annotations)
108+
mergedAnnotations := make(map[string]string)
109+
for k, v := range service.Annotations {
110+
// Skip the internal tracking annotation
111+
if k == managedKeysAnnotation {
112+
continue
113+
}
114+
// Remove annotations that NGF previously managed but no longer wants
115+
if previousManagedKeys != nil && previousManagedKeys[k] && !currentManagedKeys[k] {
116+
continue // Remove this annotation
117+
}
118+
mergedAnnotations[k] = v
119+
}
120+
121+
// Apply NGF-managed annotations (take precedence)
122+
for k, v := range objectMeta.Annotations {
123+
mergedAnnotations[k] = v
124+
}
125+
126+
// Store current managed keys for next reconciliation
127+
if len(currentManagedKeys) > 0 {
128+
var managedKeysList []string
129+
for k := range currentManagedKeys {
130+
managedKeysList = append(managedKeysList, k)
131+
}
132+
slices.Sort(managedKeysList) // Sort for deterministic output
133+
mergedAnnotations[managedKeysAnnotation] = strings.Join(managedKeysList, ",")
134+
}
135+
86136
service.Labels = objectMeta.Labels
87-
service.Annotations = objectMeta.Annotations
137+
service.Annotations = mergedAnnotations
88138
service.Spec = spec
89139
return nil
90140
}

‎internal/controller/provisioner/setter_test.go‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
package provisioner
2+
3+
import (
4+
"testing"
5+
6+
. "github.com/onsi/gomega"
7+
corev1 "k8s.io/api/core/v1"
8+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
9+
)
10+
11+
func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) {
12+
t.Parallel()
13+
14+
tests := []struct {
15+
existingAnnotations map[string]string
16+
desiredAnnotations map[string]string
17+
expectedAnnotations map[string]string
18+
name string
19+
}{
20+
{
21+
name: "preserves MetalLB annotations while adding NGF annotations",
22+
existingAnnotations: map[string]string{
23+
"metallb.universe.tf/ip-allocated-from-pool": "production-public-ips",
24+
"metallb.universe.tf/loadBalancerIPs": "192.168.1.100",
25+
},
26+
desiredAnnotations: map[string]string{
27+
"custom.annotation": "from-gateway-infrastructure",
28+
},
29+
expectedAnnotations: map[string]string{
30+
"metallb.universe.tf/ip-allocated-from-pool": "production-public-ips",
31+
"metallb.universe.tf/loadBalancerIPs": "192.168.1.100",
32+
"custom.annotation": "from-gateway-infrastructure",
33+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
34+
},
35+
},
36+
{
37+
name: "NGF annotations take precedence on conflicts",
38+
existingAnnotations: map[string]string{
39+
"custom.annotation": "old-value",
40+
"metallb.universe.tf/address-pool": "staging",
41+
},
42+
desiredAnnotations: map[string]string{
43+
"custom.annotation": "new-value",
44+
},
45+
expectedAnnotations: map[string]string{
46+
"custom.annotation": "new-value",
47+
"metallb.universe.tf/address-pool": "staging",
48+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
49+
},
50+
},
51+
{
52+
name: "creates new service with annotations",
53+
existingAnnotations: nil,
54+
desiredAnnotations: map[string]string{
55+
"custom.annotation": "value",
56+
},
57+
expectedAnnotations: map[string]string{
58+
"custom.annotation": "value",
59+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
60+
},
61+
},
62+
{
63+
name: "removes NGF-managed annotations when no longer desired",
64+
existingAnnotations: map[string]string{
65+
"custom.annotation": "should-be-removed",
66+
"metallb.universe.tf/ip-allocated-from-pool": "production",
67+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
68+
},
69+
desiredAnnotations: map[string]string{},
70+
expectedAnnotations: map[string]string{
71+
"metallb.universe.tf/ip-allocated-from-pool": "production",
72+
},
73+
},
74+
{
75+
name: "preserves cloud provider annotations",
76+
existingAnnotations: map[string]string{
77+
"service.beta.kubernetes.io/aws-load-balancer-type": "nlb",
78+
"service.beta.kubernetes.io/aws-load-balancer-scheme": "internet-facing",
79+
},
80+
desiredAnnotations: map[string]string{
81+
"custom.annotation": "from-nginxproxy-patch",
82+
},
83+
expectedAnnotations: map[string]string{
84+
"service.beta.kubernetes.io/aws-load-balancer-type": "nlb",
85+
"service.beta.kubernetes.io/aws-load-balancer-scheme": "internet-facing",
86+
"custom.annotation": "from-nginxproxy-patch",
87+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
88+
},
89+
},
90+
{
91+
name: "updates tracking annotation when managed keys change",
92+
existingAnnotations: map[string]string{
93+
"annotation-to-keep": "value1",
94+
"annotation-to-remove": "value2",
95+
"metallb.universe.tf/address-pool": "production",
96+
"gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep,annotation-to-remove",
97+
},
98+
desiredAnnotations: map[string]string{
99+
"annotation-to-keep": "value1",
100+
},
101+
expectedAnnotations: map[string]string{
102+
"annotation-to-keep": "value1",
103+
"metallb.universe.tf/address-pool": "production",
104+
"gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep",
105+
},
106+
},
107+
}
108+
109+
for _, tt := range tests {
110+
t.Run(tt.name, func(t *testing.T) {
111+
t.Parallel()
112+
g := NewWithT(t)
113+
114+
// Create existing service with annotations
115+
existingService := &corev1.Service{
116+
ObjectMeta: metav1.ObjectMeta{
117+
Name: "test-service",
118+
Namespace: "default",
119+
Annotations: tt.existingAnnotations,
120+
},
121+
}
122+
123+
// Create desired object metadata with NGF-managed annotations
124+
desiredMeta := metav1.ObjectMeta{
125+
Labels: map[string]string{
126+
"app": "nginx-gateway",
127+
},
128+
Annotations: tt.desiredAnnotations,
129+
}
130+
131+
// Create desired spec
132+
desiredSpec := corev1.ServiceSpec{
133+
Type: corev1.ServiceTypeLoadBalancer,
134+
Ports: []corev1.ServicePort{
135+
{
136+
Name: "http",
137+
Port: 80,
138+
Protocol: corev1.ProtocolTCP,
139+
},
140+
},
141+
}
142+
143+
// Execute the setter
144+
setter := serviceSpecSetter(existingService, desiredSpec, desiredMeta)
145+
err := setter()
146+
147+
g.Expect(err).ToNot(HaveOccurred())
148+
g.Expect(existingService.Annotations).To(Equal(tt.expectedAnnotations))
149+
g.Expect(existingService.Labels).To(Equal(desiredMeta.Labels))
150+
g.Expect(existingService.Spec).To(Equal(desiredSpec))
151+
})
152+
}
153+
}

0 commit comments

Comments
 (0)