Add labels validation to match gce requirements

Author: ernestas2k

examples/kubernetes/demo-regional-restricted-sc.yaml

@@ -6,7 +6,6 @@ provisioner: pd.csi.storage.gke.io
 parameters:
   type: pd-standard
   replication-type: regional-pd
-  labels: key1=value1,key2=value2
 volumeBindingMode: WaitForFirstConsumer
 allowedTopologies:
 - matchLabelExpressions:

examples/kubernetes/demo-regional-sc.yaml

@@ -6,5 +6,4 @@ provisioner: pd.csi.storage.gke.io
 parameters:
   type: pd-standard
   replication-type: regional-pd
-  labels: key1=value1,key2=value2
 volumeBindingMode: WaitForFirstConsumer

pkg/common/utils.go

@@ -18,6 +18,7 @@ package common
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
@@ -155,29 +156,55 @@ func ConvertLabelsStringToMap(labels string) (map[string]string, error) {
 	const labelsDelimiter = ","
 	const labelsKeyValueDelimiter = "="
 
-	m := make(map[string]string)
+	labelsMap := make(map[string]string)
 	if labels == "" {
-		return m, nil
+		return labelsMap, nil
 	}
 
-	errIncorrectFormat := fmt.Errorf("labels %q are invalid, correct format: 'key1=value1,key2=value2'", labels)
+	regexKey, _ := regexp.Compile("^[a-z][-_a-z0-9]{0,62}$")
+	checkLabelKeyFn := func(key string) error {
+		if !regexKey.MatchString(key) {
+			return fmt.Errorf("label key %q is invalid (lowercase, digit, _ and - chars are allowed / 1-63 characters", key)
+		}
+		return nil
+	}
+
+	regexValue, _ := regexp.Compile("^[-_a-z0-9]{0,63}$")
+	checkLabelValueFn := func(value string) error {
+		if !regexValue.MatchString(value) {
+			return fmt.Errorf("label value %q is invalid (should start with lowercase char / lowercase, 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, errIncorrectFormat
+			return nil, fmt.Errorf("labels %q are invalid, correct format: 'key1=value1,key2=value2'", labels)
 		}
 
 		key := strings.TrimSpace(keyValue[0])
-		if key == "" {
-			return nil, errIncorrectFormat
+		if err := checkLabelKeyFn(key); err != nil {
+			return nil, err
 		}
 
 		value := strings.TrimSpace(keyValue[1])
-		m[key] = value
+		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 m, nil
+
+
+	return labelsMap, nil
 }

pkg/common/utils_test.go

@@ -297,85 +297,153 @@ func TestKeyToVolumeID(t *testing.T) {
 }
 
 func TestConvertLabelsStringToMap(t *testing.T) {
-	testCases := []struct {
-		name           string
-		labels         string
-		expectedOutput map[string]string
-		expectedError  bool
-	}{
-		{
-			name:           "should return empty map when labels string is empty",
-			labels:         "",
-			expectedOutput: map[string]string{},
-			expectedError:  false,
-		},
-		{
-			name:   "single label string",
-			labels: "key=value",
-			expectedOutput: map[string]string{
-				"key": "value",
+	t.Run("parsing labels string into map", func(t *testing.T) {
+		testCases := []struct {
+			name           string
+			labels         string
+			expectedOutput map[string]string
+			expectedError  bool
+		}{
+			{
+				name:           "should return empty map when labels string is empty",
+				labels:         "",
+				expectedOutput: map[string]string{},
+				expectedError:  false,
 			},
-			expectedError: false,
-		},
-		{
-			name:   "multiple label string",
-			labels: "key1=value1,key2=value2",
-			expectedOutput: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
+			{
+				name:   "single label string",
+				labels: "key=value",
+				expectedOutput: map[string]string{
+					"key": "value",
+				},
+				expectedError: false,
 			},
-			expectedError: false,
-		},
-		{
-			name:   "multiple labels string with whitespaces gets trimmed",
-			labels: "key1=value1, key2=value2",
-			expectedOutput: map[string]string{
-				"key1": "value1",
-				"key2": "value2",
+			{
+				name:   "multiple label string",
+				labels: "key1=value1,key2=value2",
+				expectedOutput: map[string]string{
+					"key1": "value1",
+					"key2": "value2",
+				},
+				expectedError: false,
 			},
-			expectedError: false,
-		},
-		{
-			name:           "malformed labels string (no keys and values)",
-			labels:         ",,",
-			expectedOutput: nil,
-			expectedError:  true,
-		},
-		{
-			name:           "malformed labels string (incorrect format)",
-			labels:         "foo,bar",
-			expectedOutput: nil,
-			expectedError:  true,
-		},
-		{
-			name:           "malformed labels string (missing key)",
-			labels:         "key1=value1,=bar",
-			expectedOutput: nil,
-			expectedError:  true,
-		},
-		{
-			name:           "malformed labels string (missing key and value)",
-			labels:         "key1=value1,=bar,=",
-			expectedOutput: nil,
-			expectedError:  true,
-		},
-	}
+			{
+				name:   "multiple labels string with whitespaces gets trimmed",
+				labels: "key1=value1, key2=value2",
+				expectedOutput: map[string]string{
+					"key1": "value1",
+					"key2": "value2",
+				},
+				expectedError: false,
+			},
+			{
+				name:           "malformed labels string (no keys and values)",
+				labels:         ",,",
+				expectedOutput: nil,
+				expectedError:  true,
+			},
+			{
+				name:           "malformed labels string (incorrect format)",
+				labels:         "foo,bar",
+				expectedOutput: nil,
+				expectedError:  true,
+			},
+			{
+				name:           "malformed labels string (missing key)",
+				labels:         "key1=value1,=bar",
+				expectedOutput: nil,
+				expectedError:  true,
+			},
+			{
+				name:           "malformed labels string (missing key and value)",
+				labels:         "key1=value1,=bar,=",
+				expectedOutput: nil,
+				expectedError:  true,
+			},
+		}
 
-	for _, tc := range testCases {
-		t.Logf("test case: %s", tc.name)
-		output, err := ConvertLabelsStringToMap(tc.labels)
-		if err == nil && tc.expectedError {
-			t.Errorf("Expected error but got none")
+		for _, tc := range testCases {
+			t.Logf("test case: %s", tc.name)
+			output, err := ConvertLabelsStringToMap(tc.labels)
+			if tc.expectedError && err == nil {
+				t.Errorf("Expected error but got none")
+			}
+			if err != nil {
+				if !tc.expectedError {
+					t.Errorf("Did not expect error but got: %v", err)
+				}
+				continue
+			}
+
+			if !reflect.DeepEqual(output, tc.expectedOutput) {
+				t.Errorf("Got labels %v, but expected %v", output, tc.expectedOutput)
+			}
 		}
-		if err != nil {
-			if !tc.expectedError {
+	})
+
+	t.Run("checking google requirements", func(t *testing.T) {
+		testCases := []struct {
+			name          string
+			labels        string
+			expectedError bool
+		}{
+			{
+				name: "64 labels at most",
+				labels: `k1=v,k2=v,k3=v,k4=v,k5=v,k6=v,k7=v,k8=v,k9=v,k10=v,k11=v,k12=v,k13=v,k14=v,k15=v,k16=v,k17=v,k18=v,k19=v,k20=v,
+                         k21=v,k22=v,k23=v,k24=v,k25=v,k26=v,k27=v,k28=v,k29=v,k30=v,k31=v,k32=v,k33=v,k34=v,k35=v,k36=v,k37=v,k38=v,k39=v,k40=v,
+                         k41=v,k42=v,k43=v,k44=v,k45=v,k46=v,k47=v,k48=v,k49=v,k50=v,k51=v,k52=v,k53=v,k54=v,k55=v,k56=v,k57=v,k58=v,k59=v,k60=v,
+                         k61=v,k62=v,k63=v,k64=v,k65=v`,
+				expectedError: true,
+			},
+			{
+				name:          "label key must start with lowercase char",
+				labels:        "#k=v",
+				expectedError: true,
+			},
+			{
+				name:          "label key can only contain lowercase chars, digits, _ and -)",
+				labels:        "k*=v",
+				expectedError: true,
+			},
+			{
+				name:          "label key may not have over 63 characters",
+				labels:        "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234=v",
+				expectedError: true,
+			},
+			{
+				name:          "label key can have up to 63 characters",
+				labels:        "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij123=v",
+				expectedError: false,
+			},
+			{
+				name:          "label value can only contain lowercase chars, digits, _ and -)",
+				labels:        "k1=###",
+				expectedError: true,
+			},
+			{
+				name:          "label value may not have over 63 characters",
+				labels:        "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij1234=v",
+				expectedError: true,
+			},
+			{
+				name:          "label value can have up to 63 characters",
+				labels:        "abcdefghijabcdefghijabcdefghijabcdefghijabcdefghijabcdefghij123=v",
+				expectedError: false,
+			},
+		}
+
+		for _, tc := range testCases {
+			t.Logf("test case: %s", tc.name)
+			_, err := ConvertLabelsStringToMap(tc.labels)
+
+			if tc.expectedError && err == nil {
+				t.Errorf("Expected error but got none")
+			}
+
+			if !tc.expectedError && err != nil {
 				t.Errorf("Did not expect error but got: %v", err)
 			}
-			continue
 		}
+	})
 
-		if !reflect.DeepEqual(output, tc.expectedOutput) {
-			t.Errorf("Got labels %v, but expected %v", output, tc.expectedOutput)
-		}
-	}
 }

pkg/gce-pd-csi-driver/controller_test.go

@@ -684,12 +684,12 @@ func TestCreateVolumeArguments(t *testing.T) {
 			},
 		},
 		{
-			name: "success with malformed labels parameter",
+			name: "fail with malformed labels parameter",
 			req: &csi.CreateVolumeRequest{
 				Name:               name,
 				CapacityRange:      stdCapRange,
 				VolumeCapabilities: stdVolCaps,
-				Parameters:         map[string]string{"labels": "key1=value1,;;"},
+				Parameters:         map[string]string{"labels": "key1=value1,#=$;;"},
 			},
 			expErrCode: codes.InvalidArgument,
 		},