[Bug fix] Reject VM defined with volume with no matching disk #5585

iholder101 · 2021-05-03T15:20:28Z

What this PR does / why we need it:
Reject VM defined with volume with no matching disk.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Bugzilla bug: https://bugzilla.redhat.com/show_bug.cgi?id=1954667
Github issue: #5556

Release note:

[bugfix] - reject VM defined with volume with no matching disk

stu-gott · 2021-05-03T15:23:01Z

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

 	}
+
+	// Validate that volumes match disks correctly
+	for idx, volume := range spec.Volumes {


OK! this ensures there's always a disk if a volume exists.

Is the reflexive case required? Do we need a volume if a disk exists?

The reflexive case is already being checked a few lines above (line 877) and there's also a test covering it

stu-gott · 2021-05-03T15:23:53Z

This appears to be something we could explicitly test for at the functional test level as well.

vladikr · 2021-05-04T03:27:52Z

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

+
+	// Validate that volumes match disks correctly
+	for idx, volume := range spec.Volumes {
+		if _, machingDiskExists := diskNames[volume.Name]; !machingDiskExists {


@iholder-redhat volumes can also match spec.Domain.Devices.Filesystems

see [1] for example.
[1] https://github.com/kubevirt/kubevirt/blob/master/pkg/virt-launcher/virtwrap/converter/converter.go#L1373

Thanks!
diskNames is now diskAndFilesystemNames and contain both fs and disk names :)

iholder101 · 2021-05-04T09:14:29Z

This appears to be something we could explicitly test for at the functional test level as well.

@stu-gott I've added functional tests :)
p.s. do we have a policy / rule of thumb on when to add a functional test?

iholder101 · 2021-05-04T14:10:25Z

/retest

iholder101 · 2021-05-04T19:34:15Z

/test pull-kubevirt-e2e-k8s-1.17-rook-ceph

Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 · 2021-05-05T10:10:16Z

/test pull-kubevirt-unit-test

zcahana · 2021-05-05T14:09:38Z

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

@@ -861,12 +861,14 @@ func validateNetworkHasOnlyOneType(field *k8sfield.Path, cniTypesCount int, caus
 func validateBootOrder(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec, volumeNameMap map[string]*v1.Volume) (bootOrderMap map[uint]bool, causes []metav1.StatusCause) {
 	// used to validate uniqueness of boot orders among disks and interfaces
 	bootOrderMap = make(map[uint]bool)
+	// to perform as set of volume / fs names
+	diskAndFilesystemNames := make(map[string]interface{})


nit: I think it's somewhat more idiomatic/common to use a map[string]struct{}, and then "signal the presence" of a disk/fs by having a diskAndFilesystemNames[disk.Name] = struct{}{}.

Cool. I'm still new to Go, so good to know :)

Here's a related post by the prophet Dave Cheney:
https://dave.cheney.net/2014/03/25/the-empty-struct

zcahana · 2021-05-05T14:26:33Z

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

+
+	// Validate that volumes match disks and filesystems correctly
+	for idx, volume := range spec.Volumes {
+		if _, machingDiskExists := diskAndFilesystemNames[volume.Name]; !machingDiskExists {


typo-nit: machingDiskExists --> matchingDiskExists

Addresses this bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1954667 Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 · 2021-05-06T07:10:02Z

/retest

zcahana · 2021-05-06T11:58:20Z

It's worthy to mention that for plain Kubernetes pods, it's perfectly legal to define a pod volume but no volumeMount. The justification for that is unclear to me, and still it has been like that for years and there seem to be no open/closed bug about it.

Nevertheless, we can do better than pods. Thanks @iholder-redhat!
/lgtm

vladikr · 2021-05-06T12:53:26Z

Thank @iholder-redhat
/lgtm
/approve

kubevirt-bot · 2021-05-06T12:54:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vladikr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vladikr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stu-gott · 2021-05-19T11:39:57Z

/cherry-pick release-0.41

xpivarc · 2021-11-22T14:34:48Z

/cherry-pick release-0.36

kubevirt-bot · 2021-11-22T14:35:26Z

@xpivarc: #5585 failed to apply on top of branch "release-0.36":

Applying: Add tests to ensure VM volume with missing disk / fs is rejected
Applying: Reject VM defined with volume with no matching disk / fs
Using index info to reconstruct a base tree...
M	pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
M	pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
A	tests/storage/datavolume.go
Falling back to patching base and 3-way merge...
Auto-merging tests/datavolume_test.go
Auto-merging pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
Auto-merging pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
CONFLICT (content): Merge conflict in pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Reject VM defined with volume with no matching disk / fs
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.36

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels May 3, 2021

kubevirt-bot requested review from ashleyschuett and vatsalparekh May 3, 2021 15:21

stu-gott reviewed May 3, 2021

View reviewed changes

vladikr reviewed May 4, 2021

View reviewed changes

iholder101 force-pushed the bug/vm_no_cloudinit_disk branch from 9e504ef to 68046b5 Compare May 4, 2021 09:11

Add tests to ensure VM volume with missing disk / fs is rejected

0ee92b1

Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 force-pushed the bug/vm_no_cloudinit_disk branch from 68046b5 to d9d46ca Compare May 5, 2021 08:51

zcahana reviewed May 5, 2021

View reviewed changes

Reject VM defined with volume with no matching disk / fs

cd67593

Addresses this bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1954667 Signed-off-by: Itamar Holder <iholder@redhat.com>

iholder101 force-pushed the bug/vm_no_cloudinit_disk branch from d9d46ca to cd67593 Compare May 5, 2021 15:31

kubevirt-bot assigned zcahana May 6, 2021

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 6, 2021

kubevirt-bot assigned vladikr May 6, 2021

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2021

kubevirt-bot merged commit f4321c2 into kubevirt:master May 6, 2021

stu-gott mentioned this pull request May 19, 2021

[release-0.41] Reject VM defined with volume with no matching disk #5685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug fix] Reject VM defined with volume with no matching disk #5585

[Bug fix] Reject VM defined with volume with no matching disk #5585

iholder101 commented May 3, 2021

stu-gott May 3, 2021

iholder101 May 3, 2021 •

edited

stu-gott commented May 3, 2021

vladikr May 4, 2021

vladikr May 4, 2021

iholder101 May 4, 2021

iholder101 commented May 4, 2021

iholder101 commented May 4, 2021

iholder101 commented May 4, 2021

iholder101 commented May 5, 2021

zcahana May 5, 2021

iholder101 May 5, 2021

zcahana May 5, 2021

zcahana May 5, 2021

iholder101 commented May 6, 2021

zcahana commented May 6, 2021

vladikr commented May 6, 2021

kubevirt-bot commented May 6, 2021

stu-gott commented May 19, 2021

xpivarc commented Nov 22, 2021

kubevirt-bot commented Nov 22, 2021

[Bug fix] Reject VM defined with volume with no matching disk #5585

[Bug fix] Reject VM defined with volume with no matching disk #5585

Conversation

iholder101 commented May 3, 2021

Choose a reason for hiding this comment

iholder101 May 3, 2021 • edited

Choose a reason for hiding this comment

stu-gott commented May 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iholder101 commented May 4, 2021

iholder101 commented May 4, 2021

iholder101 commented May 4, 2021

iholder101 commented May 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iholder101 commented May 6, 2021

zcahana commented May 6, 2021

vladikr commented May 6, 2021

kubevirt-bot commented May 6, 2021

stu-gott commented May 19, 2021

xpivarc commented Nov 22, 2021

kubevirt-bot commented Nov 22, 2021

iholder101 May 3, 2021 •

edited