dynamic host volumes: tweak plugin fingerprint (#24711)

Instead of a plugin `version` subcommand that responds with a string
(established in #24497), respond to a `fingerprint` command with a data
structure that we may extend in the future (such as plugin capabilities,
like size constraint support?). In the immediate term, it's still just the
version: `{"version": "0.0.1"}`

In addition to leaving the door open for future expansion, I think it will
also avoid false positives detecting executables that just happen to
respond to a `version` command.

This also reverses the ordering of the fingerprint string parts
from `plugins.host_volume.version.mkdir` (which aligned with CNI)
to `plugins.host_volume.mkdir.version` (makes more sense to me)

client/fingerprint/dynamic_host_volumes.go

@@ -35,7 +35,7 @@ func (h *DynamicHostVolumePluginFingerprint) Fingerprint(request *FingerprintReq
 	// always add "mkdir" plugin
 	h.logger.Debug("detected plugin built-in",
 		"plugin_id", hvm.HostVolumePluginMkdirID, "version", hvm.HostVolumePluginMkdirVersion)
-	defer response.AddAttribute("plugins.host_volume.version."+hvm.HostVolumePluginMkdirID, hvm.HostVolumePluginMkdirVersion)
+	defer response.AddAttribute("plugins.host_volume."+hvm.HostVolumePluginMkdirID+".version", hvm.HostVolumePluginMkdirVersion)
 	response.Detected = true
 
 	// this config value will be empty in -dev mode
@@ -64,7 +64,7 @@ func (h *DynamicHostVolumePluginFingerprint) Fingerprint(request *FingerprintReq
 	// set the attribute(s)
 	for plugin, version := range plugins {
 		h.logger.Debug("detected plugin", "plugin_id", plugin, "version", version)
-		response.AddAttribute("plugins.host_volume.version."+plugin, version)
+		response.AddAttribute("plugins.host_volume."+plugin+".version", version)
 	}
 
 	return nil
@@ -103,14 +103,14 @@ func GetHostVolumePluginVersions(log hclog.Logger, pluginDir string) (map[string
 				return
 			}
 
-			version, err := p.Version(ctx)
+			fprint, err := p.Fingerprint(ctx)
 			if err != nil {
 				log.Debug("failed to get version from plugin", "error", err)
 				return
 			}
 
 			mut.Lock()
-			plugins[file] = version.String()
+			plugins[file] = fprint.Version.String()
 			mut.Unlock()
 		}(file, fullPath)
 	}

client/fingerprint/dynamic_host_volumes_test.go

@@ -53,10 +53,10 @@ func TestPluginsHostVolumeFingerprint(t *testing.T) {
 		perm     os.FileMode
 	}{
 		// only this first one should be detected as a valid plugin
-		{"happy-plugin", "#!/usr/bin/env sh\necho '0.0.1'", 0700},
-		{"not-a-plugin", "#!/usr/bin/env sh\necho 'not-a-version'", 0700},
-		{"unhappy-plugin", "#!/usr/bin/env sh\necho '0.0.2'; exit 1", 0700},
-		{"not-executable", "hello", 0400},
+		{"happy-plugin", "#!/usr/bin/env sh\necho '{\"version\": \"0.0.1\"}'", 0700},
+		{"not-a-plugin", "#!/usr/bin/env sh\necho 'not a version'", 0700},
+		{"unhappy-plugin", "#!/usr/bin/env sh\necho 'sad plugin is sad'; exit 1", 0700},
+		{"not-executable", "do not execute me", 0400},
 	}
 	for _, f := range files {
 		must.NoError(t, os.WriteFile(filepath.Join(tmp, f.name), []byte(f.contents), f.perm))
@@ -69,8 +69,8 @@ func TestPluginsHostVolumeFingerprint(t *testing.T) {
 	err := fp.Fingerprint(req, &resp)
 	must.NoError(t, err)
 	must.Eq(t, map[string]string{
-		"plugins.host_volume.version.happy-plugin": "0.0.1",
-		"plugins.host_volume.version.mkdir":        hvm.HostVolumePluginMkdirVersion, // built-in
+		"plugins.host_volume.mkdir.version":        hvm.HostVolumePluginMkdirVersion, // built-in
+		"plugins.host_volume.happy-plugin.version": "0.0.1",
 	}, resp.Attributes)
 
 	// do it again after deleting our one good plugin.
@@ -82,8 +82,8 @@ func TestPluginsHostVolumeFingerprint(t *testing.T) {
 	err = fp.Fingerprint(req, &resp)
 	must.NoError(t, err)
 	must.Eq(t, map[string]string{
-		"plugins.host_volume.version.happy-plugin": "", // empty value means removed
+		"plugins.host_volume.happy-plugin.version": "", // empty value means removed
 
-		"plugins.host_volume.version.mkdir": hvm.HostVolumePluginMkdirVersion, // built-in
+		"plugins.host_volume.mkdir.version": hvm.HostVolumePluginMkdirVersion, // built-in
 	}, resp.Attributes)
 }

client/hostvolumemanager/host_volume_plugin.go

@@ -12,7 +12,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"strings"
 
 	"github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-multierror"
@@ -21,8 +20,12 @@ import (
 	"github.com/hashicorp/nomad/helper"
 )
 
+type PluginFingerprint struct {
+	Version *version.Version `json:"version"`
+}
+
 type HostVolumePlugin interface {
-	Version(ctx context.Context) (*version.Version, error)
+	Fingerprint(ctx context.Context) (*PluginFingerprint, 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? ??
@@ -45,8 +48,11 @@ type HostVolumePluginMkdir struct {
 	log hclog.Logger
 }
 
-func (p *HostVolumePluginMkdir) Version(_ context.Context) (*version.Version, error) {
-	return version.NewVersion(HostVolumePluginMkdirVersion)
+func (p *HostVolumePluginMkdir) Fingerprint(_ context.Context) (*PluginFingerprint, error) {
+	v, err := version.NewVersion(HostVolumePluginMkdirVersion)
+	return &PluginFingerprint{
+		Version: v,
+	}, err
 }
 
 func (p *HostVolumePluginMkdir) Create(_ context.Context,
@@ -134,9 +140,9 @@ type HostVolumePluginExternal struct {
 	log hclog.Logger
 }
 
-func (p *HostVolumePluginExternal) Version(ctx context.Context) (*version.Version, error) {
-	cmd := exec.CommandContext(ctx, p.Executable, "version")
-	cmd.Env = []string{"OPERATION=version"}
+func (p *HostVolumePluginExternal) Fingerprint(ctx context.Context) (*PluginFingerprint, error) {
+	cmd := exec.CommandContext(ctx, p.Executable, "fingerprint")
+	cmd.Env = []string{"OPERATION=fingerprint"}
 	stdout, stderr, err := runCommand(cmd)
 	if err != nil {
 		p.log.Debug("error with plugin",
@@ -146,11 +152,11 @@ func (p *HostVolumePluginExternal) Version(ctx context.Context) (*version.Versio
 			"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)
+	fprint := &PluginFingerprint{}
+	if err := json.Unmarshal(stdout, fprint); err != nil {
+		return nil, fmt.Errorf("error parsing fingerprint output as json: %w", err)
 	}
-	return v, nil
+	return fprint, nil
 }
 
 func (p *HostVolumePluginExternal) Create(ctx context.Context,

client/hostvolumemanager/host_volume_plugin_test.go

@@ -33,7 +33,7 @@ func TestHostVolumePluginMkdir(t *testing.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))
+	_, err := plug.Fingerprint(timeout(t))
 	must.NoError(t, err)
 
 	t.Run("happy", func(t *testing.T) {
@@ -97,9 +97,9 @@ func TestHostVolumePluginExternal(t *testing.T) {
 			log:        log,
 		}
 
-		v, err := plug.Version(timeout(t))
+		v, err := plug.Fingerprint(timeout(t))
 		must.NoError(t, err)
-		must.Eq(t, expectVersion, v)
+		must.Eq(t, expectVersion, v.Version)
 
 		resp, err := plug.Create(timeout(t),
 			&cstructs.ClientHostVolumeCreateRequest{
@@ -147,12 +147,12 @@ func TestHostVolumePluginExternal(t *testing.T) {
 			log:        log,
 		}
 
-		v, err := plug.Version(timeout(t))
+		v, err := plug.Fingerprint(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")
+		must.StrContains(t, logged, "fingerprint: sad plugin is sad")
+		must.StrContains(t, logged, "fingerprint: it tells you all about it in stderr")
 
 		// reset logger
 		log, getLogs = logRecorder(t)

client/hostvolumemanager/test_fixtures/test_plugin.sh

@@ -12,6 +12,8 @@ test "$1" == "$OPERATION"
 echo 'all operations should ignore stderr' 1>&2
 
 case $1 in
+  fingerprint)
+    echo '{"version": "0.0.2"}' ;;
   create)
     test "$2" == "$HOST_PATH"
     test "$NODE_ID" == 'test-node'
@@ -26,8 +28,6 @@ case $1 in
     test "$NODE_ID" == 'test-node'
     test "$PARAMETERS" == '{"key":"val"}'
     rm -rfv "$2" ;;
-  version)
-    echo '0.0.2' ;;
   *)
     echo "unknown operation $1"
     exit 1 ;;

demo/hostvolume/_test-plugin.sh

@@ -4,35 +4,48 @@
 
 set -euo pipefail
 
-if [[ $# -eq 0 || "$*" =~ -h ]]; then
+help() {
   cat <<EOF
 Runs a Dynamic Host Volume plugin with dummy values.
 
 Usage:
-  $(basename "$0") <operation>
+  $0 <plugin> <operation> [target dir] [uuid]
 
-Operations:
-  create, delete, version
-  any other operation will be passed to the plugin
+Args:
+  plugin: path to plugin executable
+  operation: fingerprint, create, or delete
+    create and delete must be idempotent.
+    any other operation will be passed into the plugin,
+    to see how it handles invalid operations.
+  target dir: directory to create the volume (defaults to /tmp)
+  uuid: volume id to use (usually assigned by Nomad;
+    defaults to 74564d17-ce50-0bc1-48e5-6feaa41ede48)
 
-Environment variables:
-  PLUGIN: executable to run (default ./example-host-volume)
-  TARGET_DIR: path to place the mount dir (default /tmp,
-    usually {nomad data dir}/alloc_mounts)
+Examples:
+  $0 ./example-plugin-mkfs fingerprint
+  $0 ./example-plugin-mkfs create
+  $0 ./example-plugin-mkfs create /some/other/place
+  $0 ./example-plugin-mkfs delete
 EOF
+}
+
+if [[ $# -eq 0 || "$*" =~ -h ]]; then
+  help
   exit
 fi
+if [ $# -lt 2 ]; then
+  help
+  exit 1
+fi
 
-op="$1"
-shift
-
-plugin="${PLUGIN:-./example-host-volume}"
-alloc_mounts="${TARGET_DIR:-/tmp}"
-uuid='74564d17-ce50-0bc1-48e5-6feaa41ede48'
+plugin="$1"
+op="$2"
+alloc_mounts="${3:-/tmp}"
+uuid="${4:-74564d17-ce50-0bc1-48e5-6feaa41ede48}"
 
 case $op in
-  version)
-    args='version'
+  fingerprint)
+    args='fingerprint'
     ;;
 
 	create)
@@ -59,4 +72,4 @@ esac
 
 export OPERATION="$op"
 set -x
-eval "$plugin $* $args"
+eval "$plugin $args"

demo/hostvolume/example-plugin-mkfs

@@ -6,6 +6,11 @@
 
 set -euo pipefail
 
+version='0.0.1'
+fingerprint() {
+  printf '{"version": "%s"}' "$version"
+}
+
 help() {
   cat <<EOF
 Dynamic Host Volume plugin which creates an ext4 loopback drive of
@@ -14,7 +19,7 @@ the minimum requested size, and mounts it at the provided path argument.
 Note: Requires superuser access to mount.
 
 Usage:
-  $(basename "$0") [options] <create|delete|version> [path]
+  $(basename "$0") [options] <create|delete|fingerprint> [path]
 
 Options:
   -v|--verbose: Show shell commands (set -x)
@@ -25,21 +30,19 @@ Operations:
     required environment:
       CAPACITY_MIN_BYTES
   delete: Unmounts and deletes the device at path (required)
-  version: Outputs this plugin's version
+  version: Outputs this plugin's version: $version
+  fingerprint: Outputs plugin metadata: $(fingerprint)
 
 EOF
 }
 
-version() {
-  echo "0.0.1"
-}
-
 # parse args
 [ $# -eq 0 ] && { help; exit 1; }
 for arg in "$@"; do
   case $arg in
     -h|-help|--help) help; exit 0 ;;
-    version|--version) version; exit 0 ;;
+    fingerprint|fingerprint) fingerprint; exit 0 ;;
+    version|version) echo "$version"; exit 0 ;;
     -v|--verbose) set -x; shift; ;;
   esac
 done

nomad/host_volume_endpoint.go

@@ -485,7 +485,7 @@ func (v *HostVolume) placeHostVolume(snap *state.StateSnapshot, vol *structs.Hos
 		semverCache:  make(map[string]scheduler.VerConstraints),
 	}
 	constraints := []*structs.Constraint{{
-		LTarget: fmt.Sprintf("${attr.plugins.host_volume.version.%s}", vol.PluginID),
+		LTarget: fmt.Sprintf("${attr.plugins.host_volume.%s.version}", vol.PluginID),
 		Operand: "is_set",
 	}}
 	constraints = append(constraints, vol.Constraints...)

nomad/host_volume_endpoint_test.go

@@ -666,22 +666,22 @@ func TestHostVolumeEndpoint_placeVolume(t *testing.T) {
 
 	node0, node1, node2, node3 := mock.Node(), mock.Node(), mock.Node(), mock.Node()
 	node0.NodePool = structs.NodePoolDefault
-	node0.Attributes["plugins.host_volume.version.mkdir"] = "0.0.1"
+	node0.Attributes["plugins.host_volume.mkdir.version"] = "0.0.1"
 
 	node1.NodePool = "dev"
 	node1.Meta["rack"] = "r2"
-	node1.Attributes["plugins.host_volume.version.mkdir"] = "0.0.1"
+	node1.Attributes["plugins.host_volume.mkdir.version"] = "0.0.1"
 
 	node2.NodePool = "prod"
-	node2.Attributes["plugins.host_volume.version.mkdir"] = "0.0.1"
+	node2.Attributes["plugins.host_volume.mkdir.version"] = "0.0.1"
 
 	node3.NodePool = "prod"
 	node3.Meta["rack"] = "r3"
 	node3.HostVolumes = map[string]*structs.ClientHostVolumeConfig{"example": {
 		Name: "example",
 		Path: "/srv",
 	}}
-	node3.Attributes["plugins.host_volume.version.mkdir"] = "0.0.1"
+	node3.Attributes["plugins.host_volume.mkdir.version"] = "0.0.1"
 
 	must.NoError(t, store.UpsertNode(structs.MsgTypeTestSetup, 1000, node0))
 	must.NoError(t, store.UpsertNode(structs.MsgTypeTestSetup, 1000, node1))
@@ -785,7 +785,7 @@ func newMockHostVolumeClient(t *testing.T, srv *Server, pool string) (*mockHostV
 	c1, cleanup := client.TestRPCOnlyClient(t, func(c *config.Config) {
 		c.Node.NodePool = pool
 		c.Node.Attributes["nomad.version"] = version.Version
-		c.Node.Attributes["plugins.host_volume.version.mkdir"] = "0.0.1"
+		c.Node.Attributes["plugins.host_volume.mkdir.version"] = "0.0.1"
 		c.Node.Meta["rack"] = "r1"
 	}, srv.config.RPCAddr, map[string]any{"HostVolume": mockClientEndpoint})
 	t.Cleanup(cleanup)