docker: introduce a new hcl2-friendly mount syntax (#9635)

Introduce a new more-block friendly syntax for specifying mounts with a new `mount` block type with the target as label:

```hcl
config {
  image = "..."

  mount {
    type = "..."
    target = "target-path"
    volume_options { ... }
  }
}
```

The main benefit here is that by `mount` being a block, it can nest blocks and avoids the compatibility problems noted in https://github.com/hashicorp/nomad/pull/9634/files#diff-2161d829655a3a36ba2d916023e4eec125b9bd22873493c1c2e5e3f7ba92c691R128-R155 .

The intention is for us to promote this `mount` blocks and quietly deprecate the `mounts` type, while still honoring to preserve compatibility as much as we could.

This addresses the issue in #9604 .

CHANGELOG.md

@@ -2,9 +2,11 @@
 
 IMPROVEMENTS:
  * consul/connect: interpolate the connect, service meta, and service canary meta blocks with the task environment [[GH-9586](https://github.com/hashicorp/nomad/pull/9586)]
+ * drivers/docker: Added a new syntax for specifying `mount` [[GH-9635](https://github.com/hashicorp/nomad/issues/9635)]
 
 BUG FIXES:
  * core: Fixed a bug where ACLToken and ACLPolicy changes were ignored by the event stream [[GH-9595](https://github.com/hashicorp/nomad/issues/9595)]
+ * core: Fixed a bug to honor HCL2 variables set by environment variables or variable files [[GH-9592](https://github.com/hashicorp/nomad/issues/9592)] [[GH-9623](https://github.com/hashicorp/nomad/issues/9623)]
 
 ## 1.0.0 (December 8, 2020)
 

drivers/docker/config.go

@@ -284,6 +284,32 @@ var (
 		"disable_log_collection": hclspec.NewAttr("disable_log_collection", "bool", false),
 	})
 
+	// mountBodySpec is the hcl specification for the `mount` block
+	mountBodySpec = hclspec.NewObject(map[string]*hclspec.Spec{
+		"type": hclspec.NewDefault(
+			hclspec.NewAttr("type", "string", false),
+			hclspec.NewLiteral("\"volume\""),
+		),
+		"target":   hclspec.NewAttr("target", "string", false),
+		"source":   hclspec.NewAttr("source", "string", false),
+		"readonly": hclspec.NewAttr("readonly", "bool", false),
+		"bind_options": hclspec.NewBlock("bind_options", false, hclspec.NewObject(map[string]*hclspec.Spec{
+			"propagation": hclspec.NewAttr("propagation", "string", false),
+		})),
+		"tmpfs_options": hclspec.NewBlock("tmpfs_options", false, hclspec.NewObject(map[string]*hclspec.Spec{
+			"size": hclspec.NewAttr("size", "number", false),
+			"mode": hclspec.NewAttr("mode", "number", false),
+		})),
+		"volume_options": hclspec.NewBlock("volume_options", false, hclspec.NewObject(map[string]*hclspec.Spec{
+			"no_copy": hclspec.NewAttr("no_copy", "bool", false),
+			"labels":  hclspec.NewAttr("labels", "list(map(string))", false),
+			"driver_config": hclspec.NewBlock("driver_config", false, hclspec.NewObject(map[string]*hclspec.Spec{
+				"name":    hclspec.NewAttr("name", "string", false),
+				"options": hclspec.NewAttr("options", "list(map(string))", false),
+			})),
+		})),
+	})
+
 	// taskConfigSpec is the hcl specification for the driver config section of
 	// a task within a job. It is returned in the TaskConfigSchema RPC
 	taskConfigSpec = hclspec.NewObject(map[string]*hclspec.Spec{
@@ -331,30 +357,11 @@ var (
 		})),
 		"mac_address":       hclspec.NewAttr("mac_address", "string", false),
 		"memory_hard_limit": hclspec.NewAttr("memory_hard_limit", "number", false),
-		"mounts": hclspec.NewBlockList("mounts", hclspec.NewObject(map[string]*hclspec.Spec{
-			"type": hclspec.NewDefault(
-				hclspec.NewAttr("type", "string", false),
-				hclspec.NewLiteral("\"volume\""),
-			),
-			"target":   hclspec.NewAttr("target", "string", false),
-			"source":   hclspec.NewAttr("source", "string", false),
-			"readonly": hclspec.NewAttr("readonly", "bool", false),
-			"bind_options": hclspec.NewBlock("bind_options", false, hclspec.NewObject(map[string]*hclspec.Spec{
-				"propagation": hclspec.NewAttr("propagation", "string", false),
-			})),
-			"tmpfs_options": hclspec.NewBlock("tmpfs_options", false, hclspec.NewObject(map[string]*hclspec.Spec{
-				"size": hclspec.NewAttr("size", "number", false),
-				"mode": hclspec.NewAttr("mode", "number", false),
-			})),
-			"volume_options": hclspec.NewBlock("volume_options", false, hclspec.NewObject(map[string]*hclspec.Spec{
-				"no_copy": hclspec.NewAttr("no_copy", "bool", false),
-				"labels":  hclspec.NewAttr("labels", "list(map(string))", false),
-				"driver_config": hclspec.NewBlock("driver_config", false, hclspec.NewObject(map[string]*hclspec.Spec{
-					"name":    hclspec.NewAttr("name", "string", false),
-					"options": hclspec.NewAttr("options", "list(map(string))", false),
-				})),
-			})),
-		})),
+		// mount and mounts are effectively aliases, but `mounts` is meant for pre-1.0
+		// assignment syntax `mounts = [{type="..." ..."}]` while
+		// `mount` is 1.0 repeated block syntax `mount { type = "..." }`
+		"mount":           hclspec.NewBlockList("mount", mountBodySpec),
+		"mounts":          hclspec.NewBlockList("mounts", mountBodySpec),
 		"network_aliases": hclspec.NewAttr("network_aliases", "list(string)", false),
 		"network_mode":    hclspec.NewAttr("network_mode", "string", false),
 		"runtime":         hclspec.NewAttr("runtime", "string", false),
@@ -426,7 +433,7 @@ type TaskConfig struct {
 	Logging           DockerLogging      `codec:"logging"`
 	MacAddress        string             `codec:"mac_address"`
 	MemoryHardLimit   int64              `codec:"memory_hard_limit"`
-	Mounts            []DockerMount      `codec:"mounts"`
+	Mounts            []DockerMount      `codec:"mount"`
 	NetworkAliases    []string           `codec:"network_aliases"`
 	NetworkMode       string             `codec:"network_mode"`
 	Runtime           string             `codec:"runtime"`
@@ -448,6 +455,9 @@ type TaskConfig struct {
 	Volumes           []string           `codec:"volumes"`
 	VolumeDriver      string             `codec:"volume_driver"`
 	WorkDir           string             `codec:"work_dir"`
+
+	// MountsList supports the pre-1.0 mounts array syntax
+	MountsList []DockerMount `codec:"mounts"`
 }
 
 type DockerAuth struct {

drivers/docker/config_test.go

@@ -24,6 +24,7 @@ func TestConfig_ParseHCL(t *testing.T) {
 				Image:            "redis:3.2",
 				Devices:          []DockerDevice{},
 				Mounts:           []DockerMount{},
+				MountsList:       []DockerMount{},
 				CPUCFSPeriod:     100000,
 				ImagePullTimeout: "5m",
 			},
@@ -56,6 +57,7 @@ func TestConfig_ParseJSON(t *testing.T) {
 			expected: TaskConfig{
 				Image:            "bash:3",
 				Mounts:           []DockerMount{},
+				MountsList:       []DockerMount{},
 				Devices:          []DockerDevice{},
 				CPUCFSPeriod:     100000,
 				ImagePullTimeout: "5m",
@@ -67,6 +69,7 @@ func TestConfig_ParseJSON(t *testing.T) {
 			expected: TaskConfig{
 				Image:            "bash:3",
 				Mounts:           []DockerMount{},
+				MountsList:       []DockerMount{},
 				Devices:          []DockerDevice{},
 				CPUCFSPeriod:     100000,
 				ImagePullTimeout: "5m",
@@ -78,6 +81,7 @@ func TestConfig_ParseJSON(t *testing.T) {
 			expected: TaskConfig{
 				Image:            "bash:3",
 				Mounts:           []DockerMount{},
+				MountsList:       []DockerMount{},
 				Devices:          []DockerDevice{},
 				CPUCFSPeriod:     100000,
 				ImagePullTimeout: "5m",
@@ -89,6 +93,7 @@ func TestConfig_ParseJSON(t *testing.T) {
 			expected: TaskConfig{
 				Image:            "bash:3",
 				Mounts:           []DockerMount{},
+				MountsList:       []DockerMount{},
 				Devices:          []DockerDevice{},
 				CPUCFSPeriod:     100000,
 				ImagePullTimeout: "5m",
@@ -229,6 +234,27 @@ config {
   }
   mac_address = "02:42:ac:11:00:02"
   memory_hard_limit = 512
+
+  mount {
+    type = "bind"
+    target ="/mount-bind-target"
+    source = "/bind-source-mount"
+    readonly = true
+    bind_options {
+      propagation = "rshared"
+    }
+  }
+
+  mount {
+    type = "tmpfs"
+    target ="/mount-tmpfs-target"
+    readonly = true
+    tmpfs_options {
+      size = 30000
+      mode = 0777
+    }
+  }
+
   mounts = [
     {
       type = "bind"
@@ -361,6 +387,27 @@ config {
 		MacAddress:      "02:42:ac:11:00:02",
 		MemoryHardLimit: 512,
 		Mounts: []DockerMount{
+			{
+				Type:     "bind",
+				Target:   "/mount-bind-target",
+				Source:   "/bind-source-mount",
+				ReadOnly: true,
+				BindOptions: DockerBindOptions{
+					Propagation: "rshared",
+				},
+			},
+			{
+				Type:     "tmpfs",
+				Target:   "/mount-tmpfs-target",
+				Source:   "",
+				ReadOnly: true,
+				TmpfsOptions: DockerTmpfsOptions{
+					SizeBytes: 30000,
+					Mode:      511,
+				},
+			},
+		},
+		MountsList: []DockerMount{
 			{
 				Type:     "bind",
 				Target:   "/bind-target",

drivers/docker/driver.go

@@ -947,32 +947,18 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T
 
 	// Setup mounts
 	for _, m := range driverConfig.Mounts {
-		hm, err := m.toDockerHostMount()
+		hm, err := d.toDockerMount(&m, task)
 		if err != nil {
 			return c, err
 		}
-
-		switch hm.Type {
-		case "bind":
-			hm.Source = expandPath(task.TaskDir().Dir, hm.Source)
-
-			// paths inside alloc dir are always allowed as they mount within
-			// a container, and treated as relative to task dir
-			if !d.config.Volumes.Enabled && !isParentPath(task.AllocDir, hm.Source) {
-				return c, fmt.Errorf(
-					"volumes are not enabled; cannot mount host path: %q %q",
-					hm.Source, task.AllocDir)
-			}
-		case "tmpfs":
-			// no source, so no sandbox check required
-		default: // "volume", but also any new thing that comes along
-			if !d.config.Volumes.Enabled {
-				return c, fmt.Errorf(
-					"volumes are not enabled; cannot mount volume: %q", hm.Source)
-			}
+		hostConfig.Mounts = append(hostConfig.Mounts, *hm)
+	}
+	for _, m := range driverConfig.MountsList {
+		hm, err := d.toDockerMount(&m, task)
+		if err != nil {
+			return c, err
 		}
-
-		hostConfig.Mounts = append(hostConfig.Mounts, hm)
+		hostConfig.Mounts = append(hostConfig.Mounts, *hm)
 	}
 
 	// Setup DNS
@@ -1177,6 +1163,35 @@ func (d *Driver) createContainerConfig(task *drivers.TaskConfig, driverConfig *T
 	}, nil
 }
 
+func (d *Driver) toDockerMount(m *DockerMount, task *drivers.TaskConfig) (*docker.HostMount, error) {
+	hm, err := m.toDockerHostMount()
+	if err != nil {
+		return nil, err
+	}
+
+	switch hm.Type {
+	case "bind":
+		hm.Source = expandPath(task.TaskDir().Dir, hm.Source)
+
+		// paths inside alloc dir are always allowed as they mount within
+		// a container, and treated as relative to task dir
+		if !d.config.Volumes.Enabled && !isParentPath(task.AllocDir, hm.Source) {
+			return nil, fmt.Errorf(
+				"volumes are not enabled; cannot mount host path: %q %q",
+				hm.Source, task.AllocDir)
+		}
+	case "tmpfs":
+		// no source, so no sandbox check required
+	default: // "volume", but also any new thing that comes along
+		if !d.config.Volumes.Enabled {
+			return nil, fmt.Errorf(
+				"volumes are not enabled; cannot mount volume: %q", hm.Source)
+		}
+	}
+
+	return &hm, nil
+}
+
 // detectIP of Docker container. Returns the first IP found as well as true if
 // the IP should be advertised (bridge network IPs return false). Returns an
 // empty string and false if no IP could be found.

drivers/docker/driver_test.go

@@ -9,6 +9,7 @@ import (
 	"reflect"
 	"runtime"
 	"runtime/debug"
+	"sort"
 	"strings"
 	"syscall"
 	"testing"
@@ -1150,6 +1151,84 @@ func TestDockerDriver_CreateContainerConfig_Logging(t *testing.T) {
 	}
 }
 
+func TestDockerDriver_CreateContainerConfig_Mounts(t *testing.T) {
+	t.Parallel()
+
+	task, cfg, ports := dockerTask(t)
+	defer freeport.Return(ports)
+
+	cfg.Mounts = []DockerMount{
+		DockerMount{
+			Type:   "bind",
+			Target: "/map-bind-target",
+			Source: "/map-source",
+		},
+		DockerMount{
+			Type:   "tmpfs",
+			Target: "/map-tmpfs-target",
+		},
+	}
+	cfg.MountsList = []DockerMount{
+		{
+			Type:   "bind",
+			Target: "/list-bind-target",
+			Source: "/list-source",
+		},
+		{
+			Type:   "tmpfs",
+			Target: "/list-tmpfs-target",
+		},
+	}
+
+	expectedSrcPrefix := "/"
+	if runtime.GOOS == "windows" {
+		expectedSrcPrefix = "redis-demo\\"
+	}
+	expected := []docker.HostMount{
+		// from mount map
+		{
+			Type:        "bind",
+			Target:      "/map-bind-target",
+			Source:      expectedSrcPrefix + "map-source",
+			BindOptions: &docker.BindOptions{},
+		},
+		{
+			Type:          "tmpfs",
+			Target:        "/map-tmpfs-target",
+			TempfsOptions: &docker.TempfsOptions{},
+		},
+		// from mount list
+		{
+			Type:        "bind",
+			Target:      "/list-bind-target",
+			Source:      expectedSrcPrefix + "list-source",
+			BindOptions: &docker.BindOptions{},
+		},
+		{
+			Type:          "tmpfs",
+			Target:        "/list-tmpfs-target",
+			TempfsOptions: &docker.TempfsOptions{},
+		},
+	}
+
+	require.NoError(t, task.EncodeConcreteDriverConfig(cfg))
+
+	dh := dockerDriverHarness(t, nil)
+	driver := dh.Impl().(*Driver)
+	driver.config.Volumes.Enabled = true
+
+	cc, err := driver.createContainerConfig(task, cfg, "org/repo:0.1")
+	require.NoError(t, err)
+
+	found := cc.HostConfig.Mounts
+	sort.Slice(found, func(i, j int) bool { return strings.Compare(found[i].Target, found[j].Target) < 0 })
+	sort.Slice(expected, func(i, j int) bool {
+		return strings.Compare(expected[i].Target, expected[j].Target) < 0
+	})
+
+	require.Equal(t, expected, found)
+}
+
 func TestDockerDriver_CreateContainerConfigWithRuntimes(t *testing.T) {
 	if !tu.IsCI() {
 		t.Parallel()

helper/pluginutils/hclutils/testing.go

@@ -71,7 +71,14 @@ func (b *HCLParser) parse(t *testing.T, config, out interface{}) {
 	require.Empty(t, diags)
 
 	ctyValue, diag, errs := ParseHclInterface(config, decSpec, b.vars)
-	require.Nil(t, errs)
+	if len(errs) > 1 {
+		t.Error("unexpected errors parsing file")
+		for _, err := range errs {
+			t.Errorf(" * %v", err)
+
+		}
+		t.FailNow()
+	}
 	require.Empty(t, diag)
 
 	// encode