fix: update logic to preserve k8s specific labels & annotations (#1616)

* 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 <[email protected]>
argoproj-labs · Dec 5, 2024 · e13f2be · e13f2be
1 parent 8892f04
commit e13f2be
Show file tree

Hide file tree

Showing 5 changed files with 133 additions and 16 deletions.
diff --git 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.
+		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) ||
 			existingSpec.ServiceAccountName != podSpec.ServiceAccountName ||

diff --git 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.
+		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
 			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.
+		addKubernetesData(deploy.Spec.Template.Labels, existing.Spec.Template.Labels)
+		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
@@ -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.
+		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
 			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
@@ -1664,3 +1664,28 @@ func getApplicationSetHTTPServerHost(cr *argoproj.ArgoCD) (string, error) {
 	}
 	return host, nil
 }
+
+// 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) {
+
+	// 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 allows the operator to update Kubernetes specific data when needed.
+			if _, ok := source[key]; !ok {
+				source[key] = value
+			}
+		}
+	}
+}
diff --git 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) {
+			addKubernetesData(tt.source, tt.live)
+			assert.Equal(t, tt.expected, tt.source)
+		})
+	}
+}