Only update Argo CD Cluster secret if the values have changed

Signed-off-by: Jonathan West <[email protected]>

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
 	}

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)
+
+}

controllers/argocd/util.go

@@ -1344,32 +1344,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
 }
 

controllers/argocd/util_test.go

@@ -722,7 +722,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"]))
 
 }