From 4c2198d5636c34d1deac1d302dbaeab0893c8650 Mon Sep 17 00:00:00 2001 From: zyiyi Date: Thu, 31 Aug 2023 10:57:18 -0700 Subject: [PATCH] Brownfield fix to pass instanceID to CloudInit --- api/v1alpha1/virtualmachine_types.go | 11 ++++++++++- api/v1alpha2/virtualmachine_types.go | 9 +++++++++ .../session/session_vm_customization.go | 6 ++++++ .../providers/vsphere/vmprovider_vm_test.go | 18 ++---------------- .../validation/virtualmachine_validator.go | 19 +++++++++++++++++++ .../virtualmachine_validator_unit_test.go | 17 +++++++++++++++++ .../validation/virtualmachine_validator.go | 19 +++++++++++++++++++ .../virtualmachine_validator_unit_test.go | 18 ++++++++++++++++++ 8 files changed, 100 insertions(+), 17 deletions(-) diff --git a/api/v1alpha1/virtualmachine_types.go b/api/v1alpha1/virtualmachine_types.go index 6665b9927..f54e66943 100644 --- a/api/v1alpha1/virtualmachine_types.go +++ b/api/v1alpha1/virtualmachine_types.go @@ -80,7 +80,7 @@ const ( const ( // PauseAnnotation is an annotation that can be applied to any VirtualMachine object to prevent VM Operator from - // reconciling the object with the vSphere infrastructure. VM Operator checks the presence of this annotation to + // reconciling the object with the vSphere infrastructure. VM Operator checks the presence of this annotation to // skip the reconcile of a VirtualMachine. // // This can be used when a Virtual Machine needs to be modified out-of-band of VM Operator on the infrastructure @@ -96,6 +96,15 @@ const ( // this annotation to skip adding a default nic. VM Operator won't add default NIC to any existing VMs or new VMs // with VirtualMachineNetworkInterfaces specified. This annotation is not required for such VMs. NoDefaultNicAnnotation = GroupName + "/no-default-nic" + + // InstanceIDAnnotation is an annotation that can be applied to set Cloud-Init metadata Instance ID. + // + // This cannot be set by users. It is for VM Operator to handle corner cases. + // + // In a corner case where a VM first boot failed to bootstrap with Cloud-Init, VM Operator sets Instance ID + // the same with the first boot Instance ID to prevent Cloud-Init from treating this VM as first boot + // due to different Instance ID. This annotation is used in upgrade script. + InstanceIDAnnotation = GroupName + "/cloud-init-instance-id" ) // VirtualMachinePort is unused and can be considered deprecated. diff --git a/api/v1alpha2/virtualmachine_types.go b/api/v1alpha2/virtualmachine_types.go index 837fe4f53..8d40ef6d0 100644 --- a/api/v1alpha2/virtualmachine_types.go +++ b/api/v1alpha2/virtualmachine_types.go @@ -93,6 +93,15 @@ const ( // // The VM will not be reconciled again until this annotation is removed. PauseAnnotation = GroupName + "/pause-reconcile" + + // InstanceIDAnnotation is an annotation that can be applied to set Cloud-Init metadata Instance ID. + // + // This cannot be set by users. It is for VM Operator to handle corner cases. + // + // In a corner case where a VM first boot failed to bootstrap with Cloud-Init, VM Operator sets Instance ID + // the same with the first boot Instance ID to prevent Cloud-Init from treating this VM as first boot + // due to different Instance ID. This annotation is used in upgrade script. + InstanceIDAnnotation = GroupName + "/cloud-init-instance-id" ) // VirtualMachinePowerState defines a VM's desired and observed power states. diff --git a/pkg/vmprovider/providers/vsphere/session/session_vm_customization.go b/pkg/vmprovider/providers/vsphere/session/session_vm_customization.go index c0e652a25..11f544b99 100644 --- a/pkg/vmprovider/providers/vsphere/session/session_vm_customization.go +++ b/pkg/vmprovider/providers/vsphere/session/session_vm_customization.go @@ -105,6 +105,12 @@ func GetCloudInitMetadata(vm *vmopv1.VirtualMachine, Network: netplan, PublicKeys: data["ssh-public-keys"], } + // This is to address a bug when Cloud-Init sans configMap/secret actually got + // customized by LinuxPrep. Use annotation as instanceID to avoid + // different instanceID triggers CloudInit in brownfield scenario. + if value, ok := vm.Annotations[vmopv1.InstanceIDAnnotation]; ok { + metadataObj.InstanceID = value + } metadataBytes, err := yaml.Marshal(metadataObj) if err != nil { diff --git a/pkg/vmprovider/providers/vsphere/vmprovider_vm_test.go b/pkg/vmprovider/providers/vsphere/vmprovider_vm_test.go index b0a6d9479..899ff5015 100644 --- a/pkg/vmprovider/providers/vsphere/vmprovider_vm_test.go +++ b/pkg/vmprovider/providers/vsphere/vmprovider_vm_test.go @@ -1498,7 +1498,7 @@ func vmTests() { Context("Cloudinint Transport without userdata", func() { var ec map[string]interface{} - JustBeforeEach(func() { + It("Metadata data is included in ExtraConfig", func() { vm.Spec.VmMetadata = &vmopv1.VirtualMachineMetadata{ Transport: vmopv1.VirtualMachineMetadataCloudInitTransport, } @@ -1514,13 +1514,6 @@ func vmTests() { ec[val.Key] = val.Value.(string) } } - }) - - AfterEach(func() { - ec = nil - }) - - It("Metadata data is included in ExtraConfig", func() { By("Should include default keys and values", func() { Expect(ec).To(HaveKeyWithValue("disk.enableUUID", "TRUE")) Expect(ec).To(HaveKeyWithValue("vmware.tools.gosc.ignoretoolscheck", "TRUE")) @@ -1537,7 +1530,7 @@ func vmTests() { Context("Cloudinint Transport with userdata", func() { var ec map[string]interface{} - JustBeforeEach(func() { + It("Metadata data is included in ExtraConfig", func() { configMap := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "md-configmap-", @@ -1566,13 +1559,6 @@ func vmTests() { ec[val.Key] = val.Value.(string) } } - }) - - AfterEach(func() { - ec = nil - }) - - It("Metadata data is included in ExtraConfig", func() { By("Should include default keys and values", func() { Expect(ec).To(HaveKeyWithValue("disk.enableUUID", "TRUE")) Expect(ec).To(HaveKeyWithValue("vmware.tools.gosc.ignoretoolscheck", "TRUE")) diff --git a/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go b/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go index ccadda538..0e435fd91 100644 --- a/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go +++ b/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go @@ -64,6 +64,7 @@ const ( invalidNextRestartTimeOnCreate = "cannot restart VM on create" invalidNextRestartTimeOnUpdate = "must be formatted as RFC3339Nano" invalidNextRestartTimeOnUpdateNow = "mutation webhooks are required to restart VM" + settingAnnotationNotAllowed = "adding this annotation is not allowed" ) // +kubebuilder:webhook:verbs=create;update,path=/default-validate-vmoperator-vmware-com-v1alpha1-virtualmachine,mutating=false,failurePolicy=fail,groups=vmoperator.vmware.com,resources=virtualmachines,versions=v1alpha1,name=default.validating.virtualmachine.vmoperator.vmware.com,sideEffects=None,admissionReviewVersions=v1;v1beta1 @@ -119,6 +120,7 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission. fieldErrs = append(fieldErrs, v.validateInstanceStorageVolumes(ctx, vm, nil)...) fieldErrs = append(fieldErrs, v.validatePowerStateOnCreate(ctx, vm)...) fieldErrs = append(fieldErrs, v.validateNextRestartTimeOnCreate(ctx, vm)...) + fieldErrs = append(fieldErrs, v.validateAnnotation(ctx, vm)...) validationErrs := make([]string, 0, len(fieldErrs)) for _, fieldErr := range fieldErrs { @@ -178,6 +180,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission. fieldErrs = append(fieldErrs, v.validateReadinessProbe(ctx, vm)...) fieldErrs = append(fieldErrs, v.validateInstanceStorageVolumes(ctx, vm, oldVM)...) fieldErrs = append(fieldErrs, v.validateNextRestartTimeOnUpdate(ctx, vm, oldVM)...) + fieldErrs = append(fieldErrs, v.validateAnnotation(ctx, vm)...) validationErrs := make([]string, 0, len(fieldErrs)) for _, fieldErr := range fieldErrs { @@ -738,3 +741,19 @@ func (v validator) isNetworkRestrictedForReadinessProbe(ctx *context.WebhookRequ return configMap.Data[isRestrictedNetworkKey] == "true", nil } + +func (v validator) validateAnnotation(ctx *context.WebhookRequestContext, vm *vmopv1.VirtualMachine) field.ErrorList { + var allErrs field.ErrorList + + if ctx.IsPrivilegedAccount || vm.Annotations == nil { + return allErrs + } + + annotationPath := field.NewPath("metadata", "annotations") + + if _, exists := vm.Annotations[vmopv1.InstanceIDAnnotation]; exists { + allErrs = append(allErrs, field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), settingAnnotationNotAllowed)) + } + + return allErrs +} diff --git a/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator_unit_test.go b/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator_unit_test.go index 7b5dc5e34..19fda5408 100644 --- a/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator_unit_test.go +++ b/webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator_unit_test.go @@ -221,6 +221,7 @@ func unitTestsValidateCreate() { isSysprepTransportUsed bool powerState vmopv1.VirtualMachinePowerState nextRestartTime string + isForbiddenAnnotation bool } validateCreate := func(args createArgs, expectedAllowed bool, expectedReason string, expectedErr error) { @@ -368,6 +369,10 @@ func unitTestsValidateCreate() { ctx.vm.Labels[topology.KubernetesTopologyZoneLabelKey] = zoneName } + if args.isForbiddenAnnotation { + ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = "some-value" + } + ctx.vm.Spec.PowerState = args.powerState ctx.vm.Spec.NextRestartTime = args.nextRestartTime @@ -414,6 +419,7 @@ func unitTestsValidateCreate() { volPath := specPath.Child("volumes") nextRestartTimePath := specPath.Child("nextRestartTime") now := time.Now().UTC() + annotationPath := field.NewPath("metadata", "annotations") DescribeTable("create table", validateCreate, Entry("should allow valid", createArgs{}, true, nil, nil), @@ -506,6 +512,9 @@ func unitTestsValidateCreate() { Entry("should disallow creating VM with non-empty, invalid nextRestartTime value", createArgs{nextRestartTime: "hello"}, false, field.Invalid(nextRestartTimePath, "hello", "cannot restart VM on create").Error(), nil), + Entry("should deny cloud-init-instance-id annotation set by SSO user", createArgs{isForbiddenAnnotation: true}, false, + field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), "adding this annotation is not allowed").Error(), nil), + Entry("should allow cloud-init-instance-id annotation set by service user", createArgs{isServiceUser: true, isForbiddenAnnotation: true}, true, nil, nil), ) } @@ -533,6 +542,7 @@ func unitTestsValidateUpdate() { newPowerStateEmptyAllowed bool nextRestartTime string lastRestartTime string + isForbiddenAnnotation bool } validateUpdate := func(args updateArgs, expectedAllowed bool, expectedReason string, expectedErr error) { @@ -581,6 +591,9 @@ func unitTestsValidateUpdate() { ctx.oldVM.Spec.NextRestartTime = args.lastRestartTime ctx.vm.Spec.NextRestartTime = args.nextRestartTime + if args.isForbiddenAnnotation { + ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = "some-value" + } // Named network provider undoNamedNetProvider := initNamedNetworkProviderConfig( ctx, @@ -621,6 +634,7 @@ func unitTestsValidateUpdate() { volumesPath := field.NewPath("spec", "volumes") powerStatePath := field.NewPath("spec", "powerState") nextRestartTimePath := field.NewPath("spec", "nextRestartTime") + annotationPath := field.NewPath("metadata", "annotations") DescribeTable("update table", validateUpdate, // Immutable Fields @@ -672,6 +686,9 @@ func unitTestsValidateUpdate() { Entry("should disallow updating VM with non-empty, invalid nextRestartTime value ", updateArgs{nextRestartTime: "hello"}, false, field.Invalid(nextRestartTimePath, "hello", "must be formatted as RFC3339Nano").Error(), nil), + Entry("should deny cloud-init-instance-id annotation set by SSO user", updateArgs{isForbiddenAnnotation: true}, false, + field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), "adding this annotation is not allowed").Error(), nil), + Entry("should allow cloud-init-instance-id annotation set by service user", updateArgs{isServiceUser: true, isForbiddenAnnotation: true}, true, nil, nil), ) When("the update is performed while object deletion", func() { diff --git a/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator.go b/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator.go index ad727927f..2b5412bd1 100644 --- a/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator.go +++ b/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator.go @@ -59,6 +59,7 @@ const ( invalidNextRestartTimeOnCreate = "cannot restart VM on create" invalidNextRestartTimeOnUpdate = "must be formatted as RFC3339Nano" invalidNextRestartTimeOnUpdateNow = "mutation webhooks are required to restart VM" + settingAnnotationNotAllowed = "adding this annotation is not allowed" ) // +kubebuilder:webhook:verbs=create;update,path=/default-validate-vmoperator-vmware-com-v1alpha2-virtualmachine,mutating=false,failurePolicy=fail,groups=vmoperator.vmware.com,resources=virtualmachines,versions=v1alpha2,name=default.validating.virtualmachine.v1alpha2.vmoperator.vmware.com,sideEffects=None,admissionReviewVersions=v1;v1beta1 @@ -114,6 +115,7 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission. fieldErrs = append(fieldErrs, v.validateAdvanced(ctx, vm)...) fieldErrs = append(fieldErrs, v.validatePowerStateOnCreate(ctx, vm)...) fieldErrs = append(fieldErrs, v.validateNextRestartTimeOnCreate(ctx, vm)...) + fieldErrs = append(fieldErrs, v.validateAnnotation(ctx, vm)...) validationErrs := make([]string, 0, len(fieldErrs)) for _, fieldErr := range fieldErrs { @@ -168,6 +170,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission. fieldErrs = append(fieldErrs, v.validateAdvanced(ctx, vm)...) fieldErrs = append(fieldErrs, v.validateInstanceStorageVolumes(ctx, vm, oldVM)...) fieldErrs = append(fieldErrs, v.validateNextRestartTimeOnUpdate(ctx, vm, oldVM)...) + fieldErrs = append(fieldErrs, v.validateAnnotation(ctx, vm)...) validationErrs := make([]string, 0, len(fieldErrs)) for _, fieldErr := range fieldErrs { @@ -679,3 +682,19 @@ func (v validator) isNetworkRestrictedForReadinessProbe(ctx *context.WebhookRequ return configMap.Data[isRestrictedNetworkKey] == "true", nil } + +func (v validator) validateAnnotation(ctx *context.WebhookRequestContext, vm *vmopv1.VirtualMachine) field.ErrorList { + var allErrs field.ErrorList + + if ctx.IsPrivilegedAccount || vm.Annotations == nil { + return allErrs + } + + annotationPath := field.NewPath("metadata", "annotations") + + if _, exists := vm.Annotations[vmopv1.InstanceIDAnnotation]; exists { + allErrs = append(allErrs, field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), settingAnnotationNotAllowed)) + } + + return allErrs +} diff --git a/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator_unit_test.go b/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator_unit_test.go index 0af8f87fd..92da314c4 100644 --- a/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator_unit_test.go +++ b/webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator_unit_test.go @@ -107,6 +107,7 @@ func unitTestsValidateCreate() { isBootstrapVAppConfigInline bool powerState vmopv1.VirtualMachinePowerState nextRestartTime string + isForbiddenAnnotation bool } validateCreate := func(args createArgs, expectedAllowed bool, expectedReason string, expectedErr error) { @@ -250,6 +251,10 @@ func unitTestsValidateCreate() { } } + if args.isForbiddenAnnotation { + ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = "some-value" + } + ctx.vm.Spec.PowerState = args.powerState ctx.vm.Spec.NextRestartTime = args.nextRestartTime @@ -281,6 +286,7 @@ func unitTestsValidateCreate() { volPath := specPath.Child("volumes") nextRestartTimePath := specPath.Child("nextRestartTime") now := time.Now().UTC() + annotationPath := field.NewPath("metadata", "annotations") DescribeTable("create table", validateCreate, Entry("should allow valid", createArgs{}, true, nil, nil), @@ -362,6 +368,9 @@ func unitTestsValidateCreate() { Entry("should disallow creating VM with non-empty, invalid nextRestartTime value", createArgs{nextRestartTime: "hello"}, false, field.Invalid(nextRestartTimePath, "hello", "cannot restart VM on create").Error(), nil), + Entry("should deny cloud-init-instance-id annotation set by user", createArgs{isForbiddenAnnotation: true}, false, + field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), "adding this annotation is not allowed").Error(), nil), + Entry("should allow cloud-init-instance-id annotation set by service user", createArgs{isServiceUser: true, isForbiddenAnnotation: true}, true, nil, nil), ) } @@ -387,6 +396,7 @@ func unitTestsValidateUpdate() { newPowerStateEmptyAllowed bool nextRestartTime string lastRestartTime string + isForbiddenAnnotation bool } validateUpdate := func(args updateArgs, expectedAllowed bool, expectedReason string, expectedErr error) { @@ -440,6 +450,10 @@ func unitTestsValidateUpdate() { ctx.vm.Spec.PowerState = args.newPowerState } + if args.isForbiddenAnnotation { + ctx.vm.Annotations[vmopv1.InstanceIDAnnotation] = "some-value" + } + ctx.oldVM.Spec.NextRestartTime = args.lastRestartTime ctx.vm.Spec.NextRestartTime = args.nextRestartTime @@ -473,6 +487,7 @@ func unitTestsValidateUpdate() { volumesPath := field.NewPath("spec", "volumes") powerStatePath := field.NewPath("spec", "powerState") nextRestartTimePath := field.NewPath("spec", "nextRestartTime") + annotationPath := field.NewPath("metadata", "annotations") DescribeTable("update table", validateUpdate, Entry("should allow", updateArgs{}, true, nil, nil), @@ -517,6 +532,9 @@ func unitTestsValidateUpdate() { Entry("should disallow updating VM with non-empty, invalid nextRestartTime value ", updateArgs{nextRestartTime: "hello"}, false, field.Invalid(nextRestartTimePath, "hello", "must be formatted as RFC3339Nano").Error(), nil), + Entry("should deny cloud-init-instance-id annotation set by user", updateArgs{isForbiddenAnnotation: true}, false, + field.Forbidden(annotationPath.Child(vmopv1.InstanceIDAnnotation), "adding this annotation is not allowed").Error(), nil), + Entry("should allow cloud-init-instance-id annotation set by service user", updateArgs{isServiceUser: true, isForbiddenAnnotation: true}, true, nil, nil), ) When("the update is performed while object deletion", func() {