From 9ab73260f8e4662e7321b257c636928892f023cf Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Fri, 16 Jan 2015 12:57:08 -0800 Subject: [PATCH] Docker run -e FOO should erase FOO if FOO isn't set in client env See #10141 for more info, but the main point of this is to make sure that if you do "docker run -e FOO ..." that FOO from the current env is passed into the container. This means that if there's a value, its set. But it also means that if FOO isn't set then it should be unset in the container too - even if it has to remove it from the env. So, unset HOSTNAME docker run -e HOSTNAME busybox env should _NOT_ show HOSTNAME in the list at all Closes #10141 Signed-off-by: Doug Davis --- docs/sources/reference/commandline/cli.md | 9 ++- integration-cli/docker_cli_run_test.go | 71 +++++++++++++++++++++-- opts/opts.go | 4 ++ utils/utils.go | 29 +++++++++ 4 files changed, 106 insertions(+), 7 deletions(-) diff --git a/docs/sources/reference/commandline/cli.md b/docs/sources/reference/commandline/cli.md index e79664e6af..142ec5c61e 100644 --- a/docs/sources/reference/commandline/cli.md +++ b/docs/sources/reference/commandline/cli.md @@ -1725,9 +1725,12 @@ ports in Docker. This sets environmental variables in the container. For illustration all three flags are shown here. Where `-e`, `--env` take an environment variable and -value, or if no "=" is provided, then that variable's current value is passed -through (i.e. `$MYVAR1` from the host is set to `$MYVAR1` in the container). All -three flags, `-e`, `--env` and `--env-file` can be repeated. +value, or if no `=` is provided, then that variable's current value is passed +through (i.e. `$MYVAR1` from the host is set to `$MYVAR1` in the container). +When no `=` is provided and that variable is not defined in the client's +environment then that variable will be removed from the container's list of +environment variables. +All three flags, `-e`, `--env` and `--env-file` can be repeated. Regardless of the order of these three flags, the `--env-file` are processed first, and then `-e`, `--env` flags. This way, the `-e` or `--env` will diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 30acd9e1c4..deadb7cf32 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -795,10 +795,7 @@ func TestRunEnvironment(t *testing.T) { t.Fatal(err, out) } - actualEnv := strings.Split(out, "\n") - if actualEnv[len(actualEnv)-1] == "" { - actualEnv = actualEnv[:len(actualEnv)-1] - } + actualEnv := strings.Split(strings.TrimSpace(out), "\n") sort.Strings(actualEnv) goodEnv := []string{ @@ -826,6 +823,72 @@ func TestRunEnvironment(t *testing.T) { logDone("run - verify environment") } +func TestRunEnvironmentErase(t *testing.T) { + // Test to make sure that when we use -e on env vars that are + // not set in our local env that they're removed (if present) in + // the container + cmd := exec.Command(dockerBinary, "run", "-e", "FOO", "-e", "HOSTNAME", "busybox", "env") + cmd.Env = []string{} + out, _, err := runCommandWithOutput(cmd) + if err != nil { + t.Fatal(err, out) + } + + actualEnv := strings.Split(strings.TrimSpace(out), "\n") + sort.Strings(actualEnv) + + goodEnv := []string{ + "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + "HOME=/root", + } + sort.Strings(goodEnv) + if len(goodEnv) != len(actualEnv) { + t.Fatalf("Wrong environment: should be %d variables, not: %q\n", len(goodEnv), strings.Join(actualEnv, ", ")) + } + for i := range goodEnv { + if actualEnv[i] != goodEnv[i] { + t.Fatalf("Wrong environment variable: should be %s, not %s", goodEnv[i], actualEnv[i]) + } + } + + deleteAllContainers() + + logDone("run - verify environment erase") +} + +func TestRunEnvironmentOverride(t *testing.T) { + // Test to make sure that when we use -e on env vars that are + // already in the env that we're overriding them + cmd := exec.Command(dockerBinary, "run", "-e", "HOSTNAME", "-e", "HOME=/root2", "busybox", "env") + cmd.Env = []string{"HOSTNAME=bar"} + out, _, err := runCommandWithOutput(cmd) + if err != nil { + t.Fatal(err, out) + } + + actualEnv := strings.Split(strings.TrimSpace(out), "\n") + sort.Strings(actualEnv) + + goodEnv := []string{ + "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin", + "HOME=/root2", + "HOSTNAME=bar", + } + sort.Strings(goodEnv) + if len(goodEnv) != len(actualEnv) { + t.Fatalf("Wrong environment: should be %d variables, not: %q\n", len(goodEnv), strings.Join(actualEnv, ", ")) + } + for i := range goodEnv { + if actualEnv[i] != goodEnv[i] { + t.Fatalf("Wrong environment variable: should be %s, not %s", goodEnv[i], actualEnv[i]) + } + } + + deleteAllContainers() + + logDone("run - verify environment override") +} + func TestRunContainerNetwork(t *testing.T) { cmd := exec.Command(dockerBinary, "run", "busybox", "ping", "-c", "1", "127.0.0.1") if _, err := runCommand(cmd); err != nil { diff --git a/opts/opts.go b/opts/opts.go index 3d8c23ff77..7f40193412 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/api" flag "github.com/docker/docker/pkg/mflag" "github.com/docker/docker/pkg/parsers" + "github.com/docker/docker/utils" ) var ( @@ -168,6 +169,9 @@ func ValidateEnv(val string) (string, error) { if len(arr) > 1 { return val, nil } + if !utils.DoesEnvExist(val) { + return val, nil + } return fmt.Sprintf("%s=%s", val, os.Getenv(val)), nil } diff --git a/utils/utils.go b/utils/utils.go index ccb3ec00cb..ae30d6865f 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -401,7 +401,17 @@ func ReplaceOrAppendEnvValues(defaults, overrides []string) []string { parts := strings.SplitN(e, "=", 2) cache[parts[0]] = i } + for _, value := range overrides { + // Values w/o = means they want this env to be removed/unset. + if !strings.Contains(value, "=") { + if i, exists := cache[value]; exists { + defaults[i] = "" // Used to indicate it should be removed + } + continue + } + + // Just do a normal set/update parts := strings.SplitN(value, "=", 2) if i, exists := cache[parts[0]]; exists { defaults[i] = value @@ -409,9 +419,28 @@ func ReplaceOrAppendEnvValues(defaults, overrides []string) []string { defaults = append(defaults, value) } } + + // Now remove all entries that we want to "unset" + for i := 0; i < len(defaults); i++ { + if defaults[i] == "" { + defaults = append(defaults[:i], defaults[i+1:]...) + i-- + } + } + return defaults } +func DoesEnvExist(name string) bool { + for _, entry := range os.Environ() { + parts := strings.SplitN(entry, "=", 2) + if parts[0] == name { + return true + } + } + return false +} + // ReadSymlinkedDirectory returns the target directory of a symlink. // The target of the symbolic link may not be a file. func ReadSymlinkedDirectory(path string) (string, error) {