From 604dfb319775090f715fbf2270ded63ce7474f15 Mon Sep 17 00:00:00 2001 From: Timo Reimann Date: Sun, 18 Jun 2017 00:34:22 +0200 Subject: [PATCH] Relax restrictions on environment variable names. The POSIX standard restricts environment variable names to uppercase letters, digits, and the underscore character in shell contexts only. For generic application usage, it is stated that all other characters shall be tolerated. This change relaxes the rules to some degree. Namely, we stop requiring environment variable names to be strict C_IDENTIFIERS and start permitting lowercase, dot, and dash characters. Public container images using environment variable names beyond the shell-only context can benefit from this relaxation. Elasticsearch is one popular example. --- pkg/api/types.go | 2 +- pkg/api/validation/validation.go | 4 +- pkg/api/validation/validation_test.go | 47 +++++++++++++++---- pkg/kubectl/configmap_test.go | 2 +- pkg/kubectl/env_file.go | 2 +- pkg/kubectl/run.go | 2 +- pkg/kubectl/run_test.go | 5 ++ pkg/kubectl/secret_test.go | 2 +- pkg/kubelet/kubelet_pods.go | 4 +- pkg/kubelet/kubelet_pods_test.go | 12 ++--- .../pkg/util/validation/validation.go | 37 ++++++++++++--- 11 files changed, 88 insertions(+), 31 deletions(-) diff --git a/pkg/api/types.go b/pkg/api/types.go index 171bb1d828d..ef84c2794f4 100644 --- a/pkg/api/types.go +++ b/pkg/api/types.go @@ -1440,7 +1440,7 @@ type SecretKeySelector struct { // EnvFromSource represents the source of a set of ConfigMaps type EnvFromSource struct { - // An optional identifier to prepend to each key in the ConfigMap. Must be a C_IDENTIFIER. + // An optional identifier to prepend to each key in the ConfigMap. // +optional Prefix string // The ConfigMap to select from. diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index f6bd510bfdd..d0f4eed9cee 100644 --- a/pkg/api/validation/validation.go +++ b/pkg/api/validation/validation.go @@ -1548,7 +1548,7 @@ func ValidateEnv(vars []api.EnvVar, fldPath *field.Path) field.ErrorList { if len(ev.Name) == 0 { allErrs = append(allErrs, field.Required(idxPath.Child("name"), "")) } else { - for _, msg := range validation.IsCIdentifier(ev.Name) { + for _, msg := range validation.IsEnvVarName(ev.Name) { allErrs = append(allErrs, field.Invalid(idxPath.Child("name"), ev.Name, msg)) } } @@ -1637,7 +1637,7 @@ func ValidateEnvFrom(vars []api.EnvFromSource, fldPath *field.Path) field.ErrorL for i, ev := range vars { idxPath := fldPath.Index(i) if len(ev.Prefix) > 0 { - for _, msg := range validation.IsCIdentifier(ev.Prefix) { + for _, msg := range validation.IsEnvVarName(ev.Prefix) { allErrs = append(allErrs, field.Invalid(idxPath.Child("prefix"), ev.Prefix, msg)) } } diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go index 6f0940d03e1..77908b03e5e 100644 --- a/pkg/api/validation/validation_test.go +++ b/pkg/api/validation/validation_test.go @@ -43,7 +43,7 @@ const ( maxLengthErrMsg = "must be no more than" namePartErrMsg = "name part must consist of" nameErrMsg = "a qualified name must consist of" - idErrMsg = "a valid C identifier must" + envVarNameErrMsg = "a valid environment variable name must consist of" ) func testVolume(name string, namespace string, spec api.PersistentVolumeSpec) *api.PersistentVolume { @@ -2575,6 +2575,8 @@ func TestValidateEnv(t *testing.T) { {Name: "ABC", Value: "value"}, {Name: "AbC_123", Value: "value"}, {Name: "abc", Value: ""}, + {Name: "a.b.c", Value: "value"}, + {Name: "a-b-c", Value: "value"}, { Name: "abc", ValueFrom: &api.EnvVarSource{ @@ -2676,9 +2678,24 @@ func TestValidateEnv(t *testing.T) { expectedError: "[0].name: Required value", }, { - name: "name not a C identifier", - envs: []api.EnvVar{{Name: "a.b.c"}}, - expectedError: `[0].name: Invalid value: "a.b.c": ` + idErrMsg, + name: "illegal character", + envs: []api.EnvVar{{Name: "a!b"}}, + expectedError: `[0].name: Invalid value: "a!b": ` + envVarNameErrMsg, + }, + { + name: "dot only", + envs: []api.EnvVar{{Name: "."}}, + expectedError: `[0].name: Invalid value: ".": must not be`, + }, + { + name: "double dots only", + envs: []api.EnvVar{{Name: ".."}}, + expectedError: `[0].name: Invalid value: "..": must not be`, + }, + { + name: "leading double dots", + envs: []api.EnvVar{{Name: "..abc"}}, + expectedError: `[0].name: Invalid value: "..abc": must not start with`, }, { name: "value and valueFrom specified", @@ -2897,6 +2914,12 @@ func TestValidateEnvFrom(t *testing.T) { LocalObjectReference: api.LocalObjectReference{Name: "abc"}, }, }, + { + Prefix: "a.b", + ConfigMapRef: &api.ConfigMapEnvSource{ + LocalObjectReference: api.LocalObjectReference{Name: "abc"}, + }, + }, { SecretRef: &api.SecretEnvSource{ LocalObjectReference: api.LocalObjectReference{Name: "abc"}, @@ -2908,6 +2931,12 @@ func TestValidateEnvFrom(t *testing.T) { LocalObjectReference: api.LocalObjectReference{Name: "abc"}, }, }, + { + Prefix: "a.b", + SecretRef: &api.SecretEnvSource{ + LocalObjectReference: api.LocalObjectReference{Name: "abc"}, + }, + }, } if errs := ValidateEnvFrom(successCase, field.NewPath("field")); len(errs) != 0 { t.Errorf("expected success: %v", errs) @@ -2942,12 +2971,12 @@ func TestValidateEnvFrom(t *testing.T) { name: "invalid prefix", envs: []api.EnvFromSource{ { - Prefix: "a.b", + Prefix: "a!b", ConfigMapRef: &api.ConfigMapEnvSource{ LocalObjectReference: api.LocalObjectReference{Name: "abc"}}, }, }, - expectedError: `field[0].prefix: Invalid value: "a.b": ` + idErrMsg, + expectedError: `field[0].prefix: Invalid value: "a!b": ` + envVarNameErrMsg, }, { name: "zero-length name", @@ -2973,12 +3002,12 @@ func TestValidateEnvFrom(t *testing.T) { name: "invalid prefix", envs: []api.EnvFromSource{ { - Prefix: "a.b", + Prefix: "a!b", SecretRef: &api.SecretEnvSource{ LocalObjectReference: api.LocalObjectReference{Name: "abc"}}, }, }, - expectedError: `field[0].prefix: Invalid value: "a.b": ` + idErrMsg, + expectedError: `field[0].prefix: Invalid value: "a!b": ` + envVarNameErrMsg, }, { name: "no refs", @@ -3374,7 +3403,7 @@ func TestValidateContainers(t *testing.T) { ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, "invalid env var name": { - {Name: "abc", Image: "image", Env: []api.EnvVar{{Name: "ev.1"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, + {Name: "abc", Image: "image", Env: []api.EnvVar{{Name: "ev!1"}}, ImagePullPolicy: "IfNotPresent", TerminationMessagePolicy: "File"}, }, "unknown volume name": { {Name: "abc", Image: "image", VolumeMounts: []api.VolumeMount{{Name: "anything", MountPath: "/foo"}}, diff --git a/pkg/kubectl/configmap_test.go b/pkg/kubectl/configmap_test.go index 5690c2e0f0a..b5c98b58f4c 100644 --- a/pkg/kubectl/configmap_test.go +++ b/pkg/kubectl/configmap_test.go @@ -157,7 +157,7 @@ func TestConfigMapGenerate(t *testing.T) { expectErr: true, }, { - setup: setupEnvFile("key.1=value1"), + setup: setupEnvFile("key#1=value1"), params: map[string]interface{}{ "name": "invalid_key", "from-env-file": "file.env", diff --git a/pkg/kubectl/env_file.go b/pkg/kubectl/env_file.go index ce24c710441..19598d5354a 100644 --- a/pkg/kubectl/env_file.go +++ b/pkg/kubectl/env_file.go @@ -55,7 +55,7 @@ func proccessEnvFileLine(line []byte, filePath string, data := strings.SplitN(string(line), "=", 2) key = data[0] - if errs := validation.IsCIdentifier(key); len(errs) != 0 { + if errs := validation.IsEnvVarName(key); len(errs) != 0 { return ``, ``, fmt.Errorf("%q is not a valid key name: %s", key, strings.Join(errs, ";")) } diff --git a/pkg/kubectl/run.go b/pkg/kubectl/run.go index e1f4549d62c..dbf9ba7d0c0 100644 --- a/pkg/kubectl/run.go +++ b/pkg/kubectl/run.go @@ -868,7 +868,7 @@ func parseEnvs(envArray []string) ([]v1.EnvVar, error) { if len(name) == 0 { return nil, fmt.Errorf("invalid env: %v", env) } - if len(validation.IsCIdentifier(name)) != 0 { + if len(validation.IsEnvVarName(name)) != 0 { return nil, fmt.Errorf("invalid env: %v", env) } envVar := v1.EnvVar{Name: name, Value: value} diff --git a/pkg/kubectl/run_test.go b/pkg/kubectl/run_test.go index 16a6e9423ab..114550ff3b2 100644 --- a/pkg/kubectl/run_test.go +++ b/pkg/kubectl/run_test.go @@ -1030,6 +1030,7 @@ func TestParseEnv(t *testing.T) { { envArray: []string{ "THIS_ENV=isOK", + "this.dotted.env=isOKToo", "HAS_COMMAS=foo,bar", "HAS_EQUALS=jJnro54iUu75xNy==", }, @@ -1038,6 +1039,10 @@ func TestParseEnv(t *testing.T) { Name: "THIS_ENV", Value: "isOK", }, + { + Name: "this.dotted.env", + Value: "isOKToo", + }, { Name: "HAS_COMMAS", Value: "foo,bar", diff --git a/pkg/kubectl/secret_test.go b/pkg/kubectl/secret_test.go index 309c7848d65..e7c1e7478be 100644 --- a/pkg/kubectl/secret_test.go +++ b/pkg/kubectl/secret_test.go @@ -157,7 +157,7 @@ func TestSecretGenerate(t *testing.T) { expectErr: true, }, { - setup: setupEnvFile("key.1=value1"), + setup: setupEnvFile("key#1=value1"), params: map[string]interface{}{ "name": "invalid_key", "from-env-file": "file.env", diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index a0651a680d9..ddc62038d03 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -472,7 +472,7 @@ func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container if len(envFrom.Prefix) > 0 { k = envFrom.Prefix + k } - if errMsgs := utilvalidation.IsCIdentifier(k); len(errMsgs) != 0 { + if errMsgs := utilvalidation.IsEnvVarName(k); len(errMsgs) != 0 { invalidKeys = append(invalidKeys, k) continue } @@ -507,7 +507,7 @@ func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container if len(envFrom.Prefix) > 0 { k = envFrom.Prefix + k } - if errMsgs := utilvalidation.IsCIdentifier(k); len(errMsgs) != 0 { + if errMsgs := utilvalidation.IsEnvVarName(k); len(errMsgs) != 0 { invalidKeys = append(invalidKeys, k) continue } diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index 16b8bf68b89..986de4fac8c 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -1219,14 +1219,14 @@ func TestMakeEnvironmentVariables(t *testing.T) { Name: "test-secret", }, Data: map[string][]byte{ - "1234": []byte("abc"), - "1z": []byte("abc"), - "key": []byte("value"), + "1234": []byte("abc"), + "1z": []byte("abc"), + "key.1": []byte("value"), }, }, expectedEnvs: []kubecontainer.EnvVar{ { - Name: "key", + Name: "key.1", Value: "value", }, }, @@ -1250,12 +1250,12 @@ func TestMakeEnvironmentVariables(t *testing.T) { Name: "test-secret", }, Data: map[string][]byte{ - "1234": []byte("abc"), + "1234.name": []byte("abc"), }, }, expectedEnvs: []kubecontainer.EnvVar{ { - Name: "p_1234", + Name: "p_1234.name", Value: "abc", }, }, diff --git a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go index b1fcc570812..85ba8467bb8 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go @@ -277,6 +277,22 @@ func IsHTTPHeaderName(value string) []string { return nil } +const envVarNameFmt = "[-._a-zA-Z][-._a-zA-Z0-9]*" +const envVarNameFmtErrMsg string = "a valid environment variable name must consist of alphabetic characters, digits, '_', '-', or '.', and must not start with a digit" + +var envVarNameRegexp = regexp.MustCompile("^" + envVarNameFmt + "$") + +// IsEnvVarName tests if a string is a valid environment variable name. +func IsEnvVarName(value string) []string { + var errs []string + if !envVarNameRegexp.MatchString(value) { + errs = append(errs, RegexError(envVarNameFmtErrMsg, envVarNameFmt, "my.env-name", "MY_ENV.NAME", "MyEnvName1")) + } + + errs = append(errs, hasChDirPrefix(value)...) + return errs +} + const configMapKeyFmt = `[-._a-zA-Z0-9]+` const configMapKeyErrMsg string = "a valid config key must consist of alphanumeric characters, '-', '_' or '.'" @@ -291,13 +307,7 @@ func IsConfigMapKey(value string) []string { if !configMapKeyRegexp.MatchString(value) { errs = append(errs, RegexError(configMapKeyErrMsg, configMapKeyFmt, "key.name", "KEY_NAME", "key-name")) } - if value == "." { - errs = append(errs, `must not be '.'`) - } else if value == ".." { - errs = append(errs, `must not be '..'`) - } else if strings.HasPrefix(value, "..") { - errs = append(errs, `must not start with '..'`) - } + errs = append(errs, hasChDirPrefix(value)...) return errs } @@ -341,3 +351,16 @@ func prefixEach(msgs []string, prefix string) []string { func InclusiveRangeError(lo, hi int) string { return fmt.Sprintf(`must be between %d and %d, inclusive`, lo, hi) } + +func hasChDirPrefix(value string) []string { + var errs []string + switch { + case value == ".": + errs = append(errs, `must not be '.'`) + case value == "..": + errs = append(errs, `must not be '..'`) + case strings.HasPrefix(value, ".."): + errs = append(errs, `must not start with '..'`) + } + return errs +}