Merge pull request #5585 from iholder-redhat/bug/vm_no_cloudinit_disk

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

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]struct{})
 
 	for i, volume := range spec.Volumes {
 		volumeNameMap[volume.Name] = &spec.Volumes[i]
 	}
 
-	// Validate disks and volumes match up correctly
+	// Validate disks match volumes correctly
 	for idx, disk := range spec.Domain.Devices.Disks {
 		var matchingVolume *v1.Volume
 
@@ -901,7 +903,26 @@ func validateBootOrder(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec
 			}
 			bootOrderMap[order] = true
 		}
+
+		diskAndFilesystemNames[disk.Name] = struct{}{}
+	}
+
+	for _, fs := range spec.Domain.Devices.Filesystems {
+		diskAndFilesystemNames[fs.Name] = struct{}{}
 	}
+
+	// Validate that volumes match disks and filesystems correctly
+	for idx, volume := range spec.Volumes {
+		if _, matchingDiskExists := diskAndFilesystemNames[volume.Name]; !matchingDiskExists {
+			causes = append(causes, metav1.StatusCause{
+				Type:    metav1.CauseTypeFieldValueInvalid,
+				Message: fmt.Sprintf(nameOfTypeNotFoundMessagePattern, field.Child("domain", "volumes").Index(idx).Child("name").String(), volume.Name),
+				Field:   field.Child("domain", "volumes").Index(idx).Child("name").String(),
+			})
+		}
+
+	}
+
 	return bootOrderMap, causes
 }
 

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

@@ -558,7 +558,6 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			Expect(len(causes)).To(Equal(1))
 			Expect(causes[0].Field).To(Equal("fake.volumes"))
 		})
-
 		It("should reject disk with missing volume", func() {
 			vmi := v1.NewMinimalVMI("testvmi")
 
@@ -570,6 +569,20 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			Expect(len(causes)).To(Equal(1))
 			Expect(causes[0].Field).To(Equal("fake.domain.devices.disks[0].name"))
 		})
+		It("should reject volume with missing disk / file system", func() {
+			vmi := v1.NewMinimalVMI("testvmi")
+
+			vmi.Spec.Volumes = append(vmi.Spec.Volumes, v1.Volume{
+				Name: "testvolume",
+				VolumeSource: v1.VolumeSource{
+					CloudInitNoCloud: &v1.CloudInitNoCloudSource{UserData: " "},
+				},
+			})
+
+			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
+			Expect(len(causes)).To(Equal(1))
+			Expect(causes[0].Field).To(Equal("fake.domain.volumes[0].name"))
+		})
 		It("should reject multiple disks referencing same volume", func() {
 			vmi := v1.NewMinimalVMI("testvmi")
 

tests/storage/datavolume.go

@@ -685,8 +685,9 @@ var _ = SIGDescribe("[Serial]DataVolume Integration", func() {
 
 			table.DescribeTable("deny then allow clone request on rook-ceph", func(role *rbacv1.Role, allServiceAccounts, allServiceAccountsInNamespace bool) {
 				vm := tests.NewRandomVMWithCloneDataVolume(dataVolume.Namespace, dataVolume.Name, tests.NamespaceTestDefault)
+				const volumeName = "sa"
 				saVol := v1.Volume{
-					Name: "sa",
+					Name: volumeName,
 					VolumeSource: v1.VolumeSource{
 						ServiceAccount: &v1.ServiceAccountVolumeSource{
 							ServiceAccountName: tests.AdminServiceAccountName,
@@ -695,6 +696,7 @@ var _ = SIGDescribe("[Serial]DataVolume Integration", func() {
 				}
 				vm.Spec.DataVolumeTemplates[0].Spec.PVC.StorageClassName = pointer.StringPtr(storageClass)
 				vm.Spec.Template.Spec.Volumes = append(vm.Spec.Template.Spec.Volumes, saVol)
+				vm.Spec.Template.Spec.Domain.Devices.Disks = append(vm.Spec.Template.Spec.Domain.Devices.Disks, v1.Disk{Name: volumeName})
 
 				vmBytes, err := json.Marshal(vm)
 				Expect(err).ToNot(HaveOccurred())

tests/vmi_configuration_test.go

@@ -1507,6 +1507,40 @@ var _ = Describe("[sig-compute]Configurations", func() {
 			})
 		})
 
+		Context("with volumes, disks and filesystem defined", func() {
+
+			var vmi *v1.VirtualMachineInstance
+
+			BeforeEach(func() {
+				vmi = tests.NewRandomVMI()
+			})
+
+			It("should reject disk with missing volume", func() {
+				const diskName = "testdisk"
+				vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{
+					Name: diskName,
+				})
+				vmi, err = virtClient.VirtualMachineInstance(tests.NamespaceTestDefault).Create(vmi)
+				Expect(err).To(HaveOccurred())
+				const expectedErrMessage = "denied the request: spec.domain.devices.disks[0].Name '" + diskName + "' not found."
+				Expect(err.Error()).To(ContainSubstring(expectedErrMessage))
+			})
+
+			It("should reject volume with missing disk / file system", func() {
+				const volumeName = "testvolume"
+				vmi.Spec.Volumes = append(vmi.Spec.Volumes, v1.Volume{
+					Name: volumeName,
+					VolumeSource: v1.VolumeSource{
+						CloudInitNoCloud: &v1.CloudInitNoCloudSource{UserData: " "},
+					},
+				})
+				vmi, err = virtClient.VirtualMachineInstance(tests.NamespaceTestDefault).Create(vmi)
+				Expect(err).To(HaveOccurred())
+				const expectedErrMessage = "denied the request: spec.domain.volumes[0].name '" + volumeName + "' not found."
+				Expect(err.Error()).To(ContainSubstring(expectedErrMessage))
+			})
+
+		})
 	})
 
 	Context("[rfe_id:140][crit:medium][vendor:cnv-qe@redhat.com][level:component]with CPU spec", func() {