fix(chart): gate main ClusterRole rules by engine and provider - #432
Conversation
Greptile SummaryThis PR narrows the main Topograph server’s Kubernetes permissions according to its configured engine and provider.
Confidence Score: 4/5This PR should not merge until request-time engine and provider selection is reconciled with the install-time RBAC gates. The main role is still rendered solely from Helm values, while generate requests may override those values with any registered engine or provider; selecting a Kubernetes-backed component whose permissions were omitted causes its Kubernetes API operations to fail. Files Needing Attention: charts/topograph/templates/rbac.yaml and pkg/server/http_server.go Important Files Changed
Reviews (2): Last reviewed commit: "fix(chart): gate main ClusterRole by eng..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Hi @teerthsharma, just wanted to check if you’re planning to continue working on this PR. If so, could you please rebase it on the latest main branch when you have a chance? |
Will rebase clear & merge conflicts , always up to work !! Shall I make it open for review after rebasing? |
Signed-off-by: teerth sharma <teerth.2428010112@muj.manipal.edu>
f9b0348 to
c71a7b0
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (3)CHANGELOG.md📄 CodeRabbit inference engine (AGENTS.md)
Files:
charts/topograph/templates/**/*.yaml📄 CodeRabbit inference engine (AGENTS.md)
Files:
charts/topograph/**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Helm RBAC template now gates ClusterRole resources and Kubernetes permissions by engine and provider. Chart tests cover supported and unsupported combinations. The changelog documents the security change. ChangesRBAC gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The chart now limits ClusterRole permissions to applicable engine and provider combinations, with the affected render cases covered by passing validation. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Values as Helm values
participant Template as rbac.yaml
participant Role as ClusterRole
participant Binding as ClusterRoleBinding
Values->>Template: Provide engine and provider
Template->>Role: Render required permission rules
Template->>Binding: Render binding when a ClusterRole exists
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/ok-to-test c71a7b0 |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Nothing here blocks merge.
One note on test coverage. The gating itself lines up with what each engine and provider actually calls, and the ClusterRole/ClusterRoleBinding omission for non-Kubernetes combinations reads correctly.
- [should-fix] This is the only case that isolates the provider's node access — every other infiniband-k8s case leaves the engine at k8s, which grants nodes on its own. It checks the document count and the absence of nodes/patch, but never that nodes get,list is present, so the new
(eq $provider "infiniband-k8s")term in$needsNodeshas no test behind it: dropping that term removes the nodes rule entirely for this combination and the suite still reports 141 passed / 153 snapshots passed. The same mutation on the dra term, the infiniband-k8s arm of$needsPods, and$needsDaemonSetseach fail a test, so this is the one gap. AcontainsforapiGroups: [""] / resources: [nodes] / verbs: [get, list]at documentIndex 0 closes it, and it backs a live call at pkg/providers/infiniband/provider_k8s.go:72. (charts/topograph/tests/rbac_test.yaml:266)
| apiGroups: [""] | ||
| resources: [nodes] | ||
| verbs: [patch] | ||
| documentIndex: 0 |
There was a problem hiding this comment.
This is the only case that isolates the provider's node access — every other infiniband-k8s case leaves the engine at k8s, which grants nodes on its own. It checks the document count and the absence of nodes/patch, but never that nodes get,list is present, so the new (eq $provider "infiniband-k8s") term in $needsNodes has no test behind it: dropping that term removes the nodes rule entirely for this combination and the suite still reports 141 passed / 153 snapshots passed. The same mutation on the dra term, the infiniband-k8s arm of $needsPods, and $needsDaemonSets each fail a test, so this is the one gap. A contains for apiGroups: [""] / resources: [nodes] / verbs: [get, list] at documentIndex 0 closes it, and it backs a live call at pkg/providers/infiniband/provider_k8s.go:72.
3 participants
Description
Closes #386.
The main Topograph API server ClusterRole renders the same Kubernetes RBAC rules regardless of the selected engine and provider. A chart installed with a combination that never reaches the Kubernetes API — for example
provider: testwithengine: slurm— is still grantedpods list,nodes get/list, anddaemonsets getcluster-wide.This change gates the main ClusterRole rules on
engine.nameandprovider.name, extending the gating pattern already applied topods/exec,configmaps, andnodes patch.Gating
nodes get,listk8s,slinky,nfd, or providerdra,infiniband-k8snodes patchk8sorslinky(unchanged)pods listslinkyor providerinfiniband-k8sdaemonsets getinfiniband-k8sconfigmaps create,get,updateslinky(unchanged)pods/exec createinfiniband-k8s, or Slinky partition discovery (unchanged)The ClusterRole and ClusterRoleBinding are omitted entirely when no rule applies. The namespaced NFD
RoleandRoleBindingare unaffected and continue to render forengine: nfd. There is no change to the node-observer or node-data-broker RBAC, to any values schema key, or to any Go code.Validation
From PR head c71a7b0 rebased on main at 6466969, helm v4.1.1 with helm-unittest 1.1.1:
helm lint charts/topograph: 1/1 chart linted, 0 failed.helm unittest charts/topograph: 141/141 tests passed across 19/19 suites, 153/153 snapshots passed.The rbac suite adds coverage for the
draprovider (nodes without pods or daemonsets), theinfiniband-k8sprovider (pods, daemonsets, and pods/exec), thenfdengine (nodes without pods or node patch), theslurmengine with a Kubernetes provider (ClusterRole still rendered), andtest+slurm(0 documents rendered).Snapshot deltas are confined to the ClusterRole document in seven umbrella renders:
pods listanddaemonsets getdrop from the fourk8s-engine renders, anddaemonsets getdrops from the threeslinkyrenders. Thevalues.k8s.ib-example.yamlsnapshot is unchanged, which is the control —infiniband-k8sretains both rules.Limits
The gates read Helm values, while
/v1/generateacceptsprovider.nameandengine.namein the request payload and falls back to the configured values only when the request omits them (pkg/server/http_server.go:181). A deployment installed for one combination and sent a request naming a Kubernetes-backed engine or provider will receiveForbiddenfrom the API server. This mismatch predates the change — main already gatespods/exec,configmaps, andnodes patchthe same way — but the change widens the surface it applies to. Constraining request-time selection to the configured values, or documenting the deployment contract, is a separate decision for maintainers.Checklist