From 9849a553ad63c4af76d06040c219a7875f9a7f16 Mon Sep 17 00:00:00 2001 From: Jan Wozniak Date: Fri, 11 May 2018 14:11:41 +0200 Subject: [PATCH] oc new-app: allow 'dot' in ENV variable names and annotations fixes: https://github.com/openshift/origin/issues/8771 reuses validators from https://github.com/kubernetes/kubernetes/pull/48986 --- pkg/oc/generate/app/env.go | 9 ++++----- pkg/oc/util/env/env.go | 21 ++++++++------------- test/cmd/env.sh | 6 +++++- test/cmd/images.sh | 3 ++- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/pkg/oc/generate/app/env.go b/pkg/oc/generate/app/env.go index 8186307d3874..9a366e27b74f 100644 --- a/pkg/oc/generate/app/env.go +++ b/pkg/oc/generate/app/env.go @@ -9,7 +9,7 @@ import ( "strings" "github.com/joho/godotenv" - utilenv "github.com/openshift/origin/pkg/oc/util/env" + "k8s.io/apimachinery/pkg/util/validation" kapi "k8s.io/kubernetes/pkg/apis/core" ) @@ -39,10 +39,9 @@ func ParseEnvironment(vals ...string) (Environment, []string, []error) { duplicates := []string{} env := make(Environment) for _, s := range vals { - valid := utilenv.IsValidEnvironmentArgument(s) p := strings.SplitN(s, "=", 2) - if !valid || len(p) != 2 { - errs = append(errs, fmt.Errorf("invalid parameter assignment in %q", s)) + if err := validation.IsEnvVarName(p[0]); len(err) != 0 || len(p) != 2 { + errs = append(errs, fmt.Errorf("invalid parameter assignment in %q, %v", s, err)) continue } key, val := p[0], p[1] @@ -166,7 +165,7 @@ func LoadEnvironmentFile(filename string, stdin io.Reader) (Environment, error) return nil, fmt.Errorf("Cannot read variables from file %q: %s", errorFilename, err) } for k, v := range env { - if !utilenv.IsValidEnvironmentArgument(fmt.Sprintf("%s=%s", k, v)) { + if err := validation.IsEnvVarName(k); len(err) != 0 { return nil, fmt.Errorf("invalid parameter assignment in %s=%s", k, v) } } diff --git a/pkg/oc/util/env/env.go b/pkg/oc/util/env/env.go index 9b81f3510d93..94bbfc7d79bc 100644 --- a/pkg/oc/util/env/env.go +++ b/pkg/oc/util/env/env.go @@ -8,20 +8,16 @@ import ( "strings" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation" kapi "k8s.io/kubernetes/pkg/apis/core" ) var argumentEnvironment = regexp.MustCompile(`(?ms)^(.+)\=(.*)$`) -var validArgumentEnvironment = regexp.MustCompile(`(?ms)^(\w+)\=(.*)$`) func IsEnvironmentArgument(s string) bool { return argumentEnvironment.MatchString(s) } -func IsValidEnvironmentArgument(s string) bool { - return validArgumentEnvironment.MatchString(s) -} - func SplitEnvironmentFromResources(args []string) (resources, envArgs []string, ok bool) { first := true for _, s := range args { @@ -50,8 +46,6 @@ func parseIntoEnvVar(spec []string, defaultReader io.Reader, envVarType string) var remove []string for _, envSpec := range spec { switch { - case !IsValidEnvironmentArgument(envSpec) && !strings.HasSuffix(envSpec, "-"): - return nil, nil, fmt.Errorf("%ss must be of the form key=value and can only contain letters, numbers, and underscores", envVarType) case envSpec == "-": if defaultReader == nil { return nil, nil, fmt.Errorf("when '-' is used, STDIN must be open") @@ -63,18 +57,19 @@ func parseIntoEnvVar(spec []string, defaultReader io.Reader, envVarType string) env = append(env, fileEnv...) case strings.Contains(envSpec, "="): parts := strings.SplitN(envSpec, "=", 2) - if len(parts) != 2 { - return nil, nil, fmt.Errorf("invalid %s: %v", envVarType, envSpec) + n, v := parts[0], parts[1] + if errs := validation.IsEnvVarName(n); len(errs) != 0 { + return nil, nil, fmt.Errorf("%ss must be of the form key=value, but is %q", envVarType, envSpec) } - exists.Insert(parts[0]) + exists.Insert(n) env = append(env, kapi.EnvVar{ - Name: parts[0], - Value: parts[1], + Name: n, + Value: v, }) case strings.HasSuffix(envSpec, "-"): remove = append(remove, envSpec[:len(envSpec)-1]) default: - return nil, nil, fmt.Errorf("unknown %s: %v", envVarType, envSpec) + return nil, nil, fmt.Errorf("%ss must be of the form key=value, but is %q", envVarType, envSpec) } } for _, removeLabel := range remove { diff --git a/test/cmd/env.sh b/test/cmd/env.sh index a4c1aa86a9bc..44eaeea928cd 100755 --- a/test/cmd/env.sh +++ b/test/cmd/env.sh @@ -11,13 +11,17 @@ os::cmd::expect_success_and_text 'oc set env dc/node --list' 'deploymentconfigs os::cmd::expect_success_and_text 'oc set env dc --all --containers="node" key-' 'deploymentconfig "node" updated' os::cmd::expect_failure_and_text 'oc set env dc --all --containers="node"' 'error: at least one environment variable must be provided' os::cmd::expect_failure_and_not_text 'oc set env --from=secret/mysecret dc/node' 'error: at least one environment variable must be provided' -os::cmd::expect_failure_and_text 'oc set env dc/node test#abc=1234' 'environment variables must be of the form key=value' +os::cmd::expect_failure_and_text 'oc set env dc/node test#abc=1234' 'environment variables must be of the form key=value, but is "test#abc=1234"' # ensure deleting a var through --env does not result in an error message os::cmd::expect_success_and_text 'oc set env dc/node key=value' 'deploymentconfig "node" updated' +os::cmd::expect_success_and_text 'oc set env dc/node key.with.dots=value' 'deploymentconfig "node" updated' os::cmd::expect_success_and_text 'oc set env dc --all --containers="node" --env=key-' 'deploymentconfig "node" updated' # ensure deleting a var through --env actually deletes the env var os::cmd::expect_success_and_not_text "oc get dc/node -o jsonpath='{ .spec.template.spec.containers[?(@.name==\"node\")].env }'" 'name\:key' +os::cmd::expect_success_and_text "oc get dc/node -o jsonpath='{ .spec.template.spec.containers[?(@.name==\"node\")].env }'" 'name\:key.with.dots' +os::cmd::expect_success_and_text 'oc set env dc --all --containers="node" --env=key.with.dots-' 'deploymentconfig "node" updated' +os::cmd::expect_success_and_not_text "oc get dc/node -o jsonpath='{ .spec.template.spec.containers[?(@.name==\"node.with.dots\")].env }'" 'name\:key.with.dots' # check that env vars are not split at commas os::cmd::expect_success_and_text 'oc set env -o yaml dc/node PASS=x,y=z' 'value: x,y=z' diff --git a/test/cmd/images.sh b/test/cmd/images.sh index 1ec54e69bb69..68414103e2db 100755 --- a/test/cmd/images.sh +++ b/test/cmd/images.sh @@ -98,9 +98,10 @@ os::cmd::try_until_success 'oc get imagestreamtags tag:8' os::cmd::expect_success 'oc create imagestreamtag tag:9 --scheduled --reference-policy=Local --from-image=mysql:latest' os::cmd::expect_success 'oc create imagestream tag-b' os::cmd::expect_success 'oc create imagestreamtag tag-b:1 --from=wildfly:12.0' +os::cmd::expect_success 'oc create imagestreamtag tag-c:1 -A annotation.with.dots=are.ok' os::cmd::expect_failure_and_text 'oc create imagestreamtag tag-c --from-image=mysql:latest' 'must be of the form :' -os::cmd::expect_failure_and_text 'oc create imagestreamtag tag-c:1 -A foo' 'annotations must be of the form key=value' +os::cmd::expect_failure_and_text 'oc create imagestreamtag tag-c:1 -A foo' 'annotations must be of the form key=value, but is "foo"' os::cmd::expect_failure_and_text 'oc create imagestreamtag tag-c:2 --from=mysql --from-image=mysql:latest' '\--from and --from-image may not be used together' os::cmd::expect_success_and_text 'oc get istag/tag:1 -o jsonpath={.image.dockerImageReference}' 'wildfly.*@sha256:'