From d1b3d66dea17ef334309e69bc9b5ef8a0637a51f Mon Sep 17 00:00:00 2001 From: Arunesh Pandey Date: Thu, 5 Dec 2024 09:05:52 -0800 Subject: [PATCH] Allow imported and registered VMs to skip image validation checks on create/update 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). --- api/v1alpha3/virtualmachine_types.go | 12 + pkg/util/annotations/helpers.go | 8 + pkg/util/annotations/helpers_test.go | 34 ++ pkg/util/vmopv1/vm.go | 13 + pkg/util/vmopv1/vm_test.go | 53 +++ .../validation/virtualmachine_validator.go | 62 +++- .../virtualmachine_validator_unit_test.go | 342 +++++++++++++++++- 7 files changed, 503 insertions(+), 21 deletions(-) diff --git a/api/v1alpha3/virtualmachine_types.go b/api/v1alpha3/virtualmachine_types.go index f3cb45254..e6cd59ed9 100644 --- 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 index 1eb21160a..4079170dd 100644 --- 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 index 702825244..7ffe2ba8d 100644 --- 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 index b979e15f0..8a8d79bc3 100644 --- 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 index 8593aeecd..84c16335a 100644 --- 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 index 149f84bb2..7a6543d56 100644 --- 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)) } } diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go b/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go index 3270eb59d..4c8f9996f 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go +++ b/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go @@ -48,6 +48,8 @@ const ( dummyFirstBootDoneVal = "dummy-first-boot-done" dummyCreatedAtBuildVersionVal = "dummy-created-at-build-version" dummyCreatedAtSchemaVersionVal = "dummy-created-at-schema-version" + dummyRegisteredAnnVal = "dummy-registered-annotation" + dummyImportedAnnVal = "dummy-imported-annotation" dummyPausedVMLabelVal = "dummy-devops" dummyVmiName = "vmi-dummy" dummyNamespaceName = "dummy-vm-namespace-for-webhook-validation" @@ -65,12 +67,10 @@ type testParams struct { func doValidateWithMsg(msgs ...string) func(admission.Response) { return func(response admission.Response) { - reasons := strings.Split(string(response.Result.Reason), ", ") + reasons := string(response.Result.Reason) for _, m := range msgs { - ExpectWithOffset(1, reasons).To(ContainElement(m)) + ExpectWithOffset(1, reasons).To(ContainSubstring(m)) } - // This may be overly strict in some cases but catches missed assertions. - ExpectWithOffset(1, reasons).To(HaveLen(len(msgs))) } } @@ -580,6 +580,23 @@ func unitTestsValidateCreate() { ), // FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled + + Entry("allow empty spec.image for privileged user when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled and VM contains restored annotation", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + ctx.IsPrivilegedAccount = true + ctx.vm.Annotations = map[string]string{ + vmopv1.RegisteredVMAnnotation: "", + } + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + }, + expectAllowed: true, + }, + ), Entry("allow empty spec.image for privileged user when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled", testParams{ setup: func(ctx *unitValidatingWebhookContext) { @@ -623,6 +640,24 @@ func unitTestsValidateCreate() { ), }, ), + Entry("forbid empty spec.image for unprivileged user when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled and annotation is present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + ctx.IsPrivilegedAccount = false + ctx.vm.Annotations = map[string]string{ + vmopv1.RegisteredVMAnnotation: "", + } + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + }, + validate: doValidateWithMsg( + field.Forbidden(field.NewPath("spec", "image"), "restricted to privileged users").Error(), + ), + }, + ), Entry("require spec.image.kind for unprivileged user when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled", testParams{ setup: func(ctx *unitValidatingWebhookContext) { @@ -676,7 +711,23 @@ func unitTestsValidateCreate() { // // FSS_WCP_MOBILITY_VM_IMPORT_NEW_NET is enabled // - Entry("allow empty spec.image for privileged user when FSS_WCP_MOBILITY_VM_IMPORT_NEW_NET is enabled", + Entry("allow empty spec.image for privileged user when FSS_WCP_MOBILITY_VM_IMPORT_NEW_NET is enabled and annotation is present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + ctx.IsPrivilegedAccount = true + ctx.vm.Annotations = map[string]string{ + vmopv1.ImportedVMAnnotation: "", + } + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMImportNewNet = true + }) + }, + expectAllowed: true, + }, + ), + Entry("(To be removed) allow empty spec.image for privileged user when FSS_WCP_MOBILITY_VM_IMPORT_NEW_NET is enabled, but the annotation is not present", testParams{ setup: func(ctx *unitValidatingWebhookContext) { ctx.vm.Spec.Image = nil @@ -719,6 +770,24 @@ func unitTestsValidateCreate() { ), }, ), + Entry("forbid empty spec.image for unprivileged user when FSS_WCP_MOBILITY_VM_IMPORT_NEW_NET is enabled and annotation is present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + ctx.IsPrivilegedAccount = false + ctx.vm.Annotations = map[string]string{ + vmopv1.ImportedVMAnnotation: "", + } + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMImportNewNet = true + }) + }, + validate: doValidateWithMsg( + field.Forbidden(field.NewPath("spec", "image"), "restricted to privileged users").Error(), + ), + }, + ), Entry("require spec.image.kind for unprivileged user when FSS_WCP_MOBILITY_VM_IMPORT_NEW_NET is enabled", testParams{ setup: func(ctx *unitValidatingWebhookContext) { @@ -918,10 +987,14 @@ func unitTestsValidateCreate() { setup: func(ctx *unitValidatingWebhookContext) { ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = dummyInstanceIDVal ctx.vm.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal + ctx.vm.Annotations[vmopv1.RegisteredVMAnnotation] = dummyFirstBootDoneVal + ctx.vm.Annotations[vmopv1.ImportedVMAnnotation] = dummyFirstBootDoneVal }, validate: doValidateWithMsg( - field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), - field.Forbidden(annotationPath.Child(vmopv1.FirstBootDoneAnnotation), "modifying this annotation is not allowed for non-admin users").Error()), + field.Forbidden(annotationPath.Key(vmopv1.RegisteredVMAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.ImportedVMAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.InstanceIDAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.FirstBootDoneAnnotation), "modifying this annotation is not allowed for non-admin users").Error()), }, ), Entry("should allow creating VM with admin-only annotations set by service user", @@ -931,6 +1004,8 @@ func unitTestsValidateCreate() { ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = dummyInstanceIDVal ctx.vm.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal + ctx.vm.Annotations[vmopv1.RegisteredVMAnnotation] = dummyFirstBootDoneVal + ctx.vm.Annotations[vmopv1.ImportedVMAnnotation] = dummyFirstBootDoneVal }, expectAllowed: true, }, @@ -948,6 +1023,8 @@ func unitTestsValidateCreate() { ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = dummyInstanceIDVal ctx.vm.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal + ctx.vm.Annotations[vmopv1.RegisteredVMAnnotation] = dummyFirstBootDoneVal + ctx.vm.Annotations[vmopv1.ImportedVMAnnotation] = dummyFirstBootDoneVal }, expectAllowed: true, }, @@ -2710,16 +2787,24 @@ func unitTestsValidateUpdate() { ctx.oldVM.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal ctx.oldVM.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal ctx.oldVM.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + ctx.oldVM.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + ctx.oldVM.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal + ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = dummyInstanceIDVal + updateSuffix ctx.vm.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal + updateSuffix ctx.vm.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal + updateSuffix ctx.vm.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + updateSuffix + ctx.vm.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + updateSuffix + ctx.vm.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal + updateSuffix }, validate: doValidateWithMsg( - field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), - field.Forbidden(annotationPath.Child(vmopv1.FirstBootDoneAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), - field.Forbidden(annotationPath.Child(constants.CreatedAtBuildVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error(), - field.Forbidden(annotationPath.Child(constants.CreatedAtSchemaVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error()), + field.Forbidden(annotationPath.Key(vmopv1.InstanceIDAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.FirstBootDoneAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(constants.CreatedAtBuildVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(constants.CreatedAtSchemaVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.RegisteredVMAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.ImportedVMAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + ), }, ), Entry("should disallow removing admin-only annotations by SSO user", @@ -2729,12 +2814,17 @@ func unitTestsValidateUpdate() { ctx.oldVM.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal ctx.oldVM.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal ctx.oldVM.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + ctx.oldVM.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + ctx.oldVM.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal }, validate: doValidateWithMsg( - field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), - field.Forbidden(annotationPath.Child(vmopv1.FirstBootDoneAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), - field.Forbidden(annotationPath.Child(constants.CreatedAtBuildVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error(), - field.Forbidden(annotationPath.Child(constants.CreatedAtSchemaVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error()), + field.Forbidden(annotationPath.Key(vmopv1.InstanceIDAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.FirstBootDoneAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(constants.CreatedAtBuildVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(constants.CreatedAtSchemaVersionAnnotationKey), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.RegisteredVMAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + field.Forbidden(annotationPath.Key(vmopv1.ImportedVMAnnotation), "modifying this annotation is not allowed for non-admin users").Error(), + ), }, ), Entry("should allow updating admin-only annotations by service user", @@ -2746,10 +2836,15 @@ func unitTestsValidateUpdate() { ctx.oldVM.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal ctx.oldVM.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal ctx.oldVM.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + ctx.oldVM.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + ctx.oldVM.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal + ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = dummyInstanceIDVal + updateSuffix ctx.vm.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal + updateSuffix ctx.vm.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal + updateSuffix ctx.vm.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + updateSuffix + ctx.vm.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + updateSuffix + ctx.vm.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal + updateSuffix }, expectAllowed: true, }, @@ -2763,6 +2858,8 @@ func unitTestsValidateUpdate() { ctx.oldVM.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal ctx.oldVM.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal ctx.oldVM.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + ctx.oldVM.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + ctx.oldVM.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal }, expectAllowed: true, }, @@ -2784,10 +2881,15 @@ func unitTestsValidateUpdate() { ctx.oldVM.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal ctx.oldVM.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal ctx.oldVM.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + ctx.oldVM.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + ctx.oldVM.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal + ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = dummyInstanceIDVal + updateSuffix ctx.vm.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal + updateSuffix ctx.vm.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal + updateSuffix ctx.vm.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + updateSuffix + ctx.vm.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + updateSuffix + ctx.vm.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal + updateSuffix }, expectAllowed: true, }, @@ -2809,6 +2911,8 @@ func unitTestsValidateUpdate() { ctx.oldVM.Annotations[vmopv1.FirstBootDoneAnnotation] = dummyFirstBootDoneVal ctx.oldVM.Annotations[constants.CreatedAtBuildVersionAnnotationKey] = dummyCreatedAtBuildVersionVal ctx.oldVM.Annotations[constants.CreatedAtSchemaVersionAnnotationKey] = dummyCreatedAtSchemaVersionVal + ctx.oldVM.Annotations[vmopv1.RegisteredVMAnnotation] = dummyRegisteredAnnVal + ctx.oldVM.Annotations[vmopv1.ImportedVMAnnotation] = dummyImportedAnnVal }, expectAllowed: true, }, @@ -2861,6 +2965,214 @@ func unitTestsValidateUpdate() { ) }) + Context("Image and ImageName", func() { + DescribeTable("imageName", doTest, + Entry("forbid changing imageName to non empty value", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.oldVM.Spec.ImageName = dummyVmiName + + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.ImageName = dummyVmiName + updateSuffix + }, + validate: doValidateWithMsg( + fmt.Sprintf(`spec.imageName: Invalid value: "%v": field is immutable`, dummyVmiName+updateSuffix)), + }, + ), + + Entry("forbid unset of imageName if FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is disabled", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.oldVM.Spec.ImageName = dummyVmiName + + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.ImageName = "" + ctx.vm.Spec.Image = nil + }, + validate: doValidateWithMsg( + `spec.imageName: Invalid value: "": field is immutable`), + }, + ), + + // FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled + Entry("forbid unset of imageName if FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled, but annotation is not present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + + ctx.oldVM.Spec.ImageName = dummyVmiName + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.ImageName = "" + ctx.vm.Spec.Image = nil + }, + validate: doValidateWithMsg( + fmt.Sprintf(`spec.imageName: Invalid value: "%v": field is immutable`, "")), + }, + ), + + Entry("forbid unset of imageName by unprivileged users if FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled and registered annotation is present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + ctx.IsPrivilegedAccount = false + ctx.oldVM.Annotations = map[string]string{ + vmopv1.RegisteredVMAnnotation: "foo", + } + + ctx.oldVM.Spec.ImageName = dummyVmiName + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.ImageName = "" + ctx.vm.Spec.Image = nil + }, + validate: doValidateWithMsg( + field.Forbidden(field.NewPath("spec", "imageName"), "restricted to privileged users").Error()), + }, + ), + + Entry("allow unset of imageName for privileged users when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled, and registered annotation is present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + + ctx.IsPrivilegedAccount = true + + ctx.oldVM.Spec.ImageName = dummyVmiName + ctx.oldVM.Annotations = map[string]string{ + vmopv1.RegisteredVMAnnotation: "foo", + } + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.ImageName = "" + ctx.vm.Spec.Image = nil + }, + expectAllowed: true, + }, + ), + ) + + DescribeTable("image", doTest, + Entry("forbid changing image from nil to a non-nil value", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.oldVM.Spec.Image = nil + + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.Image = &vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName + updateSuffix, + } + }, + validate: doValidateWithMsg( + field.Invalid(field.NewPath("spec", "image"), &vmopv1.VirtualMachineImageRef{Name: dummyVmiName + updateSuffix}, apivalidation.FieldImmutableErrorMsg).Error()), + }, + ), + Entry("forbid changing image from non-nil to non-nil value", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.oldVM.Spec.Image = &vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName, + } + + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.Image = &vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName + updateSuffix, + } + }, + validate: doValidateWithMsg( + field.Invalid(field.NewPath("spec", "image"), &vmopv1.VirtualMachineImageRef{Name: dummyVmiName + updateSuffix}, apivalidation.FieldImmutableErrorMsg).Error()), + }, + ), + + Entry("forbid unset of image when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is disabled", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.oldVM.Spec.Image = &vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName, + } + + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + }, + validate: doValidateWithMsg( + field.Invalid(field.NewPath("spec", "image"), nil, apivalidation.FieldImmutableErrorMsg).Error()), + }, + ), + + Entry("forbid unset of image for privileged users when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled, but Registered annotation is not present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + + ctx.IsPrivilegedAccount = true + + ctx.oldVM.Spec.Image = &vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName, + } + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + }, + validate: doValidateWithMsg( + field.Invalid(field.NewPath("spec", "image"), nil, apivalidation.FieldImmutableErrorMsg).Error()), + }, + ), + + Entry("forbid unset of image by unprivileged users when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled and the Registered annotation is present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + ctx.IsPrivilegedAccount = false + + ctx.oldVM.Annotations = map[string]string{ + vmopv1.RegisteredVMAnnotation: "bar", + } + + ctx.oldVM.Spec.Image = &vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName, + } + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + }, + validate: doValidateWithMsg( + field.Forbidden(field.NewPath("spec", "image"), "restricted to privileged users").Error()), + }, + ), + + Entry("alow changing image for privileged users when FSS_WCP_VMSERVICE_INCREMENTAL_RESTORE is enabled, and Registered annotation is present", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) { + config.Features.VMIncrementalRestore = true + }) + + ctx.IsPrivilegedAccount = true + ctx.oldVM.Annotations = map[string]string{ + vmopv1.RegisteredVMAnnotation: "bar", + } + + ctx.oldVM.Spec.Image = &vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName, + } + ctx.vm = ctx.oldVM.DeepCopy() + ctx.vm.Spec.Image = nil + ctx.vm.Spec.ImageName = "" + }, + expectAllowed: true, + }, + ), + ) + }) + Context("ClassName", func() { DescribeTable("class name", doTest,