diff --git a/controllers/virtualmachineimagecache/virtualmachineimagecache_controller.go b/controllers/virtualmachineimagecache/virtualmachineimagecache_controller.go index 8821a72d6..75df884c0 100644 --- a/controllers/virtualmachineimagecache/virtualmachineimagecache_controller.go +++ b/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, diff --git a/pkg/providers/vsphere/vmlifecycle/create_fastdeploy.go b/pkg/providers/vsphere/vmlifecycle/create_fastdeploy.go index 176bd921e..1f250b2e5 100644 --- a/pkg/providers/vsphere/vmlifecycle/create_fastdeploy.go +++ b/pkg/providers/vsphere/vmlifecycle/create_fastdeploy.go @@ -446,6 +446,7 @@ func fastDeployDirect( ctx, logger, datacenter, + configSpec, srcDiskPaths, dstDiskPaths, diskFormat); err != nil { @@ -453,26 +454,12 @@ func fastDeployDirect( 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()) ) diff --git a/pkg/util/vsphere/library/item_cache.go b/pkg/util/vsphere/library/item_cache.go index 021da75ed..2be520ca0 100644 --- a/pkg/util/vsphere/library/item_cache.go +++ b/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") diff --git a/pkg/util/vsphere/library/item_cache_test.go b/pkg/util/vsphere/library/item_cache_test.go index 47892b931..dda32d8fb 100644 --- a/pkg/util/vsphere/library/item_cache_test.go +++ b/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.") - } - }) }) }) })