From 4047a69a713086b0ded0bcafc30c428660145647 Mon Sep 17 00:00:00 2001 From: Jayendra Parsai Date: Fri, 24 May 2024 19:00:52 +0530 Subject: [PATCH] feat: Add support for custom ClusterRoles (controller,server) for cluster scoped instances (#1357) * feat: Enable use of alternate cluster roles for cluster scoped instances Signed-off-by: Jayendra Parsai * Update role/binding unit tests, add a new line to the doc Signed-off-by: Jonathan West --------- Signed-off-by: Jayendra Parsai Signed-off-by: Jonathan West Co-authored-by: Jayendra Parsai Co-authored-by: Jonathan West --- api/v1alpha1/argocd_conversion.go | 2 + api/v1alpha1/argocd_types.go | 3 + api/v1beta1/argocd_types.go | 3 + bundle/manifests/argoproj.io_argocds.yaml | 8 +++ config/crd/bases/argoproj.io_argocds.yaml | 8 +++ controllers/argocd/configmap_test.go | 2 +- controllers/argocd/role.go | 19 ++++++ controllers/argocd/role_test.go | 61 +++++++++++++++++++ controllers/argocd/rolebinding.go | 17 ++++++ controllers/argocd/rolebinding_test.go | 52 ++++++++++++++++ controllers/argocd/statefulset.go | 4 +- controllers/argocd/util_test.go | 8 +-- .../0.9.0/argoproj.io_argocds.yaml | 8 +++ docs/usage/custom_roles.md | 25 +++++++- 14 files changed, 212 insertions(+), 8 deletions(-) diff --git a/api/v1alpha1/argocd_conversion.go b/api/v1alpha1/argocd_conversion.go index a483d6851..87c0fd290 100644 --- a/api/v1alpha1/argocd_conversion.go +++ b/api/v1alpha1/argocd_conversion.go @@ -93,6 +93,7 @@ func (src *ArgoCD) ConvertTo(dstRaw conversion.Hub) error { dst.Spec.UsersAnonymousEnabled = src.Spec.UsersAnonymousEnabled dst.Spec.Version = src.Spec.Version dst.Spec.Banner = (*v1beta1.Banner)(src.Spec.Banner) + dst.Spec.DefaultClusterScopedRoleDisabled = src.Spec.DefaultClusterScopedRoleDisabled // Status conversion dst.Status = v1beta1.ArgoCDStatus(src.Status) @@ -160,6 +161,7 @@ func (dst *ArgoCD) ConvertFrom(srcRaw conversion.Hub) error { dst.Spec.UsersAnonymousEnabled = src.Spec.UsersAnonymousEnabled dst.Spec.Version = src.Spec.Version dst.Spec.Banner = (*Banner)(src.Spec.Banner) + dst.Spec.DefaultClusterScopedRoleDisabled = src.Spec.DefaultClusterScopedRoleDisabled // Status conversion dst.Status = ArgoCDStatus(src.Status) diff --git a/api/v1alpha1/argocd_types.go b/api/v1alpha1/argocd_types.go index 2a1403e01..5d715e80c 100644 --- a/api/v1alpha1/argocd_types.go +++ b/api/v1alpha1/argocd_types.go @@ -824,6 +824,9 @@ type ArgoCDSpec struct { // Deprecated field. Support dropped in v1beta1 version. // Dex defines the Dex server options for ArgoCD. Dex *ArgoCDDexSpec `json:"dex,omitempty"` + + // DefaultClusterScopedRoleDisabled will disable creation of default ClusterRoles for a cluster scoped instance. + DefaultClusterScopedRoleDisabled bool `json:"defaultClusterScopedRoleDisabled,omitempty"` } // ArgoCDStatus defines the observed state of ArgoCD diff --git a/api/v1beta1/argocd_types.go b/api/v1beta1/argocd_types.go index 55f477912..d84996b4c 100644 --- a/api/v1beta1/argocd_types.go +++ b/api/v1beta1/argocd_types.go @@ -855,6 +855,9 @@ type ArgoCDSpec struct { // Banner defines an additional banner to be displayed in Argo CD UI Banner *Banner `json:"banner,omitempty"` + + // DefaultClusterScopedRoleDisabled will disable creation of default ClusterRoles for a cluster scoped instance. + DefaultClusterScopedRoleDisabled bool `json:"defaultClusterScopedRoleDisabled,omitempty"` } // ArgoCDStatus defines the observed state of ArgoCD diff --git a/bundle/manifests/argoproj.io_argocds.yaml b/bundle/manifests/argoproj.io_argocds.yaml index 111186987..90b52f0c3 100644 --- a/bundle/manifests/argoproj.io_argocds.yaml +++ b/bundle/manifests/argoproj.io_argocds.yaml @@ -630,6 +630,10 @@ spec: type: integer type: object type: object + defaultClusterScopedRoleDisabled: + description: DefaultClusterScopedRoleDisabled will disable creation + of default ClusterRoles for a cluster scoped instance. + type: boolean dex: description: Deprecated field. Support dropped in v1beta1 version. Dex defines the Dex server options for ArgoCD. @@ -7593,6 +7597,10 @@ spec: type: integer type: object type: object + defaultClusterScopedRoleDisabled: + description: DefaultClusterScopedRoleDisabled will disable creation + of default ClusterRoles for a cluster scoped instance. + type: boolean disableAdmin: description: DisableAdmin will disable the admin user. type: boolean diff --git a/config/crd/bases/argoproj.io_argocds.yaml b/config/crd/bases/argoproj.io_argocds.yaml index daa950122..74091cf65 100644 --- a/config/crd/bases/argoproj.io_argocds.yaml +++ b/config/crd/bases/argoproj.io_argocds.yaml @@ -621,6 +621,10 @@ spec: type: integer type: object type: object + defaultClusterScopedRoleDisabled: + description: DefaultClusterScopedRoleDisabled will disable creation + of default ClusterRoles for a cluster scoped instance. + type: boolean dex: description: Deprecated field. Support dropped in v1beta1 version. Dex defines the Dex server options for ArgoCD. @@ -7584,6 +7588,10 @@ spec: type: integer type: object type: object + defaultClusterScopedRoleDisabled: + description: DefaultClusterScopedRoleDisabled will disable creation + of default ClusterRoles for a cluster scoped instance. + type: boolean disableAdmin: description: DisableAdmin will disable the admin user. type: boolean diff --git a/controllers/argocd/configmap_test.go b/controllers/argocd/configmap_test.go index a928f6ef6..79f2c4eb6 100644 --- a/controllers/argocd/configmap_test.go +++ b/controllers/argocd/configmap_test.go @@ -94,7 +94,7 @@ func TestReconcileArgoCD_reconcileTLSCerts_configMapUpdate(t *testing.T) { } // update a new cert in argocd-tls-certs-cm - testPEM := generateEncodedPEM(t, "example.com") + testPEM := generateEncodedPEM(t) configMap.Data["example.com"] = string(testPEM) assert.NoError(t, r.Client.Update(context.TODO(), configMap)) diff --git a/controllers/argocd/role.go b/controllers/argocd/role.go index b2394081b..317783a9e 100644 --- a/controllers/argocd/role.go +++ b/controllers/argocd/role.go @@ -280,10 +280,29 @@ func (r *ReconcileArgoCD) reconcileRoleForApplicationSourceNamespaces(name strin } func (r *ReconcileArgoCD) reconcileClusterRole(name string, policyRules []v1.PolicyRule, cr *argoproj.ArgoCD) (*v1.ClusterRole, error) { + allowed := false if allowedNamespace(cr.Namespace, os.Getenv("ARGOCD_CLUSTER_CONFIG_NAMESPACES")) { allowed = true } + + // Check if it is cluster-scoped instance namespace and user doesn't want to use default ClusterRole + if allowed && cr.Spec.DefaultClusterScopedRoleDisabled { + + // In case DefaultClusterScopedRoleDisabled was false earlier and default ClusterRole was created, then delete it. + existingClusterRole := &v1.ClusterRole{} + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: GenerateUniqueResourceName(name, cr)}, existingClusterRole); err == nil { + + // Default ClusterRole exists, now delete it + if err := r.Client.Delete(context.TODO(), existingClusterRole); err != nil { + return nil, fmt.Errorf("failed to delete existing cluster role for the service account associated with %s : %s", name, err) + } + } + + // Don't create a default ClusterRole + return nil, nil + } + clusterRole := newClusterRole(name, policyRules, cr) if err := applyReconcilerHook(cr, clusterRole, ""); err != nil { return nil, err diff --git a/controllers/argocd/role_test.go b/controllers/argocd/role_test.go index d7bb9a146..5dc49cda2 100644 --- a/controllers/argocd/role_test.go +++ b/controllers/argocd/role_test.go @@ -151,6 +151,67 @@ func TestReconcileArgoCD_reconcileClusterRole(t *testing.T) { assert.Contains(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: clusterRoleName}, reconciledClusterRole).Error(), "not found") } +func TestReconcileArgoCD_reconcileClusterRole_disabled(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD() + + resObjs := []client.Object{a} + subresObjs := []client.Object{a} + runtimeObjs := []runtime.Object{} + sch := makeTestReconcilerScheme(argoproj.AddToScheme) + cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs) + r := makeTestReconciler(cl, sch) + + workloadIdentifier := common.ArgoCDApplicationControllerComponent + clusterRoleName := GenerateUniqueResourceName(workloadIdentifier, a) + expectedRules := policyRuleForApplicationController() + + // Set the namespace to be cluster-scoped + t.Setenv("ARGOCD_CLUSTER_CONFIG_NAMESPACES", a.Namespace) + + // Disable creation of default ClusterRole + a.Spec.DefaultClusterScopedRoleDisabled = true + + err := cl.Update(context.Background(), a) + assert.NoError(t, err) + + // Reconcile ClusterRole + _, err = r.reconcileClusterRole(workloadIdentifier, expectedRules, a) + assert.NoError(t, err) + + // Ensure default ClusterRole is not created + reconciledClusterRole := &v1.ClusterRole{} + err = r.Client.Get(context.TODO(), types.NamespacedName{Name: clusterRoleName}, reconciledClusterRole) + assert.Error(t, err) + assert.ErrorContains(t, err, "not found") + + // Now enable creation of default ClusterRole + a.Spec.DefaultClusterScopedRoleDisabled = false + err = cl.Update(context.Background(), a) + assert.NoError(t, err) + + // Again reconcile ClusterRole + _, err = r.reconcileClusterRole(workloadIdentifier, expectedRules, a) + assert.NoError(t, err) + + // Ensure default ClusterRole is created now + assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: clusterRoleName}, reconciledClusterRole)) + + // Once again disable creation of default ClusterRole + a.Spec.DefaultClusterScopedRoleDisabled = true + err = cl.Update(context.Background(), a) + assert.NoError(t, err) + + // Once again reconcile ClusterRole + _, err = r.reconcileClusterRole(workloadIdentifier, expectedRules, a) + assert.NoError(t, err) + + // Ensure default ClusterRole is deleted again + err = r.Client.Get(context.TODO(), types.NamespacedName{Name: clusterRoleName}, reconciledClusterRole) + assert.Error(t, err) + assert.ErrorContains(t, err, "not found") +} + func TestReconcileArgoCD_reconcileRoleForApplicationSourceNamespaces(t *testing.T) { logf.SetLogger(ZapLogger(true)) sourceNamespace := "newNamespaceTest" diff --git a/controllers/argocd/rolebinding.go b/controllers/argocd/rolebinding.go index 4e70dadc8..e652f91b5 100644 --- a/controllers/argocd/rolebinding.go +++ b/controllers/argocd/rolebinding.go @@ -340,6 +340,23 @@ func newRoleBindingWithNameForApplicationSourceNamespaces(namespace string, cr * func (r *ReconcileArgoCD) reconcileClusterRoleBinding(name string, role *v1.ClusterRole, cr *argoproj.ArgoCD) error { + // Check if user doesn't want to use default ClusterRole, hence default ClusterRoleBinding is also not required + if cr.Spec.DefaultClusterScopedRoleDisabled { + + // In case DefaultClusterScopedRoleDisabled was false earlier and default ClusterRoleBinding was created, then delete it. + existingClusterRoleBinding := &v1.ClusterRoleBinding{} + if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: GenerateUniqueResourceName(name, cr)}, existingClusterRoleBinding); err == nil { + + // Default ClusterRoleBinding exists, now delete it + if err := r.Client.Delete(context.TODO(), existingClusterRoleBinding); err != nil { + return fmt.Errorf("failed to delete existing cluster role binding for the service account associated with %s : %s", name, err) + } + } + + // Don't create a default ClusterRoleBinding + return nil + } + // get expected name roleBinding := newClusterRoleBindingWithname(name, cr) // fetch existing rolebinding by name diff --git a/controllers/argocd/rolebinding_test.go b/controllers/argocd/rolebinding_test.go index 4a66cfdf8..da6c1c132 100644 --- a/controllers/argocd/rolebinding_test.go +++ b/controllers/argocd/rolebinding_test.go @@ -184,6 +184,58 @@ func TestReconcileArgoCD_reconcileClusterRoleBinding(t *testing.T) { assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName}, clusterRoleBinding)) } +func TestReconcileArgoCD_reconcileClusterRoleBinding_disabled(t *testing.T) { + logf.SetLogger(ZapLogger(true)) + a := makeTestArgoCD() + resObjs := []client.Object{a} + subresObjs := []client.Object{a} + runtimeObjs := []runtime.Object{} + sch := makeTestReconcilerScheme(argoproj.AddToScheme) + cl := makeTestReconcilerClient(sch, resObjs, subresObjs, runtimeObjs) + r := makeTestReconciler(cl, sch) + workloadIdentifier := "x" + expectedClusterRole := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: workloadIdentifier}} + + // Disable creation of default ClusterRole, hence RoleBinding won't be created either. + a.Spec.DefaultClusterScopedRoleDisabled = true + err := cl.Update(context.Background(), a) + assert.NoError(t, err) + + // Reconcile ClusterRoleBinding + assert.NoError(t, r.reconcileClusterRoleBinding(workloadIdentifier, expectedClusterRole, a)) + + // Ensure default ClusterRoleBinding is not created + clusterRoleBinding := &rbacv1.ClusterRoleBinding{} + expectedName := fmt.Sprintf("%s-%s-%s", a.Name, a.Namespace, workloadIdentifier) + err = r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName}, clusterRoleBinding) + assert.Error(t, err) + assert.ErrorContains(t, err, "not found") + + // Now enable creation of default ClusterRole, hence RoleBinding should be created aw well. + a.Spec.DefaultClusterScopedRoleDisabled = false + err = cl.Update(context.Background(), a) + assert.NoError(t, err) + + // Again reconcile ClusterRoleBinding + assert.NoError(t, r.reconcileClusterRoleBinding(workloadIdentifier, expectedClusterRole, a)) + + // Ensure default ClusterRoleBinding is created now + assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName}, clusterRoleBinding)) + + // Once again disable creation of default ClusterRole + a.Spec.DefaultClusterScopedRoleDisabled = true + err = cl.Update(context.Background(), a) + assert.NoError(t, err) + + // Once again reconcile ClusterRoleBinding + assert.NoError(t, r.reconcileClusterRoleBinding(workloadIdentifier, expectedClusterRole, a)) + + // Ensure default ClusterRoleBinding is deleted again + err = r.Client.Get(context.TODO(), types.NamespacedName{Name: expectedName}, clusterRoleBinding) + assert.Error(t, err) + assert.ErrorContains(t, err, "not found") +} + func TestReconcileArgoCD_reconcileRoleBinding_custom_role(t *testing.T) { logf.SetLogger(ZapLogger(true)) a := makeTestArgoCD() diff --git a/controllers/argocd/statefulset.go b/controllers/argocd/statefulset.go index 78a26103e..c017e9aa6 100644 --- a/controllers/argocd/statefulset.go +++ b/controllers/argocd/statefulset.go @@ -33,7 +33,7 @@ import ( "github.com/argoproj-labs/argocd-operator/controllers/argoutil" ) -func getRedisHAReplicas(cr *argoproj.ArgoCD) *int32 { +func getRedisHAReplicas() *int32 { replicas := common.ArgoCDDefaultRedisHAReplicas // TODO: Allow override of this value through CR? return &replicas @@ -107,7 +107,7 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoproj.ArgoCD) error { }) ss.Spec.PodManagementPolicy = appsv1.OrderedReadyPodManagement - ss.Spec.Replicas = getRedisHAReplicas(cr) + ss.Spec.Replicas = getRedisHAReplicas() ss.Spec.Selector = &metav1.LabelSelector{ MatchLabels: map[string]string{ common.ArgoCDKeyName: nameWithSuffix("redis-ha", cr), diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go index f1f31621b..ff52c9e51 100644 --- a/controllers/argocd/util_test.go +++ b/controllers/argocd/util_test.go @@ -549,8 +549,8 @@ func TestGetArgoApplicationControllerCommand(t *testing.T) { func TestGetArgoApplicationContainerEnv(t *testing.T) { sync60s := []v1.EnvVar{ - v1.EnvVar{Name: "HOME", Value: "/home/argocd", ValueFrom: (*v1.EnvVarSource)(nil)}, - v1.EnvVar{Name: "REDIS_PASSWORD", Value: "", + {Name: "HOME", Value: "/home/argocd", ValueFrom: (*v1.EnvVarSource)(nil)}, + {Name: "REDIS_PASSWORD", Value: "", ValueFrom: &corev1.EnvVarSource{ SecretKeyRef: &corev1.SecretKeySelector{ LocalObjectReference: corev1.LocalObjectReference{ @@ -559,7 +559,7 @@ func TestGetArgoApplicationContainerEnv(t *testing.T) { Key: "admin.password", }, }}, - v1.EnvVar{Name: "ARGOCD_RECONCILIATION_TIMEOUT", Value: "60s", ValueFrom: (*v1.EnvVarSource)(nil)}} + {Name: "ARGOCD_RECONCILIATION_TIMEOUT", Value: "60s", ValueFrom: (*v1.EnvVarSource)(nil)}} cmdTests := []struct { name string @@ -994,7 +994,7 @@ func TestGenerateRandomString(t *testing.T) { assert.Len(t, b, 20) } -func generateEncodedPEM(t *testing.T, host string) []byte { +func generateEncodedPEM(t *testing.T) []byte { key, err := argoutil.NewPrivateKey() assert.NoError(t, err) diff --git a/deploy/olm-catalog/argocd-operator/0.9.0/argoproj.io_argocds.yaml b/deploy/olm-catalog/argocd-operator/0.9.0/argoproj.io_argocds.yaml index 111186987..90b52f0c3 100644 --- a/deploy/olm-catalog/argocd-operator/0.9.0/argoproj.io_argocds.yaml +++ b/deploy/olm-catalog/argocd-operator/0.9.0/argoproj.io_argocds.yaml @@ -630,6 +630,10 @@ spec: type: integer type: object type: object + defaultClusterScopedRoleDisabled: + description: DefaultClusterScopedRoleDisabled will disable creation + of default ClusterRoles for a cluster scoped instance. + type: boolean dex: description: Deprecated field. Support dropped in v1beta1 version. Dex defines the Dex server options for ArgoCD. @@ -7593,6 +7597,10 @@ spec: type: integer type: object type: object + defaultClusterScopedRoleDisabled: + description: DefaultClusterScopedRoleDisabled will disable creation + of default ClusterRoles for a cluster scoped instance. + type: boolean disableAdmin: description: DisableAdmin will disable the admin user. type: boolean diff --git a/docs/usage/custom_roles.md b/docs/usage/custom_roles.md index e11d6c6cc..102de533e 100644 --- a/docs/usage/custom_roles.md +++ b/docs/usage/custom_roles.md @@ -37,4 +37,27 @@ spec: value: custom-controller-role - name: SERVER_CLUSTER_ROLE value: custom-server-role -``` \ No newline at end of file +``` + +## Cluster Scoped Roles + +When the administrative user deploys Argo CD as a cluster scoped instance, the operator creates additional ClusterRoles and ClusterRoleBindings for the +application-controller and server components. These provide the additional permissions that Argo CD requires to operate at the cluster level. + +Specifying alternate ClusterRoles enables the administrative user to add or remove permissions +as needed and have them applied across all cluster scoped instances. For example, features such as the [Auto Respect RBAC For Controller](https://argo-cd.readthedocs.io/en/stable/operator-manual/declarative-setup/#auto-respect-rbac-for-controller) enables specifying more granular permissions for the application-controller service account. + +These customized ClusterRoles need to be created and referred in ClusterRoleBinding by admin. A user can disable creation of default ClusterRoles by setting `ArgoCD.Spec.DefaultClusterScopedRoleDisabled` field to `true`. + +```yaml +apiVersion: argoproj.io/v1alpha1 +kind: ArgoCD +metadata: + name: example-argocd + labels: + example: basic +spec: + defaultClusterScopedRoleDisabled: true +``` + +When `defaultClusterScopedRoleDisabled` is `true`, the default ClusterRole/ClusterRoleBindings for the Argo CD instance will not be created, and the administrative user is free to create and customize these independent of the operator. The field can later be set to `false`, to recreate these resources, if needed. \ No newline at end of file