fix: Ensure ConfigMap and StatefulSet updates are applied during oper…

…ator upgrades (#1619) * add missing check for env Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * add missing check for configmap data Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * add LogResourceCreation Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * add update log Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * Trigger test: empty commit Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * add unit test Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * add missing logs Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> * update comment Signed-off-by: Mangaal <angommeeteimangaal@gmail.com> --------- Signed-off-by: Mangaal <angommeeteimangaal@gmail.com>
argoproj-labs · Jan 9, 2025 · 5049cdf · 5049cdf
1 parent 66f463b
commit 5049cdf
Show file tree

Hide file tree

Showing 3 changed files with 129 additions and 26 deletions.
diff --git a/controllers/argocd/configmap.go b/controllers/argocd/configmap.go
@@ -651,48 +651,43 @@ func (r *ReconcileArgoCD) reconcileRedisConfiguration(cr *argoproj.ArgoCD, useTL
 // reconcileRedisHAConfigMap will ensure that the Redis HA Health ConfigMap is present for the given ArgoCD.
 func (r *ReconcileArgoCD) reconcileRedisHAHealthConfigMap(cr *argoproj.ArgoCD, useTLSForRedis bool) error {
 	cm := newConfigMapWithName(common.ArgoCDRedisHAHealthConfigMapName, cr)
-	if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
-		if !cr.Spec.HA.Enabled {
-			// ConfigMap exists but HA enabled flag has been set to false, delete the ConfigMap
-			argoutil.LogResourceDeletion(log, cm, "redis ha is disabled")
-			return r.Client.Delete(context.TODO(), cm)
-		}
-		return nil // ConfigMap found with nothing changed, move along...
-	}
-
-	if !cr.Spec.HA.Enabled {
-		return nil // HA not enabled, do nothing.
-	}
-
 	cm.Data = map[string]string{
 		"redis_liveness.sh":    getRedisLivenessScript(useTLSForRedis),
 		"redis_readiness.sh":   getRedisReadinessScript(useTLSForRedis),
 		"sentinel_liveness.sh": getSentinelLivenessScript(useTLSForRedis),
 	}
+	if !cr.Spec.HA.Enabled {
+		existingCM := &corev1.ConfigMap{}
+		if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
+			// ConfigMap exists but HA enabled flag has been set to false, delete the ConfigMap
+			argoutil.LogResourceDeletion(log, cm, "redis ha is disabled")
+			return r.Client.Delete(context.TODO(), existingCM)
+		}
+		return nil // Nothing to do since HA is not enabled and ConfigMap does not exist
+	}
 
 	if err := controllerutil.SetControllerReference(cr, cm, r.Scheme); err != nil {
 		return err
 	}
+
+	existingCM := &corev1.ConfigMap{}
+	if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
+		// Check if the data has changed
+		if !reflect.DeepEqual(cm.Data, existingCM.Data) {
+			existingCM.Data = cm.Data
+			argoutil.LogResourceUpdate(log, existingCM, "updating", "Redis HA Health ConfigMap")
+			return r.Client.Update(context.TODO(), existingCM)
+		}
+		return nil // No changes detected
+	}
 	argoutil.LogResourceCreation(log, cm)
 	return r.Client.Create(context.TODO(), cm)
 }
 
 // reconcileRedisHAConfigMap will ensure that the Redis HA ConfigMap is present for the given ArgoCD.
 func (r *ReconcileArgoCD) reconcileRedisHAConfigMap(cr *argoproj.ArgoCD, useTLSForRedis bool) error {
+	// Create or update the ConfigMap if HA is enabled
 	cm := newConfigMapWithName(common.ArgoCDRedisHAConfigMapName, cr)
-	if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, cm) {
-		if !cr.Spec.HA.Enabled {
-			// ConfigMap exists but HA enabled flag has been set to false, delete the ConfigMap
-			argoutil.LogResourceDeletion(log, cm, "redis ha is disabled")
-			return r.Client.Delete(context.TODO(), cm)
-		}
-		return nil // ConfigMap found with nothing changed, move along...
-	}
-
-	if !cr.Spec.HA.Enabled {
-		return nil // HA not enabled, do nothing.
-	}
-
 	cm.Data = map[string]string{
 		"haproxy.cfg":     getRedisHAProxyConfig(cr, useTLSForRedis),
 		"haproxy_init.sh": getRedisHAProxyScript(cr),
@@ -701,10 +696,34 @@ func (r *ReconcileArgoCD) reconcileRedisHAConfigMap(cr *argoproj.ArgoCD, useTLSF
 		"sentinel.conf":   getRedisSentinelConf(useTLSForRedis),
 	}
 
+	if !cr.Spec.HA.Enabled {
+
+		existingCM := &corev1.ConfigMap{}
+		if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
+			// ConfigMap exists but HA enabled flag has been set to false, delete the ConfigMap
+			argoutil.LogResourceDeletion(log, cm, "redis ha is disabled")
+			return r.Client.Delete(context.TODO(), existingCM)
+		}
+		return nil // Nothing to do since HA is not enabled and ConfigMap does not exist
+	}
+
+	// Set the ownership reference
 	if err := controllerutil.SetControllerReference(cr, cm, r.Scheme); err != nil {
 		return err
 	}
+
+	existingCM := &corev1.ConfigMap{}
+	if argoutil.IsObjectFound(r.Client, cr.Namespace, cm.Name, existingCM) {
+		// Check if the data has changed
+		if !reflect.DeepEqual(cm.Data, existingCM.Data) {
+			existingCM.Data = cm.Data
+			argoutil.LogResourceUpdate(log, existingCM, "updating", "Redis HA ConfigMap")
+			return r.Client.Update(context.TODO(), existingCM)
+		}
+		return nil // No changes detected
+	}
 	argoutil.LogResourceCreation(log, cm)
+	// Create the ConfigMap if it does not exist
 	return r.Client.Create(context.TODO(), cm)
 }
 

diff --git a/controllers/argocd/configmap_test.go b/controllers/argocd/configmap_test.go
@@ -110,6 +110,80 @@ func TestReconcileArgoCD_reconcileTLSCerts_configMapUpdate(t *testing.T) {
 	}
 }
 
+// TestReconcileArgoCD_reconcileRedisHAHealthConfigMap tests the reconcileRedisHAHealthConfigMap function.
+func TestReconcileArgoCD_reconcileRedisHAHealthConfigMap(t *testing.T) {
+	logf.SetLogger(ZapLogger(true))
+
+	// Create a test ArgoCD resource with HA enabled
+	cr := makeTestArgoCD()
+	cr.Spec.HA.Enabled = true
+
+	// Initialize test objects
+	resObjs := []client.Object{cr}
+	subresObjs := []client.Object{}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	// Perform initial reconciliation
+	assert.NoError(t, r.reconcileRedisHAHealthConfigMap(cr, false))
+
+	// Modify ConfigMap data to simulate external changes
+	existingCM := &corev1.ConfigMap{}
+	assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCM))
+	existingCM.Data["redis_liveness.sh"] = "modified_script_content"
+	assert.NoError(t, cl.Update(context.TODO(), existingCM))
+
+	// Reconcile again and verify changes are reverted
+	assert.NoError(t, r.reconcileRedisHAHealthConfigMap(cr, false))
+	existingCMAfter := &corev1.ConfigMap{}
+	assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCMAfter))
+	assert.Equal(t, getRedisLivenessScript(false), existingCMAfter.Data["redis_liveness.sh"])
+
+	// Disable HA and ensure ConfigMap is deleted
+	cr.Spec.HA.Enabled = false
+	assert.NoError(t, r.reconcileRedisHAHealthConfigMap(cr, false))
+	assert.False(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAHealthConfigMapName, existingCM))
+}
+
+// TestReconcileArgoCD_reconcileRedisHAConfigMap tests the reconcileRedisHAConfigMap function.
+func TestReconcileArgoCD_reconcileRedisHAConfigMap(t *testing.T) {
+	logf.SetLogger(ZapLogger(true))
+
+	// Create a test ArgoCD resource with HA enabled
+	cr := makeTestArgoCD()
+	cr.Spec.HA.Enabled = true
+
+	// Initialize test objects
+	resObjs := []client.Object{cr}
+	subresObjs := []client.Object{}
+	runtimeObjs := []runtime.Object{}
+	sch := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs)
+	r := makeTestReconciler(cl, sch)
+
+	// Perform initial reconciliation
+	assert.NoError(t, r.reconcileRedisHAConfigMap(cr, false))
+
+	// Modify ConfigMap data to simulate external changes
+	existingCM := &corev1.ConfigMap{}
+	assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCM))
+	existingCM.Data["haproxy.cfg"] = "modified_config_content"
+	assert.NoError(t, cl.Update(context.TODO(), existingCM))
+
+	// Reconcile again and verify changes are reverted
+	assert.NoError(t, r.reconcileRedisHAConfigMap(cr, false))
+	existingCMAfter := &corev1.ConfigMap{}
+	assert.True(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCMAfter))
+	assert.Equal(t, getRedisHAProxyConfig(cr, false), existingCMAfter.Data["haproxy.cfg"])
+
+	// Disable HA and ensure ConfigMap is deleted
+	cr.Spec.HA.Enabled = false
+	assert.NoError(t, r.reconcileRedisHAConfigMap(cr, false))
+	assert.False(t, argoutil.IsObjectFound(cl, cr.Namespace, common.ArgoCDRedisHAConfigMapName, existingCM))
+}
+
 func TestReconcileArgoCD_reconcileTLSCerts_withInitialCertsUpdate(t *testing.T) {
 	logf.SetLogger(ZapLogger(true))
 	a := makeTestArgoCD()

diff --git a/controllers/argocd/statefulset.go b/controllers/argocd/statefulset.go
@@ -470,6 +470,11 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoproj.ArgoCD) error {
 				explanation += fmt.Sprintf("container '%s' security context", container.Name)
 				changed = true
 			}
+
+			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
+				changed = true
+			}
 		}
 
 		if !reflect.DeepEqual(ss.Spec.Template.Spec.InitContainers[0].Resources, existing.Spec.Template.Spec.InitContainers[0].Resources) {
@@ -490,6 +495,11 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoproj.ArgoCD) error {
 			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
+			changed = true
+		}
+
 		if changed {
 			argoutil.LogResourceUpdate(log, existing, "updating", explanation)
 			return r.Client.Update(context.TODO(), existing)