Merge pull request juju#11400 from hpidcock/fix-hook-context

juju#11400 # Description of change Correctly pull env from the remote to build the correct env for hook context. Fix executing multicommands. ## QA steps Deploy charm using a container with sh or other command to a different directory than PATH variables on the operator image. Make sure the command is in the PATH of the workload container. Then run: `juju run --unit app/0 -- command` ## Documentation changes N/A ## Bug reference https://bugs.launchpad.net/juju/+bug/1870478
orbital-transfer-survey · Apr 7, 2020 · dab8595 · dab8595
2 parents 4558bb5 + ee33eba
commit dab8595
Show file tree

Hide file tree

Showing 15 changed files with 383 additions and 79 deletions.
diff --git a/Gopkg.lock b/Gopkg.lock
diff --git a/Gopkg.toml b/Gopkg.toml
@@ -512,3 +512,7 @@
 [[constraint]]
   name = "github.com/aws/aws-sdk-go"
   version = "1.29.7"
+
+[[constraint]]
+  branch = "master"
+  name = "github.com/kballard/go-shellquote"
diff --git a/caas/kubernetes/provider/exec/exec.go b/caas/kubernetes/provider/exec/exec.go
@@ -15,6 +15,7 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/loggo"
 	"github.com/juju/utils"
+	"github.com/kballard/go-shellquote"
 	core "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -133,12 +134,18 @@ func (c client) Exec(params ExecParams, cancel <-chan struct{}) error {
 	return errors.Trace(c.exec(params, cancel))
 }
 
-func processEnv(env []string) string {
+func processEnv(env []string) (string, error) {
 	out := ""
-	for _, v := range env {
-		out += fmt.Sprintf("export %s; ", v)
+	for _, s := range env {
+		values := strings.SplitN(s, "=", 2)
+		if len(values) != 2 {
+			return "", errors.NotValidf("env %q", s)
+		}
+		key := values[0]
+		value := values[1]
+		out += fmt.Sprintf("export %s=%s; ", key, shellquote.Join(value))
 	}
-	return out
+	return out, nil
 }
 
 func (c client) exec(opts ExecParams, cancel <-chan struct{}) error {
@@ -148,12 +155,16 @@ func (c client) exec(opts ExecParams, cancel <-chan struct{}) error {
 		cmd += fmt.Sprintf("cd %s; ", opts.WorkingDir)
 	}
 	if len(opts.Env) > 0 {
-		cmd += processEnv(opts.Env)
+		env, err := processEnv(opts.Env)
+		if err != nil {
+			return errors.Trace(err)
+		}
+		cmd += env
 	}
 	cmd += fmt.Sprintf("mkdir -p /tmp; echo $$ > %s; ", pidFile)
-	cmd += fmt.Sprintf("exec %s; ", strings.Join(opts.Commands, " "))
+	cmd += fmt.Sprintf("exec sh -c %s; ", shellquote.Join(strings.Join(opts.Commands, " ")))
 	cmdArgs := []string{"sh", "-c", cmd}
-	logger.Debugf("exec on pod %q for cmd %v", opts.PodName, cmdArgs)
+	logger.Debugf("exec on pod %q for cmd %+q", opts.PodName, cmdArgs)
 	req := c.clientset.CoreV1().RESTClient().Post().
 		Resource("pods").
 		Name(opts.PodName).

diff --git a/caas/kubernetes/provider/exec/exec_test.go b/caas/kubernetes/provider/exec/exec_test.go
@@ -82,11 +82,13 @@ func (s *execSuite) TestProcessEnv(c *gc.C) {
 	ctrl := s.setupExecClient(c)
 	defer ctrl.Finish()
 
-	c.Assert(exec.ProcessEnv(
+	res, err := exec.ProcessEnv(
 		[]string{
-			"AAA=1", "BBB=1", "CCC=1", "DDD=1", "EEE=1",
+			"AAA=1", "BBB=1 2", "CCC=1\n2", "DDD=1='2'", "EEE=1;2;\"foo\"",
 		},
-	), gc.Equals, "export AAA=1; export BBB=1; export CCC=1; export DDD=1; export EEE=1; ")
+	)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(res, gc.Equals, "export AAA=1; export BBB='1 2'; export CCC='1\n2'; export DDD=1=\\'2\\'; export EEE=1\\;2\\;\\\"foo\\\"; ")
 }
 
 func (s *execSuite) TestExecParamsValidatePodContainerExistence(c *gc.C) {
@@ -405,7 +407,7 @@ func (s *execSuite) TestExecCancel(c *gc.C) {
 	callNum := 0
 
 	urls := []string{
-		"/path/namespaces/test/pods/gitlab-k8s-0/exec?command=sh&command=-c&command=mkdir+-p+%2Ftmp%3B+echo+%24%24+%3E+%2Ftmp%2Frandom.pid%3B+exec+echo+%27hello+world%27%3B+&container=gitlab-container&container=gitlab-container&stderr=true&stdin=true&stdout=true",
+		"/path/namespaces/test/pods/gitlab-k8s-0/exec?command=sh&command=-c&command=mkdir+-p+%2Ftmp%3B+echo+%24%24+%3E+%2Ftmp%2Frandom.pid%3B+exec+sh+-c+%27echo+%27%5C%27%27hello+world%27%5C%27%3B+&container=gitlab-container&container=gitlab-container&stderr=true&stdin=true&stdout=true",
 		"/path/namespaces/test/pods/gitlab-k8s-0/exec?command=sh&command=-c&command=kill+-15+%24%28cat+%2Ftmp%2Frandom.pid%29&container=gitlab-container&container=gitlab-container&stderr=true&stdout=true",
 		"/path/namespaces/test/pods/gitlab-k8s-0/exec?command=sh&command=-c&command=kill+-9+-%24%28cat+%2Ftmp%2Frandom.pid%29&container=gitlab-container&container=gitlab-container&stderr=true&stdout=true",
 		"/path/namespaces/test/pods/gitlab-k8s-0/exec?command=sh&command=-c&command=kill+-9+-%24%28cat+%2Ftmp%2Frandom.pid%29&container=gitlab-container&container=gitlab-container&stderr=true&stdout=true",

diff --git a/worker/caasoperator/action.go b/worker/caasoperator/action.go
@@ -34,7 +34,7 @@ func remoteExecute(execClient exec.Executor,
 	}
 	providerID := providerIDGetter.ProviderID()
 	unitName := providerIDGetter.Name()
-	logger.Debugf("exec on pod %q for unit %q, cmd %v", providerID, unitName, params.Commands)
+	logger.Debugf("exec on pod %q for unit %q, cmd %+q", providerID, unitName, params.Commands)
 	if providerID == "" {
 		return nil, errors.NotFoundf("pod for %q", unitName)
 	}

diff --git a/worker/meterstatus/context.go b/worker/meterstatus/context.go
@@ -35,7 +35,7 @@ func NewLimitedContext(unitName string) *limitedContext {
 }
 
 // HookVars implements runner.Context.
-func (ctx *limitedContext) HookVars(paths context.Paths, remote bool) ([]string, error) {
+func (ctx *limitedContext) HookVars(paths context.Paths, remote bool, getEnv context.GetEnvFunc) ([]string, error) {
 	vars := []string{
 		"CHARM_DIR=" + paths.GetCharmDir(), // legacy
 		"JUJU_CHARM_DIR=" + paths.GetCharmDir(),
@@ -52,7 +52,7 @@ func (ctx *limitedContext) HookVars(paths context.Paths, remote bool) ([]string,
 	for key, val := range ctx.env {
 		vars = append(vars, fmt.Sprintf("%s=%s", key, val))
 	}
-	return append(vars, context.OSDependentEnvVars(paths)...), nil
+	return append(vars, context.OSDependentEnvVars(paths, getEnv)...), nil
 }
 
 // SetEnvVars sets additional environment variables to be exported by the context.

diff --git a/worker/meterstatus/context_test.go b/worker/meterstatus/context_test.go
@@ -35,7 +35,15 @@ func (*dummyPaths) ComponentDir(name string) string { return "/dummy/" + name }
 func (s *ContextSuite) TestHookContextEnv(c *gc.C) {
 	ctx := meterstatus.NewLimitedContext("u/0")
 	paths := &dummyPaths{}
-	vars, err := ctx.HookVars(paths, false)
+	vars, err := ctx.HookVars(paths, false, func(k string) string {
+		switch k {
+		case "PATH", "Path":
+			return "pathy"
+		default:
+			c.Errorf("unexpected get env call for %q", k)
+		}
+		return ""
+	})
 	c.Assert(err, jc.ErrorIsNil)
 	varMap, err := keyvalues.Parse(vars, true)
 	c.Assert(err, jc.ErrorIsNil)
@@ -58,7 +66,15 @@ func (s *ContextSuite) TestHookContextSetEnv(c *gc.C) {
 	}
 	ctx.SetEnvVars(setVars)
 	paths := &dummyPaths{}
-	vars, err := ctx.HookVars(paths, false)
+	vars, err := ctx.HookVars(paths, false, func(k string) string {
+		switch k {
+		case "PATH", "Path":
+			return "pathy"
+		default:
+			c.Errorf("unexpected get env call for %q", k)
+		}
+		return ""
+	})
 	c.Assert(err, jc.ErrorIsNil)
 	varMap, err := keyvalues.Parse(vars, true)
 	c.Assert(err, jc.ErrorIsNil)

diff --git a/worker/metrics/collect/context.go b/worker/metrics/collect/context.go
@@ -33,7 +33,7 @@ func newHookContext(unitName string, recorder spool.MetricRecorder) *hookContext
 }
 
 // HookVars implements runner.Context.
-func (ctx *hookContext) HookVars(paths context.Paths, remote bool) ([]string, error) {
+func (ctx *hookContext) HookVars(paths context.Paths, remote bool, getEnv context.GetEnvFunc) ([]string, error) {
 	vars := []string{
 		"CHARM_DIR=" + paths.GetCharmDir(), // legacy
 		"JUJU_CHARM_DIR=" + paths.GetCharmDir(),
@@ -47,7 +47,7 @@ func (ctx *hookContext) HookVars(paths context.Paths, remote bool) ([]string, er
 			"JUJU_AGENT_CA_CERT="+path.Join(paths.GetBaseDir(), caas.CACertFile),
 		)
 	}
-	return append(vars, context.OSDependentEnvVars(paths)...), nil
+	return append(vars, context.OSDependentEnvVars(paths, getEnv)...), nil
 }
 
 // UnitName implements runner.Context.

diff --git a/worker/metrics/collect/context_test.go b/worker/metrics/collect/context_test.go
@@ -65,7 +65,15 @@ func (*dummyPaths) ComponentDir(name string) string { return "/dummy/" + name }
 func (s *ContextSuite) TestHookContextEnv(c *gc.C) {
 	ctx := collect.NewHookContext("u/0", s.recorder)
 	paths := &dummyPaths{}
-	vars, err := ctx.HookVars(paths, false)
+	vars, err := ctx.HookVars(paths, false, func(k string) string {
+		switch k {
+		case "PATH", "Path":
+			return "pathy"
+		default:
+			c.Errorf("unexpected get env call for %q", k)
+		}
+		return ""
+	})
 	c.Assert(err, jc.ErrorIsNil)
 	varMap, err := keyvalues.Parse(vars, true)
 	c.Assert(err, jc.ErrorIsNil)

diff --git a/worker/uniter/runner/context/context.go b/worker/uniter/runner/context/context.go
@@ -916,7 +916,7 @@ func (ctx *HookContext) ActionData() (*ActionData, error) {
 // such that it can know what environment it's operating in, and can call back
 // into context.
 // Implements runner.Context.
-func (ctx *HookContext) HookVars(paths Paths, remote bool) ([]string, error) {
+func (ctx *HookContext) HookVars(paths Paths, remote bool, getEnv GetEnvFunc) ([]string, error) {
 	vars := ctx.legacyProxySettings.AsEnvironmentValues()
 	// TODO(thumper): as work on proxies progress, there will be additional
 	// proxy settings to be added.
@@ -979,8 +979,7 @@ func (ctx *HookContext) HookVars(paths Paths, remote bool) ([]string, error) {
 			"JUJU_ACTION_TAG="+ctx.actionData.Tag.String(),
 		)
 	}
-
-	return append(vars, OSDependentEnvVars(paths)...), nil
+	return append(vars, OSDependentEnvVars(paths, getEnv)...), nil
 }
 
 func (ctx *HookContext) handleReboot(ctxErr error) error {

diff --git a/worker/uniter/runner/context/env.go b/worker/uniter/runner/context/env.go
@@ -4,39 +4,43 @@
 package context
 
 import (
-	"os"
 	"path/filepath"
 
 	jujuos "github.com/juju/os"
 	"github.com/juju/os/series"
 )
 
+// GetEnvFunc is passed to OSDependentEnvVars and called
+// when environment variables need to be appended or otherwise
+// based off existing variables.
+type GetEnvFunc func(key string) string
+
 // OSDependentEnvVars returns the OS-dependent environment variables that
 // should be set for a hook context.
-func OSDependentEnvVars(paths Paths) []string {
+func OSDependentEnvVars(paths Paths, getEnv GetEnvFunc) []string {
 	switch jujuos.HostOS() {
 	case jujuos.Windows:
-		return windowsEnv(paths)
+		return windowsEnv(paths, getEnv)
 	case jujuos.Ubuntu:
-		return ubuntuEnv(paths)
+		return ubuntuEnv(paths, getEnv)
 	case jujuos.CentOS:
-		return centosEnv(paths)
+		return centosEnv(paths, getEnv)
 	case jujuos.OpenSUSE:
-		return opensuseEnv(paths)
+		return opensuseEnv(paths, getEnv)
 	case jujuos.GenericLinux:
-		return genericLinuxEnv(paths)
+		return genericLinuxEnv(paths, getEnv)
 	}
 	return nil
 }
 
-func appendPath(paths Paths) []string {
+func appendPath(paths Paths, getEnv GetEnvFunc) []string {
 	return []string{
-		"PATH=" + paths.GetToolsDir() + ":" + os.Getenv("PATH"),
+		"PATH=" + paths.GetToolsDir() + ":" + getEnv("PATH"),
 	}
 }
 
-func ubuntuEnv(paths Paths) []string {
-	path := appendPath(paths)
+func ubuntuEnv(paths Paths, getEnv GetEnvFunc) []string {
+	path := appendPath(paths, getEnv)
 	env := []string{
 		"APT_LISTCHANGES_FRONTEND=none",
 		"DEBIAN_FRONTEND=noninteractive",
@@ -56,8 +60,8 @@ func ubuntuEnv(paths Paths) []string {
 	return env
 }
 
-func centosEnv(paths Paths) []string {
-	path := appendPath(paths)
+func centosEnv(paths Paths, getEnv GetEnvFunc) []string {
+	path := appendPath(paths, getEnv)
 
 	env := []string{
 		"LANG=C.UTF-8",
@@ -76,8 +80,8 @@ func centosEnv(paths Paths) []string {
 	return env
 }
 
-func opensuseEnv(paths Paths) []string {
-	path := appendPath(paths)
+func opensuseEnv(paths Paths, getEnv GetEnvFunc) []string {
+	path := appendPath(paths, getEnv)
 
 	env := []string{
 		"LANG=C.UTF-8",
@@ -96,8 +100,8 @@ func opensuseEnv(paths Paths) []string {
 	return env
 }
 
-func genericLinuxEnv(paths Paths) []string {
-	path := appendPath(paths)
+func genericLinuxEnv(paths Paths, getEnv GetEnvFunc) []string {
+	path := appendPath(paths, getEnv)
 
 	env := []string{
 		"LANG=C.UTF-8",
@@ -116,11 +120,11 @@ func genericLinuxEnv(paths Paths) []string {
 // helps hooks use normal imports instead of dot sourcing modules
 // its a convenience variable. The PATH variable delimiter is
 // a semicolon instead of a colon
-func windowsEnv(paths Paths) []string {
+func windowsEnv(paths Paths, getEnv GetEnvFunc) []string {
 	charmDir := paths.GetCharmDir()
 	charmModules := filepath.Join(charmDir, "lib", "Modules")
 	return []string{
-		"Path=" + paths.GetToolsDir() + ";" + os.Getenv("Path"),
-		"PSModulePath=" + os.Getenv("PSModulePath") + ";" + charmModules,
+		"Path=" + paths.GetToolsDir() + ";" + getEnv("Path"),
+		"PSModulePath=" + getEnv("PSModulePath") + ";" + charmModules,
 	}
 }