ContainerSpec environment takes precedence over rootfs environment

Introduce process.Env environment type.

[#86540096]

Signed-off-by: Chris Brown <[email protected]>

Author: glyn

integration/lifecycle/lifecycle_test.go

@@ -133,6 +133,46 @@ var _ = Describe("Creating a container", func() {
 				Ω(stdout).Should(gbytes.Say("foo"))
 			})
 		})
+
+		FContext("when the docker image specifies $HOME and $PATH", func() {
+			BeforeEach(func() {
+				// dockerfile contains `ENV /usr/local/bin:/usr/bin:/bin:/from-ENV`,
+				// see diego-dockerfiles/with-volume
+				rootfs = "docker:///cloudfoundry/with-volume"
+			})
+
+			It("$HOME is taken from the docker image", func() {
+				stdout := gbytes.NewBuffer()
+				process, err := container.Run(garden.ProcessSpec{
+					Path: "/bin/sh",
+					Args: []string{"-c", "echo $HOME"},
+				}, garden.ProcessIO{
+					Stdout: io.MultiWriter(GinkgoWriter, stdout),
+					Stderr: GinkgoWriter,
+				})
+
+				Ω(err).ShouldNot(HaveOccurred())
+
+				process.Wait()
+				Ω(stdout).Should(gbytes.Say("/home-from-ENV"))
+			})
+
+			PIt("$PATH is taken from the docker image", func() {
+				stdout := gbytes.NewBuffer()
+				process, err := container.Run(garden.ProcessSpec{
+					Path: "/bin/sh",
+					Args: []string{"-c", "echo $PATH"},
+				}, garden.ProcessIO{
+					Stdout: io.MultiWriter(GinkgoWriter, stdout),
+					Stderr: GinkgoWriter,
+				})
+
+				Ω(err).ShouldNot(HaveOccurred())
+
+				process.Wait()
+				Ω(stdout).Should(gbytes.Say("/usr/local/bin:/usr/bin:/bin:/from-ENV"))
+			})
+		})
 	})
 
 	Context("and running a process", func() {

old/linux_backend/container_pool/container_pool.go

@@ -30,6 +30,7 @@ import (
 	"github.com/cloudfoundry-incubator/garden-linux/old/linux_backend/uid_pool"
 	"github.com/cloudfoundry-incubator/garden-linux/old/logging"
 	"github.com/cloudfoundry-incubator/garden-linux/old/sysconfig"
+	"github.com/cloudfoundry-incubator/garden-linux/process"
 )
 
 var ErrUnknownRootFSProvider = errors.New("unknown rootfs provider")
@@ -256,6 +257,16 @@ func (p *LinuxContainerPool) Create(spec garden.ContainerSpec) (c linux_backend.
 
 	pLog.Info("created")
 
+	ctrEnv, err := process.NewEnv(rootFSEnvVars)
+	if err != nil {
+		return nil, err
+	}
+
+	specEnv, err := process.NewEnv(spec.Env)
+	if err != nil {
+		return nil, err
+	}
+
 	return linux_backend.NewLinuxContainer(
 		pLog,
 		id,
@@ -270,7 +281,7 @@ func (p *LinuxContainerPool) Create(spec garden.ContainerSpec) (c linux_backend.
 		p.quotaManager,
 		bandwidth_manager.New(containerPath, id, p.runner),
 		process_tracker.New(containerPath, p.runner),
-		mergeEnv(spec.Env, rootFSEnvVars),
+		ctrEnv.Merge(specEnv),
 		p.filterProvider.ProvideFilter(id),
 	), nil
 }
@@ -333,6 +344,11 @@ func (p *LinuxContainerPool) Restore(snapshot io.Reader) (linux_backend.Containe
 
 	containerLogger := p.logger.Session(id)
 
+	containerEnv, err := process.NewEnv(containerSnapshot.EnvVars)
+	if err != nil {
+		return nil, err
+	}
+
 	container := linux_backend.NewLinuxContainer(
 		containerLogger,
 		id,
@@ -352,7 +368,7 @@ func (p *LinuxContainerPool) Restore(snapshot io.Reader) (linux_backend.Containe
 		p.quotaManager,
 		bandwidthManager,
 		process_tracker.New(containerPath, p.runner),
-		containerSnapshot.EnvVars,
+		containerEnv,
 		p.filterProvider.ProvideFilter(id),
 	)
 
@@ -635,13 +651,6 @@ func getHandle(handle, id string) string {
 	return id
 }
 
-func mergeEnv(env1, env2 []string) []string {
-	for _, entry := range env2 {
-		env1 = append(env1, entry)
-	}
-	return env1
-}
-
 func cleanup(err *error, undo func()) {
 	if *err != nil {
 		undo()

old/linux_backend/container_pool/container_pool_test.go

@@ -496,11 +496,10 @@ var _ = Describe("Container pool", func() {
 				})
 
 				Ω(err).ShouldNot(HaveOccurred())
-				Ω(container.(*linux_backend.LinuxContainer).CurrentEnvVars()).Should(Equal([]string{
+				Ω(container.(*linux_backend.LinuxContainer).CurrentEnvVars()).Should(ConsistOf([]string{
+					"var3=rootfs-value-3",
 					"var1=spec-value1",
 					"var2=spec-value2",
-					"var2=rootfs-value-2",
-					"var3=rootfs-value-3",
 				}))
 			})
 

old/linux_backend/linux_container.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cloudfoundry-incubator/garden-linux/old/linux_backend/process_tracker"
 	"github.com/cloudfoundry-incubator/garden-linux/old/linux_backend/quota_manager"
 	"github.com/cloudfoundry-incubator/garden-linux/old/logging"
+	"github.com/cloudfoundry-incubator/garden-linux/process"
 	"github.com/cloudfoundry/gunk/command_runner"
 	"github.com/pivotal-golang/lager"
 )
@@ -88,7 +89,8 @@ type LinuxContainer struct {
 
 	mtu uint32
 
-	envvars       []string
+	env process.Env
+
 	processIDPool *ProcessIDPool
 }
 
@@ -151,7 +153,7 @@ func NewLinuxContainer(
 	quotaManager quota_manager.QuotaManager,
 	bandwidthManager bandwidth_manager.BandwidthManager,
 	processTracker process_tracker.ProcessTracker,
-	envvars []string,
+	env process.Env,
 	filter network.Filter,
 ) *LinuxContainer {
 	return &LinuxContainer{
@@ -182,7 +184,7 @@ func NewLinuxContainer(
 
 		filter: filter,
 
-		envvars:       envvars,
+		env:           env,
 		processIDPool: &ProcessIDPool{},
 	}
 }
@@ -288,7 +290,7 @@ func (c *LinuxContainer) Snapshot(out io.Writer) error {
 
 		Properties: c.Properties(),
 
-		EnvVars: c.envvars,
+		EnvVars: c.env.Array(),
 	}
 
 	var err error
@@ -331,7 +333,14 @@ func (c *LinuxContainer) Restore(snapshot ContainerSnapshot) error {
 
 	c.setState(State(snapshot.State))
 
-	c.envvars = snapshot.EnvVars
+	snapshotEnv, err := process.NewEnv(snapshot.EnvVars)
+	if err != nil {
+		cLog.Error("restoring-env", err, lager.Data{
+			"env": snapshot.EnvVars,
+		})
+		return err
+	}
+	c.env = snapshotEnv
 
 	for _, ev := range snapshot.Events {
 		c.registerEvent(ev)
@@ -365,7 +374,7 @@ func (c *LinuxContainer) Restore(snapshot ContainerSnapshot) error {
 
 	net := exec.Command(path.Join(c.path, "net.sh"), "setup")
 
-	err := cRunner.Run(net)
+	err = cRunner.Run(net)
 	if err != nil {
 		cLog.Error("failed-to-reenforce-network-rules", err)
 		return err
@@ -768,11 +777,12 @@ func (c *LinuxContainer) Run(spec garden.ProcessSpec, processIO garden.ProcessIO
 
 	args := []string{"--socket", sockPath, "--user", user}
 
-	envVars := []string{}
-	envVars = append(append(envVars, c.envvars...), spec.Env...)
-	envVars = c.dedup(envVars)
+	specEnv, err := process.NewEnv(spec.Env)
+	if err != nil {
+		return nil, err
+	}
 
-	for _, envVar := range envVars {
+	for _, envVar := range c.env.Merge(specEnv).Array() {
 		args = append(args, "--env", envVar)
 	}
 
@@ -855,7 +865,7 @@ func (c *LinuxContainer) NetOut(network string, port uint32, portRange string, p
 }
 
 func (c *LinuxContainer) CurrentEnvVars() []string {
-	return c.envvars
+	return c.env.Array()
 }
 
 func (c *LinuxContainer) setState(state State) {
@@ -1089,18 +1099,3 @@ func setRLimitsEnv(cmd *exec.Cmd, rlimits garden.ResourceLimits) {
 		cmd.Env = append(cmd.Env, fmt.Sprintf("RLIMIT_STACK=%d", *rlimits.Stack))
 	}
 }
-
-func (c *LinuxContainer) dedup(envVars []string) []string {
-	seenArgs := map[string]string{}
-	result := []string{}
-	for i := len(envVars) - 1; i >= 0; i-- {
-		envVar := envVars[i]
-		keyValue := strings.SplitN(envVar, "=", 2)
-		_, containsKey := seenArgs[keyValue[0]]
-		if len(keyValue) == 2 && !containsKey {
-			result = append([]string{envVar}, result...)
-			seenArgs[keyValue[0]] = envVar
-		}
-	}
-	return (result)
-}

old/linux_backend/linux_container_test.go

@@ -28,6 +28,7 @@ import (
 	"github.com/cloudfoundry-incubator/garden-linux/old/linux_backend/process_tracker"
 	"github.com/cloudfoundry-incubator/garden-linux/old/linux_backend/process_tracker/fake_process_tracker"
 	"github.com/cloudfoundry-incubator/garden-linux/old/linux_backend/quota_manager/fake_quota_manager"
+	"github.com/cloudfoundry-incubator/garden-linux/process"
 	wfakes "github.com/cloudfoundry-incubator/garden/fakes"
 	"github.com/cloudfoundry/gunk/command_runner/fake_command_runner"
 	. "github.com/cloudfoundry/gunk/command_runner/fake_command_runner/matchers"
@@ -94,7 +95,7 @@ var _ = Describe("Linux containers", func() {
 			fakeQuotaManager,
 			fakeBandwidthManager,
 			fakeProcessTracker,
-			[]string{"env1=env1Value", "env2=env2Value"},
+			process.Env{"env1": "env1Value", "env2": "env2Value"},
 			fakeFilter,
 		)
 	})
@@ -1019,10 +1020,30 @@ var _ = Describe("Linux containers", func() {
 				containerDir + "/bin/wsh",
 				"--socket", containerDir + "/run/wshd.sock",
 				"--user", "vcap",
-				"--env", "env1=env1Value",
-				"--env", "env2=env2Value",
 				"--env", `ESCAPED=kurt "russell"`,
 				"--env", "UNESCAPED=isaac\nhayes",
+				"--env", "env1=env1Value",
+				"--env", "env2=env2Value",
+				"--pidfile", containerDir + "/processes/1.pid",
+				"/some/script",
+			}))
+		})
+
+		It("runs the script with the environment variables from the run taking precedence over the container environment variables", func() {
+			_, err := container.Run(garden.ProcessSpec{
+				Path: "/some/script",
+				Env:  []string{"env1=overridden"},
+			}, garden.ProcessIO{})
+
+			Ω(err).ShouldNot(HaveOccurred())
+
+			_, ranCmd, _, _, _ := fakeProcessTracker.RunArgsForCall(0)
+			Ω(ranCmd.Args).Should(Equal([]string{
+				containerDir + "/bin/wsh",
+				"--socket", containerDir + "/run/wshd.sock",
+				"--user", "vcap",
+				"--env", "env1=overridden",
+				"--env", "env2=env2Value",
 				"--pidfile", containerDir + "/processes/1.pid",
 				"/some/script",
 			}))

old/linux_backend/src/wsh/wshd.c

@@ -246,6 +246,29 @@ char **env__add(char **envp, const char *key, const char *value) {
   return envp;
 }
 
+int env__has(char **envp, const char* key) {
+  size_t envplen = 0;
+
+  if (envp != NULL) {
+    while(envp[envplen++] != NULL);
+
+    int i;
+    for (i = 0; i < envplen; i++) {
+      char* eq = strchr(envp[i], '=');
+        if (eq != NULL) {
+          size_t keyLen = eq - envp[i];
+          if (strlen(key) == keyLen) {
+            if (memcmp(key, envp[i], keyLen) == 0) {
+              return 1;
+            }
+          }
+        }
+    }
+  }
+
+  return 0;
+}
+
 char **child_setup_environment(struct passwd *pw, char **extra_env_vars) {
   int rv;
   char **envp = extra_env_vars;

process/environment.go

@@ -0,0 +1,68 @@
+package process
+
+import (
+	"errors"
+	"fmt"
+	"sort"
+	"strings"
+)
+
+type Env map[string]string
+
+func NewEnv(array []string) (Env, error) {
+	env := make(Env, len(array))
+
+	for _, str := range array {
+		if str == "" {
+			return nil, errors.New("malformed environment: empty string")
+		}
+
+		tokens := strings.Split(str, "=")
+
+		if len(tokens) != 2 {
+			return nil, errors.New("malformed environment: invalid format (not key=value)")
+		}
+
+		key, value := tokens[0], tokens[1]
+
+		if key == "" {
+			return nil, errors.New("malformed environment: empty key")
+		}
+
+		env[key] = value
+	}
+
+	return env, nil
+}
+
+func (env Env) Merge(other Env) Env {
+	merged := make(Env, len(env)+len(other))
+
+	for key, value := range env {
+		merged[key] = value
+	}
+
+	for key, value := range other {
+		merged[key] = value
+	}
+
+	return merged
+}
+
+func (env Env) Array() []string {
+	array := make([]string, len(env))
+
+	i := 0
+	for key, value := range env {
+		array[i] = fmt.Sprintf("%s=%s", key, value)
+		i++
+	}
+
+	sort.Strings(array)
+
+	return array
+}
+
+func (env Env) String() string {
+	return fmt.Sprintf("%#v", env)
+}