diff --git a/.changelog/11334.txt b/.changelog/11334.txt new file mode 100644 index 00000000000..df88d6e7eed --- /dev/null +++ b/.changelog/11334.txt @@ -0,0 +1,3 @@ +```release-note:improvement +client: Never embed client.alloc_dir in chroots to prevent infinite recursion from misconfiguration. +``` diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go index 537e79d18ae..0dc52029cad 100644 --- a/client/allocdir/alloc_dir.go +++ b/client/allocdir/alloc_dir.go @@ -84,6 +84,10 @@ type AllocDir struct { // TaskDirs is a mapping of task names to their non-shared directory. TaskDirs map[string]*TaskDir + // clientAllocDir is the client agent's root alloc directory. It must + // be excluded from chroots and is configured via client.alloc_dir. + clientAllocDir string + // built is true if Build has successfully run built bool @@ -104,36 +108,16 @@ type AllocDirFS interface { // NewAllocDir initializes the AllocDir struct with allocDir as base path for // the allocation directory. -func NewAllocDir(logger hclog.Logger, allocDir string) *AllocDir { +func NewAllocDir(logger hclog.Logger, clientAllocDir, allocID string) *AllocDir { logger = logger.Named("alloc_dir") + allocDir := filepath.Join(clientAllocDir, allocID) return &AllocDir{ - AllocDir: allocDir, - SharedDir: filepath.Join(allocDir, SharedAllocName), - TaskDirs: make(map[string]*TaskDir), - logger: logger, - } -} - -// Copy an AllocDir and all of its TaskDirs. Returns nil if AllocDir is -// nil. -func (d *AllocDir) Copy() *AllocDir { - if d == nil { - return nil - } - - d.mu.RLock() - defer d.mu.RUnlock() - - dcopy := &AllocDir{ - AllocDir: d.AllocDir, - SharedDir: d.SharedDir, - TaskDirs: make(map[string]*TaskDir, len(d.TaskDirs)), - logger: d.logger, - } - for k, v := range d.TaskDirs { - dcopy.TaskDirs[k] = v.Copy() + clientAllocDir: clientAllocDir, + AllocDir: allocDir, + SharedDir: filepath.Join(allocDir, SharedAllocName), + TaskDirs: make(map[string]*TaskDir), + logger: logger, } - return dcopy } // NewTaskDir creates a new TaskDir and adds it to the AllocDirs TaskDirs map. @@ -141,7 +125,7 @@ func (d *AllocDir) NewTaskDir(name string) *TaskDir { d.mu.Lock() defer d.mu.Unlock() - td := newTaskDir(d.logger, d.AllocDir, name) + td := newTaskDir(d.logger, d.clientAllocDir, d.AllocDir, name) d.TaskDirs[name] = td return td } diff --git a/client/allocdir/alloc_dir_test.go b/client/allocdir/alloc_dir_test.go index 8dc61b5cd62..86cd8d917c3 100644 --- a/client/allocdir/alloc_dir_test.go +++ b/client/allocdir/alloc_dir_test.go @@ -5,6 +5,7 @@ import ( "bytes" "context" "io" + "io/fs" "io/ioutil" "log" "os" @@ -53,7 +54,7 @@ func TestAllocDir_BuildAlloc(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") defer d.Destroy() d.NewTaskDir(t1.Name) d.NewTaskDir(t2.Name) @@ -103,7 +104,7 @@ func TestAllocDir_MountSharedAlloc(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") defer d.Destroy() if err := d.Build(); err != nil { t.Fatalf("Build() failed: %v", err) @@ -148,7 +149,7 @@ func TestAllocDir_Snapshot(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") defer d.Destroy() if err := d.Build(); err != nil { t.Fatalf("Build() failed: %v", err) @@ -235,13 +236,13 @@ func TestAllocDir_Move(t *testing.T) { defer os.RemoveAll(tmp2) // Create two alloc dirs - d1 := NewAllocDir(testlog.HCLogger(t), tmp1) + d1 := NewAllocDir(testlog.HCLogger(t), tmp1, "test") if err := d1.Build(); err != nil { t.Fatalf("Build() failed: %v", err) } defer d1.Destroy() - d2 := NewAllocDir(testlog.HCLogger(t), tmp2) + d2 := NewAllocDir(testlog.HCLogger(t), tmp2, "test") if err := d2.Build(); err != nil { t.Fatalf("Build() failed: %v", err) } @@ -296,7 +297,7 @@ func TestAllocDir_EscapeChecking(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") if err := d.Build(); err != nil { t.Fatalf("Build() failed: %v", err) } @@ -337,7 +338,7 @@ func TestAllocDir_ReadAt_SecretDir(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") if err := d.Build(); err != nil { t.Fatalf("Build() failed: %v", err) } @@ -419,25 +420,6 @@ func TestAllocDir_CreateDir(t *testing.T) { } } -// TestAllocDir_Copy asserts that AllocDir.Copy does a deep copy of itself and -// all TaskDirs. -func TestAllocDir_Copy(t *testing.T) { - a := NewAllocDir(testlog.HCLogger(t), "foo") - a.NewTaskDir("bar") - a.NewTaskDir("baz") - - b := a.Copy() - - // Clear the logger - require.Equal(t, a, b) - - // Make sure TaskDirs map is copied - a.NewTaskDir("new") - if b.TaskDirs["new"] != nil { - t.Errorf("TaskDirs map shared between copied") - } -} - func TestPathFuncs(t *testing.T) { dir, err := ioutil.TempDir("", "nomadtest-pathfuncs") if err != nil { @@ -502,3 +484,55 @@ func TestAllocDir_DetectContentType(t *testing.T) { require.Equal(expectedEncodings[file], res, "unexpected output for %v", file) } } + +// TestAllocDir_SkipAllocDir asserts that building a chroot which contains +// itself will *not* infinitely recurse. AllocDirs should always skip embedding +// themselves into chroots. +// +// Warning: If this test fails it may fill your disk before failing, so be +// careful and/or confident. +func TestAllocDir_SkipAllocDir(t *testing.T) { + MountCompatible(t) + + // Create root, alloc, and other dirs + rootDir := t.TempDir() + + clientAllocDir := filepath.Join(rootDir, "nomad") + require.NoError(t, os.Mkdir(clientAllocDir, fs.ModeDir|0o777)) + + otherDir := filepath.Join(rootDir, "etc") + require.NoError(t, os.Mkdir(otherDir, fs.ModeDir|0o777)) + + // chroot contains client.alloc_dir! This could cause infinite + // recursion. + chroot := map[string]string{ + rootDir: "/", + } + + allocDir := NewAllocDir(testlog.HCLogger(t), clientAllocDir, "test") + taskDir := allocDir.NewTaskDir("testtask") + + require.NoError(t, allocDir.Build()) + defer allocDir.Destroy() + + // Build chroot + err := taskDir.Build(true, chroot) + require.NoError(t, err) + + // Assert other directory *was* embedded + embeddedOtherDir := filepath.Join(clientAllocDir, "test", "testtask", "etc") + if _, err := os.Stat(embeddedOtherDir); os.IsNotExist(err) { + t.Fatalf("expected other directory to exist at: %q", embeddedOtherDir) + } + + // Assert client.alloc_dir was *not* embedded + embeddedChroot := filepath.Join(clientAllocDir, "test", "testtask", "nomad") + s, err := os.Stat(embeddedChroot) + if s != nil { + t.Logf("somehow you managed to embed the chroot without causing infinite recursion!") + t.Fatalf("expected chroot to not exist at: %q", embeddedChroot) + } + if !os.IsNotExist(err) { + t.Fatalf("expected chroot to not exist but error is: %v", err) + } +} diff --git a/client/allocdir/task_dir.go b/client/allocdir/task_dir.go index 9d99f971754..d516c313cf1 100644 --- a/client/allocdir/task_dir.go +++ b/client/allocdir/task_dir.go @@ -39,6 +39,10 @@ type TaskDir struct { // /secrets/ SecretsDir string + // skip embedding these paths in chroots. Used for avoiding embedding + // client.alloc_dir recursively. + skip map[string]struct{} + logger hclog.Logger } @@ -46,11 +50,14 @@ type TaskDir struct { // create paths on disk. // // Call AllocDir.NewTaskDir to create new TaskDirs -func newTaskDir(logger hclog.Logger, allocDir, taskName string) *TaskDir { +func newTaskDir(logger hclog.Logger, clientAllocDir, allocDir, taskName string) *TaskDir { taskDir := filepath.Join(allocDir, taskName) logger = logger.Named("task_dir").With("task_name", taskName) + // skip embedding client.alloc_dir in chroots + skip := map[string]struct{}{clientAllocDir: {}} + return &TaskDir{ AllocDir: allocDir, Dir: taskDir, @@ -59,21 +66,14 @@ func newTaskDir(logger hclog.Logger, allocDir, taskName string) *TaskDir { SharedTaskDir: filepath.Join(taskDir, SharedAllocName), LocalDir: filepath.Join(taskDir, TaskLocal), SecretsDir: filepath.Join(taskDir, TaskSecrets), + skip: skip, logger: logger, } } -// Copy a TaskDir. Panics if TaskDir is nil as TaskDirs should never be nil. -func (t *TaskDir) Copy() *TaskDir { - // No nested structures other than the logger which is safe to share, - // so just copy the struct - tcopy := *t - return &tcopy -} - // Build default directories and permissions in a task directory. chrootCreated // allows skipping chroot creation if the caller knows it has already been -// done. +// done. client.alloc_dir will be skipped. func (t *TaskDir) Build(createChroot bool, chroot map[string]string) error { if err := os.MkdirAll(t.Dir, 0777); err != nil { return err @@ -149,6 +149,11 @@ func (t *TaskDir) buildChroot(entries map[string]string) error { func (t *TaskDir) embedDirs(entries map[string]string) error { subdirs := make(map[string]string) for source, dest := range entries { + if _, ok := t.skip[source]; ok { + // source in skip list + continue + } + // Check to see if directory exists on host. s, err := os.Stat(source) if os.IsNotExist(err) { diff --git a/client/allocdir/task_dir_test.go b/client/allocdir/task_dir_test.go index b39c275f2ba..61aa3b3027c 100644 --- a/client/allocdir/task_dir_test.go +++ b/client/allocdir/task_dir_test.go @@ -17,7 +17,7 @@ func TestTaskDir_EmbedNonexistent(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") defer d.Destroy() td := d.NewTaskDir(t1.Name) if err := d.Build(); err != nil { @@ -39,7 +39,7 @@ func TestTaskDir_EmbedDirs(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") defer d.Destroy() td := d.NewTaskDir(t1.Name) if err := d.Build(); err != nil { @@ -96,7 +96,7 @@ func TestTaskDir_NonRoot_Image(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") defer d.Destroy() td := d.NewTaskDir(t1.Name) if err := d.Build(); err != nil { @@ -119,7 +119,7 @@ func TestTaskDir_NonRoot(t *testing.T) { } defer os.RemoveAll(tmp) - d := NewAllocDir(testlog.HCLogger(t), tmp) + d := NewAllocDir(testlog.HCLogger(t), tmp, "test") defer d.Destroy() td := d.NewTaskDir(t1.Name) if err := d.Build(); err != nil { diff --git a/client/allocdir/testing.go b/client/allocdir/testing.go index 6e870fb4231..6ea7d7bb990 100644 --- a/client/allocdir/testing.go +++ b/client/allocdir/testing.go @@ -10,13 +10,13 @@ import ( // TestAllocDir returns a built alloc dir in a temporary directory and cleanup // func. -func TestAllocDir(t testing.T, l hclog.Logger, prefix string) (*AllocDir, func()) { +func TestAllocDir(t testing.T, l hclog.Logger, prefix, id string) (*AllocDir, func()) { dir, err := ioutil.TempDir("", prefix) if err != nil { t.Fatalf("Couldn't create temp dir: %v", err) } - allocDir := NewAllocDir(l, dir) + allocDir := NewAllocDir(l, dir, id) cleanup := func() { if err := os.RemoveAll(dir); err != nil { diff --git a/client/allocrunner/alloc_runner.go b/client/allocrunner/alloc_runner.go index 31a6956c2f6..7dcf072a948 100644 --- a/client/allocrunner/alloc_runner.go +++ b/client/allocrunner/alloc_runner.go @@ -3,7 +3,6 @@ package allocrunner import ( "context" "fmt" - "path/filepath" "sync" "time" @@ -227,7 +226,7 @@ func NewAllocRunner(config *Config) (*allocRunner, error) { ar.allocBroadcaster = cstructs.NewAllocBroadcaster(ar.logger) // Create alloc dir - ar.allocDir = allocdir.NewAllocDir(ar.logger, filepath.Join(config.ClientConfig.AllocDir, alloc.ID)) + ar.allocDir = allocdir.NewAllocDir(ar.logger, config.ClientConfig.AllocDir, alloc.ID) ar.taskHookCoordinator = newTaskHookCoordinator(ar.logger, tg.Tasks) diff --git a/client/allocrunner/consul_grpc_sock_hook_test.go b/client/allocrunner/consul_grpc_sock_hook_test.go index f700c8ce4ec..2730ac62703 100644 --- a/client/allocrunner/consul_grpc_sock_hook_test.go +++ b/client/allocrunner/consul_grpc_sock_hook_test.go @@ -39,7 +39,7 @@ func TestConsulGRPCSocketHook_PrerunPostrun_Ok(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID) defer cleanup() // Start the unix socket proxy @@ -105,15 +105,15 @@ func TestConsulGRPCSocketHook_Prerun_Error(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap") - defer cleanup() - // A config without an Addr or GRPCAddr is invalid. consulConfig := &config.ConsulConfig{} alloc := mock.Alloc() connectAlloc := mock.ConnectAlloc() + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID) + defer cleanup() + { // An alloc without a Connect proxy sidecar should not return // an error. diff --git a/client/allocrunner/consul_http_sock_hook_test.go b/client/allocrunner/consul_http_sock_hook_test.go index 7fdfe04c985..3d5a97ec55e 100644 --- a/client/allocrunner/consul_http_sock_hook_test.go +++ b/client/allocrunner/consul_http_sock_hook_test.go @@ -28,7 +28,7 @@ func TestConsulSocketHook_PrerunPostrun_Ok(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask", alloc.ID) defer cleanupDir() // start unix socket proxy @@ -93,14 +93,14 @@ func TestConsulHTTPSocketHook_Prerun_Error(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask") - defer cleanupDir() - consulConfig := new(config.ConsulConfig) alloc := mock.Alloc() connectNativeAlloc := mock.ConnectNativeAlloc("bridge") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "ConnectNativeTask", alloc.ID) + defer cleanupDir() + { // an alloc without a connect native task should not return an error h := newConsulHTTPSocketHook(logger, alloc, allocDir, consulConfig) diff --git a/client/allocrunner/taskrunner/connect_native_hook_test.go b/client/allocrunner/taskrunner/connect_native_hook_test.go index b9a447fe3cc..a9c43d21016 100644 --- a/client/allocrunner/taskrunner/connect_native_hook_test.go +++ b/client/allocrunner/taskrunner/connect_native_hook_test.go @@ -272,11 +272,10 @@ func TestTaskRunner_ConnectNativeHook_Noop(t *testing.T) { t.Parallel() logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative") - defer cleanup() - alloc := mock.Alloc() task := alloc.Job.LookupTaskGroup(alloc.TaskGroup).Tasks[0] + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID) + defer cleanup() // run the connect native hook. use invalid consul address as it should not get hit h := newConnectNativeHook(newConnectNativeHookConfig(alloc, &config.ConsulConfig{ @@ -328,7 +327,7 @@ func TestTaskRunner_ConnectNativeHook_Ok(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID) defer cleanup() // register group services @@ -393,7 +392,7 @@ func TestTaskRunner_ConnectNativeHook_with_SI_token(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID) defer cleanup() // register group services @@ -470,7 +469,7 @@ func TestTaskRunner_ConnectNativeHook_shareTLS(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID) defer cleanup() // register group services @@ -590,7 +589,7 @@ func TestTaskRunner_ConnectNativeHook_shareTLS_override(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "ConnectNative", alloc.ID) defer cleanup() // register group services diff --git a/client/allocrunner/taskrunner/dispatch_hook_test.go b/client/allocrunner/taskrunner/dispatch_hook_test.go index 9ac683a146e..9f56fe0fd1d 100644 --- a/client/allocrunner/taskrunner/dispatch_hook_test.go +++ b/client/allocrunner/taskrunner/dispatch_hook_test.go @@ -26,12 +26,13 @@ func TestTaskRunner_DispatchHook_NoPayload(t *testing.T) { require := require.New(t) ctx := context.Background() logger := testlog.HCLogger(t) - allocDir := allocdir.NewAllocDir(logger, "nomadtest_nopayload") - defer allocDir.Destroy() // Default mock alloc/job is not a dispatch job alloc := mock.BatchAlloc() task := alloc.Job.TaskGroups[0].Tasks[0] + + allocDir := allocdir.NewAllocDir(logger, "nomadtest_nopayload", alloc.ID) + defer allocDir.Destroy() taskDir := allocDir.NewTaskDir(task.Name) require.NoError(taskDir.Build(false, nil)) @@ -61,8 +62,6 @@ func TestTaskRunner_DispatchHook_Ok(t *testing.T) { require := require.New(t) ctx := context.Background() logger := testlog.HCLogger(t) - allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatchok") - defer allocDir.Destroy() // Default mock alloc/job is not a dispatch job; update it alloc := mock.BatchAlloc() @@ -77,6 +76,9 @@ func TestTaskRunner_DispatchHook_Ok(t *testing.T) { task.DispatchPayload = &structs.DispatchPayloadConfig{ File: "out", } + + allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatchok", alloc.ID) + defer allocDir.Destroy() taskDir := allocDir.NewTaskDir(task.Name) require.NoError(taskDir.Build(false, nil)) @@ -104,8 +106,6 @@ func TestTaskRunner_DispatchHook_Error(t *testing.T) { require := require.New(t) ctx := context.Background() logger := testlog.HCLogger(t) - allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatcherr") - defer allocDir.Destroy() // Default mock alloc/job is not a dispatch job; update it alloc := mock.BatchAlloc() @@ -121,6 +121,9 @@ func TestTaskRunner_DispatchHook_Error(t *testing.T) { task.DispatchPayload = &structs.DispatchPayloadConfig{ File: "out", } + + allocDir := allocdir.NewAllocDir(logger, "nomadtest_dispatcherr", alloc.ID) + defer allocDir.Destroy() taskDir := allocDir.NewTaskDir(task.Name) require.NoError(taskDir.Build(false, nil)) diff --git a/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go b/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go index 5d473613fb3..33e0420f196 100644 --- a/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go +++ b/client/allocrunner/taskrunner/envoy_bootstrap_hook_test.go @@ -329,7 +329,7 @@ func TestEnvoyBootstrapHook_with_SI_token(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID) defer cleanup() // Register Group Services @@ -430,7 +430,7 @@ func TestTaskRunner_EnvoyBootstrapHook_sidecar_ok(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID) defer cleanup() // Register Group Services @@ -495,7 +495,7 @@ func TestTaskRunner_EnvoyBootstrapHook_gateway_ok(t *testing.T) { // Setup an Allocation alloc := mock.ConnectIngressGatewayAlloc("bridge") - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapIngressGateway") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapIngressGateway", alloc.ID) defer cleanupDir() // Get a Consul client @@ -573,11 +573,10 @@ func TestTaskRunner_EnvoyBootstrapHook_Noop(t *testing.T) { t.Parallel() logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap") - defer cleanup() - alloc := mock.Alloc() task := alloc.Job.LookupTaskGroup(alloc.TaskGroup).Tasks[0] + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID) + defer cleanup() // Run Envoy bootstrap Hook. Use invalid Consul address as it should // not get hit. @@ -646,7 +645,7 @@ func TestTaskRunner_EnvoyBootstrapHook_RecoverableError(t *testing.T) { logger := testlog.HCLogger(t) - allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap") + allocDir, cleanup := allocdir.TestAllocDir(t, logger, "EnvoyBootstrap", alloc.ID) defer cleanup() // Unlike the successful test above, do NOT register the group services @@ -724,7 +723,7 @@ func TestTaskRunner_EnvoyBootstrapHook_retryTimeout(t *testing.T) { Kind: structs.NewTaskKind(structs.ConnectProxyPrefix, "foo"), } tg.Tasks = append(tg.Tasks, sidecarTask) - allocDir, cleanupAlloc := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapRetryTimeout") + allocDir, cleanupAlloc := allocdir.TestAllocDir(t, logger, "EnvoyBootstrapRetryTimeout", alloc.ID) defer cleanupAlloc() // Get a Consul client diff --git a/client/allocrunner/taskrunner/envoy_version_hook_test.go b/client/allocrunner/taskrunner/envoy_version_hook_test.go index 26d739bd7c7..225c8d6e683 100644 --- a/client/allocrunner/taskrunner/envoy_version_hook_test.go +++ b/client/allocrunner/taskrunner/envoy_version_hook_test.go @@ -228,7 +228,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_standard(t *testing.T) { // Setup an Allocation alloc := mock.ConnectAlloc() alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask() - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID) defer cleanupDir() // Setup a mock for Consul API @@ -272,7 +272,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_custom(t *testing.T) { alloc := mock.ConnectAlloc() alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask() alloc.Job.TaskGroups[0].Tasks[0].Config["image"] = "custom-${NOMAD_envoy_version}:latest" - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID) defer cleanupDir() // Setup a mock for Consul API @@ -319,7 +319,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_skip(t *testing.T) { alloc.Job.TaskGroups[0].Tasks[0].Config = map[string]interface{}{ "command": "/sidecar", } - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID) defer cleanupDir() // Setup a mock for Consul API @@ -362,7 +362,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_fallback(t *testing.T) { // Setup an Allocation alloc := mock.ConnectAlloc() alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask() - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID) defer cleanupDir() // Setup a mock for Consul API @@ -403,7 +403,7 @@ func TestTaskRunner_EnvoyVersionHook_Prestart_error(t *testing.T) { // Setup an Allocation alloc := mock.ConnectAlloc() alloc.Job.TaskGroups[0].Tasks[0] = mock.ConnectSidecarTask() - allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook") + allocDir, cleanupDir := allocdir.TestAllocDir(t, logger, "EnvoyVersionHook", alloc.ID) defer cleanupDir() // Setup a mock for Consul API diff --git a/client/allocrunner/taskrunner/task_runner_test.go b/client/allocrunner/taskrunner/task_runner_test.go index 9e81abd316f..f3ef206cfd6 100644 --- a/client/allocrunner/taskrunner/task_runner_test.go +++ b/client/allocrunner/taskrunner/task_runner_test.go @@ -80,8 +80,7 @@ func testTaskRunnerConfig(t *testing.T, alloc *structs.Allocation, taskName stri } // Create the alloc dir + task dir - allocPath := filepath.Join(clientConf.AllocDir, alloc.ID) - allocDir := allocdir.NewAllocDir(logger, allocPath) + allocDir := allocdir.NewAllocDir(logger, clientConf.AllocDir, alloc.ID) if err := allocDir.Build(); err != nil { cleanup() t.Fatalf("error building alloc dir: %v", err) diff --git a/client/allocrunner/taskrunner/template/template_test.go b/client/allocrunner/taskrunner/template/template_test.go index 84fca0a9d25..32fa862a6cc 100644 --- a/client/allocrunner/taskrunner/template/template_test.go +++ b/client/allocrunner/taskrunner/template/template_test.go @@ -1581,7 +1581,7 @@ func TestTaskTemplateManager_Escapes(t *testing.T) { alloc := mock.Alloc() task := alloc.Job.TaskGroups[0].Tasks[0] logger := testlog.HCLogger(t) - allocDir := allocdir.NewAllocDir(logger, filepath.Join(clientConf.AllocDir, alloc.ID)) + allocDir := allocdir.NewAllocDir(logger, clientConf.AllocDir, alloc.ID) taskDir := allocDir.NewTaskDir(task.Name) containerEnv := func() *taskenv.Builder { diff --git a/client/allocwatcher/alloc_watcher.go b/client/allocwatcher/alloc_watcher.go index 35d3954022a..4e6bbb2cf78 100644 --- a/client/allocwatcher/alloc_watcher.go +++ b/client/allocwatcher/alloc_watcher.go @@ -509,7 +509,7 @@ func (p *remotePrevAlloc) getNodeAddr(ctx context.Context, nodeID string) (strin // Destroy on the returned allocdir if no error occurs. func (p *remotePrevAlloc) migrateAllocDir(ctx context.Context, nodeAddr string) (*allocdir.AllocDir, error) { // Create the previous alloc dir - prevAllocDir := allocdir.NewAllocDir(p.logger, filepath.Join(p.config.AllocDir, p.prevAllocID)) + prevAllocDir := allocdir.NewAllocDir(p.logger, p.config.AllocDir, p.prevAllocID) if err := prevAllocDir.Build(); err != nil { return nil, fmt.Errorf("error building alloc dir for previous alloc %q: %v", p.prevAllocID, err) } diff --git a/client/allocwatcher/alloc_watcher_test.go b/client/allocwatcher/alloc_watcher_test.go index ea92525a48e..4c4b6370240 100644 --- a/client/allocwatcher/alloc_watcher_test.go +++ b/client/allocwatcher/alloc_watcher_test.go @@ -36,12 +36,11 @@ func newFakeAllocRunner(t *testing.T, logger hclog.Logger) *fakeAllocRunner { alloc.Job.TaskGroups[0].EphemeralDisk.Sticky = true alloc.Job.TaskGroups[0].EphemeralDisk.Migrate = true - path, err := ioutil.TempDir("", "nomad_test_watcher") - require.NoError(t, err) + path := t.TempDir() return &fakeAllocRunner{ alloc: alloc, - AllocDir: allocdir.NewAllocDir(logger, path), + AllocDir: allocdir.NewAllocDir(logger, path, alloc.ID), Broadcaster: cstructs.NewAllocBroadcaster(logger), } } diff --git a/client/fs_endpoint_test.go b/client/fs_endpoint_test.go index 60bb0deefff..fa650df2767 100644 --- a/client/fs_endpoint_test.go +++ b/client/fs_endpoint_test.go @@ -31,19 +31,14 @@ import ( "github.com/stretchr/testify/require" ) -// tempAllocDir returns a new alloc dir that is rooted in a temp dir. The caller -// should destroy the temp dir. +// tempAllocDir returns a new alloc dir that is rooted in a temp dir. Caller +// should cleanup with AllocDir.Destroy() func tempAllocDir(t testing.TB) *allocdir.AllocDir { - dir, err := ioutil.TempDir("", "nomadtest") - if err != nil { - t.Fatalf("TempDir() failed: %v", err) - } + dir := t.TempDir() - if err := os.Chmod(dir, 0777); err != nil { - t.Fatalf("failed to chmod dir: %v", err) - } + require.NoError(t, os.Chmod(dir, 0o777)) - return allocdir.NewAllocDir(testlog.HCLogger(t), dir) + return allocdir.NewAllocDir(testlog.HCLogger(t), dir, "test_allocid") } type nopWriteCloser struct { @@ -1561,12 +1556,11 @@ func TestFS_findClosest(t *testing.T) { func TestFS_streamFile_NoFile(t *testing.T) { t.Parallel() - require := require.New(t) c, cleanup := TestClient(t, nil) defer cleanup() ad := tempAllocDir(t) - defer os.RemoveAll(ad.AllocDir) + defer ad.Destroy() frames := make(chan *sframer.StreamFrame, 32) framer := sframer.NewStreamFramer(frames, streamHeartbeatRate, streamBatchWindow, streamFrameSize) @@ -1575,11 +1569,11 @@ func TestFS_streamFile_NoFile(t *testing.T) { err := c.endpoints.FileSystem.streamFile( context.Background(), 0, "foo", 0, ad, framer, nil) - require.NotNil(err) + require.Error(t, err) if runtime.GOOS == "windows" { - require.Contains(err.Error(), "cannot find the file") + require.Contains(t, err.Error(), "cannot find the file") } else { - require.Contains(err.Error(), "no such file") + require.Contains(t, err.Error(), "no such file") } } @@ -1591,7 +1585,8 @@ func TestFS_streamFile_Modify(t *testing.T) { // Get a temp alloc dir ad := tempAllocDir(t) - defer os.RemoveAll(ad.AllocDir) + require.NoError(t, ad.Build()) + defer ad.Destroy() // Create a file in the temp dir streamFile := "stream_file" @@ -1660,16 +1655,15 @@ func TestFS_streamFile_Truncate(t *testing.T) { // Get a temp alloc dir ad := tempAllocDir(t) - defer os.RemoveAll(ad.AllocDir) + require.NoError(t, ad.Build()) + defer ad.Destroy() // Create a file in the temp dir data := []byte("helloworld") streamFile := "stream_file" streamFilePath := filepath.Join(ad.AllocDir, streamFile) f, err := os.Create(streamFilePath) - if err != nil { - t.Fatalf("Failed to create file: %v", err) - } + require.NoError(t, err) defer f.Close() // Start the reader @@ -1768,7 +1762,8 @@ func TestFS_streamImpl_Delete(t *testing.T) { // Get a temp alloc dir ad := tempAllocDir(t) - defer os.RemoveAll(ad.AllocDir) + require.NoError(t, ad.Build()) + defer ad.Destroy() // Create a file in the temp dir data := []byte("helloworld") @@ -1840,7 +1835,8 @@ func TestFS_logsImpl_NoFollow(t *testing.T) { // Get a temp alloc dir and create the log dir ad := tempAllocDir(t) - defer os.RemoveAll(ad.AllocDir) + require.NoError(t, ad.Build()) + defer ad.Destroy() logDir := filepath.Join(ad.SharedDir, allocdir.LogDirName) if err := os.MkdirAll(logDir, 0777); err != nil { @@ -1908,7 +1904,8 @@ func TestFS_logsImpl_Follow(t *testing.T) { // Get a temp alloc dir and create the log dir ad := tempAllocDir(t) - defer os.RemoveAll(ad.AllocDir) + require.NoError(t, ad.Build()) + defer ad.Destroy() logDir := filepath.Join(ad.SharedDir, allocdir.LogDirName) if err := os.MkdirAll(logDir, 0777); err != nil { diff --git a/command/agent/consul/int_test.go b/command/agent/consul/int_test.go index f725afd638b..12aa80f8f35 100644 --- a/command/agent/consul/int_test.go +++ b/command/agent/consul/int_test.go @@ -4,7 +4,6 @@ import ( "context" "io/ioutil" "os" - "path/filepath" "testing" "time" @@ -126,7 +125,7 @@ func TestConsul_Integration(t *testing.T) { logger := testlog.HCLogger(t) logUpdate := &mockUpdater{logger} - allocDir := allocdir.NewAllocDir(logger, filepath.Join(conf.AllocDir, alloc.ID)) + allocDir := allocdir.NewAllocDir(logger, conf.AllocDir, alloc.ID) if err := allocDir.Build(); err != nil { t.Fatalf("error building alloc dir: %v", err) } diff --git a/drivers/docker/driver_unix_test.go b/drivers/docker/driver_unix_test.go index 019b42764b1..701ed898571 100644 --- a/drivers/docker/driver_unix_test.go +++ b/drivers/docker/driver_unix_test.go @@ -765,15 +765,15 @@ func copyImage(t *testing.T, taskDir *allocdir.TaskDir, image string) { // copyFile moves an existing file to the destination func copyFile(src, dst string, t *testing.T) { + t.Helper() in, err := os.Open(src) if err != nil { t.Fatalf("copying %v -> %v failed: %v", src, dst, err) } defer in.Close() out, err := os.Create(dst) - if err != nil { - t.Fatalf("copying %v -> %v failed: %v", src, dst, err) - } + require.NoError(t, err, "copying %v -> %v failed: %v", src, dst, err) + defer func() { if err := out.Close(); err != nil { t.Fatalf("copying %v -> %v failed: %v", src, dst, err) diff --git a/drivers/shared/executor/executor_linux_test.go b/drivers/shared/executor/executor_linux_test.go index a8a71f88448..687c646350e 100644 --- a/drivers/shared/executor/executor_linux_test.go +++ b/drivers/shared/executor/executor_linux_test.go @@ -64,7 +64,7 @@ func testExecutorCommandWithChroot(t *testing.T) *testExecCmd { task := alloc.Job.TaskGroups[0].Tasks[0] taskEnv := taskenv.NewBuilder(mock.Node(), alloc, task, "global").Build() - allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), filepath.Join(os.TempDir(), alloc.ID)) + allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), os.TempDir(), alloc.ID) if err := allocDir.Build(); err != nil { t.Fatalf("AllocDir.Build() failed: %v", err) } diff --git a/drivers/shared/executor/executor_test.go b/drivers/shared/executor/executor_test.go index cd15321b65b..6a396ea6a8f 100644 --- a/drivers/shared/executor/executor_test.go +++ b/drivers/shared/executor/executor_test.go @@ -61,7 +61,7 @@ func testExecutorCommand(t *testing.T) *testExecCmd { task := alloc.Job.TaskGroups[0].Tasks[0] taskEnv := taskenv.NewBuilder(mock.Node(), alloc, task, "global").Build() - allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), filepath.Join(os.TempDir(), alloc.ID)) + allocDir := allocdir.NewAllocDir(testlog.HCLogger(t), t.TempDir(), alloc.ID) if err := allocDir.Build(); err != nil { t.Fatalf("AllocDir.Build() failed: %v", err) } diff --git a/plugins/drivers/testutils/testing.go b/plugins/drivers/testutils/testing.go index 8088381db6c..32cc2447731 100644 --- a/plugins/drivers/testutils/testing.go +++ b/plugins/drivers/testutils/testing.go @@ -83,10 +83,12 @@ func (h *DriverHarness) Kill() { func (h *DriverHarness) MkAllocDir(t *drivers.TaskConfig, enableLogs bool) func() { dir, err := ioutil.TempDir("", "nomad_driver_harness-") require.NoError(h.t, err) - t.AllocDir = dir - allocDir := allocdir.NewAllocDir(h.logger, dir) + allocDir := allocdir.NewAllocDir(h.logger, dir, t.AllocID) require.NoError(h.t, allocDir.Build()) + + t.AllocDir = allocDir.AllocDir + taskDir := allocDir.NewTaskDir(t.Name) caps, err := h.Capabilities() diff --git a/testutil/wait.go b/testutil/wait.go index 4f9965a6a82..0f4dfffed63 100644 --- a/testutil/wait.go +++ b/testutil/wait.go @@ -201,7 +201,9 @@ func WaitForRunningWithToken(t testing.TB, rpc rpcFn, job *structs.Job, token st var resp structs.JobAllocationsResponse - WaitForResult(func() (bool, error) { + // This can be quite slow if the job has expensive setup such as + // downloading large artifacts or creating a chroot. + WaitForResultRetries(2000*TestMultiplier(), func() (bool, error) { args := &structs.JobSpecificRequest{} args.JobID = job.ID args.QueryOptions.Region = job.Region diff --git a/website/content/docs/configuration/client.mdx b/website/content/docs/configuration/client.mdx index 368de73d14d..f1a9ecc43cf 100644 --- a/website/content/docs/configuration/client.mdx +++ b/website/content/docs/configuration/client.mdx @@ -189,6 +189,9 @@ environment with the most commonly used parts of the operating system. Please see the [Nomad `exec` driver documentation](/docs/drivers/exec#chroot) for the full list. +As of Nomad 1.2, Nomad will never attempt to embed the `alloc_dir` in the +chroot as doing so would cause infinite recursion. + ### `options` Parameters ~> Note: In Nomad 0.9 client configuration options for drivers were deprecated. diff --git a/website/content/docs/configuration/index.mdx b/website/content/docs/configuration/index.mdx index 1d3d2bf37fa..7013a3ef546 100644 --- a/website/content/docs/configuration/index.mdx +++ b/website/content/docs/configuration/index.mdx @@ -151,10 +151,6 @@ testing. directory to store cluster state, including the replicated log and snapshot data. This must be specified as an absolute path. - ~> **WARNING**: This directory **must not** be set to a directory that is - [included in the chroot](/docs/drivers/exec#chroot) if you use the - [`exec`](/docs/drivers/exec) driver. - - `disable_anonymous_signature` `(bool: false)` - Specifies if Nomad should provide an anonymous signature for de-duplication with the update check.