Update VM API spec.bootstrap.sysprep fields

This patch includes several changes to the VirtualMachine API's sysprep bootstrap provider, including: * The field `spec.bootstrap.sysprep.userData` is now required. This is only true if `spec.bootstrap.sysprep` is set, so it does not prevent someone from omitting a bootstrap provider. This is aligned with the VMODL used by VM Operator to customize the guest. * The sysprep bootstrap provider now defaults to the UTC timezone by setting the default value of the field `spec.bootstrap.sysprep.guiUnattended.timeZone` to 85. This is because the VMODL says guiUnattended is required (https://dp-downloads.broadcom.com/api-content/apis/API_VWSA_001/8.0U3/html/ReferenceGuides/vim.vm.customization.Sysprep.html), but we got away with it being optional in VM Op since all the contained fields default to their primitive type defaults. Except timeZone defaulted to 0, which s GMT-12 per the reference (https://learn.microsoft.com/en-us/previous-versions/windows/embedded/ms912391(v=winembedded.11)), which is not a sane default, whereas UTC is.
vmware-tanzu · Nov 1, 2024 · 3eab5a0 · 3eab5a0
1 parent 2840635
commit 3eab5a0
Show file tree

Hide file tree

Showing 13 changed files with 113 additions and 42 deletions.
diff --git a/api/v1alpha1/virtualmachine_conversion_test.go b/api/v1alpha1/virtualmachine_conversion_test.go
@@ -42,6 +42,19 @@ func TestVirtualMachineConversion(t *testing.T) {
 		g.Expect(apiequality.Semantic.DeepEqual(hubBefore, hubAfter)).To(BeTrue(), cmp.Diff(hubBefore, hubAfter))
 	}
 
+	hubSpokeHubEx := func(g *WithT, hub, hubAfter ctrlconversion.Hub, spoke ctrlconversion.Convertible) {
+		hubBefore := hub.DeepCopyObject().(ctrlconversion.Hub)
+
+		// First convert hub to spoke
+		dstCopy := spoke.DeepCopyObject().(ctrlconversion.Convertible)
+		g.Expect(dstCopy.ConvertFrom(hubBefore)).To(Succeed())
+
+		// Convert spoke back to hub and check if the resulting hub is equal to the hub before the round trip
+		g.Expect(dstCopy.ConvertTo(hubAfter)).To(Succeed())
+
+		g.Expect(apiequality.Semantic.DeepEqual(hubBefore, hubAfter)).To(BeTrue(), cmp.Diff(hubBefore, hubAfter))
+	}
+
 	spokeHubSpoke := func(g *WithT, spoke ctrlconversion.Convertible, hub ctrlconversion.Hub) {
 		spokeBefore := spoke.DeepCopyObject().(ctrlconversion.Convertible)
 
@@ -375,7 +388,7 @@ func TestVirtualMachineConversion(t *testing.T) {
 							Identification: &vmopv1sysprep.Identification{
 								DomainAdmin: "my-admin",
 							},
-							UserData: &vmopv1sysprep.UserData{
+							UserData: vmopv1sysprep.UserData{
 								FullName: "vmware",
 							},
 						},
@@ -387,6 +400,31 @@ func TestVirtualMachineConversion(t *testing.T) {
 		hubSpokeHub(g, &hub, &vmopv1a1.VirtualMachine{})
 	})
 
+	t.Run("VirtualMachine and partial inline sysprep", func(t *testing.T) {
+		t.Run("hub-spoke-hub", func(t *testing.T) {
+			g := NewWithT(t)
+			hub := vmopv1.VirtualMachine{
+				Spec: vmopv1.VirtualMachineSpec{
+					Bootstrap: &vmopv1.VirtualMachineBootstrapSpec{
+						Sysprep: &vmopv1.VirtualMachineBootstrapSysprepSpec{
+							Sysprep: &vmopv1sysprep.Sysprep{
+								UserData: vmopv1sysprep.UserData{
+									FullName: "test-user",
+									OrgName:  "test-org",
+									ProductID: &vmopv1sysprep.ProductIDSecretKeySelector{
+										Key:  "product_id",
+										Name: "vm-w9w4mn9xr2",
+									},
+								},
+							},
+						},
+					},
+				},
+			}
+			hubSpokeHubEx(g, &hub, &vmopv1.VirtualMachine{}, &vmopv1a1.VirtualMachine{})
+		})
+	})
+
 	t.Run("VirtualMachine hub-spoke-hub with Sysprep and vAppConfig", func(t *testing.T) {
 		g := NewWithT(t)
 

diff --git a/api/v1alpha2/sysprep/conversion/v1alpha2/sysprep_conversion.go b/api/v1alpha2/sysprep/conversion/v1alpha2/sysprep_conversion.go
@@ -23,7 +23,11 @@ func Convert_sysprep_Sysprep_To_sysprep_Sysprep(
 	}
 	out.GUIUnattended = (*vmopv1sysprep.GUIUnattended)(unsafe.Pointer(in.GUIUnattended))
 	out.LicenseFilePrintData = (*vmopv1sysprep.LicenseFilePrintData)(unsafe.Pointer(in.LicenseFilePrintData))
-	out.UserData = (*vmopv1sysprep.UserData)(unsafe.Pointer(in.UserData))
+	if in.UserData != nil {
+		out.UserData.FullName = in.UserData.FullName
+		out.UserData.OrgName = in.UserData.OrgName
+		out.UserData.ProductID = (*vmopv1sysprep.ProductIDSecretKeySelector)(unsafe.Pointer(in.UserData.ProductID))
+	}
 	if id := in.Identification; id != nil {
 		out.Identification = &vmopv1sysprep.Identification{
 			DomainAdmin:         id.DomainAdmin,

diff --git a/api/v1alpha2/sysprep/conversion/v1alpha3/sysprep_conversion.go b/api/v1alpha2/sysprep/conversion/v1alpha3/sysprep_conversion.go
@@ -22,7 +22,11 @@ func Convert_sysprep_Sysprep_To_sysprep_Sysprep(
 	}
 	out.GUIUnattended = (*vmopv1a2sysprep.GUIUnattended)(unsafe.Pointer(in.GUIUnattended))
 	out.LicenseFilePrintData = (*vmopv1a2sysprep.LicenseFilePrintData)(unsafe.Pointer(in.LicenseFilePrintData))
-	out.UserData = (*vmopv1a2sysprep.UserData)(unsafe.Pointer(in.UserData))
+	out.UserData = &vmopv1a2sysprep.UserData{
+		FullName:  in.UserData.FullName,
+		OrgName:   in.UserData.OrgName,
+		ProductID: (*vmopv1a2sysprep.ProductIDSecretKeySelector)(unsafe.Pointer(in.UserData.ProductID)),
+	}
 	if id := in.Identification; id != nil {
 		out.Identification = &vmopv1a2sysprep.Identification{
 			DomainAdmin:         id.DomainAdmin,

diff --git a/api/v1alpha2/virtualmachine_conversion_test.go b/api/v1alpha2/virtualmachine_conversion_test.go
@@ -84,7 +84,7 @@ func TestVirtualMachineConversion(t *testing.T) {
 							Identification: &vmopv1sysprep.Identification{
 								DomainAdmin: "my-admin",
 							},
-							UserData: &vmopv1sysprep.UserData{
+							UserData: vmopv1sysprep.UserData{
 								FullName: "vmware",
 							},
 						},
@@ -591,7 +591,7 @@ func TestVirtualMachineConversion(t *testing.T) {
 					Bootstrap: &vmopv1.VirtualMachineBootstrapSpec{
 						Sysprep: &vmopv1.VirtualMachineBootstrapSysprepSpec{
 							Sysprep: &vmopv1sysprep.Sysprep{
-								UserData: &vmopv1sysprep.UserData{
+								UserData: vmopv1sysprep.UserData{
 									FullName: "test-user",
 									OrgName:  "test-org",
 									ProductID: &vmopv1sysprep.ProductIDSecretKeySelector{

diff --git a/api/v1alpha3/sysprep/sysprep.go b/api/v1alpha3/sysprep/sysprep.go
@@ -39,10 +39,8 @@ type Sysprep struct {
 	// Server 2003.
 	LicenseFilePrintData *LicenseFilePrintData `json:"licenseFilePrintData,omitempty"`
 
-	// +optional
-
 	// UserData is a representation of the Sysprep UserData key.
-	UserData *UserData `json:"userData,omitempty"`
+	UserData UserData `json:"userData"`
 }
 
 // GUIRunOnce maps to the GuiRunOnce key in the sysprep.xml answer file.
@@ -98,13 +96,16 @@ type GUIUnattended struct {
 	// `password`.
 	Password *PasswordSecretKeySelector `json:"password,omitempty"`
 
-	// +optional
+	// +kubebuilder:default=85
+	// +kubebuilder:validation:Minimum=0
 
 	// TimeZone is the time zone index for the virtual machine.
 	//
 	// Please note that numbers correspond to time zones listed at
 	// https://bit.ly/3Rzv8oL.
-	TimeZone int32 `json:"timeZone,omitempty"`
+	//
+	// Defaults to UTC.
+	TimeZone int32 `json:"timeZone"`
 }
 
 // PasswordSecretKeySelector references the password value from a Secret resource

diff --git a/api/v1alpha3/sysprep/zz_generated.deepcopy.go b/api/v1alpha3/sysprep/zz_generated.deepcopy.go
diff --git a/config/crd/bases/vmoperator.vmware.com_virtualmachinereplicasets.yaml b/config/crd/bases/vmoperator.vmware.com_virtualmachinereplicasets.yaml
@@ -717,13 +717,19 @@ spec:
                                         - name
                                         type: object
                                       timeZone:
+                                        default: 85
                                         description: |-
                                           TimeZone is the time zone index for the virtual machine.
 
                                           Please note that numbers correspond to time zones listed at
                                           https://bit.ly/3Rzv8oL.
+
+                                          Defaults to UTC.
                                         format: int32
+                                        minimum: 0
                                         type: integer
+                                    required:
+                                    - timeZone
                                     type: object
                                   identification:
                                     description: Identification is a representation
@@ -827,6 +833,8 @@ spec:
                                     - fullName
                                     - orgName
                                     type: object
+                                required:
+                                - userData
                                 type: object
                             type: object
                           vAppConfig:

diff --git a/config/crd/bases/vmoperator.vmware.com_virtualmachines.yaml b/config/crd/bases/vmoperator.vmware.com_virtualmachines.yaml
@@ -3504,13 +3504,19 @@ spec:
                                 - name
                                 type: object
                               timeZone:
+                                default: 85
                                 description: |-
                                   TimeZone is the time zone index for the virtual machine.
 
                                   Please note that numbers correspond to time zones listed at
                                   https://bit.ly/3Rzv8oL.
+
+                                  Defaults to UTC.
                                 format: int32
+                                minimum: 0
                                 type: integer
+                            required:
+                            - timeZone
                             type: object
                           identification:
                             description: Identification is a representation of the
@@ -3613,6 +3619,8 @@ spec:
                             - fullName
                             - orgName
                             type: object
+                        required:
+                        - userData
                         type: object
                     type: object
                   vAppConfig:

diff --git a/pkg/providers/vsphere/sysprep/secret.go b/pkg/providers/vsphere/sysprep/secret.go
@@ -26,10 +26,16 @@ func GetSysprepSecretData(
 		productID, password, domainPwd string
 	)
 
-	if userData := in.UserData; userData != nil && userData.ProductID != nil {
+	if in.UserData.ProductID != nil {
 		// this is an optional secret key selector even when FullName or OrgName are set.
-		err := util.GetSecretData(ctx, k8sClient, secretNamespace, userData.ProductID.Name, userData.ProductID.Key, &productID)
-		if err != nil {
+		if err := util.GetSecretData(
+			ctx,
+			k8sClient,
+			secretNamespace,
+			in.UserData.ProductID.Name,
+			in.UserData.ProductID.Key,
+			&productID); err != nil {
+
 			return SecretData{}, err
 		}
 	}
@@ -84,16 +90,16 @@ func GetSecretResources(
 		}
 	}
 
-	if userData := in.UserData; userData != nil && userData.ProductID != nil {
+	if in.UserData.ProductID != nil {
 		s, err := util.GetSecretResource(
 			ctx,
 			k8sClient,
 			secretNamespace,
-			userData.ProductID.Name)
+			in.UserData.ProductID.Name)
 		if err != nil {
 			return nil, err
 		}
-		captureSecret(s, userData.ProductID.Name)
+		captureSecret(s, in.UserData.ProductID.Name)
 	}
 
 	if guiUnattended := in.GUIUnattended; guiUnattended != nil && guiUnattended.AutoLogon {

diff --git a/pkg/providers/vsphere/sysprep/secret_test.go b/pkg/providers/vsphere/sysprep/secret_test.go
@@ -53,7 +53,7 @@ var _ = Describe("GetSysprepSecretData", func() {
 		productIDSecretName := "product_id_secret"
 		BeforeEach(func() {
 			inlineSysprep = vmopv1sysprep.Sysprep{
-				UserData: &vmopv1sysprep.UserData{
+				UserData: vmopv1sysprep.UserData{
 					ProductID: &vmopv1sysprep.ProductIDSecretKeySelector{
 						Name: productIDSecretName,
 						Key:  "product_id",
@@ -249,7 +249,7 @@ var _ = Describe("Sysprep GetSecretResources", func() {
 		productIDSecretName := "product_id_secret"
 		BeforeEach(func() {
 			inlineSysprep = vmopv1sysprep.Sysprep{
-				UserData: &vmopv1sysprep.UserData{
+				UserData: vmopv1sysprep.UserData{
 					ProductID: &vmopv1sysprep.ProductIDSecretKeySelector{
 						Name: productIDSecretName,
 						Key:  "product_id",
@@ -375,7 +375,7 @@ var _ = Describe("Sysprep GetSecretResources", func() {
 		secretName := "same-secret"
 		BeforeEach(func() {
 			inlineSysprep = vmopv1sysprep.Sysprep{
-				UserData: &vmopv1sysprep.UserData{
+				UserData: vmopv1sysprep.UserData{
 					ProductID: &vmopv1sysprep.ProductIDSecretKeySelector{
 						Name: secretName,
 						Key:  "product_id",

diff --git a/pkg/providers/vsphere/vmlifecycle/bootstrap_sysprep.go b/pkg/providers/vsphere/vmlifecycle/bootstrap_sysprep.go
@@ -91,12 +91,18 @@ func convertTo(from *vmopv1sysprep.Sysprep, bsArgs *BootstrapArgs) *vimtypes.Cus
 	bootstrapData := bsArgs.BootstrapData
 	sysprepCustomization := &vimtypes.CustomizationSysprep{}
 
-	if from.GUIUnattended != nil {
-		sysprepCustomization.GuiUnattended = vimtypes.CustomizationGuiUnattended{
-			TimeZone:       from.GUIUnattended.TimeZone,
-			AutoLogon:      from.GUIUnattended.AutoLogon,
-			AutoLogonCount: from.GUIUnattended.AutoLogonCount,
-		}
+	sysprepCustomization.GuiUnattended = vimtypes.CustomizationGuiUnattended{}
+
+	if from.GUIUnattended == nil {
+		// If spec.bootstrap.sysprep.guiUnattended is not set, then default
+		// the timezone to UTC, which is 85 per the Microsoft documentation at
+		// https://learn.microsoft.com/en-us/previous-versions/windows/embedded/ms912391(v=winembedded.11).
+		sysprepCustomization.GuiUnattended.TimeZone = 85
+	} else {
+		sysprepCustomization.GuiUnattended.TimeZone = from.GUIUnattended.TimeZone
+		sysprepCustomization.GuiUnattended.AutoLogon = from.GUIUnattended.AutoLogon
+		sysprepCustomization.GuiUnattended.AutoLogonCount = from.GUIUnattended.AutoLogonCount
+
 		if bootstrapData.Sysprep != nil && bootstrapData.Sysprep.Password != "" {
 			sysprepCustomization.GuiUnattended.Password = &vimtypes.CustomizationPassword{
 				Value:     bootstrapData.Sysprep.Password,
@@ -111,14 +117,12 @@ func convertTo(from *vmopv1sysprep.Sysprep, bsArgs *BootstrapArgs) *vimtypes.Cus
 			Name: bsArgs.HostName,
 		},
 	}
-	if from.UserData != nil {
-		sysprepCustomization.UserData.FullName = from.UserData.FullName
-		sysprepCustomization.UserData.OrgName = from.UserData.OrgName
-		// In the case of a VMI with volume license key, this might not be set.
-		// Hence, add a check to see if the productID is set to empty.
-		if bootstrapData.Sysprep != nil && bootstrapData.Sysprep.ProductID != "" {
-			sysprepCustomization.UserData.ProductId = bootstrapData.Sysprep.ProductID
-		}
+	sysprepCustomization.UserData.FullName = from.UserData.FullName
+	sysprepCustomization.UserData.OrgName = from.UserData.OrgName
+	// In the case of a VMI with volume license key, this might not be set.
+	// Hence, add a check to see if the productID is set to empty.
+	if bootstrapData.Sysprep != nil && bootstrapData.Sysprep.ProductID != "" {
+		sysprepCustomization.UserData.ProductId = bootstrapData.Sysprep.ProductID
 	}
 
 	if from.GUIRunOnce != nil {

diff --git a/pkg/providers/vsphere/vmlifecycle/bootstrap_sysprep_test.go b/pkg/providers/vsphere/vmlifecycle/bootstrap_sysprep_test.go
@@ -122,7 +122,7 @@ var _ = Describe("SysPrep Bootstrap", func() {
 						},
 						TimeZone: 4,
 					},
-					UserData: &vmopv1sysprep.UserData{
+					UserData: vmopv1sysprep.UserData{
 						FullName:  "foo-bar",
 						OrgName:   "foo-org",
 						ProductID: &vmopv1sysprep.ProductIDSecretKeySelector{Key: "product_id_key"},
@@ -188,7 +188,7 @@ var _ = Describe("SysPrep Bootstrap", func() {
 					bsArgs.Sysprep = nil
 				})
 
-				It("does not set", func() {
+				It("still sets some fields", func() {
 					Expect(err).ToNot(HaveOccurred())
 					Expect(custSpec).ToNot(BeNil())
 
@@ -198,6 +198,8 @@ var _ = Describe("SysPrep Bootstrap", func() {
 					name, ok := sysPrep.UserData.ComputerName.(*vimtypes.CustomizationFixedName)
 					Expect(ok).To(BeTrue())
 					Expect(name.Name).To(Equal(hostName))
+
+					Expect(sysPrep.GuiUnattended.TimeZone).To(Equal(int32(85)))
 				})
 			})
 		})

diff --git a/pkg/providers/vsphere/vmprovider_vm_utils_test.go b/pkg/providers/vsphere/vmprovider_vm_utils_test.go
@@ -606,7 +606,7 @@ func vmUtilTests() {
 				productIDSecretName := "product_id_secret"
 				BeforeEach(func() {
 					vmCtx.VM.Spec.Bootstrap.Sysprep.Sysprep = &sysprep.Sysprep{
-						UserData: &sysprep.UserData{
+						UserData: sysprep.UserData{
 							ProductID: &sysprep.ProductIDSecretKeySelector{
 								Name: productIDSecretName,
 								Key:  "product_id",
@@ -1186,7 +1186,7 @@ func vmUtilTests() {
 				vmCtx.VM.Spec.Bootstrap = &vmopv1.VirtualMachineBootstrapSpec{
 					Sysprep: &vmopv1.VirtualMachineBootstrapSysprepSpec{
 						Sysprep: &sysprep.Sysprep{
-							UserData: &sysprep.UserData{
+							UserData: sysprep.UserData{
 								ProductID: &sysprep.ProductIDSecretKeySelector{
 									Name: "dummy-sysprep-secret",
 								},