From 5cce9cecd80c9c3d4d580deac9ac87f7451b9b9b Mon Sep 17 00:00:00 2001 From: Jonathan West Date: Wed, 29 Jan 2025 01:08:57 -0500 Subject: [PATCH 1/2] Only update Argo CD Cluster secret if the values have changed (#1648) Signed-off-by: Jonathan West --- controllers/argocd/secret.go | 147 ++++++++++++++++++++++-------- controllers/argocd/secret_test.go | 100 ++++++++++++++++++++ controllers/argocd/util.go | 67 +++++++++----- controllers/argocd/util_test.go | 2 +- 4 files changed, 254 insertions(+), 62 deletions(-) diff --git a/controllers/argocd/secret.go b/controllers/argocd/secret.go index 06d7cfa3d..b05d6ab3e 100644 --- a/controllers/argocd/secret.go +++ b/controllers/argocd/secret.go @@ -391,23 +391,14 @@ func (r *ReconcileArgoCD) reconcileGrafanaSecret(cr *argoproj.ArgoCD) error { return nil } -// reconcileClusterPermissionsSecret ensures ArgoCD instance is namespace-scoped -func (r *ReconcileArgoCD) reconcileClusterPermissionsSecret(cr *argoproj.ArgoCD) error { - var clusterConfigInstance bool - secret := argoutil.NewSecretWithSuffix(cr, "default-cluster-config") - secret.Labels[common.ArgoCDSecretTypeLabel] = "cluster" - dataBytes, _ := json.Marshal(map[string]interface{}{ - "tlsClientConfig": map[string]interface{}{ - "insecure": false, - }, - }) - +// generateSortedManagedNamespaceListForArgoCDCR return a list of namespaces with 'managed-by' label that are managed by this 'cr', and including the namespace containing 'cr' +func generateSortedManagedNamespaceListForArgoCDCR(cr *argoproj.ArgoCD, rClient client.Client) ([]string, error) { namespaceList := corev1.NamespaceList{} listOption := client.MatchingLabels{ common.ArgoCDManagedByLabel: cr.Namespace, } - if err := r.Client.List(context.TODO(), &namespaceList, listOption); err != nil { - return err + if err := rClient.List(context.TODO(), &namespaceList, listOption); err != nil { + return nil, err } var namespaces []string @@ -419,54 +410,132 @@ func (r *ReconcileArgoCD) reconcileClusterPermissionsSecret(cr *argoproj.ArgoCD) namespaces = append(namespaces, cr.Namespace) } sort.Strings(namespaces) + return namespaces, nil +} - secret.Data = map[string][]byte{ - "config": dataBytes, - "name": []byte("in-cluster"), - "server": []byte(common.ArgoCDDefaultServer), - "namespaces": []byte(strings.Join(namespaces, ",")), +// combineClusterSecretNamespacesWithManagedNamespaces will combine the contents of clusterSecret's .data.namespaces value, with the list of namespaces in 'managedNamespaceList', sorting them and removing duplicates. +func combineClusterSecretNamespacesWithManagedNamespaces(clusterSecret corev1.Secret, managedNamespaceList []string) string { + namespacesToManageMap := map[string]string{} + + for _, managedNamespace := range managedNamespaceList { + namespacesToManageMap[managedNamespace] = managedNamespace + } + + clusterSecretNamespaces := strings.Split(string(clusterSecret.Data["namespaces"]), ",") + for _, clusterSecretNS := range clusterSecretNamespaces { + ns := strings.TrimSpace(clusterSecretNS) + namespacesToManageMap[ns] = ns + } + + namespacesToManageList := []string{} + for namespaceToManage := range namespacesToManageMap { + namespacesToManageList = append(namespacesToManageList, namespaceToManage) } + sort.Strings(namespacesToManageList) + + namespacesToManageString := strings.Join(namespacesToManageList, ",") + + return namespacesToManageString + +} + +// reconcileClusterPermissionsSecret ensures ArgoCD instance is namespace-scoped +func (r *ReconcileArgoCD) reconcileClusterPermissionsSecret(cr *argoproj.ArgoCD) error { + + managedNamespaceList, err := generateSortedManagedNamespaceListForArgoCDCR(cr, r.Client) + if err != nil { + return err + } + + // isArgoCDAClusterConfigInstance indicates whether 'cr' is a cluster config instance (mentioned in ARGOCD_CLUSTER_CONFIG_NAMESPACES) + var isArgoCDAClusterConfigInstance bool if allowedNamespace(cr.Namespace, os.Getenv("ARGOCD_CLUSTER_CONFIG_NAMESPACES")) { - clusterConfigInstance = true + isArgoCDAClusterConfigInstance = true } + // Get all existing cluster secrets in the namespace clusterSecrets, err := r.getClusterSecrets(cr) if err != nil { return err } - for _, s := range clusterSecrets.Items { + // Find the cluster secret in the list that points to common.ArgoCDDefaultServer (default server address) + var localClusterSecret *corev1.Secret + for x, clusterSecret := range clusterSecrets.Items { + // check if cluster secret with default server address exists - if string(s.Data["server"]) == common.ArgoCDDefaultServer { - // if the cluster belongs to cluster config namespace, - // remove all namespaces from cluster secret, - // else update the list of namespaces if value differs. - var explanation string - if clusterConfigInstance { - delete(s.Data, "namespaces") + if string(clusterSecret.Data["server"]) == common.ArgoCDDefaultServer { + localClusterSecret = &clusterSecrets.Items[x] + } + } + + if localClusterSecret != nil { + + // If the default Cluster Secret already exists + + secretUpdateRequired := false + + // if the cluster belongs to cluster config namespace, + // remove all namespaces from cluster secret, + // else update the list of namespaces if value differs. + var explanation string + if isArgoCDAClusterConfigInstance { + + if _, exists := localClusterSecret.Data["namespaces"]; exists { + delete(localClusterSecret.Data, "namespaces") explanation = "removing namespaces from cluster secret" - } else { - ns := strings.Split(string(s.Data["namespaces"]), ",") - for _, n := range namespaces { - if !containsString(ns, strings.TrimSpace(n)) { - ns = append(ns, strings.TrimSpace(n)) - } - } - sort.Strings(ns) - s.Data["namespaces"] = []byte(strings.Join(ns, ",")) + secretUpdateRequired = true + } + + } else { + + namespacesToManageString := combineClusterSecretNamespacesWithManagedNamespaces(*localClusterSecret, managedNamespaceList) + + var existingNamespacesValue string + if localClusterSecret.Data["namespaces"] != nil { + existingNamespacesValue = string(localClusterSecret.Data["namespaces"]) + } + + if existingNamespacesValue != namespacesToManageString { + localClusterSecret.Data["namespaces"] = []byte(namespacesToManageString) explanation = "updating namespaces in cluster secret" + secretUpdateRequired = true } - argoutil.LogResourceUpdate(log, &s, explanation) - return r.Client.Update(context.TODO(), &s) } + + if secretUpdateRequired { + // We found the Secret, and the field needs to be updated + argoutil.LogResourceUpdate(log, localClusterSecret, explanation) + return r.Client.Update(context.TODO(), localClusterSecret) + } + + // We found the Secret, but the field hasn't changed: no update needed. + return nil } - if clusterConfigInstance { + // If ArgoCD is configured as a cluster-scoped, no need to create a Namespace containing managed namespaces + if isArgoCDAClusterConfigInstance { // do nothing return nil } + // Create the Secret, since we could not find it above + secret := argoutil.NewSecretWithSuffix(cr, "default-cluster-config") + secret.Labels[common.ArgoCDSecretTypeLabel] = "cluster" + dataBytes, _ := json.Marshal(map[string]interface{}{ + "tlsClientConfig": map[string]interface{}{ + "insecure": false, + }, + }) + + secret.Data = map[string][]byte{ + "config": dataBytes, + "name": []byte("in-cluster"), + "server": []byte(common.ArgoCDDefaultServer), + "namespaces": []byte(strings.Join(managedNamespaceList, ",")), + } + if err := controllerutil.SetControllerReference(cr, secret, r.Scheme); err != nil { return err } diff --git a/controllers/argocd/secret_test.go b/controllers/argocd/secret_test.go index 61834b98d..3218d5ab9 100644 --- a/controllers/argocd/secret_test.go +++ b/controllers/argocd/secret_test.go @@ -551,3 +551,103 @@ func Test_ReconcileArgoCD_ClusterPermissionsSecret(t *testing.T) { assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: testSecret.Name, Namespace: testSecret.Namespace}, testSecret)) assert.Nil(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: testSecret.Name, Namespace: testSecret.Namespace}, testSecret)) } + +func TestGenerateSortedManagedNamespaceListForArgoCDCR(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD() + + 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, createNamespace(r, a.Namespace, "")) + + // 1) The result of call should include both 'my-managed-namespace' and a.Namespace + managedByNamespace := corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-managed-namespace", + Labels: map[string]string{common.ArgoCDManagedByLabel: a.Namespace}, + }, + } + err := cl.Create(context.Background(), &managedByNamespace) + assert.NoError(t, err) + + res, err := generateSortedManagedNamespaceListForArgoCDCR(a, r.Client) + assert.NoError(t, err) + assert.Equal(t, res, []string{a.Namespace, managedByNamespace.Name}) + + // 2) Ensure that results returned by this function are sorted by namespace name + err = cl.Delete(context.Background(), &managedByNamespace) + assert.NoError(t, err) + + managedByNamespace = corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aaaa-first-when-sorted", + Labels: map[string]string{common.ArgoCDManagedByLabel: a.Namespace}, + }, + } + err = cl.Create(context.Background(), &managedByNamespace) + assert.NoError(t, err) + + res, err = generateSortedManagedNamespaceListForArgoCDCR(a, r.Client) + assert.NoError(t, err) + assert.Equal(t, res, []string{managedByNamespace.Name, a.Namespace}) +} + +func TestCombineClusterSecretNamespacesWithManagedNamespaces(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD() + + 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, createNamespace(r, a.Namespace, "")) + + // 1) Namespaces already listed in the cluster secret should be preserved + res := combineClusterSecretNamespacesWithManagedNamespaces(corev1.Secret{ + Data: map[string][]byte{ + "namespaces": ([]byte)("a,b"), + }, + }, []string{}) + assert.Equal(t, "a,b", res) + + // 2) Duplicates between managed namespaces and existing namespaces should be removed + res = combineClusterSecretNamespacesWithManagedNamespaces(corev1.Secret{ + Data: map[string][]byte{ + "namespaces": ([]byte)("a,b"), + }, + }, []string{"b", "c"}) + assert.Equal(t, "a,b,c", res) + + // 3) Namespace list should be fully sorted by name + res = combineClusterSecretNamespacesWithManagedNamespaces(corev1.Secret{ + Data: map[string][]byte{ + "namespaces": ([]byte)("b,a"), + }, + }, []string{"a", "d", "c", "b"}) + assert.Equal(t, "a,b,c,d", res) + + // 4) Remove duplicates in Secret + res = combineClusterSecretNamespacesWithManagedNamespaces(corev1.Secret{ + Data: map[string][]byte{ + "namespaces": ([]byte)("b,a,b,a"), + }, + }, []string{"c", "d"}) + assert.Equal(t, "a,b,c,d", res) + + // 5) Remove duplicates in string list + res = combineClusterSecretNamespacesWithManagedNamespaces(corev1.Secret{ + Data: map[string][]byte{ + "namespaces": ([]byte)("a,b"), + }, + }, []string{"c", "d", "e", "c", "d"}) + assert.Equal(t, "a,b,c,d,e", res) + +} diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index 1a1c5d1b6..572d3a5ce 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1340,32 +1340,55 @@ func deleteManagedNamespaceFromClusterSecret(ownerNS, sourceNS string, k8sClient log.Error(err, message) return fmt.Errorf("%s error: %w", message, err) } - for _, secret := range secrets.Items { - if string(secret.Data["server"]) != common.ArgoCDDefaultServer { + + // Find the cluster secret in the list that points to common.ArgoCDDefaultServer (default server address) + var localClusterSecret *corev1.Secret + for x, clusterSecret := range secrets.Items { + + // check if cluster secret with default server address exists + if string(clusterSecret.Data["server"]) == common.ArgoCDDefaultServer { + localClusterSecret = &secrets.Items[x] + } + } + + if localClusterSecret == nil { + // The Secret doesn't exist: no more work to do. + return nil + } + + // If the Secret doesn't even have a 'namespaces' field, we are done. + oldNamespacesFromClusterSecret, ok := localClusterSecret.Data["namespaces"] + if !ok { + return nil + } + + oldNamespaceListFromClusterSecret := strings.Split(string(oldNamespacesFromClusterSecret), ",") + var newNamespaceList []string + + for _, n := range oldNamespaceListFromClusterSecret { + // remove the namespace from the list of namespaces + if strings.TrimSpace(n) == sourceNS { continue } - if namespaces, ok := secret.Data["namespaces"]; ok { - namespaceList := strings.Split(string(namespaces), ",") - var result []string - - for _, n := range namespaceList { - // remove the namespace from the list of namespaces - if strings.TrimSpace(n) == sourceNS { - continue - } - result = append(result, strings.TrimSpace(n)) - sort.Strings(result) - secret.Data["namespaces"] = []byte(strings.Join(result, ",")) - } - // Update the secret with the updated list of namespaces - argoutil.LogResourceUpdate(log, &secret, "removing managed namespace", sourceNS) - if _, err = k8sClient.CoreV1().Secrets(ownerNS).Update(context.TODO(), &secret, metav1.UpdateOptions{}); err != nil { - message := fmt.Sprintf("failed to update cluster permission secret for namespace: %s", ownerNS) - log.Error(err, message) - return fmt.Errorf("%s error: %w", message, err) - } + newNamespaceList = append(newNamespaceList, strings.TrimSpace(n)) + } + sort.Strings(newNamespaceList) + newNamespaceListString := strings.Join(newNamespaceList, ",") + + // If the namespace list has changed, update the cluster secret + if string(oldNamespacesFromClusterSecret) != newNamespaceListString { + + localClusterSecret.Data["namespaces"] = []byte(newNamespaceListString) + + // Update the secret with the updated list of namespaces + argoutil.LogResourceUpdate(log, localClusterSecret, "removing managed namespace", sourceNS) + if _, err = k8sClient.CoreV1().Secrets(ownerNS).Update(context.TODO(), localClusterSecret, metav1.UpdateOptions{}); err != nil { + message := fmt.Sprintf("failed to update cluster permission secret for namespace: %s", ownerNS) + log.Error(err, message) + return fmt.Errorf("%s error: %w", message, err) } } + return nil } diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go index 4502fb2e6..a67560afb 100644 --- a/controllers/argocd/util_test.go +++ b/controllers/argocd/util_test.go @@ -727,7 +727,7 @@ func TestRemoveManagedNamespaceFromClusterSecretAfterDeletion(t *testing.T) { // secret should still exists with updated list of namespaces s, err := testClient.CoreV1().Secrets(a.Namespace).Get(context.TODO(), secret.Name, metav1.GetOptions{}) assert.NoError(t, err) - assert.Equal(t, string(s.Data["namespaces"]), "testNamespace2") + assert.Equal(t, "testNamespace2", string(s.Data["namespaces"])) } From 8210187a3743e5d4c3fa68e526d43e0d9d8f760c Mon Sep 17 00:00:00 2001 From: Mangaal Meetei Date: Tue, 4 Feb 2025 10:05:36 +0530 Subject: [PATCH 2/2] Fix issue with ArgoCD Redis HA components not updating during operator upgrades (#1652) * add change check for redis ha proxy deployment Signed-off-by: Mangaal * add change check for redis ha server deployment Signed-off-by: Mangaal * add unit test Signed-off-by: Mangaal * Remove unnecessary check Signed-off-by: Mangaal * Remove unnecessary check in unit test Signed-off-by: Mangaal * add a single test for initcontainer Signed-off-by: Mangaal --------- Signed-off-by: Mangaal --- controllers/argocd/deployment.go | 47 +++++++---- controllers/argocd/deployment_test.go | 112 +++++++++++++++++++++++++ controllers/argocd/statefulset.go | 32 +++---- controllers/argocd/statefulset_test.go | 93 ++++++++++++++++++++ 4 files changed, 253 insertions(+), 31 deletions(-) diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go index 71f031f3f..52ba1f800 100644 --- 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 index aa117cd99..f2a83a3e5 100644 --- 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 index 205b57ba1..b70218c53 100644 --- 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 index 58b2b4426..e80502f07 100644 --- 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") +}