feat: implement roleRefs API for RootSyncs #991
Conversation
|
/assign @karlkfi |
sdowell
force-pushed
the
rootsync-role-refs
branch
4 times, most recently
from
9ee283c
to
a8ae73e
Compare
|
/assign @janetkuo |
sdowell
force-pushed
the
rootsync-role-refs
branch
2 times, most recently
from
c07666a
to
4f1f633
Compare
sdowell
force-pushed
the
rootsync-role-refs
branch
from
4f1f633
to
8ea9d6c
Compare
| @@ -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? | |||
There was a problem hiding this comment.
This is supported kubernetes-sigs/controller-runtime#244
There was a problem hiding this comment.
Dug into this a bit and it looks like the implementation you linked is a setting at the level of the controller manager (SelectorsByObject). Since we use a shared controller manager for both RootSync and RepoSync reconcilers, I don't think we could filter all RoleBindings by a single label selector (based on the current labels).
This could potentially work if we added a new label that was applied to all managed objects (RoleBindings in this case, but could also apply to other objects) that is the same for both RootSync and RepoSync
There was a problem hiding this comment.
Generally when using the controller-manager you have to assume your watches are being shared, so you can't filter them without filtering them for all controllers (e.g. SelectorsByObject). There was discussion of making these dynamic based on watch selectors here, but it was never implemented due to caching complexities.
The general workaround is to use WithPredicates instead, which allows skipping reconciling of events for a specific controller unless they match the predicate.
There was a problem hiding this comment.
There's a builder.LabelSelectorPredicate, if you want.
There was a problem hiding this comment.
This could be added as a future optimization.
sdowell
force-pushed
the
rootsync-role-refs
branch
from
8ea9d6c
to
69ab777
Compare
| handler.EnqueueRequestsFromMapFunc(r.mapObjectToRootSync), | ||
| builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})). | ||
| // TODO: is it possible to watch with a label filter? | ||
| Watches(&source.Kind{Type: &rbacv1.RoleBinding{}}, |
There was a problem hiding this comment.
Can you only start watches when users opt-in to this feature?
There was a problem hiding this comment.
My understanding is that the watches are instantiated at controller startup, so I don't think this would be possible given that the feature is enabled on the RootSync.
As a side note I'm looking into adding a label selector for the shared cache, which should optimize all of these watches.
There was a problem hiding this comment.
Actually, I think we have code already that adds watches at runtime. It's not as pretty as this interface but it's doable. This could be a future optimization.
There was a problem hiding this comment.
sdowell
force-pushed
the
rootsync-role-refs
branch
from
69ab777
to
f2e36c4
Compare
sdowell
force-pushed
the
rootsync-role-refs
branch
from
f2e36c4
to
f36bc8d
Compare
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.
sdowell
force-pushed
the
rootsync-role-refs
branch
from
f36bc8d
to
03c2abf
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: janetkuo, karlkfi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit 51cb6d7
into
GoogleContainerTools:main
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.
Design doc: https://github.com/GoogleContainerTools/kpt-config-sync/blob/main/docs/design-docs/02-custom-root-reconciler-clusterrole.md