From f36bc8d77d3710b4fc5330ef9b2c1f7efb1ff76f Mon Sep 17 00:00:00 2001 From: Sam Dowell Date: Wed, 1 Nov 2023 20:19:11 -0700 Subject: [PATCH] feat: implement roleRefs API for RootSyncs This change adds the roleRefs API field to the CRD for both RootSync objects. This API is intended to streamline the management of RBAC bindings for RootSync reconcilers. --- e2e/nomostest/nt.go | 30 ++ e2e/nomostest/testpredicates/predicates.go | 33 ++ e2e/testcases/namespace_repo_test.go | 2 +- e2e/testcases/override_role_refs_test.go | 223 +++++++++++ e2e/testcases/reconciler_manager_test.go | 9 +- e2e/testcases/root_sync_test.go | 2 +- manifests/base/kustomization.yaml | 3 +- ...l => ns-reconciler-base-cluster-role.yaml} | 0 .../root-reconciler-base-cluster-role.yaml | 34 ++ manifests/rootsync-crd.yaml | 89 +++-- .../configsync/v1alpha1/resource_override.go | 34 +- .../v1alpha1/zz_generated.conversion.go | 74 ++-- .../v1alpha1/zz_generated.deepcopy.go | 36 +- .../configsync/v1beta1/resource_override.go | 37 +- .../v1beta1/zz_generated.deepcopy.go | 36 +- pkg/core/meta.go | 7 + pkg/core/names.go | 2 - .../controllers/build_names.go | 32 +- .../controllers/garbage_collector.go | 37 +- .../controllers/reconciler_base.go | 34 ++ .../controllers/reposync_controller.go | 12 +- .../reposync_controller_manager_test.go | 2 +- .../controllers/reposync_controller_test.go | 12 +- .../controllers/rootsync_controller.go | 234 ++++++++++-- .../rootsync_controller_manager_test.go | 4 +- .../controllers/rootsync_controller_test.go | 351 +++++++++++++----- pkg/syncer/syncertest/fake/storage.go | 7 + 27 files changed, 1064 insertions(+), 312 deletions(-) create mode 100644 e2e/testcases/override_role_refs_test.go rename manifests/{ns-reconciler-cluster-role.yaml => ns-reconciler-base-cluster-role.yaml} (100%) create mode 100644 manifests/root-reconciler-base-cluster-role.yaml diff --git a/e2e/nomostest/nt.go b/e2e/nomostest/nt.go index 86aae98f26..1d09f99f90 100644 --- a/e2e/nomostest/nt.go +++ b/e2e/nomostest/nt.go @@ -28,6 +28,7 @@ import ( prometheusv1 "github.com/prometheus/client_golang/api/prometheus/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/labels" "kpt.dev/configsync/e2e/nomostest/gitproviders" "kpt.dev/configsync/e2e/nomostest/ntopts" "kpt.dev/configsync/e2e/nomostest/portforwarder" @@ -38,6 +39,7 @@ import ( "kpt.dev/configsync/e2e/nomostest/testshell" "kpt.dev/configsync/e2e/nomostest/testwatcher" "kpt.dev/configsync/pkg/core" + "kpt.dev/configsync/pkg/reconcilermanager/controllers" "kpt.dev/configsync/pkg/testing/fake" "kpt.dev/configsync/pkg/util" "kpt.dev/configsync/pkg/util/log" @@ -927,3 +929,31 @@ func cloneCloudSourceRepo(nt *NT, repo string) (string, error) { } return cloneDir, nil } + +// ListReconcilerRoleBindings is a convenience method for listing all RoleBindings +// associated with a reconciler. +func (nt *NT) ListReconcilerRoleBindings(syncKind string, rsRef types.NamespacedName) ([]rbacv1.RoleBinding, error) { + opts := &client.ListOptions{} + opts.LabelSelector = client.MatchingLabelsSelector{ + Selector: labels.SelectorFromSet(controllers.ManagedObjectLabelMap(syncKind, rsRef)), + } + rbList := rbacv1.RoleBindingList{} + if err := nt.KubeClient.List(&rbList, opts); err != nil { + return nil, errors.Wrap(err, "listing RoleBindings") + } + return rbList.Items, nil +} + +// ListReconcilerClusterRoleBindings is a convenience method for listing all +// ClusterRoleBindings associated with a reconciler. +func (nt *NT) ListReconcilerClusterRoleBindings(syncKind string, rsRef types.NamespacedName) ([]rbacv1.ClusterRoleBinding, error) { + opts := &client.ListOptions{} + opts.LabelSelector = client.MatchingLabelsSelector{ + Selector: labels.SelectorFromSet(controllers.ManagedObjectLabelMap(syncKind, rsRef)), + } + crbList := rbacv1.ClusterRoleBindingList{} + if err := nt.KubeClient.List(&crbList, opts); err != nil { + return nil, errors.Wrap(err, "listing ClusterRoleBindings") + } + return crbList.Items, nil +} diff --git a/e2e/nomostest/testpredicates/predicates.go b/e2e/nomostest/testpredicates/predicates.go index 7c430badbd..2927883f21 100644 --- a/e2e/nomostest/testpredicates/predicates.go +++ b/e2e/nomostest/testpredicates/predicates.go @@ -1240,3 +1240,36 @@ func ResourceGroupStatusEquals(expected v1alpha1.ResourceGroupStatus) Predicate return nil } } + +func subjectNamesEqual(want []string, got []rbacv1.Subject) error { + if len(got) != len(want) { + return errors.Errorf("want %v subjects; got %v", want, got) + } + + found := make(map[string]bool) + for _, subj := range got { + found[subj.Name] = true + } + for _, name := range want { + if !found[name] { + return errors.Errorf("missing subject %q", name) + } + } + + return nil +} + +// ClusterRoleBindingSubjectNamesEqual checks that the ClusterRoleBinding has a list +// of subjects whose names match the specified list of names. +func ClusterRoleBindingSubjectNamesEqual(subjects ...string) func(o client.Object) error { + return func(o client.Object) error { + if o == nil { + return ErrObjectNotFound + } + r, ok := o.(*rbacv1.ClusterRoleBinding) + if !ok { + return WrongTypeErr(o, r) + } + return subjectNamesEqual(subjects, r.Subjects) + } +} diff --git a/e2e/testcases/namespace_repo_test.go b/e2e/testcases/namespace_repo_test.go index ed787e0ce0..b3afac3b6a 100644 --- a/e2e/testcases/namespace_repo_test.go +++ b/e2e/testcases/namespace_repo_test.go @@ -328,7 +328,7 @@ func checkRepoSyncResourcesNotPresent(nt *nomostest.NT, namespace string, secret return nt.Watcher.WatchForNotFound(kinds.ServiceAccount(), core.NsReconcilerName(namespace, configsync.RepoSyncName), configsync.ControllerNamespace) }) tg.Go(func() error { - return nt.Watcher.WatchForNotFound(kinds.ServiceAccount(), controllers.RepoSyncPermissionsName(), configsync.ControllerNamespace) + return nt.Watcher.WatchForNotFound(kinds.ServiceAccount(), controllers.RepoSyncBaseClusterRoleName, configsync.ControllerNamespace) }) for _, sName := range secretNames { nn := types.NamespacedName{Name: sName, Namespace: configsync.ControllerNamespace} diff --git a/e2e/testcases/override_role_refs_test.go b/e2e/testcases/override_role_refs_test.go new file mode 100644 index 0000000000..65b5147935 --- /dev/null +++ b/e2e/testcases/override_role_refs_test.go @@ -0,0 +1,223 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package e2e + +import ( + "fmt" + "testing" + + "github.com/pkg/errors" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/types" + "kpt.dev/configsync/e2e/nomostest" + "kpt.dev/configsync/e2e/nomostest/ntopts" + "kpt.dev/configsync/e2e/nomostest/taskgroup" + nomostesting "kpt.dev/configsync/e2e/nomostest/testing" + "kpt.dev/configsync/e2e/nomostest/testpredicates" + "kpt.dev/configsync/pkg/api/configsync" + "kpt.dev/configsync/pkg/api/configsync/v1beta1" + "kpt.dev/configsync/pkg/core" + "kpt.dev/configsync/pkg/kinds" + "kpt.dev/configsync/pkg/reconcilermanager/controllers" + "kpt.dev/configsync/pkg/testing/fake" +) + +func TestRootSyncRoleRefs(t *testing.T) { + nt := nomostest.New(t, nomostesting.OverrideAPI, ntopts.Unstructured, + ntopts.RootRepo("sync-a"), + ) + rootSyncA := nomostest.RootSyncObjectV1Beta1FromRootRepo(nt, "sync-a") + syncAReconcilerName := core.RootReconcilerName(rootSyncA.Name) + syncANN := types.NamespacedName{ + Name: rootSyncA.Name, + Namespace: rootSyncA.Namespace, + } + if err := nt.Validate(controllers.RootSyncLegacyClusterRoleBindingName, "", &rbacv1.ClusterRoleBinding{}, + testpredicates.ClusterRoleBindingSubjectNamesEqual(nomostest.DefaultRootReconcilerName, syncAReconcilerName)); err != nil { + nt.T.Fatal(err) + } + nt.T.Logf("Set custom roleRef overrides on RootSync %s", syncANN.Name) + rootSyncA.Spec.SafeOverride().RoleRefs = []v1beta1.RootSyncRoleRef{ + { + Kind: "Role", + Name: "foo-role", + Namespace: "foo", + }, + { + Kind: "ClusterRole", + Name: "foo-role", + }, + { + Kind: "ClusterRole", + Name: "bar-role", + }, + { + Kind: "ClusterRole", + Name: "foo-role", + Namespace: "foo", + }, + } + roleObject := fake.RoleObject(core.Name("foo-role"), core.Namespace("foo")) + clusterRoleObject := fake.ClusterRoleObject(core.Name("foo-role")) + clusterRoleObject.Rules = []rbacv1.PolicyRule{ + { // permission to manage the "safety clusterrole" + Verbs: []string{"*"}, + APIGroups: []string{"rbac.authorization.k8s.io"}, + Resources: []string{"clusterroles"}, + }, + { // permission to manage the "safety namespace" + Verbs: []string{"*"}, + APIGroups: []string{""}, + Resources: []string{"namespaces"}, + }, + } + clusterRoleObject2 := fake.ClusterRoleObject(core.Name("bar-role")) + nt.Must(nt.RootRepos[configsync.RootSyncName].Add( + nomostest.StructuredNSPath(rootSyncA.Namespace, rootSyncA.Name), + rootSyncA, + )) + nt.Must(nt.RootRepos[configsync.RootSyncName].Add( + fmt.Sprintf("acme/namespaces/%s/%s.yaml", roleObject.Namespace, roleObject.Name), + roleObject, + )) + nt.Must(nt.RootRepos[configsync.RootSyncName].Add( + fmt.Sprintf("acme/namespaces/%s.yaml", clusterRoleObject.Name), + clusterRoleObject, + )) + nt.Must(nt.RootRepos[configsync.RootSyncName].Add( + fmt.Sprintf("acme/namespaces/%s.yaml", clusterRoleObject2.Name), + clusterRoleObject2, + )) + nt.Must(nt.RootRepos[configsync.RootSyncName].CommitAndPush("Add Roles and RoleRefs")) + if err := nt.WatchForAllSyncs(); err != nil { + nt.T.Fatal(err) + } + tg := taskgroup.New() + tg.Go(func() error { + return validateRoleRefs(nt, configsync.RootSyncKind, syncANN, rootSyncA.Spec.SafeOverride().RoleRefs) + }) + tg.Go(func() error { + return nt.Validate(controllers.RootSyncLegacyClusterRoleBindingName, "", &rbacv1.ClusterRoleBinding{}, + testpredicates.ClusterRoleBindingSubjectNamesEqual(nomostest.DefaultRootReconcilerName)) + }) + tg.Go(func() error { + return nt.Validate(controllers.RootSyncBaseClusterRoleBindingName, "", &rbacv1.ClusterRoleBinding{}, + testpredicates.ClusterRoleBindingSubjectNamesEqual(syncAReconcilerName)) + }) + if err := tg.Wait(); err != nil { + nt.T.Fatal(err) + } + + nt.T.Logf("Remove some but not all roleRefs from %s to verify garbage collection", syncANN.Name) + rootSyncA.Spec.SafeOverride().RoleRefs = []v1beta1.RootSyncRoleRef{ + { + Kind: "ClusterRole", + Name: "foo-role", + }, + } + nt.Must(nt.RootRepos[configsync.RootSyncName].Add( + fmt.Sprintf("acme/namespaces/%s/%s.yaml", configsync.ControllerNamespace, rootSyncA.Name), + rootSyncA, + )) + nt.Must(nt.RootRepos[configsync.RootSyncName].CommitAndPush("Reduce RoleRefs")) + if err := nt.WatchForAllSyncs(); err != nil { + nt.T.Fatal(err) + } + tg = taskgroup.New() + tg.Go(func() error { + return validateRoleRefs(nt, configsync.RootSyncKind, syncANN, rootSyncA.Spec.SafeOverride().RoleRefs) + }) + tg.Go(func() error { + return nt.Validate(controllers.RootSyncLegacyClusterRoleBindingName, "", &rbacv1.ClusterRoleBinding{}, + testpredicates.ClusterRoleBindingSubjectNamesEqual(nomostest.DefaultRootReconcilerName)) + }) + tg.Go(func() error { + return nt.Validate(controllers.RootSyncBaseClusterRoleBindingName, "", &rbacv1.ClusterRoleBinding{}, + testpredicates.ClusterRoleBindingSubjectNamesEqual(syncAReconcilerName)) + }) + if err := tg.Wait(); err != nil { + nt.T.Fatal(err) + } + + nt.T.Logf("Delete the RootSync %s to verify garbage collection", syncANN.Name) + nt.Must(nt.RootRepos[configsync.RootSyncName].Remove( + nomostest.StructuredNSPath(rootSyncA.Namespace, rootSyncA.Name), + )) + nt.Must(nt.RootRepos[configsync.RootSyncName].CommitAndPush("Prune RootSync")) + if err := nt.WatchForSync(kinds.RootSyncV1Beta1(), configsync.RootSyncName, configsync.ControllerNamespace, + nomostest.DefaultRootSha1Fn, nomostest.RootSyncHasStatusSyncCommit, nil); err != nil { + nt.T.Fatal(err) + } + tg = taskgroup.New() + tg.Go(func() error { + return nt.ValidateNotFound(syncANN.Name, syncANN.Namespace, &v1beta1.RootSync{}) + }) + tg.Go(func() error { + return validateRoleRefs(nt, configsync.RootSyncKind, syncANN, []v1beta1.RootSyncRoleRef{}) + }) + tg.Go(func() error { + return nt.Validate(controllers.RootSyncLegacyClusterRoleBindingName, "", &rbacv1.ClusterRoleBinding{}, + testpredicates.ClusterRoleBindingSubjectNamesEqual(nomostest.DefaultRootReconcilerName)) + }) + tg.Go(func() error { + return nt.ValidateNotFound(controllers.RootSyncBaseClusterRoleBindingName, "", &rbacv1.ClusterRoleBinding{}) + }) + if err := tg.Wait(); err != nil { + nt.T.Fatal(err) + } +} + +// This helper function verifies that the specified role refs are mapped to +// bindings on the cluster. The bindings are looked up using labels based on the +// RSync kind/name/namespace. Returns an error if what is found on the cluster +// is not an exact match. +func validateRoleRefs(nt *nomostest.NT, syncKind string, rsRef types.NamespacedName, expected []v1beta1.RootSyncRoleRef) error { + roleBindings, err := nt.ListReconcilerRoleBindings(syncKind, rsRef) + if err != nil { + return err + } + actualRoleRefCount := make(map[v1beta1.RootSyncRoleRef]int) + for _, rb := range roleBindings { + roleRef := v1beta1.RootSyncRoleRef{ + Kind: rb.RoleRef.Kind, + Name: rb.RoleRef.Name, + Namespace: rb.Namespace, + } + actualRoleRefCount[roleRef]++ + } + clusterRoleBindings, err := nt.ListReconcilerClusterRoleBindings(syncKind, rsRef) + if err != nil { + return err + } + for _, crb := range clusterRoleBindings { + roleRef := v1beta1.RootSyncRoleRef{ + Kind: crb.RoleRef.Kind, + Name: crb.RoleRef.Name, + } + actualRoleRefCount[roleRef]++ + } + totalBindings := len(roleBindings) + len(clusterRoleBindings) + if len(expected) != totalBindings { + return errors.Errorf("expected %d bindings but found %d", + len(expected), totalBindings) + } + for _, roleRef := range expected { + if actualRoleRefCount[roleRef] != 1 { + return errors.Errorf("expected to find one binding mapping to %s, found %d", + roleRef, actualRoleRefCount[roleRef]) + } + } + return nil +} diff --git a/e2e/testcases/reconciler_manager_test.go b/e2e/testcases/reconciler_manager_test.go index ee847d1d01..2ba94ba6d9 100644 --- a/e2e/testcases/reconciler_manager_test.go +++ b/e2e/testcases/reconciler_manager_test.go @@ -207,7 +207,7 @@ func validateRootSyncDependencies(nt *nomostest.NT, rsName string) []client.Obje // only happens when upgrading from a very old unsupported version. rootSyncCRB := &rbacv1.ClusterRoleBinding{} - setNN(rootSyncCRB, client.ObjectKey{Name: controllers.RootSyncPermissionsName(rootSyncReconciler.Name)}) + rootSyncCRB.Name = controllers.RootSyncLegacyClusterRoleBindingName rootSyncDependencies = append(rootSyncDependencies, rootSyncCRB) rootSyncSA := &corev1.ServiceAccount{} @@ -233,10 +233,8 @@ func validateRepoSyncDependencies(nt *nomostest.NT, ns, rsName string) []client. // only happens when upgrading from a very old unsupported version. repoSyncRB := &rbacv1.RoleBinding{} - setNN(repoSyncRB, client.ObjectKey{ - Name: controllers.RepoSyncPermissionsName(), - Namespace: ns, - }) + repoSyncRB.Name = controllers.RepoSyncBaseRoleBindingName + repoSyncRB.Namespace = ns repoSyncDependencies = append(repoSyncDependencies, repoSyncRB) repoSyncSA := &corev1.ServiceAccount{} @@ -268,6 +266,7 @@ func validateRepoSyncDependencies(nt *nomostest.NT, ns, rsName string) []client. err := nt.Validate(obj.GetName(), obj.GetNamespace(), obj) require.NoError(nt.T, err) } + return repoSyncDependencies } diff --git a/e2e/testcases/root_sync_test.go b/e2e/testcases/root_sync_test.go index 9ab507b97f..9ad8e5eadf 100644 --- a/e2e/testcases/root_sync_test.go +++ b/e2e/testcases/root_sync_test.go @@ -69,7 +69,7 @@ func TestDeleteRootSyncAndRootSyncV1Alpha1(t *testing.T) { saName := core.RootReconcilerName(rs.Name) errs = multierr.Append(errs, nt.ValidateNotFound(saName, configsync.ControllerNamespace, fake.ServiceAccountObject(saName))) // validate Root Reconciler ClusterRoleBinding is no longer present. - errs = multierr.Append(errs, nt.ValidateNotFound(controllers.RootSyncPermissionsName(nomostest.DefaultRootReconcilerName), configsync.ControllerNamespace, fake.ClusterRoleBindingObject())) + errs = multierr.Append(errs, nt.ValidateNotFound(controllers.RootSyncLegacyClusterRoleBindingName, configsync.ControllerNamespace, fake.ClusterRoleBindingObject())) return errs }) if err != nil { diff --git a/manifests/base/kustomization.yaml b/manifests/base/kustomization.yaml index 4ea55913c2..213f53ef5d 100644 --- a/manifests/base/kustomization.yaml +++ b/manifests/base/kustomization.yaml @@ -17,7 +17,8 @@ resources: - ../cluster-registry-crd.yaml - ../container-default-limits.yaml - ../namespace-selector-crd.yaml -- ../ns-reconciler-cluster-role.yaml +- ../ns-reconciler-base-cluster-role.yaml +- ../root-reconciler-base-cluster-role.yaml - ../otel-agent-cm.yaml - ../reconciler-manager-service-account.yaml - ../reposync-crd.yaml diff --git a/manifests/ns-reconciler-cluster-role.yaml b/manifests/ns-reconciler-base-cluster-role.yaml similarity index 100% rename from manifests/ns-reconciler-cluster-role.yaml rename to manifests/ns-reconciler-base-cluster-role.yaml diff --git a/manifests/root-reconciler-base-cluster-role.yaml b/manifests/root-reconciler-base-cluster-role.yaml new file mode 100644 index 0000000000..2dd1cc9e95 --- /dev/null +++ b/manifests/root-reconciler-base-cluster-role.yaml @@ -0,0 +1,34 @@ +# Copyright 2023 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: configsync.gke.io:root-reconciler + labels: + configmanagement.gke.io/system: "true" + configmanagement.gke.io/arch: "csmr" +rules: +- apiGroups: ["configsync.gke.io"] + resources: ["rootsyncs"] + verbs: ["get","list","watch","update","patch"] +- apiGroups: ["configsync.gke.io"] + resources: ["rootsyncs/status"] + verbs: ["get","list","watch","update","patch"] +- apiGroups: ["kpt.dev"] + resources: ["resourcegroups"] + verbs: ["*"] +- apiGroups: ["kpt.dev"] + resources: ["resourcegroups/status"] + verbs: ["*"] diff --git a/manifests/rootsync-crd.yaml b/manifests/rootsync-crd.yaml index 87cfa9120d..952df66097 100644 --- a/manifests/rootsync-crd.yaml +++ b/manifests/rootsync-crd.yaml @@ -328,21 +328,6 @@ spec: about valid inputs: https://pkg.go.dev/time#ParseDuration. Recommended apiServerTimeout range is from "3s" to "1m".' type: string - clusterRole: - description: clusterRole controls which role to bind the service - account for this RootSync's reconciler to. - properties: - disabled: - description: "disabled turns off binding to a ClusterRole - entirely. \n It is then your responsibility to bind any - roles to the relevant service account." - type: boolean - name: - description: "name is the name of the ClusterRole to bind - to; defaults to \"cluster-admin\". \n If you specify a different - name, the ClusterRole needs to be created manually." - type: string - type: object enableShellInRendering: description: 'enableShellInRendering specifies whether to enable or disable the shell access in rendering process. Default: false. @@ -459,6 +444,33 @@ spec: x-kubernetes-int-or-string: true type: object type: array + roleRefs: + description: roleRefs is a list of Roles or ClusterRoles to create + bindings. If unset, a binding to cluster-admin will be created. + items: + description: each item references a Role or ClusterRole to create + a binding to for this reconciler. It supports a namespace + field that can be used to create RoleBindings rather than + ClusterRoleBindings. + properties: + kind: + description: kind refers to the Kind of the RBAC resource. + Accepted values are Role and ClusterRole. + pattern: ^(Role|ClusterRole)$ + type: string + name: + description: name is the name of the Role or ClusterRole + resource. + type: string + namespace: + description: namespace indicates the Namespace in which + a RoleBinding should be created. For ClusterRole objects, + will determine whether a RoleBinding or ClusterRoleBinding + is created. For Role objects, must be set to the same + namespace as the Role. + type: string + type: object + type: array statusMode: description: statusMode controls whether the actuation status such as apply failed or not should be embedded into the ResourceGroup @@ -1411,21 +1423,6 @@ spec: about valid inputs: https://pkg.go.dev/time#ParseDuration. Recommended apiServerTimeout range is from "3s" to "1m".' type: string - clusterRole: - description: clusterRole controls which role to bind the service - account for this RootSync's reconciler to. - properties: - disabled: - description: "disabled turns off binding to a ClusterRole - entirely. \n It is then your responsibility to bind any - roles to the relevant service account." - type: boolean - name: - description: "name is the name of the ClusterRole to bind - to; defaults to \"cluster-admin\". \n If you specify a different - name, the ClusterRole needs to be created manually." - type: string - type: object enableShellInRendering: description: 'enableShellInRendering specifies whether to enable or disable the shell access in rendering process. Default: false. @@ -1542,6 +1539,38 @@ spec: x-kubernetes-int-or-string: true type: object type: array + roleRefs: + description: roleRefs is a list of Roles or ClusterRoles to create + bindings. If unset, a binding to cluster-admin will be created. + items: + description: each item references a Role or ClusterRole to create + a binding to for this reconciler. It supports a namespace + field that can be used to create RoleBindings rather than + ClusterRoleBindings. + properties: + kind: + description: kind refers to the Kind of the RBAC resource. + Accepted values are Role and ClusterRole. Required. + enum: + - Role + - ClusterRole + type: string + name: + description: name is the name of the Role or ClusterRole + resource. Required. + type: string + namespace: + description: namespace indicates the Namespace in which + a RoleBinding should be created. For ClusterRole objects, + will determine whether a RoleBinding or ClusterRoleBinding + is created. For Role objects, must be set to the same + namespace as the Role. + type: string + required: + - kind + - name + type: object + type: array statusMode: description: statusMode controls whether the actuation status such as apply failed or not should be embedded into the ResourceGroup diff --git a/pkg/api/configsync/v1alpha1/resource_override.go b/pkg/api/configsync/v1alpha1/resource_override.go index 1999253483..b273a94079 100644 --- a/pkg/api/configsync/v1alpha1/resource_override.go +++ b/pkg/api/configsync/v1alpha1/resource_override.go @@ -93,29 +93,35 @@ type RootSyncOverrideSpec struct { // +optional NamespaceStrategy configsync.NamespaceStrategy `json:"namespaceStrategy,omitempty"` - // clusterRole controls which role to bind the service account for this RootSync's - // reconciler to. + // roleRefs is a list of Roles or ClusterRoles to create bindings. + // If unset, a binding to cluster-admin will be created. // // +optional - ClusterRole ClusterRoleOverrideSpec `json:"clusterRole,omitempty"` + RoleRefs []RootSyncRoleRef `json:"roleRefs,omitempty"` } -// ClusterRoleOverrideSpec allows to override settings for the root reconciler permissions -type ClusterRoleOverrideSpec struct { - // name is the name of the ClusterRole to bind to; defaults to "cluster-admin". - // - // If you specify a different name, the ClusterRole needs to be created manually. +// each item references a Role or ClusterRole to create +// a binding to for this reconciler. It supports a namespace field that can be used +// to create RoleBindings rather than ClusterRoleBindings. +// +//nolint:revive +type RootSyncRoleRef struct { + // kind refers to the Kind of the RBAC resource. + // Accepted values are Role and ClusterRole. // - // +optional + // +kubebuilder:validation:Pattern=^(Role|ClusterRole)$ + Kind string `json:"kind,omitempty"` + + // name is the name of the Role or ClusterRole resource. Name string `json:"name,omitempty"` - // disabled turns off binding to a ClusterRole entirely. - // - // It is then your responsibility to bind any roles to the relevant service - // account. + // namespace indicates the Namespace in which a RoleBinding should be created. + // For ClusterRole objects, will determine whether a RoleBinding or ClusterRoleBinding + // is created. + // For Role objects, must be set to the same namespace as the Role. // // +optional - Disabled bool `json:"disabled,omitempty"` + Namespace string `json:"namespace,omitempty"` } // RepoSyncOverrideSpec allows to override the settings for a RepoSync reconciler pod diff --git a/pkg/api/configsync/v1alpha1/zz_generated.conversion.go b/pkg/api/configsync/v1alpha1/zz_generated.conversion.go index 255fea94ad..0a8918b6d3 100644 --- a/pkg/api/configsync/v1alpha1/zz_generated.conversion.go +++ b/pkg/api/configsync/v1alpha1/zz_generated.conversion.go @@ -23,16 +23,6 @@ func init() { // RegisterConversions adds conversion functions to the given scheme. // Public to allow building arbitrary schemes. func RegisterConversions(s *runtime.Scheme) error { - if err := s.AddGeneratedConversionFunc((*ClusterRoleOverrideSpec)(nil), (*v1beta1.ClusterRoleOverrideSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1alpha1_ClusterRoleOverrideSpec_To_v1beta1_ClusterRoleOverrideSpec(a.(*ClusterRoleOverrideSpec), b.(*v1beta1.ClusterRoleOverrideSpec), scope) - }); err != nil { - return err - } - if err := s.AddGeneratedConversionFunc((*v1beta1.ClusterRoleOverrideSpec)(nil), (*ClusterRoleOverrideSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta1_ClusterRoleOverrideSpec_To_v1alpha1_ClusterRoleOverrideSpec(a.(*v1beta1.ClusterRoleOverrideSpec), b.(*ClusterRoleOverrideSpec), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*ConfigSyncError)(nil), (*v1beta1.ConfigSyncError)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha1_ConfigSyncError_To_v1beta1_ConfigSyncError(a.(*ConfigSyncError), b.(*v1beta1.ConfigSyncError), scope) }); err != nil { @@ -273,6 +263,16 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddGeneratedConversionFunc((*RootSyncRoleRef)(nil), (*v1beta1.RootSyncRoleRef)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1alpha1_RootSyncRoleRef_To_v1beta1_RootSyncRoleRef(a.(*RootSyncRoleRef), b.(*v1beta1.RootSyncRoleRef), scope) + }); err != nil { + return err + } + if err := s.AddGeneratedConversionFunc((*v1beta1.RootSyncRoleRef)(nil), (*RootSyncRoleRef)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta1_RootSyncRoleRef_To_v1alpha1_RootSyncRoleRef(a.(*v1beta1.RootSyncRoleRef), b.(*RootSyncRoleRef), scope) + }); err != nil { + return err + } if err := s.AddGeneratedConversionFunc((*RootSyncSpec)(nil), (*v1beta1.RootSyncSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha1_RootSyncSpec_To_v1beta1_RootSyncSpec(a.(*RootSyncSpec), b.(*v1beta1.RootSyncSpec), scope) }); err != nil { @@ -356,28 +356,6 @@ func RegisterConversions(s *runtime.Scheme) error { return nil } -func autoConvert_v1alpha1_ClusterRoleOverrideSpec_To_v1beta1_ClusterRoleOverrideSpec(in *ClusterRoleOverrideSpec, out *v1beta1.ClusterRoleOverrideSpec, s conversion.Scope) error { - out.Name = in.Name - out.Disabled = in.Disabled - return nil -} - -// Convert_v1alpha1_ClusterRoleOverrideSpec_To_v1beta1_ClusterRoleOverrideSpec is an autogenerated conversion function. -func Convert_v1alpha1_ClusterRoleOverrideSpec_To_v1beta1_ClusterRoleOverrideSpec(in *ClusterRoleOverrideSpec, out *v1beta1.ClusterRoleOverrideSpec, s conversion.Scope) error { - return autoConvert_v1alpha1_ClusterRoleOverrideSpec_To_v1beta1_ClusterRoleOverrideSpec(in, out, s) -} - -func autoConvert_v1beta1_ClusterRoleOverrideSpec_To_v1alpha1_ClusterRoleOverrideSpec(in *v1beta1.ClusterRoleOverrideSpec, out *ClusterRoleOverrideSpec, s conversion.Scope) error { - out.Name = in.Name - out.Disabled = in.Disabled - return nil -} - -// Convert_v1beta1_ClusterRoleOverrideSpec_To_v1alpha1_ClusterRoleOverrideSpec is an autogenerated conversion function. -func Convert_v1beta1_ClusterRoleOverrideSpec_To_v1alpha1_ClusterRoleOverrideSpec(in *v1beta1.ClusterRoleOverrideSpec, out *ClusterRoleOverrideSpec, s conversion.Scope) error { - return autoConvert_v1beta1_ClusterRoleOverrideSpec_To_v1alpha1_ClusterRoleOverrideSpec(in, out, s) -} - func autoConvert_v1alpha1_ConfigSyncError_To_v1beta1_ConfigSyncError(in *ConfigSyncError, out *v1beta1.ConfigSyncError, s conversion.Scope) error { out.Code = in.Code out.ErrorMessage = in.ErrorMessage @@ -1115,9 +1093,7 @@ func autoConvert_v1alpha1_RootSyncOverrideSpec_To_v1beta1_RootSyncOverrideSpec(i return err } out.NamespaceStrategy = configsync.NamespaceStrategy(in.NamespaceStrategy) - if err := Convert_v1alpha1_ClusterRoleOverrideSpec_To_v1beta1_ClusterRoleOverrideSpec(&in.ClusterRole, &out.ClusterRole, s); err != nil { - return err - } + out.RoleRefs = *(*[]v1beta1.RootSyncRoleRef)(unsafe.Pointer(&in.RoleRefs)) return nil } @@ -1131,9 +1107,7 @@ func autoConvert_v1beta1_RootSyncOverrideSpec_To_v1alpha1_RootSyncOverrideSpec(i return err } out.NamespaceStrategy = configsync.NamespaceStrategy(in.NamespaceStrategy) - if err := Convert_v1beta1_ClusterRoleOverrideSpec_To_v1alpha1_ClusterRoleOverrideSpec(&in.ClusterRole, &out.ClusterRole, s); err != nil { - return err - } + out.RoleRefs = *(*[]RootSyncRoleRef)(unsafe.Pointer(&in.RoleRefs)) return nil } @@ -1142,6 +1116,30 @@ func Convert_v1beta1_RootSyncOverrideSpec_To_v1alpha1_RootSyncOverrideSpec(in *v return autoConvert_v1beta1_RootSyncOverrideSpec_To_v1alpha1_RootSyncOverrideSpec(in, out, s) } +func autoConvert_v1alpha1_RootSyncRoleRef_To_v1beta1_RootSyncRoleRef(in *RootSyncRoleRef, out *v1beta1.RootSyncRoleRef, s conversion.Scope) error { + out.Kind = in.Kind + out.Name = in.Name + out.Namespace = in.Namespace + return nil +} + +// Convert_v1alpha1_RootSyncRoleRef_To_v1beta1_RootSyncRoleRef is an autogenerated conversion function. +func Convert_v1alpha1_RootSyncRoleRef_To_v1beta1_RootSyncRoleRef(in *RootSyncRoleRef, out *v1beta1.RootSyncRoleRef, s conversion.Scope) error { + return autoConvert_v1alpha1_RootSyncRoleRef_To_v1beta1_RootSyncRoleRef(in, out, s) +} + +func autoConvert_v1beta1_RootSyncRoleRef_To_v1alpha1_RootSyncRoleRef(in *v1beta1.RootSyncRoleRef, out *RootSyncRoleRef, s conversion.Scope) error { + out.Kind = in.Kind + out.Name = in.Name + out.Namespace = in.Namespace + return nil +} + +// Convert_v1beta1_RootSyncRoleRef_To_v1alpha1_RootSyncRoleRef is an autogenerated conversion function. +func Convert_v1beta1_RootSyncRoleRef_To_v1alpha1_RootSyncRoleRef(in *v1beta1.RootSyncRoleRef, out *RootSyncRoleRef, s conversion.Scope) error { + return autoConvert_v1beta1_RootSyncRoleRef_To_v1alpha1_RootSyncRoleRef(in, out, s) +} + func autoConvert_v1alpha1_RootSyncSpec_To_v1beta1_RootSyncSpec(in *RootSyncSpec, out *v1beta1.RootSyncSpec, s conversion.Scope) error { out.SourceFormat = in.SourceFormat out.SourceType = in.SourceType diff --git a/pkg/api/configsync/v1alpha1/zz_generated.deepcopy.go b/pkg/api/configsync/v1alpha1/zz_generated.deepcopy.go index 01b9af4e51..91b6be28e7 100644 --- a/pkg/api/configsync/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/api/configsync/v1alpha1/zz_generated.deepcopy.go @@ -10,21 +10,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ClusterRoleOverrideSpec) DeepCopyInto(out *ClusterRoleOverrideSpec) { - *out = *in -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterRoleOverrideSpec. -func (in *ClusterRoleOverrideSpec) DeepCopy() *ClusterRoleOverrideSpec { - if in == nil { - return nil - } - out := new(ClusterRoleOverrideSpec) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ConfigSyncError) DeepCopyInto(out *ConfigSyncError) { *out = *in @@ -614,7 +599,11 @@ func (in *RootSyncList) DeepCopyObject() runtime.Object { func (in *RootSyncOverrideSpec) DeepCopyInto(out *RootSyncOverrideSpec) { *out = *in in.OverrideSpec.DeepCopyInto(&out.OverrideSpec) - out.ClusterRole = in.ClusterRole + if in.RoleRefs != nil { + in, out := &in.RoleRefs, &out.RoleRefs + *out = make([]RootSyncRoleRef, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RootSyncOverrideSpec. @@ -627,6 +616,21 @@ func (in *RootSyncOverrideSpec) DeepCopy() *RootSyncOverrideSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RootSyncRoleRef) DeepCopyInto(out *RootSyncRoleRef) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RootSyncRoleRef. +func (in *RootSyncRoleRef) DeepCopy() *RootSyncRoleRef { + if in == nil { + return nil + } + out := new(RootSyncRoleRef) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RootSyncSpec) DeepCopyInto(out *RootSyncSpec) { *out = *in diff --git a/pkg/api/configsync/v1beta1/resource_override.go b/pkg/api/configsync/v1beta1/resource_override.go index 475eaacb9e..37d854f7f7 100644 --- a/pkg/api/configsync/v1beta1/resource_override.go +++ b/pkg/api/configsync/v1beta1/resource_override.go @@ -93,29 +93,36 @@ type RootSyncOverrideSpec struct { // +optional NamespaceStrategy configsync.NamespaceStrategy `json:"namespaceStrategy,omitempty"` - // clusterRole controls which role to bind the service account for this RootSync's - // reconciler to. + // roleRefs is a list of Roles or ClusterRoles to create bindings. + // If unset, a binding to cluster-admin will be created. // // +optional - ClusterRole ClusterRoleOverrideSpec `json:"clusterRole,omitempty"` + RoleRefs []RootSyncRoleRef `json:"roleRefs,omitempty"` } -// ClusterRoleOverrideSpec allows to override settings for the root reconciler permissions -type ClusterRoleOverrideSpec struct { - // name is the name of the ClusterRole to bind to; defaults to "cluster-admin". - // - // If you specify a different name, the ClusterRole needs to be created manually. - // - // +optional - Name string `json:"name,omitempty"` +// each item references a Role or ClusterRole to create +// a binding to for this reconciler. It supports a namespace field that can be used +// to create RoleBindings rather than ClusterRoleBindings. +// +//nolint:revive +type RootSyncRoleRef struct { - // disabled turns off binding to a ClusterRole entirely. + // kind refers to the Kind of the RBAC resource. + // Accepted values are Role and ClusterRole. Required. // - // It is then your responsibility to bind any roles to the relevant service - // account. + // +kubebuilder:validation:Enum=Role;ClusterRole + Kind string `json:"kind"` + + // name is the name of the Role or ClusterRole resource. Required. + Name string `json:"name"` + + // namespace indicates the Namespace in which a RoleBinding should be created. + // For ClusterRole objects, will determine whether a RoleBinding or ClusterRoleBinding + // is created. + // For Role objects, must be set to the same namespace as the Role. // // +optional - Disabled bool `json:"disabled,omitempty"` + Namespace string `json:"namespace,omitempty"` } // RepoSyncOverrideSpec allows to override the settings for a RepoSync reconciler pod diff --git a/pkg/api/configsync/v1beta1/zz_generated.deepcopy.go b/pkg/api/configsync/v1beta1/zz_generated.deepcopy.go index f2d34a9ca4..ff6cd0e1a8 100644 --- a/pkg/api/configsync/v1beta1/zz_generated.deepcopy.go +++ b/pkg/api/configsync/v1beta1/zz_generated.deepcopy.go @@ -10,21 +10,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ClusterRoleOverrideSpec) DeepCopyInto(out *ClusterRoleOverrideSpec) { - *out = *in -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterRoleOverrideSpec. -func (in *ClusterRoleOverrideSpec) DeepCopy() *ClusterRoleOverrideSpec { - if in == nil { - return nil - } - out := new(ClusterRoleOverrideSpec) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ConfigSyncError) DeepCopyInto(out *ConfigSyncError) { *out = *in @@ -614,7 +599,11 @@ func (in *RootSyncList) DeepCopyObject() runtime.Object { func (in *RootSyncOverrideSpec) DeepCopyInto(out *RootSyncOverrideSpec) { *out = *in in.OverrideSpec.DeepCopyInto(&out.OverrideSpec) - out.ClusterRole = in.ClusterRole + if in.RoleRefs != nil { + in, out := &in.RoleRefs, &out.RoleRefs + *out = make([]RootSyncRoleRef, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RootSyncOverrideSpec. @@ -627,6 +616,21 @@ func (in *RootSyncOverrideSpec) DeepCopy() *RootSyncOverrideSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RootSyncRoleRef) DeepCopyInto(out *RootSyncRoleRef) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RootSyncRoleRef. +func (in *RootSyncRoleRef) DeepCopy() *RootSyncRoleRef { + if in == nil { + return nil + } + out := new(RootSyncRoleRef) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RootSyncSpec) DeepCopyInto(out *RootSyncSpec) { *out = *in diff --git a/pkg/core/meta.go b/pkg/core/meta.go index 06e35c3134..a99c64f694 100644 --- a/pkg/core/meta.go +++ b/pkg/core/meta.go @@ -38,6 +38,13 @@ func Name(name string) MetaMutator { } } +// GenerateName replaces the metadata.generateName of the Object under test. +func GenerateName(generateName string) MetaMutator { + return func(o client.Object) { + o.SetGenerateName(generateName) + } +} + // UID replaces the metadata.uid of the Object under test. func UID(uid types.UID) MetaMutator { return func(o client.Object) { diff --git a/pkg/core/names.go b/pkg/core/names.go index 876b91cfe7..8270d446ab 100644 --- a/pkg/core/names.go +++ b/pkg/core/names.go @@ -27,8 +27,6 @@ const ( NsReconcilerPrefix = "ns-reconciler" // RootReconcilerPrefix is the prefix usef for all Root reconcilers. RootReconcilerPrefix = "root-reconciler" - // RootSyncPermissionsPrefix is the prefix used for all ClusterRoleBindings granting access to Root Reconcilers - RootSyncPermissionsPrefix = configsync.GroupName + ":" + RootReconcilerPrefix ) // RootReconcilerName returns the root reconciler's name in the format root-reconciler-. diff --git a/pkg/reconcilermanager/controllers/build_names.go b/pkg/reconcilermanager/controllers/build_names.go index 6c775a7b36..58025a1f83 100644 --- a/pkg/reconcilermanager/controllers/build_names.go +++ b/pkg/reconcilermanager/controllers/build_names.go @@ -21,19 +21,27 @@ import ( "kpt.dev/configsync/pkg/core" ) +const ( + // RepoSyncBaseClusterRoleName is the namespace reconciler permissions name. + // e.g. configsync.gke.io:ns-reconciler + RepoSyncBaseClusterRoleName = configsync.GroupName + ":" + core.NsReconcilerPrefix + // RootSyncBaseClusterRoleName is the root reconciler base ClusterRole name. + // e.g. configsync.gke.io:root-reconciler + RootSyncBaseClusterRoleName = configsync.GroupName + ":" + core.RootReconcilerPrefix + // RepoSyncBaseRoleBindingName is the name of the default RoleBinding created + // for RepoSync objects. This contains basic permissions for RepoSync reconcilers + //(e.g. RepoSync status update). + RepoSyncBaseRoleBindingName = RepoSyncBaseClusterRoleName + // RootSyncLegacyClusterRoleBindingName is the name of the legacy ClusterRoleBinding created + // for RootSync objects. It is always bound to cluster-admin. + RootSyncLegacyClusterRoleBindingName = RootSyncBaseClusterRoleName + // RootSyncBaseClusterRoleBindingName is the name of the default ClusterRoleBinding created + // for RootSync objects. This contains basic permissions for RootSync reconcilers + // (e.g. RootSync status update). + RootSyncBaseClusterRoleBindingName = RootSyncBaseClusterRoleName + "-base" +) + // ReconcilerResourceName returns resource name in the format -. func ReconcilerResourceName(reconcilerName, resourceName string) string { return fmt.Sprintf("%s-%s", reconcilerName, resourceName) } - -// RepoSyncPermissionsName returns namespace reconciler permissions name. -// e.g. configsync.gke.io:ns-reconciler -func RepoSyncPermissionsName() string { - return fmt.Sprintf("%s:%s", configsync.GroupName, core.NsReconcilerPrefix) -} - -// RootSyncPermissionsName returns root reconciler ClusterRoleBinding name. -// e.g. configsync.gke.io:root-reconciler:my-root-sync -func RootSyncPermissionsName(reconcilerName string) string { - return fmt.Sprintf("%s:%s", core.RootSyncPermissionsPrefix, core.RootSyncName(reconcilerName)) -} diff --git a/pkg/reconcilermanager/controllers/garbage_collector.go b/pkg/reconcilermanager/controllers/garbage_collector.go index 7c42738925..2f682a031f 100644 --- a/pkg/reconcilermanager/controllers/garbage_collector.go +++ b/pkg/reconcilermanager/controllers/garbage_collector.go @@ -24,7 +24,6 @@ import ( corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" @@ -142,8 +141,8 @@ func (r *reconcilerBase) deleteServiceAccount(ctx context.Context, reconcilerRef return r.cleanup(ctx, sa) } -func (r *RepoSyncReconciler) deleteRoleBinding(ctx context.Context, reconcilerRef, rsRef types.NamespacedName) error { - rbKey := client.ObjectKey{Namespace: rsRef.Namespace, Name: RepoSyncPermissionsName()} +func (r *RepoSyncReconciler) deleteSharedRoleBinding(ctx context.Context, reconcilerRef, rsRef types.NamespacedName) error { + rbKey := client.ObjectKey{Namespace: rsRef.Namespace, Name: RepoSyncBaseClusterRoleName} rb := &rbacv1.RoleBinding{} if err := r.client.Get(ctx, rbKey, rb); err != nil { if apierrors.IsNotFound(err) { @@ -233,8 +232,32 @@ func (r *reconcilerBase) deleteDeployment(ctx context.Context, reconcilerRef typ return r.cleanup(ctx, d) } -func (r *RootSyncReconciler) deleteClusterRoleBinding(ctx context.Context, name string) error { - crb := &rbacv1.ClusterRoleBinding{ObjectMeta: metav1.ObjectMeta{Name: name}} - - return r.cleanup(ctx, crb) +func (r *RootSyncReconciler) deleteSharedClusterRoleBinding(ctx context.Context, name string, reconcilerRef types.NamespacedName) error { + crbKey := client.ObjectKey{Name: name} + // Update the CRB to delete the subject for the deleted RootSync's reconciler + crb := &rbacv1.ClusterRoleBinding{} + if err := r.client.Get(ctx, crbKey, crb); err != nil { + if apierrors.IsNotFound(err) { + // already deleted + r.logger(ctx).Info("Managed object already deleted", + logFieldObjectRef, crbKey.String(), + logFieldObjectKind, "ClusterRoleBinding") + return nil + } + return NewObjectOperationErrorWithKey(err, crb, OperationGet, crbKey) + } + count := len(crb.Subjects) + crb.Subjects = removeSubject(crb.Subjects, r.serviceAccountSubject(reconcilerRef)) + if len(crb.Subjects) == 0 { + // Delete the whole CRB + return r.cleanup(ctx, crb) + } + if count == len(crb.Subjects) { + // No change + return nil + } + if err := r.client.Update(ctx, crb); err != nil { + return NewObjectOperationError(err, crb, OperationUpdate) + } + return nil } diff --git a/pkg/reconcilermanager/controllers/reconciler_base.go b/pkg/reconcilermanager/controllers/reconciler_base.go index 18cf5a1be9..bedd6f35c0 100644 --- a/pkg/reconcilermanager/controllers/reconciler_base.go +++ b/pkg/reconcilermanager/controllers/reconciler_base.go @@ -637,3 +637,37 @@ func (r *reconcilerBase) setupOrTeardown(ctx context.Context, syncObj client.Obj return nil } + +// ManagedObjectLabelMap returns the standard labels applied to objects related +// to a RootSync/RepoSync that are created by reconciler-manager. +func ManagedObjectLabelMap(syncKind string, rsRef types.NamespacedName) map[string]string { + return map[string]string{ + metadata.SyncNamespaceLabel: rsRef.Namespace, + metadata.SyncNameLabel: rsRef.Name, + metadata.SyncKindLabel: syncKind, + } +} + +func (r *reconcilerBase) updateRBACBinding(ctx context.Context, reconcilerRef types.NamespacedName, binding client.Object) error { + existingBinding := binding.DeepCopyObject() + subjects := []rbacv1.Subject{r.serviceAccountSubject(reconcilerRef)} + if crb, ok := binding.(*rbacv1.ClusterRoleBinding); ok { + crb.Subjects = subjects + } else if rb, ok := binding.(*rbacv1.RoleBinding); ok { + rb.Subjects = subjects + } + if equality.Semantic.DeepEqual(existingBinding, binding) { + return nil + } + if err := r.client.Update(ctx, binding); err != nil { + return err + } + bindingNN := types.NamespacedName{ + Name: binding.GetName(), + Namespace: binding.GetNamespace(), + } + r.logger(ctx).Info("Managed object update successful", + logFieldObjectRef, bindingNN.String(), + logFieldObjectKind, binding.GetObjectKind().GroupVersionKind().Kind) + return nil +} diff --git a/pkg/reconcilermanager/controllers/reposync_controller.go b/pkg/reconcilermanager/controllers/reposync_controller.go index 407aa0e80b..8c35a915b2 100644 --- a/pkg/reconcilermanager/controllers/reposync_controller.go +++ b/pkg/reconcilermanager/controllers/reposync_controller.go @@ -246,7 +246,7 @@ func (r *RepoSyncReconciler) upsertManagedObjects(ctx context.Context, reconcile } // Overwrite reconciler rolebinding. - if _, err := r.upsertRoleBinding(ctx, reconcilerRef, rsRef); err != nil { + if _, err := r.upsertSharedRoleBinding(ctx, reconcilerRef, rsRef); err != nil { return errors.Wrap(err, "upserting role binding") } @@ -436,7 +436,7 @@ func (r *RepoSyncReconciler) deleteManagedObjects(ctx context.Context, reconcile return errors.Wrap(err, "deleting secrets") } - if err := r.deleteRoleBinding(ctx, reconcilerRef, rsRef); err != nil { + if err := r.deleteSharedRoleBinding(ctx, reconcilerRef, rsRef); err != nil { return errors.Wrap(err, "deleting role binding") } @@ -762,7 +762,7 @@ func (r *RepoSyncReconciler) mapObjectToRepoSync(obj client.Object) []reconcile. // Ignore changes from resources without the ns-reconciler prefix or configsync.gke.io:ns-reconciler // because all the generated resources have the prefix. - nsRoleBindingName := RepoSyncPermissionsName() + nsRoleBindingName := RepoSyncBaseRoleBindingName if !strings.HasPrefix(objRef.Name, core.NsReconcilerPrefix) && objRef.Name != nsRoleBindingName { return nil } @@ -937,17 +937,17 @@ func (r *RepoSyncReconciler) validateNamespaceSecret(ctx context.Context, repoSy return validateSecretData(authType, secret) } -func (r *RepoSyncReconciler) upsertRoleBinding(ctx context.Context, reconcilerRef, rsRef types.NamespacedName) (client.ObjectKey, error) { +func (r *RepoSyncReconciler) upsertSharedRoleBinding(ctx context.Context, reconcilerRef, rsRef types.NamespacedName) (client.ObjectKey, error) { rbRef := client.ObjectKey{ Namespace: rsRef.Namespace, - Name: RepoSyncPermissionsName(), + Name: RepoSyncBaseRoleBindingName, } childRB := &rbacv1.RoleBinding{} childRB.Name = rbRef.Name childRB.Namespace = rbRef.Namespace op, err := CreateOrUpdate(ctx, r.client, childRB, func() error { - childRB.RoleRef = rolereference(RepoSyncPermissionsName(), "ClusterRole") + childRB.RoleRef = rolereference(RepoSyncBaseClusterRoleName, "ClusterRole") childRB.Subjects = addSubject(childRB.Subjects, r.serviceAccountSubject(reconcilerRef)) return nil }) diff --git a/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go b/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go index 009e064c1f..2e48866175 100644 --- a/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go +++ b/pkg/reconcilermanager/controllers/reposync_controller_manager_test.go @@ -401,7 +401,7 @@ func TestRepoSyncReconcilerRoleBindingDriftProtection(t *testing.T) { // reconciler-manager managed robe binding return client.ObjectKey{ Namespace: syncRef.Namespace, - Name: RepoSyncPermissionsName(), + Name: RepoSyncBaseRoleBindingName, } } var oldObj *rbacv1.RoleBinding diff --git a/pkg/reconcilermanager/controllers/reposync_controller_test.go b/pkg/reconcilermanager/controllers/reposync_controller_test.go index d2abb45ba8..9ca76699f1 100644 --- a/pkg/reconcilermanager/controllers/reposync_controller_test.go +++ b/pkg/reconcilermanager/controllers/reposync_controller_test.go @@ -280,12 +280,12 @@ func repoSyncWithHelm(ns, name string, opts ...func(*v1beta1.RepoSync)) *v1beta1 return repoSync(ns, name, opts...) } -func rolebinding(name string, opts ...core.MetaMutator) *rbacv1.RoleBinding { +func rolebinding(name, roleName, roleKind string, opts ...core.MetaMutator) *rbacv1.RoleBinding { result := fake.RoleBindingObject(opts...) result.Name = name - result.RoleRef.Name = RepoSyncPermissionsName() - result.RoleRef.Kind = "ClusterRole" + result.RoleRef.Name = roleName + result.RoleRef.Kind = roleKind result.RoleRef.APIGroup = "rbac.authorization.k8s.io" return result @@ -2086,7 +2086,7 @@ func TestMultipleRepoSyncs(t *testing.T) { wantServiceAccounts := map[core.ID]*corev1.ServiceAccount{core.IDOf(serviceAccount1): serviceAccount1} roleBinding1 := rolebinding( - RepoSyncPermissionsName(), + RepoSyncBaseRoleBindingName, RepoSyncBaseClusterRoleName, "ClusterRole", core.Namespace(rs1.Namespace), core.UID("1"), core.ResourceVersion("1"), core.Generation(1), ) @@ -2165,7 +2165,7 @@ func TestMultipleRepoSyncs(t *testing.T) { } roleBinding2 := rolebinding( - RepoSyncPermissionsName(), + RepoSyncBaseRoleBindingName, RepoSyncBaseClusterRoleName, "ClusterRole", core.Namespace(rs2.Namespace), core.UID("1"), core.ResourceVersion("1"), core.Generation(1), ) @@ -2750,7 +2750,7 @@ func TestMapObjectToRepoSync(t *testing.T) { rs1 := repoSyncWithGit("ns1", "rs1", reposyncRef(gitRevision), reposyncBranch(branch), reposyncSecretType(configsync.AuthSSH), reposyncSecretRef(reposyncSSHKey)) ns1rs1ReconcilerName := core.NsReconcilerName(rs1.Namespace, rs1.Name) rs2 := repoSyncWithGit("ns2", "rs2", reposyncRef(gitRevision), reposyncBranch(branch), reposyncSecretType(configsync.AuthSSH), reposyncSecretRef(reposyncSSHKey)) - rsRoleBindingName := RepoSyncPermissionsName() + rsRoleBindingName := RepoSyncBaseRoleBindingName testCases := []struct { name string diff --git a/pkg/reconcilermanager/controllers/rootsync_controller.go b/pkg/reconcilermanager/controllers/rootsync_controller.go index 408418caa8..01b92a72ca 100644 --- a/pkg/reconcilermanager/controllers/rootsync_controller.go +++ b/pkg/reconcilermanager/controllers/rootsync_controller.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" @@ -121,7 +122,7 @@ func (r *RootSyncReconciler) Reconcile(ctx context.Context, req controllerruntim // This code path is unlikely, because the custom finalizer should // have already deleted the managed resources and removed the // rootSyncs cache entry. But if we get here, clean up anyway. - if err := r.deleteManagedObjects(ctx, reconcilerRef); err != nil { + if err := r.deleteManagedObjects(ctx, reconcilerRef, rsRef); err != nil { r.logger(ctx).Error(err, "Failed to delete managed objects") // Failed to delete a managed object. // Return an error to trigger retry. @@ -179,11 +180,8 @@ func (r *RootSyncReconciler) upsertManagedObjects(ctx context.Context, reconcile // This is because it's in the same namespace as the Deployment, so we don't // need to copy it to the config-management-system namespace. - labelMap := map[string]string{ - metadata.SyncNamespaceLabel: rs.Namespace, - metadata.SyncNameLabel: rs.Name, - metadata.SyncKindLabel: r.syncKind, - } + rsRef := client.ObjectKeyFromObject(rs) + labelMap := ManagedObjectLabelMap(r.syncKind, rsRef) // Overwrite reconciler pod ServiceAccount. var auth configsync.AuthType @@ -206,9 +204,9 @@ func (r *RootSyncReconciler) upsertManagedObjects(ctx context.Context, reconcile return errors.Wrap(err, "upserting service account") } - // Overwrite reconciler clusterrolebinding. - if err := r.configureClusterRoleBinding(ctx, reconcilerRef, rs.Spec.SafeOverride().ClusterRole); err != nil { - return errors.Wrap(err, "configuring cluster role binding") + // Reconcile reconciler RBAC bindings. + if err := r.manageRBACBindings(ctx, reconcilerRef, rsRef, rs.Spec.SafeOverride().RoleRefs); err != nil { + return errors.Wrap(err, "configuring RBAC bindings") } containerEnvs := r.populateContainerEnvs(ctx, rs, reconcilerRef.Name) @@ -295,7 +293,8 @@ func (r *RootSyncReconciler) setup(ctx context.Context, reconcilerRef types.Name // - Convert any error into RootSync status conditions // - Update the RootSync status func (r *RootSyncReconciler) teardown(ctx context.Context, reconcilerRef types.NamespacedName, rs *v1beta1.RootSync) error { - err := r.deleteManagedObjects(ctx, reconcilerRef) + rsRef := client.ObjectKeyFromObject(rs) + err := r.deleteManagedObjects(ctx, reconcilerRef, rsRef) updated, updateErr := r.updateSyncStatus(ctx, rs, reconcilerRef, func(syncObj *v1beta1.RootSync) error { // Modify the sync status, // but keep the upsert error separate from the status update error. @@ -372,7 +371,7 @@ func (r *RootSyncReconciler) handleReconcileError(ctx context.Context, err error // deleteManagedObjects deletes objects managed by the reconciler-manager for // this RootSync. -func (r *RootSyncReconciler) deleteManagedObjects(ctx context.Context, reconcilerRef types.NamespacedName) error { +func (r *RootSyncReconciler) deleteManagedObjects(ctx context.Context, reconcilerRef, rsRef types.NamespacedName) error { r.logger(ctx).Info("Deleting managed objects") if err := r.deleteDeployment(ctx, reconcilerRef); err != nil { @@ -390,8 +389,8 @@ func (r *RootSyncReconciler) deleteManagedObjects(ctx context.Context, reconcile // Note: ReconcilerManager doesn't manage the RootSync Secret. // So we don't need to delete it here. - if err := r.deleteClusterRoleBinding(ctx, RootSyncPermissionsName(reconcilerRef.Name)); err != nil { - return errors.Wrap(err, "deleting cluster role bindings") + if err := r.cleanRBACBindings(ctx, reconcilerRef, rsRef); err != nil { + return errors.Wrap(err, "deleting RBAC bindings") } if err := r.deleteServiceAccount(ctx, reconcilerRef); err != nil { @@ -437,6 +436,10 @@ func (r *RootSyncReconciler) SetupWithManager(mgr controllerruntime.Manager, wat handler.EnqueueRequestsFromMapFunc(r.mapObjectToRootSync), builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})). Watches(&source.Kind{Type: &rbacv1.ClusterRoleBinding{}}, + handler.EnqueueRequestsFromMapFunc(r.mapObjectToRootSync), + builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})). + // TODO: is it possible to watch with a label filter? + Watches(&source.Kind{Type: &rbacv1.RoleBinding{}}, handler.EnqueueRequestsFromMapFunc(r.mapObjectToRootSync), builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})) @@ -545,7 +548,7 @@ func (r *RootSyncReconciler) mapObjectToRootSync(obj client.Object) []reconcile. // Ignore changes from resources without the root-reconciler prefix or configsync.gke.io:root-reconciler // because all the generated resources have the prefix. if !strings.HasPrefix(objRef.Name, core.RootReconcilerPrefix) && - !strings.HasPrefix(objRef.Name, core.RootSyncPermissionsPrefix) { + !strings.HasPrefix(objRef.Name, RootSyncBaseClusterRoleName) { return nil } @@ -564,6 +567,11 @@ func (r *RootSyncReconciler) mapObjectToRootSync(obj client.Object) []reconcile. // Most of the resources are mapped to a single RootSync object except ClusterRoleBinding. // All RootSync objects share the same ClusterRoleBinding object, so requeue all RootSync objects if the ClusterRoleBinding is changed. + // Note that there are two shared ClusterRoleBindings: + // - configsync.gke.io/root-reconciler -> default binding to cluster-admin (for backwards compatibility) + // - configsync.gke.io/root-reconciler-base -> created when roleRefs are in use (basic permissions, e.g. RootSync status update) + // The additional RoleBindings and ClusterRoleBindings created from roleRefs + // map to a single RootSync and are prefixed with the reconciler name. // For other resources, requeue the mapping RootSync object and then return. var requests []reconcile.Request var attachedRSNames []string @@ -571,13 +579,19 @@ func (r *RootSyncReconciler) mapObjectToRootSync(obj client.Object) []reconcile. reconcilerName := core.RootReconcilerName(rs.GetName()) switch obj.(type) { case *rbacv1.ClusterRoleBinding: - if objRef.Name == RootSyncPermissionsName(reconcilerName) { + if objRef.Name == RootSyncLegacyClusterRoleBindingName || objRef.Name == RootSyncBaseClusterRoleBindingName { requests = append(requests, reconcile.Request{ NamespacedName: client.ObjectKeyFromObject(&rs)}) attachedRSNames = append(attachedRSNames, rs.GetName()) + } else if strings.HasPrefix(objRef.Name, reconcilerName) { + return requeueRootSyncRequest(obj, &rs) + } + case *rbacv1.RoleBinding: // RoleBinding can be in any namespace + if strings.HasPrefix(objRef.Name, reconcilerName) { + return requeueRootSyncRequest(obj, &rs) } - default: // Deployment and ServiceAccount - if objRef.Name == reconcilerName { + default: // Deployment and ServiceAccount are in the controller Namespace + if objRef.Name == reconcilerName && objRef.Namespace == configsync.ControllerNamespace { return requeueRootSyncRequest(obj, &rs) } } @@ -713,6 +727,10 @@ func (r *RootSyncReconciler) validateRootSync(ctx context.Context, rs *v1beta1.R return err } + if err := r.validateRoleRefs(rs.Spec.SafeOverride().RoleRefs); err != nil { + return err + } + return r.validateValuesFileSourcesRefs(ctx, rs) } @@ -735,6 +753,15 @@ func (r *RootSyncReconciler) validateSourceSpec(ctx context.Context, rs *v1beta1 } } +func (r *RootSyncReconciler) validateRoleRefs(roleRefs []v1beta1.RootSyncRoleRef) error { + for _, roleRef := range roleRefs { + if roleRef.Kind == "Role" && roleRef.Namespace == "" { + return errors.Errorf("namespace must be provided for roleRef with kind Role.") + } + } + return nil +} + // validateValuesFileSourcesRefs validates that the ConfigMaps specified in the RSync ValuesFileSources exist, are immutable, and have the // specified data key. func (r *RootSyncReconciler) validateValuesFileSourcesRefs(ctx context.Context, rs *v1beta1.RootSync) status.Error { @@ -779,44 +806,142 @@ func (r *RootSyncReconciler) validateRootSecret(ctx context.Context, rootSync *v return validateSecretData(rootSync.Spec.Auth, secret) } -func (r *RootSyncReconciler) configureClusterRoleBinding(ctx context.Context, reconcilerRef types.NamespacedName, roleConfig v1beta1.ClusterRoleOverrideSpec) error { - // in order to be backwards compatible with behavior before https://github.com/GoogleContainerTools/kpt-config-sync/pull/938 - // we delete any ClusterRoleBinding that uses the old name. - // This ensures smooth migrations for users upgrading past that version boundary. - if err := r.deleteClusterRoleBinding(ctx, fmt.Sprintf("%s:%s", core.RootReconcilerPrefix, configsync.RootSyncName)); err != nil && !apierrors.IsNotFound(err) { - return errors.Wrap(err, "deleting old binding") +// listCurrentRoleRefs lists RBAC bindings in the cluster that were created by +// reconciler-manager based on previous roleRefs, queried using a label selector. +// A map is returned which maps RoleRef to RBAC binding for convenience to the caller. +func (r *RootSyncReconciler) listCurrentRoleRefs(ctx context.Context, rsRef types.NamespacedName) (map[v1beta1.RootSyncRoleRef]client.Object, error) { + currentRoleMap := make(map[v1beta1.RootSyncRoleRef]client.Object) + labelMap := ManagedObjectLabelMap(r.syncKind, rsRef) + opts := &client.ListOptions{} + opts.LabelSelector = client.MatchingLabelsSelector{ + Selector: labels.SelectorFromSet(labelMap), + } + rbList := rbacv1.RoleBindingList{} + if err := r.client.List(ctx, &rbList, opts); err != nil { + return nil, errors.Wrap(err, "listing RoleBindings") + } + for idx, rb := range rbList.Items { + roleRef := v1beta1.RootSyncRoleRef{ + Kind: rb.RoleRef.Kind, + Name: rb.RoleRef.Name, + Namespace: rb.Namespace, + } + currentRoleMap[roleRef] = &rbList.Items[idx] } - - if roleConfig.Disabled { - return errors.Wrap( // wrap returns nil if err is nil - r.deleteClusterRoleBinding(ctx, RootSyncPermissionsName(reconcilerRef.Name)), - "ensuring clusterrolebinding does not exist", - ) + crbList := rbacv1.ClusterRoleBindingList{} + if err := r.client.List(ctx, &crbList, opts); err != nil { + return nil, errors.Wrap(err, "listing ClusterRoleBindings") + } + for idx, crb := range crbList.Items { + roleRef := v1beta1.RootSyncRoleRef{ + Kind: crb.RoleRef.Kind, + Name: crb.RoleRef.Name, + } + currentRoleMap[roleRef] = &crbList.Items[idx] } + return currentRoleMap, nil +} - if roleConfig.Name == "" { - roleConfig.Name = "cluster-admin" +func (r *RootSyncReconciler) cleanRBACBindings(ctx context.Context, reconcilerRef, rsRef types.NamespacedName) error { + currentRefMap, err := r.listCurrentRoleRefs(ctx, rsRef) + if err != nil { + return err + } + for _, binding := range currentRefMap { + if err := r.cleanup(ctx, binding); err != nil { + return errors.Wrap(err, "deleting RBAC Binding") + } } + if err := r.deleteSharedClusterRoleBinding(ctx, RootSyncLegacyClusterRoleBindingName, reconcilerRef); err != nil && !apierrors.IsNotFound(err) { + return errors.Wrap(err, "deleting legacy binding") + } + if err := r.deleteSharedClusterRoleBinding(ctx, RootSyncBaseClusterRoleBindingName, reconcilerRef); err != nil && !apierrors.IsNotFound(err) { + return errors.Wrap(err, "deleting base binding") + } + return nil +} - _, err := r.upsertClusterRoleBinding(ctx, reconcilerRef, roleConfig.Name) - return errors.Wrap(err, "upserting cluster role binding") +// manageRBACBindings will reconcile the managed RBAC bindings on the cluster +// with what is declared in spec.overrides.roleRefs. +func (r *RootSyncReconciler) manageRBACBindings(ctx context.Context, reconcilerRef, rsRef types.NamespacedName, roleRefs []v1beta1.RootSyncRoleRef) error { + currentRefMap, err := r.listCurrentRoleRefs(ctx, rsRef) + if err != nil { + return err + } + if len(roleRefs) == 0 { + // Backwards compatible behavior to default to cluster-admin + if err := r.upsertSharedClusterRoleBinding(ctx, RootSyncLegacyClusterRoleBindingName, "cluster-admin", reconcilerRef); err != nil { + return err + } + // Clean up any RoleRefs created previously that are no longer declared + for _, binding := range currentRefMap { + if err := r.cleanup(ctx, binding); err != nil { + return errors.Wrap(err, "deleting RBAC Binding") + } + } + if err := r.deleteSharedClusterRoleBinding(ctx, RootSyncBaseClusterRoleBindingName, reconcilerRef); err != nil && !apierrors.IsNotFound(err) { + return errors.Wrap(err, "deleting base binding") + } + return nil + } + // Add the base ClusterRole for basic root reconciler functionality + if err := r.upsertSharedClusterRoleBinding(ctx, RootSyncBaseClusterRoleBindingName, RootSyncBaseClusterRoleName, reconcilerRef); err != nil { + return err + } + declaredRoleRefs := roleRefs + // Create RoleBindings which are declared but do not exist on the cluster + for _, roleRef := range declaredRoleRefs { + if _, ok := currentRefMap[roleRef]; !ok { + // we need to call a separate create method here for generateName + if _, err := r.createRBACBinding(ctx, reconcilerRef, rsRef, roleRef); err != nil { + return errors.Wrap(err, "creating RBAC Binding") + } + } + } + // convert to map for lookup + declaredRefMap := make(map[v1beta1.RootSyncRoleRef]bool) + for _, roleRef := range declaredRoleRefs { + declaredRefMap[roleRef] = true + } + // For existing RoleBindings: + // - if they are declared in roleRefs, update + // - if they are no longer declared in roleRefs, delete + for roleRef, binding := range currentRefMap { + if _, ok := declaredRefMap[roleRef]; ok { // update + if err := r.updateRBACBinding(ctx, reconcilerRef, binding); err != nil { + return errors.Wrap(err, "upserting RBAC Binding") + } + } else { // Clean up any RoleRefs created previously that are no longer declared + if err := r.cleanup(ctx, binding); err != nil { + return errors.Wrap(err, "deleting RBAC Binding") + } + } + } + // in order to be backwards compatible with behavior before https://github.com/GoogleContainerTools/kpt-config-sync/pull/938 + // we delete any ClusterRoleBinding that uses the old name (configsync.gke.io:root-reconciler). + // In older versions, this ClusterRoleBinding was always bound to cluster-admin. + // This ensures smooth migrations for users upgrading past that version boundary. + if err := r.deleteSharedClusterRoleBinding(ctx, RootSyncLegacyClusterRoleBindingName, reconcilerRef); err != nil && !apierrors.IsNotFound(err) { + return errors.Wrap(err, "deleting legacy binding") + } + return nil } -func (r *RootSyncReconciler) upsertClusterRoleBinding(ctx context.Context, reconcilerRef types.NamespacedName, clusterRole string) (client.ObjectKey, error) { - crbRef := client.ObjectKey{Name: RootSyncPermissionsName(reconcilerRef.Name)} +func (r *RootSyncReconciler) upsertSharedClusterRoleBinding(ctx context.Context, name, clusterRole string, reconcilerRef types.NamespacedName) error { + crbRef := client.ObjectKey{Name: name} childCRB := &rbacv1.ClusterRoleBinding{} childCRB.Name = crbRef.Name op, err := CreateOrUpdate(ctx, r.client, childCRB, func() error { childCRB.OwnerReferences = nil childCRB.RoleRef = rolereference(clusterRole, "ClusterRole") - childCRB.Subjects = []rbacv1.Subject{r.serviceAccountSubject(reconcilerRef)} + childCRB.Subjects = addSubject(childCRB.Subjects, r.serviceAccountSubject(reconcilerRef)) // Remove existing OwnerReferences, now that we're using finalizers. childCRB.OwnerReferences = nil return nil }) if err != nil { - return crbRef, err + return err } if op != controllerutil.OperationResultNone { r.logger(ctx).Info("Managed object upsert successful", @@ -824,7 +949,40 @@ func (r *RootSyncReconciler) upsertClusterRoleBinding(ctx context.Context, recon logFieldObjectKind, "ClusterRoleBinding", logFieldOperation, op) } - return crbRef, nil + return nil +} + +func (r *RootSyncReconciler) createRBACBinding(ctx context.Context, reconcilerRef, rsRef types.NamespacedName, roleRef v1beta1.RootSyncRoleRef) (client.ObjectKey, error) { + var binding client.Object + if roleRef.Namespace == "" { + crb := rbacv1.ClusterRoleBinding{} + crb.RoleRef = rolereference(roleRef.Name, roleRef.Kind) + crb.Subjects = []rbacv1.Subject{r.serviceAccountSubject(reconcilerRef)} + binding = &crb + } else { + rb := rbacv1.RoleBinding{} + rb.Namespace = roleRef.Namespace + rb.RoleRef = rolereference(roleRef.Name, roleRef.Kind) + rb.Subjects = []rbacv1.Subject{r.serviceAccountSubject(reconcilerRef)} + binding = &rb + } + // use generateName to produce a unique name. A predictable unique name + // is not feasible, since it would risk hitting length constraints. + // e.g. reconciler.name + roleRef.kind + roleRef.name + binding.SetGenerateName(fmt.Sprintf("%s-", reconcilerRef.Name)) + binding.SetLabels(ManagedObjectLabelMap(r.syncKind, rsRef)) + + if err := r.client.Create(ctx, binding); err != nil { + return client.ObjectKey{}, err + } + rbRef := client.ObjectKey{ + Name: binding.GetName(), + Namespace: binding.GetNamespace(), + } + r.logger(ctx).Info("Managed object create successful", + logFieldObjectRef, rbRef.String(), + logFieldObjectKind, binding.GetObjectKind().GroupVersionKind().Kind) + return rbRef, nil } func (r *RootSyncReconciler) updateSyncStatus(ctx context.Context, rs *v1beta1.RootSync, reconcilerRef types.NamespacedName, updateFn func(*v1beta1.RootSync) error) (bool, error) { diff --git a/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go b/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go index b0b5bf1fa0..22e5d2685f 100644 --- a/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go +++ b/pkg/reconcilermanager/controllers/rootsync_controller_manager_test.go @@ -407,9 +407,9 @@ func TestRootSyncReconcilerServiceAccountDriftProtection(t *testing.T) { // reverted if changed by another client. func TestRootSyncReconcilerClusterRoleBindingDriftProtection(t *testing.T) { exampleObj := &rbacv1.ClusterRoleBinding{} - objKeyFunc := func(rsKey client.ObjectKey) client.ObjectKey { + objKeyFunc := func(_ client.ObjectKey) client.ObjectKey { // reconciler-manager managed cluster role binding - return client.ObjectKey{Name: RootSyncPermissionsName(core.RootReconcilerName(rsKey.Name))} + return client.ObjectKey{Name: RootSyncLegacyClusterRoleBindingName} } var oldObj *rbacv1.ClusterRoleBinding var oldValue string diff --git a/pkg/reconcilermanager/controllers/rootsync_controller_test.go b/pkg/reconcilermanager/controllers/rootsync_controller_test.go index 9abcb3f05e..801d987d58 100644 --- a/pkg/reconcilermanager/controllers/rootsync_controller_test.go +++ b/pkg/reconcilermanager/controllers/rootsync_controller_test.go @@ -25,6 +25,7 @@ import ( "github.com/go-logr/logr/testr" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/pkg/errors" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -32,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/json" "k8s.io/utils/pointer" @@ -70,6 +72,13 @@ const ( var rootReconcilerName = core.RootReconcilerName(rootsyncName) +func role(name string, opts ...core.MetaMutator) *rbacv1.Role { + result := fake.RoleObject(opts...) + result.Name = name + + return result +} + func clusterrole(t *testing.T, name string, opts ...core.MetaMutator) *rbacv1.ClusterRole { t.Helper() @@ -90,6 +99,48 @@ func clusterrolebinding(name string, role string, opts ...core.MetaMutator) *rba return result } +func rootReconcilerClusterRoleBinding(reconcilerName, clusterRole string, opts ...core.MetaMutator) *rbacv1.ClusterRoleBinding { + defaultOpts := []core.MetaMutator{ + core.Labels(map[string]string{ + metadata.SyncKindLabel: configsync.RootSyncKind, + metadata.SyncNameLabel: rootsyncName, + metadata.SyncNamespaceLabel: configsync.ControllerNamespace, + }), + core.GenerateName(reconcilerName + "-"), + core.Generation(1), + core.UID("1"), + core.ResourceVersion("1"), + } + opts = append(defaultOpts, opts...) + result := clusterrolebinding("", clusterRole, opts...) + result.Subjects = addSubjectByName(nil, reconcilerName) + return result +} + +func rootReconcilerRoleBinding(roleRef v1beta1.RootSyncRoleRef, opts ...core.MetaMutator) *rbacv1.RoleBinding { + defaultOpts := []core.MetaMutator{ + core.Namespace(roleRef.Namespace), + core.Labels(map[string]string{ + metadata.SyncKindLabel: configsync.RootSyncKind, + metadata.SyncNameLabel: rootsyncName, + metadata.SyncNamespaceLabel: configsync.ControllerNamespace, + }), + core.GenerateName("root-reconciler-my-root-sync-"), + core.Generation(1), + core.UID("1"), + core.ResourceVersion("1"), + } + opts = append(defaultOpts, opts...) + result := rolebinding("", roleRef.Name, roleRef.Kind, opts...) + result.Subjects = addSubjectByName(nil, rootReconcilerName) + return result +} + +func defaultRootReconcilerClusterRoleBindingMap() map[v1beta1.RootSyncRoleRef]rbacv1.ClusterRoleBinding { + defaultMap := make(map[v1beta1.RootSyncRoleRef]rbacv1.ClusterRoleBinding) + return defaultMap +} + func configMapWithData(namespace, name string, data map[string]string, opts ...core.MetaMutator) *corev1.ConfigMap { baseOpts := []core.MetaMutator{ core.Labels(map[string]string{ @@ -226,9 +277,9 @@ func rootsyncOverrideAPIServerTimeout(apiServerTimout metav1.Duration) func(*v1b } } -func rootsyncOverrideClusterRole(clusterRole string) func(*v1beta1.RootSync) { +func rootsyncOverrideRoleRefs(roleRefs ...v1beta1.RootSyncRoleRef) func(*v1beta1.RootSync) { return func(rs *v1beta1.RootSync) { - rs.Spec.SafeOverride().ClusterRole.Name = clusterRole + rs.Spec.SafeOverride().RoleRefs = roleRefs } } @@ -1507,17 +1558,116 @@ func TestRootSyncUpdateOverrideAPIServerTimeout(t *testing.T) { t.Log("No need to update Deployment.") } -func TestRootSyncCreateWithOverrideClusterRole(t *testing.T) { +func validateReconcilerClusterRoleBindings(fakeClient *syncerFake.Client, rootSyncName string, want map[v1beta1.RootSyncRoleRef]rbacv1.ClusterRoleBinding) error { + got := &rbacv1.ClusterRoleBindingList{} + ctx := context.Background() + + opts := &client.ListOptions{} + opts.LabelSelector = client.MatchingLabelsSelector{ + Selector: labels.SelectorFromSet(map[string]string{ + metadata.SyncKindLabel: configsync.RootSyncKind, + metadata.SyncNameLabel: rootSyncName, + metadata.SyncNamespaceLabel: configsync.ControllerNamespace, + }), + } + err := fakeClient.List(ctx, got, opts) + if err != nil { + return err + } + if len(want) != len(got.Items) { + return errors.Errorf("want %d ClusterRoleBindings, got %d", len(want), len(got.Items)) + } + gotCRBMap := make(map[v1beta1.RootSyncRoleRef]rbacv1.ClusterRoleBinding) + for idx, crb := range got.Items { + roleRef := v1beta1.RootSyncRoleRef{ + Kind: crb.RoleRef.Kind, + Name: crb.RoleRef.Name, + } + gotCRBMap[roleRef] = got.Items[idx] + } + for roleRef, wantCRB := range want { + gotCRB, ok := gotCRBMap[roleRef] + if !ok { + return errors.Errorf("ClusterRoleBinding for roleRef %v not found", roleRef) + } + wantCRB.Name = gotCRB.Name + if diff := cmp.Diff(wantCRB, gotCRB, cmpopts.EquateEmpty()); diff != "" { + return errors.Errorf("ClusterRoleBinding[%s] diff: %s", wantCRB.Name, diff) + } + } + return nil +} + +func validateReconcilerRoleBindings(fakeClient *syncerFake.Client, syncKind string, rsRef types.NamespacedName, want map[v1beta1.RootSyncRoleRef]rbacv1.RoleBinding) error { + got := &rbacv1.RoleBindingList{} + ctx := context.Background() + + opts := &client.ListOptions{} + opts.LabelSelector = client.MatchingLabelsSelector{ + Selector: labels.SelectorFromSet(map[string]string{ + metadata.SyncKindLabel: syncKind, + metadata.SyncNameLabel: rsRef.Name, + metadata.SyncNamespaceLabel: rsRef.Namespace, + }), + } + err := fakeClient.List(ctx, got, opts) + if err != nil { + return err + } + if len(want) != len(got.Items) { + return errors.Errorf("want %d RoleBindings, got %d", len(want), len(got.Items)) + } + gotRBMap := make(map[v1beta1.RootSyncRoleRef]rbacv1.RoleBinding) + for idx, rb := range got.Items { + roleRef := v1beta1.RootSyncRoleRef{ + Kind: rb.RoleRef.Kind, + Name: rb.RoleRef.Name, + Namespace: rb.Namespace, + } + gotRBMap[roleRef] = got.Items[idx] + } + for roleRef, wantRB := range want { + gotRB, ok := gotRBMap[roleRef] + if !ok { + return errors.Errorf("ClusterRoleBinding for roleRef %v not found", roleRef) + } + wantRB.Name = gotRB.Name + if diff := cmp.Diff(wantRB, gotRB, cmpopts.EquateEmpty()); diff != "" { + return errors.Errorf("ClusterRoleBinding[%s] diff: %s", wantRB.Name, diff) + } + } + return nil +} + +func TestRootSyncCreateWithOverrideRoleRefs(t *testing.T) { // Mock out parseDeployment for testing. parseDeployment = parsedDeployment - clusterRoleName := "my-custom-role" - + clusterRoleRef1 := v1beta1.RootSyncRoleRef{ + Kind: "ClusterRole", + Name: "my-cluster-role", + } + clusterRoleRef2 := v1beta1.RootSyncRoleRef{ + Kind: "ClusterRole", + Name: "my-tenant-role", + Namespace: "tenant-ns", + } + roleRef := v1beta1.RootSyncRoleRef{ + Kind: "Role", + Name: "my-cluster-role", + Namespace: "foo-ns", + } + rootSyncNN := types.NamespacedName{ + Name: rootsyncName, + Namespace: configsync.ControllerNamespace, + } rs := rootSyncWithOCI(rootsyncName, - rootsyncOverrideClusterRole(clusterRoleName), + rootsyncOverrideRoleRefs(clusterRoleRef1, clusterRoleRef2, roleRef), rootsyncOCIAuthType(configsync.AuthGCENode)) reqNamespacedName := namespacedName(rs.Name, rs.Namespace) - fakeClient, _, testReconciler := setupRootReconciler(t, rs, clusterrole(t, clusterRoleName)) + fakeClient, _, testReconciler := setupRootReconciler(t, rs, + clusterrole(t, clusterRoleRef1.Name), clusterrole(t, clusterRoleRef2.Name), + role(roleRef.Name)) // Test creating Deployment resources. ctx := context.Background() @@ -1525,60 +1675,65 @@ func TestRootSyncCreateWithOverrideClusterRole(t *testing.T) { t.Fatalf("unexpected reconciliation error, got error: %q, want error: nil", err) } - want := clusterrolebinding( - RootSyncPermissionsName(rootReconcilerName), - clusterRoleName, - core.UID("1"), core.ResourceVersion("1"), core.Generation(1)) - want.Subjects = addSubjectByName(nil, rootReconcilerName) + wantClusterRoleBindings := defaultRootReconcilerClusterRoleBindingMap() + wantClusterRoleBindings[clusterRoleRef1] = *rootReconcilerClusterRoleBinding( + rootReconcilerName, clusterRoleRef1.Name) + wantRoleBindings := make(map[v1beta1.RootSyncRoleRef]rbacv1.RoleBinding) + wantRoleBindings[clusterRoleRef2] = *rootReconcilerRoleBinding( + clusterRoleRef2) + wantRoleBindings[roleRef] = *rootReconcilerRoleBinding( + roleRef) - if err := validateClusterRoleBinding(want, fakeClient); err != nil { - t.Errorf("ClusterRoleBinding validation failed. err: %v", err) + if err := validateReconcilerClusterRoleBindings(fakeClient, rootsyncName, wantClusterRoleBindings); err != nil { + t.Fatalf("ClusterRoleBinding validation failed. err: %v", err) } - if t.Failed() { - t.FailNow() + t.Log("ClusterRoleBindings successfully created") + if err := validateReconcilerRoleBindings(fakeClient, configsync.RootSyncKind, rootSyncNN, wantRoleBindings); err != nil { + t.Fatalf("RoleBinding validation failed. err: %v", err) + } + defaultCrb := clusterrolebinding( + RootSyncBaseClusterRoleBindingName, + RootSyncBaseClusterRoleName, + core.UID("1"), core.ResourceVersion("1"), core.Generation(1), + ) + defaultCrb.Subjects = addSubjectByName(nil, rootReconcilerName) + + if err := validateClusterRoleBinding(defaultCrb, fakeClient); err != nil { + t.Fatal(err) } - t.Log("ClusterRoleBinding successfully created") + t.Log("RoleBindings successfully created") if err := fakeClient.Get(ctx, client.ObjectKeyFromObject(rs), rs); err != nil { t.Fatalf("Failed to get RootSync: %q", err) } - rs.Spec.SafeOverride().ClusterRole.Name = "" + rs.Spec.SafeOverride().RoleRefs = nil if err := fakeClient.Update(ctx, rs); err != nil { t.Fatalf("Failed to update RootSync: %q", err) } - t.Log("RootSync updated to remove ClusteRole override") + t.Log("RootSync updated to remove RoleRefs override") if _, err := testReconciler.Reconcile(ctx, reqNamespacedName); err != nil { t.Fatalf("unexpected reconciliation error, got error: %q, want error: nil", err) } - // clusterrolebinding is updated in-place - want = clusterrolebinding( - RootSyncPermissionsName(rootReconcilerName), - "cluster-admin", - core.UID("1"), core.ResourceVersion("2"), core.Generation(2)) - want.Subjects = addSubjectByName(nil, rootReconcilerName) - if err := validateClusterRoleBinding(want, fakeClient); err != nil { - t.Errorf("ClusterRoleBinding validation failed. err: %v", err) + if err := validateReconcilerClusterRoleBindings(fakeClient, rootReconcilerName, nil); err != nil { + t.Fatalf("ClusterRoleBinding validation failed. err: %v", err) } - - if err := fakeClient.Get(ctx, client.ObjectKeyFromObject(rs), rs); err != nil { - t.Fatalf("Failed to get RootSync: %q", err) - } - - rs.Spec.SafeOverride().ClusterRole.Disabled = true - if err := fakeClient.Update(ctx, rs); err != nil { - t.Fatalf("Failed to update RootSync: %q", err) + if err := validateReconcilerRoleBindings(fakeClient, configsync.RootSyncKind, rootSyncNN, nil); err != nil { + t.Fatalf("RoleBinding validation failed. err: %v", err) } - t.Log("RootSync updated to remove ClusteRole entirely") - - if _, err := testReconciler.Reconcile(ctx, reqNamespacedName); err != nil { - t.Fatalf("unexpected reconciliation error, got error: %q, want error: nil", err) + if err := validateResourceDeleted(core.IDOf(defaultCrb), fakeClient); err != nil { + t.Fatal(err) } - - if err := validateResourceDeleted(core.IDOf(want), fakeClient); err != nil { - t.Error(err) + clusterAdminCRB := clusterrolebinding( + RootSyncLegacyClusterRoleBindingName, + "cluster-admin", + core.UID("1"), core.ResourceVersion("1"), core.Generation(1), + ) + clusterAdminCRB.Subjects = addSubjectByName(nil, rootReconcilerName) + if err := validateClusterRoleBinding(clusterAdminCRB, fakeClient); err != nil { + t.Fatal(err) } } @@ -1590,16 +1745,19 @@ func TestMigrationToIndividualClusterRoleBindingsWhenDefaultRootSyncExists(t *te // two syncs: root-sync and my-custom-sync rs1 := rootSyncWithOCI(configsync.RootSyncName, rootsyncOCIAuthType(configsync.AuthGCENode)) + rs1ReconcilerName := core.RootReconcilerName(rs1.Name) rs2 := rootSyncWithOCI("my-custom-sync", rootsyncOCIAuthType(configsync.AuthGCENode)) + rs2ReconcilerName := core.RootReconcilerName(rs2.Name) oldBinding := clusterrolebinding( - fmt.Sprintf("%s:%s", core.RootReconcilerPrefix, configsync.RootSyncName), + RootSyncLegacyClusterRoleBindingName, "cluster-admin", + core.UID("1"), core.ResourceVersion("1"), core.Generation(1), ) - oldBinding.Subjects = addSubjectByName(nil, core.RootReconcilerName(rs1.Name)) - oldBinding.Subjects = addSubjectByName(oldBinding.Subjects, core.RootReconcilerName(rs2.Name)) + oldBinding.Subjects = addSubjectByName(nil, rs1ReconcilerName) + oldBinding.Subjects = addSubjectByName(oldBinding.Subjects, rs2ReconcilerName) fakeClient, _, testReconciler := setupRootReconciler(t, rs1, rs2, oldBinding) @@ -1612,22 +1770,8 @@ func TestMigrationToIndividualClusterRoleBindingsWhenDefaultRootSyncExists(t *te t.Fatalf("unexpected reconciler error: %v", err) } - if err := validateResourceDeleted(core.IDOf(oldBinding), fakeClient); err != nil { - t.Errorf("validating deletion of old binding: %v", err) - } - - want1 := clusterrolebinding(RootSyncPermissionsName(rs1.Name), "cluster-admin", - core.UID("1"), core.ResourceVersion("1"), core.Generation(1)) - want1.Subjects = addSubjectByName(nil, core.RootReconcilerName(rs1.Name)) - if err := validateClusterRoleBinding(want1, fakeClient); err != nil { - t.Errorf("ClusterRoleBinding validation failed. err: %v", err) - } - - want2 := clusterrolebinding(RootSyncPermissionsName(rs2.Name), "cluster-admin", - core.UID("1"), core.ResourceVersion("1"), core.Generation(1)) - want2.Subjects = addSubjectByName(nil, core.RootReconcilerName(rs2.Name)) - if err := validateClusterRoleBinding(want2, fakeClient); err != nil { - t.Errorf("ClusterRoleBinding validation failed. err: %v", err) + if err := validateClusterRoleBinding(oldBinding, fakeClient); err != nil { + t.Fatal(err) } } @@ -1886,12 +2030,12 @@ func TestMultipleRootSyncs(t *testing.T) { ) wantServiceAccounts := map[core.ID]*corev1.ServiceAccount{core.IDOf(serviceAccount1): serviceAccount1} - crb1 := clusterrolebinding( - RootSyncPermissionsName(rootReconcilerName), + crb := clusterrolebinding( + RootSyncLegacyClusterRoleBindingName, "cluster-admin", core.UID("1"), core.ResourceVersion("1"), core.Generation(1), ) - crb1.Subjects = addSubjectByName(nil, rootReconcilerName) + crb.Subjects = addSubjectByName(crb.Subjects, rootReconcilerName) rootContainerEnv1 := testReconciler.populateContainerEnvs(ctx, rs1, rootReconcilerName) resourceOverrides := setContainerResourceDefaults(nil, ReconcilerContainerResourceDefaults()) rootDeployment1 := rootSyncDeployment(rootReconcilerName, @@ -1906,7 +2050,7 @@ func TestMultipleRootSyncs(t *testing.T) { if err := validateServiceAccounts(wantServiceAccounts, fakeClient); err != nil { t.Error(err) } - if err := validateClusterRoleBinding(crb1, fakeClient); err != nil { + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } if err := validateDeployments(wantDeployments, fakeDynamicClient); err != nil { @@ -1963,13 +2107,10 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - crb2 := clusterrolebinding( - RootSyncPermissionsName(rootReconcilerName2), - "cluster-admin", - core.UID("1"), core.ResourceVersion("1"), core.Generation(1), - ) - crb2.Subjects = addSubjectByName(nil, rootReconcilerName2) - if err := validateClusterRoleBinding(crb2, fakeClient); err != nil { + crb.Subjects = addSubjectByName(crb.Subjects, rootReconcilerName2) + crb.ResourceVersion = "2" + crb.Generation = 2 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } if t.Failed() { @@ -2024,13 +2165,10 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - crb3 := clusterrolebinding( - RootSyncPermissionsName(rootReconcilerName3), - "cluster-admin", - core.UID("1"), core.ResourceVersion("1"), core.Generation(1), - ) - crb3.Subjects = addSubjectByName(nil, rootReconcilerName2) - if err := validateClusterRoleBinding(crb3, fakeClient); err != nil { + crb.Subjects = addSubjectByName(crb.Subjects, rootReconcilerName3) + crb.ResourceVersion = "3" + crb.Generation = 3 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } if t.Failed() { @@ -2088,13 +2226,10 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - crb4 := clusterrolebinding( - RootSyncPermissionsName(rootReconcilerName4), - "cluster-admin", - core.UID("1"), core.ResourceVersion("1"), core.Generation(1), - ) - crb4.Subjects = addSubjectByName(nil, rootReconcilerName2) - if err := validateClusterRoleBinding(crb4, fakeClient); err != nil { + crb.Subjects = addSubjectByName(crb.Subjects, rootReconcilerName4) + crb.ResourceVersion = "4" + crb.Generation = 4 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } if t.Failed() { @@ -2153,13 +2288,10 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - crb5 := clusterrolebinding( - RootSyncPermissionsName(rootReconcilerName5), - "cluster-admin", - core.UID("1"), core.ResourceVersion("1"), core.Generation(1), - ) - crb5.Subjects = addSubjectByName(nil, rootReconcilerName2) - if err := validateClusterRoleBinding(crb5, fakeClient); err != nil { + crb.Subjects = addSubjectByName(crb.Subjects, rootReconcilerName5) + crb.ResourceVersion = "5" + crb.Generation = 5 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } if t.Failed() { @@ -2181,7 +2313,7 @@ func TestMultipleRootSyncs(t *testing.T) { } // No changes to the CRB - if err := validateClusterRoleBinding(crb1, fakeClient); err != nil { + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } @@ -2219,7 +2351,7 @@ func TestMultipleRootSyncs(t *testing.T) { } // No changes to the CRB - if err := validateClusterRoleBinding(crb2, fakeClient); err != nil { + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } @@ -2258,7 +2390,7 @@ func TestMultipleRootSyncs(t *testing.T) { } // No changes to the CRB - if err := validateClusterRoleBinding(crb3, fakeClient); err != nil { + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } @@ -2296,7 +2428,11 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - if err := validateResourceDeleted(core.IDOf(crb1), fakeClient); err != nil { + // Subject for rs1 is removed from ClusterRoleBinding.Subjects + crb.Subjects = deleteSubjectByName(crb.Subjects, rootReconcilerName) + crb.ResourceVersion = "6" + crb.Generation = 6 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } validateRootGeneratedResourcesDeleted(t, fakeClient, rootReconcilerName) @@ -2317,7 +2453,11 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - if err := validateResourceDeleted(core.IDOf(crb2), fakeClient); err != nil { + // Subject for rs2 is removed from ClusterRoleBinding.Subjects + crb.Subjects = deleteSubjectByName(crb.Subjects, rootReconcilerName2) + crb.ResourceVersion = "7" + crb.Generation = 7 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } validateRootGeneratedResourcesDeleted(t, fakeClient, rootReconcilerName2) @@ -2338,7 +2478,11 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - if err := validateResourceDeleted(core.IDOf(crb3), fakeClient); err != nil { + // Subject for rs3 is removed from ClusterRoleBinding.Subjects + crb.Subjects = deleteSubjectByName(crb.Subjects, rootReconcilerName3) + crb.ResourceVersion = "8" + crb.Generation = 8 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } validateRootGeneratedResourcesDeleted(t, fakeClient, rootReconcilerName3) @@ -2359,7 +2503,11 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - if err := validateResourceDeleted(core.IDOf(crb4), fakeClient); err != nil { + // Subject for rs4 is removed from ClusterRoleBinding.Subjects + crb.Subjects = deleteSubjectByName(crb.Subjects, rootReconcilerName4) + crb.ResourceVersion = "9" + crb.Generation = 9 + if err := validateClusterRoleBinding(crb, fakeClient); err != nil { t.Error(err) } validateRootGeneratedResourcesDeleted(t, fakeClient, rootReconcilerName4) @@ -2380,7 +2528,8 @@ func TestMultipleRootSyncs(t *testing.T) { t.Error(err) } - if err := validateResourceDeleted(core.IDOf(crb5), fakeClient); err != nil { + // Verify the ClusterRoleBinding of the root-reconciler is deleted + if err := validateResourceDeleted(core.IDOf(crb), fakeClient); err != nil { t.Error(err) } validateRootGeneratedResourcesDeleted(t, fakeClient, rootReconcilerName5) diff --git a/pkg/syncer/syncertest/fake/storage.go b/pkg/syncer/syncertest/fake/storage.go index 6a54f8d06f..d632a9caba 100644 --- a/pkg/syncer/syncertest/fake/storage.go +++ b/pkg/syncer/syncertest/fake/storage.go @@ -32,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + utilrand "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/watch" "k8s.io/klog/v2" @@ -389,6 +390,12 @@ func (ms *MemoryStorage) Create(ctx context.Context, obj client.Object, opts *cl if err != nil { return err } + // create a generated name + // See https://github.com/kubernetes/apiserver/blob/9d3d7b483aa6949b82103fc5b0f132126676c37b/pkg/storage/names/generate.go + if obj.GetName() == "" && obj.GetGenerateName() != "" { + randomString := utilrand.String(5) + obj.SetName(obj.GetGenerateName() + randomString) + } // Convert to a typed object for storage, with GVK populated. tObj, err := toTypedClientObject(obj, ms.scheme)