fix: setting mountpoint when both envParams.mountPoint and chroot are unspecified (#1698)

Author: adityathebe

shell/shell.go

@@ -5,6 +5,7 @@ import (
 	gocontext "context"
 	"fmt"
 	"io"
+	"maps"
 	"os"
 	osExec "os/exec"
 	"path"
@@ -18,12 +19,13 @@ import (
 	"github.com/flanksource/commons/logger"
 	"github.com/flanksource/commons/properties"
 	"github.com/flanksource/commons/utils"
-	"github.com/flanksource/duty/connection"
-	"github.com/flanksource/duty/context"
-	"github.com/flanksource/duty/types"
 	"github.com/google/uuid"
 	"github.com/samber/lo"
 	"github.com/samber/oops"
+
+	"github.com/flanksource/duty/connection"
+	"github.com/flanksource/duty/context"
+	"github.com/flanksource/duty/types"
 )
 
 // List of env var keys that we pass on to the exec command
@@ -139,49 +141,19 @@ func Run(ctx context.Context, exec Exec) (*ExecDetails, error) {
 
 func RunCmd(ctx context.Context, exec Exec, cmd *osExec.Cmd) (*ExecDetails, error) {
 	ctx.Logger.V(3).Infof("running: %s %s", cmd.Path, lo.Map(cmd.Args, func(arg string, _ int) string { return strings.TrimSpace(arg) }))
-	envParams, err := prepareEnvironment(ctx, exec)
+	cmdCtx, err := prepareEnvironment(ctx, exec)
 	if err != nil {
 		return nil, ctx.Oops().Wrap(err)
 	}
 
-	if exec.Chroot == "" {
-		cwd, err := os.Getwd()
-		if err != nil {
-			return nil, ctx.Oops().Wrap(err)
-		}
-		cmdDir := envParams.mountPoint
-		if cmdDir == "" {
-			cmdDir = path.Join(properties.String(cwd, "shell.tmp.dir"), "shell-tmp", uuid.New().String())
-			if err := os.MkdirAll(cmdDir, 0700); err != nil {
-				return nil, ctx.Oops().Wrap(err)
-			}
-		}
-		cmd.Dir = cmdDir
+	if mountPoint, err := getCmdDir(ctx, exec.Chroot, cmdCtx.mountPoint); err != nil {
+		return nil, ctx.Oops().Wrap(err)
 	} else {
-		if stat, err := os.Stat(exec.Chroot); err != nil {
-			return nil, ctx.Oops().Wrap(err)
-		} else if stat.IsDir() {
-			envParams.mountPoint = stat.Name()
-			return nil, fmt.Errorf("%s is not a directory", exec.Chroot)
-		} else {
-			envParams.mountPoint = filepath.Dir(stat.Name())
-		}
-		cmd.Dir = exec.Chroot
-	}
-
-	// Set to a non-nil empty slice to prevent access to current environment variables
-	cmd.Env = []string{}
-
-	for _, e := range os.Environ() {
-		key, _, ok := strings.Cut(e, "=")
-		if _, exists := allowedEnvVars[key]; exists && ok {
-			cmd.Env = append(cmd.Env, e)
-		}
+		cmdCtx.mountPoint = mountPoint
+		cmd.Dir = mountPoint
 	}
 
-	if len(envParams.envs) != 0 {
-		cmd.Env = append(cmd.Env, envParams.envs...)
-	}
+	cmd.Env = getEnvVar(cmdCtx.envs)
 
 	if setupResult, err := connection.SetupConnection(ctx, exec.Connections, cmd); err != nil {
 		return nil, ctx.Oops().Wrap(err)
@@ -197,10 +169,10 @@ func RunCmd(ctx context.Context, exec Exec, cmd *osExec.Cmd) (*ExecDetails, erro
 		}()
 	}
 
-	envParams.artifacts = exec.Artifacts
-	envParams.cmd = cmd
+	cmdCtx.artifacts = exec.Artifacts
+	cmdCtx.cmd = cmd
 
-	return runCmd(ctx, envParams)
+	return runCmd(ctx, cmdCtx)
 }
 
 type commandContext struct {
@@ -334,11 +306,55 @@ func prepareEnvironment(ctx context.Context, exec Exec) (*commandContext, error)
 		if extra, err := client.Clone(ctx, result.mountPoint); err != nil {
 			return nil, err
 		} else {
-			for k, v := range extra {
-				result.extra[k] = v
-			}
+			maps.Copy(result.extra, extra)
 		}
 	}
 
 	return &result, nil
 }
+
+func getEnvVar(userSuppliedEnvs []string) []string {
+	// Set to a non-nil empty slice to prevent access to current environment variables
+	env := []string{}
+
+	for _, e := range os.Environ() {
+		key, _, ok := strings.Cut(e, "=")
+		if _, exists := allowedEnvVars[key]; exists && ok {
+			env = append(env, e)
+		}
+	}
+
+	if len(userSuppliedEnvs) != 0 {
+		env = append(env, userSuppliedEnvs...)
+	}
+
+	return env
+}
+
+func getCmdDir(ctx context.Context, chroot, mountPoint string) (string, error) {
+	if chroot != "" {
+		if stat, err := os.Stat(chroot); err != nil {
+			return "", ctx.Oops().Wrap(err)
+		} else if !stat.IsDir() {
+			return "", fmt.Errorf("%s is not a directory", chroot)
+		} else {
+			return chroot, nil
+		}
+	}
+
+	cwd, err := os.Getwd()
+	if err != nil {
+		return "", ctx.Oops().Wrap(err)
+	}
+
+	if mountPoint != "" {
+		return mountPoint, nil
+	}
+
+	cmdDir := path.Join(properties.String(cwd, "shell.tmp.dir"), "shell-tmp", uuid.New().String())
+	if err := os.MkdirAll(cmdDir, 0700); err != nil {
+		return "", ctx.Oops().Wrap(err)
+	}
+
+	return cmdDir, nil
+}