Skip to content

Commit 15d7261

Browse files
salonichf5salonichf5
authored
Preserve external controller annotations for deployment and daemonSet (#4468) (#4473)
Problem: Users want that deployments and daemonSet preserve external annotations like how we do for services Solution: Adds a solution to track internal annotations and preserver external annotations.
1 parent 0cd966b commit 15d7261

File tree

2 files changed

+253
-49
lines changed

2 files changed

+253
-49
lines changed

‎internal/controller/provisioner/setter.go‎

Lines changed: 55 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ func deploymentSpecSetter(
4747
) controllerutil.MutateFn {
4848
return func() error {
4949
deployment.Labels = objectMeta.Labels
50-
deployment.Annotations = objectMeta.Annotations
50+
deployment.Annotations = mergeAnnotations(deployment.Annotations, objectMeta.Annotations)
5151
deployment.Spec = spec
5252
return nil
5353
}
@@ -73,7 +73,7 @@ func daemonSetSpecSetter(
7373
) controllerutil.MutateFn {
7474
return func() error {
7575
daemonSet.Labels = objectMeta.Labels
76-
daemonSet.Annotations = objectMeta.Annotations
76+
daemonSet.Annotations = mergeAnnotations(daemonSet.Annotations, objectMeta.Annotations)
7777
daemonSet.Spec = spec
7878
return nil
7979
}
@@ -85,54 +85,8 @@ func serviceSpecSetter(
8585
objectMeta metav1.ObjectMeta,
8686
) controllerutil.MutateFn {
8787
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-
maps.Copy(mergedAnnotations, objectMeta.Annotations)
123-
124-
// Store current managed keys for next reconciliation
125-
if len(currentManagedKeys) > 0 {
126-
var managedKeysList []string
127-
for k := range currentManagedKeys {
128-
managedKeysList = append(managedKeysList, k)
129-
}
130-
slices.Sort(managedKeysList) // Sort for deterministic output
131-
mergedAnnotations[managedKeysAnnotation] = strings.Join(managedKeysList, ",")
132-
}
133-
13488
service.Labels = objectMeta.Labels
135-
service.Annotations = mergedAnnotations
89+
service.Annotations = mergeAnnotations(service.Annotations, objectMeta.Annotations)
13690
service.Spec = spec
13791
return nil
13892
}
@@ -210,3 +164,55 @@ func roleBindingSpecSetter(
210164
return nil
211165
}
212166
}
167+
168+
func mergeAnnotations(existing, desired map[string]string) map[string]string {
169+
const trackingKey = "gateway.nginx.org/internal-managed-annotation-keys"
170+
desiredKeys := make(map[string]struct{}, len(desired))
171+
for key := range desired {
172+
desiredKeys[key] = struct{}{}
173+
}
174+
175+
previousKeys := make(map[string]struct{}, len(existing))
176+
if existing != nil {
177+
if prev, ok := existing[trackingKey]; ok {
178+
for splitKey := range strings.SplitSeq(prev, ",") {
179+
if splitKey != "" {
180+
previousKeys[splitKey] = struct{}{}
181+
}
182+
}
183+
}
184+
}
185+
186+
annotations := make(map[string]string)
187+
188+
// Start with existing annotations (preserves external controller annotations)
189+
for key, value := range existing {
190+
if key == trackingKey {
191+
continue
192+
}
193+
194+
// if this key was previously managed and is no longer desired, drop it
195+
if _, wasManaged := previousKeys[key]; wasManaged {
196+
if _, stillDesired := desiredKeys[key]; !stillDesired {
197+
continue
198+
}
199+
}
200+
201+
annotations[key] = value
202+
}
203+
204+
// Apply desired annotations (NGF-managed wins)
205+
maps.Copy(annotations, desired)
206+
207+
// Store current managed keys
208+
if len(desiredKeys) > 0 {
209+
keys := make([]string, 0, len(desiredKeys))
210+
for key := range desiredKeys {
211+
keys = append(keys, key)
212+
}
213+
slices.Sort(keys)
214+
annotations[trackingKey] = strings.Join(keys, ",")
215+
}
216+
217+
return annotations
218+
}

‎internal/controller/provisioner/setter_test.go‎

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"testing"
55

66
. "github.com/onsi/gomega"
7+
appsv1 "k8s.io/api/apps/v1"
78
corev1 "k8s.io/api/core/v1"
89
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
910
)
@@ -151,3 +152,200 @@ func TestServiceSpecSetter_PreservesExternalAnnotations(t *testing.T) {
151152
})
152153
}
153154
}
155+
156+
func int32Ptr(i int32) *int32 { return &i }
157+
158+
func TestDeploymentAndDaemonSetSpecSetter(t *testing.T) {
159+
t.Parallel()
160+
161+
type testCase struct {
162+
existingAnnotations map[string]string
163+
desiredAnnotations map[string]string
164+
expectedAnnotations map[string]string
165+
name string
166+
}
167+
168+
tests := []testCase{
169+
{
170+
name: "preserves external annotations while adding NGF annotations",
171+
existingAnnotations: map[string]string{
172+
"deployment.kubernetes.io/revision": "1",
173+
"field.cattle.io/publicEndpoints": "192.61.0.19",
174+
"field.cattle.io/ports": "80/tcp",
175+
},
176+
desiredAnnotations: map[string]string{
177+
"custom.annotation": "from-ngf",
178+
},
179+
expectedAnnotations: map[string]string{
180+
"deployment.kubernetes.io/revision": "1",
181+
"field.cattle.io/publicEndpoints": "192.61.0.19",
182+
"field.cattle.io/ports": "80/tcp",
183+
"custom.annotation": "from-ngf",
184+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
185+
},
186+
},
187+
{
188+
name: "preserves existing NGF-managed annotations when still desired",
189+
existingAnnotations: map[string]string{
190+
"custom.annotation": "keep-me",
191+
"argocd.argoproj.io/sync-options": "Prune=false",
192+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
193+
},
194+
desiredAnnotations: map[string]string{
195+
"custom.annotation": "keep-me",
196+
},
197+
expectedAnnotations: map[string]string{
198+
"custom.annotation": "keep-me",
199+
"argocd.argoproj.io/sync-options": "Prune=false",
200+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
201+
},
202+
},
203+
{
204+
name: "removes NGF-managed annotations when no longer desired",
205+
existingAnnotations: map[string]string{
206+
"custom.annotation": "should-be-removed",
207+
"deployment.kubernetes.io/revision": "2",
208+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
209+
},
210+
desiredAnnotations: map[string]string{},
211+
expectedAnnotations: map[string]string{
212+
"deployment.kubernetes.io/revision": "2",
213+
},
214+
},
215+
{
216+
name: "NGF annotations take precedence on conflicts",
217+
existingAnnotations: map[string]string{
218+
"custom.annotation": "old-value",
219+
"daemonSet.kubernetes.io/revision": "7",
220+
},
221+
desiredAnnotations: map[string]string{
222+
"custom.annotation": "new-value",
223+
},
224+
expectedAnnotations: map[string]string{
225+
"custom.annotation": "new-value",
226+
"daemonSet.kubernetes.io/revision": "7",
227+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
228+
},
229+
},
230+
{
231+
name: "creates new deployment with annotations",
232+
existingAnnotations: nil,
233+
desiredAnnotations: map[string]string{
234+
"custom.annotation": "value",
235+
},
236+
expectedAnnotations: map[string]string{
237+
"custom.annotation": "value",
238+
"gateway.nginx.org/internal-managed-annotation-keys": "custom.annotation",
239+
},
240+
},
241+
{
242+
name: "updates tracking annotation when managed keys change",
243+
existingAnnotations: map[string]string{
244+
"annotation-to-keep": "keep-value",
245+
"annotation-to-remove": "remove-value",
246+
"argocd.argoproj.io/sync-options": "Validate=true",
247+
"gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep,annotation-to-remove",
248+
},
249+
desiredAnnotations: map[string]string{
250+
"annotation-to-keep": "updated-keep-value",
251+
},
252+
expectedAnnotations: map[string]string{
253+
"annotation-to-keep": "updated-keep-value",
254+
"argocd.argoproj.io/sync-options": "Validate=true",
255+
"gateway.nginx.org/internal-managed-annotation-keys": "annotation-to-keep",
256+
},
257+
},
258+
}
259+
260+
labels := map[string]string{
261+
"app.kubernetes.io/name": "nginx-gateway-fabric",
262+
"app.kubernetes.io/instance": "nginx-gateway",
263+
}
264+
265+
makeDesiredMeta := func(ann map[string]string) metav1.ObjectMeta {
266+
return metav1.ObjectMeta{
267+
Labels: labels,
268+
Annotations: ann,
269+
}
270+
}
271+
272+
podTemplate := corev1.PodTemplateSpec{
273+
ObjectMeta: metav1.ObjectMeta{Labels: labels},
274+
Spec: corev1.PodSpec{
275+
Containers: []corev1.Container{{
276+
Name: "nginx-gateway",
277+
Image: "nginx:1.25",
278+
}},
279+
},
280+
}
281+
282+
runners := []struct {
283+
run func(t *testing.T, tc testCase)
284+
name string
285+
}{
286+
{
287+
name: "Deployment",
288+
run: func(t *testing.T, tc testCase) {
289+
t.Helper()
290+
g := NewWithT(t)
291+
292+
existing := &appsv1.Deployment{
293+
ObjectMeta: metav1.ObjectMeta{
294+
Name: "nginx-gateway",
295+
Namespace: "nginx-gateway",
296+
Annotations: tc.existingAnnotations,
297+
},
298+
}
299+
300+
spec := appsv1.DeploymentSpec{
301+
Replicas: int32Ptr(1),
302+
Selector: &metav1.LabelSelector{MatchLabels: labels},
303+
Template: podTemplate,
304+
}
305+
306+
err := deploymentSpecSetter(existing, spec, makeDesiredMeta(tc.desiredAnnotations))()
307+
g.Expect(err).ToNot(HaveOccurred())
308+
g.Expect(existing.Annotations).To(Equal(tc.expectedAnnotations))
309+
g.Expect(existing.Labels).To(Equal(labels))
310+
g.Expect(existing.Spec).To(Equal(spec))
311+
},
312+
},
313+
{
314+
name: "DaemonSet",
315+
run: func(t *testing.T, tc testCase) {
316+
t.Helper()
317+
g := NewWithT(t)
318+
319+
existing := &appsv1.DaemonSet{
320+
ObjectMeta: metav1.ObjectMeta{
321+
Name: "nginx-gateway",
322+
Namespace: "nginx-gateway",
323+
Annotations: tc.existingAnnotations,
324+
},
325+
}
326+
327+
spec := appsv1.DaemonSetSpec{
328+
Selector: &metav1.LabelSelector{MatchLabels: labels},
329+
Template: podTemplate,
330+
}
331+
332+
err := daemonSetSpecSetter(existing, spec, makeDesiredMeta(tc.desiredAnnotations))()
333+
g.Expect(err).ToNot(HaveOccurred())
334+
g.Expect(existing.Annotations).To(Equal(tc.expectedAnnotations))
335+
g.Expect(existing.Labels).To(Equal(labels))
336+
g.Expect(existing.Spec).To(Equal(spec))
337+
},
338+
},
339+
}
340+
341+
for _, tc := range tests {
342+
t.Run(tc.name, func(t *testing.T) {
343+
t.Parallel()
344+
for _, r := range runners {
345+
t.Run(r.name, func(t *testing.T) {
346+
r.run(t, tc)
347+
})
348+
}
349+
})
350+
}
351+
}

0 commit comments

Comments
 (0)