Merge pull request #5367 from vrothberg/ENVFIX

consolidate env handling into pkg/env

8 files changed, 201 insertions, 66 deletions

diff --git a/cmd/podman/shared/create.go b/cmd/podman/shared/create.go
index 0814eeba3..0ce578bef 100644
--- a/cmd/podman/shared/create.go
+++ b/cmd/podman/shared/create.go
@@ -18,6 +18,7 @@ import (
 	"github.com/containers/libpod/libpod"
 	"github.com/containers/libpod/libpod/image"
 	ann "github.com/containers/libpod/pkg/annotations"
+	envLib "github.com/containers/libpod/pkg/env"
 	"github.com/containers/libpod/pkg/errorhandling"
 	"github.com/containers/libpod/pkg/inspect"
 	ns "github.com/containers/libpod/pkg/namespaces"
@@ -473,19 +474,49 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
 	}
 
 	// ENVIRONMENT VARIABLES
-	env := EnvVariablesFromData(data)
+	//
+	// Precedence order (higher index wins):
+	//  1) env-host, 2) image data, 3) env-file, 4) env
+	var env map[string]string
+
+	// Start with env-host
 	if c.Bool("env-host") {
-		for _, e := range os.Environ() {
-			pair := strings.SplitN(e, "=", 2)
-			if _, ok := env[pair[0]]; !ok {
-				if len(pair) > 1 {
-					env[pair[0]] = pair[1]
-				}
+		osEnv, err := envLib.ParseSlice(os.Environ())
+		if err != nil {
+			return nil, errors.Wrap(err, "error parsing host environment variables")
+		}
+		env = osEnv
+	}
+
+	// Image data overrides any previous variables
+	if data != nil {
+		configEnv, err := envLib.ParseSlice(data.Config.Env)
+		if err != nil {
+			return nil, errors.Wrap(err, "error pasing image environment variables")
+		}
+		env = envLib.Join(env, configEnv)
+	}
+
+	// env-file overrides any previous variables
+	if c.IsSet("env-file") {
+		for _, f := range c.StringSlice("env-file") {
+			fileEnv, err := envLib.ParseFile(f)
+			if err != nil {
+				return nil, err
 			}
+			// File env is overridden by env.
+			env = envLib.Join(env, fileEnv)
 		}
 	}
-	if err := parse.ReadKVStrings(env, c.StringSlice("env-file"), c.StringArray("env")); err != nil {
-		return nil, errors.Wrapf(err, "unable to process environment variables")
+
+	// env overrides any previous variables
+	cmdlineEnv := c.StringSlice("env")
+	if len(cmdlineEnv) > 0 {
+		parsedEnv, err := envLib.ParseSlice(cmdlineEnv)
+		if err != nil {
+			return nil, err
+		}
+		env = envLib.Join(env, parsedEnv)
 	}
 
 	// LABEL VARIABLES
@@ -822,28 +853,6 @@ func CreateContainerFromCreateConfig(r *libpod.Runtime, createConfig *cc.CreateC
 	return ctr, nil
 }
 
-var defaultEnvVariables = map[string]string{
-	"PATH": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
-	"TERM": "xterm",
-}
-
-// EnvVariablesFromData gets sets the default environment variables
-// for containers, and reads the variables from the image data, if present.
-func EnvVariablesFromData(data *inspect.ImageData) map[string]string {
-	env := defaultEnvVariables
-	if data != nil {
-		for _, e := range data.Config.Env {
-			split := strings.SplitN(e, "=", 2)
-			if len(split) > 1 {
-				env[split[0]] = split[1]
-			} else {
-				env[split[0]] = ""
-			}
-		}
-	}
-	return env
-}
-
 func makeHealthCheckFromCli(c *GenericCLIResults) (*manifest.Schema2HealthConfig, error) {
 	inCommand := c.String("healthcheck-command")
 	inInterval := c.String("healthcheck-interval")
diff --git a/cmd/podman/shared/parse/parse.go b/cmd/podman/shared/parse/parse.go
index 79449029d..03cda268c 100644
--- a/cmd/podman/shared/parse/parse.go
+++ b/cmd/podman/shared/parse/parse.go
@@ -90,6 +90,8 @@ func GetAllLabels(labelFile, inputLabels []string) (map[string]string, error) {
 		// all environment variables, even those sourced from files, but
 		// that would require a substantial rework.
 		if err := parseEnvFile(labels, file); err != nil {
+			// FIXME: parseEnvFile is using parseEnv, so we need to add extra
+			// logic for labels.
 			return nil, err
 		}
 	}
@@ -107,23 +109,6 @@ func GetAllLabels(labelFile, inputLabels []string) (map[string]string, error) {
 	return labels, nil
 }
 
-// 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(env map[string]string, files []string, override []string) error {
-	for _, ef := range files {
-		if err := parseEnvFile(env, ef); err != nil {
-			return err
-		}
-	}
-	for _, line := range override {
-		if err := parseEnv(env, line); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 func parseEnv(env map[string]string, line string) error {
 	data := strings.SplitN(line, "=", 2)
 
diff --git a/pkg/adapter/containers.go b/pkg/adapter/containers.go
index 08e19edb8..a5e668b04 100644
--- a/pkg/adapter/containers.go
+++ b/pkg/adapter/containers.go
@@ -19,13 +19,13 @@ import (
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/libpod/cmd/podman/cliconfig"
 	"github.com/containers/libpod/cmd/podman/shared"
-	"github.com/containers/libpod/cmd/podman/shared/parse"
 	"github.com/containers/libpod/libpod"
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/libpod/events"
 	"github.com/containers/libpod/libpod/image"
 	"github.com/containers/libpod/libpod/logs"
 	"github.com/containers/libpod/pkg/adapter/shortcuts"
+	envLib "github.com/containers/libpod/pkg/env"
 	"github.com/containers/libpod/pkg/systemd/generate"
 	"github.com/containers/storage"
 	"github.com/pkg/errors"
@@ -987,9 +987,20 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal
 
 	// Validate given environment variables
 	env := map[string]string{}
-	if err := parse.ReadKVStrings(env, cli.EnvFile, cli.Env); err != nil {
-		return ec, errors.Wrapf(err, "unable to process environment variables")
+	if len(cli.EnvFile) > 0 {
+		for _, f := range cli.EnvFile {
+			fileEnv, err := envLib.ParseFile(f)
+			if err != nil {
+				return ec, err
+			}
+			env = envLib.Join(env, fileEnv)
+		}
+	}
+	cliEnv, err := envLib.ParseSlice(cli.Env)
+	if err != nil {
+		return ec, errors.Wrap(err, "error parsing environment variables")
 	}
+	env = envLib.Join(env, cliEnv)
 
 	streams := new(libpod.AttachStreams)
 	streams.OutputStream = os.Stdout
diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go
index 60ee3cb2d..32a84b60d 100644
--- a/pkg/adapter/containers_remote.go
+++ b/pkg/adapter/containers_remote.go
@@ -15,11 +15,11 @@ import (
 
 	"github.com/containers/libpod/cmd/podman/cliconfig"
 	"github.com/containers/libpod/cmd/podman/shared"
-	"github.com/containers/libpod/cmd/podman/shared/parse"
 	iopodman "github.com/containers/libpod/cmd/podman/varlink"
 	"github.com/containers/libpod/libpod"
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/libpod/logs"
+	envLib "github.com/containers/libpod/pkg/env"
 	"github.com/containers/libpod/pkg/varlinkapi/virtwriter"
 	"github.com/cri-o/ocicni/pkg/ocicni"
 	"github.com/docker/docker/pkg/term"
@@ -1025,16 +1025,11 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal
 	)
 	// default invalid command exit code
 	// Validate given environment variables
-	env := map[string]string{}
-	if err := parse.ReadKVStrings(env, []string{}, cli.Env); err != nil {
-		return -1, errors.Wrapf(err, "Exec unable to process environment variables")
-	}
-
-	// Build env slice of key=value strings for Exec
-	envs := []string{}
-	for k, v := range env {
-		envs = append(envs, fmt.Sprintf("%s=%s", k, v))
+	cliEnv, err := envLib.ParseSlice(cli.Env)
+	if err != nil {
+		return 0, errors.Wrap(err, "error parsing environment variables")
 	}
+	envs := envLib.Slice(cliEnv)
 
 	resize := make(chan remotecommand.TerminalSize, 5)
 	haveTerminal := terminal.IsTerminal(int(os.Stdin.Fd()))
diff --git a/pkg/adapter/pods.go b/pkg/adapter/pods.go
index 0d9fa7210..dc856cc8d 100644
--- a/pkg/adapter/pods.go
+++ b/pkg/adapter/pods.go
@@ -22,6 +22,7 @@ import (
 	"github.com/containers/libpod/libpod/image"
 	"github.com/containers/libpod/pkg/adapter/shortcuts"
 	ann "github.com/containers/libpod/pkg/annotations"
+	envLib "github.com/containers/libpod/pkg/env"
 	ns "github.com/containers/libpod/pkg/namespaces"
 	createconfig "github.com/containers/libpod/pkg/spec"
 	"github.com/containers/libpod/pkg/util"
@@ -916,9 +917,6 @@ func kubeContainerToCreateConfig(ctx context.Context, containerYAML v1.Container
 	containerConfig.User = userConfig
 	containerConfig.Security = securityConfig
 
-	// Set default environment variables and incorporate data from image, if necessary
-	envs := shared.EnvVariablesFromData(imageData)
-
 	annotations := make(map[string]string)
 	if infraID != "" {
 		annotations[ann.SandboxID] = infraID
@@ -927,6 +925,14 @@ func kubeContainerToCreateConfig(ctx context.Context, containerYAML v1.Container
 	containerConfig.Annotations = annotations
 
 	// Environment Variables
+	envs := map[string]string{}
+	if imageData != nil {
+		imageEnv, err := envLib.ParseSlice(imageData.Config.Env)
+		if err != nil {
+			return nil, errors.Wrap(err, "error parsing image environment variables")
+		}
+		envs = imageEnv
+	}
 	for _, e := range containerYAML.Env {
 		envs[e.Name] = e.Value
 	}
diff --git a/pkg/env/env.go b/pkg/env/env.go
new file mode 100644
index 000000000..31ffab03c
--- /dev/null
+++ b/pkg/env/env.go
@@ -0,0 +1,126 @@
+// Package for processing environment variables.
+package env
+
+// TODO: we need to add tests for this package.
+
+import (
+	"bufio"
+	"fmt"
+	"os"
+	"strings"
+
+	"github.com/pkg/errors"
+)
+
+// DefaultEnvVariables set $PATH, $TERM and $container.
+var DefaultEnvVariables = map[string]string{
+	"PATH":      "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
+	"TERM":      "xterm",
+	"container": "podman",
+}
+
+const whiteSpaces = " \t"
+
+// ParseSlice parses the specified slice and transforms it into an environment
+// map.
+func ParseSlice(s []string) (map[string]string, error) {
+	env := make(map[string]string, len(s))
+	for _, e := range s {
+		if err := parseEnv(env, e); err != nil {
+			return nil, err
+		}
+	}
+	return env, nil
+}
+
+// Slice transforms the specified map of environment variables into a
+// slice. If a value is non-empty, the key and value are joined with '='.
+func Slice(m map[string]string) []string {
+	env := make([]string, len(m))
+	for k, v := range m {
+		var s string
+		if len(v) > 0 {
+			s = fmt.Sprintf("%s=%s", k, v)
+		} else {
+			s = k
+		}
+		env = append(env, s)
+	}
+	return env
+}
+
+// Join joins the two environment maps with override overriding base.
+func Join(base map[string]string, override map[string]string) map[string]string {
+	if len(base) == 0 {
+		return override
+	}
+	for k, v := range override {
+		base[k] = v
+	}
+	return base
+}
+
+// ParseFile parses the specified path for environment variables and returns them
+// as a map.
+func ParseFile(path string) (env map[string]string, err error) {
+	defer func() {
+		if err != nil {
+			err = errors.Wrapf(err, "error parsing env file %q", path)
+		}
+	}()
+
+	fh, err := os.Open(path)
+	if err != nil {
+		return nil, err
+	}
+	defer fh.Close()
+
+	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, "#") {
+			if err := parseEnv(env, line); err != nil {
+				return nil, err
+			}
+		}
+	}
+	return env, scanner.Err()
+}
+
+func parseEnv(env map[string]string, line string) error {
+	data := strings.SplitN(line, "=", 2)
+
+	// catch invalid variables such as "=" or "=A"
+	if data[0] == "" {
+		return errors.Errorf("invalid environment variable: %q", line)
+	}
+
+	// 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 strings.HasSuffix(name, "*") {
+			name = strings.TrimSuffix(name, "*")
+			for _, e := range os.Environ() {
+				part := strings.SplitN(e, "=", 2)
+				if len(part) < 2 {
+					continue
+				}
+				if strings.HasPrefix(part[0], name) {
+					env[part[0]] = part[1]
+				}
+			}
+		} else if val, ok := os.LookupEnv(name); ok {
+			// if only a pass-through variable is given, clean it up.
+			env[name] = val
+		}
+	}
+	return nil
+}
diff --git a/pkg/inspect/inspect.go b/pkg/inspect/inspect.go
index 569f208d9..b04ce71a5 100644
--- a/pkg/inspect/inspect.go
+++ b/pkg/inspect/inspect.go
@@ -6,7 +6,7 @@ import (
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/libpod/libpod/driver"
 	"github.com/opencontainers/go-digest"
-	"github.com/opencontainers/image-spec/specs-go/v1"
+	v1 "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 // ImageData holds the inspect information of an image
diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go
index a4ae22efd..77f8bc657 100644
--- a/pkg/spec/spec.go
+++ b/pkg/spec/spec.go
@@ -7,6 +7,7 @@ import (
 	libpodconfig "github.com/containers/libpod/libpod/config"
 	"github.com/containers/libpod/libpod/define"
 	"github.com/containers/libpod/pkg/cgroups"
+	"github.com/containers/libpod/pkg/env"
 	"github.com/containers/libpod/pkg/rootless"
 	"github.com/containers/libpod/pkg/sysinfo"
 	"github.com/docker/go-units"
@@ -150,7 +151,6 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM
 	for key, val := range config.Annotations {
 		g.AddAnnotation(key, val)
 	}
-	g.AddProcessEnv("container", "podman")
 
 	addedResources := false
 
@@ -292,6 +292,9 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM
 		}
 	}
 
+	// Make sure to always set the default variables unless overridden in the
+	// config.
+	config.Env = env.Join(env.DefaultEnvVariables, config.Env)
 	for name, val := range config.Env {
 		g.AddProcessEnv(name, val)
 	}