From 99c25fc6353ce58b43d7d421ec1dad72bb851fb0 Mon Sep 17 00:00:00 2001 From: Daniel Bennett Date: Fri, 28 Mar 2025 11:13:13 -0400 Subject: [PATCH] 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 | 2 +- client/host_volume_endpoint.go | 3 - .../hostvolumemanager/host_volume_plugin.go | 82 ++++++++-- .../host_volume_plugin_test.go | 147 +++++++++++++++++- client/hostvolumemanager/host_volumes.go | 9 +- command/volume_create_host_test.go | 2 +- nomad/mock/host_volumes.go | 2 +- .../concepts/plugins/storage/host-volumes.mdx | 11 +- .../docs/other-specifications/volume/host.mdx | 47 +++++- 9 files changed, 271 insertions(+), 34 deletions(-) diff --git a/client/fingerprint/dynamic_host_volumes.go b/client/fingerprint/dynamic_host_volumes.go index 290733ce8..85de34ba9 100644 --- a/client/fingerprint/dynamic_host_volumes.go +++ b/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 } diff --git a/client/host_volume_endpoint.go b/client/host_volume_endpoint.go index 690d28d26..7af800d4b 100644 --- a/client/host_volume_endpoint.go +++ b/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 } diff --git a/client/hostvolumemanager/host_volume_plugin.go b/client/hostvolumemanager/host_volume_plugin.go index 364201176..92473e7f1 100644 --- a/client/hostvolumemanager/host_volume_plugin.go +++ b/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") diff --git a/client/hostvolumemanager/host_volume_plugin_test.go b/client/hostvolumemanager/host_volume_plugin_test.go index cebdb0498..dcbd162c1 100644 --- a/client/hostvolumemanager/host_volume_plugin_test.go +++ b/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") diff --git a/client/hostvolumemanager/host_volumes.go b/client/hostvolumemanager/host_volumes.go index 7b4ce33d9..bc321f4a0 100644 --- a/client/hostvolumemanager/host_volumes.go +++ b/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 } diff --git a/command/volume_create_host_test.go b/command/volume_create_host_test.go index af1fb1b61..638b4341b 100644 --- a/command/volume_create_host_test.go +++ b/command/volume_create_host_test.go @@ -57,7 +57,7 @@ capability { } parameters { - foo = "bar" + mode = "0700" } ` diff --git a/nomad/mock/host_volumes.go b/nomad/mock/host_volumes.go index d0c5b14ac..43f1a94e2 100644 --- a/nomad/mock/host_volumes.go +++ b/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 diff --git a/website/content/docs/concepts/plugins/storage/host-volumes.mdx b/website/content/docs/concepts/plugins/storage/host-volumes.mdx index df5caf368..2fcb75fe2 100644 --- a/website/content/docs/concepts/plugins/storage/host-volumes.mdx +++ b/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 diff --git a/website/content/docs/other-specifications/volume/host.mdx b/website/content/docs/other-specifications/volume/host.mdx index 173ad3606..54c7a7891 100644 --- a/website/content/docs/other-specifications/volume/host.mdx +++ b/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: )` - 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` ([Capability][capability]: <required>) - 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: )` - 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: )` - [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: )` - User ID to own the directory. Defaults to the +user running the Nomad client agent, which is commonly `0` (root). +- `gid` `(int: )` - 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 + + + +```hcl +type = "host" +name = "cool-host-vol" +plugin_id = "mkdir" +parameters = { + mode = "0755" + uid = 1000 + gid = 1000 +} +``` + + + ## 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