From 5072e7cc69ea252d3c7d780b6cf3bc53253963a1 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Thu, 5 Dec 2024 10:25:46 +0530 Subject: [PATCH 1/5] Fix update logic to preserve k8s specific labels & annotations Operator overwrites k8s added labels or annotations. This sometimes results into unexpected behaviour. Eg. A kubectl rollout restart on deployment adds an annotation "kubectl.kubernetes.io/restartedAt" with timestamp. However operator detects this as a config drift and removes the annotations resulting into k8s terminating the rollout due to change in config. This commit fixes the issue by preserving such annotations & labels added onto live object during comparison. Signed-off-by: Siddhesh Ghadi --- controllers/argocd/applicationset.go | 8 ++- controllers/argocd/deployment.go | 24 +++++---- controllers/argocd/statefulset.go | 13 +++-- controllers/argocd/util.go | 28 ++++++++++ controllers/argocd/util_test.go | 79 ++++++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 16 deletions(-) diff --git a/controllers/argocd/applicationset.go b/controllers/argocd/applicationset.go index 6db347838..4cf9a7351 100644 --- a/controllers/argocd/applicationset.go +++ b/controllers/argocd/applicationset.go @@ -252,7 +252,9 @@ func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD, } if cr.Spec.ApplicationSet.Annotations != nil { - deploy.Spec.Template.Annotations = cr.Spec.ApplicationSet.Annotations + for key, value := range cr.Spec.ApplicationSet.Annotations { + deploy.Spec.Template.Annotations[key] = value + } } if cr.Spec.ApplicationSet.Labels != nil { @@ -270,6 +272,10 @@ func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD, existingSpec := existing.Spec.Template.Spec + // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. + deploy.Spec.Template.Labels = addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) + deploy.Spec.Template.Annotations = addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) + deploymentsDifferent := !reflect.DeepEqual(existingSpec.Containers[0], podSpec.Containers) || !reflect.DeepEqual(existingSpec.Volumes, podSpec.Volumes) || existingSpec.ServiceAccountName != podSpec.ServiceAccountName || diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go index baf91fca6..074b121e5 100644 --- a/controllers/argocd/deployment.go +++ b/controllers/argocd/deployment.go @@ -395,6 +395,7 @@ func newDeploymentWithName(name string, component string, cr *argoproj.ArgoCD) * Labels: map[string]string{ common.ArgoCDKeyName: name, }, + Annotations: make(map[string]string), }, Spec: corev1.PodSpec{ NodeSelector: common.DefaultNodeSelector(), @@ -1110,7 +1111,9 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor } if cr.Spec.Repo.Annotations != nil { - deploy.Spec.Template.Annotations = cr.Spec.Repo.Annotations + for key, value := range cr.Spec.Repo.Annotations { + deploy.Spec.Template.Annotations[key] = value + } } if cr.Spec.Repo.Labels != nil { @@ -1197,15 +1200,15 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor changed = true } - deploy.Spec.Template.Annotations = cr.Spec.Repo.Annotations + // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. + deploy.Spec.Template.Labels = addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) + deploy.Spec.Template.Annotations = addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) + if !reflect.DeepEqual(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) { existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations changed = true } - for key, value := range cr.Spec.Repo.Labels { - deploy.Spec.Template.Labels[key] = value - } if !reflect.DeepEqual(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) { existing.Spec.Template.Labels = deploy.Spec.Template.Labels changed = true @@ -1405,7 +1408,9 @@ func (r *ReconcileArgoCD) reconcileServerDeployment(cr *argoproj.ArgoCD, useTLSF } if cr.Spec.Server.Annotations != nil { - deploy.Spec.Template.Annotations = cr.Spec.Server.Annotations + for key, value := range cr.Spec.Server.Annotations { + deploy.Spec.Template.Annotations[key] = value + } } if cr.Spec.Server.Labels != nil { @@ -1479,11 +1484,10 @@ func (r *ReconcileArgoCD) reconcileServerDeployment(cr *argoproj.ArgoCD, useTLSF } } - deploy.Spec.Template.Annotations = cr.Spec.Server.Annotations + // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. + deploy.Spec.Template.Labels = addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) + deploy.Spec.Template.Annotations = addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) - for key, value := range cr.Spec.Server.Labels { - deploy.Spec.Template.Labels[key] = value - } if !reflect.DeepEqual(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) { existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations changed = true diff --git a/controllers/argocd/statefulset.go b/controllers/argocd/statefulset.go index de70f2176..5a996586d 100644 --- a/controllers/argocd/statefulset.go +++ b/controllers/argocd/statefulset.go @@ -71,6 +71,7 @@ func newStatefulSetWithName(name string, component string, cr *argoproj.ArgoCD) Labels: map[string]string{ common.ArgoCDKeyName: name, }, + Annotations: make(map[string]string), }, Spec: corev1.PodSpec{ NodeSelector: common.DefaultNodeSelector(), @@ -743,7 +744,9 @@ func (r *ReconcileArgoCD) reconcileApplicationControllerStatefulSet(cr *argoproj } if cr.Spec.Controller.Annotations != nil { - ss.Spec.Template.Annotations = cr.Spec.Controller.Annotations + for key, value := range cr.Spec.Controller.Annotations { + ss.Spec.Template.Annotations[key] = value + } } if cr.Spec.Controller.Labels != nil { @@ -814,15 +817,15 @@ func (r *ReconcileArgoCD) reconcileApplicationControllerStatefulSet(cr *argoproj changed = true } - ss.Spec.Template.Annotations = cr.Spec.Controller.Annotations + // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. + ss.Spec.Template.Labels = addKubernetesData(ss.Spec.Template.Labels, existing.Spec.Template.Labels) + ss.Spec.Template.Annotations = addKubernetesData(ss.Spec.Template.Annotations, existing.Spec.Template.Annotations) + if !reflect.DeepEqual(ss.Spec.Template.Annotations, existing.Spec.Template.Annotations) { existing.Spec.Template.Annotations = ss.Spec.Template.Annotations changed = true } - for key, value := range cr.Spec.Controller.Labels { - ss.Spec.Template.Labels[key] = value - } if !reflect.DeepEqual(ss.Spec.Template.Labels, existing.Spec.Template.Labels) { existing.Spec.Template.Labels = ss.Spec.Template.Labels changed = true diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index d9f08597a..e97634bd0 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1688,3 +1688,31 @@ func UseServer(name string, cr *argoproj.ArgoCD) bool { } return true } + +// addKubernetesData checks for any Kubernetes-specific labels or annotations +// in the live object and updates the source object to ensure critical metadata +// (like scheduling, topology, or lifecycle information) is retained. +// This helps avoid loss of important Kubernetes-managed metadata during updates. +func addKubernetesData(source map[string]string, live map[string]string) map[string]string { + + // List of Kubernetes-specific substrings (wildcard match) + patterns := []string{ + "*kubernetes.io*", + "*k8s.io*", + "*openshift.io*", + } + + for key, value := range live { + found := glob.MatchStringInList(patterns, key, glob.GLOB) + if found { + // Don't override values already present in the source object. + // This ensures users have control over Kubernetes-managed data + // if they have intentionally set or modified these values in the source object. + if _, ok := source[key]; !ok { + source[key] = value + } + } + } + + return source +} diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go index 68bcba92b..d00b2d81a 100644 --- a/controllers/argocd/util_test.go +++ b/controllers/argocd/util_test.go @@ -1094,3 +1094,82 @@ func TestReconcileArgoCD_reconcileDexOAuthClientSecret(t *testing.T) { } assert.True(t, tokenExists, "Dex is enabled but unable to create oauth client secret") } + +func TestRetainKubernetesData(t *testing.T) { + tests := []struct { + name string + source map[string]string + live map[string]string + expected map[string]string + }{ + { + name: "Add Kubernetes-specific keys not in source", + source: map[string]string{ + "custom-label": "custom-value", + }, + live: map[string]string{ + "node.kubernetes.io/pod": "true", + "kubectl.kubernetes.io/restartedAt": "2024-12-05T09:46:46+05:30", + "openshift.openshift.io/restartedAt": "2024-12-05T09:46:46+05:30", + }, + expected: map[string]string{ + "custom-label": "custom-value", // unchanged + "node.kubernetes.io/pod": "true", // added + "kubectl.kubernetes.io/restartedAt": "2024-12-05T09:46:46+05:30", // added + "openshift.openshift.io/restartedAt": "2024-12-05T09:46:46+05:30", // added + }, + }, + { + name: "Ignores non-Kubernetes-specific keys", + source: map[string]string{ + "custom-label": "custom-value", + }, + live: map[string]string{ + "non-k8s-key": "non-k8s-value", + "custom-label": "live-value", + }, + expected: map[string]string{ + "custom-label": "custom-value", // unchanged + }, + }, + { + name: "Do not override existing Kubernetes-specific keys in source", + source: map[string]string{ + "node.kubernetes.io/pod": "source-true", + }, + live: map[string]string{ + "node.kubernetes.io/pod": "live-true", // should not override + }, + expected: map[string]string{ + "node.kubernetes.io/pod": "source-true", // source takes precedence + }, + }, + { + name: "Handles empty live map", + source: map[string]string{ + "custom-label": "custom-value", + }, + live: map[string]string{}, + expected: map[string]string{ + "custom-label": "custom-value", // unchanged + }, + }, + { + name: "Handles empty source map", + source: map[string]string{}, + live: map[string]string{ + "openshift.io/resource": "value", + }, + expected: map[string]string{ + "openshift.io/resource": "value", // added from live + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + actual := addKubernetesData(tt.source, tt.live) + assert.Equal(t, tt.expected, actual) + }) + } +} From a59f4a8590911181b39413d109fe11f6f8b91e46 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Thu, 5 Dec 2024 16:05:54 +0530 Subject: [PATCH 2/5] refactor: remove return Signed-off-by: Siddhesh Ghadi --- controllers/argocd/applicationset.go | 4 ++-- controllers/argocd/deployment.go | 8 ++++---- controllers/argocd/statefulset.go | 4 ++-- controllers/argocd/util.go | 4 +--- controllers/argocd/util_test.go | 4 ++-- 5 files changed, 11 insertions(+), 13 deletions(-) diff --git a/controllers/argocd/applicationset.go b/controllers/argocd/applicationset.go index 4cf9a7351..0b6168a36 100644 --- a/controllers/argocd/applicationset.go +++ b/controllers/argocd/applicationset.go @@ -273,8 +273,8 @@ func (r *ReconcileArgoCD) reconcileApplicationSetDeployment(cr *argoproj.ArgoCD, existingSpec := existing.Spec.Template.Spec // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. - deploy.Spec.Template.Labels = addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) - deploy.Spec.Template.Annotations = addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) + addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) + addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) deploymentsDifferent := !reflect.DeepEqual(existingSpec.Containers[0], podSpec.Containers) || !reflect.DeepEqual(existingSpec.Volumes, podSpec.Volumes) || diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go index 074b121e5..6d0897e5c 100644 --- a/controllers/argocd/deployment.go +++ b/controllers/argocd/deployment.go @@ -1201,8 +1201,8 @@ func (r *ReconcileArgoCD) reconcileRepoDeployment(cr *argoproj.ArgoCD, useTLSFor } // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. - deploy.Spec.Template.Labels = addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) - deploy.Spec.Template.Annotations = addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) + addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) + addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) if !reflect.DeepEqual(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) { existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations @@ -1485,8 +1485,8 @@ func (r *ReconcileArgoCD) reconcileServerDeployment(cr *argoproj.ArgoCD, useTLSF } // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. - deploy.Spec.Template.Labels = addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) - deploy.Spec.Template.Annotations = addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) + addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels) + addKubernetesData(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) if !reflect.DeepEqual(deploy.Spec.Template.Annotations, existing.Spec.Template.Annotations) { existing.Spec.Template.Annotations = deploy.Spec.Template.Annotations diff --git a/controllers/argocd/statefulset.go b/controllers/argocd/statefulset.go index 5a996586d..80c9400e5 100644 --- a/controllers/argocd/statefulset.go +++ b/controllers/argocd/statefulset.go @@ -818,8 +818,8 @@ func (r *ReconcileArgoCD) reconcileApplicationControllerStatefulSet(cr *argoproj } // Add Kubernetes-specific labels/annotations from the live object in the source to preserve metadata. - ss.Spec.Template.Labels = addKubernetesData(ss.Spec.Template.Labels, existing.Spec.Template.Labels) - ss.Spec.Template.Annotations = addKubernetesData(ss.Spec.Template.Annotations, existing.Spec.Template.Annotations) + addKubernetesData(ss.Spec.Template.Labels, existing.Spec.Template.Labels) + addKubernetesData(ss.Spec.Template.Annotations, existing.Spec.Template.Annotations) if !reflect.DeepEqual(ss.Spec.Template.Annotations, existing.Spec.Template.Annotations) { existing.Spec.Template.Annotations = ss.Spec.Template.Annotations diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index e97634bd0..9fd06a3ff 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1693,7 +1693,7 @@ func UseServer(name string, cr *argoproj.ArgoCD) bool { // in the live object and updates the source object to ensure critical metadata // (like scheduling, topology, or lifecycle information) is retained. // This helps avoid loss of important Kubernetes-managed metadata during updates. -func addKubernetesData(source map[string]string, live map[string]string) map[string]string { +func addKubernetesData(source map[string]string, live map[string]string) { // List of Kubernetes-specific substrings (wildcard match) patterns := []string{ @@ -1713,6 +1713,4 @@ func addKubernetesData(source map[string]string, live map[string]string) map[str } } } - - return source } diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go index d00b2d81a..c1b9c3b0c 100644 --- a/controllers/argocd/util_test.go +++ b/controllers/argocd/util_test.go @@ -1168,8 +1168,8 @@ func TestRetainKubernetesData(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - actual := addKubernetesData(tt.source, tt.live) - assert.Equal(t, tt.expected, actual) + addKubernetesData(tt.source, tt.live) + assert.Equal(t, tt.expected, tt.source) }) } } From 933ecc34ea91f5908fec85f7a832b7ed2c8aa0a0 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Thu, 5 Dec 2024 16:37:56 +0530 Subject: [PATCH 3/5] remove edge case check Signed-off-by: Siddhesh Ghadi --- controllers/argocd/util.go | 7 +------ controllers/argocd/util_test.go | 12 ------------ 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index 9fd06a3ff..14490aee1 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1705,12 +1705,7 @@ func addKubernetesData(source map[string]string, live map[string]string) { for key, value := range live { found := glob.MatchStringInList(patterns, key, glob.GLOB) if found { - // Don't override values already present in the source object. - // This ensures users have control over Kubernetes-managed data - // if they have intentionally set or modified these values in the source object. - if _, ok := source[key]; !ok { - source[key] = value - } + source[key] = value } } } diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go index c1b9c3b0c..d336ca35e 100644 --- a/controllers/argocd/util_test.go +++ b/controllers/argocd/util_test.go @@ -1132,18 +1132,6 @@ func TestRetainKubernetesData(t *testing.T) { "custom-label": "custom-value", // unchanged }, }, - { - name: "Do not override existing Kubernetes-specific keys in source", - source: map[string]string{ - "node.kubernetes.io/pod": "source-true", - }, - live: map[string]string{ - "node.kubernetes.io/pod": "live-true", // should not override - }, - expected: map[string]string{ - "node.kubernetes.io/pod": "source-true", // source takes precedence - }, - }, { name: "Handles empty live map", source: map[string]string{ From 7667f068445a2974a03f46f8b864d2ab12d44803 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Thu, 5 Dec 2024 16:42:51 +0530 Subject: [PATCH 4/5] Revert "remove edge case check" This reverts commit 933ecc34ea91f5908fec85f7a832b7ed2c8aa0a0. --- controllers/argocd/util.go | 7 ++++++- controllers/argocd/util_test.go | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index 14490aee1..9fd06a3ff 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1705,7 +1705,12 @@ func addKubernetesData(source map[string]string, live map[string]string) { for key, value := range live { found := glob.MatchStringInList(patterns, key, glob.GLOB) if found { - source[key] = value + // Don't override values already present in the source object. + // This ensures users have control over Kubernetes-managed data + // if they have intentionally set or modified these values in the source object. + if _, ok := source[key]; !ok { + source[key] = value + } } } } diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go index d336ca35e..c1b9c3b0c 100644 --- a/controllers/argocd/util_test.go +++ b/controllers/argocd/util_test.go @@ -1132,6 +1132,18 @@ func TestRetainKubernetesData(t *testing.T) { "custom-label": "custom-value", // unchanged }, }, + { + name: "Do not override existing Kubernetes-specific keys in source", + source: map[string]string{ + "node.kubernetes.io/pod": "source-true", + }, + live: map[string]string{ + "node.kubernetes.io/pod": "live-true", // should not override + }, + expected: map[string]string{ + "node.kubernetes.io/pod": "source-true", // source takes precedence + }, + }, { name: "Handles empty live map", source: map[string]string{ From 2b1b23343d71edeb4edb531d52964816f25fce01 Mon Sep 17 00:00:00 2001 From: Siddhesh Ghadi Date: Thu, 5 Dec 2024 16:58:08 +0530 Subject: [PATCH 5/5] Rephrase the comment Signed-off-by: Siddhesh Ghadi --- controllers/argocd/util.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index 9fd06a3ff..a52eed32c 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1706,8 +1706,7 @@ func addKubernetesData(source map[string]string, live map[string]string) { found := glob.MatchStringInList(patterns, key, glob.GLOB) if found { // Don't override values already present in the source object. - // This ensures users have control over Kubernetes-managed data - // if they have intentionally set or modified these values in the source object. + // This allows the operator to update Kubernetes specific data when needed. if _, ok := source[key]; !ok { source[key] = value }