Repository navigation
fix(helm): narrow and separate controller RBAC permissions - #3378
Conversation
timflannagan
left a comment
There was a problem hiding this comment.
Makes sense. One question, and just playing catch up here, why do we need two separate roles/RBs when they both attach to the same SA? Couldn't we just have a single controller role here? That type of split would make more sense if there was a dedicated management and reconcile SA
To easily shut off the high impact apiserver capabilities if you don't need them. |
Is the approach we'd want to take in the future, or does that work today? Fine to build on the current split, but would be surprised if this works today without breaking the control plane without more investment into shoring that up? |
Most likely it would break, more of a forward thinking investment |
Just wanted to confirm that first. I think I'd still advocate for a single controller owned role, and still descope some of these permissions since it's simpler for now while we still build out the core. The downside is that if we did want to split into N roles, make the investment in refactoring the control plane + chart structure to make it composable based on the functionality you need, etc. we'd have to figure out the migration path since role ref is immutable in k8s |
I think the controller code might randomly get mad, but i don't think the installation would break |
|
@EItanya merge conflicts but otherwise lgtm |
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
0e116cf to
933751f
Compare
The controller's shared Roles grant access to every core resource, including pods/exec, plus workload APIs that are no longer used now that runtimes run through Substrate. This PR replaces the core wildcard with explicit permissions and removes unused apps, batch, Gateway API, legacy sandbox, and RBAC-resource permissions.
Permissions are organized into reconciliation and management Roles, with both bindings always enabled on the controller ServiceAccount. The reconciliation Role owns shared reads, status updates, and finalizers; the management Role owns API writes. SandboxTemplate finalizers use the main resource endpoint, so reconciliation also needs update on SandboxTemplates. The structural split preserves the combined permissions.
Core access is limited to:
Both Roles follow the existing namespace scope, becoming ClusterRoles for cluster-wide installations. Leader election keeps its installation-namespace Role for leases and event writes. Secret list/watch remains necessary for the current informer-based dependency updates.
Validated with all 336 Helm tests, Helm lint, and 732 authorization assertions against an isolated Kubernetes API server, including each Role independently. Compared the rendered grants before and after the structural split in both scopes. Regression coverage includes required permissions, role responsibilities, bindings, and denied workload/RBAC access.
Related to #3329. Disabling management writes, get-only Secret reads, and separate WorkerPool watch namespaces remain follow-up work.
Fixes #3463.