Merge pull request #11334 from hashicorp/f-chroot-skip-allocdir

client: never embed alloc_dir in chroot

.changelog/11334.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+client: Never embed client.alloc_dir in chroots to prevent infinite recursion from misconfiguration.
+```

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,44 +108,24 @@ 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,
+		clientAllocDir: clientAllocDir,
-		SharedDir: filepath.Join(allocDir, SharedAllocName),
+		AllocDir:       allocDir,
-		TaskDirs:  make(map[string]*TaskDir),
+		SharedDir:      filepath.Join(allocDir, SharedAllocName),
-		logger:    logger,
+		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()
-	}
-	return dcopy
-}
-
 // NewTaskDir creates a new TaskDir and adds it to the AllocDirs TaskDirs map.
 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
 }

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)
+	}
+}

client/allocdir/task_dir.go

@@ -39,6 +39,10 @@ type TaskDir struct {
 	// <task_dir>/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) {

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 {

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 {

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)
 

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.

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)

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

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))
 

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

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

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, clientConf.AllocDir, alloc.ID)
-	allocDir := allocdir.NewAllocDir(logger, allocPath)
 	if err := allocDir.Build(); err != nil {
 		cleanup()
 		t.Fatalf("error building alloc dir: %v", err)

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 {

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)
 	}

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")
+	path := t.TempDir()
-	require.NoError(t, err)
 
 	return &fakeAllocRunner{
 		alloc:       alloc,
-		AllocDir:    allocdir.NewAllocDir(logger, path),
+		AllocDir:    allocdir.NewAllocDir(logger, path, alloc.ID),
 		Broadcaster: cstructs.NewAllocBroadcaster(logger),
 	}
 }

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
+// tempAllocDir returns a new alloc dir that is rooted in a temp dir. Caller
-// should destroy the temp dir.
+// should cleanup with AllocDir.Destroy()
 func tempAllocDir(t testing.TB) *allocdir.AllocDir {
-	dir, err := ioutil.TempDir("", "nomadtest")
+	dir := t.TempDir()
-	if err != nil {
-		t.Fatalf("TempDir() failed: %v", err)
-	}
 
-	if err := os.Chmod(dir, 0777); err != nil {
+	require.NoError(t, os.Chmod(dir, 0o777))
-		t.Fatalf("failed to chmod dir: %v", err)
-	}
 
-	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 {
+	require.NoError(t, err)
-		t.Fatalf("Failed to create file: %v", 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 {

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)
 	}

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 {
+	require.NoError(t, err, "copying %v -> %v failed: %v", src, dst, err)
-		t.Fatalf("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)

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)
 	}

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)
 	}

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()

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

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.

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.