chore: set the default value of namespaceRestriction to false #3104
Conversation
cr7258
requested review from
hutm,
biswapanda,
ishandhanani,
julienmancuso,
hhzhang16,
nnshah1,
mohammedabdulwahhab and
atchernych
as code owners
|
👋 Hi cr7258! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
bot
added
chore
external-contribution
Signed-off-by: cr7258 <[email protected]>
cr7258
force-pushed
the
namespaceRestriction
branch
from
fe7001a to
5bad4c7
Compare
WalkthroughA single Helm values change updates deploy/cloud/helm/platform/values.yaml to set dynamo-operator.namespaceRestriction.enabled from true to false. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| namespaceRestriction: | ||
| # -- Whether to restrict operator to specific namespaces | ||
| enabled: true | ||
| enabled: false |
There was a problem hiding this comment.
Yes, this seems to be a problem for unfamiliar users. Especially since we did not set a specific namespace in the example. https://github.com/ai-dynamo/dynamo/tree/main/components/backends/vllm/deploy
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/cloud/helm/platform/values.yaml (2)
33-37: Call out the default change in chart release notes and bump chart version.This is a security‑relevant default flip; document prominently in CHANGELOG/NOTES.txt and bump the chart minor version.
33-37: Optional: safer “open by default” via opt‑in labels.Future enhancement: even with restriction disabled, require namespaces to carry a label (e.g., dynamo.nvidia.com/enabled=true) to be reconciled. This avoids accidental cluster‑wide adoption.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deploy/cloud/helm/platform/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.649Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for upgrades because the Helm chart automatically populates dynamo-operator.dynamo.mpiRun.secretName with a default value of "mpi-run-ssh-secret" and includes SSH key generation functionality via sshKeygen.enabled: true.
📚 Learning: 2025-09-17T22:35:40.649Z
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.649Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for upgrades because the Helm chart automatically populates dynamo-operator.dynamo.mpiRun.secretName with a default value of "mpi-run-ssh-secret" and includes SSH key generation functionality via sshKeygen.enabled: true.
Applied to files:
deploy/cloud/helm/platform/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
deploy/cloud/helm/platform/values.yaml (1)
33-37: Verify RBAC and watch-scope behavior with namespaceRestriction default (enabled: false)Search produced no matches; cannot verify. From repo root run and paste outputs:
# find where the flag is referenced rg -n --hidden -S '\.Values\.namespaceRestriction|namespaceRestriction' deploy/cloud/helm -g '!**/values.yaml' # find WATCH_NAMESPACE / namespace args/env in templates rg -n --hidden -S 'WATCH_NAMESPACE|--namespace|watchNamespace|watch.*namespace' deploy/cloud/helm -g '!**/values.yaml' # find RBAC resources in templates rg -n --hidden -S 'ClusterRole|ClusterRoleBinding|RoleBinding|Role\b' deploy/cloud/helm -g '!**/values.yaml' # render templates for both cases (requires helm) helm template test-release deploy/cloud/helm -f deploy/cloud/helm/platform/values.yaml --set namespaceRestriction.enabled=false | rg -n 'ClusterRole|ClusterRoleBinding|RoleBinding|Role\b|WATCH_NAMESPACE|env:|--namespace' helm template test-release deploy/cloud/helm -f deploy/cloud/helm/platform/values.yaml --set namespaceRestriction.enabled=true --set namespaceRestriction.targetNamespace=foo | rg -n 'ClusterRole|ClusterRoleBinding|RoleBinding|Role\b|WATCH_NAMESPACE|env:|--namespace'Expected outcomes:
- enabled=false → ClusterRole/ClusterRoleBinding present; no WATCH_NAMESPACE env/arg (manager watches cluster‑wide).
- enabled=true → namespaced Role/RoleBinding rendered; WATCH_NAMESPACE (or equivalent) set to targetNamespace.
- Confirm no duplicate operator instances watch the same namespace in multi‑ns installs.
| namespaceRestriction: | ||
| # -- Whether to restrict operator to specific namespaces | ||
| enabled: true | ||
| enabled: false | ||
| # -- Target namespace for operator deployment (leave empty for current namespace) | ||
| targetNamespace: |
There was a problem hiding this comment.
Changing default to cluster-wide reconciliation broadens scope; add an explicit warning (and consider alternative rollout).
This default can surprise multi‑tenant/prod users by reconciling CRs in all namespaces. At minimum, add a prominent warning next to the value. Prefer shipping a quickstart values file for examples instead of changing the global default.
Apply this inline doc tweak:
namespaceRestriction:
- # -- Whether to restrict operator to specific namespaces
- enabled: false
+ # -- Whether to restrict operator to specific namespaces
+ # WARNING: Disabling namespace restriction makes the operator reconcile across ALL namespaces.
+ # Not recommended for multi-tenant or production clusters. Set to true for production.
+ enabled: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| namespaceRestriction: | |
| # -- Whether to restrict operator to specific namespaces | |
| enabled: true | |
| enabled: false | |
| # -- Target namespace for operator deployment (leave empty for current namespace) | |
| targetNamespace: | |
| namespaceRestriction: | |
| # -- Whether to restrict operator to specific namespaces | |
| # WARNING: Disabling namespace restriction makes the operator reconcile across ALL namespaces. | |
| # Not recommended for multi-tenant or production clusters. Set to true for production. | |
| enabled: false | |
| # -- Target namespace for operator deployment (leave empty for current namespace) | |
| targetNamespace: |
🤖 Prompt for AI Agents
In deploy/cloud/helm/platform/values.yaml around lines 33 to 37, the inline docs
do not warn users that leaving namespaceRestriction disabled enables
cluster-wide reconciliation; add a prominent warning comment immediately above
or on the enabled field calling out that false = cluster-wide reconciliation and
may be unsafe in multi-tenant/prod environments, and suggest setting enabled:
true and targetNamespace for scoped installs; additionally add a note
referencing a new quickstart-values.yaml (in the same directory) that contains
example scoped and cluster-wide configurations for safe rollout.
Overview:
By default,
namespaceRestrictionis set to true. As per the NVIDIA Dynamo production deployment guide, the Dynamo Operator will only reconcile DynamoGraphDeployment resources in the namespace where it is running (in this case, thedynamo-kubernetesnamespace).However, if a user then deploys a DynamoGraphDeployment using the provided examples (e.g., agg.yaml), which is deployed in the
defaultnamespace, the Dynamo Operator will not reconcile this DynamoGraphDeployment. Consequently, the Dynamo Operator does not create any vLLM instances, and no relevant logs are emitted. This behavior can be very confusing for new users.To ensure new users can smoothly experience Dynamo, this PR sets the default value of namespaceRestriction to false, allowing users to successfully deploy DynamoGraphDeployment by following the existing installation guide and example.
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Summary by CodeRabbit