From a72c3b4abf1f60e4b322ce683c41055d09393520 Mon Sep 17 00:00:00 2001 From: Sai Diliyaer Date: Mon, 12 Aug 2024 16:00:04 -0400 Subject: [PATCH] Update CD-ROM API fields and related webhook code (#659) This PR updates CD-ROM API fields and docs for clarity and improves the VM webhook for ISO VM deployment. Specifically, this patch includes: - Fix `AllowGuestControl` and `Connected` fields with boolean pointers. - Update VM mutating webhook to not set the default CD-ROM name and re-populate `VirtualMachineImage` kind if previously set to default. - Update VM validating webhook to require `guestID` if VM has a CD-ROM and handle duplicates across namespace and cluster scope images (since they will point to the same backing file name). --- .../virtualmachine_conversion_test.go | 4 +- .../virtualmachine_conversion_test.go | 4 +- api/v1alpha3/virtualmachine_types.go | 29 +++--- api/v1alpha3/zz_generated.deepcopy.go | 14 ++- ....vmware.com_virtualmachinereplicasets.yaml | 29 ++++-- ...vmoperator.vmware.com_virtualmachines.yaml | 29 ++++-- docs/ref/api/v1alpha3.md | 47 +++++++--- .../vsphere/session/session_vm_update_test.go | 4 +- .../vsphere/vmprovider_vm_utils_test.go | 4 +- pkg/util/vmopv1/resize_overwrite_test.go | 2 + pkg/util/vsphere/vm/cdrom.go | 19 ++-- pkg/util/vsphere/vm/cdrom_test.go | 89 ++++++++++--------- test/builder/dummies.go | 22 ++--- .../mutation/virtualmachine_mutator.go | 39 ++++++-- .../virtualmachine_mutator_intg_test.go | 83 +++++++++++++++-- .../virtualmachine_mutator_unit_test.go | 55 ++++++++++-- .../validation/virtualmachine_validator.go | 35 ++++---- .../virtualmachine_validator_intg_test.go | 67 +++++++++++--- .../virtualmachine_validator_unit_test.go | 81 +++++++++++++---- 19 files changed, 477 insertions(+), 179 deletions(-) diff --git a/api/v1alpha1/virtualmachine_conversion_test.go b/api/v1alpha1/virtualmachine_conversion_test.go index 840b1a875..fa7b997b8 100644 --- a/api/v1alpha1/virtualmachine_conversion_test.go +++ b/api/v1alpha1/virtualmachine_conversion_test.go @@ -224,8 +224,8 @@ func TestVirtualMachineConversion(t *testing.T) { Name: "my-cdrom-image", Kind: "VirtualMachineImage", }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptrOf(true), + Connected: ptrOf(true), }, }, }, diff --git a/api/v1alpha2/virtualmachine_conversion_test.go b/api/v1alpha2/virtualmachine_conversion_test.go index ac2e202d7..0b203d329 100644 --- a/api/v1alpha2/virtualmachine_conversion_test.go +++ b/api/v1alpha2/virtualmachine_conversion_test.go @@ -246,8 +246,8 @@ func TestVirtualMachineConversion(t *testing.T) { Name: "my-cdrom-image", Kind: "VirtualMachineImage", }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptrOf(true), + Connected: ptrOf(true), }, }, }, diff --git a/api/v1alpha3/virtualmachine_types.go b/api/v1alpha3/virtualmachine_types.go index 89f2a045b..13cdb9578 100644 --- a/api/v1alpha3/virtualmachine_types.go +++ b/api/v1alpha3/virtualmachine_types.go @@ -276,9 +276,10 @@ type VirtualMachineImageRef struct { type VirtualMachineCdromSpec struct { // +kubebuilder:validation:Pattern="^[a-z0-9]{2,}$" - // Name describes the unique and immutable name of this CD-ROM device. - // If omitted or empty, the controller will generate it as "cdrom" + "num", - // where "num" is the device's index in the spec.cdrom list. + // Name consists of at least two lowercase letters or digits of this CD-ROM. + // It must be unique among all CD-ROM devices attached to the VM. + // + // This field is immutable when the VM is powered on. Name string `json:"name"` // Image describes the reference to an ISO type VirtualMachineImage or @@ -303,8 +304,11 @@ type VirtualMachineCdromSpec struct { // disconnected from the VM. If the CD-ROM device already exists, it is // updated to a disconnected state. // + // Please note that disconnecting a CD-ROM during guest OS installation may + // not work since the CD-ROM might be locked by the guest. + // // Defaults to true if omitted. - Connected bool `json:"connected,omitempty"` + Connected *bool `json:"connected,omitempty"` // +optional // +kubebuilder:default=true @@ -313,7 +317,7 @@ type VirtualMachineCdromSpec struct { // may be used to connect/disconnect the CD-ROM device. // // Defaults to true if omitted. - AllowGuestControl bool `json:"allowGuestControl,omitempty"` + AllowGuestControl *bool `json:"allowGuestControl,omitempty"` } // VirtualMachineSpec defines the desired state of a VirtualMachine. @@ -326,7 +330,9 @@ type VirtualMachineSpec struct { // // Each CD-ROM device requires a reference to an ISO-type // VirtualMachineImage or ClusterVirtualMachineImage resource as backing. - // More than one CD-ROM device with the backing image is disallowed. + // + // Multiple CD-ROM devices using the same backing image, regardless of image + // kinds (namespace or cluster scope), are not allowed. // // CD-ROM devices can be added, updated, or removed when the VM is powered // off. When the VM is powered on, only the connection state of existing @@ -590,11 +596,10 @@ type VirtualMachineSpec struct { // // The logic that determines the guest ID is as follows: // - // 1. If this field is set, then its value is used. - // 2. Otherwise, if the VirtualMachineClass used to deploy the VM contains a - // non-empty guest ID, then it is used. - // 3. Finally, if this field is still undetermined, and the VM is deployed - // from an OVF template that defines a guest ID, then that value is used. + // If this field is set, then its value is used. + // Otherwise, if the VM is deployed from an OVF template that defines a + // guest ID, then that value is used. + // The guest ID from VirtualMachineClass used to deploy the VM is ignored. // // Please refer to https://bit.ly/4elnjP3 for a complete list of supported // guest operating system identifiers. @@ -602,6 +607,8 @@ type VirtualMachineSpec struct { // Please note that this field is immutable after the VM is powered on. // To change the guest ID after the VM is powered on, the VM must be powered // off and then powered on again with the updated guest ID spec. + // + // This field is required when the VM has any CD-ROM devices attached. GuestID string `json:"guestID,omitempty"` } diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index c4383de5d..e759a1b55 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -573,6 +573,16 @@ func (in *VirtualMachineBootstrapVAppConfigSpec) DeepCopy() *VirtualMachineBoots func (in *VirtualMachineCdromSpec) DeepCopyInto(out *VirtualMachineCdromSpec) { *out = *in out.Image = in.Image + if in.Connected != nil { + in, out := &in.Connected, &out.Connected + *out = new(bool) + **out = **in + } + if in.AllowGuestControl != nil { + in, out := &in.AllowGuestControl, &out.AllowGuestControl + *out = new(bool) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new VirtualMachineCdromSpec. @@ -2030,7 +2040,9 @@ func (in *VirtualMachineSpec) DeepCopyInto(out *VirtualMachineSpec) { if in.Cdrom != nil { in, out := &in.Cdrom, &out.Cdrom *out = make([]VirtualMachineCdromSpec, len(*in)) - copy(*out, *in) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } } if in.Image != nil { in, out := &in.Image, &out.Image diff --git a/config/crd/bases/vmoperator.vmware.com_virtualmachinereplicasets.yaml b/config/crd/bases/vmoperator.vmware.com_virtualmachinereplicasets.yaml index d5ab1bef1..612d9ab7a 100644 --- a/config/crd/bases/vmoperator.vmware.com_virtualmachinereplicasets.yaml +++ b/config/crd/bases/vmoperator.vmware.com_virtualmachinereplicasets.yaml @@ -993,7 +993,10 @@ spec: Each CD-ROM device requires a reference to an ISO-type VirtualMachineImage or ClusterVirtualMachineImage resource as backing. - More than one CD-ROM device with the backing image is disallowed. + + + Multiple CD-ROM devices using the same backing image, regardless of image + kinds (namespace or cluster scope), are not allowed. CD-ROM devices can be added, updated, or removed when the VM is powered @@ -1028,6 +1031,10 @@ spec: updated to a disconnected state. + Please note that disconnecting a CD-ROM during guest OS installation may + not work since the CD-ROM might be locked by the guest. + + Defaults to true if omitted. type: boolean image: @@ -1059,9 +1066,11 @@ spec: type: object name: description: |- - Name describes the unique and immutable name of this CD-ROM device. - If omitted or empty, the controller will generate it as "cdrom" + "num", - where "num" is the device's index in the spec.cdrom list. + Name consists of at least two lowercase letters or digits of this CD-ROM. + It must be unique among all CD-ROM devices attached to the VM. + + + This field is immutable when the VM is powered on. pattern: ^[a-z0-9]{2,}$ type: string required: @@ -1091,11 +1100,10 @@ spec: The logic that determines the guest ID is as follows: - 1. If this field is set, then its value is used. - 2. Otherwise, if the VirtualMachineClass used to deploy the VM contains a - non-empty guest ID, then it is used. - 3. Finally, if this field is still undetermined, and the VM is deployed - from an OVF template that defines a guest ID, then that value is used. + If this field is set, then its value is used. + Otherwise, if the VM is deployed from an OVF template that defines a + guest ID, then that value is used. + The guest ID from VirtualMachineClass used to deploy the VM is ignored. Please refer to https://bit.ly/4elnjP3 for a complete list of supported @@ -1105,6 +1113,9 @@ spec: Please note that this field is immutable after the VM is powered on. To change the guest ID after the VM is powered on, the VM must be powered off and then powered on again with the updated guest ID spec. + + + This field is required when the VM has any CD-ROM devices attached. type: string image: description: |- diff --git a/config/crd/bases/vmoperator.vmware.com_virtualmachines.yaml b/config/crd/bases/vmoperator.vmware.com_virtualmachines.yaml index c9293b316..638364d7c 100644 --- a/config/crd/bases/vmoperator.vmware.com_virtualmachines.yaml +++ b/config/crd/bases/vmoperator.vmware.com_virtualmachines.yaml @@ -3980,7 +3980,10 @@ spec: Each CD-ROM device requires a reference to an ISO-type VirtualMachineImage or ClusterVirtualMachineImage resource as backing. - More than one CD-ROM device with the backing image is disallowed. + + + Multiple CD-ROM devices using the same backing image, regardless of image + kinds (namespace or cluster scope), are not allowed. CD-ROM devices can be added, updated, or removed when the VM is powered @@ -4015,6 +4018,10 @@ spec: updated to a disconnected state. + Please note that disconnecting a CD-ROM during guest OS installation may + not work since the CD-ROM might be locked by the guest. + + Defaults to true if omitted. type: boolean image: @@ -4046,9 +4053,11 @@ spec: type: object name: description: |- - Name describes the unique and immutable name of this CD-ROM device. - If omitted or empty, the controller will generate it as "cdrom" + "num", - where "num" is the device's index in the spec.cdrom list. + Name consists of at least two lowercase letters or digits of this CD-ROM. + It must be unique among all CD-ROM devices attached to the VM. + + + This field is immutable when the VM is powered on. pattern: ^[a-z0-9]{2,}$ type: string required: @@ -4078,11 +4087,10 @@ spec: The logic that determines the guest ID is as follows: - 1. If this field is set, then its value is used. - 2. Otherwise, if the VirtualMachineClass used to deploy the VM contains a - non-empty guest ID, then it is used. - 3. Finally, if this field is still undetermined, and the VM is deployed - from an OVF template that defines a guest ID, then that value is used. + If this field is set, then its value is used. + Otherwise, if the VM is deployed from an OVF template that defines a + guest ID, then that value is used. + The guest ID from VirtualMachineClass used to deploy the VM is ignored. Please refer to https://bit.ly/4elnjP3 for a complete list of supported @@ -4092,6 +4100,9 @@ spec: Please note that this field is immutable after the VM is powered on. To change the guest ID after the VM is powered on, the VM must be powered off and then powered on again with the updated guest ID spec. + + + This field is required when the VM has any CD-ROM devices attached. type: string image: description: |- diff --git a/docs/ref/api/v1alpha3.md b/docs/ref/api/v1alpha3.md index c8b764647..78ee59b2c 100644 --- a/docs/ref/api/v1alpha3.md +++ b/docs/ref/api/v1alpha3.md @@ -692,9 +692,11 @@ _Appears in:_ | Field | Description | | --- | --- | -| `name` _string_ | Name describes the unique and immutable name of this CD-ROM device. -If omitted or empty, the controller will generate it as "cdrom" + "num", -where "num" is the device's index in the spec.cdrom list. | +| `name` _string_ | Name consists of at least two lowercase letters or digits of this CD-ROM. +It must be unique among all CD-ROM devices attached to the VM. + + +This field is immutable when the VM is powered on. | | `image` _[VirtualMachineImageRef](#virtualmachineimageref)_ | Image describes the reference to an ISO type VirtualMachineImage or ClusterVirtualMachineImage resource used as the backing for the CD-ROM. If the image kind is omitted, it defaults to VirtualMachineImage. @@ -717,6 +719,10 @@ disconnected from the VM. If the CD-ROM device already exists, it is updated to a disconnected state. +Please note that disconnecting a CD-ROM during guest OS installation may +not work since the CD-ROM might be locked by the guest. + + Defaults to true if omitted. | | `allowGuestControl` _boolean_ | AllowGuestControl describes whether or not a web console connection may be used to connect/disconnect the CD-ROM device. @@ -822,6 +828,21 @@ _Appears in:_ +### VirtualMachineImageDiskInfo + + + +VirtualMachineImageDiskInfo describes information about any disks associated with +this image. + +_Appears in:_ +- [VirtualMachineImageStatus](#virtualmachineimagestatus) + +| Field | Description | +| --- | --- | +| `capacity` _[Quantity](#quantity)_ | Capacity is the virtual disk capacity in bytes. | +| `size` _[Quantity](#quantity)_ | Size is the estimated populated size of the virtual disk in bytes. | + ### VirtualMachineImageOSInfo @@ -942,6 +963,7 @@ image. | | `vmwareSystemProperties` _KeyValuePair array_ | VMwareSystemProperties describes the observed VMware system properties defined for this image. | | `productInfo` _[VirtualMachineImageProductInfo](#virtualmachineimageproductinfo)_ | ProductInfo describes the observed product information for this image. | +| `disks` _[VirtualMachineImageDiskInfo](#virtualmachineimagediskinfo) array_ | Disks describes the observed disk information for this image. | | `providerContentVersion` _string_ | ProviderContentVersion describes the content version from the provider item that this image corresponds to. If the provider of this image is a Content Library, this will be the version of the corresponding Content Library item. | @@ -2090,7 +2112,10 @@ _Appears in:_ Each CD-ROM device requires a reference to an ISO-type VirtualMachineImage or ClusterVirtualMachineImage resource as backing. -More than one CD-ROM device with the backing image is disallowed. + + +Multiple CD-ROM devices using the same backing image, regardless of image +kinds (namespace or cluster scope), are not allowed. CD-ROM devices can be added, updated, or removed when the VM is powered @@ -2303,11 +2328,10 @@ default value for spec.bootstrap.cloudInit.instanceID if it is omitted. | The logic that determines the guest ID is as follows: -1. If this field is set, then its value is used. -2. Otherwise, if the VirtualMachineClass used to deploy the VM contains a - non-empty guest ID, then it is used. -3. Finally, if this field is still undetermined, and the VM is deployed -from an OVF template that defines a guest ID, then that value is used. +If this field is set, then its value is used. +Otherwise, if the VM is deployed from an OVF template that defines a +guest ID, then that value is used. +The guest ID from VirtualMachineClass used to deploy the VM is ignored. Please refer to https://bit.ly/4elnjP3 for a complete list of supported @@ -2316,7 +2340,10 @@ guest operating system identifiers. Please note that this field is immutable after the VM is powered on. To change the guest ID after the VM is powered on, the VM must be powered -off and then powered on again with the updated guest ID spec. | +off and then powered on again with the updated guest ID spec. + + +This field is required when the VM has any CD-ROM devices attached. | ### VirtualMachineStatus diff --git a/pkg/providers/vsphere/session/session_vm_update_test.go b/pkg/providers/vsphere/session/session_vm_update_test.go index ac0f49534..7af202d6d 100644 --- a/pkg/providers/vsphere/session/session_vm_update_test.go +++ b/pkg/providers/vsphere/session/session_vm_update_test.go @@ -1553,8 +1553,8 @@ var _ = Describe("UpdateVirtualMachine", func() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) diff --git a/pkg/providers/vsphere/vmprovider_vm_utils_test.go b/pkg/providers/vsphere/vmprovider_vm_utils_test.go index 338c61d26..b10de80b3 100644 --- a/pkg/providers/vsphere/vmprovider_vm_utils_test.go +++ b/pkg/providers/vsphere/vmprovider_vm_utils_test.go @@ -207,10 +207,10 @@ func vmUtilTests() { ) BeforeEach(func() { - nsVMImage = builder.DummyVirtualMachineImage(builder.DummyVMIID) + nsVMImage = builder.DummyVirtualMachineImage(builder.DummyVMIName) nsVMImage.Namespace = vmCtx.VM.Namespace conditions.MarkTrue(nsVMImage, vmopv1.ReadyConditionType) - clusterVMImage = builder.DummyClusterVirtualMachineImage(builder.DummyVMIID) + clusterVMImage = builder.DummyClusterVirtualMachineImage(builder.DummyCVMIName) conditions.MarkTrue(clusterVMImage, vmopv1.ReadyConditionType) }) diff --git a/pkg/util/vmopv1/resize_overwrite_test.go b/pkg/util/vmopv1/resize_overwrite_test.go index 449d44805..b36858eab 100644 --- a/pkg/util/vmopv1/resize_overwrite_test.go +++ b/pkg/util/vmopv1/resize_overwrite_test.go @@ -91,6 +91,8 @@ var _ = Describe("OverwriteResizeConfigSpec", func() { vmAdvSpec := func(advSpec vmopv1.VirtualMachineAdvancedSpec) vmopv1.VirtualMachine { vm := builder.DummyVirtualMachine() vm.Spec.Advanced = &advSpec + // Empty guestID to avoid it being set in the returned ConfigSpec. + vm.Spec.GuestID = "" return *vm } diff --git a/pkg/util/vsphere/vm/cdrom.go b/pkg/util/vsphere/vm/cdrom.go index 88e6a77e8..4a8367295 100644 --- a/pkg/util/vsphere/vm/cdrom.go +++ b/pkg/util/vsphere/vm/cdrom.go @@ -17,6 +17,7 @@ import ( vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context" "github.com/vmware-tanzu/vm-operator/pkg/util" + "github.com/vmware-tanzu/vm-operator/pkg/util/ptr" ) const ( @@ -43,7 +44,7 @@ func UpdateCdromDeviceChanges( for _, specCdrom := range vmCtx.VM.Spec.Cdrom { imageRef := specCdrom.Image // Sync the content library file if needed to connect the CD-ROM device. - syncFile := specCdrom.Connected + syncFile := ptr.Deref(specCdrom.Connected) bFileName, err := getBackingFileNameByImageRef(vmCtx, libManager, k8sClient, syncFile, imageRef) if err != nil { return nil, fmt.Errorf("error getting backing file name by image ref %s: %w", imageRef, err) @@ -122,7 +123,7 @@ func UpdateConfigSpecCdromDeviceConnection( for _, specCdrom := range cdromSpec { imageRef := specCdrom.Image // Sync the content library file if needed to connect the CD-ROM device. - syncFile := specCdrom.Connected + syncFile := ptr.Deref(specCdrom.Connected) bFileName, err := getBackingFileNameByImageRef(vmCtx, libManager, k8sClient, syncFile, imageRef) if err != nil { return fmt.Errorf("error getting backing file name by image ref %s: %w", imageRef, err) @@ -265,9 +266,9 @@ func createNewCdrom( VirtualDevice: vimtypes.VirtualDevice{ Backing: backing, Connectable: &vimtypes.VirtualDeviceConnectInfo{ - AllowGuestControl: cdromSpec.AllowGuestControl, - StartConnected: cdromSpec.Connected, - Connected: cdromSpec.Connected, + AllowGuestControl: ptr.Deref(cdromSpec.AllowGuestControl), + StartConnected: ptr.Deref(cdromSpec.Connected), + Connected: ptr.Deref(cdromSpec.Connected), }, }, } @@ -378,10 +379,10 @@ func updateCurCdromsConnectionState( for b, spec := range backingFileNameToCdromSpec { if cdrom, ok := backingFileNameToCdrom[b]; ok { if c := cdrom.GetVirtualDevice().Connectable; c != nil && - (c.Connected != spec.Connected || c.AllowGuestControl != spec.AllowGuestControl) { - c.StartConnected = spec.Connected - c.Connected = spec.Connected - c.AllowGuestControl = spec.AllowGuestControl + (c.Connected != ptr.Deref(spec.Connected) || c.AllowGuestControl != ptr.Deref(spec.AllowGuestControl)) { + c.StartConnected = ptr.Deref(spec.Connected) + c.Connected = ptr.Deref(spec.Connected) + c.AllowGuestControl = ptr.Deref(spec.AllowGuestControl) deviceChanges = append(deviceChanges, &vimtypes.VirtualDeviceConfigSpec{ Device: cdrom, diff --git a/pkg/util/vsphere/vm/cdrom_test.go b/pkg/util/vsphere/vm/cdrom_test.go index cd755d882..ef7ed5850 100644 --- a/pkg/util/vsphere/vm/cdrom_test.go +++ b/pkg/util/vsphere/vm/cdrom_test.go @@ -21,6 +21,7 @@ import ( vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3" pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config" pkgctx "github.com/vmware-tanzu/vm-operator/pkg/context" + "github.com/vmware-tanzu/vm-operator/pkg/util/ptr" pkgclient "github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/client" vmutil "github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm" "github.com/vmware-tanzu/vm-operator/test/builder" @@ -110,8 +111,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -243,8 +244,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, { Name: cdromName2, @@ -252,8 +253,8 @@ func cdromTests() { Name: cvmiName, Kind: cvmiKind, }, - AllowGuestControl: false, - Connected: false, + AllowGuestControl: ptr.To(false), + Connected: ptr.To(false), }, } @@ -307,8 +308,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } curDevices = object.VirtualDeviceList{ @@ -353,8 +354,8 @@ func cdromTests() { Kind: vmiKind, }, // Disconnect the CD-ROM device and disallow guest control. - AllowGuestControl: false, - Connected: false, + AllowGuestControl: ptr.To(false), + Connected: ptr.To(false), }, } curDevices = object.VirtualDeviceList{ @@ -407,8 +408,8 @@ func cdromTests() { Name: "non-existent-vmi", Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -430,8 +431,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -453,8 +454,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -474,8 +475,8 @@ func cdromTests() { Name: "non-existent-cvmi", Kind: cvmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -497,8 +498,8 @@ func cdromTests() { Name: cvmiName, Kind: cvmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -520,8 +521,8 @@ func cdromTests() { Name: cvmiName, Kind: cvmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -541,8 +542,8 @@ func cdromTests() { Name: vmiName, Kind: "invalid-kind", }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -564,8 +565,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -587,8 +588,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -610,8 +611,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -673,8 +674,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -702,8 +703,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } @@ -815,8 +816,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -863,8 +864,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: false, - Connected: false, + AllowGuestControl: ptr.To(false), + Connected: ptr.To(false), }, } }) @@ -900,8 +901,8 @@ func cdromTests() { Name: "non-existent-vmi", Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -951,8 +952,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) @@ -974,8 +975,8 @@ func cdromTests() { Name: vmiName, Kind: vmiKind, }, - AllowGuestControl: true, - Connected: true, + AllowGuestControl: ptr.To(true), + Connected: ptr.To(true), }, } }) diff --git a/test/builder/dummies.go b/test/builder/dummies.go index 5ea11631b..c9b779086 100644 --- a/test/builder/dummies.go +++ b/test/builder/dummies.go @@ -29,7 +29,8 @@ import ( ) const ( - DummyVMIID = "vmi-0123456789" + DummyVMIName = "vmi-0123456789" + DummyCVMIName = "vmi-9876543210" DummyImageName = "dummy-image-name" DummyClassName = "dummyClassName" DummyVolumeName = "dummy-volume-name" @@ -303,7 +304,7 @@ func DummyBasicVirtualMachine(name, namespace string) *vmopv1.VirtualMachine { Spec: vmopv1.VirtualMachineSpec{ Image: &vmopv1.VirtualMachineImageRef{ Kind: vmiKind, - Name: DummyVMIID, + Name: DummyVMIName, }, ImageName: DummyImageName, ClassName: DummyClassName, @@ -327,7 +328,7 @@ func DummyVirtualMachine() *vmopv1.VirtualMachine { Spec: vmopv1.VirtualMachineSpec{ Image: &vmopv1.VirtualMachineImageRef{ Kind: vmiKind, - Name: DummyVMIID, + Name: DummyVMIName, }, ImageName: DummyImageName, ClassName: DummyClassName, @@ -359,21 +360,22 @@ func DummyVirtualMachine() *vmopv1.VirtualMachine { Name: "cdrom1", Image: vmopv1.VirtualMachineImageRef{ Kind: vmiKind, - Name: DummyVMIID, + Name: DummyVMIName, }, - Connected: true, - AllowGuestControl: true, + Connected: ptr.To(true), + AllowGuestControl: ptr.To(true), }, { Name: "cdrom2", Image: vmopv1.VirtualMachineImageRef{ Kind: cvmiKind, - Name: DummyVMIID, + Name: DummyCVMIName, }, - Connected: true, - AllowGuestControl: true, + Connected: ptr.To(true), + AllowGuestControl: ptr.To(true), }, }, + GuestID: DummyOSType, }, } } @@ -401,7 +403,7 @@ func DummyVirtualMachineReplicaSet() *vmopv1.VirtualMachineReplicaSet { Spec: vmopv1.VirtualMachineSpec{ Image: &vmopv1.VirtualMachineImageRef{ Kind: vmiKind, - Name: DummyVMIID, + Name: DummyVMIName, }, ImageName: DummyImageName, ClassName: DummyClassName, diff --git a/webhooks/virtualmachine/mutation/virtualmachine_mutator.go b/webhooks/virtualmachine/mutation/virtualmachine_mutator.go index 960ef08f4..1b7a4d775 100644 --- a/webhooks/virtualmachine/mutation/virtualmachine_mutator.go +++ b/webhooks/virtualmachine/mutation/virtualmachine_mutator.go @@ -120,7 +120,7 @@ func (m mutator) Mutate(ctx *pkgctx.WebhookRequestContext) admission.Response { SetCreatedAtAnnotations(ctx, modified) AddDefaultNetworkInterface(ctx, m.client, modified) SetDefaultPowerState(ctx, m.client, modified) - SetDefaultCdromNameAndImgKind(ctx, modified) + SetDefaultCdromImgKindOnCreate(ctx, modified) SetImageNameFromCdrom(ctx, modified) if _, err := SetDefaultInstanceUUID(ctx, m.client, modified); err != nil { return admission.Denied(err.Error()) @@ -148,6 +148,10 @@ func (m mutator) Mutate(ctx *pkgctx.WebhookRequestContext) admission.Response { } else if ok { wasMutated = true } + + if ok := SetDefaultCdromImgKindOnUpdate(ctx, modified, oldVM); ok { + wasMutated = true + } } if !wasMutated { @@ -486,20 +490,41 @@ func SetLastResizeAnnotation( return false, nil } -func SetDefaultCdromNameAndImgKind( +func SetDefaultCdromImgKindOnCreate( ctx *pkgctx.WebhookRequestContext, vm *vmopv1.VirtualMachine) { for i, c := range vm.Spec.Cdrom { - if c.Name == "" { - // Name has a required pattern ("^[a-z0-9]{2,}$") in the CRD schema. - vm.Spec.Cdrom[i].Name = fmt.Sprintf("%s%d", defaultCdromNamePrefix, i+1) + if c.Image.Kind == "" { + vm.Spec.Cdrom[i].Image.Kind = vmiKind } + } +} - if c.Image.Kind == "" { +func SetDefaultCdromImgKindOnUpdate( + ctx *pkgctx.WebhookRequestContext, + vm, oldVM *vmopv1.VirtualMachine) bool { + + var ( + mutated bool + imgNameToOldKind = make(map[string]string, len(oldVM.Spec.Cdrom)) + ) + + for _, c := range oldVM.Spec.Cdrom { + imgNameToOldKind[c.Image.Name] = c.Image.Kind + } + + for i, c := range vm.Spec.Cdrom { + // Repopulate the image kind only if it was previously set to default. + // This ensures an error is returned if the image kind was reset from + // a different value other than the default VirtualMachineImage kind. + if c.Image.Kind == "" && imgNameToOldKind[c.Image.Name] == vmiKind { vm.Spec.Cdrom[i].Image.Kind = vmiKind + mutated = true } } + + return mutated } func SetImageNameFromCdrom( @@ -514,7 +539,7 @@ func SetImageNameFromCdrom( var cdromImageName string for _, cdrom := range vm.Spec.Cdrom { // Set the image name to the first connected CD-ROM image name. - if cdrom.Connected { + if cdrom.Connected != nil && *cdrom.Connected { cdromImageName = cdrom.Image.Name break } diff --git a/webhooks/virtualmachine/mutation/virtualmachine_mutator_intg_test.go b/webhooks/virtualmachine/mutation/virtualmachine_mutator_intg_test.go index 9ef42bea9..ec6afb9ec 100644 --- a/webhooks/virtualmachine/mutation/virtualmachine_mutator_intg_test.go +++ b/webhooks/virtualmachine/mutation/virtualmachine_mutator_intg_test.go @@ -1,4 +1,4 @@ -// Copyright (c) 2019-2023 VMware, Inc. All Rights Reserved. +// Copyright (c) 2019-2024 VMware, Inc. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package mutation_test @@ -62,7 +62,7 @@ func intgTestsMutating() { BeforeEach(func() { ctx = newIntgMutatingWebhookContext() - img = builder.DummyVirtualMachineImage(builder.DummyVMIID) + img = builder.DummyVirtualMachineImage(builder.DummyVMIName) img.Namespace = ctx.vm.Namespace Expect(ctx.Client.Create(ctx, img)).To(Succeed()) img.Status.Name = ctx.vm.Spec.ImageName @@ -289,7 +289,7 @@ func intgTestsMutating() { ExpectWithOffset(1, ctx.Client.Get(ctx, client.ObjectKeyFromObject(ctx.vm), modified)).To(Succeed()) ExpectWithOffset(1, modified.Spec.Image).ToNot(BeNil()) ExpectWithOffset(1, modified.Spec.Image.Kind).To(Equal(vmiKind)) - ExpectWithOffset(1, modified.Spec.Image.Name).To(Equal(builder.DummyVMIID)) + ExpectWithOffset(1, modified.Spec.Image.Name).To(Equal(builder.DummyVMIName)) ExpectWithOffset(1, modified.Spec.ImageName).To(Equal(ctx.vm.Spec.ImageName)) } @@ -308,7 +308,7 @@ func intgTestsMutating() { }) When("spec.imageName is vmi", func() { BeforeEach(func() { - ctx.vm.Spec.ImageName = builder.DummyVMIID + ctx.vm.Spec.ImageName = builder.DummyVMIName }) It("Should mutate Image but not ImageName", func() { shouldMutateImageButNotImageName() @@ -335,7 +335,7 @@ func intgTestsMutating() { When("spec.image is non-empty", func() { BeforeEach(func() { ctx.vm.Spec.Image = &vmopv1.VirtualMachineImageRef{ - Name: builder.DummyVMIID, + Name: builder.DummyVMIName, Kind: vmiKind, } }) @@ -482,4 +482,77 @@ func intgTestsMutating() { }) }) }) + + Context("CD-ROM", func() { + + When("creating a VM", func() { + + When("spec.cdrom.image.kind is empty", func() { + + BeforeEach(func() { + for i, c := range ctx.vm.Spec.Cdrom { + c.Image.Kind = "" + ctx.vm.Spec.Cdrom[i] = c + } + }) + + It("should set default kind to VirtualMachineImage", func() { + Expect(ctx.Client.Create(ctx, ctx.vm)).To(Succeed()) + vm := &vmopv1.VirtualMachine{} + Expect(ctx.Client.Get(ctx, client.ObjectKeyFromObject(ctx.vm), vm)).To(Succeed()) + for _, c := range vm.Spec.Cdrom { + Expect(c.Image.Kind).To(Equal("VirtualMachineImage")) + } + }) + }) + + When("spec.imageName is empty", func() { + + BeforeEach(func() { + ctx.vm.Spec.ImageName = "" + }) + + It("should set the spec.imageName to the first connected CD-ROM image name", func() { + Expect(ctx.Client.Create(ctx, ctx.vm)).To(Succeed()) + vm := &vmopv1.VirtualMachine{} + Expect(ctx.Client.Get(ctx, client.ObjectKeyFromObject(ctx.vm), vm)).To(Succeed()) + Expect(vm.Spec.ImageName).To(Equal(ctx.vm.Spec.Cdrom[0].Image.Name)) + }) + }) + }) + + When("updating a VM", func() { + + var imgNameToOldKind map[string]string + + BeforeEach(func() { + Expect(ctx.Client.Create(ctx, ctx.vm)).To(Succeed()) + imgNameToOldKind = make(map[string]string, len(ctx.vm.Spec.Cdrom)) + }) + + When("spec.cdrom.image.kind is reset", func() { + + BeforeEach(func() { + for i, c := range ctx.vm.Spec.Cdrom { + imgNameToOldKind[c.Image.Name] = c.Image.Kind + ctx.vm.Spec.Cdrom[i].Image.Kind = "" + } + }) + + It("should repopulate the default kind only if it was previously set to default", func() { + Expect(ctx.Client.Update(ctx, ctx.vm)).To(Succeed()) + vm := &vmopv1.VirtualMachine{} + Expect(ctx.Client.Get(ctx, client.ObjectKeyFromObject(ctx.vm), vm)).To(Succeed()) + defaultKind := "VirtualMachineImage" + for _, c := range vm.Spec.Cdrom { + if imgNameToOldKind[c.Image.Name] == defaultKind { + Expect(c.Image.Kind).To(Equal(defaultKind)) + } else { + Expect(c.Image.Kind).To(BeEmpty()) + } + } + }) + }) + }) + }) } diff --git a/webhooks/virtualmachine/mutation/virtualmachine_mutator_unit_test.go b/webhooks/virtualmachine/mutation/virtualmachine_mutator_unit_test.go index 995e2c2bd..e41678143 100644 --- a/webhooks/virtualmachine/mutation/virtualmachine_mutator_unit_test.go +++ b/webhooks/virtualmachine/mutation/virtualmachine_mutator_unit_test.go @@ -24,6 +24,7 @@ import ( "github.com/vmware-tanzu/vm-operator/pkg/constants" "github.com/vmware-tanzu/vm-operator/pkg/constants/testlabels" "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/config" + "github.com/vmware-tanzu/vm-operator/pkg/util/ptr" vmopv1util "github.com/vmware-tanzu/vm-operator/pkg/util/vmopv1" "github.com/vmware-tanzu/vm-operator/test/builder" "github.com/vmware-tanzu/vm-operator/webhooks/virtualmachine/mutation" @@ -1156,16 +1157,18 @@ func unitTestsMutating() { }) }) - Describe("SetDefaultCdromNameAndImgKind", func() { + Describe("SetDefaultCdromImgKindOnCreate", func() { BeforeEach(func() { ctx.vm.Spec.Cdrom = []vmopv1.VirtualMachineCdromSpec{ { + Name: "cdrom1", Image: vmopv1.VirtualMachineImageRef{ Name: "vmi-1", }, }, { + Name: "cdrom2", Image: vmopv1.VirtualMachineImageRef{ Name: "vmi-2", }, @@ -1173,15 +1176,46 @@ func unitTestsMutating() { } }) - It("should set the default cdrom name and image kind", func() { - mutation.SetDefaultCdromNameAndImgKind(&ctx.WebhookRequestContext, ctx.vm) - Expect(ctx.vm.Spec.Cdrom[0].Name).To(Equal("cdrom1")) + It("should set the default image kind as VirtualMachineImage", func() { + mutation.SetDefaultCdromImgKindOnCreate(&ctx.WebhookRequestContext, ctx.vm) Expect(ctx.vm.Spec.Cdrom[0].Image.Kind).To(Equal("VirtualMachineImage")) - Expect(ctx.vm.Spec.Cdrom[1].Name).To(Equal("cdrom2")) Expect(ctx.vm.Spec.Cdrom[1].Image.Kind).To(Equal("VirtualMachineImage")) }) }) + Describe("SetDefaultCdromImgKindOnUpdate", func() { + + var oldVM *vmopv1.VirtualMachine + + BeforeEach(func() { + ctx.vm.Spec.Cdrom = []vmopv1.VirtualMachineCdromSpec{ + { + Name: "cdrom1", + Image: vmopv1.VirtualMachineImageRef{ + Name: "vmi-1", + Kind: "VirtualMachineImage", + }, + }, + { + Name: "cdrom2", + Image: vmopv1.VirtualMachineImageRef{ + Name: "vmi-2", + Kind: "ClusterVirtualMachineImage", + }, + }, + } + oldVM = ctx.vm.DeepCopy() + ctx.vm.Spec.Cdrom[0].Image.Kind = "" + ctx.vm.Spec.Cdrom[1].Image.Kind = "" + }) + + It("should set the default image kind if previously set to default", func() { + mutation.SetDefaultCdromImgKindOnUpdate(&ctx.WebhookRequestContext, ctx.vm, oldVM) + Expect(ctx.vm.Spec.Cdrom[0].Image.Kind).To(Equal("VirtualMachineImage")) + Expect(ctx.vm.Spec.Cdrom[1].Image.Kind).To(BeEmpty()) + }) + }) + Describe("SetImageNameFromCdrom", func() { BeforeEach(func() { @@ -1207,15 +1241,20 @@ func unitTestsMutating() { }) It("should set to the first connected CD-ROM if multiple exist", func() { + // Update the existing CD-ROM to be disconnected to ensure the first + // connected CD-ROM's image name is used. + for i := range ctx.vm.Spec.Cdrom { + ctx.vm.Spec.Cdrom[i].Connected = ptr.To(false) + } ctx.vm.Spec.Cdrom = append(ctx.vm.Spec.Cdrom, vmopv1.VirtualMachineCdromSpec{ Image: vmopv1.VirtualMachineImageRef{ - Name: "vmi-cdrom2", + Name: "vmi-new", }, - Connected: true, + Connected: ptr.To(true), }) ctx.vm.Spec.ImageName = "" mutation.SetImageNameFromCdrom(&ctx.WebhookRequestContext, ctx.vm) - Expect(ctx.vm.Spec.ImageName).To(Equal("vmi-cdrom2")) + Expect(ctx.vm.Spec.ImageName).To(Equal("vmi-new")) }) }) } diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator.go b/webhooks/virtualmachine/validation/virtualmachine_validator.go index 9397f4909..253ed773a 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator.go +++ b/webhooks/virtualmachine/validation/virtualmachine_validator.go @@ -1305,29 +1305,26 @@ func (v *validator) validateCdrom( return append(allErrs, field.Forbidden(f, fmt.Sprintf(featureNotEnabled, "CD-ROM (ISO) support"))) } - var ( - vmiNames = make(map[string]struct{}, len(vm.Spec.Cdrom)) - cvmiNames = make(map[string]struct{}, len(vm.Spec.Cdrom)) - ) + // GuestID must be set when deploying an ISO VM with CD-ROMs. + if vm.Spec.GuestID == "" { + allErrs = append(allErrs, field.Required(field.NewPath("spec", "guestID"), "when deploying a VM with CD-ROMs")) + } + // Validate image kind and uniqueness for each CD-ROM. + // Namespace and cluster scope images with the same name are considered + // duplicates since they come from the same content library item file. + imgNames := make(map[string]struct{}, len(vm.Spec.Cdrom)) for i, c := range vm.Spec.Cdrom { imgPath := f.Index(i).Child("image") + imgKind := c.Image.Kind + if imgKind != vmiKind && imgKind != cvmiKind { + allErrs = append(allErrs, field.NotSupported(imgPath.Child("kind"), imgKind, []string{vmiKind, cvmiKind})) + } imgName := c.Image.Name - switch c.Image.Kind { - case vmiKind: - if _, ok := vmiNames[imgName]; ok { - allErrs = append(allErrs, field.Duplicate(imgPath, imgName)) - } else { - vmiNames[imgName] = struct{}{} - } - case cvmiKind: - if _, ok := cvmiNames[imgName]; ok { - allErrs = append(allErrs, field.Duplicate(imgPath, imgName)) - } else { - cvmiNames[imgName] = struct{}{} - } - default: - allErrs = append(allErrs, field.NotSupported(imgPath.Child("kind"), c.Image.Kind, []string{vmiKind, cvmiKind})) + if _, ok := imgNames[imgName]; ok { + allErrs = append(allErrs, field.Duplicate(imgPath.Child("name"), imgName)) + } else { + imgNames[imgName] = struct{}{} } } diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator_intg_test.go b/webhooks/virtualmachine/validation/virtualmachine_validator_intg_test.go index e99bb4277..c3c7f60ee 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator_intg_test.go +++ b/webhooks/virtualmachine/validation/virtualmachine_validator_intg_test.go @@ -74,13 +74,6 @@ func newIntgValidatingWebhookContext() *intgValidatingWebhookContext { func intgTestsValidateCreate() { - const ( - invalid = "invalid" - vmiKind = "VirtualMachineImage" - cvmiKind = "Cluster" + vmiKind - invalidImageKind = "supported: " + vmiKind + "; " + cvmiKind - ) - var ( ctx *intgValidatingWebhookContext ) @@ -99,7 +92,7 @@ func intgTestsValidateCreate() { ctx.vm.Spec.Image.Kind = "" } if args.invalidImageKind { - ctx.vm.Spec.Image.Kind = invalid + ctx.vm.Spec.Image.Kind = invalidKind } err := ctx.Client.Create(ctx, ctx.vm) @@ -129,15 +122,17 @@ func intgTestsValidateCreate() { Entry("should not work for empty image", createArgs{emptyImage: true}, false, field.Required(specPath.Child("image"), "").Error(), nil), Entry("should not work for empty image kind", createArgs{emptyImageKind: true}, false, - field.Required(specPath.Child("image").Child("kind"), invalidImageKind).Error(), nil), + field.Required(specPath.Child("image").Child("kind"), invalidImageKindMsg).Error(), nil), Entry("should not work for invalid image kind", createArgs{invalidImageKind: true}, false, - field.Invalid(specPath.Child("image").Child("kind"), invalid, invalidImageKind).Error(), nil), + field.Invalid(specPath.Child("image").Child("kind"), invalidKind, invalidImageKindMsg).Error(), nil), ) } func intgTestsValidateUpdate() { const ( immutableFieldMsg = "field is immutable" + unsupportedValMsg = "Unsupported value" + duplicateValMsg = "Duplicate value" ) var ( @@ -232,6 +227,31 @@ func intgTestsValidateUpdate() { }) }) + When("update is performed with changed cd-rom", func() { + When("cd-rom image kind is invalid", func() { + BeforeEach(func() { + ctx.vm.Spec.Cdrom[0].Image.Kind = invalidKind + }) + It("should deny the request", func() { + Expect(err).To(HaveOccurred()) + expectedPath := field.NewPath("spec", "cdrom[0]", "image", "kind") + Expect(err.Error()).To(ContainSubstring(expectedPath.String())) + Expect(err.Error()).To(ContainSubstring(unsupportedValMsg)) + }) + }) + When("cd-rom image name is duplicate with others", func() { + BeforeEach(func() { + ctx.vm.Spec.Cdrom[0].Image.Name = ctx.vm.Spec.Cdrom[1].Image.Name + }) + It("should deny the request", func() { + Expect(err).To(HaveOccurred()) + expectedPath := field.NewPath("spec", "cdrom[1]", "image", "name") + Expect(err.Error()).To(ContainSubstring(expectedPath.String())) + Expect(err.Error()).To(ContainSubstring(duplicateValMsg)) + }) + }) + }) + Context("VirtualMachine update while VM is powered on", func() { BeforeEach(func() { ctx.vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn @@ -306,6 +326,33 @@ func intgTestsValidateUpdate() { Expect(err.Error()).To(ContainSubstring(expectedReason)) }) }) + + When("CD-ROM name is updated", func() { + BeforeEach(func() { + // Name can only consist of lowercase letters and digits. + ctx.vm.Spec.Cdrom[0].Name = "dummy2" + }) + + It("rejects the request", func() { + expectedReason := field.Forbidden(field.NewPath("spec", "cdrom[0]", "name"), + "updates to this field is not allowed when VM power is on").Error() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expectedReason)) + }) + }) + + When("CD-ROM image ref is updated", func() { + BeforeEach(func() { + ctx.vm.Spec.Cdrom[0].Image.Name = "dummy-new-image-name" + }) + + It("rejects the request", func() { + expectedReason := field.Forbidden(field.NewPath("spec", "cdrom[0]", "image"), + "updates to this field is not allowed when VM power is on").Error() + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expectedReason)) + }) + }) }) } diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go b/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go index 6d20f4e82..d88b7e9c7 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go +++ b/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go @@ -46,7 +46,8 @@ const ( dummyNamespaceName = "dummy-vm-namespace-for-webhook-validation" vmiKind = "VirtualMachineImage" cvmiKind = "Cluster" + vmiKind - invalidImageKind = "supported: " + vmiKind + "; " + cvmiKind + invalidKind = "InvalidKind" + invalidImageKindMsg = "supported: " + vmiKind + "; " + cvmiKind ) type testParams struct { @@ -464,7 +465,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKind).Error(), + field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKindMsg).Error(), ), }, ), @@ -479,7 +480,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKind).Error(), + field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKindMsg).Error(), ), }, ), @@ -496,7 +497,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKind).Error(), + field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKindMsg).Error(), ), }, ), @@ -513,7 +514,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKind).Error(), + field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKindMsg).Error(), ), }, ), @@ -545,7 +546,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKind).Error(), + field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKindMsg).Error(), ), }, ), @@ -575,7 +576,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKind).Error(), + field.Required(field.NewPath("spec", "image").Child("kind"), invalidImageKindMsg).Error(), ), }, ), @@ -592,7 +593,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKind).Error(), + field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKindMsg).Error(), ), }, ), @@ -609,7 +610,7 @@ func unitTestsValidateCreate() { }) }, validate: doValidateWithMsg( - field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKind).Error(), + field.Invalid(field.NewPath("spec", "image").Child("kind"), "invalid", invalidImageKindMsg).Error(), ), }, ), @@ -2046,6 +2047,18 @@ func unitTestsValidateCreate() { }, ), + Entry("disallow creating a VM with CD-ROM and empty guest ID", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.vm.Spec.GuestID = "" + }, + validate: doValidateWithMsg( + `spec.guestID: Required value: when deploying a VM with CD-ROMs`, + ), + expectAllowed: false, + }, + ), + Entry("disallow creating a VM with invalid CD-ROM image ref kind", testParams{ setup: func(ctx *unitValidatingWebhookContext) { @@ -2082,7 +2095,7 @@ func unitTestsValidateCreate() { ctx.vm.Spec.Cdrom = append(ctx.vm.Spec.Cdrom, ctx.vm.Spec.Cdrom[0]) }, validate: doValidateWithMsg( - `spec.cdrom[1].image: Duplicate value: "vmi-dummy"`, + `spec.cdrom[1].image.name: Duplicate value: "vmi-dummy"`, ), expectAllowed: false, }, @@ -2103,7 +2116,33 @@ func unitTestsValidateCreate() { ctx.vm.Spec.Cdrom = append(ctx.vm.Spec.Cdrom, ctx.vm.Spec.Cdrom[0]) }, validate: doValidateWithMsg( - `spec.cdrom[1].image: Duplicate value: "vmi-dummy"`), + `spec.cdrom[1].image.name: Duplicate value: "vmi-dummy"`), + expectAllowed: false, + }, + ), + + Entry("disallow creating a VM with duplicate CD-ROM image ref from VMI and CVMI kinds", + testParams{ + setup: func(ctx *unitValidatingWebhookContext) { + ctx.vm.Spec.Cdrom = []vmopv1.VirtualMachineCdromSpec{ + { + Name: "cdromDupVmi", + Image: vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName, + Kind: vmiKind, + }, + }, + { + Name: "cdromDupCvmi", + Image: vmopv1.VirtualMachineImageRef{ + Name: dummyVmiName, + Kind: cvmiKind, + }, + }, + } + }, + validate: doValidateWithMsg( + `spec.cdrom[1].image.name: Duplicate value: "vmi-dummy"`), expectAllowed: false, }, ), @@ -2908,7 +2947,7 @@ func unitTestsValidateUpdate() { testParams{ setup: func(ctx *unitValidatingWebhookContext) { ctx.vm.Spec.Cdrom = append(ctx.vm.Spec.Cdrom, vmopv1.VirtualMachineCdromSpec{ - Name: "cdromNew", + Name: "new", Image: vmopv1.VirtualMachineImageRef{ Name: "vmi-new", Kind: vmiKind, @@ -2924,7 +2963,7 @@ func unitTestsValidateUpdate() { testParams{ setup: func(ctx *unitValidatingWebhookContext) { ctx.vm.Spec.Cdrom = append(ctx.vm.Spec.Cdrom, vmopv1.VirtualMachineCdromSpec{ - Name: "cdromNew", + Name: "new2", Image: vmopv1.VirtualMachineImageRef{ Name: "vmi-new", Kind: vmiKind, @@ -2975,7 +3014,7 @@ func unitTestsValidateUpdate() { Entry("disallow changing CD-ROM name when VM is powered on", testParams{ setup: func(ctx *unitValidatingWebhookContext) { - ctx.vm.Spec.Cdrom[0].Name = "cdromNew" + ctx.vm.Spec.Cdrom[0].Name = "new3" ctx.vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn }, validate: doValidateWithMsg( @@ -3017,8 +3056,10 @@ func unitTestsValidateUpdate() { Entry("allow changing CD-ROM connection when VM is powered on", testParams{ setup: func(ctx *unitValidatingWebhookContext) { - ctx.vm.Spec.Cdrom[0].Connected = !ctx.vm.Spec.Cdrom[0].Connected - ctx.vm.Spec.Cdrom[0].AllowGuestControl = !ctx.vm.Spec.Cdrom[0].AllowGuestControl + oldConnected := ptr.Deref(ctx.oldVM.Spec.Cdrom[0].Connected) + oldAllowGuestControl := ptr.Deref(ctx.oldVM.Spec.Cdrom[0].AllowGuestControl) + ctx.vm.Spec.Cdrom[0].Connected = ptr.To(!oldConnected) + ctx.vm.Spec.Cdrom[0].AllowGuestControl = ptr.To(!oldAllowGuestControl) ctx.vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOn }, expectAllowed: true, @@ -3028,8 +3069,10 @@ func unitTestsValidateUpdate() { Entry("allow changing CD-ROM connection when VM is powered off", testParams{ setup: func(ctx *unitValidatingWebhookContext) { - ctx.vm.Spec.Cdrom[0].Connected = !ctx.vm.Spec.Cdrom[0].Connected - ctx.vm.Spec.Cdrom[0].AllowGuestControl = !ctx.vm.Spec.Cdrom[0].AllowGuestControl + oldConnected := ptr.Deref(ctx.oldVM.Spec.Cdrom[0].Connected) + oldAllowGuestControl := ptr.Deref(ctx.oldVM.Spec.Cdrom[0].AllowGuestControl) + ctx.vm.Spec.Cdrom[0].Connected = ptr.To(!oldConnected) + ctx.vm.Spec.Cdrom[0].AllowGuestControl = ptr.To(!oldAllowGuestControl) ctx.vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff }, expectAllowed: true, @@ -3058,7 +3101,7 @@ func unitTestsValidateUpdate() { ctx.vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff }, validate: doValidateWithMsg( - `spec.cdrom[1].image: Duplicate value: "vmi-0123456789"`, + `spec.cdrom[1].image.name: Duplicate value: "vmi-0123456789"`, ), expectAllowed: false, },