Put docker volume support behind conf flag

Also add tests and fix bug with logging driver configuration.

client/driver/docker.go

@@ -58,6 +58,10 @@ const (
 	// driver
 	dockerDriverAttr = "driver.docker"
 
+	dockerSELinuxLabelConfigOption = "docker.volumes.selinuxlabel"
+	dockerVolumesConfigOption      = "docker.volumes.enabled"
+	dockerPrivilegedConfigOption   = "docker.privileged.enabled"
+
 	// dockerTimeout is the length of time a request can be outstanding before
 	// it is timed out.
 	dockerTimeout = 1 * time.Minute
@@ -116,27 +120,9 @@ func (c *DockerDriverConfig) Validate() error {
 
 	c.PortMap = mapMergeStrInt(c.PortMapRaw...)
 	c.Labels = mapMergeStrStr(c.LabelsRaw...)
-
-	//if no logging is present, set default values. Otherwise, convert the raw logging data into a logging object
-	if len(c.Logging) != 0 {
+	if len(c.Logging) > 0 {
 		c.Logging[0].Config = mapMergeStrStr(c.Logging[0].ConfigRaw...)
-	} else {
-		c.Logging = []DockerLoggingOpts{
-			{
-				Type:   "syslog",
-				Config: make(map[string]string),
-			},
-		}
-	}
-
-	if c.Volumes == nil {
-		c.Volumes = make([]string, 0)
 	}
-
-	if c.VolumesFrom == nil {
-		c.VolumesFrom = make([]string, 0)
-	}
-
 	return nil
 }
 
@@ -358,9 +344,9 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
 		return false, nil
 	}
 
-	privileged := d.config.ReadBoolDefault("docker.privileged.enabled", false)
+	privileged := d.config.ReadBoolDefault(dockerPrivilegedConfigOption, false)
 	if privileged {
-		node.Attributes["docker.privileged.enabled"] = "1"
+		node.Attributes[dockerPrivilegedConfigOption] = "1"
 	}
 
 	// This is the first operation taken on the client so we'll try to
@@ -377,10 +363,17 @@ func (d *DockerDriver) Fingerprint(cfg *config.Config, node *structs.Node) (bool
 
 	node.Attributes[dockerDriverAttr] = "1"
 	node.Attributes["driver.docker.version"] = env.Get("Version")
+
+	if d.config.ReadBoolDefault(dockerVolumesConfigOption, false) {
+		node.Attributes["driver."+dockerVolumesConfigOption] = "1"
+	}
+
 	return true, nil
 }
 
-func (d *DockerDriver) containerBinds(alloc *allocdir.AllocDir, task *structs.Task) ([]string, error) {
+func (d *DockerDriver) containerBinds(driverConfig *DockerDriverConfig, alloc *allocdir.AllocDir,
+	task *structs.Task) ([]string, error) {
+
 	shared := alloc.SharedDir
 	local, ok := alloc.TaskDirs[task.Name]
 	if !ok {
@@ -394,17 +387,34 @@ func (d *DockerDriver) containerBinds(alloc *allocdir.AllocDir, task *structs.Ta
 	allocDirBind := fmt.Sprintf("%s:%s", shared, allocdir.SharedAllocContainerPath)
 	taskLocalBind := fmt.Sprintf("%s:%s", local, allocdir.TaskLocalContainerPath)
 	secretDirBind := fmt.Sprintf("%s:%s", secret, allocdir.TaskSecretsContainerPath)
+	binds := []string{allocDirBind, taskLocalBind, secretDirBind}
 
-	if selinuxLabel := d.config.Read("docker.volumes.selinuxlabel"); selinuxLabel != "" {
-		allocDirBind = fmt.Sprintf("%s:%s", allocDirBind, selinuxLabel)
-		taskLocalBind = fmt.Sprintf("%s:%s", taskLocalBind, selinuxLabel)
-		secretDirBind = fmt.Sprintf("%s:%s", secretDirBind, selinuxLabel)
+	var merr multierror.Error
+	volumesEnabled := d.config.ReadBoolDefault(dockerVolumesConfigOption, false)
+	if len(driverConfig.Volumes) > 0 && !volumesEnabled {
+		merr.Errors = append(merr.Errors, fmt.Errorf(dockerVolumesConfigOption+" is false; cannot use Docker Volumes: %+q", driverConfig.Volumes))
 	}
-	return []string{
-		allocDirBind,
-		taskLocalBind,
-		secretDirBind,
-	}, nil
+
+	if len(driverConfig.VolumesFrom) > 0 && !volumesEnabled {
+		merr.Errors = append(merr.Errors, fmt.Errorf(dockerVolumesConfigOption+" is false; cannot use Docker VolumesFrom: %+q", driverConfig.VolumesFrom))
+	}
+
+	if err := merr.ErrorOrNil(); err != nil {
+		return nil, err
+	}
+
+	if len(driverConfig.Volumes) > 0 {
+		binds = append(binds, driverConfig.Volumes...)
+	}
+
+	if selinuxLabel := d.config.Read(dockerSELinuxLabelConfigOption); selinuxLabel != "" {
+		// Apply SELinux Label to each volume
+		for i := range binds {
+			binds[i] = fmt.Sprintf("%s:%s", binds[i], selinuxLabel)
+		}
+	}
+
+	return binds, nil
 }
 
 // createContainer initializes a struct needed to call docker.client.CreateContainer()
@@ -418,7 +428,7 @@ func (d *DockerDriver) createContainer(ctx *ExecContext, task *structs.Task,
 		return c, fmt.Errorf("task.Resources is empty")
 	}
 
-	binds, err := d.containerBinds(ctx.AllocDir, task)
+	binds, err := d.containerBinds(driverConfig, ctx.AllocDir, task)
 	if err != nil {
 		return c, err
 	}
@@ -442,20 +452,15 @@ func (d *DockerDriver) createContainer(ctx *ExecContext, task *structs.Task,
 
 	memLimit := int64(task.Resources.MemoryMB) * 1024 * 1024
 
-	if len(driverConfig.Logging) != 0 {
+	if len(driverConfig.Logging) == 0 {
 		d.logger.Printf("[DEBUG] driver.docker: Setting default logging options to syslog and %s", syslogAddr)
-		if driverConfig.Logging[0].Type == "" {
-			driverConfig.Logging[0].Type = "syslog"
-			driverConfig.Logging[0].Config["syslog-address"] = syslogAddr
+		driverConfig.Logging = []DockerLoggingOpts{
+			{Type: "syslog", Config: map[string]string{"syslog-address": syslogAddr}},
 		}
 	}
 
 	d.logger.Printf("[DEBUG] driver.docker: Using config for logging: %+v", driverConfig.Logging[0])
 
-	//Merge nomad container binds and user specified binds
-	d.logger.Printf("[DEBUG] Unmodified binds from nomad: %+v\n", binds)
-	binds = append(binds, driverConfig.Volumes...)
-
 	hostConfig := &docker.HostConfig{
 		// Convert MB to bytes. This is an absolute value.
 		Memory:     memLimit,
@@ -477,10 +482,12 @@ func (d *DockerDriver) createContainer(ctx *ExecContext, task *structs.Task,
 	d.logger.Printf("[DEBUG] driver.docker: using %d bytes memory for %s", hostConfig.Memory, task.Name)
 	d.logger.Printf("[DEBUG] driver.docker: using %d cpu shares for %s", hostConfig.CPUShares, task.Name)
 	d.logger.Printf("[DEBUG] driver.docker: binding directories %#v for %s", hostConfig.Binds, task.Name)
-	d.logger.Printf("[DEBUG] driver.docker: binding Volumes-From: %#v for %s", hostConfig.VolumesFrom, task.Name)
+	if d.config.ReadBoolDefault(dockerVolumesConfigOption, false) {
+		d.logger.Printf("[DEBUG] driver.docker: binding Volumes-From: %#v for %s", hostConfig.VolumesFrom, task.Name)
+	}
 
 	//  set privileged mode
-	hostPrivileged := d.config.ReadBoolDefault("docker.privileged.enabled", false)
+	hostPrivileged := d.config.ReadBoolDefault(dockerPrivilegedConfigOption, false)
 	if driverConfig.Privileged && !hostPrivileged {
 		return c, fmt.Errorf(`Docker privileged mode is disabled on this Nomad agent`)
 	}

client/driver/docker_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"io/ioutil"
 	"math/rand"
+	"os"
+	"path"
 	"path/filepath"
 	"reflect"
 	"runtime/debug"
@@ -17,6 +19,7 @@ import (
 	"github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/client/driver/env"
 	"github.com/hashicorp/nomad/client/testutil"
+	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	tu "github.com/hashicorp/nomad/testutil"
 )
@@ -861,6 +864,97 @@ func TestDockerDriver_Stats(t *testing.T) {
 
 }
 
+func setupDockerVolumes(t *testing.T, cfg *config.Config) (*structs.Task, Driver, *ExecContext, string, func()) {
+	if !testutil.DockerIsConnected(t) {
+		t.SkipNow()
+	}
+
+	tmpvol, err := ioutil.TempDir("", "nomadtest_dockerdriver_volumes")
+	if err != nil {
+		t.Fatalf("error creating temporary dir: %v", err)
+	}
+
+	randfn := fmt.Sprintf("test-%d", rand.Int())
+	hostpath := path.Join(tmpvol, randfn)
+	contpath := path.Join("/mnt/vol", randfn)
+
+	task := &structs.Task{
+		Name: "ls",
+		Config: map[string]interface{}{
+			"image":   "busybox",
+			"load":    []string{"busybox.tar"},
+			"command": "touch",
+			"args":    []string{contpath},
+			"volumes": []string{fmt.Sprintf("%s:/mnt/vol", tmpvol)},
+		},
+		LogConfig: &structs.LogConfig{
+			MaxFiles:      10,
+			MaxFileSizeMB: 10,
+		},
+		Resources: basicResources,
+	}
+
+	allocDir := allocdir.NewAllocDir(filepath.Join(cfg.AllocDir, structs.GenerateUUID()), task.Resources.DiskMB)
+	allocDir.Build([]*structs.Task{task})
+	alloc := mock.Alloc()
+	execCtx := NewExecContext(allocDir, alloc.ID)
+	cleanup := func() {
+		execCtx.AllocDir.Destroy()
+		os.RemoveAll(tmpvol)
+	}
+
+	taskEnv, err := GetTaskEnv(allocDir, cfg.Node, task, alloc, "")
+	if err != nil {
+		cleanup()
+		t.Fatalf("Failed to get task env: %v", err)
+	}
+
+	driverCtx := NewDriverContext(task.Name, cfg, cfg.Node, testLogger(), taskEnv)
+	driver := NewDockerDriver(driverCtx)
+	copyImage(execCtx, task, "busybox.tar", t)
+
+	return task, driver, execCtx, hostpath, cleanup
+}
+
+func TestDockerDriver_VolumesDisabled(t *testing.T) {
+	cfg := testConfig()
+
+	task, driver, execCtx, _, cleanup := setupDockerVolumes(t, cfg)
+	defer cleanup()
+
+	_, err := driver.Start(execCtx, task)
+	if err == nil {
+		t.Fatalf("Started driver successfully when volumes should have been disabled.")
+	}
+}
+
+func TestDockerDriver_VolumesEnabled(t *testing.T) {
+	cfg := testConfig()
+	cfg.Options = map[string]string{dockerVolumesConfigOption: "true"}
+
+	task, driver, execCtx, hostpath, cleanup := setupDockerVolumes(t, cfg)
+	defer cleanup()
+
+	handle, err := driver.Start(execCtx, task)
+	if err != nil {
+		t.Fatalf("Failed to start docker driver: %v", err)
+	}
+	defer handle.Kill()
+
+	select {
+	case res := <-handle.WaitCh():
+		if !res.Successful() {
+			t.Fatalf("unexpected err: %v", res)
+		}
+	case <-time.After(time.Duration(tu.TestMultiplier()*10) * time.Second):
+		t.Fatalf("timeout")
+	}
+
+	if _, err := ioutil.ReadFile(hostpath); err != nil {
+		t.Fatalf("unexpected error reading %s: %v", hostpath, err)
+	}
+}
+
 func copyImage(execCtx *ExecContext, task *structs.Task, image string, t *testing.T) {
 	taskDir, _ := execCtx.AllocDir.TaskDirs[task.Name]
 	dst := filepath.Join(taskDir, allocdir.TaskLocal, image)

command/agent/config.go

@@ -432,6 +432,9 @@ func DevConfig() *Config {
 	conf.Client.Options = map[string]string{
 		"driver.raw_exec.enable": "true",
 	}
+	conf.Client.Options = map[string]string{
+		"driver.docker.volumes": "true",
+	}
 
 	return conf
 }

website/source/docs/drivers/docker.html.md

@@ -137,6 +137,14 @@ The `docker` driver supports the following configuration in the job spec:
 
 * `shm_size` - (Optional) The size (bytes) of /dev/shm for the container.
 
+* `volumes` - (Optional) A list of `host_path:container_path` strings to bind
+  host paths to container paths. Can only be run on clients with the
+  `docker.volumes.enabled` option set to true.
+
+* `volumes_from` - (Optional) A list of volumes to inherit from another
+  container. Can only be run on clients with the `docker.volumes.enabled`
+  option set to true.
+
 * `work_dir` - (Optional) The working directory inside the container.
 
 ### Container Name
@@ -329,8 +337,14 @@ options](/docs/agent/config.html#options):
 * `docker.cleanup.image` Defaults to `true`. Changing this to `false` will
   prevent Nomad from removing images from stopped tasks.
 
+* `docker.volumes.enabled`: Defaults to `false`. Allows tasks to bind host
+  paths (`volumes`) or other containers (`volums_from`) inside their container.
+  Disabled by default as it removes the isolation between containers' data.
+
 * `docker.volumes.selinuxlabel`: Allows the operator to set a SELinux
-  label to the allocation and task local bind-mounts to containers.
+  label to the allocation and task local bind-mounts to containers. If used
+  with `docker.volumes.enabled` set to false, the labels will still be applied
+  to the standard binds in the container. 
 
 * `docker.privileged.enabled` Defaults to `false`. Changing this to `true` will
   allow containers to use `privileged` mode, which gives the containers full