Merge pull request #944 from hashicorp/f-artifact-location

Download artifacts to relative locations inside the task directory

api/tasks.go

@@ -99,6 +99,7 @@ type Task struct {
 type TaskArtifact struct {
 	GetterSource  string
 	GetterOptions map[string]string
+	RelativeDest  string
 }
 
 // NewTask creates and initializes a new Task.

client/driver/exec.go

@@ -113,7 +113,7 @@ func (d *ExecDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle,
 	}, executorCtx)
 	if err != nil {
 		pluginClient.Kill()
-		return nil, fmt.Errorf("error starting process via the plugin: %v", err)
+		return nil, err
 	}
 	d.logger.Printf("[DEBUG] driver.exec: started process via plugin with pid: %v", ps.Pid)
 

client/driver/executor/executor.go

@@ -7,6 +7,7 @@ import (
 	"net"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"sync"
@@ -159,20 +160,36 @@ func (e *UniversalExecutor) LaunchCmd(command *ExecCommand, ctx *ExecutorContext
 	e.cmd.Stdout = e.lro
 	e.cmd.Stderr = e.lre
 
-	// setting the env, path and args for the command
 	e.ctx.TaskEnv.Build()
-	e.cmd.Env = ctx.TaskEnv.EnvList()
-	e.cmd.Path = ctx.TaskEnv.ReplaceEnv(command.Cmd)
-	e.cmd.Args = append([]string{e.cmd.Path}, ctx.TaskEnv.ParseAndReplace(command.Args)...)
 
-	// Ensure that the binary being started is executable.
+	// Look up the binary path and make it executable
-	if err := e.makeExecutable(e.cmd.Path); err != nil {
+	absPath, err := e.lookupBin(ctx.TaskEnv.ReplaceEnv(command.Cmd))
+	if err != nil {
 		return nil, err
 	}
 
-	// starting the process
+	if err := e.makeExecutable(absPath); err != nil {
+		return nil, err
+	}
+
+	// Determine the path to run as it may have to be relative to the chroot.
+	path := absPath
+	if e.command.FSIsolation {
+		rel, err := filepath.Rel(e.taskDir, absPath)
+		if err != nil {
+			return nil, err
+		}
+		path = rel
+	}
+
+	// Set the commands arguments
+	e.cmd.Path = path
+	e.cmd.Args = append([]string{path}, ctx.TaskEnv.ParseAndReplace(command.Args)...)
+	e.cmd.Env = ctx.TaskEnv.EnvList()
+
+	// Start the process
 	if err := e.cmd.Start(); err != nil {
-		return nil, fmt.Errorf("error starting command: %v", err)
+		return nil, err
 	}
 	go e.wait()
 	ic := &cstructs.IsolationConfig{Cgroup: e.groups}
@@ -328,8 +345,36 @@ func (e *UniversalExecutor) configureTaskDir() error {
 	return nil
 }
 
-// makeExecutablePosix makes the given file executable for root,group,others.
+// lookupBin looks for path to the binary to run by looking for the binary in
-func (e *UniversalExecutor) makeExecutablePosix(binPath string) error {
+// the following locations, in-order: task/local/, task/, based on host $PATH.
+// The return path is absolute.
+func (e *UniversalExecutor) lookupBin(bin string) (string, error) {
+	// Check in the local directory
+	local := filepath.Join(e.taskDir, allocdir.TaskLocal, bin)
+	if _, err := os.Stat(local); err == nil {
+		return local, nil
+	}
+
+	// Check at the root of the task's directory
+	root := filepath.Join(e.taskDir, bin)
+	if _, err := os.Stat(root); err == nil {
+		return root, nil
+	}
+
+	// Check the $PATH
+	if host, err := exec.LookPath(bin); err == nil {
+		return host, nil
+	}
+
+	return "", fmt.Errorf("binary %q could not be found", bin)
+}
+
+// makeExecutable makes the given file executable for root,group,others.
+func (e *UniversalExecutor) makeExecutable(binPath string) error {
+	if runtime.GOOS == "windows" {
+		return nil
+	}
+
 	fi, err := os.Stat(binPath)
 	if err != nil {
 		if os.IsNotExist(err) {

client/driver/executor/executor_basic.go

@@ -2,25 +2,7 @@
 
 package executor
 
-import (
+import cgroupConfig "github.com/opencontainers/runc/libcontainer/configs"
-	"path/filepath"
-	"runtime"
-
-	cgroupConfig "github.com/opencontainers/runc/libcontainer/configs"
-)
-
-func (e *UniversalExecutor) makeExecutable(binPath string) error {
-	if runtime.GOOS == "windows" {
-		return nil
-	}
-
-	path := binPath
-	if !filepath.IsAbs(binPath) {
-		// The path must be relative the allocations directory.
-		path = filepath.Join(e.taskDir, binPath)
-	}
-	return e.makeExecutablePosix(path)
-}
 
 func (e *UniversalExecutor) configureChroot() error {
 	return nil

client/driver/executor/executor_linux.go

@@ -36,18 +36,6 @@ var (
 	}
 )
 
-func (e *UniversalExecutor) makeExecutable(binPath string) error {
-	path := binPath
-	if e.command.FSIsolation {
-		// The path must be relative the chroot
-		path = filepath.Join(e.taskDir, binPath)
-	} else if !filepath.IsAbs(binPath) {
-		// The path must be relative the allocations directory.
-		path = filepath.Join(e.taskDir, binPath)
-	}
-	return e.makeExecutablePosix(path)
-}
-
 // configureIsolation configures chroot and creates cgroups
 func (e *UniversalExecutor) configureIsolation() error {
 	if e.command.FSIsolation {

client/driver/executor/executor_test.go

@@ -232,3 +232,38 @@ func TestExecutor_Start_Kill(t *testing.T) {
 		t.Fatalf("Command output incorrectly: want %v; got %v", expected, act)
 	}
 }
+
+func TestExecutor_MakeExecutable(t *testing.T) {
+	// Create a temp file
+	f, err := ioutil.TempFile("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+	defer os.Remove(f.Name())
+
+	// Set its permissions to be non-executable
+	f.Chmod(os.FileMode(0610))
+
+	// Make a fake exececutor
+	ctx := testExecutorContext(t)
+	defer ctx.AllocDir.Destroy()
+	executor := NewExecutor(log.New(os.Stdout, "", log.LstdFlags))
+
+	err = executor.(*UniversalExecutor).makeExecutable(f.Name())
+	if err != nil {
+		t.Fatalf("makeExecutable() failed: %v", err)
+	}
+
+	// Check the permissions
+	stat, err := f.Stat()
+	if err != nil {
+		t.Fatalf("Stat() failed: %v", err)
+	}
+
+	act := stat.Mode().Perm()
+	exp := os.FileMode(0755)
+	if act != exp {
+		t.Fatalf("expected permissions %v; got %v", err)
+	}
+}

client/driver/java.go

@@ -173,7 +173,7 @@ func (d *JavaDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle,
 	}, executorCtx)
 	if err != nil {
 		pluginClient.Kill()
-		return nil, fmt.Errorf("error starting process via the plugin: %v", err)
+		return nil, err
 	}
 	d.logger.Printf("[DEBUG] driver.java: started process with pid: %v", ps.Pid)
 

client/driver/qemu.go

@@ -198,7 +198,7 @@ func (d *QemuDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle,
 	ps, err := exec.LaunchCmd(&executor.ExecCommand{Cmd: args[0], Args: args[1:]}, executorCtx)
 	if err != nil {
 		pluginClient.Kill()
-		return nil, fmt.Errorf("error starting process via the plugin: %v", err)
+		return nil, err
 	}
 	d.logger.Printf("[INFO] Started new QemuVM: %s", vmID)
 

client/driver/raw_exec.go

@@ -103,7 +103,7 @@ func (d *RawExecDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandl
 	ps, err := exec.LaunchCmd(&executor.ExecCommand{Cmd: command, Args: driverConfig.Args}, executorCtx)
 	if err != nil {
 		pluginClient.Kill()
-		return nil, fmt.Errorf("error starting process via the plugin: %v", err)
+		return nil, err
 	}
 	d.logger.Printf("[DEBUG] driver.raw_exec: started process with pid: %v", ps.Pid)
 

client/driver/rkt.go

@@ -246,7 +246,7 @@ func (d *RktDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle, e
 	ps, err := execIntf.LaunchCmd(&executor.ExecCommand{Cmd: absPath, Args: cmdArgs}, executorCtx)
 	if err != nil {
 		pluginClient.Kill()
-		return nil, fmt.Errorf("error starting process via the plugin: %v", err)
+		return nil, err
 	}
 
 	d.logger.Printf("[DEBUG] driver.rkt: started ACI %q with: %v", img, cmdArgs)

client/getter/getter.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"log"
 	"net/url"
+	"path/filepath"
 	"sync"
 
 	gg "github.com/hashicorp/go-getter"
@@ -59,16 +60,17 @@ func getGetterUrl(artifact *structs.TaskArtifact) (string, error) {
 	return u.String(), nil
 }
 
-// GetArtifact downloads an artifact into the specified destination directory.
+// GetArtifact downloads an artifact into the specified task directory.
-func GetArtifact(artifact *structs.TaskArtifact, destDir string, logger *log.Logger) error {
+func GetArtifact(artifact *structs.TaskArtifact, taskDir string, logger *log.Logger) error {
 	url, err := getGetterUrl(artifact)
 	if err != nil {
 		return err
 	}
 
 	// Download the artifact
-	if err := getClient(url, destDir).Get(); err != nil {
+	dest := filepath.Join(taskDir, artifact.RelativeDest)
-		return err
+	if err := getClient(url, dest).Get(); err != nil {
+		return fmt.Errorf("GET error: %v", err)
 	}
 
 	return nil

client/getter/getter_test.go

@@ -21,11 +21,11 @@ func TestGetArtifact_FileAndChecksum(t *testing.T) {
 	defer ts.Close()
 
 	// Create a temp directory to download into
-	destDir, err := ioutil.TempDir("", "nomad-test")
+	taskDir, err := ioutil.TempDir("", "nomad-test")
 	if err != nil {
 		t.Fatalf("failed to make temp directory: %v", err)
 	}
-	defer os.RemoveAll(destDir)
+	defer os.RemoveAll(taskDir)
 
 	// Create the artifact
 	file := "test.sh"
@@ -38,13 +38,48 @@ func TestGetArtifact_FileAndChecksum(t *testing.T) {
 
 	// Download the artifact
 	logger := log.New(os.Stderr, "", log.LstdFlags)
-	if err := GetArtifact(artifact, destDir, logger); err != nil {
+	if err := GetArtifact(artifact, taskDir, logger); err != nil {
 		t.Fatalf("GetArtifact failed: %v", err)
 	}
 
 	// Verify artifact exists
-	if _, err := os.Stat(filepath.Join(destDir, file)); err != nil {
+	if _, err := os.Stat(filepath.Join(taskDir, file)); err != nil {
-		t.Fatalf("source path error: %s", err)
+		t.Fatalf("file not found: %s", err)
+	}
+}
+
+func TestGetArtifact_File_RelativeDest(t *testing.T) {
+	// Create the test server hosting the file to download
+	ts := httptest.NewServer(http.FileServer(http.Dir(filepath.Dir("./test-fixtures/"))))
+	defer ts.Close()
+
+	// Create a temp directory to download into
+	taskDir, err := ioutil.TempDir("", "nomad-test")
+	if err != nil {
+		t.Fatalf("failed to make temp directory: %v", err)
+	}
+	defer os.RemoveAll(taskDir)
+
+	// Create the artifact
+	file := "test.sh"
+	relative := "foo/"
+	artifact := &structs.TaskArtifact{
+		GetterSource: fmt.Sprintf("%s/%s", ts.URL, file),
+		GetterOptions: map[string]string{
+			"checksum": "md5:bce963762aa2dbfed13caf492a45fb72",
+		},
+		RelativeDest: relative,
+	}
+
+	// Download the artifact
+	logger := log.New(os.Stderr, "", log.LstdFlags)
+	if err := GetArtifact(artifact, taskDir, logger); err != nil {
+		t.Fatalf("GetArtifact failed: %v", err)
+	}
+
+	// Verify artifact was downloaded to the correct path
+	if _, err := os.Stat(filepath.Join(taskDir, relative, file)); err != nil {
+		t.Fatalf("file not found: %s", err)
 	}
 }
 
@@ -54,11 +89,11 @@ func TestGetArtifact_InvalidChecksum(t *testing.T) {
 	defer ts.Close()
 
 	// Create a temp directory to download into
-	destDir, err := ioutil.TempDir("", "nomad-test")
+	taskDir, err := ioutil.TempDir("", "nomad-test")
 	if err != nil {
 		t.Fatalf("failed to make temp directory: %v", err)
 	}
-	defer os.RemoveAll(destDir)
+	defer os.RemoveAll(taskDir)
 
 	// Create the artifact with an incorrect checksum
 	file := "test.sh"
@@ -71,7 +106,7 @@ func TestGetArtifact_InvalidChecksum(t *testing.T) {
 
 	// Download the artifact and expect an error
 	logger := log.New(os.Stderr, "", log.LstdFlags)
-	if err := GetArtifact(artifact, destDir, logger); err == nil {
+	if err := GetArtifact(artifact, taskDir, logger); err == nil {
 		t.Fatalf("GetArtifact should have failed")
 	}
 }
@@ -116,17 +151,17 @@ func TestGetArtifact_Archive(t *testing.T) {
 
 	// Create a temp directory to download into and create some of the same
 	// files that exist in the artifact to ensure they are overriden
-	destDir, err := ioutil.TempDir("", "nomad-test")
+	taskDir, err := ioutil.TempDir("", "nomad-test")
 	if err != nil {
 		t.Fatalf("failed to make temp directory: %v", err)
 	}
-	defer os.RemoveAll(destDir)
+	defer os.RemoveAll(taskDir)
 
 	create := map[string]string{
 		"exist/my.config": "to be replaced",
 		"untouched":       "existing top-level",
 	}
-	createContents(destDir, create, t)
+	createContents(taskDir, create, t)
 
 	file := "archive.tar.gz"
 	artifact := &structs.TaskArtifact{
@@ -137,7 +172,7 @@ func TestGetArtifact_Archive(t *testing.T) {
 	}
 
 	logger := log.New(os.Stderr, "", log.LstdFlags)
-	if err := GetArtifact(artifact, destDir, logger); err != nil {
+	if err := GetArtifact(artifact, taskDir, logger); err != nil {
 		t.Fatalf("GetArtifact failed: %v", err)
 	}
 
@@ -148,5 +183,5 @@ func TestGetArtifact_Archive(t *testing.T) {
 		"new/my.config":   "hello world\n",
 		"test.sh":         "sleep 1\n",
 	}
-	checkContents(destDir, expected, t)
+	checkContents(taskDir, expected, t)
 }

client/task_runner.go

@@ -240,7 +240,18 @@ func (r *TaskRunner) run() {
 				return
 			}
 
-			for _, artifact := range r.task.Artifacts {
+			for i, artifact := range r.task.Artifacts {
+				// Verify the artifact doesn't escape the task directory.
+				if err := artifact.Validate(); err != nil {
+					// If this error occurs there is potentially a server bug or
+					// mallicious, server spoofing.
+					r.setState(structs.TaskStateDead,
+						structs.NewTaskEvent(structs.TaskArtifactDownloadFailed).SetDownloadError(err))
+					r.logger.Printf("[ERR] client: allocation %q, task %v, artifact %v (%v) fails validation",
+						r.alloc.ID, r.task.Name, artifact, i)
+					return
+				}
+
 				if err := getter.GetArtifact(artifact, taskDir, r.logger); err != nil {
 					r.setState(structs.TaskStateDead,
 						structs.NewTaskEvent(structs.TaskArtifactDownloadFailed).SetDownloadError(err))

jobspec/parse.go

@@ -617,6 +617,7 @@ func parseArtifacts(result *[]*structs.TaskArtifact, list *ast.ObjectList) error
 		valid := []string{
 			"source",
 			"options",
+			"destination",
 		}
 		if err := checkHCLKeys(o.Val, valid); err != nil {
 			return err
@@ -629,6 +630,11 @@ func parseArtifacts(result *[]*structs.TaskArtifact, list *ast.ObjectList) error
 
 		delete(m, "options")
 
+		// Default to downloading to the local directory.
+		if _, ok := m["destination"]; !ok {
+			m["destination"] = "local/"
+		}
+
 		var ta structs.TaskArtifact
 		if err := mapstructure.WeakDecode(m, &ta); err != nil {
 			return err

jobspec/parse_test.go

@@ -131,12 +131,14 @@ func TestParse(t *testing.T) {
 								Artifacts: []*structs.TaskArtifact{
 									{
 										GetterSource: "http://foo.com/artifact",
+										RelativeDest: "local/",
 										GetterOptions: map[string]string{
 											"checksum": "md5:b8a4f3f72ecab0510a6a31e997461c5f",
 										},
 									},
 									{
 										GetterSource: "http://bar.com/artifact",
+										RelativeDest: "local/",
 										GetterOptions: map[string]string{
 											"checksum": "md5:ff1cc0d3432dad54d607c1505fb7245c",
 										},
@@ -320,6 +322,58 @@ func TestParse(t *testing.T) {
 			nil,
 			true,
 		},
+
+		{
+			"artifacts.hcl",
+			&structs.Job{
+				ID:       "binstore-storagelocker",
+				Name:     "binstore-storagelocker",
+				Type:     "service",
+				Priority: 50,
+				Region:   "global",
+
+				TaskGroups: []*structs.TaskGroup{
+					&structs.TaskGroup{
+						Name:  "binsl",
+						Count: 1,
+						Tasks: []*structs.Task{
+							&structs.Task{
+								Name:   "binstore",
+								Driver: "docker",
+								Resources: &structs.Resources{
+									CPU:      100,
+									MemoryMB: 10,
+									DiskMB:   300,
+									IOPS:     0,
+								},
+								LogConfig: &structs.LogConfig{
+									MaxFiles:      10,
+									MaxFileSizeMB: 10,
+								},
+								Artifacts: []*structs.TaskArtifact{
+									{
+										GetterSource:  "http://foo.com/bar",
+										GetterOptions: map[string]string{},
+										RelativeDest:  "",
+									},
+									{
+										GetterSource:  "http://foo.com/baz",
+										GetterOptions: map[string]string{},
+										RelativeDest:  "local/",
+									},
+									{
+										GetterSource:  "http://foo.com/bam",
+										GetterOptions: map[string]string{},
+										RelativeDest:  "var/foo",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			false,
+		},
 	}
 
 	for _, tc := range cases {

jobspec/test-fixtures/artifacts.hcl

@@ -0,0 +1,21 @@
+job "binstore-storagelocker" {
+    group "binsl" {
+        task "binstore" {
+            driver = "docker"
+
+            artifact {
+                source = "http://foo.com/bar"
+                destination = ""
+            }
+
+            artifact {
+                source = "http://foo.com/baz"
+            }
+            artifact {
+                source = "http://foo.com/bam"
+                destination = "var/foo"
+            }
+            resources {}
+        }
+    }
+}

nomad/structs/structs.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"io"
 	"net/url"
+	"path/filepath"
 	"reflect"
 	"regexp"
 	"strconv"
@@ -1913,6 +1914,10 @@ type TaskArtifact struct {
 	// GetterOptions are options to use when downloading the artifact using
 	// go-getter.
 	GetterOptions map[string]string `mapstructure:"options"`
+
+	// RelativeDest is the download destination given relative to the task's
+	// directory.
+	RelativeDest string `mapstructure:"destination"`
 }
 
 func (ta *TaskArtifact) Copy() *TaskArtifact {
@@ -1925,16 +1930,36 @@ func (ta *TaskArtifact) Copy() *TaskArtifact {
 	return nta
 }
 
+func (ta *TaskArtifact) GoString() string {
+	return fmt.Sprintf("%+v", ta)
+}
+
 func (ta *TaskArtifact) Validate() error {
 	// Verify the source
 	var mErr multierror.Error
 	if ta.GetterSource == "" {
 		mErr.Errors = append(mErr.Errors, fmt.Errorf("source must be specified"))
+	} else {
+		_, err := url.Parse(ta.GetterSource)
+		if err != nil {
+			mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid source URL %q: %v", ta.GetterSource, err))
+		}
 	}
 
-	_, err := url.Parse(ta.GetterSource)
+	// Verify the destination doesn't escape the tasks directory
+	alloc := "/foo/bar/"
+	abs, err := filepath.Abs(filepath.Join(alloc, ta.RelativeDest))
 	if err != nil {
-		mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid source URL %q: %v", ta.GetterSource, err))
+		mErr.Errors = append(mErr.Errors, err)
+		return mErr.ErrorOrNil()
+	}
+	rel, err := filepath.Rel(alloc, abs)
+	if err != nil {
+		mErr.Errors = append(mErr.Errors, err)
+		return mErr.ErrorOrNil()
+	}
+	if strings.HasPrefix(rel, "..") {
+		mErr.Errors = append(mErr.Errors, fmt.Errorf("destination escapes task's directory"))
 	}
 
 	// Verify the checksum

nomad/structs/structs_test.go

@@ -777,6 +777,28 @@ func TestTaskArtifact_Validate_Source(t *testing.T) {
 	}
 }
 
+func TestTaskArtifact_Validate_Dest(t *testing.T) {
+	valid := &TaskArtifact{GetterSource: "google.com"}
+	if err := valid.Validate(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	valid.RelativeDest = "local/"
+	if err := valid.Validate(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	valid.RelativeDest = "local/.."
+	if err := valid.Validate(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	valid.RelativeDest = "local/../.."
+	if err := valid.Validate(); err == nil {
+		t.Fatalf("expected error: %v", err)
+	}
+}
+
 func TestTaskArtifact_Validate_Checksum(t *testing.T) {
 	cases := []struct {
 		Input *TaskArtifact

website/source/docs/jobspec/index.html.md

@@ -430,6 +430,10 @@ The `artifact` object maps supports the following keys:
 
 * `source` - The path to the artifact to download.
 
+* `destination` - An optional path to download the artifact into relative to the
+  root of the task's directory. If the `destination` key is omitted, it will
+  default to `local/`.
+
 * `options` - The `options` block allows setting parameters for `go-getter`. An
   example is given below: 
 

website/source/docs/jobspec/json.html.md

@@ -414,13 +414,16 @@ is started.
 
 The `Artifact` object maps supports the following keys:
 
-* `Source` - The path to the artifact to download.
+* `GetterSource` - The path to the artifact to download.
 
-* `Options` - The `options` block allows setting parameters for `go-getter`. An
+* `RelativeDest` - The destination to download the artifact relative the task's
+  directory.
+
+* `GetterOptions` - A `map[string]string` block of options for  `go-getter`. An
   example is given below:
 
 ```
-"Options": {
+"GetterOptions": {
     "checksum": "md5:c4aa853ad2215426eb7d70a21922e794",
 
     "aws_access_key_id": "<id>",