Add gce disk labels support via create volume parameters

Add labels validation to match gce requirements

add e2e test case for labels

Author: mattcary

README.md

@@ -68,6 +68,7 @@ See Github [Issues](https://github.com/kubernetes-sigs/gcp-compute-persistent-di
 | type             | `pd-ssd` OR `pd-standard` | `pd-standard` | Type allows you to choose between standard Persistent Disks  or Solid State Drive Persistent Disks |
 | replication-type | `none` OR `regional-pd`   | `none`        | Replication type allows you to choose between Zonal Persistent Disks or Regional Persistent Disks  |
 | disk-encryption-kms-key | Fully qualified resource identifier for the key to use to encrypt new disks. | Empty string. | Encrypt disk using Customer Managed Encryption Key (CMEK). See [GKE Docs](https://cloud.google.com/kubernetes-engine/docs/how-to/using-cmek#create_a_cmek_protected_attached_disk) for details. |
+| labels           | `key1=value1,key2=value2` |               | Labels allow you to assign custom [GCE Disk labels](https://cloud.google.com/compute/docs/labeling-resources). |
 
 ### Topology
 

cmd/gce-pd-csi-driver/main.go

@@ -24,7 +24,7 @@ import (
 
 	"k8s.io/klog"
 
-	cliflag "k8s.io/component-base/cli/flag"
+	"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
 	gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute"
 	metadataservice "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/metadata"
 	driver "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-pd-csi-driver"
@@ -36,7 +36,7 @@ var (
 	endpoint             = flag.String("endpoint", "unix:/tmp/csi.sock", "CSI endpoint")
 	runControllerService = flag.Bool("run-controller-service", true, "If set to false then the CSI driver does not activate its controller service (default: true)")
 	runNodeService       = flag.Bool("run-node-service", true, "If set to false then the CSI driver does not activate its node service (default: true)")
-	extraVolumeLabels    map[string]string
+	extraVolumeLabelsStr = flag.String("extra-labels", "", "Extra labels to attach to each PD created. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'. See https://cloud.google.com/compute/docs/labeling-resources for details")
 	version              string
 )
 
@@ -55,7 +55,6 @@ func init() {
 }
 
 func main() {
-	flag.Var(cliflag.NewMapStringString(&extraVolumeLabels), "extra-labels", "Extra labels to attach to each PD created. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
 	flag.Parse()
 	rand.Seed(time.Now().UnixNano())
 	handle()
@@ -70,6 +69,14 @@ func handle() {
 	}
 	klog.V(2).Infof("Driver vendor version %v", version)
 
+	if len(*extraVolumeLabelsStr) > 0 && !*runControllerService {
+		klog.Fatalf("Extra volume labels provided but not running controller")
+	}
+	extraVolumeLabels, err := common.ConvertLabelsStringToMap(*extraVolumeLabelsStr)
+	if err != nil {
+		klog.Fatalf("Bad extra volume labels: %v", err)
+	}
+
 	gceDriver := driver.GetGCEDriver()
 
 	//Initialize GCE Driver

deploy/kubernetes/overlays/prow-gke-release-staging-rc-master/disk_labels.yaml

@@ -0,0 +1,4 @@
+# for gce-pd-driver
+- op: add
+  path: /spec/template/spec/containers/4/args/2
+  value: "--extra-labels=csi=gce-pd-driver  # Used for testing, not to be merged to stable"

deploy/kubernetes/overlays/prow-gke-release-staging-rc-master/kustomization.yaml

@@ -4,5 +4,13 @@ namespace:
   gce-pd-csi-driver
 resources:
 - ../stable-master
+patchesJson6902:
+# disk_labels.yaml is used for testing and should not be merged to stable.
+- target:
+    group: apps
+    version: v1
+    kind: Deployment
+    name: csi-gce-pd-controller
+  path: disk_labels.yaml
 transformers:
 - ../../images/prow-gke-release-staging-rc-master

examples/kubernetes/demo-labeled-sc.yaml

@@ -0,0 +1,8 @@
+apiVersion: storage.k8s.io/v1
+kind: StorageClass
+metadata:
+  name: csi-gce-pd
+provisioner: pd.csi.storage.gke.io
+parameters:
+  labels: key1=value1,key2=value2
+volumeBindingMode: WaitForFirstConsumer

pkg/common/parameters.go

@@ -25,6 +25,7 @@ const (
 	ParameterKeyType                 = "type"
 	ParameterKeyReplicationType      = "replication-type"
 	ParameterKeyDiskEncryptionKmsKey = "disk-encryption-kms-key"
+	ParameterKeyLabels               = "labels"
 
 	replicationTypeNone = "none"
 
@@ -33,7 +34,7 @@ const (
 	ParameterKeyPVCNamespace = "csi.storage.k8s.io/pvc/namespace"
 	ParameterKeyPVName       = "csi.storage.k8s.io/pv/name"
 
-	// Keys for tags to attach to the provisioned disk.
+	// Keys for tags to put in the provisioned disk description.
 	tagKeyCreatedForClaimNamespace = "kubernetes.io/created-for/pvc/namespace"
 	tagKeyCreatedForClaimName      = "kubernetes.io/created-for/pvc/name"
 	tagKeyCreatedForVolumeName     = "kubernetes.io/created-for/pv/name"
@@ -60,7 +61,9 @@ type DiskParameters struct {
 }
 
 // ExtractAndDefaultParameters will take the relevant parameters from a map and
-// put them into a well defined struct making sure to default unspecified fields
+// put them into a well defined struct making sure to default unspecified fields.
+// extraVolumeLabels are added as labels; if there are also labels specified in
+// parameters, any matching extraVolumeLabels will be overridden.
 func ExtractAndDefaultParameters(parameters map[string]string, driverName string, extraVolumeLabels map[string]string) (DiskParameters, error) {
 	p := DiskParameters{
 		DiskType:             "pd-standard",           // Default
@@ -70,6 +73,10 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
 		Labels:               make(map[string]string), // Default
 	}
 
+	for k, v := range extraVolumeLabels {
+		p.Labels[k] = v
+	}
+
 	for k, v := range parameters {
 		if k == "csiProvisionerSecretName" || k == "csiProvisionerSecretNamespace" {
 			// These are hardcoded secrets keys required to function but not needed by GCE PD
@@ -93,15 +100,21 @@ func ExtractAndDefaultParameters(parameters map[string]string, driverName string
 			p.Tags[tagKeyCreatedForClaimNamespace] = v
 		case ParameterKeyPVName:
 			p.Tags[tagKeyCreatedForVolumeName] = v
+		case ParameterKeyLabels:
+			paramLabels, err := ConvertLabelsStringToMap(v)
+			if err != nil {
+				return p, fmt.Errorf("parameters contain invalid labels parameter: %w", err)
+			}
+			// Override any existing labels with those from this parameter.
+			for labelKey, labelValue := range paramLabels {
+				p.Labels[labelKey] = labelValue
+			}
 		default:
 			return p, fmt.Errorf("parameters contains invalid option %q", k)
 		}
 	}
 	if len(p.Tags) > 0 {
 		p.Tags[tagKeyCreatedBy] = driverName
 	}
-	for k, v := range extraVolumeLabels {
-		p.Labels[k] = v
-	}
 	return p, nil
 }

pkg/common/parameters_test.go

@@ -37,38 +37,41 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				DiskType:             "pd-standard",
 				ReplicationType:      "none",
 				DiskEncryptionKMSKey: "",
-				Tags:                 make(map[string]string),
-				Labels:               make(map[string]string),
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{},
 			},
 		},
 		{
 			name:       "specified empties",
-			parameters: map[string]string{ParameterKeyType: "", ParameterKeyReplicationType: "", ParameterKeyDiskEncryptionKmsKey: ""},
+			parameters: map[string]string{ParameterKeyType: "", ParameterKeyReplicationType: "", ParameterKeyDiskEncryptionKmsKey: "", ParameterKeyLabels: ""},
 			labels:     map[string]string{},
 			expectParams: DiskParameters{
 				DiskType:             "pd-standard",
 				ReplicationType:      "none",
 				DiskEncryptionKMSKey: "",
-				Tags:                 make(map[string]string),
-				Labels:               make(map[string]string),
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{},
 			},
 		},
 		{
 			name:       "random keys",
-			parameters: map[string]string{ParameterKeyType: "", "foo": "", ParameterKeyDiskEncryptionKmsKey: ""},
+			parameters: map[string]string{ParameterKeyType: "", "foo": "", ParameterKeyDiskEncryptionKmsKey: "", ParameterKeyLabels: ""},
 			labels:     map[string]string{},
 			expectErr:  true,
 		},
 		{
 			name:       "real values",
-			parameters: map[string]string{ParameterKeyType: "pd-ssd", ParameterKeyReplicationType: "regional-pd", ParameterKeyDiskEncryptionKmsKey: "foo/key"},
+			parameters: map[string]string{ParameterKeyType: "pd-ssd", ParameterKeyReplicationType: "regional-pd", ParameterKeyDiskEncryptionKmsKey: "foo/key", ParameterKeyLabels: "key1=value1,key2=value2"},
 			labels:     map[string]string{},
 			expectParams: DiskParameters{
 				DiskType:             "pd-ssd",
 				ReplicationType:      "regional-pd",
 				DiskEncryptionKMSKey: "foo/key",
-				Tags:                 make(map[string]string),
-				Labels:               make(map[string]string),
+				Tags:                 map[string]string{},
+				Labels: map[string]string{
+					"key1": "value1",
+					"key2": "value2",
+				},
 			},
 		},
 		{
@@ -79,8 +82,8 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				DiskType:             "pd-balanced",
 				ReplicationType:      "regional-pd",
 				DiskEncryptionKMSKey: "foo/key",
-				Tags:                 make(map[string]string),
-				Labels:               make(map[string]string),
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{},
 			},
 		},
 		{
@@ -91,8 +94,8 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				DiskType:             "pd-standard",
 				ReplicationType:      "none",
 				DiskEncryptionKMSKey: "foo/key",
-				Tags:                 make(map[string]string),
-				Labels:               make(map[string]string),
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{},
 			},
 		},
 		{
@@ -104,11 +107,11 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				ReplicationType:      "none",
 				DiskEncryptionKMSKey: "",
 				Tags:                 map[string]string{tagKeyCreatedForClaimName: "testPVCName", tagKeyCreatedForClaimNamespace: "testPVCNamespace", tagKeyCreatedForVolumeName: "testPVName", tagKeyCreatedBy: "testDriver"},
-				Labels:               make(map[string]string),
+				Labels:               map[string]string{},
 			},
 		},
 		{
-			name:       "labels",
+			name:       "extra labels",
 			parameters: map[string]string{},
 			labels:     map[string]string{"label-1": "label-value-1", "label-2": "label-value-2"},
 			expectParams: DiskParameters{
@@ -119,6 +122,30 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				Labels:               map[string]string{"label-1": "label-value-1", "label-2": "label-value-2"},
 			},
 		},
+		{
+			name:       "parameter and extra labels",
+			parameters: map[string]string{ParameterKeyLabels: "key1=value1,key2=value2"},
+			labels:     map[string]string{"label-1": "label-value-1", "label-2": "label-value-2"},
+			expectParams: DiskParameters{
+				DiskType:             "pd-standard",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "",
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{"key1": "value1", "key2": "value2", "label-1": "label-value-1", "label-2": "label-value-2"},
+			},
+		},
+		{
+			name:       "parameter and extra labels, overlapping",
+			parameters: map[string]string{ParameterKeyLabels: "label-1=value-a,key1=value1"},
+			labels:     map[string]string{"label-1": "label-value-1", "label-2": "label-value-2"},
+			expectParams: DiskParameters{
+				DiskType:             "pd-standard",
+				ReplicationType:      "none",
+				DiskEncryptionKMSKey: "",
+				Tags:                 map[string]string{},
+				Labels:               map[string]string{"key1": "value1", "label-1": "value-a", "label-2": "label-value-2"},
+			},
+		},
 	}
 
 	for _, tc := range tests {

pkg/common/utils.go

@@ -18,6 +18,7 @@ package common
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
@@ -156,3 +157,60 @@ func CreateNodeID(project, zone, name string) string {
 func CreateZonalVolumeID(project, zone, name string) string {
 	return fmt.Sprintf(volIDZonalFmt, project, zone, name)
 }
+
+// ConvertLabelsStringToMap converts the labels from string to map
+// example: "key1=value1,key2=value2" gets converted into {"key1": "value1", "key2": "value2"}
+func ConvertLabelsStringToMap(labels string) (map[string]string, error) {
+	const labelsDelimiter = ","
+	const labelsKeyValueDelimiter = "="
+
+	labelsMap := make(map[string]string)
+	if labels == "" {
+		return labelsMap, nil
+	}
+
+	regexKey, _ := regexp.Compile(`^\p{Ll}[\p{Ll}0-9_-]{0,62}$`)
+	checkLabelKeyFn := func(key string) error {
+		if !regexKey.MatchString(key) {
+			return fmt.Errorf("label value %q is invalid (should start with lowercase letter / lowercase letter, digit, _ and - chars are allowed / 1-63 characters", key)
+		}
+		return nil
+	}
+
+	regexValue, _ := regexp.Compile(`^[\p{Ll}0-9_-]{0,63}$`)
+	checkLabelValueFn := func(value string) error {
+		if !regexValue.MatchString(value) {
+			return fmt.Errorf("label value %q is invalid (lowercase letter, digit, _ and - chars are allowed / 0-63 characters", value)
+		}
+
+		return nil
+	}
+
+	keyValueStrings := strings.Split(labels, labelsDelimiter)
+	for _, keyValue := range keyValueStrings {
+		keyValue := strings.Split(keyValue, labelsKeyValueDelimiter)
+
+		if len(keyValue) != 2 {
+			return nil, fmt.Errorf("labels %q are invalid, correct format: 'key1=value1,key2=value2'", labels)
+		}
+
+		key := strings.TrimSpace(keyValue[0])
+		if err := checkLabelKeyFn(key); err != nil {
+			return nil, err
+		}
+
+		value := strings.TrimSpace(keyValue[1])
+		if err := checkLabelValueFn(value); err != nil {
+			return nil, err
+		}
+
+		labelsMap[key] = value
+	}
+
+	const maxNumberOfLabels = 64
+	if len(labelsMap) > maxNumberOfLabels {
+		return nil, fmt.Errorf("more than %d labels is not allowed, given: %d", maxNumberOfLabels, len(labelsMap))
+	}
+
+	return labelsMap, nil
+}