fix: harden CacheRuntime controller ClusterRole to least privilege - #6071
Conversation
Remove overly broad permissions that pose security risks: - Remove clusterroles/clusterrolebindings create/delete (privilege escalation risk) - Remove serviceaccounts create/delete (unused by cache engine) - Remove duplicate "advance stateful set resources" block (pods create/delete, pods/status, events full CRUD, duplicate PVC, controllerrevisions) - Remove native statefulsets/statefulsets/status (cache uses AdvancedStatefulSet) - Remove cacheruntimes create/delete (controller reconciles, not creates CRDs) - Tighten PVC verbs to get/list/watch/create/delete (no update/patch needed) - Tighten pods to read-only (pods managed by workload controllers) - Tighten services to remove patch (only create/delete used) - Tighten configmaps to remove patch (only update used) - Consolidate advancedstatefulsets/status/finalizers into single rule block - Add cacheruntimes/finalizers (needed by controller but was missing) Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors and tightens the RBAC permissions in the cache role's RBAC configuration by removing several unused resources (such as service accounts, cluster roles, pods, and persistent volume claims) and restricting verbs. The review feedback suggests further tightening these permissions to better adhere to the principle of least privilege. Specifically, it recommends separating the read-only cacheruntimeclasses from write-enabled resources, removing unused daemonsets/status permissions and unnecessary verbs for daemonsets, and removing unused subresource permissions (status and finalizers) for advancedstatefulsets.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - apiGroups: | ||
| - data.fluid.io | ||
| resources: | ||
| - cacheruntimes | ||
| - cacheruntimes/status | ||
| - cacheruntimes/finalizers | ||
| - cacheruntimeclasses | ||
| - datasets | ||
| - cacheruntimes/status | ||
| - datasets/status | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch |
There was a problem hiding this comment.
The cacheruntimeclasses resource is a read-only reference configuration for the controller. The controller only needs to read (get, list, watch) these classes and never updates or patches them. Separating cacheruntimeclasses from the other data.fluid.io resources that require write permissions (update, patch) ensures better adherence to the principle of least privilege.
Note: Please also remember to update the corresponding kubebuilder RBAC annotations in pkg/controllers/v1alpha1/cacheruntime/cacheruntime_controller.go to keep them in sync with these tightened Helm chart permissions.
- apiGroups:
- data.fluid.io
resources:
- cacheruntimes
- cacheruntimes/status
- cacheruntimes/finalizers
- datasets
- datasets/status
verbs:
- get
- list
- watch
- update
- patch
- apiGroups:
- data.fluid.io
resources:
- cacheruntimeclasses
verbs:
- get
- list
- watch| - apiGroups: | ||
| - apps | ||
| resources: | ||
| - daemonsets | ||
| - statefulsets | ||
| - daemonsets/status | ||
| - statefulsets/status | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete | ||
| # advance stateful set resources begin | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - pods | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - pods/status | ||
| verbs: | ||
| - get | ||
| - update | ||
| - patch | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - events | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - persistentvolumeclaims | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete |
There was a problem hiding this comment.
The CacheRuntime controller only reads and creates DaemonSet workloads (via DaemonSetManager). It does not support updating/patching them (as SyncComponentSpec is explicitly not supported for DaemonSet components), and deletion is handled automatically by Kubernetes garbage collection via OwnerReferences. Additionally, the controller only reads the daemonset status as part of the main resource Get call, so the daemonsets/status subresource permission is completely unused. We can safely remove daemonsets/status and restrict the verbs to get, list, watch, and create to achieve true least privilege.
- apiGroups:
- apps
resources:
- daemonsets
verbs:
- get
- list
- watch
- create| - apiGroups: | ||
| - workload.fluid.io | ||
| resources: | ||
| - advancedstatefulsets | ||
| - advancedstatefulsets/status | ||
| - advancedstatefulsets/finalizers | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create | ||
| - update | ||
| - patch |
There was a problem hiding this comment.
The CacheRuntime controller (via AdvancedStatefulSetManager) only gets, creates, and patches the main advancedstatefulsets resource. It does not update or patch the advancedstatefulsets/status or advancedstatefulsets/finalizers subresources directly. Therefore, these subresource permissions are unused and can be safely removed to adhere to the principle of least privilege.
- apiGroups:
- workload.fluid.io
resources:
- advancedstatefulsets
verbs:
- get
- list
- watch
- create
- update
- patch
- Separate cacheruntimeclasses into read-only rule (get/list/watch only) - Remove daemonsets/status and update/delete verbs from daemonsets (DaemonSetManager only does get and create, no in-place update) - Remove advancedstatefulsets/status and advancedstatefulsets/finalizers (controller does not update these subresources directly) Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the CacheRuntime controller RBAC in the Helm chart by removing high-risk/unneeded permissions and aligning allowed verbs/resources with the CacheRuntime controller’s actual Kubernetes API usage.
Changes:
- Removes write access to high-risk RBAC resources and other unused permissions (e.g., clusterroles/clusterrolebindings, serviceaccounts create/delete, broad pod/event/controllerrevision access).
- Updates the cache controller’s workload permissions to focus on
AdvancedStatefulSet(workload.fluid.io) rather than nativeStatefulSet. - Adds missing subresource permissions (e.g.,
cacheruntimes/finalizers) and tightens verbs across several core resources.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - create |
| resources: | ||
| - cacheruntimes | ||
| - cacheruntimes/status | ||
| - cacheruntimes/finalizers | ||
| - cacheruntimeclasses |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6071 +/- ##
=======================================
Coverage 64.77% 64.77%
=======================================
Files 484 484
Lines 33892 33892
=======================================
Hits 21954 21954
Misses 10215 10215
Partials 1723 1723 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| - update | ||
| - patch | ||
| - delete | ||
| # advance stateful set resources begin |
There was a problem hiding this comment.
this is need for advancestatefulset controller, see the README.md in https://github.com/fluid-cloudnative/advanced-statefulset ?
| - apiGroups: | ||
| - apps | ||
| resources: | ||
| - daemonsets |
There was a problem hiding this comment.
As the advancestatefulset part can not be modified, daemonset should be keeped.
c1f812b to
20f4053
Compare
Restore permissions required by the embedded advanced-statefulset controller (controllerrevisions, pods full CRUD, pods/status, events, PVC update/patch, advancedstatefulsets/status and finalizers) and keep daemonsets update/patch/status per reviewer feedback. Also print key diagnostic logs (controller logs, runtime/dataset describe, pod list, events) directly to CI console on e2e test failure so issues can be diagnosed without downloading artifacts. Signed-off-by: cheyang <cheyang@163.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates RBAC rules in the cache role template and enhances E2E test scripts to collect and dump diagnostic logs upon test failure. Feedback on the RBAC changes highlights that the permissions for pods have been expanded to full CRUD, which contradicts the goal of tightening permissions to read-only and violates the principle of least privilege.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - get | ||
| - list | ||
| - watch | ||
| - update | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - pods/exec | ||
| verbs: | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete |
There was a problem hiding this comment.
The PR description states: "Tighten verbs to match actual code usage: pods to read-only". However, the actual changes in rbac.yaml expand the pods verbs to include create, update, patch, and delete (full CRUD).
Since the CacheRuntime controller manages pods indirectly via AdvancedStatefulSet and DaemonSet, it does not need write permissions on pods directly. To adhere to the principle of least privilege and match the PR's stated goal, these verbs should be restricted to read-only (get, list, watch).
- get
- list
- watchThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
charts/fluid/fluid/templates/role/cache/rbac.yaml:90
- The
eventsrule grants get/list/watch/update/delete in addition to create/patch. The controller uses an EventRecorder (Eventf), which typically only requirescreateandpatch; there’s no code path reading or deleting Events. Tightening this reduces permissions without impacting normal recording.
- events
verbs:
- get
- list
- watch
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - update | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - pods/exec | ||
| verbs: | ||
| - create |
| - pods/status | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - update | ||
| - patch |
| - apps | ||
| resources: | ||
| - persistentvolumeclaims | ||
| - controllerrevisions | ||
| verbs: | ||
| - get |
- Move diagnostic collection and console log dump inside the failure check so successful tests skip the verbose collection entirely - Replace bash-specific &> and &>> redirections with POSIX-compatible > and >> forms for shell compatibility Signed-off-by: cheyang <cheyang@163.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
| - watch | ||
| - update | ||
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - pods/exec | ||
| verbs: | ||
| - create | ||
| - update | ||
| - patch | ||
| - delete |
| verbs: | ||
| - get | ||
| - list | ||
| - watch | ||
| - update | ||
| - patch |
| @@ -138,43 +92,42 @@ | |||
| - update | |||
| - create | ||
| - update | ||
| - patch | ||
| - delete |
|
|
||
| helm_get() { | ||
| run helm get all -n ${runtime_namespace} "${1}" &>"$diagnose_dir/helm-${1}.yaml" | ||
| run helm get all -n ${runtime_namespace} "${1}" >"$diagnose_dir/helm-${1}.yaml" |
| run kubectl get po -owide -n ${namespace} >"$diagnose_dir/pods-${namespace}.log" | ||
| run kubectl get po -oyaml -n ${namespace} >>"$diagnose_dir/pods-${namespace}.log" |
|
|
||
| helm_get() { | ||
| run helm get all -n ${runtime_namespace} "${1}" &>"$diagnose_dir/helm-${1}.yaml" | ||
| run helm get all -n ${runtime_namespace} "${1}" >"$diagnose_dir/helm-${1}.yaml" |
|
|
||
| helm_get_runtime() { | ||
| run env HELM_DRIVER=configmap helm get all -n ${runtime_namespace} "${1}" &>"$diagnose_dir/helm-${1}.yaml" | ||
| run env HELM_DRIVER=configmap helm get all -n ${runtime_namespace} "${1}" >"$diagnose_dir/helm-${1}.yaml" |
| run kubectl get po -owide -n ${namespace} >"$diagnose_dir/pods-${namespace}.log" | ||
| run kubectl get po -oyaml -n ${namespace} >>"$diagnose_dir/pods-${namespace}.log" |
| function dump_env_and_clean_up() { | ||
| bash tools/diagnose-fluid-juicefs.sh collect --name $dataset_name --namespace default --collect-path ./e2e-tmp/testcase-juicefs.tgz | ||
| local exit_code=$? | ||
| if [[ $exit_code -ne 0 ]]; then | ||
| bash tools/diagnose-fluid-juicefs.sh collect --name $dataset_name --namespace default --collect-path ./e2e-tmp/testcase-juicefs.tgz | ||
| syslog "=== Diagnostic logs for failed test ===" | ||
| syslog "--- juicefsruntime-controller logs (last 100 lines) ---" | ||
| kubectl logs -n fluid-system -l control-plane=juicefsruntime-controller -c manager --tail=100 2>&1 || true |
Verify that after deleting CacheRuntime and Dataset: - AdvancedStatefulSet, DaemonSet, Service are garbage collected via OwnerReferences (validates that delete verb is not needed) - Node labels are cleaned up by controller Shutdown - PV/PVC are properly removed Signed-off-by: cheyang <cheyang@163.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
Scale worker from 1 to 2 then back to 1 to exercise: - patch on advancedstatefulsets (controller updates replicas) - delete on pods (advanced-statefulset controller removes excess pods) - status reconciliation after scaling This validates that the RBAC permissions for advancedstatefulsets (without delete verb) and pods (with delete verb) are sufficient for scale operations. Signed-off-by: cheyang <cheyang@163.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
KIND CI uses a single-node cluster where pod anti-affinity prevents scheduling a second worker pod. Skip the scale-up/scale-down test when fewer than 2 nodes are available. Signed-off-by: cheyang <cheyang@163.com> Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RongGu, xliuqq The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Summary
clusterroles,clusterrolebindings, andserviceaccountscreate/delete (unused by cache engine code)podscreate/delete/patch,pods/status,eventsfull CRUD, duplicatepersistentvolumeclaims,controllerrevisions(all unused)statefulsets/statefulsets/status(cache runtime usesAdvancedStatefulSetfromworkload.fluid.io, not native StatefulSet)cacheruntimes/finalizerspermission (used by controller but was absent)Security risks addressed
clusterroles/clusterrolebindingscreate/deleteserviceaccountscreate/deletepodscreate/delete (duplicate block)pods/statusupdate/patcheventsget/list/watch/update/deletecreate/patchneeded forEventRecordercontrollerrevisionsfull CRUDTest plan
CacheRuntimeCR and verify the controller reconciles successfully (master/worker/client components created)pods/execstill works for UFS mount commandsCacheRuntimeand verify cleanup (node label removal, PV/PVC deletion) completes🤖 Generated with Claude Code