Fix up handling of environment variables

The way docker works is if a user specifies a non `-e Name=Value`, IE
just a `-e Name`, then the environment variable Name from the clients
OS.ENV is used.

Also by default Docker containers run with the HOSTNAME environment set
to the HOSTNAME specified for the container.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Closes: #21
Approved by: baude

.papr.yml

@@ -19,8 +19,8 @@ tests:
 
 inherit: true
 host:
-  distro: centos/7/atomic/alpha
+  distro: centos/7/atomic/smoketested
   specs:
      ram: 8192
 
-context: centos/7/atomic/alpha
+context: centos/7/atomic/smoketested

cmd/kpod/create.go

@@ -27,7 +27,10 @@ const (
 )
 
 var (
-	defaultEnvVariables = []string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", "TERM=xterm"}
+	defaultEnvVariables = map[string]string{
+		"PATH": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
+		"TERM": "xterm",
+	}
 )
 
 type createResourceConfig struct {
@@ -64,16 +67,16 @@ type createConfig struct {
 	cidFile            string
 	cgroupParent       string // cgroup-parent
 	command            []string
-	detach             bool         // detach
+	detach             bool              // detach
-	devices            []*pb.Device // device
+	devices            []*pb.Device      // device
-	dnsOpt             []string     //dns-opt
+	dnsOpt             []string          //dns-opt
-	dnsSearch          []string     //dns-search
+	dnsSearch          []string          //dns-search
-	dnsServers         []string     //dns
+	dnsServers         []string          //dns
-	entrypoint         string       //entrypoint
+	entrypoint         string            //entrypoint
-	env                []string     //env
+	env                map[string]string //env
-	expose             []string     //expose
+	expose             []string          //expose
-	groupAdd           []uint32     // group-add
+	groupAdd           []uint32          // group-add
-	hostname           string       //hostname
+	hostname           string            //hostname
 	image              string
 	interactive        bool              //interactive
 	ip6Address         string            //ipv6
@@ -264,8 +267,8 @@ func parseCreateOpts(c *cli.Context, runtime *libpod.Runtime) (*createConfig, er
 		return &createConfig{}, errors.Wrapf(err, "unable to process labels")
 	}
 	// ENVIRONMENT VARIABLES
-	env, err := getAllEnvironmentVariables(c.StringSlice("env-file"), c.StringSlice("env"))
+	env := defaultEnvVariables
-	if err != nil {
+	if err := readKVStrings(env, c.StringSlice("env-file"), c.StringSlice("env")); err != nil {
 		return &createConfig{}, errors.Wrapf(err, "unable to process environment variables")
 	}
 
@@ -338,7 +341,7 @@ func parseCreateOpts(c *cli.Context, runtime *libpod.Runtime) (*createConfig, er
 		dnsServers:     c.StringSlice("dns"),
 		entrypoint:     c.String("entrypoint"),
 		env:            env,
-		expose:         c.StringSlice("env"),
+		expose:         c.StringSlice("expose"),
 		groupAdd:       groupAdd,
 		hostname:       c.String("hostname"),
 		image:          image,

cmd/kpod/create_cli.go

@@ -7,44 +7,14 @@ import (
 )
 
 func getAllLabels(labelFile, inputLabels []string) (map[string]string, error) {
-	var labelValues []string
 	labels := make(map[string]string)
-	labelValues, labelErr := readKVStrings(labelFile, inputLabels)
+	labelErr := readKVStrings(labels, labelFile, inputLabels)
 	if labelErr != nil {
 		return labels, errors.Wrapf(labelErr, "unable to process labels from --label and label-file")
 	}
-	// Process KEY=VALUE stringslice in string map for WithLabels func
-	if len(labelValues) > 0 {
-		for _, i := range labelValues {
-			spliti := strings.Split(i, "=")
-			if len(spliti) < 2 {
-				return labels, errors.Errorf("labels must be in KEY=VALUE format: %s is invalid", i)
-			}
-			labels[spliti[0]] = spliti[1]
-		}
-	}
 	return labels, nil
 }
 
-func getAllEnvironmentVariables(envFiles, envInput []string) ([]string, error) {
-	env, err := readKVStrings(envFiles, envInput)
-	if err != nil {
-		return []string{}, errors.Wrapf(err, "unable to process variables from --env and --env-file")
-	}
-	// Add default environment variables if nothing defined
-	if len(env) == 0 {
-		env = append(env, defaultEnvVariables...)
-	}
-	// Each environment variable must be in the K=V format
-	for _, i := range env {
-		spliti := strings.Split(i, "=")
-		if len(spliti) != 2 {
-			return env, errors.Errorf("environment variables must be in the format KEY=VALUE: %s is invalid", i)
-		}
-	}
-	return env, nil
-}
-
 func convertStringSliceToMap(strSlice []string, delimiter string) (map[string]string, error) {
 	sysctl := make(map[string]string)
 	for _, inputSysctl := range strSlice {

cmd/kpod/create_cli_test.go

@@ -68,32 +68,3 @@ func TestGetAllLabelsFile(t *testing.T) {
 	result, _ := getAllLabels(fileLabels, Var1)
 	assert.Equal(t, len(result), 3)
 }
-
-func TestGetAllEnvironmentVariables(t *testing.T) {
-	fileEnvs := []string{}
-	result, _ := getAllEnvironmentVariables(fileEnvs, Var1)
-	assert.Equal(t, len(result), 2)
-}
-
-func TestGetAllEnvironmentVariablesBadKeyValue(t *testing.T) {
-	inEnvs := []string{"ONE1", "TWO=2"}
-	fileEnvs := []string{}
-	_, err := getAllEnvironmentVariables(fileEnvs, inEnvs)
-	assert.Error(t, err, assert.AnError)
-}
-
-func TestGetAllEnvironmentVariablesBadEnvFile(t *testing.T) {
-	fileEnvs := []string{"/foobar5001/be"}
-	_, err := getAllEnvironmentVariables(fileEnvs, Var1)
-	assert.Error(t, err, assert.AnError)
-}
-
-func TestGetAllEnvironmentVariablesFile(t *testing.T) {
-	content := []byte("THREE=3")
-	tFile, err := createTmpFile(content)
-	defer os.Remove(tFile)
-	assert.NoError(t, err)
-	fileEnvs := []string{tFile}
-	result, _ := getAllEnvironmentVariables(fileEnvs, Var1)
-	assert.Equal(t, len(result), 3)
-}

cmd/kpod/parse.go

@@ -327,55 +327,62 @@ func doesEnvExist(name string) bool {
 // reads a file of line terminated key=value pairs, and overrides any keys
 // present in the file with additional pairs specified in the override parameter
 // for env-file and labels-file flags
-func readKVStrings(files []string, override []string) ([]string, error) {
+func readKVStrings(env map[string]string, files []string, override []string) error {
-	envVariables := []string{}
 	for _, ef := range files {
-		parsedVars, err := parseEnvFile(ef)
+		if err := parseEnvFile(env, ef); err != nil {
-		if err != nil {
+			return err
-			return nil, err
 		}
-		envVariables = append(envVariables, parsedVars...)
 	}
-	// parse the '-e' and '--env' after, to allow override
+	for _, line := range override {
-	envVariables = append(envVariables, override...)
+		if err := parseEnv(env, line); err != nil {
+			return err
+		}
+	}
+	return nil
+}
 
-	return envVariables, nil
+func parseEnv(env map[string]string, line string) error {
+	data := strings.SplitN(line, "=", 2)
+
+	// trim the front of a variable, but nothing else
+	name := strings.TrimLeft(data[0], whiteSpaces)
+	if strings.ContainsAny(name, whiteSpaces) {
+		return errors.Errorf("name %q has white spaces, poorly formatted name", name)
+	}
+
+	if len(data) > 1 {
+		env[name] = data[1]
+	} else {
+		// if only a pass-through variable is given, clean it up.
+		val, exists := os.LookupEnv(name)
+		if !exists {
+			return errors.Errorf("environment variable %q does not exist", name)
+		}
+		env[name] = val
+	}
+	return nil
 }
 
 // parseEnvFile reads a file with environment variables enumerated by lines
-func parseEnvFile(filename string) ([]string, error) {
+func parseEnvFile(env map[string]string, filename string) error {
 	fh, err := os.Open(filename)
 	if err != nil {
-		return []string{}, err
+		return err
 	}
 	defer fh.Close()
 
-	lines := []string{}
 	scanner := bufio.NewScanner(fh)
 	for scanner.Scan() {
 		// trim the line from all leading whitespace first
 		line := strings.TrimLeft(scanner.Text(), whiteSpaces)
 		// line is not empty, and not starting with '#'
 		if len(line) > 0 && !strings.HasPrefix(line, "#") {
-			data := strings.SplitN(line, "=", 2)
+			if err := parseEnv(env, line); err != nil {
-
+				return err
-			// trim the front of a variable, but nothing else
-			variable := strings.TrimLeft(data[0], whiteSpaces)
-			if strings.ContainsAny(variable, whiteSpaces) {
-				return []string{}, errors.Errorf("variable %q has white spaces, poorly formatted environment", variable)
-			}
-
-			if len(data) > 1 {
-
-				// pass the value through, no trimming
-				lines = append(lines, fmt.Sprintf("%s=%s", variable, data[1]))
-			} else {
-				// if only a pass-through variable is given, clean it up.
-				lines = append(lines, fmt.Sprintf("%s=%s", strings.TrimSpace(line), os.Getenv(line)))
 			}
 		}
 	}
-	return lines, scanner.Err()
+	return scanner.Err()
 }
 
 // NsIpc represents the container ipc stack.

cmd/kpod/spec.go

@@ -52,6 +52,9 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 	}
 	g.SetRootReadonly(config.readOnlyRootfs)
 	g.SetHostname(config.hostname)
+	if config.hostname != "" {
+		g.AddProcessEnv("HOSTNAME", config.hostname)
+	}
 
 	for _, sysctl := range config.sysctl {
 		s := strings.SplitN(sysctl, "=", 2)
@@ -124,6 +127,10 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 		g.AddTmpfsMount(spliti[0], options)
 	}
 
+	for name, val := range config.env {
+		g.AddProcessEnv(name, val)
+	}
+
 	configSpec := g.Spec()
 
 	if config.seccompProfilePath != "" && config.seccompProfilePath != "unconfined" {
@@ -138,8 +145,6 @@ func createConfigToOCISpec(config *createConfig) (*spec.Spec, error) {
 		configSpec.Linux.Seccomp = &seccompConfig
 	}
 
-	configSpec.Process.Env = config.env
-
 	// BIND MOUNTS
 	configSpec.Mounts = append(configSpec.Mounts, config.GetVolumeMounts()...)
 

test/helpers.bash

@@ -14,7 +14,7 @@ elif [[ ! -z "$TRAVIS" ]]; then
 elif [[ ! -z "$PAPR" ]]; then
     CRIO_ROOT="/var/tmp/checkout"
 else
-    CRIO_ROOT=$(cd "$INTEGRATION_ROOT/../.."; pwd -P)}
+    CRIO_ROOT=$(cd "$INTEGRATION_ROOT/.."; pwd -P)
 fi
 
 KPOD_BINARY=${KPOD_BINARY:-${CRIO_ROOT}/bin/kpod}

test/kpod_run.bats

@@ -60,3 +60,34 @@ ALPINE="docker.io/library/alpine:latest"
     [ "$status" -eq 0 ]
 
 }
+
+@test "run environment test" {
+
+    ${KPOD_BINARY} ${KPOD_OPTIONS} pull ${ALPINE}
+
+    run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run -env FOO=BAR ${ALPINE} printenv FOO | tr -d '\r'"
+    echo "$output"
+    [ "$status" -eq 0 ]
+    [ $output = "BAR" ]
+
+    run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run -env PATH="/bin" ${ALPINE} printenv PATH | tr -d '\r'"
+    echo "$output"
+    [ "$status" -eq 0 ]
+    [ $output = "/bin" ]
+
+    run bash -c "export FOO=BAR; ${KPOD_BINARY} ${KPOD_OPTIONS} run -env FOO ${ALPINE} printenv FOO | tr -d '\r'"
+    echo "$output"
+    [ "$status" -eq 0 ]
+    [ "$output" = "BAR" ]
+
+    run ${KPOD_BINARY} ${KPOD_OPTIONS} run -env FOO ${ALPINE} printenv
+    echo "$output"
+    [ "$status" -ne 0 ]
+
+#    We don't currently set the hostname in containers, since we are not setting up
+#    networking.  As soon as kpod run gets network support we need to uncomment this
+#    test.
+#    run bash -c "${KPOD_BINARY} ${KPOD_OPTIONS} run ${ALPINE} sh -c printenv | grep HOSTNAME"
+#    echo "$output"
+#    [ "$status" -eq 0 ]
+}