dynamic host volumes: test client RPC and plugins (#24535)

also ensure that volume ID is uuid-shaped so user-provided input
like `id = "../../../"` which is used as part of the target directory
can not find its way very far into the volume submission process

client/host_volume_endpoint_test.go

@@ -0,0 +1,94 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package client
+
+import (
+	"path/filepath"
+	"testing"
+
+	"github.com/hashicorp/nomad/ci"
+	"github.com/hashicorp/nomad/client/hostvolumemanager"
+	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/helper/testlog"
+	"github.com/shoenig/test/must"
+)
+
+func TestHostVolume(t *testing.T) {
+	ci.Parallel(t)
+
+	client, cleanup := TestClient(t, nil)
+	defer cleanup()
+
+	tmp := t.TempDir()
+	expectDir := filepath.Join(tmp, "test-vol-id")
+	hvm := hostvolumemanager.NewHostVolumeManager(tmp, testlog.HCLogger(t))
+	client.hostVolumeManager = hvm
+
+	t.Run("happy", func(t *testing.T) {
+		req := &cstructs.ClientHostVolumeCreateRequest{
+			ID:       "test-vol-id",
+			Name:     "test-vol-name",
+			PluginID: "mkdir", // real plugin really makes a dir
+		}
+		var resp cstructs.ClientHostVolumeCreateResponse
+		err := client.ClientRPC("HostVolume.Create", req, &resp)
+		must.NoError(t, err)
+		must.Eq(t, cstructs.ClientHostVolumeCreateResponse{
+			HostPath:      expectDir,
+			CapacityBytes: 0, // "mkdir" always returns zero
+		}, resp)
+		// technically this is testing "mkdir" more than the RPC
+		must.DirExists(t, expectDir)
+
+		delReq := &cstructs.ClientHostVolumeDeleteRequest{
+			ID:       "test-vol-id",
+			PluginID: "mkdir",
+			HostPath: expectDir,
+		}
+		var delResp cstructs.ClientHostVolumeDeleteResponse
+		err = client.ClientRPC("HostVolume.Delete", delReq, &delResp)
+		must.NoError(t, err)
+		must.NotNil(t, delResp)
+		// again, actually testing the "mkdir" plugin
+		must.DirNotExists(t, expectDir)
+	})
+
+	t.Run("missing plugin", func(t *testing.T) {
+		req := &cstructs.ClientHostVolumeCreateRequest{
+			PluginID: "non-existent",
+		}
+		var resp cstructs.ClientHostVolumeCreateResponse
+		err := client.ClientRPC("HostVolume.Create", req, &resp)
+		must.EqError(t, err, `no such plugin "non-existent"`)
+
+		delReq := &cstructs.ClientHostVolumeDeleteRequest{
+			PluginID: "non-existent",
+		}
+		var delResp cstructs.ClientHostVolumeDeleteResponse
+		err = client.ClientRPC("HostVolume.Delete", delReq, &delResp)
+		must.EqError(t, err, `no such plugin "non-existent"`)
+	})
+
+	t.Run("error from plugin", func(t *testing.T) {
+		// "mkdir" plugin can't create a directory within a file
+		client.hostVolumeManager = hostvolumemanager.NewHostVolumeManager("host_volume_endpoint_test.go", testlog.HCLogger(t))
+
+		req := &cstructs.ClientHostVolumeCreateRequest{
+			ID:       "test-vol-id",
+			Name:     "test-vol-name",
+			PluginID: "mkdir",
+		}
+		var resp cstructs.ClientHostVolumeCreateResponse
+		err := client.ClientRPC("HostVolume.Create", req, &resp)
+		must.ErrorContains(t, err, "host_volume_endpoint_test.go/test-vol-id: not a directory")
+
+		delReq := &cstructs.ClientHostVolumeDeleteRequest{
+			ID:       "test-vol-id",
+			PluginID: "mkdir",
+		}
+		var delResp cstructs.ClientHostVolumeDeleteResponse
+		err = client.ClientRPC("HostVolume.Delete", delReq, &delResp)
+		must.ErrorContains(t, err, "host_volume_endpoint_test.go/test-vol-id: not a directory")
+	})
+}

client/hostvolumemanager/host_volume_plugin.go

@@ -12,15 +12,17 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 
 	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-multierror"
+	"github.com/hashicorp/go-version"
 	cstructs "github.com/hashicorp/nomad/client/structs"
 	"github.com/hashicorp/nomad/helper"
 )
 
 type HostVolumePlugin interface {
-	Version(ctx context.Context) (string, error)
+	Version(ctx context.Context) (*version.Version, error)
 	Create(ctx context.Context, req *cstructs.ClientHostVolumeCreateRequest) (*HostVolumePluginCreateResponse, error)
 	Delete(ctx context.Context, req *cstructs.ClientHostVolumeDeleteRequest) error
 	// db TODO(1.10.0): update? resize? ??
@@ -41,8 +43,8 @@ type HostVolumePluginMkdir struct {
 	log hclog.Logger
 }
 
-func (p *HostVolumePluginMkdir) Version(_ context.Context) (string, error) {
-	return "0.0.1", nil
+func (p *HostVolumePluginMkdir) Version(_ context.Context) (*version.Version, error) {
+	return version.NewVersion("0.0.1")
 }
 
 func (p *HostVolumePluginMkdir) Create(_ context.Context,
@@ -97,8 +99,23 @@ type HostVolumePluginExternal struct {
 	log hclog.Logger
 }
 
-func (p *HostVolumePluginExternal) Version(_ context.Context) (string, error) {
-	return "0.0.1", nil // db TODO(1.10.0): call the plugin, use in fingerprint
+func (p *HostVolumePluginExternal) Version(ctx context.Context) (*version.Version, error) {
+	cmd := exec.CommandContext(ctx, p.Executable, "version")
+	cmd.Env = []string{"OPERATION=version"}
+	stdout, stderr, err := runCommand(cmd)
+	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)
+	}
+	v, err := version.NewVersion(strings.TrimSpace(string(stdout)))
+	if err != nil {
+		return nil, fmt.Errorf("error with version from plugin: %w", err)
+	}
+	return v, nil
 }
 
 func (p *HostVolumePluginExternal) Create(ctx context.Context,
@@ -118,7 +135,7 @@ func (p *HostVolumePluginExternal) Create(ctx context.Context,
 
 	stdout, _, err := p.runPlugin(ctx, "create", req.ID, envVars)
 	if err != nil {
-		return nil, fmt.Errorf("error creating volume %q with plugin %q: %w", req.ID, req.PluginID, err)
+		return nil, fmt.Errorf("error creating volume %q with plugin %q: %w", req.ID, p.ID, err)
 	}
 
 	var pluginResp HostVolumePluginCreateResponse
@@ -143,7 +160,7 @@ func (p *HostVolumePluginExternal) Delete(ctx context.Context,
 
 	_, _, err = p.runPlugin(ctx, "delete", req.ID, envVars)
 	if err != nil {
-		return fmt.Errorf("error deleting volume %q with plugin %q: %w", req.ID, req.PluginID, err)
+		return fmt.Errorf("error deleting volume %q with plugin %q: %w", req.ID, p.ID, err)
 	}
 	return nil
 }
@@ -166,10 +183,23 @@ func (p *HostVolumePluginExternal) runPlugin(ctx context.Context,
 		"HOST_PATH=" + path,
 	}, env...)
 
+	stdout, stderr, err = runCommand(cmd)
+
+	log = log.With(
+		"stdout", string(stdout),
+		"stderr", string(stderr),
+	)
+	if err != nil {
+		log.Debug("error with plugin", "error", err)
+		return stdout, stderr, err
+	}
+	log.Debug("plugin ran successfully")
+	return stdout, stderr, nil
+}
+
+func runCommand(cmd *exec.Cmd) (stdout, stderr []byte, err error) {
 	var errBuf bytes.Buffer
 	cmd.Stderr = io.Writer(&errBuf)
-
-	// run the command and capture output
 	mErr := &multierror.Error{}
 	stdout, err = cmd.Output()
 	if err != nil {
@@ -179,16 +209,5 @@ func (p *HostVolumePluginExternal) runPlugin(ctx context.Context,
 	if err != nil {
 		mErr = multierror.Append(mErr, err)
 	}
-
-	log = log.With(
-		"stdout", string(stdout),
-		"stderr", string(stderr),
-	)
-	if mErr.ErrorOrNil() != nil {
-		err = helper.FlattenMultierror(mErr)
-		log.Debug("error with plugin", "error", err)
-		return stdout, stderr, err
-	}
-	log.Debug("plugin ran successfully")
-	return stdout, stderr, nil
+	return stdout, stderr, helper.FlattenMultierror(mErr.ErrorOrNil())
 }

client/hostvolumemanager/host_volume_plugin_test.go

@@ -0,0 +1,217 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: BUSL-1.1
+
+package hostvolumemanager
+
+import (
+	"bytes"
+	"context"
+	"io"
+	"path/filepath"
+	"runtime"
+	"testing"
+	"time"
+
+	"github.com/hashicorp/go-hclog"
+	"github.com/hashicorp/go-version"
+	cstructs "github.com/hashicorp/nomad/client/structs"
+	"github.com/hashicorp/nomad/helper/testlog"
+	"github.com/shoenig/test"
+	"github.com/shoenig/test/must"
+)
+
+func TestHostVolumePluginMkdir(t *testing.T) {
+	volID := "test-vol-id"
+	tmp := t.TempDir()
+	target := filepath.Join(tmp, volID)
+
+	plug := &HostVolumePluginMkdir{
+		ID:         "test-mkdir-plugin",
+		TargetPath: tmp,
+		log:        testlog.HCLogger(t),
+	}
+
+	// contexts don't matter here, since they're thrown away by this plugin,
+	// but sending timeout contexts anyway, in case the plugin changes later.
+	_, err := plug.Version(timeout(t))
+	must.NoError(t, err)
+
+	t.Run("happy", func(t *testing.T) {
+		resp, err := plug.Create(timeout(t),
+			&cstructs.ClientHostVolumeCreateRequest{
+				ID: volID, // minimum required by this plugin
+			})
+		must.NoError(t, err)
+		must.Eq(t, &HostVolumePluginCreateResponse{
+			Path:      target,
+			SizeBytes: 0,
+			Context:   map[string]string{},
+		}, resp)
+		must.DirExists(t, target)
+
+		err = plug.Delete(timeout(t),
+			&cstructs.ClientHostVolumeDeleteRequest{
+				ID: volID,
+			})
+		must.NoError(t, err)
+		must.DirNotExists(t, target)
+	})
+
+	t.Run("sad", func(t *testing.T) {
+		// can't mkdir inside a file
+		plug.TargetPath = "host_volume_plugin_test.go"
+
+		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.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")
+	})
+}
+
+func TestHostVolumePluginExternal(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("skipped because windows") // db TODO(1.10.0)
+	}
+
+	volID := "test-vol-id"
+	tmp := t.TempDir()
+	target := filepath.Join(tmp, volID)
+
+	expectVersion, err := version.NewVersion("0.0.2")
+	must.NoError(t, err)
+
+	t.Run("happy", func(t *testing.T) {
+
+		log, getLogs := logRecorder(t)
+		plug := &HostVolumePluginExternal{
+			ID:         "test-external-plugin",
+			Executable: "./test_fixtures/test_plugin.sh",
+			TargetPath: tmp,
+			log:        log,
+		}
+
+		v, err := plug.Version(timeout(t))
+		must.NoError(t, err)
+		must.Eq(t, expectVersion, v)
+
+		resp, err := plug.Create(timeout(t),
+			&cstructs.ClientHostVolumeCreateRequest{
+				ID:                        volID,
+				NodeID:                    "test-node",
+				RequestedCapacityMinBytes: 5,
+				RequestedCapacityMaxBytes: 10,
+				Parameters:                map[string]string{"key": "val"},
+			})
+		must.NoError(t, err)
+
+		must.Eq(t, &HostVolumePluginCreateResponse{
+			Path:      target,
+			SizeBytes: 5,
+			Context:   map[string]string{"key": "val"},
+		}, resp)
+		must.DirExists(t, target)
+		logged := getLogs()
+		must.StrContains(t, logged, "OPERATION=create") // stderr from `env`
+		must.StrContains(t, logged, `stdout="{`)        // stdout from printf
+
+		// reset logger for next call
+		log, getLogs = logRecorder(t)
+		plug.log = log
+
+		err = plug.Delete(timeout(t),
+			&cstructs.ClientHostVolumeDeleteRequest{
+				ID:         volID,
+				NodeID:     "test-node",
+				Parameters: map[string]string{"key": "val"},
+			})
+		must.NoError(t, err)
+		must.DirNotExists(t, target)
+		logged = getLogs()
+		must.StrContains(t, logged, "OPERATION=delete")  // stderr from `env`
+		must.StrContains(t, logged, "removed directory") // stdout from `rm -v`
+	})
+
+	t.Run("sad", func(t *testing.T) {
+
+		log, getLogs := logRecorder(t)
+		plug := &HostVolumePluginExternal{
+			ID:         "test-external-plugin-sad",
+			Executable: "./test_fixtures/test_plugin_sad.sh",
+			TargetPath: tmp,
+			log:        log,
+		}
+
+		v, err := plug.Version(timeout(t))
+		must.EqError(t, err, `error getting version from plugin "test-external-plugin-sad": exit status 1`)
+		must.Nil(t, v)
+		logged := getLogs()
+		must.StrContains(t, logged, "version: sad plugin is sad")
+		must.StrContains(t, logged, "version: it tells you all about it in stderr")
+
+		// reset logger
+		log, getLogs = logRecorder(t)
+		plug.log = log
+
+		resp, err := plug.Create(timeout(t),
+			&cstructs.ClientHostVolumeCreateRequest{
+				ID:                        volID,
+				NodeID:                    "test-node",
+				RequestedCapacityMinBytes: 5,
+				RequestedCapacityMaxBytes: 10,
+				Parameters:                map[string]string{"key": "val"},
+			})
+		must.EqError(t, err, `error creating volume "test-vol-id" with plugin "test-external-plugin-sad": exit status 1`)
+		must.Nil(t, resp)
+		logged = getLogs()
+		must.StrContains(t, logged, "create: sad plugin is sad")
+		must.StrContains(t, logged, "create: it tells you all about it in stderr")
+
+		log, getLogs = logRecorder(t)
+		plug.log = log
+
+		err = plug.Delete(timeout(t),
+			&cstructs.ClientHostVolumeDeleteRequest{
+				ID:         volID,
+				NodeID:     "test-node",
+				Parameters: map[string]string{"key": "val"},
+			})
+		must.EqError(t, err, `error deleting volume "test-vol-id" with plugin "test-external-plugin-sad": exit status 1`)
+		logged = getLogs()
+		must.StrContains(t, logged, "delete: sad plugin is sad")
+		must.StrContains(t, logged, "delete: it tells you all about it in stderr")
+	})
+}
+
+// timeout provides a context that times out in 1 second
+func timeout(t *testing.T) context.Context {
+	t.Helper()
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	t.Cleanup(cancel)
+	return ctx
+}
+
+// logRecorder is here so we can assert that stdout/stderr appear in logs
+func logRecorder(t *testing.T) (hclog.Logger, func() string) {
+	t.Helper()
+	buf := &bytes.Buffer{}
+	logger := hclog.New(&hclog.LoggerOptions{
+		Name:            "log-recorder",
+		Output:          buf,
+		Level:           hclog.Debug,
+		IncludeLocation: true,
+		DisableTime:     true,
+	})
+	return logger, func() string {
+		bts, err := io.ReadAll(buf)
+		test.NoError(t, err)
+		return string(bts)
+	}
+}

client/hostvolumemanager/test_fixtures/test_plugin.sh

@@ -0,0 +1,34 @@
+#!/usr/bin/env bash
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: BUSL-1.1
+
+# plugin for host_volume_plugin_test.go
+set -xeuo pipefail
+
+env 1>&2
+
+test "$1" == "$OPERATION"
+
+echo 'all operations should ignore stderr' 1>&2
+
+case $1 in
+  create)
+    test "$2" == "$HOST_PATH"
+    test "$NODE_ID" == 'test-node'
+    test "$PARAMETERS" == '{"key":"val"}'
+    test "$CAPACITY_MIN_BYTES" -eq 5
+    test "$CAPACITY_MAX_BYTES" -eq 10
+    mkdir "$2"
+    printf '{"path": "%s", "bytes": 5, "context": %s}' "$2" "$PARAMETERS"
+    ;;
+  delete)
+    test "$2" == "$HOST_PATH"
+    test "$NODE_ID" == 'test-node'
+    test "$PARAMETERS" == '{"key":"val"}'
+    rm -rfv "$2" ;;
+  version)
+    echo '0.0.2' ;;
+  *)
+    echo "unknown operation $1"
+    exit 1 ;;
+esac

client/hostvolumemanager/test_fixtures/test_plugin_sad.sh

@@ -0,0 +1,7 @@
+#!/usr/bin/env bash
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: BUSL-1.1
+
+echo "$1: sad plugin is sad"
+echo "$1: it tells you all about it in stderr" 1>&2
+exit 1

nomad/structs/host_volumes.go

@@ -134,6 +134,10 @@ func (hv *HostVolume) Validate() error {
 
 	var mErr *multierror.Error
 
+	if hv.ID != "" && !helper.IsUUID(hv.ID) {
+		mErr = multierror.Append(mErr, errors.New("invalid ID"))
+	}
+
 	if hv.Name == "" {
 		mErr = multierror.Append(mErr, errors.New("missing name"))
 	}
@@ -167,7 +171,7 @@ func (hv *HostVolume) Validate() error {
 		}
 	}
 
-	return mErr.ErrorOrNil()
+	return helper.FlattenMultierror(mErr.ErrorOrNil())
 }
 
 // ValidateUpdate verifies that an update to a volume is safe to make.

nomad/structs/host_volumes_test.go

@@ -66,7 +66,13 @@ func TestHostVolume_Validate(t *testing.T) {
 
 `)
 
+	invalid = &HostVolume{Name: "example"}
+	err = invalid.Validate()
+	// single error should be flattened
+	must.EqError(t, err, "must include at least one capability block")
+
 	invalid = &HostVolume{
+		ID:       "../../not-a-uuid",
 		Name:     "example",
 		PluginID: "example-plugin",
 		Constraints: []*Constraint{{
@@ -87,7 +93,8 @@ func TestHostVolume_Validate(t *testing.T) {
 		},
 	}
 	err = invalid.Validate()
-	must.EqError(t, err, `3 errors occurred:
+	must.EqError(t, err, `4 errors occurred:
+	* invalid ID
 	* capacity_max (100000) must be larger than capacity_min (200000)
 	* invalid attachment mode: "bad"
 	* invalid constraint: 1 error occurred: