Allow imported and registered VMs to skip image validation checks on …

…create/update (#814) VMs that are imported, or registered already have the virtual machine created on the infrastructure. As such, we don't need to apply the same strict validations for them such as making sure the image is not null / valid. To that end, this change introduces annotations that will be applied on the Imported, and Registered VMs. This will allow our webhooks to skip these checks if the annotation is present. For now, maintain the existing mechanism which allows _all_ VM creations to be imageless if the import or the incremental restore feature is enabled. Once this change is merged, we will yank out that code to make that check stricter (only annotation based).
vmware-tanzu · Dec 9, 2024 · c5fecaf · c5fecaf
1 parent f415352
commit c5fecaf
Show file tree

Hide file tree

Showing 7 changed files with 503 additions and 21 deletions.
diff --git a/api/v1alpha3/virtualmachine_types.go b/api/v1alpha3/virtualmachine_types.go
@@ -197,6 +197,18 @@ const (
 	// ManagerID on a VirtualMachine contains the UUID of the
 	// VMware vCenter (VC) that is managing this virtual machine.
 	ManagerID = GroupName + "/manager-id"
+
+	// RegisteredVMAnnotation on a VirtualMachine represents that a virtual machine has
+	// been registered using the RegisterVM API after a restore, or a fail-over operation by
+	// a vendor. The presence of this annotation is used to bypass some validation checks
+	// that are otherwise applicable to all VirtualMachine create/update requests.
+	RegisteredVMAnnotation = GroupName + "/registered-vm"
+
+	// ImportedVMAnnotation on a VirtualMachine represents that a traditional virtual
+	// machine has been imported into Supervisor using the ImportVM API. The presence of this
+	// annotation is used to bypass some validation checks that are otherwise applicable
+	// to all VirtualMachine create/update requests.
+	ImportedVMAnnotation = GroupName + "/imported-vm"
 )
 
 const (

diff --git a/pkg/util/annotations/helpers.go b/pkg/util/annotations/helpers.go
@@ -9,6 +9,14 @@ import (
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3"
 )
 
+func HasRegisterVM(o metav1.Object) bool {
+	return hasAnnotation(o, vmopv1.RegisteredVMAnnotation)
+}
+
+func HasImportVM(o metav1.Object) bool {
+	return hasAnnotation(o, vmopv1.ImportedVMAnnotation)
+}
+
 func HasForceEnableBackup(o metav1.Object) bool {
 	return hasAnnotation(o, vmopv1.ForceEnableBackupAnnotation)
 }

diff --git a/pkg/util/annotations/helpers_test.go b/pkg/util/annotations/helpers_test.go
@@ -46,3 +46,37 @@ var _ = DescribeTable(
 	Entry("present but empty ", map[string]string{vmopv1.ForceEnableBackupAnnotation: ""}, true),
 	Entry("present and not empty ", map[string]string{vmopv1.ForceEnableBackupAnnotation: "true"}, true),
 )
+
+var _ = DescribeTable(
+	"HasRegisterVM",
+	func(in map[string]string, out bool) {
+		vm := &vmopv1.VirtualMachine{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: in,
+			},
+		}
+		actual := annotations.HasRegisterVM(vm)
+		Expect(actual).To(Equal(out))
+	},
+	Entry("nil", nil, false),
+	Entry("not present", map[string]string{"foo": "bar"}, false),
+	Entry("present but empty ", map[string]string{vmopv1.RegisteredVMAnnotation: ""}, true),
+	Entry("present and not empty ", map[string]string{vmopv1.RegisteredVMAnnotation: "true"}, true),
+)
+
+var _ = DescribeTable(
+	"HasImportVM",
+	func(in map[string]string, out bool) {
+		vm := &vmopv1.VirtualMachine{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: in,
+			},
+		}
+		actual := annotations.HasImportVM(vm)
+		Expect(actual).To(Equal(out))
+	},
+	Entry("nil", nil, false),
+	Entry("not present", map[string]string{"foo": "bar"}, false),
+	Entry("present but empty ", map[string]string{vmopv1.ImportedVMAnnotation: ""}, true),
+	Entry("present and not empty ", map[string]string{vmopv1.ImportedVMAnnotation: "true"}, true),
+)
diff --git a/pkg/util/vmopv1/vm.go b/pkg/util/vmopv1/vm.go
@@ -233,6 +233,19 @@ func IsImagelessVM(vm vmopv1.VirtualMachine) bool {
 	return vm.Spec.Image == nil && vm.Spec.ImageName == ""
 }
 
+// ImageRefsEqual returns true if the two image refs match.
+func ImageRefsEqual(ref1, ref2 *vmopv1.VirtualMachineImageRef) bool {
+	if ref1 == nil && ref2 == nil {
+		return true
+	}
+
+	if ref1 != nil && ref2 != nil {
+		return *ref1 == *ref2
+	}
+
+	return false
+}
+
 // SyncStorageUsageForNamespace updates the StoragePolicyUsage resource for
 // the given namespace and storage class with the reported usage information
 // for VMs in that namespace that use the specified storage class.

diff --git a/pkg/util/vmopv1/vm_test.go b/pkg/util/vmopv1/vm_test.go
@@ -522,6 +522,59 @@ var _ = DescribeTable("IsImageLessVM",
 	),
 )
 
+var _ = DescribeTable("ImageRefsEqual",
+	func(
+		ref1 *vmopv1.VirtualMachineImageRef,
+		ref2 *vmopv1.VirtualMachineImageRef,
+		expected bool,
+	) {
+		Ω(vmopv1util.ImageRefsEqual(ref1, ref2)).Should(Equal(expected))
+	},
+	Entry(
+		"both refs nil",
+		nil,
+		nil,
+		true,
+	),
+	Entry(
+		"ref1 is nil, ref2 is not",
+		nil,
+		&vmopv1.VirtualMachineImageRef{
+			Name: "dummy",
+		},
+		false,
+	),
+	Entry(
+		"ref1 is not nil, ref2 is nil",
+		&vmopv1.VirtualMachineImageRef{
+			Name: "dummy",
+		},
+		nil,
+		false,
+	),
+	Entry(
+		"both not nil, one containing an extra field",
+		&vmopv1.VirtualMachineImageRef{
+			Name: "dummy",
+		},
+		&vmopv1.VirtualMachineImageRef{
+			Name: "dummy",
+			Kind: "ClusterVirtualMachineImage",
+		},
+		false,
+	),
+	Entry(
+		"both not nil, same values",
+		&vmopv1.VirtualMachineImageRef{
+			Name: "dummy",
+		},
+		&vmopv1.VirtualMachineImageRef{
+			Name: "dummy",
+		},
+		true,
+	),
+)
+
 var _ = Describe("SyncStorageUsageForNamespace", func() {
 	var (
 		ctx          context.Context

diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator.go b/webhooks/virtualmachine/validation/virtualmachine_validator.go
@@ -41,6 +41,7 @@ import (
 	"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/config"
 	"github.com/vmware-tanzu/vm-operator/pkg/topology"
 	"github.com/vmware-tanzu/vm-operator/pkg/util"
+	"github.com/vmware-tanzu/vm-operator/pkg/util/annotations"
 	cloudinitvalidate "github.com/vmware-tanzu/vm-operator/pkg/util/cloudinit/validate"
 	kubeutil "github.com/vmware-tanzu/vm-operator/pkg/util/kube"
 	vmopv1util "github.com/vmware-tanzu/vm-operator/pkg/util/vmopv1"
@@ -153,6 +154,37 @@ func (v validator) ValidateDelete(*pkgctx.WebhookRequestContext) admission.Respo
 	return admission.Allowed("")
 }
 
+// Updates to VM's image are only allowed if it is a Registered VM (used for failover
+// in disaster recovery).
+func (v validator) validateImageOnUpdate(ctx *pkgctx.WebhookRequestContext, vm, oldVM *vmopv1.VirtualMachine) field.ErrorList {
+	var allErrs field.ErrorList
+
+	if vmopv1util.IsImagelessVM(*vm) && pkgcfg.FromContext(ctx).Features.VMIncrementalRestore {
+		if annotations.HasRegisterVM(vm) {
+			if !vmopv1util.ImageRefsEqual(vm.Spec.Image, oldVM.Spec.Image) {
+				if !ctx.IsPrivilegedAccount {
+					allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "image"), restrictedToPrivUsers))
+				}
+			}
+
+			if oldVM.Spec.ImageName != vm.Spec.ImageName {
+				if !ctx.IsPrivilegedAccount {
+					allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "imageName"), restrictedToPrivUsers))
+				}
+			}
+
+			return allErrs
+		}
+	}
+
+	allErrs = append(allErrs,
+		validation.ValidateImmutableField(vm.Spec.ImageName, oldVM.Spec.ImageName, field.NewPath("spec", "imageName"))...)
+	allErrs = append(allErrs,
+		validation.ValidateImmutableField(vm.Spec.Image, oldVM.Spec.Image, field.NewPath("spec", "image"))...)
+
+	return allErrs
+}
+
 // ValidateUpdate validates if the given VirtualMachineSpec update is valid.
 // Changes to following fields are not allowed:
 //   - Image
@@ -369,8 +401,19 @@ func (v validator) validateImageOnCreate(ctx *pkgctx.WebhookRequestContext, vm *
 	case vmopv1util.IsImagelessVM(*vm) &&
 		(pkgcfg.FromContext(ctx).Features.VMImportNewNet ||
 			pkgcfg.FromContext(ctx).Features.VMIncrementalRestore):
+		// TODO: Simplify this once mobility operator starts creating VMs with correct annotation.
+		// Skip validations on images if it is a VM created using ImportVM, or RegisterVM.
+		if annotations.HasImportVM(vm) || annotations.HasRegisterVM(vm) {
+			// Restrict creating imageless VM resources to privileged users.
+			if !ctx.IsPrivilegedAccount {
+				allErrs = append(allErrs, field.Forbidden(f, restrictedToPrivUsers))
+			}
+
+			return allErrs
+		}
 
 		// Restrict creating imageless VM resources to privileged users.
+		// TODO: This should be removed once all consumers have migrated to using annotations.
 		if !ctx.IsPrivilegedAccount {
 			allErrs = append(allErrs, field.Forbidden(f, restrictedToPrivUsers))
 		}
@@ -1143,8 +1186,7 @@ func (v validator) validateImmutableFields(ctx *pkgctx.WebhookRequestContext, vm
 	var allErrs field.ErrorList
 	specPath := field.NewPath("spec")
 
-	allErrs = append(allErrs, validation.ValidateImmutableField(vm.Spec.Image, oldVM.Spec.Image, specPath.Child("image"))...)
-	allErrs = append(allErrs, validation.ValidateImmutableField(vm.Spec.ImageName, oldVM.Spec.ImageName, specPath.Child("imageName"))...)
+	allErrs = append(allErrs, v.validateImageOnUpdate(ctx, vm, oldVM)...)
 	allErrs = append(allErrs, v.validateClassOnUpdate(ctx, vm, oldVM)...)
 	allErrs = append(allErrs, validation.ValidateImmutableField(vm.Spec.StorageClass, oldVM.Spec.StorageClass, specPath.Child("storageClass"))...)
 	// New VMs always have non-empty biosUUID. Existing VMs being upgraded may have an empty biosUUID.
@@ -1288,21 +1330,29 @@ func (v validator) validateAnnotation(ctx *pkgctx.WebhookRequestContext, vm, old
 	annotationPath := field.NewPath("metadata", "annotations")
 
 	if vm.Annotations[vmopv1.InstanceIDAnnotation] != oldVM.Annotations[vmopv1.InstanceIDAnnotation] {
-		allErrs = append(allErrs, field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), modifyAnnotationNotAllowedForNonAdmin))
+		allErrs = append(allErrs, field.Forbidden(annotationPath.Key(vmopv1.InstanceIDAnnotation), modifyAnnotationNotAllowedForNonAdmin))
 	}
 
 	if vm.Annotations[vmopv1.FirstBootDoneAnnotation] != oldVM.Annotations[vmopv1.FirstBootDoneAnnotation] {
-		allErrs = append(allErrs, field.Forbidden(annotationPath.Child(vmopv1.FirstBootDoneAnnotation), modifyAnnotationNotAllowedForNonAdmin))
+		allErrs = append(allErrs, field.Forbidden(annotationPath.Key(vmopv1.FirstBootDoneAnnotation), modifyAnnotationNotAllowedForNonAdmin))
+	}
+
+	if vm.Annotations[vmopv1.RegisteredVMAnnotation] != oldVM.Annotations[vmopv1.RegisteredVMAnnotation] {
+		allErrs = append(allErrs, field.Forbidden(annotationPath.Key(vmopv1.RegisteredVMAnnotation), modifyAnnotationNotAllowedForNonAdmin))
+	}
+
+	if vm.Annotations[vmopv1.ImportedVMAnnotation] != oldVM.Annotations[vmopv1.ImportedVMAnnotation] {
+		allErrs = append(allErrs, field.Forbidden(annotationPath.Key(vmopv1.ImportedVMAnnotation), modifyAnnotationNotAllowedForNonAdmin))
 	}
 
 	// The following annotations will be added by the mutation webhook upon VM creation.
 	if !reflect.DeepEqual(oldVM, &vmopv1.VirtualMachine{}) {
 		if vm.Annotations[constants.CreatedAtBuildVersionAnnotationKey] != oldVM.Annotations[constants.CreatedAtBuildVersionAnnotationKey] {
-			allErrs = append(allErrs, field.Forbidden(annotationPath.Child(constants.CreatedAtBuildVersionAnnotationKey), modifyAnnotationNotAllowedForNonAdmin))
+			allErrs = append(allErrs, field.Forbidden(annotationPath.Key(constants.CreatedAtBuildVersionAnnotationKey), modifyAnnotationNotAllowedForNonAdmin))
 		}
 
 		if vm.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] != oldVM.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] {
-			allErrs = append(allErrs, field.Forbidden(annotationPath.Child(constants.CreatedAtSchemaVersionAnnotationKey), modifyAnnotationNotAllowedForNonAdmin))
+			allErrs = append(allErrs, field.Forbidden(annotationPath.Key(constants.CreatedAtSchemaVersionAnnotationKey), modifyAnnotationNotAllowedForNonAdmin))
 		}
 	}