Skip to content

Add gce disk labels support via create volume parameters #718

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
Mar 15, 2021
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
13 changes: 10 additions & 3 deletions cmd/gce-pd-csi-driver/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -39,7 +39,7 @@ var (
runNodeService = flag.Bool("run-node-service", true, "If set to false then the CSI driver does not activate its node service (default: true)")
httpEndpoint = flag.String("http-endpoint", "", "The TCP network address where the prometheus metrics endpoint will listen (example: `:8080`). The default is empty string, which means metrics endpoint is disabled.")
metricsPath = flag.String("metrics-path", "/metrics", "The HTTP path where prometheus metrics will be exposed. Default is `/metrics`.")
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
)

Expand All @@ -58,7 +58,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()
Expand All @@ -79,6 +78,14 @@ func handle() {
mm.EmitGKEComponentVersion()
}

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
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
Original file line number Diff line number Diff line change
Expand Up @@ -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
10 changes: 10 additions & 0 deletions examples/kubernetes/demo-labeled-sc.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Example StorageClass that adds labels to the GCP PD.
# This requires v1.2.1 or higher.
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: csi-gce-pd
provisioner: pd.csi.storage.gke.io
parameters:
Copy link
Member

Choose a reason for hiding this comment

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

We should put a comment somewhere, about which overlay build would honor this label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

labels: key1=value1,key2=value2
volumeBindingMode: WaitForFirstConsumer
23 changes: 18 additions & 5 deletions pkg/common/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const (
ParameterKeyType = "type"
ParameterKeyReplicationType = "replication-type"
ParameterKeyDiskEncryptionKmsKey = "disk-encryption-kms-key"
ParameterKeyLabels = "labels"

replicationTypeNone = "none"

Expand All @@ -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"
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
}
61 changes: 44 additions & 17 deletions pkg/common/parameters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,50 +37,53 @@ 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"},
name: "values from parameters",
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",
},
},
},
{
name: "real values, checking balanced pd",
name: "values from parameters, checking balanced pd",
parameters: map[string]string{ParameterKeyType: "pd-balanced", ParameterKeyReplicationType: "regional-pd", ParameterKeyDiskEncryptionKmsKey: "foo/key"},
labels: map[string]string{},
expectParams: DiskParameters{
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{},
},
},
{
Expand All @@ -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{},
},
},
{
Expand All @@ -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{
Expand All @@ -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 {
Expand Down
59 changes: 59 additions & 0 deletions pkg/common/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package common

import (
"fmt"
"regexp"
"strings"

"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
Expand Down Expand Up @@ -156,3 +157,61 @@ 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"}
// See https://cloud.google.com/compute/docs/labeling-resources#label_format for details.
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}$`)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can put a pointer to the label format https://cloud.google.com/compute/docs/labeling-resources#label_format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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
}
Loading