oc new-app: allow 'dot' in ENV variable names and annotations

fixes: openshift#8771

reuses validators from kubernetes/kubernetes#48986

Author: Jan Wozniak

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)
 		}
 	}

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 {

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 dots.in.a.key=dots.in.a.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=dots.in.a.key-' '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\:dots.in.a.key'
 
 # 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'

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 <stream_name>:<tag>'
-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:'