Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download artifacts to relative locations inside the task directory #944

Merged
merged 6 commits into from
Mar 19, 2016
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/tasks.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion client/driver/exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
65 changes: 55 additions & 10 deletions client/driver/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net"
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"sync"
Expand Down Expand Up @@ -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.
if err := e.makeExecutable(e.cmd.Path); err != nil {
// Look up the binary path and make it executable
absPath, err := e.lookupBin(ctx.TaskEnv.ReplaceEnv(command.Cmd))
if err != nil {
return nil, err
}

if err := e.makeExecutable(absPath); err != nil {
return nil, err
}

// starting the process
// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defense in depth, I would re-verify that taskDir is a parent of absPath

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing the check here is unnecessary as if the path has escaped, it will be invalid inside the chroot.

}

// 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}
Expand Down Expand Up @@ -328,8 +345,36 @@ func (e *UniversalExecutor) configureTaskDir() error {
return nil
}

// makeExecutablePosix makes the given file executable for root,group,others.
func (e *UniversalExecutor) makeExecutablePosix(binPath string) error {
// lookupBin looks for path to the binary to run by looking for the binary in
// 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) {
Expand Down
20 changes: 1 addition & 19 deletions client/driver/executor/executor_basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,7 @@

package executor

import (
"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)
}
import cgroupConfig "github.com/opencontainers/runc/libcontainer/configs"

func (e *UniversalExecutor) configureChroot() error {
return nil
Expand Down
12 changes: 0 additions & 12 deletions client/driver/executor/executor_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
35 changes: 35 additions & 0 deletions client/driver/executor/executor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
2 changes: 1 addition & 1 deletion client/driver/java.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion client/driver/qemu.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion client/driver/raw_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion client/driver/rkt.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 6 additions & 4 deletions client/getter/getter.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"log"
"net/url"
"path/filepath"
"sync"

gg "github.com/hashicorp/go-getter"
Expand Down Expand Up @@ -59,16 +60,17 @@ func getGetterUrl(artifact *structs.TaskArtifact) (string, error) {
return u.String(), nil
}

// GetArtifact downloads an artifact into the specified destination directory.
func GetArtifact(artifact *structs.TaskArtifact, destDir string, logger *log.Logger) error {
// GetArtifact downloads an artifact into the specified task directory.
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 {
return err
dest := filepath.Join(taskDir, artifact.RelativeDest)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guard this destination as well.

if err := getClient(url, dest).Get(); err != nil {
return fmt.Errorf("GET error: %v", err)
}

return nil
Expand Down
61 changes: 48 additions & 13 deletions client/getter/getter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {
t.Fatalf("source path error: %s", err)
if _, err := os.Stat(filepath.Join(taskDir, file)); err != nil {
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)
}
}

Expand All @@ -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"
Expand All @@ -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")
}
}
Expand Down Expand Up @@ -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{
Expand All @@ -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)
}

Expand All @@ -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)
}
Loading