feat(deployments): grant k8s backend RBAC in deploy chart (AIRCORE-757 phase 6) - #575
Merged
Conversation
Contributor
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends controller RBAC for ChangesRBAC rule and validation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-deployments/README.md`:
- Around line 89-92: The README permission summary is inaccurate for `pods/log`:
it currently says `get`/`list`/`watch`, but the actual role only grants
`get`/`list`. Update the permissions description in the deployment docs so the
`pods/log` verbs match the real RBAC definition, and keep the rest of the
Kubernetes verb summary unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5c8a65d5-96d6-4631-a719-6ada462c62e3
📒 Files selected for processing (3)
k8s/helm/templates/core/controller-role.yamlplugins/nemo-deployments/README.mdplugins/nemo-deployments/tests/unit/backends/k8s/test_rbac_manifest.py
Merged
5 tasks
mckornfield
approved these changes
Jul 7, 2026
…7 phase 6) Extends the nmp-core controller Role with the two permissions the Phase 5 k8s DeploymentBackend needs but didn't have: create/delete on apps/v1 Deployments, and a new services rule (get/list/create/delete). The DeploymentsController runs inside the existing nmp-core controller pod, so this reuses that pod's ServiceAccount/Role rather than adding a parallel one. Also documents the k8s executor config and RBAC scope in the plugin README, and adds a unit test asserting the chart's rendered RBAC rules. Signed-off-by: Tyler Bray <tbray@nvidia.com>
The example used my personal dev-blue namespace (tbray-dev) as the "default" default_namespace value. Replace with the literal default from K8sExecutorConfig plus a comment explaining what belongs there, so the doc doesn't imply a personal namespace is the expected default. Signed-off-by: Tyler Bray <tbray@nvidia.com>
CodeRabbit caught that the RBAC summary grouped pods and pods/log under the same get/list/watch verb set, but controller-role.yaml only grants get/list on pods/log (no watch). Split the two out. Signed-off-by: Tyler Bray <tbray@nvidia.com>
- test_rbac_manifest.py: extract the repeated controller-role.yaml path segments (previously duplicated in _repo_root() and _controller_role_source()) into a single _CONTROLLER_ROLE_RELATIVE_PATH constant. - README: drop the "(a RoleBinding in that namespace, or a ClusterRole/ClusterRoleBinding)" parenthetical from the RBAC section — self-evident to anyone hitting the "additional RBAC" gap it describes, and trims a sentence that was otherwise dense with cross-references. Signed-off-by: Tyler Bray <tbray@nvidia.com>
tylersbray
force-pushed
the
757-k8s-phase6-rbac-chart/tbray
branch
from
July 7, 2026 17:30
5265cea to
57fb965
Compare
tylersbray
added a commit
that referenced
this pull request
Jul 7, 2026
…end (AIRCORE-757 phase 7) Mirror the existing Docker integration-test layout for the k8s deployments backend: PVC lifecycle, Job success, ConfigMap mount round-trip, Deployment+Service readiness, and label-gated delete rejection, plus a reconciler-level prerequisite-chain test. All tests skip cleanly when no kubeconfig/cluster is reachable, matching the Docker suite's CI behavior. Branched from origin/main ahead of Phase 6 (#575) landing, since these tests instantiate K8sDeploymentBackend directly with the developer's own kubeconfig rather than the restricted nmp-core ServiceAccount that Phase 6's RBAC changes target -- no functional dependency between the two. Will rebase onto main once #575 merges. Signed-off-by: Tyler Bray <tbray@nvidia.com>
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…7 phase 6) (#575) * feat(deployments): grant k8s backend RBAC in deploy chart (AIRCORE-757 phase 6) Extends the nmp-core controller Role with the two permissions the Phase 5 k8s DeploymentBackend needs but didn't have: create/delete on apps/v1 Deployments, and a new services rule (get/list/create/delete). The DeploymentsController runs inside the existing nmp-core controller pod, so this reuses that pod's ServiceAccount/Role rather than adding a parallel one. Also documents the k8s executor config and RBAC scope in the plugin README, and adds a unit test asserting the chart's rendered RBAC rules. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): use generic namespace in k8s executor README example The example used my personal dev-blue namespace (tbray-dev) as the "default" default_namespace value. Replace with the literal default from K8sExecutorConfig plus a comment explaining what belongs there, so the doc doesn't imply a personal namespace is the expected default. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): correct pods/log verb claim in README RBAC summary CodeRabbit caught that the RBAC summary grouped pods and pods/log under the same get/list/watch verb set, but controller-role.yaml only grants get/list on pods/log (no watch). Split the two out. Signed-off-by: Tyler Bray <tbray@nvidia.com> * fix(deployments): address mckornfield review feedback on RBAC chart PR - test_rbac_manifest.py: extract the repeated controller-role.yaml path segments (previously duplicated in _repo_root() and _controller_role_source()) into a single _CONTROLLER_ROLE_RELATIVE_PATH constant. - README: drop the "(a RoleBinding in that namespace, or a ClusterRole/ClusterRoleBinding)" parenthetical from the RBAC section — self-evident to anyone hitting the "additional RBAC" gap it describes, and trims a sentence that was otherwise dense with cross-references. Signed-off-by: Tyler Bray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com>
arpitsardhana
pushed a commit
that referenced
this pull request
Jul 9, 2026
…end (AIRCORE-757 phase 7) (#581) * test(deployments): add kind/kubeconfig integration tests for k8s backend (AIRCORE-757 phase 7) Mirror the existing Docker integration-test layout for the k8s deployments backend: PVC lifecycle, Job success, ConfigMap mount round-trip, Deployment+Service readiness, and label-gated delete rejection, plus a reconciler-level prerequisite-chain test. All tests skip cleanly when no kubeconfig/cluster is reachable, matching the Docker suite's CI behavior. Branched from origin/main ahead of Phase 6 (#575) landing, since these tests instantiate K8sDeploymentBackend directly with the developer's own kubeconfig rather than the restricted nmp-core ServiceAccount that Phase 6's RBAC changes target -- no functional dependency between the two. Will rebase onto main once #575 merges. Signed-off-by: Tyler Bray <tbray@nvidia.com> * test(deployments): address CodeRabbit feedback on k8s integration tests Fixes 3 actionable review comments on #581: - _wait_for_status now fails explicitly with an attempt count instead of silently returning on timeout, so callers get a clear message instead of a later ambiguous assertion failure. - test_pvc_lifecycle no longer asserts inside its finally block, so a cleanup-path failure can't mask the real assertion failure from the try block. - test_reconcile_k8s's teardown deletes puller and server concurrently via asyncio.gather(return_exceptions=True) so a failure deleting one doesn't leak the other. Also addresses 2 nitpicks: - README: added a prerequisites block ahead of the kind/dev-blue run commands in the new Cluster validation section. - test_reconcile_k8s's get_side_effect now raises KeyError for any Deployment name other than "puller"/"server" instead of silently falling back to server_dep, so a typo fails loudly. Left 2 nitpicks as-is (noted in the PR): the `from __future__ import annotations` import and the import-time cluster/daemon availability probe both match the pre-existing docker_availability.py / test_reconcile_docker.py convention already used throughout this test suite; fixing only the new k8s files would create inconsistency, and a broader refactor of the shared pattern is out of scope here. Signed-off-by: Tyler Bray <tbray@nvidia.com> * docs(deployments): clarify ambient kubeconfig wording in README Addresses mckornfield's PR #581 nitpick that "ambient" reads oddly out of context. Signed-off-by: tbray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com> Signed-off-by: tbray <tbray@nvidia.com>
Zenodia
pushed a commit
to Zenodia/nemo-platform
that referenced
this pull request
Jul 10, 2026
…end (AIRCORE-757 phase 7) (NVIDIA-NeMo#581) * test(deployments): add kind/kubeconfig integration tests for k8s backend (AIRCORE-757 phase 7) Mirror the existing Docker integration-test layout for the k8s deployments backend: PVC lifecycle, Job success, ConfigMap mount round-trip, Deployment+Service readiness, and label-gated delete rejection, plus a reconciler-level prerequisite-chain test. All tests skip cleanly when no kubeconfig/cluster is reachable, matching the Docker suite's CI behavior. Branched from origin/main ahead of Phase 6 (NVIDIA-NeMo#575) landing, since these tests instantiate K8sDeploymentBackend directly with the developer's own kubeconfig rather than the restricted nmp-core ServiceAccount that Phase 6's RBAC changes target -- no functional dependency between the two. Will rebase onto main once NVIDIA-NeMo#575 merges. Signed-off-by: Tyler Bray <tbray@nvidia.com> * test(deployments): address CodeRabbit feedback on k8s integration tests Fixes 3 actionable review comments on NVIDIA-NeMo#581: - _wait_for_status now fails explicitly with an attempt count instead of silently returning on timeout, so callers get a clear message instead of a later ambiguous assertion failure. - test_pvc_lifecycle no longer asserts inside its finally block, so a cleanup-path failure can't mask the real assertion failure from the try block. - test_reconcile_k8s's teardown deletes puller and server concurrently via asyncio.gather(return_exceptions=True) so a failure deleting one doesn't leak the other. Also addresses 2 nitpicks: - README: added a prerequisites block ahead of the kind/dev-blue run commands in the new Cluster validation section. - test_reconcile_k8s's get_side_effect now raises KeyError for any Deployment name other than "puller"/"server" instead of silently falling back to server_dep, so a typo fails loudly. Left 2 nitpicks as-is (noted in the PR): the `from __future__ import annotations` import and the import-time cluster/daemon availability probe both match the pre-existing docker_availability.py / test_reconcile_docker.py convention already used throughout this test suite; fixing only the new k8s files would create inconsistency, and a broader refactor of the shared pattern is out of scope here. Signed-off-by: Tyler Bray <tbray@nvidia.com> * docs(deployments): clarify ambient kubeconfig wording in README Addresses mckornfield's PR NVIDIA-NeMo#581 nitpick that "ambient" reads oddly out of context. Signed-off-by: tbray <tbray@nvidia.com> --------- Signed-off-by: Tyler Bray <tbray@nvidia.com> Signed-off-by: tbray <tbray@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AIRCORE-757 Phase 6 — grants the k8s deployments backend (Phase 5, #553) the RBAC it needs, wired into the deploy chart.
nmp-corecontroller Role (k8s/helm/templates/core/controller-role.yaml) with the two verbs the k8sDeploymentBackendneeds but didn't have:create/deleteonapps/v1.Deployment, and a new rule forservices(get/list/create/delete).plugins/nemo-deployments/README.md.plugins/nemo-deployments/tests/unit/backends/k8s/test_rbac_manifest.pyasserting the chart's RBAC rules, plus a regression guard over the pre-existing base rules (pods, pods/log, jobs, PVCs, configmaps).Design decisions
Extended the existing
nmp-corecontroller Role instead of a dedicated ServiceAccount/Role for the deployments plugin.DeploymentsControlleris registered via thenemo.controllersentry point and runs inside the existingnmp-corecontroller pod (--controller-group=all), undercontrollerServiceAccountName. A parallel Role bound to that same pod's ServiceAccount would be redundant, so this PR extends the Role already granting that pod's RBAC (pods, pods/log, jobs, PVCs, configmaps) rather than introducing a second one.RBAC audit was exhaustive against every k8s API call in the Phase 1-5 backend code, not just the ticket's suggested verb list:
volumes.py,jobs.py,deployments.py,compiler.py. Result: exactly two gaps —apps/deploymentswasget/list/watchonly (backend also creates/deletes), andserviceshad no rule at all (backend gets/creates/deletes). Both additions are minimal: noupdate/patchon Deployments since the backend recreates rather than patches.RBAC additions are ungated (no new
rbac.*values flag), consistent with the existing jobs/PVC/configmap base rules — keepsvalues.yamlandhelm-docs-generated README untouched, since this isn't an optional third-party integration like Volcano or the NIM Operator.RBAC test uses raw-template regex parsing, not a
helm templaterender — this diverged from the original plan.k8s/helm/Chart.yamldeclares an unconditionalk8s-nim-operatorchart dependency (only the installed resources are gated by.Values.rbac.k8sNimOperatorEnabled, not the dependency-presence check), sohelm templaterefuses to run at all unless that subchart.tgzis fetched from an NGC repo intocharts/first — verified locally that this fails even withk8s-nim-operator.enabled=falseand--show-only. There's no existing helm lint/template CI job in this repo and no committedChart.lock/charts/dir, so a skip-gated render would silently skip in CI 100% of the time. The test instead parses the static YAML rule entries directly from the Go-template source, which is safe because the base RBAC rules (unlike the Volcano/NIM-Operator blocks) are plain YAML with no Helm expressions inside them. Runs deterministically with nohelmbinary or network dependency.No functional carryover from Phase 5 review — confirmed against #553's merge history; mckornfield's review comments were style-only and already addressed pre-merge.
Forward-looking note: AIRCORE-759 interaction
AIRCORE-759 (models adopts this plugin, drops
k8s-nim-operator) is backlogged but directly bears on the decisions above:K8sDeploymentBackend/DeploymentsController/nmp-corecontroller pod this PR grants RBAC to. A dedicated per-plugin ServiceAccount would have needed reconciling with models' path later; extending the shared Role avoids that.helm templateblocker above: AIRCORE-759 requires removing thek8s-nim-operatorsubchart dependency, its CRDs, and its RBAC (nimservices/nimcachesrules) from this same chart. Once that lands,helm template k8s/helmshould render standalone andtest_rbac_manifest.pycould be upgraded from raw-text parsing to a real chart render.(Volume, puller, server)DeploymentConfigprimitives that already flow through the backend code audited here — no new verbs anticipated purely from that migration.backend_config.k8s.image_pull_secrets, but that field doesn't exist yet onK8sDeploymentConfig/compiler.pytoday (verified) — will need a small compiler addition, and possibly broadersecretsverbs (currently onlycreate/delete) if the plugin needs to read/compare an existing pull secret.tbray-devpod's missingnemo-models-vllmRole/RoleBinding (noted separately during manual-smoke-test setup) is models' current, pre-759 standalone k8s-emission identity from AIRCORE-694. It becomes obsolete once AIRCORE-759 lands and models delegates to this plugin'snmp-coreRole — a natural line item for that ticket's own RBAC-cleanup requirement, not addressed here.Test plan
uv run --frozen pytest plugins/nemo-deployments/tests/unit -q— 258 passeduv run ruff check/uv run ruff format --check/uv run --frozen ty check— cleanhelm template(locally cachedk8s-nim-operatorsubchart) that the rendered Role contains the expected verbsuv run pre-commit run -a— all hooks pass exceptstudio-lint-staged, a pre-existing Node-version environment gap scoped toweb/(untouched by this PR) that self-skips in CISummary by CodeRabbit