[WIP] Updated logic for role/rolebinding reconciliation

controllers/argocd/argocd_controller.go

@@ -102,7 +102,7 @@ func (r *ReconcileArgoCD) Reconcile(ctx context.Context, request ctrl.Request) (
 				}
 			}
 
-			if err := r.removeUnmanagedSourceNamespaceResources(argocd); err != nil {
+			if err := r.removeUnmanagedSourceNamespaceResources(ctx, argocd); err != nil {
 				return reconcile.Result{}, fmt.Errorf("failed to remove resources from sourceNamespaces, error: %w", err)
 			}
 
@@ -112,9 +112,7 @@ func (r *ReconcileArgoCD) Reconcile(ctx context.Context, request ctrl.Request) (
 
 			// remove namespace of deleted Argo CD instance from deprecationEventEmissionTracker (if exists) so that if another instance
 			// is created in the same namespace in the future, that instance is appropriately tracked
-			if _, ok := DeprecationEventEmissionTracker[argocd.Namespace]; ok {
-				delete(DeprecationEventEmissionTracker, argocd.Namespace)
-			}
+			delete(DeprecationEventEmissionTracker, argocd.Namespace)
 		}
 		return reconcile.Result{}, nil
 	}
@@ -138,7 +136,7 @@ func (r *ReconcileArgoCD) Reconcile(ctx context.Context, request ctrl.Request) (
 		return reconcile.Result{}, err
 	}
 
-	if err := r.reconcileResources(argocd); err != nil {
+	if err := r.reconcileResources(ctx, argocd); err != nil {
 		// Error reconciling ArgoCD sub-resources - requeue the request.
 		return reconcile.Result{}, err
 	}

controllers/argocd/dex.go

@@ -411,7 +411,7 @@ func (r *ReconcileArgoCD) reconcileDexService(cr *argoprojv1a1.ArgoCD) error {
 // and deletion of dex resources based on the specified configuration of dex
 func (r *ReconcileArgoCD) reconcileDexResources(cr *argoprojv1a1.ArgoCD) error {
 
-	if _, err := r.reconcileRole(common.ArgoCDDexServerComponent, policyRuleForDexServer(), cr); err != nil {
+	if err := r.reconcileRole(common.ArgoCDDexServerComponent, policyRuleForDexServer(), cr); err != nil {
 		log.Error(err, "error reconciling dex role")
 	}
 

controllers/argocd/dex_test.go

@@ -934,7 +934,7 @@ func TestReconcileArgoCD_reconcileRole_dex_disabled(t *testing.T) {
 				test.setEnvFunc(t, "false")
 			}
 
-			_, err := r.reconcileRole(common.ArgoCDDexServerComponent, rules, test.argoCD)
+			err := r.reconcileRole(common.ArgoCDDexServerComponent, rules, test.argoCD)
 			assert.NoError(t, err)
 
 			// ensure role was created correctly
@@ -948,7 +948,7 @@ func TestReconcileArgoCD_reconcileRole_dex_disabled(t *testing.T) {
 				test.updateCrFunc(test.argoCD)
 			}
 
-			_, err = r.reconcileRole(common.ArgoCDDexServerComponent, rules, test.argoCD)
+			err = r.reconcileRole(common.ArgoCDDexServerComponent, rules, test.argoCD)
 			assert.NoError(t, err)
 
 			err = r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: test.argoCD.Namespace}, role)

controllers/argocd/role_test.go

@@ -27,7 +27,7 @@ func TestReconcileArgoCD_reconcileRole(t *testing.T) {
 
 	workloadIdentifier := "x"
 	expectedRules := policyRuleForApplicationController()
-	_, err := r.reconcileRole(workloadIdentifier, expectedRules, a)
+	err := r.reconcileRole(workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 
 	expectedName := fmt.Sprintf("%s-%s", a.Name, workloadIdentifier)
@@ -45,35 +45,32 @@ func TestReconcileArgoCD_reconcileRole(t *testing.T) {
 
 	// Check if the RedisHa policy rules are overwritten to Application Controller
 	// policy rules by the reconciler
-	_, err = r.reconcileRole(workloadIdentifier, expectedRules, a)
+	err = r.reconcileRole(workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: a.Namespace}, reconciledRole))
 	assert.Equal(t, expectedRules, reconciledRole.Rules)
 }
-func TestReconcileArgoCD_reconcileRole_for_new_namespace(t *testing.T) {
+
+func TestReconcileArgoCD_reconcileRole_for_new_namespaces(t *testing.T) {
 	logf.SetLogger(ZapLogger(true))
 	a := makeTestArgoCD()
 	r := makeTestReconciler(t, a)
 	assert.NoError(t, createNamespace(r, a.Namespace, ""))
 	assert.NoError(t, createNamespace(r, "newNamespaceTest", a.Namespace))
 
-	// 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
+	role := &v1.Role{}
 	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)
+	expectedName := fmt.Sprintf("%s-%s", a.Name, workloadIdentifier)
+	assert.NoError(t, r.reconcileRole(workloadIdentifier, expectedDexServerRules, a))
+	assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: "newTestNamespace"}, role))
+
 	// check no redisHa role is created for the new namespace with managed-by label
 	workloadIdentifier = common.ArgoCDRedisHAComponent
 	expectedRedisHaRules := policyRuleForRedisHa(r.Client)
-	redisHaRoles, err := r.reconcileRole(workloadIdentifier, expectedRedisHaRules, a)
-	assert.NoError(t, err)
-	assert.Equal(t, expectedNumberOfRoles, len(redisHaRoles))
-	assert.Equal(t, expectedRoleNamespace, redisHaRoles[0].ObjectMeta.Namespace)
+	assert.NoError(t, r.reconcileRole(workloadIdentifier, expectedRedisHaRules, a))
+	assert.Error(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: "newTestNamespace"}, role))
 }
 
 func TestReconcileArgoCD_reconcileClusterRole(t *testing.T) {
@@ -125,6 +122,7 @@ func TestReconcileArgoCD_reconcileClusterRole(t *testing.T) {
 func TestReconcileArgoCD_reconcileRoleForApplicationSourceNamespaces(t *testing.T) {
 	logf.SetLogger(ZapLogger(true))
 	sourceNamespace := "newNamespaceTest"
+	ctx := context.Background()
 	a := makeTestArgoCD()
 	a.Spec = v1alpha1.ArgoCDSpec{
 		SourceNamespaces: []string{
@@ -137,7 +135,7 @@ func TestReconcileArgoCD_reconcileRoleForApplicationSourceNamespaces(t *testing.
 
 	workloadIdentifier := common.ArgoCDServerComponent
 	expectedRules := policyRuleForServerApplicationSourceNamespaces()
-	_, err := r.reconcileRoleForApplicationSourceNamespaces(workloadIdentifier, expectedRules, a)
+	err := r.reconcileRoleForApplicationSourceNamespaces(ctx, workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 
 	expectedName := getRoleNameForApplicationSourceNamespaces(sourceNamespace, a)
@@ -156,13 +154,21 @@ func TestReconcileArgoCD_RoleHooks(t *testing.T) {
 	assert.NoError(t, createNamespace(r, a.Namespace, ""))
 	Register(testRoleHook)
 
-	roles, err := r.reconcileRole(common.ArgoCDApplicationControllerComponent, []v1.PolicyRule{}, a)
-	role := roles[0]
+	workloadIdentifier := common.ArgoCDApplicationControllerComponent
+	assert.NoError(t, r.reconcileRole(workloadIdentifier, []v1.PolicyRule{}, a))
+
+	// Fetch the role that should have been created and perform assertions.
+	role := &v1.Role{}
+	err := r.Client.Get(context.TODO(), types.NamespacedName{Name: generateResourceName(workloadIdentifier, a), Namespace: a.Namespace}, role)
 	assert.NoError(t, err)
 	assert.Equal(t, role.Rules, testRules())
 
-	roles, err = r.reconcileRole("test", []v1.PolicyRule{}, a)
-	role = roles[0]
+	workloadIdentifier = "test"
+	assert.NoError(t, r.reconcileRole(workloadIdentifier, []v1.PolicyRule{}, a))
+
+	// Fetch the role that should have been created and perform assertions.
+	role = &v1.Role{}
+	err = r.Client.Get(context.TODO(), types.NamespacedName{Name: generateResourceName(workloadIdentifier, a), Namespace: a.Namespace}, role)
 	assert.NoError(t, err)
 	assert.Equal(t, role.Rules, []v1.PolicyRule{})
 }
@@ -176,7 +182,7 @@ func TestReconcileArgoCD_reconcileRole_custom_role(t *testing.T) {
 
 	workloadIdentifier := "argocd-application-controller"
 	expectedRules := policyRuleForApplicationController()
-	_, err := r.reconcileRole(workloadIdentifier, expectedRules, a)
+	err := r.reconcileRole(workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 
 	expectedName := fmt.Sprintf("%s-%s", a.Name, workloadIdentifier)
@@ -191,7 +197,7 @@ func TestReconcileArgoCD_reconcileRole_custom_role(t *testing.T) {
 	// set the custom role as env variable
 	t.Setenv(common.ArgoCDControllerClusterRoleEnvName, "custom-role")
 
-	_, err = r.reconcileRole(workloadIdentifier, expectedRules, a)
+	err = r.reconcileRole(workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 
 	// check if the default cluster roles are removed
@@ -219,7 +225,7 @@ func TestReconcileRoles_ManagedTerminatingNamespace(t *testing.T) {
 
 	workloadIdentifier := "x"
 	expectedRules := policyRuleForApplicationController()
-	_, err := r.reconcileRole(workloadIdentifier, expectedRules, a)
+	err := r.reconcileRole(workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 
 	expectedName := fmt.Sprintf("%s-%s", a.Name, workloadIdentifier)
@@ -254,7 +260,7 @@ func TestReconcileRoles_ManagedTerminatingNamespace(t *testing.T) {
 	r.Client.Get(context.TODO(), types.NamespacedName{Namespace: "managedNS", Name: "managedNS"}, newNS)
 	assert.NotEqual(t, newNS.DeletionTimestamp, nil)
 
-	_, err = r.reconcileRole(workloadIdentifier, expectedRules, a)
+	err = r.reconcileRole(workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 
 	// Verify that the roles are deleted
@@ -265,7 +271,7 @@ func TestReconcileRoles_ManagedTerminatingNamespace(t *testing.T) {
 	assert.NoError(t, createNamespace(r, "managedNS2", a.Namespace))
 
 	// Check if roles are created for the new namespace as well
-	_, err = r.reconcileRole(workloadIdentifier, expectedRules, a)
+	err = r.reconcileRole(workloadIdentifier, expectedRules, a)
 	assert.NoError(t, err)
 
 	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: "managedNS2"}, reconciledRole))

controllers/argocd/rolebinding.go

@@ -58,15 +58,15 @@ func newRoleBinding(cr *argoprojv1a1.ArgoCD) *v1.RoleBinding {
 func newRoleBindingForSupportNamespaces(cr *argoprojv1a1.ArgoCD, namespace string) *v1.RoleBinding {
 	return &v1.RoleBinding{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:        getRoleBindingNameForSourceNamespaces(cr.Name, cr.Namespace, namespace),
+			Name:        getRoleBindingNameForSourceNamespaces(cr.Name, namespace),
 			Labels:      argoutil.LabelsForCluster(cr),
 			Annotations: argoutil.AnnotationsForCluster(cr),
 			Namespace:   namespace,
 		},
 	}
 }
 
-func getRoleBindingNameForSourceNamespaces(argocdName, argocdNamespace, targetNamespace string) string {
+func getRoleBindingNameForSourceNamespaces(argocdName, targetNamespace string) string {
 	return fmt.Sprintf("%s_%s", argocdName, targetNamespace)
 }
 
@@ -98,15 +98,14 @@ func (r *ReconcileArgoCD) reconcileRoleBindings(cr *argoprojv1a1.ArgoCD) error {
 // reconcileRoleBinding, creates RoleBindings for every role and associates it with the right ServiceAccount.
 // This would create RoleBindings for all the namespaces managed by the ArgoCD instance.
 func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) error {
-	var sa *corev1.ServiceAccount
-	var error error
-
-	if sa, error = r.reconcileServiceAccount(name, cr); error != nil {
-		return error
+	sa, err := r.reconcileServiceAccount(name, cr)
+	if err != nil {
+		return err
 	}
 
-	if _, error = r.reconcileRole(name, rules, cr); error != nil {
-		return error
+	err = r.reconcileRole(name, rules, cr)
+	if err != nil {
+		return err
 	}
 
 	for _, namespace := range r.ManagedNamespaces.Items {
@@ -243,7 +242,7 @@ func (r *ReconcileArgoCD) reconcileRoleBinding(name string, rules []v1.PolicyRul
 			}
 
 			// get expected name
-			roleBinding := newRoleBindingWithNameForApplicationSourceNamespaces(name, namespace.Name, cr)
+			roleBinding := newRoleBindingWithNameForApplicationSourceNamespaces(namespace.Name, cr)
 			roleBinding.Namespace = namespace.Name
 
 			roleBinding.RoleRef = v1.RoleRef{
@@ -324,7 +323,7 @@ func getRoleNameForApplicationSourceNamespaces(targetNamespace string, cr *argop
 }
 
 // newRoleBindingWithNameForApplicationSourceNamespaces creates a new RoleBinding with the given name for the source namespaces of ArgoCD Server.
-func newRoleBindingWithNameForApplicationSourceNamespaces(name, namespace string, cr *argoprojv1a1.ArgoCD) *v1.RoleBinding {
+func newRoleBindingWithNameForApplicationSourceNamespaces(namespace string, cr *argoprojv1a1.ArgoCD) *v1.RoleBinding {
 	roleBinding := newRoleBindingForSupportNamespaces(cr, namespace)
 
 	labels := roleBinding.ObjectMeta.Labels

controllers/argocd/rolebinding_test.go

@@ -219,7 +219,7 @@ func TestReconcileArgoCD_reconcileRoleBinding_forSourceNamespaces(t *testing.T)
 	assert.NoError(t, r.reconcileRoleBinding(workloadIdentifier, p, a))
 
 	roleBinding := &rbacv1.RoleBinding{}
-	expectedName := getRoleBindingNameForSourceNamespaces(a.Name, a.Namespace, sourceNamespace)
+	expectedName := getRoleBindingNameForSourceNamespaces(a.Name, sourceNamespace)
 
 	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName, Namespace: sourceNamespace}, roleBinding))