Emit a webhook warning when OvfEnv transport is specified (#231)

This change emits a warning when a user specifies OvfEnv transport in
a VM's spec.  In order to support warnings, modify the
BuildValidationResponse so it can now take warnings from any
validations.
At some point, we should move our webhooks to controller-runtime
generated webhooks which will remove the need to synthesize the
response from errors and warnings ourselves.

webhooks/common/response.go

@@ -19,6 +19,7 @@ import (
 // errors returned attempting to validate the ingress data.
 func BuildValidationResponse(
 	ctx *context.WebhookRequestContext,
+	validationWarnings admission.Warnings,
 	validationErrs []string,
 	err error,
 	additionalValidationErrors ...string) (response admission.Response) {
@@ -76,5 +77,5 @@ func BuildValidationResponse(
 		}
 	}
 
-	return admission.Allowed("")
+	return admission.Allowed("").WithWarnings(validationWarnings...)
 }

webhooks/common/response_test.go

@@ -35,26 +35,36 @@ var _ = Describe("Validation Response", func() {
 
 	When("No errors occur", func() {
 		It("Returns allowed", func() {
-			response := common.BuildValidationResponse(ctx, nil, nil)
+			response := common.BuildValidationResponse(ctx, nil, nil, nil)
 			Expect(response.Allowed).To(BeTrue())
 		})
 	})
 
 	When("Validation errors occur", func() {
 		It("Returns denied", func() {
 			validationErrs := []string{"this is required"}
-			response := common.BuildValidationResponse(ctx, validationErrs, nil)
+			response := common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 			Expect(response.Allowed).To(BeFalse())
 			Expect(response.Result).ToNot(BeNil())
 			Expect(response.Result.Code).To(Equal(int32(http.StatusUnprocessableEntity)))
 			Expect(string(response.Result.Reason)).To(ContainSubstring(validationErrs[0]))
 		})
 	})
 
-	Context("Returns denied for expected well-known errors", func() {
+	When("Validation has warnings", func() {
+		It("Returns allowed, with warnings", func() {
+			validationWarnings := []string{"this is deprecated"}
+			response := common.BuildValidationResponse(ctx, validationWarnings, nil, nil)
+			Expect(response.Allowed).To(BeTrue())
+			Expect(response.Warnings).To(Equal(validationWarnings))
+			Expect(response.Result).ToNot(BeNil())
+			Expect(response.Result.Code).To(Equal(int32(http.StatusOK)))
+		})
+	})
 
+	Context("Returns denied for expected well-known errors", func() {
 		wellKnownError := func(err error, expectedCode int) {
-			response := common.BuildValidationResponse(ctx, nil, err)
+			response := common.BuildValidationResponse(ctx, nil, nil, err)
 			Expect(response.Allowed).To(BeFalse())
 			Expect(response.Result).ToNot(BeNil())
 			Expect(response.Result.Code).To(Equal(int32(expectedCode)))

webhooks/persistentvolumeclaim/validation/persistentvolumeclaim_validator.go

@@ -78,7 +78,7 @@ func (v validator) For() schema.GroupVersionKind {
 
 func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.Response {
 	if isPrivilegedAccountForISPVC(ctx) {
-		return common.BuildValidationResponse(ctx, nil, nil)
+		return common.BuildValidationResponse(ctx, nil, nil, nil)
 	}
 
 	var fieldErrs field.ErrorList
@@ -87,12 +87,12 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.
 			fmt.Sprintf(operationNotAllowedOnPVC, admissionv1.Create)))
 	}
 
-	return common.BuildValidationResponse(ctx, convertToStringArray(fieldErrs), nil)
+	return common.BuildValidationResponse(ctx, nil, convertToStringArray(fieldErrs), nil)
 }
 
 func (v validator) ValidateDelete(ctx *context.WebhookRequestContext) admission.Response {
 	if isPrivilegedAccountForISPVC(ctx) {
-		return common.BuildValidationResponse(ctx, nil, nil)
+		return common.BuildValidationResponse(ctx, nil, nil, nil)
 	}
 
 	var fieldErrs field.ErrorList
@@ -101,12 +101,12 @@ func (v validator) ValidateDelete(ctx *context.WebhookRequestContext) admission.
 			fmt.Sprintf(operationNotAllowedOnPVC, admissionv1.Delete)))
 	}
 
-	return common.BuildValidationResponse(ctx, convertToStringArray(fieldErrs), nil)
+	return common.BuildValidationResponse(ctx, nil, convertToStringArray(fieldErrs), nil)
 }
 
 func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.Response {
 	if isPrivilegedAccountForISPVC(ctx) {
-		return common.BuildValidationResponse(ctx, nil, nil)
+		return common.BuildValidationResponse(ctx, nil, nil, nil)
 	}
 	var fieldErrs field.ErrorList
 	// If instance storage labels already exists for resource, do not allow update resource
@@ -117,7 +117,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 		fieldErrs = append(fieldErrs, field.Forbidden(labelPath, addingISLabelNotAllowed))
 	}
 
-	return common.BuildValidationResponse(ctx, convertToStringArray(fieldErrs), nil)
+	return common.BuildValidationResponse(ctx, nil, convertToStringArray(fieldErrs), nil)
 }
 
 // isInstanceStorageLabelPresent - returns true/false depending on presence of instance storage label.

webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator.go

@@ -65,6 +65,8 @@ const (
 	invalidNextRestartTimeOnUpdate            = "must be formatted as RFC3339Nano"
 	invalidNextRestartTimeOnUpdateNow         = "mutation webhooks are required to restart VM"
 	modifyAnnotationNotAllowedForNonAdmin     = "modifying this annotation is not allowed for non-admin users"
+	OVFEnvTransportDeprecated                 = "OvfEnv transport is deprecated. Please use CloudInit or vAppConfig transport instead."
+	ExtraConfigTransportDeprecated            = "ExtraConfig transport is deprecated. Please use CloudInit or vAppConfig transport instead."
 )
 
 // +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.v1alpha1.vmoperator.vmware.com,sideEffects=None,admissionReviewVersions=v1;v1beta1
@@ -107,8 +109,12 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.
 	}
 
 	var fieldErrs field.ErrorList
+	var validationWarnings admission.Warnings
+
+	warnings, errs := v.validateMetadata(ctx, vm)
+	fieldErrs = append(fieldErrs, errs...)
+	validationWarnings = append(validationWarnings, warnings...)
 
-	fieldErrs = append(fieldErrs, v.validateMetadata(ctx, vm)...)
 	fieldErrs = append(fieldErrs, v.validateAvailabilityZone(ctx, vm, nil)...)
 	fieldErrs = append(fieldErrs, v.validateImage(ctx, vm)...)
 	fieldErrs = append(fieldErrs, v.validateClass(ctx, vm)...)
@@ -127,7 +133,7 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, validationWarnings, validationErrs, nil)
 }
 
 func (v validator) ValidateDelete(*context.WebhookRequestContext) admission.Response {
@@ -162,6 +168,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 	}
 
 	var fieldErrs field.ErrorList
+	var validationWarnings admission.Warnings
 
 	// Check if an immutable field has been modified.
 	fieldErrs = append(fieldErrs, v.validateImmutableFields(ctx, vm, oldVM)...)
@@ -172,7 +179,10 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 
 	// Validations for allowed updates. Return validation responses here for
 	// conditional updates regardless of whether the update is allowed or not.
-	fieldErrs = append(fieldErrs, v.validateMetadata(ctx, vm)...)
+	warnings, errs := v.validateMetadata(ctx, vm)
+	fieldErrs = append(fieldErrs, errs...)
+	validationWarnings = append(validationWarnings, warnings...)
+
 	fieldErrs = append(fieldErrs, v.validateAvailabilityZone(ctx, vm, oldVM)...)
 	fieldErrs = append(fieldErrs, v.validateNetwork(ctx, vm)...)
 	fieldErrs = append(fieldErrs, v.validateVolumes(ctx, vm)...)
@@ -187,18 +197,31 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, validationWarnings, validationErrs, nil)
 }
 
-func (v validator) validateMetadata(ctx *context.WebhookRequestContext, vm *vmopv1.VirtualMachine) field.ErrorList {
+func (v validator) validateMetadata(ctx *context.WebhookRequestContext, vm *vmopv1.VirtualMachine) (admission.Warnings, field.ErrorList) {
 	var allErrs field.ErrorList
+	var allWarnings admission.Warnings
 
 	if vm.Spec.VmMetadata == nil {
-		return allErrs
+		return allWarnings, allErrs
 	}
 
 	mdPath := field.NewPath("spec", "vmMetadata")
 
+	// OvfEnv transport is marked as deprecated.  Emit a warnings to indicate that to users.
+	if vm.Spec.VmMetadata.Transport == vmopv1.VirtualMachineMetadataOvfEnvTransport {
+		allWarnings = append(allWarnings,
+			fmt.Sprint(OVFEnvTransportDeprecated))
+	}
+
+	// ExtraConfig transport is marked as deprecated.  Emit a warnings to indicate that to users.
+	if vm.Spec.VmMetadata.Transport == vmopv1.VirtualMachineMetadataExtraConfigTransport {
+		allWarnings = append(allWarnings,
+			fmt.Sprint(ExtraConfigTransportDeprecated))
+	}
+
 	if vm.Spec.VmMetadata.ConfigMapName != "" && vm.Spec.VmMetadata.SecretName != "" {
 		allErrs = append(allErrs, field.Invalid(mdPath.Child("configMapName"), vm.Spec.VmMetadata.ConfigMapName,
 			fmt.Sprintf(metadataTransportResourcesInvalid, mdPath.Child("configMapName"), mdPath.Child("secretName"))))
@@ -213,7 +236,7 @@ func (v validator) validateMetadata(ctx *context.WebhookRequestContext, vm *vmop
 		}
 	}
 
-	return allErrs
+	return allWarnings, allErrs
 }
 
 func (v validator) validateImage(ctx *context.WebhookRequestContext, vm *vmopv1.VirtualMachine) field.ErrorList {

webhooks/virtualmachine/v1alpha1/validation/virtualmachine_validator_unit_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/vmware-tanzu/vm-operator/pkg/vmprovider/providers/vsphere/config"
 	"github.com/vmware-tanzu/vm-operator/pkg/vmprovider/providers/vsphere/network"
 	"github.com/vmware-tanzu/vm-operator/test/builder"
+	vmopv1validation "github.com/vmware-tanzu/vm-operator/webhooks/virtualmachine/v1alpha1/validation"
 )
 
 const (
@@ -42,7 +43,9 @@ const (
 
 func unitTests() {
 	Describe("Invoking ValidateCreate", unitTestsValidateCreate)
+	Describe("Invoking ValidateCreate for Webhook Warnings", unitTestsValidateCreateWarnings)
 	Describe("Invoking ValidateUpdate", unitTestsValidateUpdate)
+	Describe("Invoking ValidateUpdate for Webhook Warnings", unitTestsValidateUpdateWarnings)
 	Describe("Invoking ValidateDelete", unitTestsValidateDelete)
 }
 
@@ -526,6 +529,100 @@ func unitTestsValidateCreate() {
 	)
 }
 
+func unitTestsValidateCreateWarnings() {
+	var (
+		ctx *unitValidatingWebhookContext
+	)
+
+	Context("VM Metadata transport deprecation warnings", func() {
+		var err error
+		BeforeEach(func() {
+			ctx = newUnitTestContextForValidatingWebhook(false)
+		})
+
+		When("OvfEnv transport is specified in VM Metadata", func() {
+			BeforeEach(func() {
+				ctx.vm.Spec.VmMetadata.Transport = vmopv1.VirtualMachineMetadataOvfEnvTransport
+				ctx.WebhookRequestContext.Obj, err = builder.ToUnstructured(ctx.vm)
+				Expect(err).ToNot(HaveOccurred())
+			})
+			It("the request should succeed, but with a warning", func() {
+				response := ctx.ValidateCreate(&ctx.WebhookRequestContext)
+				Expect(response.Allowed).To(Equal(true))
+				Expect(response.Warnings).To(Equal([]string{vmopv1validation.OVFEnvTransportDeprecated}))
+			})
+		})
+		When("ExtraConfig transport is specified in VM Metadata", func() {
+			BeforeEach(func() {
+				ctx.vm.Spec.VmMetadata.Transport = vmopv1.VirtualMachineMetadataExtraConfigTransport
+				ctx.WebhookRequestContext.Obj, err = builder.ToUnstructured(ctx.vm)
+				Expect(err).ToNot(HaveOccurred())
+			})
+			It("the request should succeed, but with a warning", func() {
+				response := ctx.ValidateCreate(&ctx.WebhookRequestContext)
+				Expect(response.Allowed).To(Equal(true))
+
+				Expect(response.Warnings).To(Equal([]string{vmopv1validation.ExtraConfigTransportDeprecated}))
+			})
+		})
+	})
+}
+
+func unitTestsValidateUpdateWarnings() {
+	var (
+		ctx *unitValidatingWebhookContext
+	)
+
+	Context("VM Metadata transport deprecation warnings", func() {
+		var err error
+		BeforeEach(func() {
+			ctx = newUnitTestContextForValidatingWebhook(true)
+		})
+		AfterEach(func() {
+			ctx = nil
+		})
+
+		When("OvfEnv transport is specified in VM Metadata", func() {
+			BeforeEach(func() {
+				// Updates to metadata are only allowed in powered off state.
+				ctx.vm.Spec.PowerState = vmopv1.VirtualMachinePoweredOff
+				ctx.vm.Spec.VmMetadata.Transport = vmopv1.VirtualMachineMetadataOvfEnvTransport
+
+				ctx.WebhookRequestContext.Obj, err = builder.ToUnstructured(ctx.vm)
+				Expect(err).ToNot(HaveOccurred())
+				ctx.WebhookRequestContext.OldObj, err = builder.ToUnstructured(ctx.oldVM)
+				Expect(err).ToNot(HaveOccurred())
+
+			})
+			It("the request should succeed, but with a warning", func() {
+				response := ctx.ValidateUpdate(&ctx.WebhookRequestContext)
+				Expect(response.Allowed).To(Equal(true))
+				Expect(response.Warnings).To(Equal([]string{vmopv1validation.OVFEnvTransportDeprecated}))
+			})
+		})
+		When("ExtraConfig transport is specified in VM Metadata", func() {
+			BeforeEach(func() {
+
+				// Updates to metadata are only allowed in powered off state.
+				ctx.vm.Spec.PowerState = vmopv1.VirtualMachinePoweredOff
+				ctx.vm.Spec.VmMetadata.Transport = vmopv1.VirtualMachineMetadataExtraConfigTransport
+
+				ctx.WebhookRequestContext.Obj, err = builder.ToUnstructured(ctx.vm)
+				Expect(err).ToNot(HaveOccurred())
+				ctx.WebhookRequestContext.OldObj, err = builder.ToUnstructured(ctx.oldVM)
+				Expect(err).ToNot(HaveOccurred())
+
+			})
+			It("the request should succeed, but with a warning", func() {
+				response := ctx.ValidateUpdate(&ctx.WebhookRequestContext)
+				Expect(response.Allowed).To(Equal(true))
+
+				Expect(response.Warnings).To(Equal([]string{vmopv1validation.ExtraConfigTransportDeprecated}))
+			})
+		})
+	})
+}
+
 func unitTestsValidateUpdate() {
 	var (
 		ctx *unitValidatingWebhookContext

webhooks/virtualmachine/v1alpha2/validation/virtualmachine_validator.go

@@ -122,7 +122,7 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) ValidateDelete(*context.WebhookRequestContext) admission.Response {
@@ -177,7 +177,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) validateBootstrap(

webhooks/virtualmachineclass/v1alpha1/validation/virtualmachineclass_validator.go

@@ -78,7 +78,7 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) ValidateDelete(*context.WebhookRequestContext) admission.Response {
@@ -92,7 +92,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) validatePolicies(ctx *context.WebhookRequestContext, vmClass *vmopv1.VirtualMachineClass,

webhooks/virtualmachineclass/v1alpha2/validation/virtualmachineclass_validator.go

@@ -78,7 +78,7 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) ValidateDelete(*context.WebhookRequestContext) admission.Response {
@@ -92,7 +92,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) validatePolicies(ctx *context.WebhookRequestContext, vmClass *vmopv1.VirtualMachineClass,

webhooks/virtualmachinepublishrequest/v1alpha1/validation/virtualmachinepublishrequest_validator.go

@@ -65,7 +65,7 @@ func (v validator) For() schema.GroupVersionKind {
 
 func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.Response {
 	if !lib.IsWCPVMImageRegistryEnabled() {
-		return common.BuildValidationResponse(ctx, []string{"WCP_VM_Image_Registry feature not enabled"}, nil)
+		return common.BuildValidationResponse(ctx, nil, []string{"WCP_VM_Image_Registry feature not enabled"}, nil)
 	}
 
 	vmpub, err := v.vmPublishRequestFromUnstructured(ctx.Obj)
@@ -83,7 +83,7 @@ func (v validator) ValidateCreate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) ValidateDelete(*context.WebhookRequestContext) admission.Response {
@@ -111,7 +111,7 @@ func (v validator) ValidateUpdate(ctx *context.WebhookRequestContext) admission.
 		validationErrs = append(validationErrs, fieldErr.Error())
 	}
 
-	return common.BuildValidationResponse(ctx, validationErrs, nil)
+	return common.BuildValidationResponse(ctx, nil, validationErrs, nil)
 }
 
 func (v validator) validateSource(ctx *context.WebhookRequestContext, vmpub *vmopv1.VirtualMachinePublishRequest) field.ErrorList {