Skip to content

Commit b12b092

Browse files
ciarams87ciarams87
committed
Preserve external controller annotations on Services
1 parent dd60012 commit b12b092

File tree

2 files changed

+201
-1
lines changed

2 files changed

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

0 commit comments

Comments
 (0)