Skip to content

fix(helm): narrow and separate controller RBAC permissions - #3378

Merged
EItanya merged 3 commits into
mainfrom
fix/obsolete-controller-rbac
Nov 2, 2026
Merged

EItanya merged 3 commits into
mainfrom
fix/obsolete-controller-rbac

Conversation

@EItanya

@EItanya EItanya commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • Secrets and ConfigMaps: get/list/watch and create/update/delete for configuration resolution and the management APIs.
  • Services: get/list/watch and delete for the existing tool-server deletion path.
  • Namespaces: get/list/watch in cluster-wide mode for namespace discovery.

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.

@EItanya
EItanya requested a review from a team as a code owner October 26, 2026 17:20
@github-actions github-actions Bot added the bug Something isn't working label Oct 26, 2026
@EItanya EItanya changed the title fix(helm): remove obsolete controller workload permissions fix(helm): narrow controller RBAC permissions Oct 26, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Oct 26, 2026
@EItanya EItanya changed the title fix(helm): narrow controller RBAC permissions fix(helm): narrow and separate controller RBAC permissions Oct 26, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Oct 26, 2026

@timflannagan timflannagan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@EItanya

EItanya commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

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.

@timflannagan

Copy link
Copy Markdown
Contributor

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?

@EItanya

EItanya commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

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

@timflannagan

Copy link
Copy Markdown
Contributor

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

@EItanya

EItanya commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

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

timflannagan
timflannagan previously approved these changes Oct 30, 2026
@timflannagan

Copy link
Copy Markdown
Contributor

@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>
@EItanya
EItanya force-pushed the fix/obsolete-controller-rbac branch from 0e116cf to 933751f Compare November 2, 2026 13:31
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Nov 2, 2026
@EItanya
EItanya added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit 7260ac1 Nov 2, 2026
32 checks passed
@EItanya
EItanya deleted the fix/obsolete-controller-rbac branch November 2, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Narrow the controller ClusterRoles to the core resources it uses (remove * wildcards)

2 participants

⌂ 换网址