From 714ef76d5d1f8f3beb1065bf0bbc0314036062a3 Mon Sep 17 00:00:00 2001 From: saad-ali Date: Thu, 6 Aug 2020 03:29:04 -0700 Subject: [PATCH] Add metadata info in tag on PD --- .../enable-extra-create-metadata.yaml | 4 ++ .../kustomization.yaml | 6 ++ pkg/common/parameters.go | 33 +++++++++-- pkg/common/parameters_test.go | 16 +++++- pkg/gce-cloud-provider/compute/gce-compute.go | 56 +++++++++++++++++-- pkg/gce-pd-csi-driver/controller.go | 4 +- test/e2e/tests/single_zone_e2e_test.go | 36 ++++++++++++ 7 files changed, 142 insertions(+), 13 deletions(-) create mode 100644 deploy/kubernetes/overlays/prow-gke-release-staging-head/enable-extra-create-metadata.yaml diff --git a/deploy/kubernetes/overlays/prow-gke-release-staging-head/enable-extra-create-metadata.yaml b/deploy/kubernetes/overlays/prow-gke-release-staging-head/enable-extra-create-metadata.yaml new file mode 100644 index 000000000..55abf2f8c --- /dev/null +++ b/deploy/kubernetes/overlays/prow-gke-release-staging-head/enable-extra-create-metadata.yaml @@ -0,0 +1,4 @@ +# Enable extra-create-metadata flag for external-provisioner +- op: add + path: /spec/template/spec/containers/0/args/- + value: "--extra-create-metadata" diff --git a/deploy/kubernetes/overlays/prow-gke-release-staging-head/kustomization.yaml b/deploy/kubernetes/overlays/prow-gke-release-staging-head/kustomization.yaml index 4bdc0e4ab..90581f8c8 100644 --- a/deploy/kubernetes/overlays/prow-gke-release-staging-head/kustomization.yaml +++ b/deploy/kubernetes/overlays/prow-gke-release-staging-head/kustomization.yaml @@ -23,3 +23,9 @@ patchesJson6902: kind: Deployment name: csi-gce-pd-controller path: increase-sidecar-operation-timeout.yaml +- target: + group: apps + version: v1 + kind: Deployment + name: csi-gce-pd-controller + path: enable-extra-create-metadata.yaml diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go index 26117316c..ba3dbcac1 100644 --- a/pkg/common/parameters.go +++ b/pkg/common/parameters.go @@ -27,6 +27,17 @@ const ( ParameterKeyDiskEncryptionKmsKey = "disk-encryption-kms-key" replicationTypeNone = "none" + + // Keys for PV and PVC parameters as reported by external-provisioner + ParameterKeyPVCName = "csi.storage.k8s.io/pvc/name" + ParameterKeyPVCNamespace = "csi.storage.k8s.io/pvc/namespace" + ParameterKeyPVName = "csi.storage.k8s.io/pv/name" + + // Keys for tags to attach to the provisioned disk. + tagKeyCreatedForClaimNamespace = "kubernetes.io/created-for/pvc/namespace" + tagKeyCreatedForClaimName = "kubernetes.io/created-for/pvc/name" + tagKeyCreatedForVolumeName = "kubernetes.io/created-for/pv/name" + tagKeyCreatedBy = "storage.gke.io/created-by" ) // DiskParameters contains normalized and defaulted disk parameters @@ -40,16 +51,21 @@ type DiskParameters struct { // Values: {string} // Default: "" DiskEncryptionKMSKey string + // Values: {map[string]string} + // Default: "" + Tags map[string]string } // ExtractAndDefaultParameters will take the relevant parameters from a map and // put them into a well defined struct making sure to default unspecified fields -func ExtractAndDefaultParameters(parameters map[string]string) (DiskParameters, error) { +func ExtractAndDefaultParameters(parameters map[string]string, driverName string) (DiskParameters, error) { p := DiskParameters{ - DiskType: "pd-standard", // Default - ReplicationType: replicationTypeNone, // Default - DiskEncryptionKMSKey: "", // Default + DiskType: "pd-standard", // Default + ReplicationType: replicationTypeNone, // Default + DiskEncryptionKMSKey: "", // Default + Tags: make(map[string]string), // Default } + for k, v := range parameters { if k == "csiProvisionerSecretName" || k == "csiProvisionerSecretNamespace" { // These are hardcoded secrets keys required to function but not needed by GCE PD @@ -67,9 +83,18 @@ func ExtractAndDefaultParameters(parameters map[string]string) (DiskParameters, case ParameterKeyDiskEncryptionKmsKey: // Resource names (e.g. "keyRings", "cryptoKeys", etc.) are case sensitive, so do not change case p.DiskEncryptionKMSKey = v + case ParameterKeyPVCName: + p.Tags[tagKeyCreatedForClaimName] = v + case ParameterKeyPVCNamespace: + p.Tags[tagKeyCreatedForClaimNamespace] = v + case ParameterKeyPVName: + p.Tags[tagKeyCreatedForVolumeName] = v default: return p, fmt.Errorf("parameters contains invalid option %q", k) } } + if len(p.Tags) > 0 { + p.Tags[tagKeyCreatedBy] = driverName + } return p, nil } diff --git a/pkg/common/parameters_test.go b/pkg/common/parameters_test.go index 825bb2141..4133ceb03 100644 --- a/pkg/common/parameters_test.go +++ b/pkg/common/parameters_test.go @@ -35,6 +35,7 @@ func TestExtractAndDefaultParameters(t *testing.T) { DiskType: "pd-standard", ReplicationType: "none", DiskEncryptionKMSKey: "", + Tags: make(map[string]string), }, }, { @@ -44,6 +45,7 @@ func TestExtractAndDefaultParameters(t *testing.T) { DiskType: "pd-standard", ReplicationType: "none", DiskEncryptionKMSKey: "", + Tags: make(map[string]string), }, }, { @@ -58,6 +60,7 @@ func TestExtractAndDefaultParameters(t *testing.T) { DiskType: "pd-ssd", ReplicationType: "regional-pd", DiskEncryptionKMSKey: "foo/key", + Tags: make(map[string]string), }, }, { @@ -67,13 +70,24 @@ func TestExtractAndDefaultParameters(t *testing.T) { DiskType: "pd-standard", ReplicationType: "none", DiskEncryptionKMSKey: "foo/key", + Tags: make(map[string]string), + }, + }, + { + name: "tags", + parameters: map[string]string{ParameterKeyPVCName: "testPVCName", ParameterKeyPVCNamespace: "testPVCNamespace", ParameterKeyPVName: "testPVName"}, + expectParams: DiskParameters{ + DiskType: "pd-standard", + ReplicationType: "none", + DiskEncryptionKMSKey: "", + Tags: map[string]string{tagKeyCreatedForClaimName: "testPVCName", tagKeyCreatedForClaimNamespace: "testPVCNamespace", tagKeyCreatedForVolumeName: "testPVName", tagKeyCreatedBy: "testDriver"}, }, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - p, err := ExtractAndDefaultParameters(tc.parameters) + p, err := ExtractAndDefaultParameters(tc.parameters, "testDriver") if gotErr := err != nil; gotErr != tc.expectErr { t.Fatalf("ExtractAndDefaultParameters(%+v) = %v; expectedErr: %v", tc.parameters, err, tc.expectErr) } diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index e738b734f..37c189c53 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -15,6 +15,7 @@ limitations under the License. package gcecloudprovider import ( + "encoding/json" "fmt" "strings" "time" @@ -268,21 +269,42 @@ func ValidateDiskParameters(disk *CloudDisk, params common.DiskParameters) error func (cloud *CloudProvider) InsertDisk(ctx context.Context, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string) error { klog.V(5).Infof("Inserting disk %v", volKey) + + description, err := encodeDiskTags(params.Tags) + if err != nil { + return err + } + switch volKey.Type() { case meta.Zonal: - return cloud.insertZonalDisk(ctx, volKey, params, capBytes, capacityRange, snapshotID) + if description == "" { + description = "Disk created by GCE-PD CSI Driver" + } + return cloud.insertZonalDisk(ctx, volKey, params, capBytes, capacityRange, snapshotID, description) case meta.Regional: - return cloud.insertRegionalDisk(ctx, volKey, params, capBytes, capacityRange, replicaZones, snapshotID) + if description == "" { + description = "Regional disk created by GCE-PD CSI Driver" + } + return cloud.insertRegionalDisk(ctx, volKey, params, capBytes, capacityRange, replicaZones, snapshotID, description) default: return fmt.Errorf("could not insert disk, key was neither zonal nor regional, instead got: %v", volKey.String()) } } -func (cloud *CloudProvider) insertRegionalDisk(ctx context.Context, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string) error { +func (cloud *CloudProvider) insertRegionalDisk( + ctx context.Context, + volKey *meta.Key, + params common.DiskParameters, + capBytes int64, + capacityRange *csi.CapacityRange, + replicaZones []string, + snapshotID string, + description string) error { + diskToCreate := &computev1.Disk{ Name: volKey.Name, SizeGb: common.BytesToGb(capBytes), - Description: "Regional disk created by GCE-PD CSI Driver", + Description: description, Type: cloud.GetDiskTypeURI(volKey, params.DiskType), } if snapshotID != "" { @@ -337,11 +359,18 @@ func (cloud *CloudProvider) insertRegionalDisk(ctx context.Context, volKey *meta return nil } -func (cloud *CloudProvider) insertZonalDisk(ctx context.Context, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, snapshotID string) error { +func (cloud *CloudProvider) insertZonalDisk( + ctx context.Context, + volKey *meta.Key, + params common.DiskParameters, + capBytes int64, + capacityRange *csi.CapacityRange, + snapshotID string, + description string) error { diskToCreate := &computev1.Disk{ Name: volKey.Name, SizeGb: common.BytesToGb(capBytes), - Description: "Disk created by GCE-PD CSI Driver", + Description: description, Type: cloud.GetDiskTypeURI(volKey, params.DiskType), } @@ -788,3 +817,18 @@ func removeCryptoKeyVersion(kmsKey string) string { } return kmsKey } + +// encodeDiskTags encodes requested volume tags into JSON string, as GCE does +// not support tags on GCE PDs and we use Description field as fallback. +func encodeDiskTags(tags map[string]string) (string, error) { + if len(tags) == 0 { + // No tags -> empty JSON + return "", nil + } + + enc, err := json.Marshal(tags) + if err != nil { + return "", fmt.Errorf("failed to encodeDiskTags %v: %v", tags, err) + } + return string(enc), nil +} diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 353b3f7fb..8be34ab68 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -110,7 +110,7 @@ func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.Cre // Apply Parameters (case-insensitive). We leave validation of // the values to the cloud provider. - params, err := common.ExtractAndDefaultParameters(req.GetParameters()) + params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err) } @@ -471,7 +471,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context } // Validate the disk parameters match the disk we GET - params, err := common.ExtractAndDefaultParameters(req.GetParameters()) + params, err := common.ExtractAndDefaultParameters(req.GetParameters(), gceCS.Driver.name) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err) } diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index 6035fda2d..51196ce0f 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -676,6 +676,42 @@ var _ = Describe("GCE PD CSI Driver", func() { Expect(err).To(BeNil(), "Failed to go through volume lifecycle") }) + It("Should successfully create disk with PVC/PV tags", func() { + Expect(testContexts).ToNot(BeEmpty()) + testContext := getRandomTestContext() + + controllerInstance := testContext.Instance + controllerClient := testContext.Client + + p, z, _ := controllerInstance.GetIdentity() + + // Create Disk + volName := testNamePrefix + string(uuid.NewUUID()) + volID, err := controllerClient.CreateVolume(volName, map[string]string{ + common.ParameterKeyPVCName: "test-pvc", + common.ParameterKeyPVCNamespace: "test-pvc-namespace", + common.ParameterKeyPVName: "test-pv-name", + }, defaultSizeGb, nil /* topReq */) + Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err) + + // Validate Disk Created + cloudDisk, err := computeService.Disks.Get(p, z, volName).Do() + Expect(err).To(BeNil(), "Could not get disk from cloud directly") + Expect(cloudDisk.Type).To(ContainSubstring(standardDiskType)) + Expect(cloudDisk.Status).To(Equal(readyState)) + Expect(cloudDisk.SizeGb).To(Equal(defaultSizeGb)) + Expect(cloudDisk.Name).To(Equal(volName)) + Expect(cloudDisk.Description).To(Equal("{\"kubernetes.io/created-for/pv/name\":\"test-pv-name\",\"kubernetes.io/created-for/pvc/name\":\"test-pvc\",\"kubernetes.io/created-for/pvc/namespace\":\"test-pvc-namespace\",\"storage.gke.io/created-by\":\"pd.csi.storage.gke.io\"}")) + defer func() { + // Delete Disk + controllerClient.DeleteVolume(volID) + Expect(err).To(BeNil(), "DeleteVolume failed") + + // Validate Disk Deleted + _, err = computeService.Disks.Get(p, z, volName).Do() + Expect(gce.IsGCEError(err, "notFound")).To(BeTrue(), "Expected disk to not be found") + }() + }) }) func equalWithinEpsilon(a, b, epsiolon int64) bool {