[x-port] [9.1.0] CopyVirtualDisk set ProfileID and CryptoSpec

controllers/virtualmachineimagecache/virtualmachineimagecache_controller.go

@@ -13,6 +13,7 @@ import (
 	"strings"
 
 	"github.com/go-logr/logr"
+	"github.com/vmware/govmomi/crypto"
 	"github.com/vmware/govmomi/fault"
 	"github.com/vmware/govmomi/find"
 	"github.com/vmware/govmomi/object"
@@ -44,6 +45,7 @@ import (
 	clprov "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/contentlibrary"
 	"github.com/vmware-tanzu/vm-operator/pkg/record"
 	pkgutil "github.com/vmware-tanzu/vm-operator/pkg/util"
+	kubeutil "github.com/vmware-tanzu/vm-operator/pkg/util/kube"
 	"github.com/vmware-tanzu/vm-operator/pkg/util/kube/cource"
 	"github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/client"
 	clsutil "github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/library"
@@ -373,10 +375,24 @@ func (r *reconciler) reconcileLocations(
 			dstDatastores[spec.DatastoreID].mo.Info.
 				GetDatastoreInfo().SupportedVDiskFormats...)
 
+		cryptoSpec, err := r.getStorageProfileCrypto(
+			ctx,
+			vimClient,
+			spec.ProfileID)
+		if err != nil {
+			conditions = conditions.MarkError(
+				vmopv1.ReadyConditionType,
+				conditionReasonFailed,
+				err)
+			status.Conditions = conditions
+			continue
+		}
+
 		// Update the srcFiles elements with the profile and format info.
 		for i := range srcFiles {
 			srcFiles[i].DstProfileID = spec.ProfileID
 			srcFiles[i].DstDiskFormat = dstDiskFormat
+			srcFiles[i].CryptoSpec = cryptoSpec
 		}
 
 		cachedFiles, err := r.cacheFiles(
@@ -402,6 +418,38 @@ func (r *reconciler) reconcileLocations(
 	}
 }
 
+func (r *reconciler) getStorageProfileCrypto(
+	ctx context.Context,
+	vimClient *vim25.Client,
+	profileID string) (*vimtypes.CryptoSpecEncrypt, error) {
+
+	if profileID == "" {
+		return nil, nil
+	}
+
+	isEnc, err := kubeutil.IsEncryptedStorageProfile(ctx, r.Client, profileID)
+	if err != nil || !isEnc {
+		return nil, err
+	}
+
+	// TODO: We don't have an EncryptionClass but the default provider is
+	// optional, so what is the proper fallback?
+	m := crypto.NewManagerKmip(vimClient)
+	providerID, err := m.GetDefaultKmsClusterID(ctx, nil, true)
+	if err != nil {
+		return nil, fmt.Errorf("failed to get default key provider: %w", err)
+	}
+
+	return &vimtypes.CryptoSpecEncrypt{
+		CryptoKeyId: vimtypes.CryptoKeyId{
+			ProviderId: &vimtypes.KeyProviderId{
+				Id: providerID,
+			},
+			KeyId: "",
+		},
+	}, nil
+}
+
 func (r *reconciler) cacheFiles(
 	ctx context.Context,
 	vimClient *vim25.Client,

pkg/providers/vsphere/vmlifecycle/create_fastdeploy.go

@@ -446,33 +446,20 @@ func fastDeployDirect(
 		ctx,
 		logger,
 		datacenter,
+		configSpec,
 		srcDiskPaths,
 		dstDiskPaths,
 		diskFormat); err != nil {
 
 		return nil, err
 	}
 
-	_, isVMEncrypted := configSpec.Crypto.(*vimtypes.CryptoSpecEncrypt)
-
 	for i := range diskSpecs {
 		ds := diskSpecs[i]
 
 		// Set the file operation to an empty string since the disk already
 		// exists.
 		ds.FileOperation = ""
-
-		if isVMEncrypted {
-			// If the VM is to be encrypted, then the disks need to be updated
-			// so they are not marked as encrypted upon VM creation. This is
-			// because it is not possible to change the encryption state of VM
-			// disks when they are being attached. Instead the disks must be
-			// encrypted after they are attached to the VM.
-			ds.Profile = nil
-			if ds.Backing != nil {
-				ds.Backing.Crypto = nil
-			}
-		}
 	}
 
 	return fastDeployCreateVM(ctx, logger, folder, pool, host, configSpec)
@@ -516,6 +503,7 @@ func fastDeployDirectCopyDisks(
 	ctx context.Context,
 	logger logr.Logger,
 	datacenter *object.Datacenter,
+	configSpec vimtypes.VirtualMachineConfigSpec,
 	srcDiskPaths,
 	dstDiskPaths []string,
 	diskFormat vimtypes.DatastoreSectorFormat) error {
@@ -530,9 +518,8 @@ func fastDeployDirectCopyDisks(
 				DiskType:    string(vimtypes.VirtualDiskTypeThin),
 			},
 			SectorFormat: string(diskFormat),
-			// CopyVirtualDisk API simply ignores the Profile
-			// and Crypto parameters. We send neither until the
-			// API is enhanced to support both.
+			Profile:      configSpec.VmProfile,
+			Crypto:       configSpec.Crypto,
 		}
 		diskManager = object.NewVirtualDiskManager(datacenter.Client())
 	)

pkg/util/vsphere/library/item_cache.go

@@ -30,9 +30,11 @@ type SourceFile struct {
 	DstProfileID  string
 	DstDiskFormat vimtypes.DatastoreSectorFormat
 
+	CryptoSpec *vimtypes.CryptoSpecEncrypt
+
 	// TODO(akutz) In the future there may be additional information about the
-	//             disk, such as its sector format (512 vs 4k), is encrypted,
-	//             thin-provisioned, adapter type, etc.
+	//             disk, such as its sector format (512 vs 4k), thin-provisioned,
+	//             adapter type, etc.
 }
 
 // CachedFile refers to file that has been cached.
@@ -75,7 +77,7 @@ type CacheStorageURIsClient interface {
 }
 
 // CacheStorageURIs copies the file(s) from srcFiles to dstDir and returns the
-// the information about the copied file(s).
+// information about the copied file(s).
 func CacheStorageURIs(
 	ctx context.Context,
 	client CacheStorageURIsClient,
@@ -131,6 +133,7 @@ func copyFile(
 	logger = logger.WithValues(
 		"srcFilePath", srcFile.Path,
 		"dstFilePath", dstFilePath,
+		"dstProfileID", srcFile.DstProfileID,
 		"dstDatacenter", dstDatacenter.Reference().Value,
 		"srcDatacenter", srcDatacenter.Reference().Value)
 	if isDisk {
@@ -173,22 +176,41 @@ func copyFile(
 
 	if isDisk {
 		logger.Info("Caching disk")
+
+		ds := &vimtypes.FileBackedVirtualDiskSpec{
+			VirtualDiskSpec: vimtypes.VirtualDiskSpec{
+				AdapterType: string(vimtypes.VirtualDiskAdapterTypeLsiLogic),
+				DiskType:    string(vimtypes.VirtualDiskTypeThin),
+			},
+			SectorFormat: string(srcFile.DstDiskFormat),
+			Profile: []vimtypes.BaseVirtualMachineProfileSpec{
+				&vimtypes.VirtualMachineDefinedProfileSpec{
+					ProfileId: srcFile.DstProfileID,
+				},
+			},
+		}
+
+		if cs := srcFile.CryptoSpec; cs != nil {
+			if id := cs.CryptoKeyId.ProviderId; id != nil && id.Id != "" {
+				ds.Crypto = cs
+			} else {
+				// TODO: The storage profile is encrypted but there is no
+				// default key provider configured. CopyVirtualDisk used
+				// to just ignore the Profile and Crypto fields, but now it
+				// actually honors them, an encrypted profile needs crypto.
+				// For now, effectively revert to that prior behavior by not
+				// specifying the profile.
+				ds.Profile = nil
+			}
+		}
+
 		copyTask, err = client.CopyVirtualDisk(
 			ctx,
 			srcFile.Path,
 			srcDatacenter,
 			dstFilePath,
 			dstDatacenter,
-			&vimtypes.FileBackedVirtualDiskSpec{
-				VirtualDiskSpec: vimtypes.VirtualDiskSpec{
-					AdapterType: string(vimtypes.VirtualDiskAdapterTypeLsiLogic),
-					DiskType:    string(vimtypes.VirtualDiskTypeThin),
-				},
-				SectorFormat: string(srcFile.DstDiskFormat),
-				// CopyVirtualDisk API simply ignores the Profile
-				// and Crypto parameters. We send neither until the
-				// API is enhanced to support both.
-			},
+			ds,
 			false)
 	} else {
 		logger.Info("Caching non-disk file")

pkg/util/vsphere/library/item_cache_test.go

@@ -29,10 +29,9 @@ type fakeCacheStorageURIsClient struct {
 	queryErr   error
 	queryCalls int32
 
-	copyErr          error
-	copyResult       *object.Task
-	copyCalls        int32
-	lastCopyDiskSpec vimtypes.BaseVirtualDiskSpec // Capture the spec passed to CopyVirtualDisk
+	copyErr    error
+	copyResult *object.Task
+	copyCalls  int32
 
 	makeErr   error
 	makeCalls int32
@@ -57,7 +56,6 @@ func (m *fakeCacheStorageURIsClient) CopyVirtualDisk(
 	dstSpec vimtypes.BaseVirtualDiskSpec, force bool) (*object.Task, error) {
 
 	_ = atomic.AddInt32(&m.copyCalls, 1)
-	m.lastCopyDiskSpec = dstSpec // Capture the spec for verification
 	return m.copyResult, m.copyErr
 }
 
@@ -319,21 +317,6 @@ var _ = Describe("CacheStorageURIs", func() {
 										Path: dstDir + "/" + "b020a5eae7f68a91d.vmdk",
 									}}))
 							})
-
-							It("should not send Profile parameter to CopyVirtualDisk", func() {
-								// Verify that CopyVirtualDisk was called
-								Expect(client.copyCalls).To(BeNumerically(">", 0))
-
-								// Verify the captured spec does not contain Profile parameter
-								Expect(client.lastCopyDiskSpec).ToNot(BeNil())
-
-								// Cast to FileBackedVirtualDiskSpec to check Profile field
-								if fbSpec, ok := client.lastCopyDiskSpec.(*vimtypes.FileBackedVirtualDiskSpec); ok {
-									// Profile should be nil or empty
-									Expect(fbSpec.Profile).To(BeEmpty(),
-										"Profile parameter should not be sent to CopyVirtualDisk API.")
-								}
-							})
 						})
 					})
 				})