feat(helm): support cluster-wide metadata RBAC - #621
Conversation
c4819c5 to
29448d5
Compare
WalkthroughThe Helm chart adds an opt-in ChangesKubernetes RBAC configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: 🔵 Low · up to When enabled, the chart grants metadata access across namespaces, including full ConfigMap access, while the documentation does not consistently explain that scope or the ServiceAccount prerequisite. The default behavior remains unchanged, so the PR is mergeable with explicit owner awareness and documentation follow-up. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/DEPLOYMENT.md`:
- Around line 108-109: Update the RBAC guidance in docs/DEPLOYMENT.md lines
108-109 and helm/README.md lines 224-226 to state that chart-managed RBAC
requires both serviceAccount.create=true and serviceAccount.rbac.enabled=true;
alternatively, explain that when using an existing ServiceAccount, RBAC must be
applied externally.
In `@helm/values.yaml`:
- Around line 29-31: Update the clusterWide RBAC description in helm/values.yaml
(29-31) to explicitly warn that cluster-wide mode grants access to ConfigMaps in
all namespaces; add the same warning to the deployment procedure in
docs/DEPLOYMENT.md (110-113) and the Helm example in helm/README.md (238-241),
without changing behavior.
🪄 Autofix
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: 897b98b3-d0c9-4ab2-a8fd-74ba2490f3f5
📒 Files selected for processing (4)
docs/DEPLOYMENT.mdhelm/README.mdhelm/templates/rbac.yamlhelm/values.yaml
29448d5 to
f4d2e1e
Compare
Signed-off-by: Kai-Hsun Chen <kaihsun@apache.org>
f4d2e1e to
6aebd79
Compare
|
cc @ganeshku1 @KavinKrishnan for review. Thanks! |
Summary
A Kubernetes production best practice is to deploy a controller or control-plane service in a dedicated namespace while allowing its namespaced custom resources to live alongside workloads in other namespaces.
ModelExpress currently cannot support this deployment pattern with the RBAC generated by its Helm chart. For example, when the ModelExpress server is deployed in the
modelexpressnamespace andModelMetadataorModelCacheEntryresources are created in thedefaultnamespace, the server receives a403 Forbiddenerror:This happens because the Helm chart currently creates a namespace-scoped
RoleandRoleBindingin the Helm release namespace. Those resources only grant the ModelExpress ServiceAccount access to resources in the same namespace.This PR changes the resources created by
serviceAccount.rbac.enabled=trueto aClusterRoleandClusterRoleBinding, allowing a ModelExpress server deployed in a dedicated namespace to manage its metadata resources in other namespaces.The setting remains disabled by default.
Future RBAC granularity
If ModelExpress needs more granular, least-privilege RBAC in the future, the Helm chart can add a namespaced installation mode and explicit watch-namespace configuration. The KubeRay Helm chart RBAC design provides a useful reference for supporting both cluster-scoped RBAC and namespace-scoped RBAC based on the operator's installation and watch scope.
Usage
Validation
modelexpressand its metadata namespace set todefaultRoleandRoleBindingcause a403 Forbiddenerror andCrashLoopBackOffClusterRoleandClusterRoleBindingkubectl auth can-ireturnsyesfor the ModelExpress ServiceAccount indefault1/1 Runningafter applying this PRhelm lint helmserviceAccount.rbac.enabled=falsegit diff --checkFull kind reproduction: #621 (comment)