[webhooks]: Reject invalid kernel boot container spec

- Reject container boot defined without an image - Reject container boot defined without both kernel & initrd path - Add unit & functional tests to ensure webhook rejection - Rename IsKernelBootDefinedProperly() to HasKernelBootContainerImage() Signed-off-by: Itamar Holder <[email protected]>
attlee-wang · May 22, 2021 · eacd904 · eacd904
1 parent 3035f4c
commit eacd904
Show file tree

Hide file tree

Showing 11 changed files with 208 additions and 53 deletions.
diff --git a/examples/vmi-kernel-boot.yaml b/examples/vmi-kernel-boot.yaml
@@ -15,8 +15,6 @@ spec:
           initrdPath: /boot/initramfs-virt
           kernelPath: /boot/vmlinuz-virt
         kernelArgs: console=ttyS0
-    machine:
-      type: ""
     resources:
       requests:
         memory: 1Gi

diff --git a/pkg/container-disk/container-disk.go b/pkg/container-disk/container-disk.go
@@ -214,11 +214,7 @@ func GenerateContainers(vmi *v1.VirtualMachineInstance, podVolumeName string, bi
 }
 
 func GenerateKernelBootContainer(vmi *v1.VirtualMachineInstance, podVolumeName string, binVolumeName string) *kubev1.Container {
-	if !util.IsKernelBootDefinedProperly(vmi) {
-		if f := vmi.Spec.Domain.Firmware; f != nil && f.KernelBoot != nil && f.KernelBoot.Container != nil &&
-			f.KernelBoot.Container.InitrdPath == "" && f.KernelBoot.Container.KernelPath == "" {
-			log.Log.Object(vmi).Warning("Kernel boot container does not contain kernel path nor initrd path - ignoring kernel-boot configuration")
-		}
+	if !util.HasKernelBootContainerImage(vmi) {
 		return nil
 	}
 

diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -71,7 +71,7 @@ func ResourceNameToEnvVar(prefix string, resourceName string) string {
 }
 
 // Checks if kernel boot is defined in a valid way
-func IsKernelBootDefinedProperly(vmi *v1.VirtualMachineInstance) bool {
+func HasKernelBootContainerImage(vmi *v1.VirtualMachineInstance) bool {
 	if vmi == nil {
 		return false
 	}
@@ -81,9 +81,5 @@ func IsKernelBootDefinedProperly(vmi *v1.VirtualMachineInstance) bool {
 		return false
 	}
 
-	if c := vmiFirmware.KernelBoot.Container; (c.KernelPath == "") && (c.InitrdPath == "") {
-		return false
-	}
-
 	return true
 }
diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/BUILD.bazel b/pkg/virt-api/webhooks/validating-webhook/admitters/BUILD.bazel
@@ -74,6 +74,7 @@ go_test(
         "//staging/src/kubevirt.io/client-go/generated/kubevirt/clientset/versioned/fake:go_default_library",
         "//staging/src/kubevirt.io/client-go/kubecli:go_default_library",
         "//staging/src/kubevirt.io/client-go/testutils:go_default_library",
+        "//tools/vms-generator/utils:go_default_library",
         "//vendor/github.com/golang/mock/gomock:go_default_library",
         "//vendor/github.com/onsi/ginkgo:go_default_library",
         "//vendor/github.com/onsi/ginkgo/extensions/table:go_default_library",

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
@@ -1568,6 +1568,7 @@ func validateFirmware(field *k8sfield.Path, firmware *v1.Firmware) []metav1.Stat
 
 	if firmware != nil {
 		causes = append(causes, validateBootloader(field.Child("bootloader"), firmware.Bootloader)...)
+		causes = append(causes, validateKernelBoot(field.Child("kernelBoot"), firmware.KernelBoot)...)
 	}
 
 	return causes
@@ -2274,3 +2275,31 @@ func validateDisks(field *k8sfield.Path, disks []v1.Disk) []metav1.StatusCause {
 
 	return causes
 }
+
+// Rejects kernel boot defined with initrd/kernel path but without an image
+func validateKernelBoot(field *k8sfield.Path, kernelBoot *v1.KernelBoot) (causes []metav1.StatusCause) {
+	if kernelBoot == nil || kernelBoot.Container == nil {
+		return
+	}
+
+	container := kernelBoot.Container
+	containerField := field.Child("container").String()
+
+	if container.Image == "" {
+		causes = append(causes, metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueRequired,
+			Message: fmt.Sprintf("%s must be defined with an image", containerField),
+			Field:   containerField,
+		})
+	}
+
+	if container.InitrdPath == "" && container.KernelPath == "" {
+		causes = append(causes, metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueRequired,
+			Message: fmt.Sprintf("%s must be defined with at least one of the following: kernelPath, initrdPath", containerField),
+			Field:   containerField,
+		})
+	}
+
+	return
+}
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
@@ -25,6 +25,8 @@ import (
 	"strconv"
 	"strings"
 
+	"kubevirt.io/kubevirt/tools/vms-generator/utils"
+
 	"kubevirt.io/kubevirt/pkg/virt-operator/resource/generate/rbac"
 
 	. "github.com/onsi/ginkgo"
@@ -2213,6 +2215,57 @@ var _ = Describe("Validating VMICreate Admitter", func() {
 			Expect(causes[0].Field).To(Equal("fake.startStrategy"))
 			Expect(causes[0].Message).To(Equal("either fake.startStrategy or fake.livenessProbe should be provided.Pausing VMI with LivenessProbe is not supported"))
 		})
+		Context("with kernel boot defined", func() {
+
+			const (
+				fakeKernelArgs = "args"
+				fakeImage      = "image"
+				fakeInitrd     = "initrd"
+				fakeKernel     = "kernel"
+			)
+
+			table.DescribeTable("", func(kernelArgs, initrdPath, kernelPath, image string, defineContainerNil bool, shouldBeValid bool) {
+				vmi := utils.GetVMIKernelBoot()
+
+				kb := vmi.Spec.Domain.Firmware.KernelBoot
+
+				if defineContainerNil {
+					kb.Container = nil
+				} else {
+					kb.KernelArgs = kernelArgs
+					kb.Container.KernelPath = kernelPath
+					kb.Container.InitrdPath = initrdPath
+					kb.Container.Image = image
+				}
+
+				kernelBootField := k8sfield.NewPath("spec").Child("domain").Child("firmware").Child("kernelBoot")
+				causes := validateKernelBoot(kernelBootField, kb)
+
+				if shouldBeValid {
+					Expect(causes).To(BeEmpty())
+				} else {
+					Expect(causes).ToNot(BeEmpty())
+				}
+			},
+				table.Entry("without kernel args and null container - should approve",
+					"", "", "", "", true, true),
+				table.Entry("with kernel args and null container - should approve",
+					fakeKernelArgs, "", "", "", true, true),
+				table.Entry("without kernel args, with container that has image & kernel & initrd defined - should approve",
+					"", fakeInitrd, fakeKernel, fakeImage, false, true),
+				table.Entry("with kernel args, with container that has image & kernel & initrd defined - should approve",
+					fakeKernelArgs, fakeInitrd, fakeKernel, fakeImage, false, true),
+				table.Entry("with kernel args, with container that has image & kernel defined - should approve",
+					fakeKernelArgs, "", fakeKernel, fakeImage, false, true),
+				table.Entry("with kernel args, with container that has image & initrd defined - should approve",
+					fakeKernelArgs, fakeInitrd, "", fakeImage, false, true),
+				table.Entry("with kernel args, with container that has only image defined - should reject",
+					fakeKernelArgs, "", "", fakeImage, false, false),
+				table.Entry("with kernel args, with container that has initrd and kernel defined but without image - should reject",
+					fakeKernelArgs, fakeInitrd, fakeKernel, "", false, false),
+				table.Entry("with kernel args, with container that has nothing defined", "", "", "", "", false, false),
+			)
+		})
 	})
 	Context("with cpu pinning", func() {
 		var vmi *v1.VirtualMachineInstance

diff --git a/pkg/virt-controller/services/template.go b/pkg/virt-controller/services/template.go
@@ -1212,7 +1212,7 @@ func (t *templateService) renderLaunchManifest(vmi *v1.VirtualMachineInstance, t
 
 	var initContainers []k8sv1.Container
 
-	if HaveContainerDiskVolume(vmi.Spec.Volumes) || util.IsKernelBootDefinedProperly(vmi) {
+	if HaveContainerDiskVolume(vmi.Spec.Volumes) || util.HasKernelBootContainerImage(vmi) {
 
 		initContainerVolumeMounts := []k8sv1.VolumeMount{
 			{

diff --git a/pkg/virt-handler/container-disk/mount.go b/pkg/virt-handler/container-disk/mount.go
@@ -375,7 +375,7 @@ func (m *mounter) MountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo
 
 	log.Log.Object(vmi).Infof("mounting kernel artifacts")
 
-	if !util.IsKernelBootDefinedProperly(vmi) {
+	if !util.HasKernelBootContainerImage(vmi) {
 		log.Log.Object(vmi).Infof("kernel boot not defined - nothing to mount")
 		return nil
 	}
@@ -485,7 +485,7 @@ func (m *mounter) MountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo
 }
 
 func (m *mounter) UnmountKernelArtifacts(vmi *v1.VirtualMachineInstance) error {
-	if !util.IsKernelBootDefinedProperly(vmi) {
+	if !util.HasKernelBootContainerImage(vmi) {
 		return nil
 	}
 

diff --git a/pkg/virt-launcher/virtwrap/converter/converter.go b/pkg/virt-launcher/virtwrap/converter/converter.go
@@ -1154,7 +1154,7 @@ func Convert_v1_VirtualMachineInstance_To_api_Domain(vmi *v1.VirtualMachineInsta
 			domain.Spec.SysInfo.System = append(domain.Spec.SysInfo.System, api.Entry{Name: "serial", Value: string(vmi.Spec.Domain.Firmware.Serial)})
 		}
 
-		if util.IsKernelBootDefinedProperly(vmi) {
+		if util.HasKernelBootContainerImage(vmi) {
 			kb := vmi.Spec.Domain.Firmware.KernelBoot
 
 			log.Log.Object(vmi).Infof("kernel boot defined for VMI. Converting to domain XML")