feat: add roleRefs to RSync override API

The roleRefs field is intended to allow more flexible and simple
management of bindings for both RootSync and RepoSync objects. The field
allows a list of references to Role and ClusterRole objects. For
RepoSyncs, this will always create a RoleBinding in the same namespace
as the RepoSync. For RootSyncs, the user can either choose to create a
RoleBinding or ClusterRoleBinding via the namespace field.

This is an iteration upon the previous design which added a single
ClusterRoleBinding configuration for RootSyncs. This design allows more
flexible configuration for RootSyncs, as well as a consistent API for
RepoSyncs which simplifies the management of RepoSync RoleBindings.

docs/design-docs/02-custom-root-reconciler-clusterrole.md

@@ -1,6 +1,6 @@
 # Custom `ClusterRole` assignments for `RootSync`s
 
-* Author(s): @tomasaschan
+* Author(s): @tomasaschan, @sdowell
 * Approver: @sdowell, @janetkuo
 * Status: approved
 
@@ -9,7 +9,13 @@
 When creating a reconciler deployment for a `RootSync`, Config Sync currently [creates a
 `ClusterRoleBinding`] for the generated `ServiceAccount` granting it `cluster-admin`.
 
-This proposal introduces a mechanism to customize this behavior.
+On the other hand, when creating a reconciler deployment for a `RepoSync`, Config Sync
+currently creates `RoleBinding` for the generated `ServiceAccount` granting
+it `configsync.gke.io:ns-reconciler`. This default `ClusterRole` grants basic permissions required for the `RepoSync`
+reconciler to function, but it's up to users to manage additional `RoleBinding`s.
+
+This proposal introduces a manage the lifecycle of RBAC bindings for both `RootSync`
+and `RepoSync` objects.
 
 [creates a `ClusterRoleBinding`]: https://github.com/GoogleContainerTools/kpt-config-sync/blob/199db6dbfa0b9eb1824925c8c687574de095a294/pkg/reconcilermanager/controllers/rootsync_controller.go#L789
 
@@ -18,65 +24,130 @@ This proposal introduces a mechanism to customize this behavior.
 If one wants to allow deploying resources from a source where one doesn't have full
 control of the contents of the upstream config source, and want to limit what another
 team or third party can accomplish in the cluster, one can imagine limiting what
-permissions are granted to the `ServiceAccount` running the reconciler.
+permissions are granted to the `ServiceAccount` running the `RootSync` reconciler.
 
 However, as Config Sync is currently granting `cluster-admin`, any custom role bindings
-are effectively ignored; the reconciler will, due to the binding added by Config Sync,
+are effectively ignored; the `RootSync` reconciler will, due to the binding added by Config Sync,
 have access to do _anything_ regardless.
 
 To follow the [principle of least privilege], one should ensure the reconciler only has
 access to deploy the expected resources.
 
+Additionally, the `RepoSync` API currently requires for users to manage the lifecycle
+of `RoleBinding` objects themselves. The proposed API here is intended to also
+simplify the management of `RoleBinding`s for `RepoSync`s.
+
 See also [#935].
 
 [principle of least privilege]: https://en.wikipedia.org/wiki/Principle_of_least_privilege
 [#935]: https://github.com/GoogleContainerTools/kpt-config-sync/issues/935
 
 ## Design Overview
 
-By providing the name of a user-defined `ClusterRole` for a `RootSync`, a user can
+### `RootSync`s
+
+By providing the name of a list user-defined `RoleRef`s for a `RootSync`, a user can
 override which role Config Sync binds to. This configuration is exposed as a new field
 on `spec.overrides` for a `RootSync`:
 
 ```yaml
+kind: RootSync
+metadata:
+  name: my-root-sync
+  namespace: config-management-system
 spec:
   overrides:
-    clusterRole:
-      name: my-custom-role
+    roleRefs:
+    - kind: ClusterRole # Creates a ClusterRoleBinding bound to ClusterRole my-cluster-role
+      name: my-cluster-role
+    - kind: ClusterRole # Creates a RoleBinding in my-tenant-namespace bound to ClusterRole tenant-cluster-role
+      name: tenant-cluster-role
+      namespace: my-tenant-namespace
+    - kind: Role # Creates a RoleBinding in my-tenant-namespace bound to Role my-tenant-role
+      name: my-tenant-role
+      namespace: my-tenant-namespace
 ```
 
-This role is defaulted to `cluster-admin` in order to stay backwards compatible.
+For `RootSync` objects, the default behavior will create a ClusterRoleBinding
+to `cluster-admin` in order to stay backwards compatible.
+
+For convenience, `RootSync` reconcilers will also be bound to a base `ClusterRole`
+which gives the reconciler the permissions for basic functionality. This will
+be comparable to the pre-existing base ClusterRole for `RepoSync` reconcilers,
+which includes permissions such as status writing on `RepoSync` objects. Leaving
+this permission to the user to manage would create unneeded toil.
+
+### `RepoSync`s
 
-For the case where a single `ClusterRole` is not expressive enough to configure the
-permissions a user want, you can instead set `disabled: true` on the override object:
+For `RepoSync` objects, the `namespace` field will not be present on `roleRefs`.
+Instead, the `namespace` will always be derived from the `RepoSync` object itself.
+This follows the assumption that `RepoSync` objects are always scoped to a single
+Namespace.
 
 ```yaml
+kind: RepoSync
+metadata:
+  name: my-repo-sync
+  namespace: example-ns
 spec:
   overrides:
-    clusterRole:
-      disabled: true
+    roleRefs:
+    - kind: ClusterRole # Creates a RoleBinding in example-ns bound to ClusterRole my-cluster-role
+      name: my-cluster-role
+    - kind: Role # Creates a RoleBinding in example-ns bound to Role my-tenant-role
+      name: my-tenant-role
 ```
-This disables creating the `ClusterRoleBinding` entirely.
 
-If both `disabled: true` and a custom name is specified, `disabled` "wins" and no binding
-is created.
+### Lifecycle management
+
+Given this API provides configuration for a list of RoleRefs, it requires some form
+of lifecycle management to clean up stale bindings. For example if a user removes
+one roleRef from the list of roleRefs, they would reasonably expect that the
+binding will be garbage collected.
+
+This essentially requires for the `reconciler-manager` to be able to track an
+inventory of bindings that were previously created for a given `RootSync` or `RepoSync`.
+This can be accomplished by applying a label whenever a new binding is created,
+and then querying using a label selector on subsequent reconciliation loops.
+
+The following label will be applied to binding objects:
+```yaml
+metadata:
+  labels:
+    configsync.gke.io-reconciler: <RECONCILER_NAME>
+```
+
+The reconciler name is the name of the reconciler Deployment in the `config-management-system`
+namespace, and is guaranteed to be unique for each reconciler.
 
 ## User Guide
 
-To take advantage of this new feature, set `spec.overrides.clusterRole` to the name of a
-`ClusterRole` you wish this `RootSync` to be reconciled under; you must create this role
-yourself, but Config Sync will create the `ClusterRoleBinding` for you.
+### `RootSync`
+
+To use this feature for a `RootSync`, set `spec.overrides.roleRefs` to reference
+any number of `ClusterRole` or `Role` objects you wish this `RootSync` to be bound
+to. If you wish to create a `RoleBinding` rather than a `ClusterRoleBinding`,
+set the `namespace` field of the `roleRef` to the desired Namespace.
+You must create the `ClusterRole`/`Role` yourself, but Config Sync will create the
+`ClusterRoleBinding`/`RoleBinding` for you.
 
 ```yaml
 apiVersion: configsync.gke.io/v1beta1
 kind: RootSync
 metadata:
-  name: root-sync
+  name: my-root-sync
   namespace: config-management-system
 spec:
   overrides:
-    clusterRole:
+    roleRefs:
+    - kind: ClusterRole # Create ClusterRoleBinding to my-cluster-role
       name: my-cluster-role
+    - kind: ClusterRole # Create RoleBinding to my-tenant-role
+      name: my-tenant-role
+      namespace: my-ns
+    - kind: Role # Create RoleBinding to my-role
+      name: my-role
+      namespace: my-ns
   # ...
 ---
 apiVersion: rbac.authorization.k8s.io/v1
@@ -85,26 +156,64 @@ metadata:
   name: my-cluster-role
 rules:
 # ...
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: my-tenant-role
+rules:
+# ...
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: my-role
+  namespace: my-ns
+rules:
+# ...
 ```
 
-You can also disable automatic binding to any role.
+### `RepoSync`
+
+To use this feature for a `RepoSync`, set `spec.overrides.roleRefs` to reference
+any number of `ClusterRole` or `Role` objects you wish this `RepoSync` to be bound
+to. Config Sync will only create `RoleBinding`s for `RepoSync`s in the same
+Namespace as the `RepoSync`. `ClusterRoleBinding` objects will not be created
+for `RepoSync`s, as they are scoped to a single Namespace.
+You must create the `ClusterRole`/`Role` yourself, but Config Sync will create the
+`RoleBinding` for you.
 
 ```yaml
 apiVersion: configsync.gke.io/v1beta1
-kind: RootSync
+kind: RepoSync
 metadata:
-  name: root-sync
-  namespace: config-management-system
+  name: my-repo-sync
+  namespace: my-ns
 spec:
   overrides:
-    clusterRole:
-      disabled: true
+    roleRefs:
+    - kind: ClusterRole # Create RoleBinding to my-tenant-role
+      name: my-tenant-role
+    - kind: Role # Create RoleBinding to my-role
+      name: my-role
   # ...
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: my-tenant-role
+rules:
+# ...
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: my-role
+  namespace: my-ns
+rules:
+# ...
 ```
 
-The reconciler will then run with effectively no permissions, until you manually create
-some role or cluster role bindings for it.
-
 **Note** that while the name of the relevant service account will _often_ be predictable
 as `root-reconciler-<root-reconciler-name>`, this is not guaranteed by Config Sync. You
 can always find the service account by its labels; in particular, the labels
@@ -128,6 +237,8 @@ One could imagine exposing settings with slightly different semantics - e.g. a s
 boolean for turning the `ClusterRoleBinding` off, or a setting to change what service
 account the reconciler is running with. However, these both put a bigger burden on the
 user in order to utilize them even for the simple use cases, which is why changing which
-role to bind to is probably the most user-friendly knob to expose. With the inclusion of
-a switch to turn off creating the binding entirely, the first of these alternatives is effectively
-supported as well, and the API supports extending for other use cases in the future. 
+roles to bind to is probably the most user-friendly knob to expose. Exposing an
+API which binds to a single `ClusterRole` was also considered, but this was decided
+against due to lack of flexibility. The proposed solution allows for binding to
+an arbitrary number of `Role`/`ClusterRole` objects, and should fit most expected
+use cases for users.

e2e/nomostest/nt.go

@@ -39,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"
@@ -966,3 +967,29 @@ 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(reconcilerName string) ([]rbacv1.RoleBinding, error) {
+	opts := []client.ListOption{
+		withLabelListOption(controllers.ReconcilerBindingLabelKey, reconcilerName),
+	}
+	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(reconcilerName string) ([]rbacv1.ClusterRoleBinding, error) {
+	opts := []client.ListOption{
+		withLabelListOption(controllers.ReconcilerBindingLabelKey, reconcilerName),
+	}
+	crbList := rbacv1.ClusterRoleBindingList{}
+	if err := nt.KubeClient.List(&crbList, opts...); err != nil {
+		return nil, errors.Wrap(err, "listing ClusterRoleBindings")
+	}
+	return crbList.Items, nil
+}

e2e/testcases/namespace_repo_test.go

@@ -329,7 +329,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}