Merge pull request #754 from akutz/feature/support-pvcs-with-byok

✨ Support PVCs with BYOK

controllers/virtualmachine/volume/volume_controller.go

@@ -525,6 +525,11 @@ func (r *Reconciler) createInstanceStoragePVC(
 		},
 	}
 
+	if c := ctx.VM.Spec.Crypto; c != nil && c.EncryptionClassName != "" {
+		// Assign the InstanceStorage PVC the same EncryptionClass as the VM.
+		pvc.Annotations[constants.EncryptionClassNameAnnotation] = c.EncryptionClassName
+	}
+
 	if err := controllerutil.SetControllerReference(ctx.VM, pvc, r.Client.Scheme()); err != nil {
 		// This is an unexpected error.
 		return fmt.Errorf("cannot set controller reference on PersistentVolumeClaim: %w", err)

controllers/virtualmachine/volume/volume_controller_intg_test.go

@@ -244,7 +244,9 @@ func intgTestsReconcile() {
 				config.Features.InstanceStorage = true
 				config.InstanceStorage.PVPlacementFailedTTL = 0
 			})
+		})
 
+		JustBeforeEach(func() {
 			vm.Spec.Volumes = append(vm.Spec.Volumes, builder.DummyInstanceStorageVirtualMachineVolumes()...)
 			vm.Labels = map[string]string{constants.InstanceStorageLabelKey: "true"}
 			Expect(ctx.Client.Create(ctx, vm)).To(Succeed())
@@ -305,6 +307,42 @@ func intgTestsReconcile() {
 				waitForVirtualMachineInstanceStorage(vmKey, false, false)
 			})
 		})
+
+		When("VM has non-empty spec.crypto.encryptionClassName", func() {
+			BeforeEach(func() {
+				if vm.Annotations == nil {
+					vm.Annotations = map[string]string{}
+				}
+				vm.Annotations[constants.InstanceStorageSelectedNodeAnnotationKey] = dummySelectedNode
+				vm.Annotations[constants.InstanceStorageSelectedNodeMOIDAnnotationKey] = dummySelectedNodeMOID
+
+				vm.Spec.Crypto = &vmopv1.VirtualMachineCryptoSpec{
+					EncryptionClassName: "my-encryption-class",
+				}
+			})
+			Specify("PVCs are created with the expected annotation", func() {
+				Expect(ctx.Client.Get(ctx, vmKey, vm)).To(Succeed())
+
+				volumes := instancestorage.FilterVolumes(vm)
+				Expect(volumes).ToNot(BeEmpty())
+
+				Eventually(func(g Gomega) {
+					for i := range volumes {
+						var obj corev1.PersistentVolumeClaim
+						g.Expect(ctx.Client.Get(
+							ctx,
+							client.ObjectKey{
+								Namespace: vm.Namespace,
+								Name:      volumes[i].PersistentVolumeClaim.ClaimName,
+							},
+							&obj)).To(Succeed())
+						g.Expect(obj.Annotations).To(HaveKeyWithValue(
+							constants.EncryptionClassNameAnnotation,
+							vm.Spec.Crypto.EncryptionClassName))
+					}
+				}).Should(Succeed())
+			})
+		})
 	})
 
 	Context("Reconcile", func() {

controllers/virtualmachine/volume/volume_controller_unit_test.go

@@ -270,6 +270,57 @@ func unitTestsReconcile() {
 				Expect(err.Error()).To(ContainSubstring("insufficient quota"))
 				expectPVCsStatus(volCtx, ctx, true, false, 0)
 			})
+
+			When("VM has nil spec.crypto", func() {
+				BeforeEach(func() {
+					vm.Spec.Crypto = nil
+				})
+				Specify("PVCs are created sans the expected annotation", func() {
+					Expect(reconciler.ReconcileNormal(volCtx)).To(Succeed())
+					pvcList, err := getInstanceStoragePVCs(volCtx, ctx)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(pvcList).ToNot(BeEmpty())
+					for i := range pvcList {
+						Expect(pvcList[i].Annotations).ToNot(HaveKey(constants.EncryptionClassNameAnnotation))
+					}
+				})
+			})
+
+			When("VM has empty spec.crypto.encryptionClassName", func() {
+				BeforeEach(func() {
+					vm.Spec.Crypto = &vmopv1.VirtualMachineCryptoSpec{
+						EncryptionClassName: "",
+					}
+				})
+				Specify("PVCs are created sans the expected annotation", func() {
+					Expect(reconciler.ReconcileNormal(volCtx)).To(Succeed())
+					pvcList, err := getInstanceStoragePVCs(volCtx, ctx)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(pvcList).ToNot(BeEmpty())
+					for i := range pvcList {
+						Expect(pvcList[i].Annotations).ToNot(HaveKey(constants.EncryptionClassNameAnnotation))
+					}
+				})
+			})
+
+			When("VM has non-empty spec.crypto.encryptionClassName", func() {
+				BeforeEach(func() {
+					vm.Spec.Crypto = &vmopv1.VirtualMachineCryptoSpec{
+						EncryptionClassName: "my-encryption-class",
+					}
+				})
+				Specify("PVCs are created with the expected annotation", func() {
+					Expect(reconciler.ReconcileNormal(volCtx)).To(Succeed())
+					pvcList, err := getInstanceStoragePVCs(volCtx, ctx)
+					Expect(err).ToNot(HaveOccurred())
+					Expect(pvcList).ToNot(BeEmpty())
+					for i := range pvcList {
+						Expect(pvcList[i].Annotations).To(HaveKeyWithValue(
+							constants.EncryptionClassNameAnnotation,
+							vm.Spec.Crypto.EncryptionClassName))
+					}
+				})
+			})
 		})
 
 		When("VM does not have BiosUUID", func() {

pkg/providers/vsphere/constants/constants.go

@@ -68,6 +68,11 @@ const (
 	CloudInitGuestInfoUserdata         = "guestinfo.userdata"
 	CloudInitGuestInfoUserdataEncoding = "guestinfo.userdata.encoding"
 
+	// EncryptionClassNameAnnotation specifies the name of an EncryptionClass
+	// resource. This is used by APIs that participate in BYOK but cannot modify
+	// their spec to do so, such as the PersistentVolumeClaim API.
+	EncryptionClassNameAnnotation = "encryption.vmware.com/encryption-class-name"
+
 	// InstanceStoragePVCNamePrefix prefix of auto-generated PVC names.
 	InstanceStoragePVCNamePrefix = "instance-pvc-"
 	// InstanceStorageLabelKey identifies resources related to instance storage.

webhooks/virtualmachine/validation/virtualmachine_validator.go

@@ -827,40 +827,29 @@ func (v validator) validateVolumes(ctx *pkgctx.WebhookRequestContext, vm *vmopv1
 		if vol.PersistentVolumeClaim == nil {
 			allErrs = append(allErrs, field.Required(volPath.Child("persistentVolumeClaim"), ""))
 		} else {
-			allErrs = append(allErrs, v.validateVolumeWithPVC(ctx, vm, vol, volPath)...)
+			allErrs = append(allErrs, v.validateVolumeWithPVC(vol, volPath)...)
 		}
 	}
 
 	return allErrs
 }
 
 func (v validator) validateVolumeWithPVC(
-	ctx *pkgctx.WebhookRequestContext,
-	vm *vmopv1.VirtualMachine,
 	vol vmopv1.VirtualMachineVolume,
 	volPath *field.Path) field.ErrorList {
 
 	var (
-		allErrs      field.ErrorList
-		encClassName string
-		pvcPath      = volPath.Child("persistentVolumeClaim")
-		claimName    = vol.PersistentVolumeClaim.ClaimName
+		allErrs   field.ErrorList
+		pvcPath   = volPath.Child("persistentVolumeClaim")
+		claimName = vol.PersistentVolumeClaim.ClaimName
 	)
 
-	if vm.Spec.Crypto != nil {
-		encClassName = vm.Spec.Crypto.EncryptionClassName
-	}
-
 	if claimName == "" {
 		allErrs = append(
 			allErrs,
 			field.Required(pvcPath.Child("claimName"), ""))
-	} else if encClassName != "" && pkgcfg.FromContext(ctx).Features.BringYourOwnEncryptionKey {
-		allErrs = append(allErrs, field.Invalid(
-			pvcPath.Child("claimName"),
-			claimName,
-			fmt.Sprintf(invalidPVCBYOKFmt, encClassName)))
 	}
+
 	if vol.PersistentVolumeClaim.ReadOnly {
 		allErrs = append(allErrs, field.NotSupported(pvcPath.Child("readOnly"), true, []string{"false"}))
 	}

webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go

@@ -860,7 +860,7 @@ func unitTestsValidateCreate() {
 					`spec.crypto.encryptionClassName: Invalid value: "fake": requires spec.storageClass specify an encryption storage class`),
 			},
 		),
-		Entry("disallow volume when spec.crypto.encryptionClassName is non-empty when FSS_WCP_VMSERVICE_BYOK is enabled",
+		Entry("allow volume when spec.crypto.encryptionClassName is non-empty when FSS_WCP_VMSERVICE_BYOK is enabled",
 			testParams{
 				setup: func(ctx *unitValidatingWebhookContext) {
 					storageClass1 := builder.DummyStorageClass()
@@ -902,61 +902,6 @@ func unitTestsValidateCreate() {
 						config.Features.BringYourOwnEncryptionKey = true
 					})
 				},
-				validate: doValidateWithMsg(
-					`spec.volumes[0].persistentVolumeClaim.claimName: Invalid value: "dummyPVCName": cannot attach volume to vm with spec.crypto.encryptionClassName="fake"`),
-			},
-		),
-		Entry("allow volume when spec.crypto.encryptionClassName is empty when FSS_WCP_VMSERVICE_BYOK is enabled",
-			testParams{
-				setup: func(ctx *unitValidatingWebhookContext) {
-					storageClass1 := builder.DummyStorageClass()
-					Expect(ctx.Client.Create(ctx, storageClass1)).To(Succeed())
-
-					storageClass2 := builder.DummyStorageClass()
-					storageClass2.Name += "2"
-					Expect(ctx.Client.Create(ctx, storageClass2)).To(Succeed())
-
-					resourceQuota := builder.DummyResourceQuota(
-						ctx.vm.Namespace,
-						storageClass1.Name+".storageclass.storage.k8s.io/persistentvolumeclaims",
-						storageClass2.Name+".storageclass.storage.k8s.io/persistentvolumeclaims")
-					Expect(ctx.Client.Create(ctx, resourceQuota)).To(Succeed())
-
-					pvc := builder.DummyPersistentVolumeClaim()
-					pvc.Name = builder.DummyPVCName
-					pvc.Namespace = ctx.vm.Namespace
-					pvc.Spec.StorageClassName = ptr.To(storageClass2.Name)
-					Expect(ctx.Client.Create(ctx, pvc)).To(Succeed())
-
-					ctx.vm.Spec.StorageClass = storageClass1.Name
-					ctx.vm.Spec.Crypto = &vmopv1.VirtualMachineCryptoSpec{}
-
-					var vmStorageClass storagev1.StorageClass
-					Expect(ctx.Client.Get(
-						ctx,
-						client.ObjectKey{Name: ctx.vm.Spec.StorageClass},
-						&vmStorageClass)).To(Succeed())
-					Expect(kubeutil.MarkEncryptedStorageClass(
-						ctx,
-						ctx.Client,
-						vmStorageClass,
-						false)).To(Succeed())
-
-					var pvcStorageClass storagev1.StorageClass
-					Expect(ctx.Client.Get(
-						ctx,
-						client.ObjectKey{Name: *pvc.Spec.StorageClassName},
-						&pvcStorageClass)).To(Succeed())
-					Expect(kubeutil.MarkEncryptedStorageClass(
-						ctx,
-						ctx.Client,
-						pvcStorageClass,
-						false)).To(Succeed())
-
-					pkgcfg.SetContext(ctx, func(config *pkgcfg.Config) {
-						config.Features.BringYourOwnEncryptionKey = true
-					})
-				},
 				expectAllowed: true,
 			},
 		),