From 2b04d47ac2e3f1d6911eddbb9fb9dfffab3f1eef Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Fri, 22 Nov 2024 12:22:50 -0500 Subject: [PATCH] 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 | 94 ++++++++ .../hostvolumemanager/host_volume_plugin.go | 61 +++-- .../host_volume_plugin_test.go | 217 ++++++++++++++++++ .../test_fixtures/test_plugin.sh | 34 +++ .../test_fixtures/test_plugin_sad.sh | 7 + nomad/structs/host_volumes.go | 6 +- nomad/structs/host_volumes_test.go | 9 +- 7 files changed, 405 insertions(+), 23 deletions(-) create mode 100644 client/host_volume_endpoint_test.go create mode 100644 client/hostvolumemanager/host_volume_plugin_test.go create mode 100755 client/hostvolumemanager/test_fixtures/test_plugin.sh create mode 100755 client/hostvolumemanager/test_fixtures/test_plugin_sad.sh diff --git a/client/host_volume_endpoint_test.go b/client/host_volume_endpoint_test.go new file mode 100644 index 000000000..c3a4ae838 --- /dev/null +++ b/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") + }) +} diff --git a/client/hostvolumemanager/host_volume_plugin.go b/client/hostvolumemanager/host_volume_plugin.go index 357eb2ef6..e8297a32f 100644 --- a/client/hostvolumemanager/host_volume_plugin.go +++ b/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()) } diff --git a/client/hostvolumemanager/host_volume_plugin_test.go b/client/hostvolumemanager/host_volume_plugin_test.go new file mode 100644 index 000000000..954686d74 --- /dev/null +++ b/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) + } +} diff --git a/client/hostvolumemanager/test_fixtures/test_plugin.sh b/client/hostvolumemanager/test_fixtures/test_plugin.sh new file mode 100755 index 000000000..b60229fd3 --- /dev/null +++ b/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 diff --git a/client/hostvolumemanager/test_fixtures/test_plugin_sad.sh b/client/hostvolumemanager/test_fixtures/test_plugin_sad.sh new file mode 100755 index 000000000..6f883297a --- /dev/null +++ b/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 diff --git a/nomad/structs/host_volumes.go b/nomad/structs/host_volumes.go index 21e827c3a..11745526a 100644 --- a/nomad/structs/host_volumes.go +++ b/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. diff --git a/nomad/structs/host_volumes_test.go b/nomad/structs/host_volumes_test.go index da25ad3cb..499bc27d1 100644 --- a/nomad/structs/host_volumes_test.go +++ b/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: