Reject VM defined with volume with no matching disk / fs

Addresses this bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1954667 Issue on github: #5556 Signed-off-by: Itamar Holder <iholder@redhat.com>
kubevirt · May 5, 2021 · d9d46ca · d9d46ca
1 parent 0ee92b1
commit d9d46ca
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 3 deletions.
diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go b/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{})
 
 	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] = nil
+	}
+
+	for _, fs := range spec.Domain.Devices.Filesystems {
+		diskAndFilesystemNames[fs.Name] = nil
 	}
+
+	// Validate that volumes match disks and filesystems correctly
+	for idx, volume := range spec.Volumes {
+		if _, machingDiskExists := diskAndFilesystemNames[volume.Name]; !machingDiskExists {
+			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
 }
 

diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go
@@ -581,7 +581,7 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 
 			causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), &vmi.Spec, config)
 			Expect(len(causes)).To(Equal(1))
-			//Expect(causes[0].Field).To(Equal("fake.domain.devices.disks[0].name"))
+			Expect(causes[0].Field).To(Equal("fake.domain.volumes[0].name"))
 		})
 		It("should reject multiple disks referencing same volume", func() {
 			vmi := v1.NewMinimalVMI("testvmi")

diff --git a/tests/storage/datavolume.go b/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())