dhv: mkdir plugin parameters: uid,guid,mode (#25533)

also remove Error logs from client rpc and promote plugin Debug logs to Error (since they have more info in them)

client/fingerprint/dynamic_host_volumes.go

@@ -105,7 +105,7 @@ func GetHostVolumePluginVersions(log hclog.Logger, pluginDir, nodePool string) (
 
 			fprint, err := p.Fingerprint(ctx)
 			if err != nil {
-				log.Debug("failed to get version from plugin", "error", err)
+				// Fingerprint logs the error
 				return
 			}
 

client/host_volume_endpoint.go

@@ -32,7 +32,6 @@ func (v *HostVolume) Create(
 
 	cresp, err := v.c.hostVolumeManager.Create(ctx, req)
 	if err != nil {
-		v.c.logger.Error("failed to create host volume", "name", req.Name, "error", err)
 		return err
 	}
 
@@ -53,7 +52,6 @@ func (v *HostVolume) Register(
 
 	err := v.c.hostVolumeManager.Register(ctx, req)
 	if err != nil {
-		v.c.logger.Error("failed to register host volume", "name", req.Name, "error", err)
 		return err
 	}
 
@@ -70,7 +68,6 @@ func (v *HostVolume) Delete(
 
 	_, err := v.c.hostVolumeManager.Delete(ctx, req)
 	if err != nil {
-		v.c.logger.Error("failed to delete host volume", "ID", req.ID, "error", err)
 		return err
 	}
 

client/hostvolumemanager/host_volume_plugin.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"time"
 
 	"github.com/hashicorp/go-hclog"
@@ -71,6 +72,14 @@ type HostVolumePluginDeleteResponse struct {
 const HostVolumePluginMkdirID = "mkdir"
 const HostVolumePluginMkdirVersion = "0.0.1"
 
+// HostVolumePluginMkdirParams represents the parameters{} that the "mkdir"
+// plugin will accept.
+type HostVolumePluginMkdirParams struct {
+	Uid  int
+	Gid  int
+	Mode os.FileMode
+}
+
 var _ HostVolumePlugin = &HostVolumePluginMkdir{}
 
 // HostVolumePluginMkdir is a plugin that creates a directory within the
@@ -110,20 +119,66 @@ func (p *HostVolumePluginMkdir) Create(_ context.Context,
 		return resp, nil
 	} else if !os.IsNotExist(err) {
 		// doesn't exist, but some other path error
-		log.Debug("error with plugin", "error", err)
+		log.Error("error with path", "error", err)
 		return nil, err
 	}
 
-	err := os.MkdirAll(path, 0o700)
+	params, err := decodeMkdirParams(req.Parameters)
 	if err != nil {
-		log.Debug("error with plugin", "error", err)
+		log.Error("error with parameters", "error", err)
 		return nil, err
 	}
 
+	err = os.MkdirAll(path, params.Mode)
+	if err != nil {
+		log.Error("error creating directory", "error", err)
+		return nil, fmt.Errorf("error creating directory: %w", err)
+	}
+
+	// Chown note: A uid or gid of -1 means to not change that value.
+	if err = os.Chown(path, params.Uid, params.Gid); err != nil {
+		log.Error("error changing owner/group", "error", err, "uid", params.Uid, "gid", params.Gid)
+		return nil, fmt.Errorf("error changing owner/group: %w", err)
+	}
+
 	log.Debug("plugin ran successfully")
 	return resp, nil
 }
 
+func decodeMkdirParams(in map[string]string) (HostVolumePluginMkdirParams, error) {
+	// default values if their associated keys are not in the input map
+	out := HostVolumePluginMkdirParams{
+		Mode: 0o700, // "0700"
+		Uid:  -1,    // this default translates to "do not set" in os.Chown
+		Gid:  -1,    // ditto
+	}
+	var err error
+
+	for param, val := range in {
+		switch param {
+		case "mode":
+			// mode needs special treatment - it's octal. note that this does
+			// not check whether it's a *reasonable* mode for a directory.
+			// that will be discovered during MkdirAll and subsequent usage
+			// by workloads (which we cannot predict).
+			var number uint64
+			number, err = strconv.ParseUint(val, 8, 32)
+			out.Mode = os.FileMode(number)
+		case "uid":
+			out.Uid, err = strconv.Atoi(val)
+		case "gid":
+			out.Gid, err = strconv.Atoi(val)
+		default:
+			err = fmt.Errorf("unknown mkdir parameter: %q", param)
+		}
+		if err != nil {
+			return out, fmt.Errorf("invalid value for %q: %w", param, err)
+		}
+	}
+
+	return out, nil
+}
+
 func (p *HostVolumePluginMkdir) Delete(_ context.Context, req *cstructs.ClientHostVolumeDeleteRequest) error {
 	path := filepath.Join(p.VolumesDir, req.ID)
 	log := p.log.With(
@@ -134,7 +189,7 @@ func (p *HostVolumePluginMkdir) Delete(_ context.Context, req *cstructs.ClientHo
 
 	err := os.RemoveAll(path)
 	if err != nil {
-		log.Debug("error with plugin", "error", err)
+		log.Error("error with plugin", "error", err)
 		return err
 	}
 
@@ -203,17 +258,20 @@ func (p *HostVolumePluginExternal) Fingerprint(ctx context.Context) (*PluginFing
 	cmd := exec.CommandContext(ctx, p.Executable, "fingerprint")
 	cmd.Env = []string{EnvOperation + "=fingerprint"}
 	stdout, stderr, err := runCommand(cmd)
+	log := p.log.With(
+		"operation", "fingerprint",
+		"stdout", string(stdout),
+		"stderr", string(stderr),
+	)
 	if err != nil {
-		p.log.Debug("error with plugin",
-			"operation", "version",
-			"stdout", string(stdout),
-			"stderr", string(stderr),
-			"error", err)
-		return nil, fmt.Errorf("error getting version from plugin %q: %w", p.ID, err)
+		log.Error("error running plugin", "error", err)
+		return nil, fmt.Errorf("error fingerprinting plugin %q: %w", p.ID, err)
 	}
 	fprint := &PluginFingerprint{}
 	if err := json.Unmarshal(stdout, fprint); err != nil {
-		return nil, fmt.Errorf("error parsing fingerprint output as json: %w", err)
+		err = fmt.Errorf("error parsing fingerprint output as json: %w", err)
+		log.Error("error parsing plugin output", "error", err)
+		return nil, err
 	}
 	return fprint, nil
 }
@@ -364,7 +422,7 @@ func (p *HostVolumePluginExternal) runPlugin(ctx context.Context, log hclog.Logg
 		"stderr", string(stderr),
 	)
 	if err != nil {
-		log.Debug("error with plugin", "error", err)
+		log.Error("error with plugin", "error", err)
 		return stdout, stderr, err
 	}
 	log.Debug("plugin ran successfully")

client/hostvolumemanager/host_volume_plugin_test.go

@@ -4,20 +4,22 @@
 package hostvolumemanager
 
 import (
+	"os"
+	"os/user"
 	"path/filepath"
 	"runtime"
 	"testing"
 
 	"github.com/hashicorp/go-version"
+	"github.com/hashicorp/nomad/ci"
 	cstructs "github.com/hashicorp/nomad/client/structs"
 	"github.com/hashicorp/nomad/helper/testlog"
 	"github.com/shoenig/test/must"
 )
 
 func TestHostVolumePluginMkdir(t *testing.T) {
-	volID := "test-vol-id"
+	ci.Parallel(t)
 	tmp := t.TempDir()
-	target := filepath.Join(tmp, volID)
 
 	plug := &HostVolumePluginMkdir{
 		ID:         "test-mkdir-plugin",
@@ -31,6 +33,8 @@ func TestHostVolumePluginMkdir(t *testing.T) {
 	must.NoError(t, err)
 
 	t.Run("happy", func(t *testing.T) {
+		volID := "happy"
+		target := filepath.Join(tmp, volID)
 		// run multiple times, should be idempotent
 		for range 2 {
 			resp, err := plug.Create(timeout(t),
@@ -43,6 +47,7 @@ func TestHostVolumePluginMkdir(t *testing.T) {
 				SizeBytes: 0,
 			}, resp)
 			must.DirExists(t, target)
+			must.DirMode(t, target, 0o700+os.ModeDir)
 		}
 
 		// delete should be idempotent, too
@@ -57,25 +62,156 @@ func TestHostVolumePluginMkdir(t *testing.T) {
 	})
 
 	t.Run("sad", func(t *testing.T) {
+		volID := "sad"
 		// can't mkdir inside a file
 		plug.VolumesDir = "host_volume_plugin_test.go"
+		t.Cleanup(func() {
+			plug.VolumesDir = tmp
+		})
 
 		resp, err := plug.Create(timeout(t),
 			&cstructs.ClientHostVolumeCreateRequest{
 				ID: volID, // minimum required by this plugin
 			})
-		must.ErrorContains(t, err, "host_volume_plugin_test.go/test-vol-id: not a directory")
+		must.ErrorContains(t, err, "host_volume_plugin_test.go/sad: not a directory")
 		must.Nil(t, resp)
 
 		err = plug.Delete(timeout(t),
 			&cstructs.ClientHostVolumeDeleteRequest{
 				ID: volID,
 			})
-		must.ErrorContains(t, err, "host_volume_plugin_test.go/test-vol-id: not a directory")
+		must.ErrorContains(t, err, "host_volume_plugin_test.go/sad: not a directory")
+	})
+
+	t.Run("happy params", func(t *testing.T) {
+		volID := "happy_params"
+		target := filepath.Join(tmp, volID)
+		currentUser, err := user.Current()
+		must.NoError(t, err)
+		// run multiple times, should be idempotent
+		for range 2 {
+			_, err := plug.Create(timeout(t),
+				&cstructs.ClientHostVolumeCreateRequest{
+					ID: volID,
+					Parameters: map[string]string{
+						"uid":  currentUser.Uid,
+						"gid":  currentUser.Gid,
+						"mode": "0400",
+					},
+				})
+			must.NoError(t, err)
+			must.DirExists(t, target)
+			must.DirMode(t, target, 0o400+os.ModeDir)
+		}
+
+		err = plug.Delete(timeout(t),
+			&cstructs.ClientHostVolumeDeleteRequest{
+				ID: volID,
+			})
+		must.NoError(t, err)
+		must.DirNotExists(t, target)
+	})
+
+	t.Run("sad params", func(t *testing.T) {
+		volID := "sad_params"
+		// test one representative error; decodeMkdirParams has its own tests
+		resp, err := plug.Create(timeout(t),
+			&cstructs.ClientHostVolumeCreateRequest{
+				ID: volID,
+				Parameters: map[string]string{
+					"mode": "invalid",
+				},
+			})
+		must.ErrorContains(t, err, "invalid value")
+		must.Nil(t, resp)
 	})
 }
 
+func TestDecodeMkdirParams(t *testing.T) {
+	ci.Parallel(t)
+
+	cases := []struct {
+		name   string
+		params map[string]string
+		expect HostVolumePluginMkdirParams
+		err    string
+	}{
+		{
+			name:   "none ok",
+			params: nil,
+			expect: HostVolumePluginMkdirParams{
+				Uid:  -1,
+				Gid:  -1,
+				Mode: os.FileMode(0700),
+			},
+		},
+		{
+			name: "all ok",
+			params: map[string]string{
+				"uid":  "1",
+				"gid":  "2",
+				"mode": "0444",
+			},
+			expect: HostVolumePluginMkdirParams{
+				Uid:  1,
+				Gid:  2,
+				Mode: os.FileMode(0444),
+			},
+		},
+		{
+			name: "invalid mode: bad number",
+			params: map[string]string{
+				// this is what happens if you put mode=0700 instead of
+				// mode="0700" in the HCL spec.
+				"mode": "493",
+			},
+			err: `invalid value for "mode"`,
+		},
+		{
+			name: "invalid mode: string",
+			params: map[string]string{
+				"mode": "consider the lobster",
+			},
+			err: `invalid value for "mode"`,
+		},
+		{
+			name: "invalid uid",
+			params: map[string]string{
+				"uid": "a supposedly fun thing i'll never do again",
+			},
+			err: `invalid value for "uid"`,
+		},
+		{
+			name: "invalid gid",
+			params: map[string]string{
+				"gid": "surely you jest",
+			},
+			err: `invalid value for "gid"`,
+		},
+		{
+			name: "unknown param",
+			params: map[string]string{
+				"what": "the hell is water?",
+			},
+			err: `unknown mkdir parameter: "what"`,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := decodeMkdirParams(tc.params)
+			if tc.err != "" {
+				must.ErrorContains(t, err, tc.err)
+			} else {
+				must.NoError(t, err)
+				must.Eq(t, tc.expect, got)
+			}
+		})
+	}
+}
+
 func TestNewHostVolumePluginExternal(t *testing.T) {
+	ci.Parallel(t)
 	log := testlog.HCLogger(t)
 	var err error
 
@@ -104,6 +240,7 @@ func TestNewHostVolumePluginExternal(t *testing.T) {
 }
 
 func TestHostVolumePluginExternal(t *testing.T) {
+	ci.Parallel(t)
 	if runtime.GOOS == "windows" {
 		t.Skip("skipped because windows") // db TODO(1.10.0)
 	}
@@ -174,7 +311,7 @@ func TestHostVolumePluginExternal(t *testing.T) {
 		must.NoError(t, err)
 
 		v, err := plug.Fingerprint(timeout(t))
-		must.EqError(t, err, `error getting version from plugin "test_plugin_sad.sh": exit status 1`)
+		must.EqError(t, err, `error fingerprinting plugin "test_plugin_sad.sh": exit status 1`)
 		must.Nil(t, v)
 		logged := getLogs()
 		must.StrContains(t, logged, "fingerprint: sad plugin is sad")

client/hostvolumemanager/host_volumes.go

@@ -153,15 +153,20 @@ func (hvm *HostVolumeManager) Create(ctx context.Context,
 func (hvm *HostVolumeManager) Register(ctx context.Context,
 	req *cstructs.ClientHostVolumeRegisterRequest) error {
 
+	log := hvm.log.With("volume_name", req.Name, "volume_id", req.ID)
+
 	// can't have two of the same volume name w/ different IDs per client node
 	if _, err := hvm.locker.lock(req.Name, req.ID); err != nil {
+		log.Error("duplicate volume name", "error", err)
 		return err
 	}
 
 	_, err := os.Stat(req.HostPath)
 	if err != nil {
 		hvm.locker.release(req.Name)
-		return fmt.Errorf("could not verify host path for %q: %w", req.Name, err)
+		err = fmt.Errorf("could not verify host path for %q: %w", req.Name, err)
+		log.Error("error registering volume", "error", err)
+		return err
 	}
 
 	// generate a stub create request and plugin response for the fingerprint
@@ -178,7 +183,7 @@ func (hvm *HostVolumeManager) Register(ctx context.Context,
 		HostPath:  req.HostPath,
 	}
 	if err := hvm.stateMgr.PutDynamicHostVolume(volState); err != nil {
-		hvm.log.Error("failed to save volume in state", "volume_id", req.ID, "error", err)
+		log.Error("failed to save volume in state", "error", err)
 		hvm.locker.release(req.Name)
 		return err
 	}

command/volume_create_host_test.go

@@ -57,7 +57,7 @@ capability {
 }
 
 parameters {
-  foo = "bar"
+  mode = "0700"
 }
 `
 

nomad/mock/host_volumes.go

@@ -23,7 +23,7 @@ func HostVolumeRequest(ns string) *structs.HostVolume {
 		},
 		RequestedCapacityMinBytes: 100000,
 		RequestedCapacityMaxBytes: 200000,
-		Parameters:                map[string]string{"foo": "bar"},
+		Parameters:                map[string]string{"mode": "0700"},
 		State:                     structs.HostVolumeStatePending,
 	}
 	return vol

website/content/docs/concepts/plugins/storage/host-volumes.mdx

@@ -13,12 +13,12 @@ This page describes the plugin specification for
 your own plugin to dynamically configure persistent storage on your Nomad
 client nodes.
 
-Nomad has one built-in plugin called `mkdir`, which creates a directory on the
-host in the Nomad agent's [host_volumes_dir][]. The `mkdir` plugin does not
-support the `capacity_min` or `capacity_max` fields.
+If you do not need to write your own plugin, consider Nomad's built-in
+[`mkdir` plugin][mkdir_plugin], which creates a directory on the host.
 
-Review the examples first to get a sense of the specification in action.
-A plugin can be as basic as a short shell script.
+If you have more complex needs than that, review the examples here to get a
+sense of the specification in action. A plugin can be as basic as a short shell
+script.
 
 The full [specification](#specification) is after the examples,
 followed by a list of [general considerations](#considerations).
@@ -425,4 +425,5 @@ Plugin authors should consider these details when writing plugins.
 [api-create]: /nomad/api-docs/volumes#create-dynamic-host-volume
 [cli-delete]: /nomad/docs/commands/volume/delete
 [api-delete]: /nomad/api-docs/volumes#delete-dynamic-host-volume
+[mkdir_plugin]: /nomad/docs/other-specifications/volume/host#mkdir-plugin
 [host_volumes_dir]: /nomad/docs/configuration/client#host_volumes_dir

website/content/docs/other-specifications/volume/host.mdx

@@ -48,7 +48,7 @@ the API.
   behavior is up to the plugin. If you want to specify an exact size, set
   `capacity_min` and `capacity_max` to the same value. Accepts human-friendly
   suffixes such as `"100GiB"`. Plugins that cannot restrict the size of volumes,
-  such as the built-in `mkdir` plugin, may ignore this field.
+  such as the built-in [`mkdir`][mkdir_plugin] plugin, may ignore this field.
 
 - `capacity_max` `(string: <optional>)` - Option for requesting a maximum
   capacity, in bytes. The capacity of a volume may be the physical size of a
@@ -57,7 +57,7 @@ the API.
   behavior is up to the plugin. If you want to specify an exact size, set
   `capacity_min` and `capacity_max` to the same value. Accepts human-friendly
   suffixes such as `"100GiB"`. Plugins that cannot restrict the size of volumes,
-  such as the built-in `mkdir` plugin, may ignore this field.
+  such as the built-in [`mkdir`][mkdir_plugin] plugin, may ignore this field.
 
 - `capability` <code>([Capability][capability]: &lt;required&gt;)</code> -
   Option for validating the capability of a volume.
@@ -101,12 +101,50 @@ the API.
 
 - `plugin_id` `(string)` - The ID of the [dynamic host volume
   plugin][dhv_plugin] that manages this volume. Required for volume
-  creation. Nomad has one built-in plugin called `mkdir`, which creates a
-  directory on the host in the Nomad agent's [host_volumes_dir][].
+  creation. Nomad has one built-in plugin called [`mkdir`][mkdir_plugin].
 
 - `type` `(string: <required>)` - The type of volume. Must be `"host"` for
   dynamic host volumes.
 
+## mkdir plugin
+
+Nomad has one built-in plugin called `mkdir`, which creates a directory on the
+host in the Nomad agent's [host_volumes_dir][]. The directory name is the
+volume's ID.
+
+`mkdir` ignores `capacity_min` and `capacity_max` volume configuration,
+since it has no way of enforcing them.
+
+### mkdir parameters
+
+- `mode` `(string: <optional>)` - [Numeric
+notation](https://en.wikipedia.org/wiki/File-system_permissions#Numeric_notation)
+to apply to the created directory. Defaults to "0700", so only the owner can
+access it. Must be a string, or you may get unexpected results.
+- `uid` `(int: <optional>)` - User ID to own the directory. Defaults to the
+user running the Nomad client agent, which is commonly `0` (root).
+- `gid` `(int: <optional>)` - Group ID to own the directory. Defaults to the
+user running the Nomad client agent, which is commonly `0` (root).
+
+The user and group IDs must be present on any node that may receive the volume.
+
+### mkdir example
+
+<CodeBlockConfig filename="mkdir.volume.hcl">
+
+```hcl
+type      = "host"
+name      = "cool-host-vol"
+plugin_id = "mkdir"
+parameters = {
+  mode = "0755"
+  uid  = 1000
+  gid  = 1000
+}
+```
+
+</CodeBlockConfig>
+
 ## Differences Between Create and Register
 
 Several fields are set automatically by Nomad or the plugin when `volume create`
@@ -307,4 +345,5 @@ registered.
 [`volume register`]: /nomad/docs/commands/volume/register
 [`volume status`]: /nomad/docs/commands/volume/status
 [dhv_plugin]: /nomad/docs/concepts/plugins/storage/host-volumes
+[mkdir_plugin]: #mkdir-plugin
 [host_volumes_dir]: /nomad/docs/configuration/client#host_volumes_dir