Merge pull request #5511 from hashicorp/b-executor-path-executable

This is a follow up work to #4813 to fix #4809 and fix a regression introduced in 0.9 in marking files in libcontainer executable.

#4809 bug is that `lookupBin` uses `exec.LookPath` when not inspecting task dir files.  `exec.LookPath` only returns a file if it's already marked as an executable path in https://github.com/golang/go/blob/go1.12.1/src/os/exec/lp_unix.go#L24-L27 .  This affects raw exec as if passed an absolute path to file, `lookupBin` returns an error if file isn't already an executable.  This explains why the error manifests when an absolute interpolated path is used (e.g. `${NOMAD_TASK_DIR}/hellov1`) but not when using a task rel dir (e.g. `local/hellov1`) in the above examples used in ticket.

PR #4813 remedied this problem for raw exec but inadvertably broke libcontainer executor, as it made `lookupBin` returns the paths to host files rather than ones found inside chroot.

This PR reorders the evaluation, so we go back to 0.8 behavior of looking up task directories first, but then check for host paths before using `exec.LookPath`.

This PR is broken into three commits to illustrate evolution and confirming hypothesis:
1. 9adab75 : Adding a test illustrating how libcontainer executor fails at marking processes as executable in https://travis-ci.org/hashicorp/nomad/jobs/514942694 - note that the test doesn't depend on artifacts or interpolated paths
2. d441cdd: reverting PR #4809 and showing the test fail now with raw_exec case (as expected) in https://travis-ci.org/hashicorp/nomad/jobs/514944065
2. 244544b: in where we add the check in appropriate place next to `exec.LookPath(...)` for absolute paths and have a green job in https://travis-ci.org/hashicorp/nomad/jobs/514945024

## Future work

Inspecting `lookupBin` in 0.8 and 0.9 case, we have a bug in using `exec.LookPath` for the libcontainer executor case.  We should be looking up paths based on the container chroot and container PATH rather than the host's.  However, this is not a 0.9.0 regression and was present in 0.8; so punting to fix it post 0.9.

drivers/shared/executor/executor.go

@@ -549,12 +549,6 @@ func (e *UniversalExecutor) handleStats(ch chan *cstructs.TaskResourceUsage, ctx
 // the following locations, in-order: task/local/, task/, based on host $PATH.
 // The return path is absolute.
 func lookupBin(taskDir string, bin string) (string, error) {
-	// Check the binary path first
-	// This handles the case where the job spec sends a fully interpolated path to the binary
-	if _, err := os.Stat(bin); err == nil {
-		return bin, nil
-	}
-
 	// Check in the local directory
 	local := filepath.Join(taskDir, allocdir.TaskLocal, bin)
 	if _, err := os.Stat(local); err == nil {
@@ -567,6 +561,14 @@ func lookupBin(taskDir string, bin string) (string, error) {
 		return root, nil
 	}
 
+	// when checking host paths, check with Stat first if path is absolute
+	// as exec.LookPath only considers files already marked as executable
+	// and only consider this for absolute paths to avoid depending on
+	// current directory of nomad which may cause unexpected behavior
+	if _, err := os.Stat(bin); err == nil && filepath.IsAbs(bin) {
+		return bin, nil
+	}
+
 	// Check the $PATH
 	if host, err := exec.LookPath(bin); err == nil {
 		return host, nil

drivers/shared/executor/executor_test.go

@@ -544,3 +544,62 @@ func TestExecutor_Start_Kill_Immediately_WithGrace(pt *testing.T) {
 		})
 	}
 }
+
+// TestExecutor_Start_NonExecutableBinaries asserts that executor marks binary as executable
+// before starting
+func TestExecutor_Start_NonExecutableBinaries(pt *testing.T) {
+	pt.Parallel()
+
+	for name, factory := range executorFactories {
+		pt.Run(name, func(t *testing.T) {
+			require := require.New(t)
+
+			tmpDir, err := ioutil.TempDir("", "nomad-executor-tests")
+			require.NoError(err)
+			defer os.RemoveAll(tmpDir)
+
+			nonExecutablePath := filepath.Join(tmpDir, "nonexecutablefile")
+			ioutil.WriteFile(nonExecutablePath,
+				[]byte("#!/bin/sh\necho hello world"),
+				0600)
+
+			testExecCmd := testExecutorCommand(t)
+			execCmd, allocDir := testExecCmd.command, testExecCmd.allocDir
+			execCmd.Cmd = nonExecutablePath
+			factory.configureExecCmd(t, execCmd)
+
+			executor := factory.new(testlog.HCLogger(t))
+			defer executor.Shutdown("", 0)
+
+			// need to configure path in chroot with that file if using isolation executor
+			if _, ok := executor.(*UniversalExecutor); !ok {
+				taskName := filepath.Base(testExecCmd.command.TaskDir)
+				err := allocDir.NewTaskDir(taskName).Build(true, map[string]string{
+					tmpDir: tmpDir,
+				})
+				require.NoError(err)
+			}
+
+			defer allocDir.Destroy()
+			ps, err := executor.Launch(execCmd)
+			require.NoError(err)
+			require.NotZero(ps.Pid)
+
+			ps, err = executor.Wait(context.Background())
+			require.NoError(err)
+			require.NoError(executor.Shutdown("SIGINT", 100*time.Millisecond))
+
+			expected := "hello world"
+			tu.WaitForResult(func() (bool, error) {
+				act := strings.TrimSpace(string(testExecCmd.stdout.String()))
+				if expected != act {
+					return false, fmt.Errorf("expected: '%s' actual: '%s'", expected, act)
+				}
+				return true, nil
+			}, func(err error) {
+				require.NoError(err)
+			})
+		})
+	}
+
+}