chore: cleanup reconcilation of roles, role bindings, service accounts

controllers/argocd/hooks_test.go

@@ -9,6 +9,7 @@ import (
 	v1 "k8s.io/api/rbac/v1"
 
 	argoprojv1alpha1 "github.com/argoproj-labs/argocd-operator/api/v1alpha1"
+	"github.com/argoproj-labs/argocd-operator/common"
 )
 
 var errMsg = errors.New("this is a test error")
@@ -33,7 +34,7 @@ func testClusterRoleHook(cr *argoprojv1alpha1.ArgoCD, v interface{}, s string) e
 func testRoleHook(cr *argoprojv1alpha1.ArgoCD, v interface{}, s string) error {
 	switch o := v.(type) {
 	case *v1.Role:
-		if o.Name == cr.Name+"-"+applicationController {
+		if o.Name == cr.Name+"-"+common.ArgoCDApplicationControllerComponent {
 			o.Rules = append(o.Rules, testRules()...)
 		}
 	}

controllers/argocd/policyrule.go

@@ -2,6 +2,8 @@ package argocd
 
 import (
 	v1 "k8s.io/api/rbac/v1"
+
+	"github.com/argoproj-labs/argocd-operator/common"
 )
 
 func policyRuleForApplicationController() []v1.PolicyRule {
@@ -173,3 +175,45 @@ func policyRuleForServerClusterRole() []v1.PolicyRule {
 		},
 	}
 }
+
+func getPolicyRuleList() []struct {
+	name       string
+	policyRule []v1.PolicyRule
+} {
+	return []struct {
+		name       string
+		policyRule []v1.PolicyRule
+	}{
+		{
+			name:       common.ArgoCDApplicationControllerComponent,
+			policyRule: policyRuleForApplicationController(),
+		}, {
+			name:       common.ArgoCDDexServerComponent,
+			policyRule: policyRuleForDexServer(),
+		}, {
+			name:       common.ArgoCDServerComponent,
+			policyRule: policyRuleForServer(),
+		}, {
+			name:       common.ArgoCDRedisHAComponent,
+			policyRule: policyRuleForRedisHa(),
+		},
+	}
+}
+
+func getPolicyRuleClusterRoleList() []struct {
+	name       string
+	policyRule []v1.PolicyRule
+} {
+	return []struct {
+		name       string
+		policyRule []v1.PolicyRule
+	}{
+		{
+			name:       common.ArgoCDApplicationControllerComponent,
+			policyRule: policyRuleForApplicationController(),
+		}, {
+			name:       common.ArgoCDServerComponent,
+			policyRule: policyRuleForServerClusterRole(),
+		},
+	}
+}

controllers/argocd/role.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"reflect"
 
 	v1 "k8s.io/api/rbac/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -13,16 +14,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	argoprojv1a1 "github.com/argoproj-labs/argocd-operator/api/v1alpha1"
+	"github.com/argoproj-labs/argocd-operator/common"
 	"github.com/argoproj-labs/argocd-operator/controllers/argoutil"
 )
 
-const (
-	applicationController = "argocd-application-controller"
-	server                = "argocd-server"
-	redisHa               = "argocd-redis-ha"
-	dexServer             = "argocd-dex-server"
-)
-
 // newRole returns a new Role instance.
 func newRole(name string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) *v1.Role {
 	return &v1.Role{
@@ -57,28 +52,20 @@ func newClusterRole(name string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD)
 
 // reconcileRoles will ensure that all ArgoCD Service Accounts are configured.
 func (r *ReconcileArgoCD) reconcileRoles(cr *argoprojv1a1.ArgoCD) error {
-	if _, err := r.reconcileRole(applicationController, policyRuleForApplicationController(), cr); err != nil {
-		return err
-	}
-
-	if _, err := r.reconcileRole(dexServer, policyRuleForDexServer(), cr); err != nil {
-		return err
-	}
+	params := getPolicyRuleList()
 
-	if _, err := r.reconcileRole(server, policyRuleForServer(), cr); err != nil {
-		return err
-	}
-
-	if _, err := r.reconcileRole(redisHa, policyRuleForRedisHa(), cr); err != nil {
-		return err
+	for _, param := range params {
+		if _, err := r.reconcileRole(param.name, param.policyRule, cr); err != nil {
+			return err
+		}
 	}
 
-	if _, err := r.reconcileClusterRole(applicationController, policyRuleForApplicationController(), cr); err != nil {
-		return err
-	}
+	clusterParams := getPolicyRuleClusterRoleList()
 
-	if _, err := r.reconcileClusterRole(server, policyRuleForServerClusterRole(), cr); err != nil {
-		return err
+	for _, clusterParam := range clusterParams {
+		if _, err := r.reconcileRole(clusterParam.name, clusterParam.policyRule, cr); err != nil {
+			return err
+		}
 	}
 
 	return nil
@@ -92,7 +79,7 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule
 	// create policy rules for each namespace
 	for _, namespace := range r.ManagedNamespaces.Items {
 		// only create dexServer and redisHa roles for the namespace where the argocd instance is deployed
-		if cr.ObjectMeta.Namespace != namespace.Name && (name == dexServer || name == redisHa) {
+		if cr.ObjectMeta.Namespace != namespace.Name && (name == common.ArgoCDDexServerComponent || name == common.ArgoCDRedisHAComponent) {
 			break
 		}
 		customRole := getCustomRoleName(name)
@@ -111,7 +98,7 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule
 				continue // skip creating default role if custom cluster role is provided
 			}
 			roles = append(roles, role)
-			if name == dexServer && isDexDisabled() {
+			if name == common.ArgoCDDexServerComponent && isDexDisabled() {
 				continue // Dex is disabled, do nothing
 			}
 
@@ -127,24 +114,21 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule
 			continue
 		}
 
-		if customRole != "" {
-			// Delete the existing default role if custom role is specified
+		// Delete the existing default role if custom role is specified
+		// or if there is an existing Role created for Dex
+		if customRole != "" || (name == common.ArgoCDDexServerComponent && isDexDisabled()) {
 			if err := r.Client.Delete(context.TODO(), &existingRole); err != nil {
 				return nil, err
 			}
 			continue
 		}
 
-		if name == dexServer && isDexDisabled() {
-			// Delete any existing Role created for Dex
-			if err := r.Client.Delete(context.TODO(), &existingRole); err != nil {
+		// if the Rules differ, update the Role
+		if !reflect.DeepEqual(existingRole.Rules, role.Rules) {
+			existingRole.Rules = role.Rules
+			if err := r.Client.Update(context.TODO(), &existingRole); err != nil {
 				return nil, err
 			}
-			continue
-		}
-		existingRole.Rules = role.Rules
-		if err := r.Client.Update(context.TODO(), &existingRole); err != nil {
-			return nil, err
 		}
 		roles = append(roles, &existingRole)
 	}
@@ -178,8 +162,14 @@ func (r *ReconcileArgoCD) reconcileClusterRole(name string, policyRules []v1.Pol
 		return nil, r.Client.Delete(context.TODO(), existingClusterRole)
 	}
 
-	existingClusterRole.Rules = clusterRole.Rules
-	return existingClusterRole, r.Client.Update(context.TODO(), existingClusterRole)
+	// if the Rules differ, update the Role
+	if !reflect.DeepEqual(existingClusterRole.Rules, clusterRole.Rules) {
+		existingClusterRole.Rules = clusterRole.Rules
+		if err := r.Client.Update(context.TODO(), existingClusterRole); err != nil {
+			return nil, err
+		}
+	}
+	return existingClusterRole, nil
 }
 
 func deleteClusterRoles(c client.Client, clusterRoleList *v1.ClusterRoleList) error {

controllers/argocd/role_test.go

@@ -57,15 +57,15 @@ func TestReconcileArgoCD_reconcileRole_for_new_namespace(t *testing.T) {
 	// only 1 role for the Argo CD instance namespace will be created
 	expectedNumberOfRoles := 1
 	// check no dexServer role is created for the new namespace with managed-by label
-	workloadIdentifier := dexServer
+	workloadIdentifier := common.ArgoCDDexServerComponent
 	expectedRoleNamespace := a.Namespace
 	expectedDexServerRules := policyRuleForDexServer()
 	dexRoles, err := r.reconcileRole(workloadIdentifier, expectedDexServerRules, a)
 	assert.NoError(t, err)
 	assert.Equal(t, expectedNumberOfRoles, len(dexRoles))
 	assert.Equal(t, expectedRoleNamespace, dexRoles[0].ObjectMeta.Namespace)
 	// check no redisHa role is created for the new namespace with managed-by label
-	workloadIdentifier = redisHa
+	workloadIdentifier = common.ArgoCDRedisHAComponent
 	expectedRedisHaRules := policyRuleForRedisHa()
 	redisHaRoles, err := r.reconcileRole(workloadIdentifier, expectedRedisHaRules, a)
 	assert.NoError(t, err)
@@ -80,10 +80,10 @@ func TestReconcileArgoCD_reconcileRole_dex_disabled(t *testing.T) {
 	assert.NoError(t, createNamespace(r, a.Namespace, ""))
 
 	rules := policyRuleForDexServer()
-	role := newRole(dexServer, rules, a)
+	role := newRole(common.ArgoCDDexServerComponent, rules, a)
 
 	// Dex is enabled
-	_, err := r.reconcileRole(dexServer, rules, a)
+	_, err := r.reconcileRole(common.ArgoCDDexServerComponent, rules, a)
 	assert.NoError(t, err)
 	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: a.Namespace}, role))
 	assert.Equal(t, rules, role.Rules)
@@ -92,7 +92,7 @@ func TestReconcileArgoCD_reconcileRole_dex_disabled(t *testing.T) {
 	os.Setenv("DISABLE_DEX", "true")
 	defer os.Unsetenv("DISABLE_DEX")
 
-	_, err = r.reconcileRole(dexServer, rules, a)
+	_, err = r.reconcileRole(common.ArgoCDDexServerComponent, rules, a)
 	assert.NoError(t, err)
 	//assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: a.Namespace}, role), "not found")
 	//TODO: https://github.com/stretchr/testify/pull/1022 introduced ErrorContains, but is not yet available in a tagged release. Revert to ErrorContains once this becomes available
@@ -153,7 +153,7 @@ func TestReconcileArgoCD_RoleHooks(t *testing.T) {
 	assert.NoError(t, createNamespace(r, a.Namespace, ""))
 	Register(testRoleHook)
 
-	roles, err := r.reconcileRole(applicationController, []v1.PolicyRule{}, a)
+	roles, err := r.reconcileRole(common.ArgoCDApplicationControllerComponent, []v1.PolicyRule{}, a)
 	role := roles[0]
 	assert.NoError(t, err)
 	assert.Equal(t, role.Rules, testRules())

controllers/argocd/rolebinding.go

@@ -68,20 +68,15 @@ func newRoleBindingWithname(name string, cr *argoprojv1a1.ArgoCD) *v1.RoleBindin
 
 // reconcileRoleBindings will ensure that all ArgoCD RoleBindings are configured.
 func (r *ReconcileArgoCD) reconcileRoleBindings(cr *argoprojv1a1.ArgoCD) error {
-	if err := r.reconcileRoleBinding(applicationController, policyRuleForApplicationController(), cr); err != nil {
-		return fmt.Errorf("error reconciling roleBinding for %q: %w", applicationController, err)
-	}
-	if err := r.reconcileRoleBinding(dexServer, policyRuleForDexServer(), cr); err != nil {
-		return fmt.Errorf("error reconciling roleBinding for %q: %w", dexServer, err)
-	}
 
-	if err := r.reconcileRoleBinding(redisHa, policyRuleForRedisHa(), cr); err != nil {
-		return fmt.Errorf("error reconciling roleBinding for %q: %w", redisHa, err)
-	}
+	params := getPolicyRuleList()
 
-	if err := r.reconcileRoleBinding(server, policyRuleForServer(), cr); err != nil {
-		return fmt.Errorf("error reconciling roleBinding for %q: %w", server, err)
+	for _, param := range params {
+		if err := r.reconcileRoleBinding(param.name, param.policyRule, cr); err != nil {
+			return fmt.Errorf("error reconciling roleBinding for %q: %w", param.name, err)
+		}
 	}
+
 	return nil
 }
 
@@ -101,7 +96,7 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
 
 	for _, namespace := range r.ManagedNamespaces.Items {
 		// only create dexServer and redisHa rolebindings for the namespace where the argocd instance is deployed
-		if cr.ObjectMeta.Namespace != namespace.Name && (name == dexServer || name == redisHa) {
+		if cr.ObjectMeta.Namespace != namespace.Name && (name == common.ArgoCDDexServerComponent || name == common.ArgoCDRedisHAComponent) {
 			break
 		}
 		// get expected name
@@ -116,7 +111,7 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
 			if !errors.IsNotFound(err) {
 				return fmt.Errorf("failed to get the rolebinding associated with %s : %s", name, err)
 			}
-			if name == dexServer && isDexDisabled() {
+			if name == common.ArgoCDDexServerComponent && isDexDisabled() {
 				continue // Dex is disabled, do nothing
 			}
 			roleBindingExists = false
@@ -146,7 +141,7 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
 		}
 
 		if roleBindingExists {
-			if name == dexServer && isDexDisabled() {
+			if name == common.ArgoCDDexServerComponent && isDexDisabled() {
 				// Delete any existing RoleBinding created for Dex
 				if err = r.Client.Delete(context.TODO(), existingRoleBinding); err != nil {
 					return err
@@ -160,9 +155,12 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
 					return err
 				}
 			} else {
-				existingRoleBinding.Subjects = roleBinding.Subjects
-				if err = r.Client.Update(context.TODO(), existingRoleBinding); err != nil {
-					return err
+				// if the Subjects differ, update the role bindings
+				if !reflect.DeepEqual(roleBinding.Subjects, existingRoleBinding.Subjects) {
+					existingRoleBinding.Subjects = roleBinding.Subjects
+					if err = r.Client.Update(context.TODO(), existingRoleBinding); err != nil {
+						return err
+					}
 				}
 				continue
 			}
@@ -182,10 +180,10 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
 }
 
 func getCustomRoleName(name string) string {
-	if name == applicationController {
+	if name == common.ArgoCDApplicationControllerComponent {
 		return os.Getenv(common.ArgoCDControllerClusterRoleEnvName)
 	}
-	if name == server {
+	if name == common.ArgoCDServerComponent {
 		return os.Getenv(common.ArgoCDServerClusterRoleEnvName)
 	}
 	return ""

controllers/argocd/rolebinding_test.go

@@ -54,14 +54,14 @@ func TestReconcileArgoCD_reconcileRoleBinding_for_new_namespace(t *testing.T) {
 
 	// check no dexServer rolebinding is created for the new namespace with managed-by label
 	roleBinding := &rbacv1.RoleBinding{}
-	workloadIdentifier := dexServer
+	workloadIdentifier := common.ArgoCDDexServerComponent
 	expectedDexServerRules := policyRuleForDexServer()
 	expectedName := fmt.Sprintf("%s-%s", a.Name, workloadIdentifier)
 	assert.NoError(t, r.reconcileRoleBinding(workloadIdentifier, expectedDexServerRules, a))
 	assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: "newTestNamespace"}, roleBinding))
 
 	// check no redisHa rolebinding is created for the new namespace with managed-by label
-	workloadIdentifier = redisHa
+	workloadIdentifier = common.ArgoCDRedisHAComponent
 	expectedRedisHaRules := policyRuleForRedisHa()
 	assert.NoError(t, r.reconcileRoleBinding(workloadIdentifier, expectedRedisHaRules, a))
 	assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: "newTestNamespace"}, roleBinding))
@@ -74,19 +74,19 @@ func TestReconcileArgoCD_reconcileRoleBinding_dex_disabled(t *testing.T) {
 	assert.NoError(t, createNamespace(r, a.Namespace, ""))
 
 	rules := policyRuleForDexServer()
-	rb := newRoleBindingWithname(dexServer, a)
+	rb := newRoleBindingWithname(common.ArgoCDDexServerComponent, a)
 
 	// Dex is enabled, creates a role binding
-	assert.NoError(t, r.reconcileRoleBinding(dexServer, rules, a))
+	assert.NoError(t, r.reconcileRoleBinding(common.ArgoCDDexServerComponent, rules, a))
 	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: rb.Name, Namespace: a.Namespace}, rb))
 
 	// Disable Dex, deletes the existing role binding
 	os.Setenv("DISABLE_DEX", "true")
 	defer os.Unsetenv("DISABLE_DEX")
 
-	_, err := r.reconcileRole(dexServer, rules, a)
+	_, err := r.reconcileRole(common.ArgoCDDexServerComponent, rules, a)
 	assert.NoError(t, err)
-	assert.NoError(t, r.reconcileRoleBinding(dexServer, rules, a))
+	assert.NoError(t, r.reconcileRoleBinding(common.ArgoCDDexServerComponent, rules, a))
 	//assert.ErrorContains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: rb.Name, Namespace: a.Namespace}, rb), "not found")
 	//TODO: https://github.com/stretchr/testify/pull/1022 introduced ErrorContains, but is not yet available in a tagged release. Revert to ErrorContains once this becomes available
 	assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: rb.Name, Namespace: a.Namespace}, rb))
@@ -156,7 +156,7 @@ func TestReconcileArgoCD_reconcileRoleBinding_custom_role(t *testing.T) {
 
 	assert.NoError(t, os.Setenv(common.ArgoCDControllerClusterRoleEnvName, "custom-controller-role"))
 	defer os.Unsetenv(common.ArgoCDControllerClusterRoleEnvName)
-	assert.NoError(t, r.reconcileRoleBinding(applicationController, p, a))
+	assert.NoError(t, r.reconcileRoleBinding(common.ArgoCDApplicationControllerComponent, p, a))
 
 	expectedName = fmt.Sprintf("%s-%s", a.Name, "argocd-application-controller")
 	checkForUpdatedRoleRef(t, "custom-controller-role", expectedName)