From afef9d8471ed49dd4a037642897447d544582108 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Mon, 12 Jun 2023 21:30:27 -0400 Subject: [PATCH 01/10] Updated logic for role/rolebinding reconciliation Signed-off-by: Yi Cai --- controllers/argocd/argocd_controller.go | 4 +- controllers/argocd/role.go | 10 ++-- controllers/argocd/role_test.go | 3 +- controllers/argocd/util.go | 46 +++++++++---------- .../01-argocd-with-source-namespaces.yaml | 1 - .../01-assert.yaml | 1 - .../02-assert.yaml | 1 - .../02-errors.yaml | 1 - .../03-assert.yaml | 1 - .../03-delete-argocd.yaml | 3 +- .../03-errors.yaml | 1 - .../99-delete.yaml | 3 +- 12 files changed, 32 insertions(+), 43 deletions(-) diff --git a/controllers/argocd/argocd_controller.go b/controllers/argocd/argocd_controller.go index e2e7abb00..1d5ea914b 100644 --- a/controllers/argocd/argocd_controller.go +++ b/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) } @@ -138,7 +138,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 } diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index 3eec12ff3..56621e775 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -63,7 +63,7 @@ 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 { +func (r *ReconcileArgoCD) reconcileRoles(ctx context.Context, cr *argoprojv1a1.ArgoCD) error { params := getPolicyRuleList(r.Client) for _, param := range params { @@ -83,13 +83,13 @@ func (r *ReconcileArgoCD) reconcileRoles(cr *argoprojv1a1.ArgoCD) error { log.Info("reconciling roles for source namespaces") policyRuleForApplicationSourceNamespaces := policyRuleForServerApplicationSourceNamespaces() // reconcile roles is source namespaces for ArgoCD Server - if _, err := r.reconcileRoleForApplicationSourceNamespaces(common.ArgoCDServerComponent, policyRuleForApplicationSourceNamespaces, cr); err != nil { + if _, err := r.reconcileRoleForApplicationSourceNamespaces(ctx, common.ArgoCDServerComponent, policyRuleForApplicationSourceNamespaces, cr); err != nil { return err } log.Info("performing cleanup for source namespaces") // remove resources for namespaces not part of SourceNamespaces - if err := r.removeUnmanagedSourceNamespaceResources(cr); err != nil { + if err := r.removeUnmanagedSourceNamespaceResources(ctx, cr); err != nil { return err } @@ -186,7 +186,7 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule return roles, nil } -func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) ([]*v1.Role, error) { +func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx context.Context, name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) ([]*v1.Role, error) { var roles []*v1.Role // create policy rules for each source namespace for ArgoCD Server @@ -205,7 +205,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(name strin // if managed-by-cluster-argocd label is also present, remove the namespace from the ManagedSourceNamespaces. if val, ok1 := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel]; ok1 && val == cr.Namespace { delete(r.ManagedSourceNamespaces, namespace.Name) - if err := r.cleanupUnmanagedSourceNamespaceResources(cr, namespace.Name); err != nil { + if err := r.cleanupUnmanagedSourceNamespaceResources(ctx, cr, namespace.Name); err != nil { log.Error(err, fmt.Sprintf("error cleaning up resources for namespace %s", namespace.Name)) } } diff --git a/controllers/argocd/role_test.go b/controllers/argocd/role_test.go index eb84550ec..4188f5069 100644 --- a/controllers/argocd/role_test.go +++ b/controllers/argocd/role_test.go @@ -125,6 +125,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 +138,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) diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index dfef38c35..83b20c1bb 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -703,7 +703,7 @@ func (r *ReconcileArgoCD) redisShouldUseTLS(cr *argoprojv1a1.ArgoCD) bool { } // reconcileResources will reconcile common ArgoCD resources. -func (r *ReconcileArgoCD) reconcileResources(cr *argoprojv1a1.ArgoCD) error { +func (r *ReconcileArgoCD) reconcileResources(ctx context.Context, cr *argoprojv1a1.ArgoCD) error { // reconcile SSO first, because dex resources get reconciled through other function calls as well, not just through reconcileSSO (this is important // so that dex resources can be appropriately cleaned up when DISABLE_DEX is set to true and the operator pod restarts but doesn't enter @@ -721,7 +721,7 @@ func (r *ReconcileArgoCD) reconcileResources(cr *argoprojv1a1.ArgoCD) error { } log.Info("reconciling roles") - if err := r.reconcileRoles(cr); err != nil { + if err := r.reconcileRoles(ctx, cr); err != nil { log.Info(err.Error()) return err } @@ -1412,33 +1412,28 @@ func (r *ReconcileArgoCD) setManagedSourceNamespaces(cr *argoproj.ArgoCD) error // removeUnmanagedSourceNamespaceResources cleansup resources from SourceNamespaces if namespace is not managed by argocd instance. // It also removes the managed-by-cluster-argocd label from the namespace -func (r *ReconcileArgoCD) removeUnmanagedSourceNamespaceResources(cr *argoproj.ArgoCD) error { - - for ns, _ := range r.ManagedSourceNamespaces { +func (r *ReconcileArgoCD) removeUnmanagedSourceNamespaceResources(ctx context.Context, cr *argoproj.ArgoCD) error { + for ns := range r.ManagedSourceNamespaces { managedNamespace := false if cr.GetDeletionTimestamp() == nil { - for _, namespace := range cr.Spec.SourceNamespaces { - if namespace == ns { - managedNamespace = true - break - } - } + managedNamespace = contains(cr.Spec.SourceNamespaces, ns) } if !managedNamespace { - if err := r.cleanupUnmanagedSourceNamespaceResources(cr, ns); err != nil { - log.Error(err, fmt.Sprintf("error cleaning up resources for namespace %s", ns)) - continue + if err := r.cleanupUnmanagedSourceNamespaceResources(ctx, cr, ns); err != nil { + return fmt.Errorf("error cleaning up resources for namespace %s: %v", ns, err) + } + if _, exists := r.ManagedSourceNamespaces[ns]; exists { + delete(r.ManagedSourceNamespaces, ns) } - delete(r.ManagedSourceNamespaces, ns) } } return nil } -func (r *ReconcileArgoCD) cleanupUnmanagedSourceNamespaceResources(cr *argoproj.ArgoCD, ns string) error { +func (r *ReconcileArgoCD) cleanupUnmanagedSourceNamespaceResources(ctx context.Context, cr *argoproj.ArgoCD, ns string) error { namespace := corev1.Namespace{} - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: ns}, &namespace); err != nil { + if err := r.Client.Get(ctx, types.NamespacedName{Name: ns}, &namespace); err != nil { if !errors.IsNotFound(err) { return err } @@ -1446,33 +1441,34 @@ func (r *ReconcileArgoCD) cleanupUnmanagedSourceNamespaceResources(cr *argoproj. } // Remove managed-by-cluster-argocd from the namespace delete(namespace.Labels, common.ArgoCDManagedByClusterArgoCDLabel) - if err := r.Client.Update(context.TODO(), &namespace); err != nil { - log.Error(err, fmt.Sprintf("failed to remove label from namespace [%s]", namespace.Name)) + if err := r.Client.Update(ctx, &namespace); err != nil { + return fmt.Errorf("failed to remove label from namespace [%s]: %v", namespace.Name, err) } // Delete Roles for SourceNamespaces existingRole := v1.Role{} roleName := getRoleNameForApplicationSourceNamespaces(namespace.Name, cr) - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: roleName, Namespace: namespace.Name}, &existingRole); err != nil { + if err := r.Client.Get(ctx, types.NamespacedName{Name: roleName, Namespace: namespace.Name}, &existingRole); err != nil { if !errors.IsNotFound(err) { - return fmt.Errorf("failed to fetch the role for the service account associated with %s : %s", common.ArgoCDServerComponent, err) + return fmt.Errorf("failed to fetch the role for the service account associated with %s: %v", common.ArgoCDServerComponent, err) } } if existingRole.Name != "" { - if err := r.Client.Delete(context.TODO(), &existingRole); err != nil { + if err := r.Client.Delete(ctx, &existingRole); err != nil { return err } } + // Delete RoleBindings for SourceNamespaces existingRoleBinding := &v1.RoleBinding{} roleBindingName := getRoleBindingNameForSourceNamespaces(cr.Name, cr.Namespace, namespace.Name) - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: roleBindingName, Namespace: namespace.Name}, existingRoleBinding); err != nil { + if err := r.Client.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace.Name}, existingRoleBinding); err != nil { if !errors.IsNotFound(err) { - return fmt.Errorf("failed to get the rolebinding associated with %s : %s", common.ArgoCDServerComponent, err) + return fmt.Errorf("failed to get the rolebinding associated with %s: %v", common.ArgoCDServerComponent, err) } } if existingRoleBinding.Name != "" { - if err := r.Client.Delete(context.TODO(), existingRoleBinding); err != nil { + if err := r.Client.Delete(ctx, existingRoleBinding); err != nil { return err } } diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/01-argocd-with-source-namespaces.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/01-argocd-with-source-namespaces.yaml index b890b7dcb..678e41b2b 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/01-argocd-with-source-namespaces.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/01-argocd-with-source-namespaces.yaml @@ -1,4 +1,3 @@ ---- apiVersion: v1 kind: Namespace metadata: diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/01-assert.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/01-assert.yaml index 9c6646491..9566c849c 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/01-assert.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/01-assert.yaml @@ -1,4 +1,3 @@ ---- apiVersion: v1 kind: Namespace metadata: diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/02-assert.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/02-assert.yaml index aca6c501e..9409311db 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/02-assert.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/02-assert.yaml @@ -1,4 +1,3 @@ ---- apiVersion: v1 kind: Namespace metadata: diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/02-errors.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/02-errors.yaml index 0d37ea521..937cbd084 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/02-errors.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/02-errors.yaml @@ -1,4 +1,3 @@ ---- apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/03-assert.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/03-assert.yaml index d5460cc7e..457b62c61 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/03-assert.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/03-assert.yaml @@ -1,4 +1,3 @@ ---- apiVersion: v1 kind: Namespace metadata: diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml index 8f02301dc..64b3d47b8 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/03-delete-argocd.yaml @@ -1,4 +1,3 @@ ---- apiVersion: kuttl.dev/v1beta1 kind: TestStep delete: @@ -8,4 +7,4 @@ delete: namespace: central-argocd commands: # Sleep to allow resources to be completely deleted - - command: sleep 30s \ No newline at end of file + - script: sleep 30 \ No newline at end of file diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/03-errors.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/03-errors.yaml index ea1d87b46..155ab3dbb 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/03-errors.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/03-errors.yaml @@ -1,4 +1,3 @@ ---- apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: diff --git a/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml b/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml index 0d87c9379..316e86b4b 100644 --- a/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml +++ b/tests/k8s/1-024_validate_apps_in_any_namespace/99-delete.yaml @@ -1,4 +1,3 @@ ---- apiVersion: kuttl.dev/v1beta1 kind: TestStep delete: @@ -12,4 +11,4 @@ delete: kind: Namespace name: central-argocd commands: - - command: sleep 30s \ No newline at end of file + - script: sleep 30 \ No newline at end of file From 9949a88e082151560390a888ba197e47ef73ea95 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Tue, 13 Jun 2023 18:08:53 -0400 Subject: [PATCH 02/10] Fixed golangci-lint issue Signed-off-by: Yi Cai --- controllers/argocd/dex.go | 2 +- controllers/argocd/dex_test.go | 4 +- controllers/argocd/role.go | 87 ++++++++++++++++++------------- controllers/argocd/role_test.go | 51 ++++++++++-------- controllers/argocd/rolebinding.go | 13 +++-- 5 files changed, 87 insertions(+), 70 deletions(-) diff --git a/controllers/argocd/dex.go b/controllers/argocd/dex.go index b81b90103..bf4b47563 100644 --- a/controllers/argocd/dex.go +++ b/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") } diff --git a/controllers/argocd/dex_test.go b/controllers/argocd/dex_test.go index 7107bcb18..fcf881525 100644 --- a/controllers/argocd/dex_test.go +++ b/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) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index 56621e775..036b757e4 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -31,7 +31,7 @@ func newRole(name string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) *v1.Ro } } -func newRoleForApplicationSourceNamespaces(name, namespace string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) *v1.Role { +func newRoleForApplicationSourceNamespaces(namespace string, rules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) *v1.Role { return &v1.Role{ ObjectMeta: metav1.ObjectMeta{ Name: getRoleNameForApplicationSourceNamespaces(namespace, cr), @@ -67,7 +67,7 @@ func (r *ReconcileArgoCD) reconcileRoles(ctx context.Context, cr *argoprojv1a1.A params := getPolicyRuleList(r.Client) for _, param := range params { - if _, err := r.reconcileRole(param.name, param.policyRule, cr); err != nil { + if err := r.reconcileRole(param.name, param.policyRule, cr); err != nil { return err } } @@ -98,14 +98,16 @@ func (r *ReconcileArgoCD) reconcileRoles(ctx context.Context, cr *argoprojv1a1.A // reconcileRole, reconciles the policy rules for different ArgoCD components, for each namespace // Managed by a single instance of ArgoCD. -func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) ([]*v1.Role, error) { - var roles []*v1.Role +func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) error { + // var roles []*v1.Role + log.Info(fmt.Sprintf("Starting reconcileRole with name: %s", name)) // create policy rules for each namespace for _, namespace := range r.ManagedNamespaces.Items { // If encountering a terminating namespace remove managed-by label from it and skip reconciliation - This should trigger // clean-up of roles/rolebindings and removal of namespace from cluster secret if namespace.DeletionTimestamp != nil { + log.Info("Found terminating namespace. Removing managed-by label and skipping reconciliation.") if _, ok := namespace.Labels[common.ArgoCDManagedByLabel]; ok { delete(namespace.Labels, common.ArgoCDManagedByLabel) _ = r.Client.Update(context.TODO(), &namespace) @@ -117,8 +119,10 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule listOption := &client.ListOptions{Namespace: namespace.Name} err := r.Client.List(context.TODO(), list, listOption) if err != nil { - return nil, err + return err } + log.Info(fmt.Sprintf("ArgoCD list for namespace '%s' contains '%d' items", namespace.Name, len(list.Items))) + // only skip creation of dex and redisHa roles for namespaces that no argocd instance is deployed in if len(list.Items) < 1 { // only create dexServer and redisHa roles for the namespace where the argocd instance is deployed @@ -126,64 +130,73 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule continue } } - customRole := getCustomRoleName(name) + role := newRole(name, policyRules, cr) if err := applyReconcilerHook(cr, role, ""); err != nil { - return nil, err + return err } role.Namespace = namespace.Name existingRole := v1.Role{} err = r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: role.Namespace}, &existingRole) + roleExists := true if err != nil { if !errors.IsNotFound(err) { - return nil, fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", name, err) - } - if customRole != "" { - continue // skip creating default role if custom cluster role is provided + return fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", name, err) } - roles = append(roles, role) if name == common.ArgoCDDexServerComponent && !UseDex(cr) { - + log.Info("Dex installation not requested. Skipping role creation.") continue // Dex installation not requested, do nothing } - // Only set ownerReferences for roles in same namespace as ArgoCD CR - if cr.Namespace == role.Namespace { - if err = controllerutil.SetControllerReference(cr, role, r.Scheme); err != nil { - return nil, fmt.Errorf("failed to set ArgoCD CR \"%s\" as owner for role \"%s\": %s", cr.Name, role.Name, err) + roleExists = false + } + + customRole := getCustomRoleName(name) + + if roleExists { + // Delete the existing default role if custom role is specified + // or if there is an existing Role created for Dex but dex is disabled or not configured + if customRole != "" || name == common.ArgoCDDexServerComponent && !UseDex(cr) { + log.Info("deleting the existing Dex role because dex is not configured") + if err := r.Client.Delete(context.TODO(), &existingRole); err != nil { + log.Error(err, fmt.Sprintf("Failed to delete existing role %s in namespace %s: %v", existingRole.Name, existingRole.Namespace, err)) + return err } } - log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, cr.Namespace)) - if err := r.Client.Create(context.TODO(), role); err != nil { - return nil, err + // 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 { + log.Error(err, fmt.Sprintf("Failed to update role %s in namespace %s: %v", existingRole.Name, existingRole.Namespace, err)) + return err + } } continue } - // Delete the existing default role if custom role is specified - // or if there is an existing Role created for Dex but dex is disabled or not configured - if customRole != "" || - (name == common.ArgoCDDexServerComponent && !UseDex(cr)) { + if customRole != "" { + log.Info(fmt.Sprintf("Custom role '%s' found. Skipping default role creation.", customRole)) + continue // skip creating default role if custom cluster role is provided + } - log.Info("deleting the existing Dex role because dex is not configured") - if err := r.Client.Delete(context.TODO(), &existingRole); err != nil { - return nil, err + // Only set ownerReferences for roles in same namespace as ArgoCD CR + if cr.Namespace == role.Namespace { + if err = controllerutil.SetControllerReference(cr, role, r.Scheme); err != nil { + return fmt.Errorf("failed to set ArgoCD CR \"%s\" as owner for role \"%s\": %s", cr.Name, role.Name, err) } - continue } - // 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 - } + log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, cr.Namespace)) + if err := r.Client.Create(context.TODO(), role); err != nil { + log.Error(err, fmt.Sprintf("Failed to create role %s in namespace %s: %v", role.Name, role.Namespace, err)) + return err } - roles = append(roles, &existingRole) + log.Info(fmt.Sprintf("Role %s created successfully in namespace %s", role.Name, role.Namespace)) + } - return roles, nil + return nil } func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx context.Context, name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) ([]*v1.Role, error) { @@ -214,7 +227,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex log.Info(fmt.Sprintf("Reconciling role for %s", namespace.Name)) - role := newRoleForApplicationSourceNamespaces(name, namespace.Name, policyRules, cr) + role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) if err := applyReconcilerHook(cr, role, ""); err != nil { return nil, err } diff --git a/controllers/argocd/role_test.go b/controllers/argocd/role_test.go index 4188f5069..97d53dcba 100644 --- a/controllers/argocd/role_test.go +++ b/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) { @@ -157,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{}) } @@ -177,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) @@ -192,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 @@ -220,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) @@ -255,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 @@ -266,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)) diff --git a/controllers/argocd/rolebinding.go b/controllers/argocd/rolebinding.go index 2547b5bfb..7ef83fd4a 100644 --- a/controllers/argocd/rolebinding.go +++ b/controllers/argocd/rolebinding.go @@ -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 { From d72f2a707d8b78892b420918a1eff1407a543d8a Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Wed, 14 Jun 2023 09:45:12 -0400 Subject: [PATCH 03/10] golangci-lint fix Signed-off-by: Yi Cai --- controllers/argocd/argocd_controller.go | 4 +--- controllers/argocd/role.go | 22 +++++++++------------- controllers/argocd/role_test.go | 2 +- controllers/argocd/rolebinding.go | 8 ++++---- controllers/argocd/rolebinding_test.go | 2 +- controllers/argocd/util.go | 10 +++------- 6 files changed, 19 insertions(+), 29 deletions(-) diff --git a/controllers/argocd/argocd_controller.go b/controllers/argocd/argocd_controller.go index 1d5ea914b..ed3455aa8 100644 --- a/controllers/argocd/argocd_controller.go +++ b/controllers/argocd/argocd_controller.go @@ -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 } diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index 036b757e4..e8c3badde 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -83,7 +83,7 @@ func (r *ReconcileArgoCD) reconcileRoles(ctx context.Context, cr *argoprojv1a1.A log.Info("reconciling roles for source namespaces") policyRuleForApplicationSourceNamespaces := policyRuleForServerApplicationSourceNamespaces() // reconcile roles is source namespaces for ArgoCD Server - if _, err := r.reconcileRoleForApplicationSourceNamespaces(ctx, common.ArgoCDServerComponent, policyRuleForApplicationSourceNamespaces, cr); err != nil { + if err := r.reconcileRoleForApplicationSourceNamespaces(ctx, common.ArgoCDServerComponent, policyRuleForApplicationSourceNamespaces, cr); err != nil { return err } @@ -99,7 +99,6 @@ func (r *ReconcileArgoCD) reconcileRoles(ctx context.Context, cr *argoprojv1a1.A // reconcileRole, reconciles the policy rules for different ArgoCD components, for each namespace // Managed by a single instance of ArgoCD. func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) error { - // var roles []*v1.Role log.Info(fmt.Sprintf("Starting reconcileRole with name: %s", name)) // create policy rules for each namespace @@ -199,15 +198,13 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule return nil } -func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx context.Context, name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) ([]*v1.Role, error) { - var roles []*v1.Role - +func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx context.Context, name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) error { // create policy rules for each source namespace for ArgoCD Server for _, sourceNamespace := range cr.Spec.SourceNamespaces { namespace := &corev1.Namespace{} if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: sourceNamespace}, namespace); err != nil { - return nil, err + return err } // do not reconcile roles for namespaces already containing managed-by label @@ -229,14 +226,14 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) if err := applyReconcilerHook(cr, role, ""); err != nil { - return nil, err + return err } role.Namespace = namespace.Name existingRole := v1.Role{} err := r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, &existingRole) if err != nil { if !errors.IsNotFound(err) { - return nil, fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", name, err) + return fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", name, err) } } @@ -247,7 +244,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex if reflect.DeepEqual(existingRole, v1.Role{}) { log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, namespace)) if err := r.Client.Create(context.TODO(), role); err != nil { - return nil, err + return err } } continue @@ -261,7 +258,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex // Get the latest value of namespace before updating it if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: namespace.Name}, namespace); err != nil { - return nil, err + return err } // Update namespace with managed-by-cluster-argocd label namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace @@ -272,17 +269,16 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex if !reflect.DeepEqual(existingRole.Rules, role.Rules) { existingRole.Rules = role.Rules if err := r.Client.Update(context.TODO(), &existingRole); err != nil { - return nil, err + return err } } - roles = append(roles, &existingRole) if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; !ok { r.ManagedSourceNamespaces[sourceNamespace] = "" } } - return roles, nil + return nil } func (r *ReconcileArgoCD) reconcileClusterRole(name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) (*v1.ClusterRole, error) { diff --git a/controllers/argocd/role_test.go b/controllers/argocd/role_test.go index 97d53dcba..691a72d7a 100644 --- a/controllers/argocd/role_test.go +++ b/controllers/argocd/role_test.go @@ -135,7 +135,7 @@ func TestReconcileArgoCD_reconcileRoleForApplicationSourceNamespaces(t *testing. workloadIdentifier := common.ArgoCDServerComponent expectedRules := policyRuleForServerApplicationSourceNamespaces() - _, err := r.reconcileRoleForApplicationSourceNamespaces(ctx, workloadIdentifier, expectedRules, a) + err := r.reconcileRoleForApplicationSourceNamespaces(ctx, workloadIdentifier, expectedRules, a) assert.NoError(t, err) expectedName := getRoleNameForApplicationSourceNamespaces(sourceNamespace, a) diff --git a/controllers/argocd/rolebinding.go b/controllers/argocd/rolebinding.go index 7ef83fd4a..9d4011e16 100644 --- a/controllers/argocd/rolebinding.go +++ b/controllers/argocd/rolebinding.go @@ -58,7 +58,7 @@ 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, @@ -66,7 +66,7 @@ func newRoleBindingForSupportNamespaces(cr *argoprojv1a1.ArgoCD, namespace strin } } -func getRoleBindingNameForSourceNamespaces(argocdName, argocdNamespace, targetNamespace string) string { +func getRoleBindingNameForSourceNamespaces(argocdName, targetNamespace string) string { return fmt.Sprintf("%s_%s", argocdName, targetNamespace) } @@ -242,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{ @@ -323,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 diff --git a/controllers/argocd/rolebinding_test.go b/controllers/argocd/rolebinding_test.go index 89a5d3767..1d5d6e128 100644 --- a/controllers/argocd/rolebinding_test.go +++ b/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)) diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index 83b20c1bb..0a12e15ed 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1253,9 +1253,7 @@ func namespaceFilterPredicate() predicate.Predicate { // if a namespace is deleted, remove it from deprecationEventEmissionTracker (if exists) so that if a namespace with the same name // is created in the future and contains an Argo CD instance, it will be tracked appropriately - if _, ok := DeprecationEventEmissionTracker[e.Object.GetName()]; ok { - delete(DeprecationEventEmissionTracker, e.Object.GetName()) - } + delete(DeprecationEventEmissionTracker, e.Object.GetName()) return false }, @@ -1423,9 +1421,7 @@ func (r *ReconcileArgoCD) removeUnmanagedSourceNamespaceResources(ctx context.Co if err := r.cleanupUnmanagedSourceNamespaceResources(ctx, cr, ns); err != nil { return fmt.Errorf("error cleaning up resources for namespace %s: %v", ns, err) } - if _, exists := r.ManagedSourceNamespaces[ns]; exists { - delete(r.ManagedSourceNamespaces, ns) - } + delete(r.ManagedSourceNamespaces, ns) } } return nil @@ -1461,7 +1457,7 @@ func (r *ReconcileArgoCD) cleanupUnmanagedSourceNamespaceResources(ctx context.C // Delete RoleBindings for SourceNamespaces existingRoleBinding := &v1.RoleBinding{} - roleBindingName := getRoleBindingNameForSourceNamespaces(cr.Name, cr.Namespace, namespace.Name) + roleBindingName := getRoleBindingNameForSourceNamespaces(cr.Name, namespace.Name) if err := r.Client.Get(ctx, types.NamespacedName{Name: roleBindingName, Namespace: namespace.Name}, existingRoleBinding); err != nil { if !errors.IsNotFound(err) { return fmt.Errorf("failed to get the rolebinding associated with %s: %v", common.ArgoCDServerComponent, err) From 2901b9a5d67dac325d429377330ade885d869a95 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Wed, 14 Jun 2023 11:15:05 -0400 Subject: [PATCH 04/10] refactored reconcileRoleForApplicationSourceNamespaces Signed-off-by: Yi Cai --- controllers/argocd/role.go | 94 +++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 48 deletions(-) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index e8c3badde..326a20be2 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -207,76 +207,74 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex return err } + value, ok := namespace.Labels[common.ArgoCDManagedByLabel] // do not reconcile roles for namespaces already containing managed-by label // as it already contains roles with permissions to manipulate application resources // reconciled during reconcilation of ManagedNamespaces - if value, ok := namespace.Labels[common.ArgoCDManagedByLabel]; ok && value != "" { + if ok && value != "" { log.Info(fmt.Sprintf("Skipping reconciling resources for namespace %s as it is already managed-by namespace %s.", namespace.Name, value)) // if managed-by-cluster-argocd label is also present, remove the namespace from the ManagedSourceNamespaces. - if val, ok1 := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel]; ok1 && val == cr.Namespace { + val, ok1 := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] + if ok1 && val == cr.Namespace { delete(r.ManagedSourceNamespaces, namespace.Name) if err := r.cleanupUnmanagedSourceNamespaceResources(ctx, cr, namespace.Name); err != nil { log.Error(err, fmt.Sprintf("error cleaning up resources for namespace %s", namespace.Name)) } } - continue - } - - log.Info(fmt.Sprintf("Reconciling role for %s", namespace.Name)) + } else { + log.Info(fmt.Sprintf("Reconciling role for %s", namespace.Name)) - role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) - if err := applyReconcilerHook(cr, role, ""); err != nil { - return err - } - role.Namespace = namespace.Name - existingRole := v1.Role{} - err := r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, &existingRole) - if err != nil { - if !errors.IsNotFound(err) { + role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) + if err := applyReconcilerHook(cr, role, ""); err != nil { + return err + } + role.Namespace = namespace.Name + existingRole := v1.Role{} + err := r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, &existingRole) + if err != nil && !errors.IsNotFound(err) { return fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", name, err) } - } - // do not reconcile roles for namespaces already containing managed-by-cluster-argocd label - // as it already contains roles reconciled during reconcilation of ManagedNamespaces - if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; ok { - // If sourceNamespace includes the name but role is missing in the namespace, create the role - if reflect.DeepEqual(existingRole, v1.Role{}) { - log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, namespace)) - if err := r.Client.Create(context.TODO(), role); err != nil { - return err + // do not reconcile roles for namespaces already containing managed-by-cluster-argocd label + // as it already contains roles reconciled during reconcilation of ManagedNamespaces + if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; ok { + // If sourceNamespace includes the name but role is missing in the namespace, create the role + if reflect.DeepEqual(existingRole, v1.Role{}) { + log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, namespace)) + if err := r.Client.Create(context.TODO(), role); err != nil { + return err + } } + continue } - continue - } - // reconcile roles only if another ArgoCD instance is not already set as value for managed-by-cluster-argocd label - if value, ok := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel]; ok && value != "" { - log.Info(fmt.Sprintf("Namespace already has label set to argocd instance %s. Thus, skipping namespace %s", value, namespace.Name)) - continue - } + // reconcile roles only if another ArgoCD instance is not already set as value for managed-by-cluster-argocd label + if value, ok := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel]; ok && value != "" { + log.Info(fmt.Sprintf("Namespace already has label set to argocd instance %s. Thus, skipping namespace %s", value, namespace.Name)) + continue + } - // Get the latest value of namespace before updating it - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: namespace.Name}, namespace); err != nil { - return err - } - // Update namespace with managed-by-cluster-argocd label - namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace - if err := r.Client.Update(context.TODO(), namespace); err != nil { - log.Error(err, fmt.Sprintf("failed to add label from namespace [%s]", namespace.Name)) - } - // 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 { + // Get the latest value of namespace before updating it + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: namespace.Name}, namespace); err != nil { return err } - } + // Update namespace with managed-by-cluster-argocd label + namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace + if err := r.Client.Update(context.TODO(), namespace); err != nil { + log.Error(err, fmt.Sprintf("failed to add label from namespace [%s]", namespace.Name)) + } + // 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 err + } + } - if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; !ok { - r.ManagedSourceNamespaces[sourceNamespace] = "" + if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; !ok { + r.ManagedSourceNamespaces[sourceNamespace] = "" + } } - } return nil } From 3d776f52e21203cd5a1c50d2066d114c7b1d4442 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Wed, 14 Jun 2023 11:31:53 -0400 Subject: [PATCH 05/10] Replace context.TODO() with passed ctx Signed-off-by: Yi Cai --- controllers/argocd/role.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index 326a20be2..25fa9da44 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -203,7 +203,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex for _, sourceNamespace := range cr.Spec.SourceNamespaces { namespace := &corev1.Namespace{} - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: sourceNamespace}, namespace); err != nil { + if err := r.Client.Get(ctx, types.NamespacedName{Name: sourceNamespace}, namespace); err != nil { return err } @@ -230,7 +230,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex } role.Namespace = namespace.Name existingRole := v1.Role{} - err := r.Client.Get(context.TODO(), types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, &existingRole) + err := r.Client.Get(ctx, types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, &existingRole) if err != nil && !errors.IsNotFound(err) { return fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", name, err) } @@ -241,7 +241,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex // If sourceNamespace includes the name but role is missing in the namespace, create the role if reflect.DeepEqual(existingRole, v1.Role{}) { log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, namespace)) - if err := r.Client.Create(context.TODO(), role); err != nil { + if err := r.Client.Create(ctx, role); err != nil { return err } } @@ -255,18 +255,18 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex } // Get the latest value of namespace before updating it - if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: namespace.Name}, namespace); err != nil { + if err := r.Client.Get(ctx, types.NamespacedName{Name: namespace.Name}, namespace); err != nil { return err } // Update namespace with managed-by-cluster-argocd label namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace - if err := r.Client.Update(context.TODO(), namespace); err != nil { + if err := r.Client.Update(ctx, namespace); err != nil { log.Error(err, fmt.Sprintf("failed to add label from namespace [%s]", namespace.Name)) } // 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 { + if err := r.Client.Update(ctx, &existingRole); err != nil { return err } } From 1a13d156230eb49048dd016dc82f138e555f98fe Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Wed, 14 Jun 2023 11:56:31 -0400 Subject: [PATCH 06/10] Updated error log in reconcileRoleForApplicationSourceNamespaces Signed-off-by: Yi Cai --- controllers/argocd/role.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index 25fa9da44..bddee958e 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -226,13 +226,13 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) if err := applyReconcilerHook(cr, role, ""); err != nil { - return err + return fmt.Errorf("failed to apply reconciler hook for %s: %w", name, err) } role.Namespace = namespace.Name existingRole := v1.Role{} err := r.Client.Get(ctx, types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, &existingRole) if err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to reconcile the role for the service account associated with %s : %s", name, err) + return fmt.Errorf("failed to reconcile the role for the service account associated with %s: %w", name, err) } // do not reconcile roles for namespaces already containing managed-by-cluster-argocd label @@ -242,7 +242,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex if reflect.DeepEqual(existingRole, v1.Role{}) { log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, namespace)) if err := r.Client.Create(ctx, role); err != nil { - return err + return fmt.Errorf("failed to create role for %s: %w", name, err) } } continue @@ -256,7 +256,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex // Get the latest value of namespace before updating it if err := r.Client.Get(ctx, types.NamespacedName{Name: namespace.Name}, namespace); err != nil { - return err + return fmt.Errorf("failed to get latest namespace for %s: %w", name, err) } // Update namespace with managed-by-cluster-argocd label namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace @@ -267,7 +267,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex if !reflect.DeepEqual(existingRole.Rules, role.Rules) { existingRole.Rules = role.Rules if err := r.Client.Update(ctx, &existingRole); err != nil { - return err + return fmt.Errorf("failed to update role for %s: %w", name, err) } } From c24544099fddfdecbabecb8b5e86a8275d2934c8 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Wed, 14 Jun 2023 13:32:59 -0400 Subject: [PATCH 07/10] Added return err to r.Client.Update Signed-off-by: Yi Cai --- controllers/argocd/role.go | 16 ++++++++-------- controllers/argocd/util.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index bddee958e..6e1dc11a6 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -207,15 +207,15 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex return err } - value, ok := namespace.Labels[common.ArgoCDManagedByLabel] + managedByLabelval, isManagedByLabelPresent := namespace.Labels[common.ArgoCDManagedByLabel] + managedByClusterArgocdVal, isManagedByClusterArgoCDLabelPresent := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] // do not reconcile roles for namespaces already containing managed-by label // as it already contains roles with permissions to manipulate application resources // reconciled during reconcilation of ManagedNamespaces - if ok && value != "" { - log.Info(fmt.Sprintf("Skipping reconciling resources for namespace %s as it is already managed-by namespace %s.", namespace.Name, value)) + if isManagedByLabelPresent && managedByLabelval != "" { + log.Info(fmt.Sprintf("Skipping reconciling resources for namespace %s as it is already managed-by namespace %s.", namespace.Name, managedByLabelval)) // if managed-by-cluster-argocd label is also present, remove the namespace from the ManagedSourceNamespaces. - val, ok1 := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] - if ok1 && val == cr.Namespace { + if isManagedByClusterArgoCDLabelPresent && managedByClusterArgocdVal == cr.Namespace { delete(r.ManagedSourceNamespaces, namespace.Name) if err := r.cleanupUnmanagedSourceNamespaceResources(ctx, cr, namespace.Name); err != nil { log.Error(err, fmt.Sprintf("error cleaning up resources for namespace %s", namespace.Name)) @@ -249,8 +249,8 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex } // reconcile roles only if another ArgoCD instance is not already set as value for managed-by-cluster-argocd label - if value, ok := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel]; ok && value != "" { - log.Info(fmt.Sprintf("Namespace already has label set to argocd instance %s. Thus, skipping namespace %s", value, namespace.Name)) + if isManagedByClusterArgoCDLabelPresent && managedByClusterArgocdVal != "" { + log.Info(fmt.Sprintf("Namespace already has label set to argocd instance %s. Thus, skipping namespace %s", managedByClusterArgocdVal, namespace.Name)) continue } @@ -261,7 +261,7 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex // Update namespace with managed-by-cluster-argocd label namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace if err := r.Client.Update(ctx, namespace); err != nil { - log.Error(err, fmt.Sprintf("failed to add label from namespace [%s]", namespace.Name)) + return fmt.Errorf("failed to add label from namespace [%s]: %w", namespace.Name, err) } // if the Rules differ, update the Role if !reflect.DeepEqual(existingRole.Rules, role.Rules) { diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index 0a12e15ed..e69e5b11d 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -888,7 +888,7 @@ func (r *ReconcileArgoCD) removeManagedByLabelFromNamespaces(namespace string) e } delete(ns.Labels, common.ArgoCDManagedByLabel) if err := r.Client.Update(context.TODO(), ns); err != nil { - log.Error(err, fmt.Sprintf("failed to remove label from namespace [%s]", ns.Name)) + return fmt.Errorf("failed to remove label from namespace [%s]: %w", ns.Name, err) } } return nil From 1c14100e256bdea052672bb96c69f4c5d1569622 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Fri, 16 Jun 2023 12:00:13 -0400 Subject: [PATCH 08/10] Improved logging with more context and revised some func Signed-off-by: Yi Cai --- controllers/argocd/util.go | 183 ++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 85 deletions(-) diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go index e69e5b11d..9d7731583 100644 --- a/controllers/argocd/util.go +++ b/controllers/argocd/util.go @@ -1183,118 +1183,130 @@ type DeprecationEventEmissionStatus struct { // This is temporary and can be removed in v0.0.6 when we remove the deprecated fields. var DeprecationEventEmissionTracker = make(map[string]DeprecationEventEmissionStatus) -func namespaceFilterPredicate() predicate.Predicate { - return predicate.Funcs{ - UpdateFunc: func(e event.UpdateEvent) bool { - // This checks if ArgoCDManagedByLabel exists in newMeta, if exists then - - // 1. Check if oldMeta had the label or not? if no, return true - // 2. if yes, check if the old and new values are different, if yes, - // first deleteRBACs for the old value & return true. - // Event is then handled by the reconciler, which would create appropriate RBACs. - if valNew, ok := e.ObjectNew.GetLabels()[common.ArgoCDManagedByLabel]; ok { - if valOld, ok := e.ObjectOld.GetLabels()[common.ArgoCDManagedByLabel]; ok && valOld != valNew { - k8sClient, err := initK8sClient() - if err != nil { - return false - } - if err := deleteRBACsForNamespace(e.ObjectOld.GetName(), k8sClient); err != nil { - log.Error(err, fmt.Sprintf("failed to delete RBACs for namespace: %s", e.ObjectOld.GetName())) - } else { - log.Info(fmt.Sprintf("Successfully removed the RBACs for namespace: %s", e.ObjectOld.GetName())) - } +// responds to an event where a namespace's labels have been updated +func handleUpdate(e event.UpdateEvent) bool { + // Check if the new object has the ArgoCDManagedByLabel + if managedByLabelNewVal, isNewLabeled := e.ObjectNew.GetLabels()[common.ArgoCDManagedByLabel]; isNewLabeled { + k8sClient, err := initK8sClient() + if err != nil { + log.Error(err, "Failed to initialize k8s client in handleUpdate, when the new object has the ArgoCDManagedByLabel") + return false + } + if managedByLabelOldVal, isOldLabeled := e.ObjectOld.GetLabels()[common.ArgoCDManagedByLabel]; isOldLabeled && managedByLabelOldVal != managedByLabelNewVal { + err := handleNamespaceLabelChange(e.ObjectOld.GetName(), managedByLabelOldVal, k8sClient) + return err == nil + } + return true + } - // Delete namespace from cluster secret of previously managing argocd instance - if err = deleteManagedNamespaceFromClusterSecret(valOld, e.ObjectOld.GetName(), k8sClient); err != nil { - log.Error(err, fmt.Sprintf("unable to delete namespace %s from cluster secret", e.ObjectOld.GetName())) - } else { - log.Info(fmt.Sprintf("Successfully deleted namespace %s from cluster secret", e.ObjectOld.GetName())) - } - } - return true - } - // This checks if the old meta had the label, if it did, delete the RBACs for the namespace - // which were created when the label was added to the namespace. - if ns, ok := e.ObjectOld.GetLabels()[common.ArgoCDManagedByLabel]; ok && ns != "" { - k8sClient, err := initK8sClient() - if err != nil { - return false - } - if err := deleteRBACsForNamespace(e.ObjectOld.GetName(), k8sClient); err != nil { - log.Error(err, fmt.Sprintf("failed to delete RBACs for namespace: %s", e.ObjectOld.GetName())) - } else { - log.Info(fmt.Sprintf("Successfully removed the RBACs for namespace: %s", e.ObjectOld.GetName())) - } + // Check if the old object had the label but the new one doesn't + if managedByLabelOldVal, isOldLabeled := e.ObjectOld.GetLabels()[common.ArgoCDManagedByLabel]; isOldLabeled && managedByLabelOldVal != "" { + k8sClient, err := initK8sClient() + if err != nil { + log.Error(err, "Failed to initialize k8s client in handleUpdate, when the old object had the label but the new one doesn't") + return false + } + err = handleNamespaceLabelChange(e.ObjectOld.GetName(), managedByLabelOldVal, k8sClient) + return err == nil + } - // Delete managed namespace from cluster secret - if err = deleteManagedNamespaceFromClusterSecret(ns, e.ObjectOld.GetName(), k8sClient); err != nil { - log.Error(err, fmt.Sprintf("unable to delete namespace %s from cluster secret", e.ObjectOld.GetName())) - } else { - log.Info(fmt.Sprintf("Successfully deleted namespace %s from cluster secret", e.ObjectOld.GetName())) - } + return false +} - } +func handleDelete(e event.DeleteEvent) bool { + // check if the object has the ArgoCDManagedByLabel + if managedByLabelVal, ok := e.Object.GetLabels()[common.ArgoCDManagedByLabel]; ok && managedByLabelVal != "" { + k8sClient, err := initK8sClient() + if err != nil { + log.Error(err, "Failed to initialize k8s client in handleDelete", "event", e) return false - }, - DeleteFunc: func(e event.DeleteEvent) bool { - if ns, ok := e.Object.GetLabels()[common.ArgoCDManagedByLabel]; ok && ns != "" { - k8sClient, err := initK8sClient() + } + err = handleNamespaceDeletion(e.Object.GetName(), managedByLabelVal, k8sClient) + return err == nil + } - if err != nil { - return false - } - // Delete managed namespace from cluster secret - err = deleteManagedNamespaceFromClusterSecret(ns, e.Object.GetName(), k8sClient) - if err != nil { - log.Error(err, fmt.Sprintf("unable to delete namespace %s from cluster secret", e.Object.GetName())) - } else { - log.Info(fmt.Sprintf("Successfully deleted namespace %s from cluster secret", e.Object.GetName())) - } - } + // if a namespace is deleted, remove it from deprecationEventEmissionTracker (if exists) so that if a namespace with the same name + // is created in the future and contains an Argo CD instance, it will be tracked appropriately + delete(DeprecationEventEmissionTracker, e.Object.GetName()) - // if a namespace is deleted, remove it from deprecationEventEmissionTracker (if exists) so that if a namespace with the same name - // is created in the future and contains an Argo CD instance, it will be tracked appropriately - delete(DeprecationEventEmissionTracker, e.Object.GetName()) + return false +} - return false - }, +// returns a Predicate that filters events based on changes to a namespace's ArgoCDManagedByLabel +func namespaceFilterPredicate() predicate.Predicate { + return predicate.Funcs{ + UpdateFunc: handleUpdate, + DeleteFunc: handleDelete, + } +} + +func handleNamespaceLabelChange(namespace, labelValue string, k8sClient kubernetes.Interface) error { + if err := deleteRBACsForNamespace(namespace, k8sClient); err != nil { + log.Error(err, fmt.Sprintf("failed to delete RBACs for namespace: %s", namespace)) + return err + } + log.Info(fmt.Sprintf("Successfully removed the RBACs for namespace: %s", namespace)) + + // Delete namespace from cluster secret of previously managing argocd instance + if err := deleteManagedNamespaceFromClusterSecret(labelValue, namespace, k8sClient); err != nil { + log.Error(err, fmt.Sprintf("unable to delete namespace %s from cluster secret", namespace)) + return err + } + log.Info(fmt.Sprintf("Successfully deleted namespace %s from cluster secret", namespace)) + + return nil +} + +func handleNamespaceDeletion(namespace, labelValue string, k8sClient kubernetes.Interface) error { + err := deleteManagedNamespaceFromClusterSecret(labelValue, namespace, k8sClient) + if err != nil { + log.Error(err, fmt.Sprintf("unable to delete namespace %s from cluster secret", namespace)) + return err } + log.Info(fmt.Sprintf("Successfully deleted namespace %s from cluster secret", namespace)) + + return nil } // deleteRBACsForNamespace deletes the RBACs when the label from the namespace is removed. func deleteRBACsForNamespace(sourceNS string, k8sClient kubernetes.Interface) error { log.Info(fmt.Sprintf("Removing the RBACs created for the namespace: %s", sourceNS)) - // List all the roles created for ArgoCD using the label selector labelSelector := metav1.LabelSelector{MatchLabels: map[string]string{common.ArgoCDKeyPartOf: common.ArgoCDAppName}} - roles, err := k8sClient.RbacV1().Roles(sourceNS).List(context.TODO(), metav1.ListOptions{LabelSelector: labels.Set(labelSelector.MatchLabels).String()}) + listOptions := metav1.ListOptions{LabelSelector: labels.Set(labelSelector.MatchLabels).String()} + var allErrors []string + + // List and delete all the roles + roles, err := k8sClient.RbacV1().Roles(sourceNS).List(context.TODO(), listOptions) if err != nil { log.Error(err, fmt.Sprintf("failed to list roles for namespace: %s", sourceNS)) return err } - - // Delete all the retrieved roles for _, role := range roles.Items { - err = k8sClient.RbacV1().Roles(sourceNS).Delete(context.TODO(), role.Name, metav1.DeleteOptions{}) - if err != nil { - log.Error(err, fmt.Sprintf("failed to delete roles for namespace: %s", sourceNS)) + if err = k8sClient.RbacV1().Roles(sourceNS).Delete(context.TODO(), role.Name, metav1.DeleteOptions{}); err != nil { + log.Error(err, fmt.Sprintf("failed to delete role for namespace: %s", sourceNS)) + allErrors = append(allErrors, err.Error()) } } - // List all the roles bindings created for ArgoCD using the label selector - roleBindings, err := k8sClient.RbacV1().RoleBindings(sourceNS).List(context.TODO(), metav1.ListOptions{LabelSelector: labels.Set(labelSelector.MatchLabels).String()}) + // List and delete all the role bindings + roleBindings, err := k8sClient.RbacV1().RoleBindings(sourceNS).List(context.TODO(), listOptions) if err != nil { log.Error(err, fmt.Sprintf("failed to list role bindings for namespace: %s", sourceNS)) return err } - - // Delete all the retrieved role bindings for _, roleBinding := range roleBindings.Items { - err = k8sClient.RbacV1().RoleBindings(sourceNS).Delete(context.TODO(), roleBinding.Name, metav1.DeleteOptions{}) - if err != nil { + if err = k8sClient.RbacV1().RoleBindings(sourceNS).Delete(context.TODO(), roleBinding.Name, metav1.DeleteOptions{}); err != nil { log.Error(err, fmt.Sprintf("failed to delete role binding for namespace: %s", sourceNS)) + allErrors = append(allErrors, err.Error()) } } + // If there were any errors, return them + if len(allErrors) > 0 { + return fmt.Errorf(strings.Join(allErrors, "; ")) + } + return nil } @@ -1317,13 +1329,14 @@ func deleteManagedNamespaceFromClusterSecret(ownerNS, sourceNS string, k8sClient for _, n := range namespaceList { // remove the namespace from the list of namespaces - if strings.TrimSpace(n) == sourceNS { - continue + if strings.TrimSpace(n) != sourceNS { + result = append(result, strings.TrimSpace(n)) } - result = append(result, strings.TrimSpace(n)) - sort.Strings(result) - secret.Data["namespaces"] = []byte(strings.Join(result, ",")) } + + sort.Strings(result) + secret.Data["namespaces"] = []byte(strings.Join(result, ",")) + // Update the secret with the updated list of namespaces if _, err = k8sClient.CoreV1().Secrets(ownerNS).Update(context.TODO(), &secret, metav1.UpdateOptions{}); err != nil { log.Error(err, fmt.Sprintf("failed to update cluster permission secret for namespace: %s", ownerNS)) From 32ac431c4f5716d6e0b69b10eccb7af058b9ab98 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Fri, 16 Jun 2023 13:03:50 -0400 Subject: [PATCH 09/10] Refactored function for debugging Signed-off-by: Yi Cai --- controllers/argocd/role.go | 129 ++++++++++++++++++++++--------------- 1 file changed, 76 insertions(+), 53 deletions(-) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index 6e1dc11a6..008266afa 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -199,7 +199,6 @@ func (r *ReconcileArgoCD) reconcileRole(name string, policyRules []v1.PolicyRule } func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx context.Context, name string, policyRules []v1.PolicyRule, cr *argoprojv1a1.ArgoCD) error { - // create policy rules for each source namespace for ArgoCD Server for _, sourceNamespace := range cr.Spec.SourceNamespaces { namespace := &corev1.Namespace{} @@ -209,73 +208,97 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex managedByLabelval, isManagedByLabelPresent := namespace.Labels[common.ArgoCDManagedByLabel] managedByClusterArgocdVal, isManagedByClusterArgoCDLabelPresent := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] - // do not reconcile roles for namespaces already containing managed-by label - // as it already contains roles with permissions to manipulate application resources - // reconciled during reconcilation of ManagedNamespaces if isManagedByLabelPresent && managedByLabelval != "" { - log.Info(fmt.Sprintf("Skipping reconciling resources for namespace %s as it is already managed-by namespace %s.", namespace.Name, managedByLabelval)) - // if managed-by-cluster-argocd label is also present, remove the namespace from the ManagedSourceNamespaces. + log.Info("Namespace is already managed by another namespace, skipping", "namespace", namespace.Name, "managedBy", managedByLabelval) if isManagedByClusterArgoCDLabelPresent && managedByClusterArgocdVal == cr.Namespace { delete(r.ManagedSourceNamespaces, namespace.Name) if err := r.cleanupUnmanagedSourceNamespaceResources(ctx, cr, namespace.Name); err != nil { - log.Error(err, fmt.Sprintf("error cleaning up resources for namespace %s", namespace.Name)) + log.Error(err, "Error cleaning up resources", "namespace", namespace.Name) } } - } else { - log.Info(fmt.Sprintf("Reconciling role for %s", namespace.Name)) + continue + } - role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) - if err := applyReconcilerHook(cr, role, ""); err != nil { - return fmt.Errorf("failed to apply reconciler hook for %s: %w", name, err) - } - role.Namespace = namespace.Name - existingRole := v1.Role{} - err := r.Client.Get(ctx, types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, &existingRole) - if err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to reconcile the role for the service account associated with %s: %w", name, err) - } + log.Info("Reconciling role", "namespace", namespace.Name) - // do not reconcile roles for namespaces already containing managed-by-cluster-argocd label - // as it already contains roles reconciled during reconcilation of ManagedNamespaces - if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; ok { - // If sourceNamespace includes the name but role is missing in the namespace, create the role - if reflect.DeepEqual(existingRole, v1.Role{}) { - log.Info(fmt.Sprintf("creating role %s for Argo CD instance %s in namespace %s", role.Name, cr.Name, namespace)) - if err := r.Client.Create(ctx, role); err != nil { - return fmt.Errorf("failed to create role for %s: %w", name, err) - } - } - continue - } + role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) + if err := applyReconcilerHook(cr, role, ""); err != nil { + return fmt.Errorf("failed to apply reconciler hook for %s: %w", name, err) + } + role.Namespace = namespace.Name - // reconcile roles only if another ArgoCD instance is not already set as value for managed-by-cluster-argocd label - if isManagedByClusterArgoCDLabelPresent && managedByClusterArgocdVal != "" { - log.Info(fmt.Sprintf("Namespace already has label set to argocd instance %s. Thus, skipping namespace %s", managedByClusterArgocdVal, namespace.Name)) - continue + if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; ok { + if err := r.createRoleIfNotExists(ctx, role, namespace.Name, cr.Name); err != nil { + return err } + continue + } - // Get the latest value of namespace before updating it - if err := r.Client.Get(ctx, types.NamespacedName{Name: namespace.Name}, namespace); err != nil { - return fmt.Errorf("failed to get latest namespace for %s: %w", name, err) - } - // Update namespace with managed-by-cluster-argocd label - namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace - if err := r.Client.Update(ctx, namespace); err != nil { - return fmt.Errorf("failed to add label from namespace [%s]: %w", namespace.Name, err) - } - // if the Rules differ, update the Role - if !reflect.DeepEqual(existingRole.Rules, role.Rules) { - existingRole.Rules = role.Rules - if err := r.Client.Update(ctx, &existingRole); err != nil { - return fmt.Errorf("failed to update role for %s: %w", name, err) - } - } + if isManagedByClusterArgoCDLabelPresent && managedByClusterArgocdVal != "" { + log.Info("Namespace already has label set to another ArgoCD instance, skipping", "namespace", namespace.Name, "ArgoCD", managedByClusterArgocdVal) + continue + } - if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; !ok { - r.ManagedSourceNamespaces[sourceNamespace] = "" + if err := r.createOrUpdateRole(ctx, role, namespace, cr); err != nil { + return err + } + + if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; !ok { + r.ManagedSourceNamespaces[sourceNamespace] = "" + } + } + return nil +} + +func (r *ReconcileArgoCD) createRoleIfNotExists(ctx context.Context, role *v1.Role, namespace, crName string) error { + existingRole := &v1.Role{} + err := r.Client.Get(ctx, types.NamespacedName{Name: role.Name, Namespace: namespace}, existingRole) + switch { + case err == nil: + log.Info("Role already exists, skipping creation", "role", role.Name, "namespace", namespace) + case errors.IsNotFound(err): + log.Info("Creating new role", "role", role.Name, "namespace", namespace, "ArgoCD", crName) + if err := r.Client.Create(ctx, role); err != nil { + log.Error(err, "Failed to create role", "role", role.Name, "namespace", namespace) + return err + } + default: + log.Error(err, "Failed to get role", "role", role.Name, "namespace", namespace) + return err + } + return nil +} + +func (r *ReconcileArgoCD) createOrUpdateRole(ctx context.Context, role *v1.Role, namespace *corev1.Namespace, cr *argoprojv1a1.ArgoCD) error { + existingRole := &v1.Role{} + err := r.Client.Get(ctx, types.NamespacedName{Name: role.Name, Namespace: namespace.Name}, existingRole) + switch { + case err == nil: + if !reflect.DeepEqual(existingRole.Rules, role.Rules) { + existingRole.Rules = role.Rules + if err := r.Client.Update(ctx, existingRole); err != nil { + log.Error(err, "Failed to update role", "role", role.Name, "namespace", namespace.Name) + return err } + log.Info("Role rules updated successfully", "role", role.Name, "namespace", namespace.Name) + } + case errors.IsNotFound(err): + log.Info("Creating new role", "role", role.Name, "namespace", namespace.Name, "ArgoCD", cr.Name) + if err := r.Client.Create(ctx, role); err != nil { + log.Error(err, "Failed to create role", "role", role.Name, "namespace", namespace.Name) + return err } + default: + log.Error(err, "Failed to get role", "role", role.Name, "namespace", namespace.Name) + return err + } + + namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace + if err := r.Client.Update(ctx, namespace); err != nil { + log.Error(err, "Failed to update namespace label", "namespace", namespace.Name) + return err } + log.Info("Namespace label updated successfully", "namespace", namespace.Name) return nil } From 75e61c19f9890db96317244013441c22b2d258d9 Mon Sep 17 00:00:00 2001 From: Yi Cai Date: Fri, 16 Jun 2023 18:19:03 -0400 Subject: [PATCH 10/10] Added comments Signed-off-by: Yi Cai --- controllers/argocd/role.go | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index 008266afa..0e1fe076c 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -208,41 +208,54 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(ctx contex managedByLabelval, isManagedByLabelPresent := namespace.Labels[common.ArgoCDManagedByLabel] managedByClusterArgocdVal, isManagedByClusterArgoCDLabelPresent := namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] + + // If namespace has managed-by label, skip reconciling roles for namespace as it already contains roles with permissions to + // manipulate application resources reconciled in the reconcilation of ManagedNamespaces if isManagedByLabelPresent && managedByLabelval != "" { log.Info("Namespace is already managed by another namespace, skipping", "namespace", namespace.Name, "managedBy", managedByLabelval) + // If namespace also has managed-by-cluster-argocd label and matches Argo CD instance, clean up the resources if isManagedByClusterArgoCDLabelPresent && managedByClusterArgocdVal == cr.Namespace { + // Remove the namespace from ManagedSourceNamespace delete(r.ManagedSourceNamespaces, namespace.Name) if err := r.cleanupUnmanagedSourceNamespaceResources(ctx, cr, namespace.Name); err != nil { log.Error(err, "Error cleaning up resources", "namespace", namespace.Name) } } + + // Skip the rest of the loop and continue with the next namespace continue } log.Info("Reconciling role", "namespace", namespace.Name) - role := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) - if err := applyReconcilerHook(cr, role, ""); err != nil { + expectedRole := newRoleForApplicationSourceNamespaces(namespace.Name, policyRules, cr) + if err := applyReconcilerHook(cr, expectedRole, ""); err != nil { return fmt.Errorf("failed to apply reconciler hook for %s: %w", name, err) } - role.Namespace = namespace.Name + expectedRole.Namespace = namespace.Name + // Check if the namespace is managed by current ArgoCD instance with managed-by-cluster-argocd label if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; ok { - if err := r.createRoleIfNotExists(ctx, role, namespace.Name, cr.Name); err != nil { + // If it's managed by current ArgoCD instance, create role if does not exist + if err := r.createRoleIfNotExists(ctx, expectedRole, namespace.Name, cr.Name); err != nil { return err } + // Continue to the next sourceNamespace in the loop continue } + // Check if another ArgoCD instance is already set as value of managed-by-cluster-argocd label if isManagedByClusterArgoCDLabelPresent && managedByClusterArgocdVal != "" { - log.Info("Namespace already has label set to another ArgoCD instance, skipping", "namespace", namespace.Name, "ArgoCD", managedByClusterArgocdVal) + log.Info("Namespace already has managed-by-cluster-argocd label set to another ArgoCD instance, skipping", "namespace", namespace.Name, "ArgoCD", managedByClusterArgocdVal) continue } - if err := r.createOrUpdateRole(ctx, role, namespace, cr); err != nil { + // Create/Update role and update the namespace label after successfully reconciled role + if err := r.createOrUpdateRole(ctx, expectedRole, namespace, cr); err != nil { return err } + // Save sourceNamespace to ManagedSourceNamespaces map as sourceNamespace label has been updated successfully if _, ok := r.ManagedSourceNamespaces[sourceNamespace]; !ok { r.ManagedSourceNamespaces[sourceNamespace] = "" } @@ -293,6 +306,7 @@ func (r *ReconcileArgoCD) createOrUpdateRole(ctx context.Context, role *v1.Role, return err } + // Update namespace with managed-by-cluster-argocd label as required role has been successfully created/updated namespace.Labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace if err := r.Client.Update(ctx, namespace); err != nil { log.Error(err, "Failed to update namespace label", "namespace", namespace.Name)