From fd1e856aa06850f883fd2e7275b8bc92f86accb5 Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Tue, 30 Apr 2024 09:42:44 +0200 Subject: [PATCH 1/7] Support worker config volume source image --- pkg/apis/gcp/types_worker.go | 3 +++ pkg/apis/gcp/v1alpha1/types_worker.go | 4 ++++ pkg/controller/worker/machines.go | 26 +++++++++++++++++++------- 3 files changed, 26 insertions(+), 7 deletions(-) diff --git a/pkg/apis/gcp/types_worker.go b/pkg/apis/gcp/types_worker.go index ab43096c5..6a4837821 100644 --- a/pkg/apis/gcp/types_worker.go +++ b/pkg/apis/gcp/types_worker.go @@ -44,6 +44,9 @@ type Volume struct { // Encryption refers to the disk encryption details for this volume Encryption *DiskEncryption + + // SourceImage is the image to create this disk + SourceImage *string } // DiskEncryption encapsulates the encryption configuration for a disk. diff --git a/pkg/apis/gcp/v1alpha1/types_worker.go b/pkg/apis/gcp/v1alpha1/types_worker.go index c6b1b295d..bd329d931 100644 --- a/pkg/apis/gcp/v1alpha1/types_worker.go +++ b/pkg/apis/gcp/v1alpha1/types_worker.go @@ -50,6 +50,10 @@ type Volume struct { // Encryption refers to the disk encryption details for this volume // +optional Encryption *DiskEncryption `json:"encryption,omitempty"` + + // SourceImage is the image to create this disk + // +optional + SourceImage *string `json:"sourceImage,omitempty"` } // DiskEncryption encapsulates the encryption configuration for a disk. diff --git a/pkg/controller/worker/machines.go b/pkg/controller/worker/machines.go index 670da761c..c96a734d2 100644 --- a/pkg/controller/worker/machines.go +++ b/pkg/controller/worker/machines.go @@ -127,7 +127,7 @@ func (w *workerDelegate) generateMachineConfig(ctx context.Context) error { disks := make([]map[string]interface{}, 0) // root volume if pool.Volume != nil { - disk, err := createDiskSpec(pool.Volume.Size, true, &machineImage, pool.Volume.Type, workerConfig, poolLabels) + disk, err := createDiskSpecForVolume(pool.Volume, machineImage, workerConfig, poolLabels) if err != nil { return err } @@ -136,9 +136,7 @@ func (w *workerDelegate) generateMachineConfig(ctx context.Context) error { // additional volumes for _, volume := range pool.DataVolumes { - // Don't set machine image for data volumes. Any pre-existing data on the disk can interfere with the boot disk. - // See https://github.com/gardener/gardener-extension-provider-gcp/issues/323 - disk, err := createDiskSpec(volume.Size, false, nil, volume.Type, workerConfig, poolLabels) + disk, err := createDiskSpecForDataVolume(volume, workerConfig, poolLabels) if err != nil { return err } @@ -280,7 +278,21 @@ func (w *workerDelegate) generateMachineConfig(ctx context.Context) error { return nil } -func createDiskSpec(size string, boot bool, machineImage, volumeType *string, workerConfig *apisgcp.WorkerConfig, labels map[string]interface{}) (map[string]interface{}, error) { +func createDiskSpecForVolume(volume *v1alpha1.Volume, image string, workerConfig *apisgcp.WorkerConfig, labels map[string]interface{}) (map[string]interface{}, error) { + return createDiskSpec(volume.Size, true, &image, volume.Type, workerConfig, labels) +} + +func createDiskSpecForDataVolume(volume v1alpha1.DataVolume, workerConfig *apisgcp.WorkerConfig, labels map[string]interface{}) (map[string]interface{}, error) { + // Careful when setting machine image for data volumes. Any pre-existing data on the disk can interfere with the boot disk. + // See https://github.com/gardener/gardener-extension-provider-gcp/issues/323 + var dataVolumeImage *string + if workerConfig.Volume != nil { + dataVolumeImage = workerConfig.Volume.SourceImage + } + return createDiskSpec(volume.Size, false, dataVolumeImage, volume.Type, workerConfig, labels) +} + +func createDiskSpec(size string, boot bool, image, volumeType *string, workerConfig *apisgcp.WorkerConfig, labels map[string]interface{}) (map[string]interface{}, error) { volumeSize, err := worker.DiskSize(size) if err != nil { return nil, err @@ -296,8 +308,8 @@ func createDiskSpec(size string, boot bool, machineImage, volumeType *string, wo disk["labels"] = labels } - if machineImage != nil { - disk["image"] = *machineImage + if image != nil { + disk["image"] = *image } if volumeType != nil { From c00c82208574d3f0eb8049ce44773a02581974f5 Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Tue, 30 Apr 2024 09:44:17 +0200 Subject: [PATCH 2/7] Make generate --- hack/api-reference/api.md | 12 ++++++++++++ pkg/apis/gcp/v1alpha1/zz_generated.conversion.go | 2 ++ pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go | 5 +++++ pkg/apis/gcp/zz_generated.deepcopy.go | 5 +++++ 4 files changed, 24 insertions(+) diff --git a/hack/api-reference/api.md b/hack/api-reference/api.md index b39d321fc..3d28c9709 100644 --- a/hack/api-reference/api.md +++ b/hack/api-reference/api.md @@ -1338,6 +1338,18 @@ DiskEncryption

Encryption refers to the disk encryption details for this volume

+ + +sourceImage
+ +string + + + +(Optional) +

SourceImage is the image to create this disk

+ +

WorkerStatus diff --git a/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go b/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go index c38b925cd..c24f70aac 100644 --- a/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go @@ -857,6 +857,7 @@ func Convert_gcp_VPC_To_v1alpha1_VPC(in *gcp.VPC, out *VPC, s conversion.Scope) func autoConvert_v1alpha1_Volume_To_gcp_Volume(in *Volume, out *gcp.Volume, s conversion.Scope) error { out.LocalSSDInterface = (*string)(unsafe.Pointer(in.LocalSSDInterface)) out.Encryption = (*gcp.DiskEncryption)(unsafe.Pointer(in.Encryption)) + out.SourceImage = (*string)(unsafe.Pointer(in.SourceImage)) return nil } @@ -868,6 +869,7 @@ func Convert_v1alpha1_Volume_To_gcp_Volume(in *Volume, out *gcp.Volume, s conver func autoConvert_gcp_Volume_To_v1alpha1_Volume(in *gcp.Volume, out *Volume, s conversion.Scope) error { out.LocalSSDInterface = (*string)(unsafe.Pointer(in.LocalSSDInterface)) out.Encryption = (*DiskEncryption)(unsafe.Pointer(in.Encryption)) + out.SourceImage = (*string)(unsafe.Pointer(in.SourceImage)) return nil } diff --git a/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go index 3ed9cf393..9b5c2dd07 100644 --- a/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go @@ -611,6 +611,11 @@ func (in *Volume) DeepCopyInto(out *Volume) { *out = new(DiskEncryption) (*in).DeepCopyInto(*out) } + if in.SourceImage != nil { + in, out := &in.SourceImage, &out.SourceImage + *out = new(string) + **out = **in + } return } diff --git a/pkg/apis/gcp/zz_generated.deepcopy.go b/pkg/apis/gcp/zz_generated.deepcopy.go index 2bd4d2715..c1eb08e9d 100644 --- a/pkg/apis/gcp/zz_generated.deepcopy.go +++ b/pkg/apis/gcp/zz_generated.deepcopy.go @@ -611,6 +611,11 @@ func (in *Volume) DeepCopyInto(out *Volume) { *out = new(DiskEncryption) (*in).DeepCopyInto(*out) } + if in.SourceImage != nil { + in, out := &in.SourceImage, &out.SourceImage + *out = new(string) + **out = **in + } return } From 648306b42745b1e3342f95b214be2f1c39560f4a Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Tue, 30 Apr 2024 15:38:08 +0200 Subject: [PATCH 3/7] Add docu for worker config sourceImage --- docs/usage/usage.md | 7 +++++++ pkg/apis/gcp/types_worker.go | 4 ++++ pkg/apis/gcp/v1alpha1/types_worker.go | 4 ++++ 3 files changed, 15 insertions(+) diff --git a/docs/usage/usage.md b/docs/usage/usage.md index bc5564870..205e4a1ee 100644 --- a/docs/usage/usage.md +++ b/docs/usage/usage.md @@ -155,6 +155,7 @@ The worker configuration contains: If you attach the disk with `SCRATCH` type, either an `NVMe` interface or a `SCSI` interface must be specified. It is only meaningful to provide this volume interface if only `SCRATCH` data volumes are used. + * Volume Encryption config that specifies values for `kmsKeyName` and `kmsKeyServiceAccountName`. * The `kmsKeyName` is the key name of the cloud kms disk encryption key and must be specified if CMEK disk encryption is needed. @@ -166,6 +167,12 @@ The worker configuration contains: gcloud projects add-iam-policy-binding projectId --member serviceAccount:name@projectIdgserviceaccount.com --role roles/cloudkms.cryptoKeyEncrypterDecrypter ``` + +* Setting a volume image with `volume.sourceImage`. + However, this parameter should only be used with particular caution. + For example Gardenlinux works with filesystem LABELs only and creating another disk form the very same image causes the LABELs to be duplicated. + See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323 + * Service Account with their specified scopes, authorized for this worker. Service accounts created in advance that generate access tokens that can be accessed through the metadata server and used to authenticate applications on the instance. diff --git a/pkg/apis/gcp/types_worker.go b/pkg/apis/gcp/types_worker.go index 6a4837821..ec7bf494e 100644 --- a/pkg/apis/gcp/types_worker.go +++ b/pkg/apis/gcp/types_worker.go @@ -46,6 +46,10 @@ type Volume struct { Encryption *DiskEncryption // SourceImage is the image to create this disk + // However, this parameter should only be used with particular caution. + // For example Gardenlinux works with filesystem LABELs only and creating + // another disk form the very same image causes the LABELs to be duplicated. + // See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323 SourceImage *string } diff --git a/pkg/apis/gcp/v1alpha1/types_worker.go b/pkg/apis/gcp/v1alpha1/types_worker.go index bd329d931..39b4d9b7b 100644 --- a/pkg/apis/gcp/v1alpha1/types_worker.go +++ b/pkg/apis/gcp/v1alpha1/types_worker.go @@ -52,6 +52,10 @@ type Volume struct { Encryption *DiskEncryption `json:"encryption,omitempty"` // SourceImage is the image to create this disk + // However, this parameter should only be used with particular caution. + // For example Gardenlinux works with filesystem LABELs only and creating + // another disk form the very same image causes the LABELs to be duplicated. + // See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323 // +optional SourceImage *string `json:"sourceImage,omitempty"` } From 43cf20d56caa4fbfee2e9a373e17425cd0b11bce Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Tue, 30 Apr 2024 15:50:01 +0200 Subject: [PATCH 4/7] Make generate --- hack/api-reference/api.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hack/api-reference/api.md b/hack/api-reference/api.md index 3d28c9709..5ed235887 100644 --- a/hack/api-reference/api.md +++ b/hack/api-reference/api.md @@ -1347,7 +1347,11 @@ string (Optional) -

SourceImage is the image to create this disk

+

SourceImage is the image to create this disk +However, this parameter should only be used with particular caution. +For example Gardenlinux works with filesystem LABELs only and creating +another disk form the very same image causes the LABELs to be duplicated. +See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323

From 9802b7695bcb62ee4c2980cd8f3834fea9e9eb7c Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Fri, 10 May 2024 11:17:26 +0200 Subject: [PATCH 5/7] Move imageSource to DataVolume config --- hack/api-reference/api.md | 75 +++++++++++++++---- pkg/apis/gcp/types_worker.go | 12 ++- pkg/apis/gcp/v1alpha1/types_worker.go | 13 +++- .../gcp/v1alpha1/zz_generated.conversion.go | 36 ++++++++- .../gcp/v1alpha1/zz_generated.deepcopy.go | 33 ++++++-- pkg/apis/gcp/validation/worker.go | 51 +++++++++++-- pkg/apis/gcp/validation/worker_test.go | 24 ++++++ pkg/apis/gcp/zz_generated.deepcopy.go | 33 ++++++-- pkg/controller/worker/machines.go | 14 +++- 9 files changed, 249 insertions(+), 42 deletions(-) diff --git a/hack/api-reference/api.md b/hack/api-reference/api.md index 5ed235887..a42e0b3ea 100644 --- a/hack/api-reference/api.md +++ b/hack/api-reference/api.md @@ -240,6 +240,20 @@ Volume +dataVolumes
+ + +[]DataVolume + + + + +(Optional) +

DataVolumes contains configuration for the additional disks attached to VMs.

+ + + + minCpuPlatform
string @@ -487,6 +501,51 @@ string +

DataVolume +

+

+(Appears on: +WorkerConfig) +

+

+

DataVolume contains configuration for data volumes attached to VMs.

+

+ + + + + + + + + + + + + + + + + +
FieldDescription
+name
+ +string + +		 +

Name is the name of the data volume this configuration applies to.

+
+sourceImage
+ +string + +		 +

SourceImage is the image to create this disk +However, this parameter should only be used with particular caution. +For example GardenLinux works with filesystem LABELs only and creating +another disk form the very same image causes the LABELs to be duplicated. +See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323

+

DiskEncryption

@@ -1338,22 +1397,6 @@ DiskEncryption

Encryption refers to the disk encryption details for this volume

- - -sourceImage
- -string - - - -(Optional) -

SourceImage is the image to create this disk -However, this parameter should only be used with particular caution. -For example Gardenlinux works with filesystem LABELs only and creating -another disk form the very same image causes the LABELs to be duplicated. -See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323

- -

WorkerStatus diff --git a/pkg/apis/gcp/types_worker.go b/pkg/apis/gcp/types_worker.go index ec7bf494e..18571012d 100644 --- a/pkg/apis/gcp/types_worker.go +++ b/pkg/apis/gcp/types_worker.go @@ -22,6 +22,10 @@ type WorkerConfig struct { // Volume contains configuration for the root disks attached to VMs. Volume *Volume + // DataVolumes contains configuration for the additional disks attached to VMs. + // +optional + DataVolumes []DataVolume + // MinCpuPlatform is the name of the minimum CPU platform that is to be // requested for the VM. MinCpuPlatform *string @@ -44,10 +48,16 @@ type Volume struct { // Encryption refers to the disk encryption details for this volume Encryption *DiskEncryption +} + +// DataVolume contains configuration for data volumes attached to VMs. +type DataVolume struct { + // Name is the name of the data volume this configuration applies to. + Name string // SourceImage is the image to create this disk // However, this parameter should only be used with particular caution. - // For example Gardenlinux works with filesystem LABELs only and creating + // For example GardenLinux works with filesystem LABELs only and creating // another disk form the very same image causes the LABELs to be duplicated. // See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323 SourceImage *string diff --git a/pkg/apis/gcp/v1alpha1/types_worker.go b/pkg/apis/gcp/v1alpha1/types_worker.go index 39b4d9b7b..caa20411c 100644 --- a/pkg/apis/gcp/v1alpha1/types_worker.go +++ b/pkg/apis/gcp/v1alpha1/types_worker.go @@ -24,6 +24,10 @@ type WorkerConfig struct { // +optional Volume *Volume `json:"volume,omitempty"` + // DataVolumes contains configuration for the additional disks attached to VMs. + // +optional + DataVolumes []DataVolume `json:"dataVolumes,omitempty"` + // MinCpuPlatform is the name of the minimum CPU platform that is to be // requested for the VM. MinCpuPlatform *string `json:"minCpuPlatform,omitempty"` @@ -50,13 +54,18 @@ type Volume struct { // Encryption refers to the disk encryption details for this volume // +optional Encryption *DiskEncryption `json:"encryption,omitempty"` +} + +// DataVolume contains configuration for data volumes attached to VMs. +type DataVolume struct { + // Name is the name of the data volume this configuration applies to. + Name string `json:"name"` // SourceImage is the image to create this disk // However, this parameter should only be used with particular caution. - // For example Gardenlinux works with filesystem LABELs only and creating + // For example GardenLinux works with filesystem LABELs only and creating // another disk form the very same image causes the LABELs to be duplicated. // See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323 - // +optional SourceImage *string `json:"sourceImage,omitempty"` } diff --git a/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go b/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go index c24f70aac..2a385f32b 100644 --- a/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/gcp/v1alpha1/zz_generated.conversion.go @@ -75,6 +75,16 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddGeneratedConversionFunc((*DataVolume)(nil), (*gcp.DataVolume)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1alpha1_DataVolume_To_gcp_DataVolume(a.(*DataVolume), b.(*gcp.DataVolume), scope) + }); err != nil { + return err + } + if err := s.AddGeneratedConversionFunc((*gcp.DataVolume)(nil), (*DataVolume)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_gcp_DataVolume_To_v1alpha1_DataVolume(a.(*gcp.DataVolume), b.(*DataVolume), scope) + }); err != nil { + return err + } if err := s.AddGeneratedConversionFunc((*DiskEncryption)(nil), (*gcp.DiskEncryption)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1alpha1_DiskEncryption_To_gcp_DiskEncryption(a.(*DiskEncryption), b.(*gcp.DiskEncryption), scope) }); err != nil { @@ -410,6 +420,28 @@ func Convert_gcp_ControlPlaneConfig_To_v1alpha1_ControlPlaneConfig(in *gcp.Contr return autoConvert_gcp_ControlPlaneConfig_To_v1alpha1_ControlPlaneConfig(in, out, s) } +func autoConvert_v1alpha1_DataVolume_To_gcp_DataVolume(in *DataVolume, out *gcp.DataVolume, s conversion.Scope) error { + out.Name = in.Name + out.SourceImage = (*string)(unsafe.Pointer(in.SourceImage)) + return nil +} + +// Convert_v1alpha1_DataVolume_To_gcp_DataVolume is an autogenerated conversion function. +func Convert_v1alpha1_DataVolume_To_gcp_DataVolume(in *DataVolume, out *gcp.DataVolume, s conversion.Scope) error { + return autoConvert_v1alpha1_DataVolume_To_gcp_DataVolume(in, out, s) +} + +func autoConvert_gcp_DataVolume_To_v1alpha1_DataVolume(in *gcp.DataVolume, out *DataVolume, s conversion.Scope) error { + out.Name = in.Name + out.SourceImage = (*string)(unsafe.Pointer(in.SourceImage)) + return nil +} + +// Convert_gcp_DataVolume_To_v1alpha1_DataVolume is an autogenerated conversion function. +func Convert_gcp_DataVolume_To_v1alpha1_DataVolume(in *gcp.DataVolume, out *DataVolume, s conversion.Scope) error { + return autoConvert_gcp_DataVolume_To_v1alpha1_DataVolume(in, out, s) +} + func autoConvert_v1alpha1_DiskEncryption_To_gcp_DiskEncryption(in *DiskEncryption, out *gcp.DiskEncryption, s conversion.Scope) error { out.KmsKeyName = (*string)(unsafe.Pointer(in.KmsKeyName)) out.KmsKeyServiceAccount = (*string)(unsafe.Pointer(in.KmsKeyServiceAccount)) @@ -857,7 +889,6 @@ func Convert_gcp_VPC_To_v1alpha1_VPC(in *gcp.VPC, out *VPC, s conversion.Scope) func autoConvert_v1alpha1_Volume_To_gcp_Volume(in *Volume, out *gcp.Volume, s conversion.Scope) error { out.LocalSSDInterface = (*string)(unsafe.Pointer(in.LocalSSDInterface)) out.Encryption = (*gcp.DiskEncryption)(unsafe.Pointer(in.Encryption)) - out.SourceImage = (*string)(unsafe.Pointer(in.SourceImage)) return nil } @@ -869,7 +900,6 @@ func Convert_v1alpha1_Volume_To_gcp_Volume(in *Volume, out *gcp.Volume, s conver func autoConvert_gcp_Volume_To_v1alpha1_Volume(in *gcp.Volume, out *Volume, s conversion.Scope) error { out.LocalSSDInterface = (*string)(unsafe.Pointer(in.LocalSSDInterface)) out.Encryption = (*DiskEncryption)(unsafe.Pointer(in.Encryption)) - out.SourceImage = (*string)(unsafe.Pointer(in.SourceImage)) return nil } @@ -881,6 +911,7 @@ func Convert_gcp_Volume_To_v1alpha1_Volume(in *gcp.Volume, out *Volume, s conver func autoConvert_v1alpha1_WorkerConfig_To_gcp_WorkerConfig(in *WorkerConfig, out *gcp.WorkerConfig, s conversion.Scope) error { out.GPU = (*gcp.GPU)(unsafe.Pointer(in.GPU)) out.Volume = (*gcp.Volume)(unsafe.Pointer(in.Volume)) + out.DataVolumes = *(*[]gcp.DataVolume)(unsafe.Pointer(&in.DataVolumes)) out.MinCpuPlatform = (*string)(unsafe.Pointer(in.MinCpuPlatform)) out.ServiceAccount = (*gcp.ServiceAccount)(unsafe.Pointer(in.ServiceAccount)) out.NodeTemplate = (*extensionsv1alpha1.NodeTemplate)(unsafe.Pointer(in.NodeTemplate)) @@ -895,6 +926,7 @@ func Convert_v1alpha1_WorkerConfig_To_gcp_WorkerConfig(in *WorkerConfig, out *gc func autoConvert_gcp_WorkerConfig_To_v1alpha1_WorkerConfig(in *gcp.WorkerConfig, out *WorkerConfig, s conversion.Scope) error { out.GPU = (*GPU)(unsafe.Pointer(in.GPU)) out.Volume = (*Volume)(unsafe.Pointer(in.Volume)) + out.DataVolumes = *(*[]DataVolume)(unsafe.Pointer(&in.DataVolumes)) out.MinCpuPlatform = (*string)(unsafe.Pointer(in.MinCpuPlatform)) out.ServiceAccount = (*ServiceAccount)(unsafe.Pointer(in.ServiceAccount)) out.NodeTemplate = (*extensionsv1alpha1.NodeTemplate)(unsafe.Pointer(in.NodeTemplate)) diff --git a/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go index 9b5c2dd07..257e5a67d 100644 --- a/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/gcp/v1alpha1/zz_generated.deepcopy.go @@ -181,6 +181,27 @@ func (in *ControlPlaneConfig) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DataVolume) DeepCopyInto(out *DataVolume) { + *out = *in + if in.SourceImage != nil { + in, out := &in.SourceImage, &out.SourceImage + *out = new(string) + **out = **in + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataVolume. +func (in *DataVolume) DeepCopy() *DataVolume { + if in == nil { + return nil + } + out := new(DataVolume) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DiskEncryption) DeepCopyInto(out *DiskEncryption) { *out = *in @@ -611,11 +632,6 @@ func (in *Volume) DeepCopyInto(out *Volume) { *out = new(DiskEncryption) (*in).DeepCopyInto(*out) } - if in.SourceImage != nil { - in, out := &in.SourceImage, &out.SourceImage - *out = new(string) - **out = **in - } return } @@ -643,6 +659,13 @@ func (in *WorkerConfig) DeepCopyInto(out *WorkerConfig) { *out = new(Volume) (*in).DeepCopyInto(*out) } + if in.DataVolumes != nil { + in, out := &in.DataVolumes, &out.DataVolumes + *out = make([]DataVolume, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.MinCpuPlatform != nil { in, out := &in.MinCpuPlatform, &out.MinCpuPlatform *out = new(string) diff --git a/pkg/apis/gcp/validation/worker.go b/pkg/apis/gcp/validation/worker.go index 1b38b1762..5f2d6cb69 100644 --- a/pkg/apis/gcp/validation/worker.go +++ b/pkg/apis/gcp/validation/worker.go @@ -10,6 +10,7 @@ import ( "github.com/gardener/gardener/pkg/apis/core" extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" + "golang.org/x/exp/slices" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/util/sets" @@ -22,8 +23,9 @@ import ( var ( validVolumeLocalSSDInterfacesTypes = sets.New("NVME", "SCSI") - providerFldPath = field.NewPath("providerConfig") - volumeFldPath = providerFldPath.Child("volume") + providerFldPath = field.NewPath("providerConfig") + volumeFldPath = providerFldPath.Child("volume") + dataVolumeFldPath = providerFldPath.Child("dataVolume") ) // ValidateWorkerConfig validates a WorkerConfig object. @@ -42,6 +44,9 @@ func ValidateWorkerConfig(workerConfig *gcp.WorkerConfig, dataVolumes []core.Dat allErrs = append(allErrs, validateDiskEncryption(workerConfig.Volume.Encryption, volumeFldPath.Child("encryption"))...) } allErrs = append(allErrs, validateNodeTemplate(workerConfig.NodeTemplate, providerFldPath.Child("nodeTemplate"))...) + if workerConfig.DataVolumes != nil { + allErrs = append(allErrs, validateDataVolumeNames(workerConfig.DataVolumes, dataVolumes)...) + } } return allErrs @@ -120,22 +125,54 @@ func validateDataVolume(workerConfig *gcp.WorkerConfig, volume core.DataVolume, allErrs = append(allErrs, field.Required(fldPath.Child("type"), "must not be empty")) return allErrs } - if *volume.Type == worker.VolumeTypeScratch { + + allErrs = append(allErrs, validateScratchDisk(*volume.Type, workerConfig)...) + + return allErrs +} + +func validateScratchDisk(volumeType string, workerConfig *gcp.WorkerConfig) field.ErrorList { + allErrs := field.ErrorList{} + + interfacePath := volumeFldPath.Child("interface") + encryptionPath := volumeFldPath.Child("encryption") + + if volumeType == worker.VolumeTypeScratch { if workerConfig == nil || workerConfig.Volume == nil || workerConfig.Volume.LocalSSDInterface == nil { - allErrs = append(allErrs, field.Required(volumeFldPath.Child("interface"), fmt.Sprintf("must be set when using %s volumes", worker.VolumeTypeScratch))) + allErrs = append(allErrs, field.Required(interfacePath, fmt.Sprintf("must be set when using %s volumes", worker.VolumeTypeScratch))) } else { if !validVolumeLocalSSDInterfacesTypes.Has(*workerConfig.Volume.LocalSSDInterface) { - allErrs = append(allErrs, field.NotSupported(volumeFldPath.Child("interface"), *workerConfig.Volume.LocalSSDInterface, validVolumeLocalSSDInterfacesTypes.UnsortedList())) + allErrs = append(allErrs, field.NotSupported(interfacePath, *workerConfig.Volume.LocalSSDInterface, validVolumeLocalSSDInterfacesTypes.UnsortedList())) } } // DiskEncryption not allowed for type SCRATCH if workerConfig != nil && workerConfig.Volume != nil && workerConfig.Volume.Encryption != nil { - allErrs = append(allErrs, field.Invalid(volumeFldPath.Child("encryption"), *workerConfig.Volume.Encryption, fmt.Sprintf("must not be set in combination with %s volumes", worker.VolumeTypeScratch))) + allErrs = append(allErrs, field.Invalid(encryptionPath, *workerConfig.Volume.Encryption, fmt.Sprintf("must not be set in combination with %s volumes", worker.VolumeTypeScratch))) } } else { // LocalSSDInterface only allowed for type SCRATCH if workerConfig != nil && workerConfig.Volume != nil && workerConfig.Volume.LocalSSDInterface != nil { - allErrs = append(allErrs, field.Invalid(volumeFldPath.Child("interface"), *workerConfig.Volume.LocalSSDInterface, fmt.Sprintf("is only allowed for type %s", worker.VolumeTypeScratch))) + allErrs = append(allErrs, field.Invalid(encryptionPath, *workerConfig.Volume.LocalSSDInterface, fmt.Sprintf("is only allowed for type %s", worker.VolumeTypeScratch))) + } + } + return allErrs +} + +func validateDataVolumeNames(workerConfigDataVolumes []gcp.DataVolume, dataVolumes []core.DataVolume) field.ErrorList { + allErrs := field.ErrorList{} + + // Extracting a dataVolume names + var dataVolumeNames []string + for _, dv := range dataVolumes { + dataVolumeNames = append(dataVolumeNames, dv.Name) + } + + for _, configDataVolume := range workerConfigDataVolumes { + if !slices.Contains(dataVolumeNames, configDataVolume.Name) { + allErrs = append(allErrs, field.Invalid( + dataVolumeFldPath, + configDataVolume.Name, + fmt.Sprintf("could not find dataVolume with name %s", configDataVolume.Name))) } } diff --git a/pkg/apis/gcp/validation/worker_test.go b/pkg/apis/gcp/validation/worker_test.go index d2686a785..9f9adc453 100644 --- a/pkg/apis/gcp/validation/worker_test.go +++ b/pkg/apis/gcp/validation/worker_test.go @@ -37,6 +37,7 @@ var _ = Describe("#ValidateWorkers", func() { }, DataVolumes: []core.DataVolume{ { + Name: "foo", Type: ptr.To("Volume"), VolumeSize: "30G", }, @@ -280,6 +281,29 @@ var _ = Describe("#ValidateWorkers", func() { Expect(errorList).To(BeEmpty()) }) + It("should allow valid dataVolume name", func() { + errorList := validateWorkerConfig([]core.Worker{workers[0]}, &gcp.WorkerConfig{ + DataVolumes: []gcp.DataVolume{{ + Name: "foo", + }}, + }) + Expect(errorList).To(BeEmpty()) + }) + + It("should forbid invalid dataVolume name", func() { + errorList := validateWorkerConfig([]core.Worker{workers[0]}, &gcp.WorkerConfig{ + DataVolumes: []gcp.DataVolume{{ + Name: "foo-invalid", + }}, + }) + Expect(errorList).To(ConsistOf( + PointTo(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(field.ErrorTypeInvalid), + "Field": Equal("providerConfig.dataVolume"), + })), + )) + }) + Describe("#Volume type SCRATCH", func() { It("should pass because worker config is configured correctly", func() { workers[0].DataVolumes[0].Type = ptr.To(worker.VolumeTypeScratch) diff --git a/pkg/apis/gcp/zz_generated.deepcopy.go b/pkg/apis/gcp/zz_generated.deepcopy.go index c1eb08e9d..1ed03df3a 100644 --- a/pkg/apis/gcp/zz_generated.deepcopy.go +++ b/pkg/apis/gcp/zz_generated.deepcopy.go @@ -181,6 +181,27 @@ func (in *ControlPlaneConfig) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DataVolume) DeepCopyInto(out *DataVolume) { + *out = *in + if in.SourceImage != nil { + in, out := &in.SourceImage, &out.SourceImage + *out = new(string) + **out = **in + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataVolume. +func (in *DataVolume) DeepCopy() *DataVolume { + if in == nil { + return nil + } + out := new(DataVolume) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DiskEncryption) DeepCopyInto(out *DiskEncryption) { *out = *in @@ -611,11 +632,6 @@ func (in *Volume) DeepCopyInto(out *Volume) { *out = new(DiskEncryption) (*in).DeepCopyInto(*out) } - if in.SourceImage != nil { - in, out := &in.SourceImage, &out.SourceImage - *out = new(string) - **out = **in - } return } @@ -643,6 +659,13 @@ func (in *WorkerConfig) DeepCopyInto(out *WorkerConfig) { *out = new(Volume) (*in).DeepCopyInto(*out) } + if in.DataVolumes != nil { + in, out := &in.DataVolumes, &out.DataVolumes + *out = make([]DataVolume, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } if in.MinCpuPlatform != nil { in, out := &in.MinCpuPlatform, &out.MinCpuPlatform *out = new(string) diff --git a/pkg/controller/worker/machines.go b/pkg/controller/worker/machines.go index c96a734d2..9313d409e 100644 --- a/pkg/controller/worker/machines.go +++ b/pkg/controller/worker/machines.go @@ -285,10 +285,7 @@ func createDiskSpecForVolume(volume *v1alpha1.Volume, image string, workerConfig func createDiskSpecForDataVolume(volume v1alpha1.DataVolume, workerConfig *apisgcp.WorkerConfig, labels map[string]interface{}) (map[string]interface{}, error) { // Careful when setting machine image for data volumes. Any pre-existing data on the disk can interfere with the boot disk. // See https://github.com/gardener/gardener-extension-provider-gcp/issues/323 - var dataVolumeImage *string - if workerConfig.Volume != nil { - dataVolumeImage = workerConfig.Volume.SourceImage - } + dataVolumeImage := getDataVolumeImage(volume.Name, workerConfig.DataVolumes) return createDiskSpec(volume.Size, false, dataVolumeImage, volume.Type, workerConfig, labels) } @@ -342,6 +339,15 @@ func addDiskEncryptionDetails(disk map[string]interface{}, encryption *apisgcp.D disk["encryption"] = encryptionMap } +func getDataVolumeImage(volumeName string, dataVolumes []apisgcp.DataVolume) *string { + for _, dv := range dataVolumes { + if dv.Name == volumeName { + return dv.SourceImage + } + } + return nil +} + func getGcePoolLabels(worker *v1alpha1.Worker, pool v1alpha1.WorkerPool) map[string]interface{} { gceInstanceLabels := map[string]interface{}{ "name": SanitizeGcpLabelValue(worker.Name), From bfc9b2e5ee5ffac0c2fedc3602619020b3265703 Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Fri, 10 May 2024 11:22:25 +0200 Subject: [PATCH 6/7] Adjust docu --- docs/usage/usage.md | 2 +- example/30-worker.yaml | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/usage/usage.md b/docs/usage/usage.md index 205e4a1ee..bc35217a6 100644 --- a/docs/usage/usage.md +++ b/docs/usage/usage.md @@ -168,7 +168,7 @@ The worker configuration contains: serviceAccount:name@projectIdgserviceaccount.com --role roles/cloudkms.cryptoKeyEncrypterDecrypter ``` -* Setting a volume image with `volume.sourceImage`. +* Setting a volume image with `dataVolume.sourceImage`. However, this parameter should only be used with particular caution. For example Gardenlinux works with filesystem LABELs only and creating another disk form the very same image causes the LABELs to be duplicated. See: https://github.com/gardener/gardener-extension-provider-gcp/issues/323 diff --git a/example/30-worker.yaml b/example/30-worker.yaml index e35a4c042..3923ffbf7 100644 --- a/example/30-worker.yaml +++ b/example/30-worker.yaml @@ -95,6 +95,8 @@ spec: # encryption: # Encryption Details # kmsKeyName: "projects/projectId/locations//keyRings//cryptoKeys/alpha" # kmsKeyServiceAccount: "user@projectId.iam.gserviceaccount.com" + # dataVolume: # provider specific dataVolume configuration + # sourceImage: "projects/sap-se-gcp-gardenlinux/global/images/..." # gpu: # acceleratorType: nvidia-tesla-t4 # count: 1 From 3dccc37a880ffa9836bc16e549372a9ef1b0d1c0 Mon Sep 17 00:00:00 2001 From: Alexander Hebel Date: Fri, 14 Jun 2024 14:57:44 +0200 Subject: [PATCH 7/7] Use slice.ContainsFunc in validation --- pkg/apis/gcp/validation/worker.go | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/pkg/apis/gcp/validation/worker.go b/pkg/apis/gcp/validation/worker.go index 5f2d6cb69..2a9e774e9 100644 --- a/pkg/apis/gcp/validation/worker.go +++ b/pkg/apis/gcp/validation/worker.go @@ -161,19 +161,16 @@ func validateScratchDisk(volumeType string, workerConfig *gcp.WorkerConfig) fiel func validateDataVolumeNames(workerConfigDataVolumes []gcp.DataVolume, dataVolumes []core.DataVolume) field.ErrorList { allErrs := field.ErrorList{} - // Extracting a dataVolume names - var dataVolumeNames []string - for _, dv := range dataVolumes { - dataVolumeNames = append(dataVolumeNames, dv.Name) - } - for _, configDataVolume := range workerConfigDataVolumes { - if !slices.Contains(dataVolumeNames, configDataVolume.Name) { - allErrs = append(allErrs, field.Invalid( - dataVolumeFldPath, - configDataVolume.Name, - fmt.Sprintf("could not find dataVolume with name %s", configDataVolume.Name))) + if slices.ContainsFunc(dataVolumes, func(dataVolume core.DataVolume) bool { + return configDataVolume.Name == dataVolume.Name + }) { + continue } + allErrs = append(allErrs, field.Invalid( + dataVolumeFldPath, + configDataVolume.Name, + fmt.Sprintf("could not find dataVolume with name %s", configDataVolume.Name))) } return allErrs