From 0bad8f498b654709a2e886585bf871f7c683cba0 Mon Sep 17 00:00:00 2001 From: akutz Date: Thu, 17 Oct 2024 11:06:52 -0500 Subject: [PATCH 1/2] Support PVCs with BYOK This patch allows PVCs to be attached to VMs encrypted with BYOK. --- .../validation/virtualmachine_validator.go | 21 ++----- .../virtualmachine_validator_unit_test.go | 57 +------------------ 2 files changed, 6 insertions(+), 72 deletions(-) diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator.go b/webhooks/virtualmachine/validation/virtualmachine_validator.go index 4203ce0a6..21c3d6527 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator.go +++ b/webhooks/virtualmachine/validation/virtualmachine_validator.go @@ -827,7 +827,7 @@ 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)...) } } @@ -835,32 +835,21 @@ func (v validator) validateVolumes(ctx *pkgctx.WebhookRequestContext, vm *vmopv1 } 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"})) } diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go b/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go index 056a9a807..73e8976a3 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator_unit_test.go +++ b/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, }, ), From f7d67529ec86dc9e3edcdb620cae2b960906b687 Mon Sep 17 00:00:00 2001 From: akutz Date: Thu, 17 Oct 2024 11:33:21 -0500 Subject: [PATCH 2/2] Support BYOK for Instance Storage PVCs This patch adds support for creating Instance Storage PVCs with the EncryptionClass annotation that matches the VM's EncryptionClass. --- .../volume/volume_controller.go | 5 ++ .../volume/volume_controller_intg_test.go | 38 ++++++++++++++ .../volume/volume_controller_unit_test.go | 51 +++++++++++++++++++ pkg/providers/vsphere/constants/constants.go | 5 ++ 4 files changed, 99 insertions(+) diff --git a/controllers/virtualmachine/volume/volume_controller.go b/controllers/virtualmachine/volume/volume_controller.go index ae5feacf7..0db408e34 100644 --- a/controllers/virtualmachine/volume/volume_controller.go +++ b/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) diff --git a/controllers/virtualmachine/volume/volume_controller_intg_test.go b/controllers/virtualmachine/volume/volume_controller_intg_test.go index 9b917efcc..698f5bffe 100644 --- a/controllers/virtualmachine/volume/volume_controller_intg_test.go +++ b/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() { diff --git a/controllers/virtualmachine/volume/volume_controller_unit_test.go b/controllers/virtualmachine/volume/volume_controller_unit_test.go index caad41abe..72cb4cde4 100644 --- a/controllers/virtualmachine/volume/volume_controller_unit_test.go +++ b/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() { diff --git a/pkg/providers/vsphere/constants/constants.go b/pkg/providers/vsphere/constants/constants.go index 27cc2d672..0fdf9ccba 100644 --- a/pkg/providers/vsphere/constants/constants.go +++ b/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.