Repository navigation
fix(kubernetes): let kubevirt-csi hot-unplug from an orphaned VMI - #4563
Conversation
The driver falls back to the VMI-level removevolume subresource when a volume is still attached to the VMI but no longer in the VM spec. The -kcsi Role grants only the VM-level subresources, and nothing else bound to the service account covers the VMI one: kubevirt.io: default grants no subresource of it, and the kubevirt.io:edit and admin roles that do are aggregated only into the built-in edit and admin roles, which the tenant namespace never binds to this account. Every such call ends in 403, the unpublish retries until it fails, and the hot-plug pod keeps the infra volume attached. The call stays needed once the driver stops falling back for a VM-owned VMI (kubevirt/csi-driver#382), because it still makes it for a VMI no VM owns. Deleting a VM with orphan propagation leaves its VMI running, since the KubeVirt VM controller then drops its finalizer without stopping the VMI, so the tenant node stays up and its volumes stay attached until the driver detaches them. The grant adds no reach the account lacks: it can already remove a volume from any VM in the namespace, and virt-api refuses the VMI call on a VM-owned VMI once the HotplugVolumes gate is off. virtualmachineinstances/addvolume stays ungranted, as the driver only attaches through the VM. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/kubernetes
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe infra-cluster CSI Role now grants ChangesCSI VMI RBAC
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The permission change appears ready to merge after normal checks; no actionable merge-blocking risk is identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new permission is limited to the CSI controller’s namespace and enables a specific recovery path. The controller checks that the requested hot-plug volume is still reported by the VMI, but the available evidence does not fully establish who can initiate a detach or how volume ownership is enforced. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM.
Small, correct RBAC change. The -kcsi Role gains update on virtualmachineinstances/removevolume and nothing else, which is exactly the subresource the driver's hot-unplug fallback calls. I checked the grant against the pinned client contract, the caller path, data safety, tenant scope, and the new test.
What I checked:
- Contract.
RemoveVolumeFromVMIin the pinned fork (kubevirt/[email protected],pkg/kubevirt/client.go:205) issuesPUT /apis/subresources.kubevirt.io/<ver>/namespaces/<ns>/virtualmachineinstances/<name>/removevolume. apiGroup, resource/subresource and verb (update== PUT) match the new rule. - Caller.
unpublishHotplugVolume(controller.go:421-465) is the only call site, and without this rule the apiserver returns 403 before virt-api ever sees the request. The Go driver is untouched here, so this is the companion grant for the fallback that already landed. - Data safety. The fallback runs only inside
ControllerUnpublishVolume, only for the exact volume id in the request, and only when the VMI still reports that id as aHotplugVolume. ANotFoundVMI returns success with no detach; any other get error returnsUnavailableand retries. It can't detach an unrelated or still-wanted volume, and the namespace is pinned to the infra namespace rather than taken from the request. - Least privilege.
addvolumeon the VMI is deliberately left out. The Role is namespaced and bound to the single-kcsiservice account, with nothing aggregating into it. The template is gated off on the proxmox substrate, so the grant renders only where the KubeVirt CSI runs. - Tests. The new helm-unittest pins the whole
subresources.kubevirt.iorule, and reverting the grant reddens it. The full chart suite is green (343/343) on the branch merged with currentmain.
One note, out of scope for this PR: unpublishHotplugVolume reads VMI volume status and then removes by name, so a volume detached and reattached under the same name between those two steps is a theoretical race. That code is already merged and isn't in this diff, the window is narrow, and the volume is an infra PVC under the CSI's own teardown, so I'm only flagging it for awareness.
2 participants
What this PR does
This lets the kubevirt-csi controller detach a volume from a VM instance that no VM owns. Right now that call is always denied. Closes #4452.
The driver has a fallback for a volume that is still attached to the VMI but is no longer in the VM spec. It calls the VMI-level
removevolumesubresource. The-kcsiRole grants only the two VM-level subresources, so the call ends in 403. Nothing else reaches this service account with that grant. Only its own Role and ClusterRole name it as a subject, and the group bindings that cover every service account grant nothing on this subresource. At v1.9.0,kubevirt.io:defaultgrants onlykubevirts,versionandguestfs.kubevirt.io:editandkubevirt.io:admindo include the VMI subresource, but they aggregate only into the built-ineditandadminroles, and no tenant binding gives those to this account. The cozystack tenant roles aggregate on their own labels and don't list it.The fallback is still needed after the flag in #4453 goes to false. The flag only skips VMIs that a VM owns. When a VM is deleted with
propagationPolicy=Orphan, the KubeVirt VM controller drops its finalizer and leaves the VMI running (vm.goat v1.9.0). The tenant node stays up with its volumes attached. When a pod moves off that node, the driver has to detach through the VMI, and on a 403 the hot-plug pod keeps holding the infra volume. Until #4453 lands, the fallback also runs for VM-owned VMIs and gets the same 403.The grant is
updateonvirtualmachineinstances/removevolumeand nothing more. The account can already remove a volume from any VM in the namespace, so it gets no new reach. virt-api also rejects the VMI-level call on a VM-owned VMI onceHotplugVolumesis off.virtualmachineinstances/addvolumestays out because the driver only attaches through the VM.The new helm-unittest pins the Role's
subresources.kubevirt.iorule to these three subresources withupdate. I haven't reproduced the 403 on a cluster. This shows it on a tenant namespace, before and after:Upstream kubevirt/csi-driver has the same gap in
deploy/infra-cluster-service-account.yaml.Screenshots
No UI change.
Downstream repositories
Release note
Summary by CodeRabbit