From 82c9eec1648aa8ce342be6f3c799dcc62dd47070 Mon Sep 17 00:00:00 2001 From: Yang Guo Date: Fri, 17 Feb 2017 17:37:15 +0100 Subject: [PATCH 1/3] Expose single annotation/label via downward API --- hack/import-restrictions.yaml | 1 + pkg/apis/core/v1/BUILD | 1 + pkg/apis/core/v1/conversion.go | 6 +- pkg/apis/core/validation/BUILD | 1 + pkg/apis/core/validation/validation.go | 52 ++++++++--- pkg/apis/core/validation/validation_test.go | 55 ++++++++++-- pkg/fieldpath/fieldpath.go | 41 ++++++++- pkg/fieldpath/fieldpath_test.go | 96 +++++++++++++++++++++ pkg/volume/projected/projected_test.go | 42 +++++++++ 9 files changed, 276 insertions(+), 19 deletions(-) diff --git a/hack/import-restrictions.yaml b/hack/import-restrictions.yaml index bd4479bd455..c9b99ae2436 100644 --- a/hack/import-restrictions.yaml +++ b/hack/import-restrictions.yaml @@ -4,6 +4,7 @@ - k8s.io/apiserver/pkg/util/feature - k8s.io/kubernetes/pkg/apis/core - k8s.io/kubernetes/pkg/features + - k8s.io/kubernetes/pkg/fieldpath - k8s.io/kubernetes/pkg/util - k8s.io/api/core/v1 diff --git a/pkg/apis/core/v1/BUILD b/pkg/apis/core/v1/BUILD index 5796c614076..a82315ad5fa 100644 --- a/pkg/apis/core/v1/BUILD +++ b/pkg/apis/core/v1/BUILD @@ -16,6 +16,7 @@ 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", diff --git a/pkg/apis/core/v1/conversion.go b/pkg/apis/core/v1/conversion.go index 2e650739db5..70de6b9734a 100644 --- a/pkg/apis/core/v1/conversion.go +++ b/pkg/apis/core/v1/conversion.go @@ -28,6 +28,7 @@ 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 @@ -156,7 +157,8 @@ func addConversionFuncs(scheme *runtime.Scheme) error { // Add field conversion funcs. err = scheme.AddFieldLabelConversionFunc("v1", "Pod", func(label, value string) (string, string, error) { - switch label { + path, _ := fieldpath.SplitMaybeSubscriptedPath(label) + switch path { case "metadata.annotations", "metadata.labels", "metadata.name", @@ -170,7 +172,7 @@ func addConversionFuncs(scheme *runtime.Scheme) error { "status.hostIP", "status.podIP": return label, value, nil - // This is for backwards compatibility with old v1 clients which send spec.host + // This is for backwards compatibility with old v1 clients which send spec.host case "spec.host": return "spec.nodeName", value, nil default: diff --git a/pkg/apis/core/validation/BUILD b/pkg/apis/core/validation/BUILD index acdbf6cc8ad..18282ae464e 100644 --- a/pkg/apis/core/validation/BUILD +++ b/pkg/apis/core/validation/BUILD @@ -22,6 +22,7 @@ go_library( "//pkg/apis/core/v1/helper:go_default_library", "//pkg/capabilities:go_default_library", "//pkg/features:go_default_library", + "//pkg/fieldpath:go_default_library", "//pkg/security/apparmor:go_default_library", "//vendor/github.com/golang/glog:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 5eb0fb12206..cb063ad396d 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -50,6 +50,7 @@ import ( v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" "k8s.io/kubernetes/pkg/capabilities" "k8s.io/kubernetes/pkg/features" + "k8s.io/kubernetes/pkg/fieldpath" "k8s.io/kubernetes/pkg/security/apparmor" ) @@ -960,11 +961,13 @@ func validateFlockerVolumeSource(flocker *core.FlockerVolumeSource, fldPath *fie return allErrs } -var validDownwardAPIFieldPathExpressions = sets.NewString( +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 { @@ -975,7 +978,7 @@ func validateDownwardAPIVolumeFile(file *core.DownwardAPIVolumeFile, fldPath *fi } allErrs = append(allErrs, validateLocalNonReservedPath(file.Path, fldPath.Child("path"))...) if file.FieldRef != nil { - allErrs = append(allErrs, validateObjectFieldSelector(file.FieldRef, &validDownwardAPIFieldPathExpressions, fldPath.Child("fieldRef"))...) + allErrs = append(allErrs, validateObjectFieldSelector(file.FieldRef, &validVolumeDownwardAPIFieldPathExpressions, fldPath.Child("fieldRef"))...) if file.ResourceFieldRef != nil { allErrs = append(allErrs, field.Invalid(fldPath, "resource", "fieldRef and resourceFieldRef can not be specified simultaneously")) } @@ -1898,7 +1901,16 @@ func ValidateEnv(vars []core.EnvVar, fldPath *field.Path) field.ErrorList { return allErrs } -var validFieldPathExpressionsEnv = sets.NewString("metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP") +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", + "spec.nodeName", + "spec.serviceAccountName", + "status.hostIP", + "status.podIP") var validContainerResourceFieldPathExpressions = sets.NewString("limits.cpu", "limits.memory", "limits.ephemeral-storage", "requests.cpu", "requests.memory", "requests.ephemeral-storage") func validateEnvVarValueFrom(ev core.EnvVar, fldPath *field.Path) field.ErrorList { @@ -1912,7 +1924,7 @@ func validateEnvVarValueFrom(ev core.EnvVar, fldPath *field.Path) field.ErrorLis if ev.ValueFrom.FieldRef != nil { numSources++ - allErrs = append(allErrs, validateObjectFieldSelector(ev.ValueFrom.FieldRef, &validFieldPathExpressionsEnv, fldPath.Child("fieldRef"))...) + allErrs = append(allErrs, validateObjectFieldSelector(ev.ValueFrom.FieldRef, &validEnvDownwardAPIFieldPathExpressions, fldPath.Child("fieldRef"))...) } if ev.ValueFrom.ResourceFieldRef != nil { numSources++ @@ -1945,14 +1957,32 @@ func validateObjectFieldSelector(fs *core.ObjectFieldSelector, expressions *sets if len(fs.APIVersion) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("apiVersion"), "")) - } else if len(fs.FieldPath) == 0 { + return allErrs + } + if len(fs.FieldPath) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("fieldPath"), "")) - } else { - internalFieldPath, _, err := legacyscheme.Scheme.ConvertFieldLabel(fs.APIVersion, "Pod", fs.FieldPath, "") - if err != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("fieldPath"), fs.FieldPath, fmt.Sprintf("error converting fieldPath: %v", err))) - } else if !expressions.Has(internalFieldPath) { - allErrs = append(allErrs, field.NotSupported(fldPath.Child("fieldPath"), internalFieldPath, expressions.List())) + return allErrs + } + + internalFieldPath, _, err := legacyscheme.Scheme.ConvertFieldLabel(fs.APIVersion, "Pod", fs.FieldPath, "") + if err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("fieldPath"), fs.FieldPath, fmt.Sprintf("error converting fieldPath: %v", err))) + return allErrs + } + + path, subscript := fieldpath.SplitMaybeSubscriptedPath(internalFieldPath) + if len(subscript) > 0 { + // This is to indicate that the internalFieldPath has a subscript, so + // that we can compare the path against the allowed path set easily. + path += "[]" + } + if !expressions.Has(path) { + 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)) } } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 62c4c3d0597..ec9903e2558 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -2628,6 +2628,13 @@ func TestValidateVolumes(t *testing.T) { FieldPath: "metadata.labels", }, }, + { + Path: "labels with subscript", + FieldRef: &core.ObjectFieldSelector{ + APIVersion: "v1", + FieldPath: "metadata.labels['key']", + }, + }, { Path: "annotations", FieldRef: &core.ObjectFieldSelector{ @@ -2635,6 +2642,13 @@ func TestValidateVolumes(t *testing.T) { FieldPath: "metadata.annotations", }, }, + { + Path: "annotations with subscript", + FieldRef: &core.ObjectFieldSelector{ + APIVersion: "v1", + FieldPath: "metadata.annotations['key']", + }, + }, { Path: "namespace", FieldRef: &core.ObjectFieldSelector{ @@ -3824,6 +3838,24 @@ func TestValidateEnv(t *testing.T) { {Name: "abc", Value: ""}, {Name: "a.b.c", Value: "value"}, {Name: "a-b-c", Value: "value"}, + { + Name: "abc", + ValueFrom: &core.EnvVarSource{ + FieldRef: &core.ObjectFieldSelector{ + APIVersion: legacyscheme.Registry.GroupOrDie(core.GroupName).GroupVersion.String(), + FieldPath: "metadata.annotations['key']", + }, + }, + }, + { + Name: "abc", + ValueFrom: &core.EnvVarSource{ + FieldRef: &core.ObjectFieldSelector{ + APIVersion: legacyscheme.Registry.GroupOrDie(core.GroupName).GroupVersion.String(), + FieldPath: "metadata.labels['key']", + }, + }, + }, { Name: "abc", ValueFrom: &core.EnvVarSource{ @@ -4095,7 +4127,20 @@ func TestValidateEnv(t *testing.T) { expectedError: `[0].valueFrom.fieldRef.fieldPath: Invalid value: "metadata.whoops": error converting fieldPath`, }, { - name: "invalid fieldPath labels", + name: "metadata.name with subscript", + envs: []core.EnvVar{{ + Name: "labels", + ValueFrom: &core.EnvVarSource{ + FieldRef: &core.ObjectFieldSelector{ + FieldPath: "metadata.name['key']", + APIVersion: "v1", + }, + }, + }}, + 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"`, + }, + { + name: "metadata.labels without subscript", envs: []core.EnvVar{{ Name: "labels", ValueFrom: &core.EnvVarSource{ @@ -4105,10 +4150,10 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - 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"`, + 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"`, }, { - name: "invalid fieldPath annotations", + name: "metadata.annotations without subscript", envs: []core.EnvVar{{ Name: "abc", ValueFrom: &core.EnvVarSource{ @@ -4118,7 +4163,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - 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"`, + 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"`, }, { name: "unsupported fieldPath", @@ -4131,7 +4176,7 @@ func TestValidateEnv(t *testing.T) { }, }, }}, - expectedError: `valueFrom.fieldRef.fieldPath: Unsupported value: "status.phase": supported values: "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.annotations[]", "metadata.labels[]", "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`, }, } for _, tc := range errorCases { diff --git a/pkg/fieldpath/fieldpath.go b/pkg/fieldpath/fieldpath.go index caf0096ca0e..531a7d6a642 100644 --- a/pkg/fieldpath/fieldpath.go +++ b/pkg/fieldpath/fieldpath.go @@ -42,7 +42,20 @@ func ExtractFieldPathAsString(obj interface{}, fieldPath string) (string, error) return "", nil } - switch fieldPath { + path, subscript := SplitMaybeSubscriptedPath(fieldPath) + + if len(subscript) > 0 { + switch path { + case "metadata.annotations": + return accessor.GetAnnotations()[subscript], nil + case "metadata.labels": + return accessor.GetLabels()[subscript], nil + default: + return "", fmt.Errorf("fieldPath %q does not support subscript", fieldPath) + } + } + + switch path { case "metadata.annotations": return FormatMap(accessor.GetAnnotations()), nil case "metadata.labels": @@ -57,3 +70,29 @@ func ExtractFieldPathAsString(obj interface{}, fieldPath string) (string, error) return "", fmt.Errorf("unsupported fieldPath: %v", fieldPath) } + +// SplitMaybeSubscriptedPath checks whether the specified fieldPath is +// subscripted, and +// - if yes, this function splits the fieldPath into path and subscript, and +// returns (path, subscript). +// - if no, this function returns (fieldPath, ""). +// +// Example inputs and outputs: +// - "metadata.annotations['myKey']" --> ("metadata.annotations", "myKey") +// - "metadata.annotations['a[b]c']" --> ("metadata.annotations", "a[b]c") +// - "metadata.labels" --> ("metadata.labels", "") +// - "metadata.labels['']" --> ("metadata.labels", "") +func SplitMaybeSubscriptedPath(fieldPath string) (string, string) { + if !strings.HasSuffix(fieldPath, "']") { + return fieldPath, "" + } + s := strings.TrimSuffix(fieldPath, "']") + parts := strings.SplitN(s, "['", 2) + if len(parts) < 2 { + return fieldPath, "" + } + if len(parts[0]) == 0 || len(parts[1]) == 0 { + return fieldPath, "" + } + return parts[0], parts[1] +} diff --git a/pkg/fieldpath/fieldpath_test.go b/pkg/fieldpath/fieldpath_test.go index 7e543952195..f5eaba11618 100644 --- a/pkg/fieldpath/fieldpath_test.go +++ b/pkg/fieldpath/fieldpath_test.go @@ -88,6 +88,16 @@ func TestExtractFieldPathAsString(t *testing.T) { }, expectedValue: "builder=\"john-doe\"", }, + { + name: "ok - annotation", + fieldPath: "metadata.annotations['spec.pod.beta.kubernetes.io/statefulset-index']", + obj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"spec.pod.beta.kubernetes.io/statefulset-index": "1"}, + }, + }, + expectedValue: "1", + }, { name: "invalid expression", @@ -99,6 +109,16 @@ func TestExtractFieldPathAsString(t *testing.T) { }, expectedMessageFragment: "unsupported fieldPath", }, + { + name: "invalid annotation", + fieldPath: "metadata.annotations['unknown.key']", + obj: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"foo": "bar"}, + }, + }, + expectedMessageFragment: "unsupported fieldPath", + }, } for _, tc := range cases { @@ -116,3 +136,79 @@ func TestExtractFieldPathAsString(t *testing.T) { } } } + +func TestSplitMaybeSubscriptedPath(t *testing.T) { + cases := []struct { + fieldPath string + expectedPath string + expectedSubscript string + }{ + { + fieldPath: "metadata.annotations['key']", + expectedPath: "metadata.annotations", + expectedSubscript: "key", + }, + { + fieldPath: "metadata.annotations['a[b']c']", + expectedPath: "metadata.annotations", + expectedSubscript: "a[b']c", + }, + { + fieldPath: "metadata.labels['['key']", + expectedPath: "metadata.labels", + expectedSubscript: "['key", + }, + { + fieldPath: "metadata.labels['key']']", + expectedPath: "metadata.labels", + expectedSubscript: "key']", + }, + { + fieldPath: "metadata.labels[ 'key' ]", + expectedPath: "metadata.labels[ 'key' ]", + expectedSubscript: "", + }, + { + fieldPath: "metadata.labels['']", + expectedPath: "metadata.labels['']", + expectedSubscript: "", + }, + { + fieldPath: "metadata.labels[' ']", + expectedPath: "metadata.labels", + expectedSubscript: " ", + }, + { + fieldPath: "metadata.labels[]", + expectedPath: "metadata.labels[]", + expectedSubscript: "", + }, + { + fieldPath: "metadata.labels[']", + expectedPath: "metadata.labels[']", + expectedSubscript: "", + }, + { + fieldPath: "metadata.labels['key']foo", + expectedPath: "metadata.labels['key']foo", + expectedSubscript: "", + }, + { + fieldPath: "['key']", + expectedPath: "['key']", + expectedSubscript: "", + }, + { + fieldPath: "metadata.labels", + expectedPath: "metadata.labels", + expectedSubscript: "", + }, + } + for _, tc := range cases { + path, subscript := SplitMaybeSubscriptedPath(tc.fieldPath) + if path != tc.expectedPath || subscript != tc.expectedSubscript { + t.Errorf("SplitMaybeSubscriptedPath(%q) = (%q, %q), expect (%q, %q)", + tc.fieldPath, path, subscript, tc.expectedPath, tc.expectedSubscript) + } + } +} diff --git a/pkg/volume/projected/projected_test.go b/pkg/volume/projected/projected_test.go index 4979cf97e0f..ec07a797355 100644 --- a/pkg/volume/projected/projected_test.go +++ b/pkg/volume/projected/projected_test.go @@ -544,6 +544,48 @@ func TestCollectDataWithDownwardAPI(t *testing.T) { payload map[string]util.FileProjection success bool }{ + { + name: "annotation", + volumeFile: []v1.DownwardAPIVolumeFile{ + {Path: "annotation", FieldRef: &v1.ObjectFieldSelector{ + FieldPath: "metadata.annotations['a1']"}}}, + pod: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testNamespace, + Annotations: map[string]string{ + "a1": "value1", + "a2": "value2", + }, + UID: testPodUID}, + }, + mode: 0644, + payload: map[string]util.FileProjection{ + "annotation": {Data: []byte("value1"), Mode: 0644}, + }, + success: true, + }, + { + name: "annotation-error", + volumeFile: []v1.DownwardAPIVolumeFile{ + {Path: "annotation", FieldRef: &v1.ObjectFieldSelector{ + FieldPath: "metadata.annotations['']"}}}, + pod: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: testPodName, + Namespace: testNamespace, + Annotations: map[string]string{ + "a1": "value1", + "a2": "value2", + }, + UID: testPodUID}, + }, + mode: 0644, + payload: map[string]util.FileProjection{ + "annotation": {Data: []byte("does-not-matter-because-this-test-case-will-fail-anyway"), Mode: 0644}, + }, + success: false, + }, { name: "labels", volumeFile: []v1.DownwardAPIVolumeFile{ From 34a7b3dea8739abfc113c4e42935289ac1e89d64 Mon Sep 17 00:00:00 2001 From: Yang Guo Date: Wed, 22 Nov 2017 11:02:20 -0800 Subject: [PATCH 2/3] Create a separate conversion function for the field labels used by downward API --- pkg/apis/core/BUILD | 1 + pkg/apis/core/pods/BUILD | 30 +++++++++ pkg/apis/core/pods/helpers.go | 51 +++++++++++++++ pkg/apis/core/pods/helpers_test.go | 87 ++++++++++++++++++++++++++ pkg/apis/core/v1/conversion.go | 7 +-- pkg/apis/core/validation/BUILD | 1 + pkg/apis/core/validation/validation.go | 4 +- pkg/kubelet/BUILD | 2 +- pkg/kubelet/kubelet_pods.go | 4 +- 9 files changed, 176 insertions(+), 11 deletions(-) create mode 100644 pkg/apis/core/pods/BUILD create mode 100644 pkg/apis/core/pods/helpers.go create mode 100644 pkg/apis/core/pods/helpers_test.go diff --git a/pkg/apis/core/BUILD b/pkg/apis/core/BUILD index ddd461bd522..a4bdda2a888 100644 --- a/pkg/apis/core/BUILD +++ b/pkg/apis/core/BUILD @@ -51,6 +51,7 @@ filegroup( "//pkg/apis/core/fuzzer:all-srcs", "//pkg/apis/core/helper:all-srcs", "//pkg/apis/core/install:all-srcs", + "//pkg/apis/core/pods:all-srcs", "//pkg/apis/core/v1:all-srcs", "//pkg/apis/core/validation:all-srcs", ], diff --git a/pkg/apis/core/pods/BUILD b/pkg/apis/core/pods/BUILD new file mode 100644 index 00000000000..b78e256e7c1 --- /dev/null +++ b/pkg/apis/core/pods/BUILD @@ -0,0 +1,30 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["helpers.go"], + importpath = "k8s.io/kubernetes/pkg/apis/core/pods", + visibility = ["//visibility:public"], + deps = ["//pkg/fieldpath:go_default_library"], +) + +go_test( + name = "go_default_test", + srcs = ["helpers_test.go"], + importpath = "k8s.io/kubernetes/pkg/apis/core/pods", + library = ":go_default_library", +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) diff --git a/pkg/apis/core/pods/helpers.go b/pkg/apis/core/pods/helpers.go new file mode 100644 index 00000000000..0b4d14db022 --- /dev/null +++ b/pkg/apis/core/pods/helpers.go @@ -0,0 +1,51 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pods + +import ( + "fmt" + + "k8s.io/kubernetes/pkg/fieldpath" +) + +func ConvertDownwardAPIFieldLabel(version, label, value string) (string, string, error) { + if version != "v1" { + return "", "", fmt.Errorf("unsupported pod version: %s", version) + } + + path, _ := fieldpath.SplitMaybeSubscriptedPath(label) + switch path { + case "metadata.annotations", + "metadata.labels", + "metadata.name", + "metadata.namespace", + "metadata.uid", + "spec.nodeName", + "spec.restartPolicy", + "spec.serviceAccountName", + "spec.schedulerName", + "status.phase", + "status.hostIP", + "status.podIP": + return label, value, nil + // This is for backwards compatibility with old v1 clients which send spec.host + case "spec.host": + return "spec.nodeName", value, nil + default: + return "", "", fmt.Errorf("field label not supported: %s", label) + } +} diff --git a/pkg/apis/core/pods/helpers_test.go b/pkg/apis/core/pods/helpers_test.go new file mode 100644 index 00000000000..9db95a014ba --- /dev/null +++ b/pkg/apis/core/pods/helpers_test.go @@ -0,0 +1,87 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pods + +import ( + "testing" +) + +func TestConvertDownwardAPIFieldLabel(t *testing.T) { + testCases := []struct { + version string + label string + value string + expectedErr bool + expectedLabel string + expectedValue string + }{ + { + version: "v2", + label: "metadata.name", + value: "test-pod", + expectedErr: true, + }, + { + version: "v1", + label: "invalid-label", + value: "value", + expectedErr: true, + }, + { + version: "v1", + label: "metadata.name", + value: "test-pod", + expectedLabel: "metadata.name", + expectedValue: "test-pod", + }, + { + version: "v1", + label: "metadata.annotations", + value: "myValue", + expectedLabel: "metadata.annotations", + expectedValue: "myValue", + }, + { + version: "v1", + label: "metadata.annotations['myKey']", + value: "myValue", + expectedLabel: "metadata.annotations['myKey']", + expectedValue: "myValue", + }, + { + version: "v1", + label: "spec.host", + value: "127.0.0.1", + expectedLabel: "spec.nodeName", + expectedValue: "127.0.0.1", + }, + } + for _, tc := range testCases { + label, value, err := ConvertDownwardAPIFieldLabel(tc.version, tc.label, tc.value) + if err != nil { + if tc.expectedErr { + continue + } + t.Errorf("ConvertDownwardAPIFieldLabel(%s, %s, %s) failed: %s", + tc.version, tc.label, tc.value, err) + } + if tc.expectedLabel != label || tc.expectedValue != value { + t.Errorf("ConvertDownwardAPIFieldLabel(%s, %s, %s) = (%s, %s, nil), expected (%s, %s, nil)", + tc.version, tc.label, tc.value, label, value, tc.expectedLabel, tc.expectedValue) + } + } +} diff --git a/pkg/apis/core/v1/conversion.go b/pkg/apis/core/v1/conversion.go index 70de6b9734a..371982f0fb3 100644 --- a/pkg/apis/core/v1/conversion.go +++ b/pkg/apis/core/v1/conversion.go @@ -159,17 +159,12 @@ func addConversionFuncs(scheme *runtime.Scheme) error { func(label, value string) (string, string, error) { path, _ := fieldpath.SplitMaybeSubscriptedPath(label) switch path { - case "metadata.annotations", - "metadata.labels", - "metadata.name", + case "metadata.name", "metadata.namespace", - "metadata.uid", "spec.nodeName", "spec.restartPolicy", - "spec.serviceAccountName", "spec.schedulerName", "status.phase", - "status.hostIP", "status.podIP": return label, value, nil // This is for backwards compatibility with old v1 clients which send spec.host diff --git a/pkg/apis/core/validation/BUILD b/pkg/apis/core/validation/BUILD index 18282ae464e..e2d08109daa 100644 --- a/pkg/apis/core/validation/BUILD +++ b/pkg/apis/core/validation/BUILD @@ -18,6 +18,7 @@ go_library( "//pkg/api/service:go_default_library", "//pkg/apis/core:go_default_library", "//pkg/apis/core/helper:go_default_library", + "//pkg/apis/core/pods:go_default_library", "//pkg/apis/core/v1:go_default_library", "//pkg/apis/core/v1/helper:go_default_library", "//pkg/capabilities:go_default_library", diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index cb063ad396d..b3b094e3e78 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -42,10 +42,10 @@ import ( "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" - "k8s.io/kubernetes/pkg/api/legacyscheme" apiservice "k8s.io/kubernetes/pkg/api/service" "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/helper" + podshelper "k8s.io/kubernetes/pkg/apis/core/pods" corev1 "k8s.io/kubernetes/pkg/apis/core/v1" v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" "k8s.io/kubernetes/pkg/capabilities" @@ -1964,7 +1964,7 @@ func validateObjectFieldSelector(fs *core.ObjectFieldSelector, expressions *sets return allErrs } - internalFieldPath, _, err := legacyscheme.Scheme.ConvertFieldLabel(fs.APIVersion, "Pod", fs.FieldPath, "") + internalFieldPath, _, err := podshelper.ConvertDownwardAPIFieldLabel(fs.APIVersion, fs.FieldPath, "") if err != nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("fieldPath"), fs.FieldPath, fmt.Sprintf("error converting fieldPath: %v", err))) return allErrs diff --git a/pkg/kubelet/BUILD b/pkg/kubelet/BUILD index 14874ca9aec..7fb53501242 100644 --- a/pkg/kubelet/BUILD +++ b/pkg/kubelet/BUILD @@ -29,10 +29,10 @@ go_library( ], importpath = "k8s.io/kubernetes/pkg/kubelet", deps = [ - "//pkg/api/legacyscheme:go_default_library", "//pkg/api/v1/pod:go_default_library", "//pkg/api/v1/resource:go_default_library", "//pkg/apis/core:go_default_library", + "//pkg/apis/core/pods:go_default_library", "//pkg/apis/core/v1:go_default_library", "//pkg/apis/core/v1/helper:go_default_library", "//pkg/apis/core/v1/helper/qos:go_default_library", diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 8a180747423..233787c86c5 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -43,9 +43,9 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/tools/remotecommand" - "k8s.io/kubernetes/pkg/api/legacyscheme" podutil "k8s.io/kubernetes/pkg/api/v1/pod" "k8s.io/kubernetes/pkg/api/v1/resource" + podshelper "k8s.io/kubernetes/pkg/apis/core/pods" v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" v1qos "k8s.io/kubernetes/pkg/apis/core/v1/helper/qos" "k8s.io/kubernetes/pkg/apis/core/v1/validation" @@ -733,7 +733,7 @@ func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container // podFieldSelectorRuntimeValue returns the runtime value of the given // selector for a pod. func (kl *Kubelet) podFieldSelectorRuntimeValue(fs *v1.ObjectFieldSelector, pod *v1.Pod, podIP string) (string, error) { - internalFieldPath, _, err := legacyscheme.Scheme.ConvertFieldLabel(fs.APIVersion, "Pod", fs.FieldPath, "") + internalFieldPath, _, err := podshelper.ConvertDownwardAPIFieldLabel(fs.APIVersion, fs.FieldPath, "") if err != nil { return "", err } From c74b97b29db12c851f31810896cc10979d110323 Mon Sep 17 00:00:00 2001 From: Yang Guo Date: Wed, 22 Nov 2017 12:38:09 -0800 Subject: [PATCH 3/3] Validate key subscript for metadata.annotations and metadata.labels separately --- pkg/apis/core/pods/helpers.go | 16 +++- pkg/apis/core/v1/BUILD | 1 - pkg/apis/core/v1/conversion.go | 4 +- pkg/apis/core/validation/validation.go | 30 ++++---- pkg/apis/core/validation/validation_test.go | 48 +++++++++++- pkg/fieldpath/BUILD | 5 +- pkg/fieldpath/fieldpath.go | 37 +++++---- pkg/fieldpath/fieldpath_test.go | 83 ++++++++++++++------- 8 files changed, 153 insertions(+), 71 deletions(-) diff --git a/pkg/apis/core/pods/helpers.go b/pkg/apis/core/pods/helpers.go index 0b4d14db022..cf199cee737 100644 --- a/pkg/apis/core/pods/helpers.go +++ b/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) - switch path { + 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", diff --git a/pkg/apis/core/v1/BUILD b/pkg/apis/core/v1/BUILD index a82315ad5fa..5796c614076 100644 --- a/pkg/apis/core/v1/BUILD +++ b/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", diff --git a/pkg/apis/core/v1/conversion.go b/pkg/apis/core/v1/conversion.go index 371982f0fb3..8f6e09b673e 100644 --- a/pkg/apis/core/v1/conversion.go +++ b/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 path { + switch label { case "metadata.name", "metadata.namespace", "spec.nodeName", diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index b3b094e3e78..18650b282e3 100644 --- a/pkg/apis/core/validation/validation.go +++ b/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,21 +1966,23 @@ func validateObjectFieldSelector(fs *core.ObjectFieldSelector, expressions *sets return allErrs } - path, subscript := fieldpath.SplitMaybeSubscriptedPath(internalFieldPath) - if len(subscript) > 0 { - // This is to indicate that the internalFieldPath has a subscript, so - // that we can compare the path against the allowed path set easily. - path += "[]" - } - if !expressions.Has(path) { + if path, subscript, ok := fieldpath.SplitMaybeSubscriptedPath(internalFieldPath); ok { + switch path { + case "metadata.annotations": + for _, msg := range validation.IsQualifiedName(strings.ToLower(subscript)) { + allErrs = append(allErrs, field.Invalid(fldPath, subscript, msg)) + } + case "metadata.labels": + 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 } - if len(subscript) > 0 { - for _, msg := range validation.IsQualifiedName(subscript) { - allErrs = append(allErrs, field.Invalid(fldPath, subscript, msg)) - } - } return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index ec9903e2558..71fc0aeb263 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/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 { diff --git a/pkg/fieldpath/BUILD b/pkg/fieldpath/BUILD index 054686cbd5c..0e4dae6a921 100644 --- a/pkg/fieldpath/BUILD +++ b/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( diff --git a/pkg/fieldpath/fieldpath.go b/pkg/fieldpath/fieldpath.go index 531a7d6a642..43754145892 100644 --- a/pkg/fieldpath/fieldpath.go +++ b/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 len(subscript) > 0 { + if path, subscript, ok := SplitMaybeSubscriptedPath(fieldPath); ok { 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). -// - if no, this function returns (fieldPath, ""). +// returns (path, subscript, true). +// - if no, this function returns (fieldPath, "", false). // // Example inputs and outputs: -// - "metadata.annotations['myKey']" --> ("metadata.annotations", "myKey") -// - "metadata.annotations['a[b]c']" --> ("metadata.annotations", "a[b]c") -// - "metadata.labels" --> ("metadata.labels", "") -// - "metadata.labels['']" --> ("metadata.labels", "") -func SplitMaybeSubscriptedPath(fieldPath string) (string, string) { +// - "metadata.annotations['myKey']" --> ("metadata.annotations", "myKey", true) +// - "metadata.annotations['a[b]c']" --> ("metadata.annotations", "a[b]c", true) +// - "metadata.labels['']" --> ("metadata.labels", "", true) +// - "metadata.labels" --> ("metadata.labels", "", false) +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 { - return fieldPath, "" + if len(parts[0]) == 0 { + return fieldPath, "", false } - return parts[0], parts[1] + return parts[0], parts[1], true } diff --git a/pkg/fieldpath/fieldpath_test.go b/pkg/fieldpath/fieldpath_test.go index f5eaba11618..aa87f02a8e1 100644 --- a/pkg/fieldpath/fieldpath_test.go +++ b/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", - fieldPath: "metadata.annotations['unknown.key']", + name: "invalid annotation 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']", - }, - { - fieldPath: "metadata.labels[ 'key' ]", - expectedPath: "metadata.labels[ 'key' ]", - expectedSubscript: "", + expectedOK: true, }, { fieldPath: "metadata.labels['']", - expectedPath: "metadata.labels['']", + expectedPath: "metadata.labels", expectedSubscript: "", + expectedOK: true, }, { fieldPath: "metadata.labels[' ']", expectedPath: "metadata.labels", expectedSubscript: " ", + expectedOK: true, }, { - fieldPath: "metadata.labels[]", - expectedPath: "metadata.labels[]", - expectedSubscript: "", + fieldPath: "metadata.labels[ 'key' ]", + expectedOK: false, }, { - fieldPath: "metadata.labels[']", - expectedPath: "metadata.labels[']", - expectedSubscript: "", + fieldPath: "metadata.labels[]", + expectedOK: false, }, { - fieldPath: "metadata.labels['key']foo", - expectedPath: "metadata.labels['key']foo", - expectedSubscript: "", + fieldPath: "metadata.labels[']", + expectedOK: false, }, { - fieldPath: "['key']", - expectedPath: "['key']", - expectedSubscript: "", + fieldPath: "metadata.labels['key']foo", + expectedOK: false, }, { - fieldPath: "metadata.labels", - expectedPath: "metadata.labels", - expectedSubscript: "", + fieldPath: "['key']", + expectedOK: false, + }, + { + fieldPath: "metadata.labels", + expectedOK: false, }, } 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) } }