From ad8e0e5e49a96146d52cfa41945afbe973ba30af Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Wed, 19 Feb 2020 15:55:14 +0100 Subject: consolidate env handling into pkg/env Env-variable related code is scattered across several packages making it hard to maintain and extend. Consolidate the code into a new pkg/env package. Signed-off-by: Valentin Rothberg Signed-off-by: Matthew Heon --- cmd/podman/shared/create.go | 71 ++++++++++++---------- cmd/podman/shared/parse/parse.go | 19 +----- pkg/adapter/containers.go | 17 +++++- pkg/adapter/containers_remote.go | 15 ++--- pkg/adapter/pods.go | 12 +++- pkg/env/env.go | 126 +++++++++++++++++++++++++++++++++++++++ pkg/inspect/inspect.go | 2 +- pkg/spec/spec.go | 5 +- 8 files changed, 201 insertions(+), 66 deletions(-) create mode 100644 pkg/env/env.go 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) } -- cgit v1.2.3-54-g00ecf