Skip to content

Commit 86bf2ba

Browse files
IvanHuntersIvanHunters
authored
fix(kubernetes): let kubevirt-csi hot-unplug from an orphaned VMI (#4563)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## 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 `removevolume` subresource. The `-kcsi` Role 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:default` grants only `kubevirts`, `version` and `guestfs`. `kubevirt.io:edit` and `kubevirt.io:admin` do include the VMI subresource, but they aggregate only into the built-in `edit` and `admin` roles, 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.go` at 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 `update` on `virtualmachineinstances/removevolume` and 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 once `HotplugVolumes` is off. `virtualmachineinstances/addvolume` stays out because the driver only attaches through the VM. The new helm-unittest pins the Role's `subresources.kubevirt.io` rule to these three subresources with `update`. I haven't reproduced the 403 on a cluster. This shows it on a tenant namespace, before and after: ```bash kubectl auth can-i update virtualmachineinstances.subresources.kubevirt.io --subresource=removevolume \ --namespace <tenant-ns> --as system:serviceaccount:<tenant-ns>:<release>-kcsi ``` Upstream kubevirt/csi-driver has the same gap in `deploy/infra-cluster-service-account.yaml`. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> No UI change. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(kubernetes): the kubevirt-csi controller can now hot-unplug a volume from a VMI that no VM owns, instead of failing with 403 Forbidden. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Kubernetes CSI volume removal for virtual machine instances, including cases where a volume is not listed in the virtual machine specification. This helps the driver complete volume removal in scenarios where the Kubernetes virtualization API accepts the instance-level request. Existing permissions for adding and removing volumes from virtual machines remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents ec4d26d + 2b7e0db commit 86bf2ba

2 files changed

Lines changed: 28 additions & 1 deletion

File tree

‎packages/apps/kubernetes/templates/csi/infra-cluster-service-account.yaml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,11 @@ rules:
2323
- apiGroups: ["kubevirt.io"]
2424
resources: ["virtualmachineinstances", "virtualmachines"]
2525
verbs: ["list", "get"]
26+
# virtualmachineinstances/removevolume: the driver's fallback for a volume
27+
# missing from the VM spec. Without the HotplugVolumes gate virt-api accepts
28+
# it only on a VMI no VM owns, e.g. after an orphan-propagation delete.
2629
- apiGroups: ["subresources.kubevirt.io"]
27-
resources: ["virtualmachines/addvolume", "virtualmachines/removevolume"]
30+
resources: ["virtualmachines/addvolume", "virtualmachines/removevolume", "virtualmachineinstances/removevolume"]
2831
verbs: ["update"]
2932
- apiGroups: ["snapshot.storage.k8s.io"]
3033
resources: ["volumesnapshots"]

‎packages/apps/kubernetes/tests/csi_rbac_test.yaml‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
suite: kubevirt-csi infra-cluster Role
2+
3+
templates:
4+
- templates/csi/infra-cluster-service-account.yaml
5+
6+
values:
7+
- values/common.yaml
8+
9+
release:
10+
name: kubernetes-test
11+
namespace: tenant-test
12+
13+
tests:
14+
- it: grants only the VM hotplug subresources and VMI-level removevolume
15+
documentSelector:
16+
path: kind
17+
value: Role
18+
asserts:
19+
- equal:
20+
path: rules[?(@.apiGroups[0] == "subresources.kubevirt.io")]
21+
value:
22+
apiGroups: ["subresources.kubevirt.io"]
23+
resources: ["virtualmachines/addvolume", "virtualmachines/removevolume", "virtualmachineinstances/removevolume"]
24+
verbs: ["update"]

0 commit comments

Comments
 (0)