Fix issue with ArgoCD Redis HA components not updating during operato…

…r upgrades (#1652) * add change check for redis ha proxy deployment Signed-off-by: Mangaal <[email protected]> * add change check for redis ha server deployment Signed-off-by: Mangaal <[email protected]> * add unit test Signed-off-by: Mangaal <[email protected]> * Remove unnecessary check Signed-off-by: Mangaal <[email protected]> * Remove unnecessary check in unit test Signed-off-by: Mangaal <[email protected]> * add a single test for initcontainer Signed-off-by: Mangaal <[email protected]> --------- Signed-off-by: Mangaal <[email protected]>
argoproj-labs · Feb 4, 2025 · 8210187 · 8210187
1 parent 5cce9ce
commit 8210187
Show file tree

Hide file tree

Showing 4 changed files with 253 additions and 31 deletions.
diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go
@@ -834,43 +834,56 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyDeployment(cr *argoproj.ArgoCD) e
 			changed = true
 		}
 		updateNodePlacement(existing, deploy, &changed, &explanation)
-
-		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Containers[0].Resources, existing.Spec.Template.Spec.Containers[0].Resources) {
-			existing.Spec.Template.Spec.Containers[0].Resources = deploy.Spec.Template.Spec.Containers[0].Resources
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Volumes, existing.Spec.Template.Spec.Volumes) {
+			existing.Spec.Template.Spec.Volumes = deploy.Spec.Template.Spec.Volumes
 			if changed {
 				explanation += ", "
 			}
-			explanation += "container resources"
+			explanation += "volumes"
 			changed = true
 		}
-
-		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Containers[0].SecurityContext, existing.Spec.Template.Spec.Containers[0].SecurityContext) {
-			existing.Spec.Template.Spec.Containers[0].SecurityContext = deploy.Spec.Template.Spec.Containers[0].SecurityContext
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Containers[0].VolumeMounts,
+			existing.Spec.Template.Spec.Containers[0].VolumeMounts) {
+			existing.Spec.Template.Spec.Containers[0].VolumeMounts = deploy.Spec.Template.Spec.Containers[0].VolumeMounts
 			if changed {
 				explanation += ", "
 			}
-			explanation += "container security context"
+			explanation += "container volume mounts"
 			changed = true
 		}
-
-		if !reflect.DeepEqual(deploy.Spec.Template.Spec.InitContainers[0].Resources, existing.Spec.Template.Spec.InitContainers[0].Resources) {
-			existing.Spec.Template.Spec.InitContainers[0].Resources = deploy.Spec.Template.Spec.InitContainers[0].Resources
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.InitContainers, existing.Spec.Template.Spec.InitContainers) {
+			existing.Spec.Template.Spec.InitContainers = deploy.Spec.Template.Spec.InitContainers
 			if changed {
 				explanation += ", "
 			}
-			explanation += "init container resources"
+			explanation += "init containers"
 			changed = true
 		}
-
-		if !reflect.DeepEqual(deploy.Spec.Template.Spec.InitContainers[0].SecurityContext, existing.Spec.Template.Spec.InitContainers[0].SecurityContext) {
-			existing.Spec.Template.Spec.InitContainers[0].SecurityContext = deploy.Spec.Template.Spec.InitContainers[0].SecurityContext
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Containers[0].Env,
+			existing.Spec.Template.Spec.Containers[0].Env) {
+			existing.Spec.Template.Spec.Containers[0].Env = deploy.Spec.Template.Spec.Containers[0].Env
 			if changed {
 				explanation += ", "
 			}
-			explanation += "init container security context"
+			explanation += "container env"
+			changed = true
+		}
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Containers[0].Resources, existing.Spec.Template.Spec.Containers[0].Resources) {
+			existing.Spec.Template.Spec.Containers[0].Resources = deploy.Spec.Template.Spec.Containers[0].Resources
+			if changed {
+				explanation += ", "
+			}
+			explanation += "container resources"
+			changed = true
+		}
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Containers[0].SecurityContext, existing.Spec.Template.Spec.Containers[0].SecurityContext) {
+			existing.Spec.Template.Spec.Containers[0].SecurityContext = deploy.Spec.Template.Spec.Containers[0].SecurityContext
+			if changed {
+				explanation += ", "
+			}
+			explanation += "container security context"
 			changed = true
 		}
-
 		if changed {
 			argoutil.LogResourceUpdate(log, existing, "updating", explanation)
 			return r.Client.Update(context.TODO(), existing)

diff --git a/controllers/argocd/deployment_test.go b/controllers/argocd/deployment_test.go
@@ -887,6 +887,118 @@ func TestReconcileArgoCD_reconcileDeployments_HA_proxy_with_resources(t *testing
 	assert.Equal(t, deployment.Spec.Template.Spec.Containers[0].Resources, newResources)
 	assert.Equal(t, deployment.Spec.Template.Spec.InitContainers[0].Resources, newResources)
 }
+func TestReconcileArgoCD_reconcileRedisHAProxyDeployment_ModifyContainerSpec(t *testing.T) {
+	logf.SetLogger(ZapLogger(true))
+
+	a := makeTestArgoCD(func(a *argoproj.ArgoCD) {
+		a.Spec.HA.Enabled = true
+	})
+
+	resObjs := []client.Object{a}
+	subresObjs := []client.Object{a}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	assert.NoError(t, r.reconcileRedisHAProxyDeployment(a))
+
+	deployment := &appsv1.Deployment{}
+	assert.NoError(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-redis-ha-haproxy",
+			Namespace: a.Namespace,
+		},
+		deployment))
+
+	// Modify the deployment container environment variables
+	deployment.Spec.Template.Spec.Containers[0].Env = append(deployment.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{
+		Name:  "TEST_ENV",
+		Value: "test",
+	})
+
+	assert.NoError(t, r.Client.Update(context.TODO(), deployment))
+
+	// Reconcile again
+	assert.NoError(t, r.reconcileRedisHAProxyDeployment(a))
+
+	// Check if the environment variable changes were reverted
+	assert.NoError(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-redis-ha-haproxy",
+			Namespace: a.Namespace,
+		},
+		deployment))
+
+	assert.NotContains(t, deployment.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{
+		Name:  "TEST_ENV",
+		Value: "test",
+	})
+
+	// Modify the deployment initcontainer environment variables
+	deployment.Spec.Template.Spec.InitContainers[0].Env = append(deployment.Spec.Template.Spec.InitContainers[0].Env, corev1.EnvVar{
+		Name:  "TEST_ENV",
+		Value: "test",
+	})
+
+	assert.NoError(t, r.Client.Update(context.TODO(), deployment))
+
+	// Reconcile again
+	assert.NoError(t, r.reconcileRedisHAProxyDeployment(a))
+
+	// Check if the environment variable changes were reverted
+	assert.NoError(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-redis-ha-haproxy",
+			Namespace: a.Namespace,
+		},
+		deployment))
+
+	assert.NotContains(t, deployment.Spec.Template.Spec.InitContainers[0].Env, corev1.EnvVar{
+		Name:  "TEST_ENV",
+		Value: "test",
+	})
+
+	// Modify the deployment volumes
+	deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, corev1.Volume{
+		Name: "test-volume",
+		VolumeSource: corev1.VolumeSource{
+			EmptyDir: &corev1.EmptyDirVolumeSource{},
+		},
+	})
+	deployment.Spec.Template.Spec.Containers[0].VolumeMounts = append(deployment.Spec.Template.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{
+		Name:      "test-volume",
+		MountPath: "/test",
+	})
+
+	assert.NoError(t, r.Client.Update(context.TODO(), deployment))
+
+	// Reconcile again
+	assert.NoError(t, r.reconcileRedisHAProxyDeployment(a))
+
+	// Check if the volume changes were reverted
+	assert.NoError(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-redis-ha-haproxy",
+			Namespace: a.Namespace,
+		},
+		deployment))
+
+	assert.NotContains(t, deployment.Spec.Template.Spec.Volumes, corev1.Volume{
+		Name: "test-volume",
+		VolumeSource: corev1.VolumeSource{
+			EmptyDir: &corev1.EmptyDirVolumeSource{},
+		},
+	})
+	assert.NotContains(t, deployment.Spec.Template.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{
+		Name:      "test-volume",
+		MountPath: "/test",
+	})
+}
 
 func TestReconcileArgoCD_reconcileRepoDeployment_updatesVolumeMounts(t *testing.T) {
 	logf.SetLogger(ZapLogger(true))

diff --git a/controllers/argocd/statefulset.go b/controllers/argocd/statefulset.go
@@ -452,6 +452,14 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoproj.ArgoCD) error {
 				explanation += fmt.Sprintf("container '%s' image", container.Name)
 				changed = true
 			}
+			if !reflect.DeepEqual(ss.Spec.Template.Spec.Containers[i].VolumeMounts, existing.Spec.Template.Spec.Containers[i].VolumeMounts) {
+				existing.Spec.Template.Spec.Containers[i].VolumeMounts = ss.Spec.Template.Spec.Containers[i].VolumeMounts
+				if changed {
+					explanation += ", "
+				}
+				explanation += fmt.Sprintf("container '%s' VolumeMounts", container.Name)
+				changed = true
+			}
 
 			if !reflect.DeepEqual(ss.Spec.Template.Spec.Containers[i].Resources, existing.Spec.Template.Spec.Containers[i].Resources) {
 				existing.Spec.Template.Spec.Containers[i].Resources = ss.Spec.Template.Spec.Containers[i].Resources
@@ -473,33 +481,29 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoproj.ArgoCD) error {
 
 			if !reflect.DeepEqual(ss.Spec.Template.Spec.Containers[i].Env, existing.Spec.Template.Spec.Containers[i].Env) {
 				existing.Spec.Template.Spec.Containers[i].Env = ss.Spec.Template.Spec.Containers[i].Env
+				if changed {
+					explanation += ", "
+				}
+				explanation += fmt.Sprintf("container '%s' env", container.Name)
 				changed = true
 			}
 		}
-
-		if !reflect.DeepEqual(ss.Spec.Template.Spec.InitContainers[0].Resources, existing.Spec.Template.Spec.InitContainers[0].Resources) {
-			existing.Spec.Template.Spec.InitContainers[0].Resources = ss.Spec.Template.Spec.InitContainers[0].Resources
+		if !reflect.DeepEqual(ss.Spec.Template.Spec.Volumes, existing.Spec.Template.Spec.Volumes) {
+			existing.Spec.Template.Spec.Volumes = ss.Spec.Template.Spec.Volumes
 			if changed {
 				explanation += ", "
 			}
-			explanation += fmt.Sprintf("init container '%s' resources", existing.Spec.Template.Spec.InitContainers[0].Name)
+			explanation += "volumes"
 			changed = true
 		}
-
-		if !reflect.DeepEqual(ss.Spec.Template.Spec.InitContainers[0].SecurityContext, existing.Spec.Template.Spec.InitContainers[0].SecurityContext) {
-			existing.Spec.Template.Spec.InitContainers[0].SecurityContext = ss.Spec.Template.Spec.InitContainers[0].SecurityContext
+		if !reflect.DeepEqual(ss.Spec.Template.Spec.InitContainers, existing.Spec.Template.Spec.InitContainers) {
+			existing.Spec.Template.Spec.InitContainers = ss.Spec.Template.Spec.InitContainers
 			if changed {
 				explanation += ", "
 			}
-			explanation += fmt.Sprintf("init container '%s' security context", existing.Spec.Template.Spec.InitContainers[0].Name)
-			changed = true
-		}
-
-		if !reflect.DeepEqual(ss.Spec.Template.Spec.InitContainers[0].Env, existing.Spec.Template.Spec.InitContainers[0].Env) {
-			existing.Spec.Template.Spec.InitContainers[0].Env = ss.Spec.Template.Spec.InitContainers[0].Env
+			explanation += "init containers"
 			changed = true
 		}
-
 		if changed {
 			argoutil.LogResourceUpdate(log, existing, "updating", explanation)
 			return r.Client.Update(context.TODO(), existing)

diff --git a/controllers/argocd/statefulset_test.go b/controllers/argocd/statefulset_test.go
@@ -836,3 +836,96 @@ func TestReconcileArgoCD_sidecarcontainer(t *testing.T) {
 
 	assert.Equal(t, 1, len(ss.Spec.Template.Spec.Containers))
 }
+func TestReconcileArgoCD_reconcileRedisStatefulSet_ModifyContainerSpec(t *testing.T) {
+	logf.SetLogger(ZapLogger(true))
+
+	a := makeTestArgoCD()
+	a.Spec.HA.Enabled = true
+
+	resObjs := []client.Object{a}
+	subresObjs := []client.Object{a}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	// Initial reconciliation to create the StatefulSet
+	assert.NoError(t, r.reconcileRedisStatefulSet(a))
+
+	s := newStatefulSetWithSuffix("redis-ha-server", "redis", a)
+	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: s.Name, Namespace: a.Namespace}, s))
+
+	// Modify the container environment variable
+	s.Spec.Template.Spec.Containers[0].Env = append(s.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{
+		Name:  "NEW_ENV_VAR",
+		Value: "new-value",
+	})
+	assert.NoError(t, r.Client.Update(context.TODO(), s))
+
+	// Reconcile again and check if the environment variable is reverted
+	assert.NoError(t, r.reconcileRedisStatefulSet(a))
+	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: s.Name, Namespace: a.Namespace}, s))
+
+	envVarFound := false
+	for _, env := range s.Spec.Template.Spec.Containers[0].Env {
+		if env.Name == "NEW_ENV_VAR" {
+			envVarFound = true
+			break
+		}
+	}
+	assert.False(t, envVarFound, "NEW_ENV_VAR should not be present")
+
+	// Modify the initcontainer environment variable
+	s.Spec.Template.Spec.Containers[0].Env = append(s.Spec.Template.Spec.InitContainers[0].Env, corev1.EnvVar{
+		Name:  "NEW_ENV_VAR",
+		Value: "new-value",
+	})
+	assert.NoError(t, r.Client.Update(context.TODO(), s))
+
+	// Reconcile again and check if the environment variable is reverted
+	assert.NoError(t, r.reconcileRedisStatefulSet(a))
+	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: s.Name, Namespace: a.Namespace}, s))
+
+	envVarFound = false
+	for _, env := range s.Spec.Template.Spec.InitContainers[0].Env {
+		if env.Name == "NEW_ENV_VAR" {
+			envVarFound = true
+			break
+		}
+	}
+	assert.False(t, envVarFound, "NEW_ENV_VAR should not be present")
+
+	// Modify the container  volume and volume mount
+	s.Spec.Template.Spec.Containers[0].VolumeMounts = append(s.Spec.Template.Spec.Containers[0].VolumeMounts, corev1.VolumeMount{
+		Name:      "new-volume",
+		MountPath: "/new/path",
+	})
+	s.Spec.Template.Spec.Volumes = append(s.Spec.Template.Spec.Volumes, corev1.Volume{
+		Name: "new-volume",
+		VolumeSource: corev1.VolumeSource{
+			EmptyDir: &corev1.EmptyDirVolumeSource{},
+		},
+	})
+	assert.NoError(t, r.Client.Update(context.TODO(), s))
+	// Reconcile again and check if the volume mount is reverted
+	assert.NoError(t, r.reconcileRedisStatefulSet(a))
+	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: s.Name, Namespace: a.Namespace}, s))
+
+	volumeMountFound := false
+	for _, vm := range s.Spec.Template.Spec.Containers[0].VolumeMounts {
+		if vm.Name == "new-volume" {
+			volumeMountFound = true
+			break
+		}
+	}
+	assert.False(t, volumeMountFound, "new-volume should not be present in volume mounts")
+
+	volumeFound := false
+	for _, v := range s.Spec.Template.Spec.Volumes {
+		if v.Name == "new-volume" {
+			volumeFound = true
+			break
+		}
+	}
+	assert.False(t, volumeFound, "new-volume should not be present in volumes")
+}