operator: run controller plugin privileged when log rotation is enabled (fixes Helm default incompatibility with SELinux) - #592
Conversation
|
Friendly note for reviewers: the CI workflows on this PR are currently in the |
@abhinav-phi if its written using AI please add assisted-by or co-authored-by to the commit |
| // Some platforms require controller plugin to run privileged, | ||
| // For example, OpenShift with SELinux restrictions requires the pod to be privileged to write to hostPath. | ||
| // To enable logrotation for csi pods. | ||
| // Deprecated: when log rotation is enabled, the operator always runs the |
There was a problem hiding this comment.
i dont think we need any deprecation notice , this is as per the design https://github.com/ceph/ceph-csi-operator/blob/c88282b004301a8514c98751ebf115c7661dce8a/docs/design/logrotate.md not all platforms will have selinux enabled, it need to be set when selinux is enabled.
There was a problem hiding this comment.
Agreed, Madhu. The deprecation notice and the unconditional privilege escalation are both too broad, so I propose dropping both changes.
The narrower revision would keep the existing controllerPlugin.privileged setting as an explicit opt-in for SELinux deployments that use hostPath log rotation, and would document that setting in the Helm values and in troubleshooting guidance. Non-SELinux deployments would keep their current unprivileged defaults. One limitation worth noting for mixed clusters: this is a per-driver deployment setting, so on SELinux hosts the controller pods would also need node placement rules, because the security context cannot be chosen per node after scheduling.
This proposal was prepared with AI assistance, and no code has been revised yet. Does this documentation-focused approach match your intended design? I will revise the implementation once we agree on the direction.
|
Madhu, I have replied in the open review thread with a narrower proposal that preserves the existing On the AI assistance trailer: I have amended the commit to add |
dea210c to
34a7abb
Compare
|
@Madhu-1, before I change the implementation, could you explicitly confirm whether you agree with this narrower scope: remove the blanket privilege escalation and field deprecation, preserve the existing explicit |
i would just go with documentation for this one. |
|
Confirming docs-only: revert escalation+deprecation, keep explicit setting + Helm/docs guidance - starting revision. |
|
@Madhu-1 docs-only revision pushed, please re-review. HEAD: 34a7abb -> a3d3502 (normal push, no force). Reverted behavior change per your direction (5725566891):
Docs-only (no auto-detection logic, API unchanged, privileged:false default kept):
Verify: git diff --check clean, go build ./internal/controller/... passes, no Privileged:ptr.To(true) auto-escalation remains (only pre-existing node/sidecar privileged). Thanks! |
| If log rotation (`log.rotation.enabled`) uses a hostPath volume (`log.rotation.logHostPath`) | ||
| on hosts with SELinux in enforcing mode (for example, OpenShift), the controller plugin | ||
| containers need `controllerPlugin.privileged:true` to write the rotated log files, as | ||
| Kubernetes does not relabel hostPath volumes for SELinux. See `docs/design/logrotate.md`. |
There was a problem hiding this comment.
provide a complete link to it
| # -- Enable log rotation (default: true) | ||
| # On SELinux-enforcing hosts, combine with controllerPlugin.privileged:true | ||
| # when using hostPath log storage; the operator does not auto-escalate. | ||
| # See docs/design/logrotate.md. |
There was a problem hiding this comment.
remove the link to the document as it will not be helpful from here
|
@Madhu-1 Done in f607294 — the troubleshooting section in drivers-chart.gotmpl.md now uses the complete design-doc URL, and all the See-docs lines are removed from values.yaml (operational guidance kept). drivers-chart.md is hand-synced to match since I don't have helm-docs locally — CI will confirm it matches a regen. PTAL, thanks! |
|
@abhinav-phi can you please squash the commits to 1, changes LGTM |
Document in Helm values.yaml that SELinux-enforcing hosts using hostPath log rotation should set controllerPlugin.privileged:true alongside log.rotation.enabled/logHostPath (see docs/design/logrotate.md), with privileged:false default unchanged and no auto-escalation. Add troubleshooting section in drivers chart docs, including mixed-cluster node-placement caveat. Troubleshooting section links the full design doc URL; values.yaml keeps operational guidance without repeating the link. Generated drivers-chart.md is hand-synced to match (no helm-docs locally); CI will confirm. Signed-off-by: abhinav-phi <alpha9coder@gmail.com>
f607294 to
45d9aa7
Compare
This branch has not been deployed
2 participants
Summary
This PR addresses the controller plugin startup problem reported in #577 when hostPath log rotation is used on SELinux-enforcing hosts. The original implementation enables privilege whenever log rotation is enabled. Madhu raised a valid objection: that changes security settings on every platform and ignores the existing explicit configuration option.
Status: awaiting agreement on a narrower design. The proposed revision below is not implemented. The only recent commit change adds an AI assistance trailer; the source tree is unchanged. The open review thread has not been marked resolved.
Problem and existing design
The reported deployment uses log rotation with a hostPath log directory while the controller plugin is configured as unprivileged. Access to that directory can be denied by SELinux policy. This does not mean every deployment with log rotation needs privileged containers, nor does it establish that privileged containers are the only possible solution.
The existing log rotation design already documents
controllerPlugin.privilegedas an explicit setting for SELinux platforms that require privilege to write to the hostPath. That setting should remain available rather than being deprecated or silently overridden.What the current branch changes
The current implementation runs the log-writing controller containers privileged whenever log rotation is enabled, even when
controllerPlugin.privilegedis explicitly false. It also marks that field deprecated and ignored. The branch includes corresponding unit tests, Helm values documentation, release notes, and generated API and installation artifacts.These are the disputed changes under review, not an accepted solution. The branch has not yet been revised to restore the original explicit privilege semantics.
Proposed revision
The proposal in the review-thread reply is to remove both the deprecation and the unconditional escalation, retain the existing explicit privilege setting, and narrow the contribution to clear Helm configuration and troubleshooting guidance for SELinux deployments using hostPath log rotation. Non-SELinux defaults would remain unchanged.
This would be configuration guidance rather than automatic SELinux detection or a claim that the default installation now works on every platform. In a mixed cluster, administrators would also need to align controller pod placement with the intended nodes, because a Deployment template does not select a different security context after each pod is scheduled.
Please confirm whether this documentation-focused direction matches the intended design before the implementation is revised.
Validation and limitations
The branch contains unit tests for the original behavior; those tests do not validate the proposed revision. No tests were rerun for the recent commit-message-only amendment, and no SELinux-enabled deployment was tested during this follow-up. DCO passed after the amendment, but the PR workflows require maintainer approval, so this is not a claim that the complete CI suite is green.
The existing sign-off is preserved. The added
Assisted-by: ZCode AI assistant (review and revision planning)trailer records the verified scope of AI assistance during this follow-up.Related issue
Related to #577. Resolution remains pending review of the design and validation of any subsequent revision.