Skip to content

Add metadata info in tag on PD #570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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"
Original file line number Diff line number Diff line change
Expand Up @@ -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
33 changes: 29 additions & 4 deletions pkg/common/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to set this regardless?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to retain the existing behavior (description set to Regional disk created by GCE-PD CSI Driver or Disk created by GCE-PD CSI Driver depending on disk) if the PVC/PV parameters aren't present.

}
return p, nil
}
16 changes: 15 additions & 1 deletion pkg/common/parameters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
DiskType: "pd-standard",
ReplicationType: "none",
DiskEncryptionKMSKey: "",
Tags: make(map[string]string),
},
},
{
Expand All @@ -44,6 +45,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
DiskType: "pd-standard",
ReplicationType: "none",
DiskEncryptionKMSKey: "",
Tags: make(map[string]string),
},
},
{
Expand All @@ -58,6 +60,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
DiskType: "pd-ssd",
ReplicationType: "regional-pd",
DiskEncryptionKMSKey: "foo/key",
Tags: make(map[string]string),
},
},
{
Expand All @@ -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)
}
Expand Down
56 changes: 50 additions & 6 deletions pkg/gce-cloud-provider/compute/gce-compute.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ limitations under the License.
package gcecloudprovider

import (
"encoding/json"
"fmt"
"strings"
"time"
Expand Down Expand Up @@ -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 != "" {
Expand Down Expand Up @@ -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),
}

Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another PR adding labels (#566). My plan is to merge this, and poke that PR to incorporate these as labels as well.

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
}
4 changes: 2 additions & 2 deletions pkg/gce-pd-csi-driver/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
36 changes: 36 additions & 0 deletions test/e2e/tests/single_zone_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down