Validate key subscript for metadata.annotations and metadata.labels separately

pkg/apis/core/pods/helpers.go

@@ -22,13 +22,25 @@ import (
 	"k8s.io/kubernetes/pkg/fieldpath"
 )
 
+// ConvertDownwardAPIFieldLabel converts the specified downward API field label
+// and its value in the pod of the specified version to the internal version,
+// and returns the converted label and value. This function returns an error if
+// the conversion fails.
 func ConvertDownwardAPIFieldLabel(version, label, value string) (string, string, error) {
 	if version != "v1" {
 		return "", "", fmt.Errorf("unsupported pod version: %s", version)
 	}
 
-	path, _ := fieldpath.SplitMaybeSubscriptedPath(label)
+	if path, _, ok := fieldpath.SplitMaybeSubscriptedPath(label); ok {
 		switch path {
+		case "metadata.annotations", "metadata.labels":
+			return label, value, nil
+		default:
+			return "", "", fmt.Errorf("field label does not support subscript: %s", label)
+		}
+	}
+
+	switch label {
 	case "metadata.annotations",
 		"metadata.labels",
 		"metadata.name",

pkg/apis/core/v1/BUILD

@@ -16,7 +16,6 @@ go_library(
         "//pkg/apis/core:go_default_library",
         "//pkg/apis/extensions:go_default_library",
         "//pkg/features:go_default_library",
-        "//pkg/fieldpath:go_default_library",
         "//pkg/util/parsers:go_default_library",
         "//pkg/util/pointer:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",

pkg/apis/core/v1/conversion.go

@@ -28,7 +28,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/extensions"
-	"k8s.io/kubernetes/pkg/fieldpath"
 )
 
 // This is a "fast-path" that avoids reflection for common types. It focuses on the objects that are
@@ -157,8 +156,7 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
 	// Add field conversion funcs.
 	err = scheme.AddFieldLabelConversionFunc("v1", "Pod",
 		func(label, value string) (string, string, error) {
-			path, _ := fieldpath.SplitMaybeSubscriptedPath(label)
+			switch label {
-			switch path {
 			case "metadata.name",
 				"metadata.namespace",
 				"spec.nodeName",

pkg/apis/core/validation/validation.go

@@ -965,9 +965,7 @@ var validVolumeDownwardAPIFieldPathExpressions = sets.NewString(
 	"metadata.name",
 	"metadata.namespace",
 	"metadata.labels",
-	"metadata.labels[]", // represents "metadata.labels" with an arbitary subscript
 	"metadata.annotations",
-	"metadata.annotations[]", // represents "metadata.annotations" with an arbitary subscript
 	"metadata.uid")
 
 func validateDownwardAPIVolumeFile(file *core.DownwardAPIVolumeFile, fldPath *field.Path) field.ErrorList {
@@ -1902,8 +1900,6 @@ func ValidateEnv(vars []core.EnvVar, fldPath *field.Path) field.ErrorList {
 }
 
 var validEnvDownwardAPIFieldPathExpressions = sets.NewString(
-	"metadata.annotations[]", // represents "metadata.annotations" with an arbitary subscript
-	"metadata.labels[]",      // represents "metadata.labels" with an arbitary subscript
 	"metadata.name",
 	"metadata.namespace",
 	"metadata.uid",
@@ -1970,20 +1966,22 @@ func validateObjectFieldSelector(fs *core.ObjectFieldSelector, expressions *sets
 		return allErrs
 	}
 
-	path, subscript := fieldpath.SplitMaybeSubscriptedPath(internalFieldPath)
+	if path, subscript, ok := fieldpath.SplitMaybeSubscriptedPath(internalFieldPath); ok {
-	if len(subscript) > 0 {
+		switch path {
-		// This is to indicate that the internalFieldPath has a subscript, so
+		case "metadata.annotations":
-		// that we can compare the path against the allowed path set easily.
+			for _, msg := range validation.IsQualifiedName(strings.ToLower(subscript)) {
-		path += "[]"
+				allErrs = append(allErrs, field.Invalid(fldPath, subscript, msg))
 			}
-	if !expressions.Has(path) {
+		case "metadata.labels":
-		allErrs = append(allErrs, field.NotSupported(fldPath.Child("fieldPath"), path, expressions.List()))
-		return allErrs
-	}
-	if len(subscript) > 0 {
 			for _, msg := range validation.IsQualifiedName(subscript) {
 				allErrs = append(allErrs, field.Invalid(fldPath, subscript, msg))
 			}
+		default:
+			allErrs = append(allErrs, field.Invalid(fldPath, path, "does not support subscript"))
+		}
+	} else if !expressions.Has(path) {
+		allErrs = append(allErrs, field.NotSupported(fldPath.Child("fieldPath"), path, expressions.List()))
+		return allErrs
 	}
 
 	return allErrs

pkg/apis/core/validation/validation_test.go

@@ -2635,6 +2635,13 @@ func TestValidateVolumes(t *testing.T) {
 									FieldPath:  "metadata.labels['key']",
 								},
 							},
+							{
+								Path: "labels with complex subscript",
+								FieldRef: &core.ObjectFieldSelector{
+									APIVersion: "v1",
+									FieldPath:  "metadata.labels['test.example.com/key']",
+								},
+							},
 							{
 								Path: "annotations",
 								FieldRef: &core.ObjectFieldSelector{
@@ -2649,6 +2656,13 @@ func TestValidateVolumes(t *testing.T) {
 									FieldPath:  "metadata.annotations['key']",
 								},
 							},
+							{
+								Path: "annotations with complex subscript",
+								FieldRef: &core.ObjectFieldSelector{
+									APIVersion: "v1",
+									FieldPath:  "metadata.annotations['TEST.EXAMPLE.COM/key']",
+								},
+							},
 							{
 								Path: "namespace",
 								FieldRef: &core.ObjectFieldSelector{
@@ -4137,7 +4151,7 @@ func TestValidateEnv(t *testing.T) {
 					},
 				},
 			}},
-			expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.name[]": supported values: "metadata.annotations[]", "metadata.labels[]", "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
+			expectedError: `[0].valueFrom.fieldRef.fieldPath: Invalid value: "metadata.name['key']": error converting fieldPath: field label does not support subscript`,
 		},
 		{
 			name: "metadata.labels without subscript",
@@ -4150,7 +4164,7 @@ func TestValidateEnv(t *testing.T) {
 					},
 				},
 			}},
-			expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.labels": supported values: "metadata.annotations[]", "metadata.labels[]", "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
+			expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.labels": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
 		},
 		{
 			name: "metadata.annotations without subscript",
@@ -4163,7 +4177,33 @@ func TestValidateEnv(t *testing.T) {
 					},
 				},
 			}},
-			expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.annotations": supported values: "metadata.annotations[]", "metadata.labels[]", "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
+			expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.annotations": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
+		},
+		{
+			name: "metadata.annotations with invalid key",
+			envs: []core.EnvVar{{
+				Name: "abc",
+				ValueFrom: &core.EnvVarSource{
+					FieldRef: &core.ObjectFieldSelector{
+						FieldPath:  "metadata.annotations['invalid~key']",
+						APIVersion: "v1",
+					},
+				},
+			}},
+			expectedError: `field[0].valueFrom.fieldRef: Invalid value: "invalid~key"`,
+		},
+		{
+			name: "metadata.labels with invalid key",
+			envs: []core.EnvVar{{
+				Name: "abc",
+				ValueFrom: &core.EnvVarSource{
+					FieldRef: &core.ObjectFieldSelector{
+						FieldPath:  "metadata.labels['Www.k8s.io/test']",
+						APIVersion: "v1",
+					},
+				},
+			}},
+			expectedError: `field[0].valueFrom.fieldRef: Invalid value: "Www.k8s.io/test"`,
 		},
 		{
 			name: "unsupported fieldPath",
@@ -4176,7 +4216,7 @@ func TestValidateEnv(t *testing.T) {
 					},
 				},
 			}},
-			expectedError: `valueFrom.fieldRef.fieldPath: Unsupported value: "status.phase": supported values: "metadata.annotations[]", "metadata.labels[]", "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
+			expectedError: `valueFrom.fieldRef.fieldPath: Unsupported value: "status.phase": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
 		},
 	}
 	for _, tc := range errorCases {

pkg/fieldpath/BUILD

@@ -13,7 +13,10 @@ go_library(
         "fieldpath.go",
     ],
     importpath = "k8s.io/kubernetes/pkg/fieldpath",
-    deps = ["//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library"],
+    deps = [
+        "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",
+    ],
 )
 
 go_test(

pkg/fieldpath/fieldpath.go

@@ -21,6 +21,7 @@ import (
 	"strings"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	"k8s.io/apimachinery/pkg/util/validation"
 )
 
 // FormatMap formats map[string]string to a string.
@@ -42,20 +43,24 @@ func ExtractFieldPathAsString(obj interface{}, fieldPath string) (string, error)
 		return "", nil
 	}
 
-	path, subscript := SplitMaybeSubscriptedPath(fieldPath)
+	if path, subscript, ok := SplitMaybeSubscriptedPath(fieldPath); ok {
-
-	if len(subscript) > 0 {
 		switch path {
 		case "metadata.annotations":
+			if errs := validation.IsQualifiedName(strings.ToLower(subscript)); len(errs) != 0 {
+				return "", fmt.Errorf("invalid key subscript in %s: %s", fieldPath, strings.Join(errs, ";"))
+			}
 			return accessor.GetAnnotations()[subscript], nil
 		case "metadata.labels":
+			if errs := validation.IsQualifiedName(subscript); len(errs) != 0 {
+				return "", fmt.Errorf("invalid key subscript in %s: %s", fieldPath, strings.Join(errs, ";"))
+			}
 			return accessor.GetLabels()[subscript], nil
 		default:
 			return "", fmt.Errorf("fieldPath %q does not support subscript", fieldPath)
 		}
 	}
 
-	switch path {
+	switch fieldPath {
 	case "metadata.annotations":
 		return FormatMap(accessor.GetAnnotations()), nil
 	case "metadata.labels":
@@ -74,25 +79,25 @@ func ExtractFieldPathAsString(obj interface{}, fieldPath string) (string, error)
 // SplitMaybeSubscriptedPath checks whether the specified fieldPath is
 // subscripted, and
 //  - if yes, this function splits the fieldPath into path and subscript, and
-//    returns (path, subscript).
+//    returns (path, subscript, true).
-//  - if no, this function returns (fieldPath, "").
+//  - if no, this function returns (fieldPath, "", false).
 //
 // Example inputs and outputs:
-//  - "metadata.annotations['myKey']" --> ("metadata.annotations", "myKey")
+//  - "metadata.annotations['myKey']" --> ("metadata.annotations", "myKey", true)
-//  - "metadata.annotations['a[b]c']" --> ("metadata.annotations", "a[b]c")
+//  - "metadata.annotations['a[b]c']" --> ("metadata.annotations", "a[b]c", true)
-//  - "metadata.labels"               --> ("metadata.labels", "")
+//  - "metadata.labels['']"           --> ("metadata.labels", "", true)
-//  - "metadata.labels['']"           --> ("metadata.labels", "")
+//  - "metadata.labels"               --> ("metadata.labels", "", false)
-func SplitMaybeSubscriptedPath(fieldPath string) (string, string) {
+func SplitMaybeSubscriptedPath(fieldPath string) (string, string, bool) {
 	if !strings.HasSuffix(fieldPath, "']") {
-		return fieldPath, ""
+		return fieldPath, "", false
 	}
 	s := strings.TrimSuffix(fieldPath, "']")
 	parts := strings.SplitN(s, "['", 2)
 	if len(parts) < 2 {
-		return fieldPath, ""
+		return fieldPath, "", false
 	}
-	if len(parts[0]) == 0 || len(parts[1]) == 0 {
+	if len(parts[0]) == 0 {
-		return fieldPath, ""
+		return fieldPath, "", false
 	}
-	return parts[0], parts[1]
+	return parts[0], parts[1], true
 }

pkg/fieldpath/fieldpath_test.go

@@ -36,7 +36,6 @@ func TestExtractFieldPathAsString(t *testing.T) {
 			name:      "not an API object",
 			fieldPath: "metadata.name",
 			obj:       "",
-			expectedMessageFragment: "expected struct",
 		},
 		{
 			name:      "ok - namespace",
@@ -98,7 +97,16 @@ func TestExtractFieldPathAsString(t *testing.T) {
 			},
 			expectedValue: "1",
 		},
-
+		{
+			name:      "ok - annotation",
+			fieldPath: "metadata.annotations['Www.k8s.io/test']",
+			obj: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{"Www.k8s.io/test": "1"},
+				},
+			},
+			expectedValue: "1",
+		},
 		{
 			name:      "invalid expression",
 			fieldPath: "metadata.whoops",
@@ -110,14 +118,24 @@ func TestExtractFieldPathAsString(t *testing.T) {
 			expectedMessageFragment: "unsupported fieldPath",
 		},
 		{
-			name:      "invalid annotation",
+			name:      "invalid annotation key",
-			fieldPath: "metadata.annotations['unknown.key']",
+			fieldPath: "metadata.annotations['invalid~key']",
 			obj: &v1.Pod{
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: map[string]string{"foo": "bar"},
 				},
 			},
-			expectedMessageFragment: "unsupported fieldPath",
+			expectedMessageFragment: "invalid key subscript in metadata.annotations",
+		},
+		{
+			name:      "invalid label key",
+			fieldPath: "metadata.labels['Www.k8s.io/test']",
+			obj: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{"foo": "bar"},
+				},
+			},
+			expectedMessageFragment: "invalid key subscript in metadata.labels",
 		},
 	}
 
@@ -131,6 +149,8 @@ func TestExtractFieldPathAsString(t *testing.T) {
 			} else {
 				t.Errorf("%v: unexpected error: %v", tc.name, err)
 			}
+		} else if tc.expectedMessageFragment != "" {
+			t.Errorf("%v: expected error: %v", tc.name, tc.expectedMessageFragment)
 		} else if e := tc.expectedValue; e != "" && e != actual {
 			t.Errorf("%v: unexpected result; got %q, expected %q", tc.name, actual, e)
 		}
@@ -142,72 +162,79 @@ func TestSplitMaybeSubscriptedPath(t *testing.T) {
 		fieldPath         string
 		expectedPath      string
 		expectedSubscript string
+		expectedOK        bool
 	}{
 		{
 			fieldPath:         "metadata.annotations['key']",
 			expectedPath:      "metadata.annotations",
 			expectedSubscript: "key",
+			expectedOK:        true,
 		},
 		{
 			fieldPath:         "metadata.annotations['a[b']c']",
 			expectedPath:      "metadata.annotations",
 			expectedSubscript: "a[b']c",
+			expectedOK:        true,
 		},
 		{
 			fieldPath:         "metadata.labels['['key']",
 			expectedPath:      "metadata.labels",
 			expectedSubscript: "['key",
+			expectedOK:        true,
 		},
 		{
 			fieldPath:         "metadata.labels['key']']",
 			expectedPath:      "metadata.labels",
 			expectedSubscript: "key']",
-		},
+			expectedOK:        true,
-		{
-			fieldPath:         "metadata.labels[ 'key' ]",
-			expectedPath:      "metadata.labels[ 'key' ]",
-			expectedSubscript: "",
 		},
 		{
 			fieldPath:         "metadata.labels['']",
-			expectedPath:      "metadata.labels['']",
+			expectedPath:      "metadata.labels",
 			expectedSubscript: "",
+			expectedOK:        true,
 		},
 		{
 			fieldPath:         "metadata.labels[' ']",
 			expectedPath:      "metadata.labels",
 			expectedSubscript: " ",
+			expectedOK:        true,
+		},
+		{
+			fieldPath:  "metadata.labels[ 'key' ]",
+			expectedOK: false,
 		},
 		{
 			fieldPath:  "metadata.labels[]",
-			expectedPath:      "metadata.labels[]",
+			expectedOK: false,
-			expectedSubscript: "",
 		},
 		{
 			fieldPath:  "metadata.labels[']",
-			expectedPath:      "metadata.labels[']",
+			expectedOK: false,
-			expectedSubscript: "",
 		},
 		{
 			fieldPath:  "metadata.labels['key']foo",
-			expectedPath:      "metadata.labels['key']foo",
+			expectedOK: false,
-			expectedSubscript: "",
 		},
 		{
 			fieldPath:  "['key']",
-			expectedPath:      "['key']",
+			expectedOK: false,
-			expectedSubscript: "",
 		},
 		{
 			fieldPath:  "metadata.labels",
-			expectedPath:      "metadata.labels",
+			expectedOK: false,
-			expectedSubscript: "",
 		},
 	}
 	for _, tc := range cases {
-		path, subscript := SplitMaybeSubscriptedPath(tc.fieldPath)
+		path, subscript, ok := SplitMaybeSubscriptedPath(tc.fieldPath)
+		if !ok {
+			if tc.expectedOK {
+				t.Errorf("SplitMaybeSubscriptedPath(%q) expected to return (_, _, true)", tc.fieldPath)
+			}
+			continue
+		}
 		if path != tc.expectedPath || subscript != tc.expectedSubscript {
-			t.Errorf("SplitMaybeSubscriptedPath(%q) = (%q, %q), expect (%q, %q)",
+			t.Errorf("SplitMaybeSubscriptedPath(%q) = (%q, %q, true), expect (%q, %q, true)",
 				tc.fieldPath, path, subscript, tc.expectedPath, tc.expectedSubscript)
 		}
 	}