Skip to content

chore: Describe RBAC rules, remove unnecessary rules#810

Draft
NickLarsenNZ wants to merge 1 commit intomainfrom
chore/rbac-review
Draft

chore: Describe RBAC rules, remove unnecessary rules#810
NickLarsenNZ wants to merge 1 commit intomainfrom
chore/rbac-review

Conversation

@NickLarsenNZ
Copy link
Member

@NickLarsenNZ NickLarsenNZ commented Mar 25, 2026

Part of stackabletech/issues#798

Note

This was initially generated by a coding assistant to see how well it can inspect code and review the RBAC rules. the changes will be properly checked before reviews are requested.

  • Document each rule
  • Check the docs make sense. Rewrite where necessary
  • Remove unnecessary permissions
  • Attach explanations to PR description
  • Run all tests
  • Split operator and product roles into separate files

Explanation

Rule Change Reason
nodes list+watch Remove entirely Framework fetches kubelet config via nodes/proxy GET using node name from downward API — no list/watch needed
pods Remove Operator never manages pods; StatefulSets do
endpoints Remove Auto-created by Kubernetes for Services
batch/jobs rule Remove entirely Druid operator never creates Jobs; boilerplate from initial template
update verb (all rules) Remove SSA uses patch (HTTP PATCH); no client.update() anywhere
secrets verbs Change to create, delete, get, patch Direct client.apply_patch / client.get_opt / client.delete in internal_secret.rs; no list/watch needed
serviceaccounts watch Remove Not watched by controller (no .owns() or .watches())
rolebindings watch Remove Not watched by controller
poddisruptionbudgets watch Remove Not watched by controller
listeners watch Remove Not watched by controller
druiddclusters patch Remove Operator only patches druiddclusters/status; no apply_patch on the main object
Details

● Removed entirely

  • nodes list+watch — The framework detects the cluster domain by calling GET /api/v1/nodes/{name}/proxy/configz (that's nodes/proxy get). The node name is injected via the downward API; no listing of nodes is ever done.
  • pods — The operator never creates or manages Pod objects directly. StatefulSets create pods. This was boilerplate from the initial template.
  • endpoints — Auto-created by Kubernetes when a Service is created; the operator never touches them.
  • batch/jobs rule — Present since the very first commit (copied from the operator-templating repo in Nov 2021). The Druid operator has never created any Jobs. The orphan cleanup silently skips with a 403 if list is denied, so
    there's no harm removing this entirely.

● Verb removals

  • update on all rules — All resource creation/mutation goes through client.apply_patch() (SSA = HTTP PATCH). No client.update() or api.replace() (HTTP PUT) calls exist anywhere in the codebase.
  • watch on serviceaccounts, rolebindings, poddisruptionbudgets, listeners — These are applied via SSA and tracked for orphan cleanup, but none of them appear in .owns() or .watches() in main.rs, so the controller does not need
    to react to changes in these resources.
  • list and watch on secrets — Secrets are managed directly in internal_secret.rs via client.get_opt(), client.apply_patch(), and client.delete(). They are not managed through ClusterResources, so the orphan cleanup for secrets
    gets a 403 and silently skips — which is the intended behaviour.
  • patch on druiddclusters — The operator only patches the status subresource via client.apply_patch_status(), which is covered by the separate druiddclusters/status rule. No client.apply_patch() is ever called on the
    DruidCluster object itself.

● The reorganisation was a direct consequence of the verb changes. The original file grouped pods, configmaps, secrets, services, endpoints, and serviceaccounts into a single rule because they all shared the same verb set:

verbs: [create, delete, get, list, patch, update, watch]

After the audit, those resources need different subsets of verbs:

  • configmaps + services: need watch (owned by controller), need list (orphan cleanup)
  • secrets: no list, no watch (not in ClusterResources, not watched)
  • serviceaccounts: need list (orphan cleanup), but no watch

A single RBAC rule applies its verb list uniformly to all resources in it, so you can't express "list+watch for configmaps but not for secrets" in one rule. The only way to set different verbs per resource is to split them into
separate rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant