From f3d49ffaed0fe63b11c965118cb5b4aef7210df3 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 7 Oct 2020 14:52:14 -0400 Subject: [PATCH 1/2] csi: validate mount options during volume registration Volumes using attachment mode `file-system` use the CSI filesystem API when they're mounted, and can be passed mount options. But `block-device` mode volumes don't have this option. When RPCs are made to plugins, we are silently dropping the mount options we don't expect to see, but this results in a poor operator experience when the mount options aren't honored. This changeset makes passing mount options to a `block-device` volume a validation error. --- CHANGELOG.md | 1 + nomad/csi_endpoint_test.go | 17 +++++++++++++++-- nomad/structs/csi.go | 7 ++++++- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aeee24203b50..37c0333348fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ IMPROVEMENTS: __BACKWARDS INCOMPATIBILITIES:__ * core: null characters are prohibited in region, datacenter, job name/ID, task group name, and task name [[GH-9020](https://github.com/hashicorp/nomad/issues/9020)] + * csi: registering a CSI volume with a `block-device` attachment mode and `mount_options` now returns a validation error, instead of silently dropping the `mount_options`. [[GH-9044](https://github.com/hashicorp/nomad/issues/9044)] * driver/docker: Tasks are now issued SIGTERM instead of SIGINT when stopping [[GH-8932](https://github.com/hashicorp/nomad/issues/8932)] BUG FIXES: diff --git a/nomad/csi_endpoint_test.go b/nomad/csi_endpoint_test.go index 03194c3914d2..d21098458650 100644 --- a/nomad/csi_endpoint_test.go +++ b/nomad/csi_endpoint_test.go @@ -141,8 +141,12 @@ func TestCSIVolumeEndpoint_Register(t *testing.T) { Namespace: "notTheNamespace", PluginID: "minnie", AccessMode: structs.CSIVolumeAccessModeMultiNodeReader, - AttachmentMode: structs.CSIVolumeAttachmentModeFilesystem, - Secrets: structs.CSISecrets{"mysecret": "secretvalue"}, + AttachmentMode: structs.CSIVolumeAttachmentModeBlockDevice, + MountOptions: &structs.CSIMountOptions{ + FSType: "ext4", MountFlags: []string{"sensitive"}}, + Secrets: structs.CSISecrets{"mysecret": "secretvalue"}, + Parameters: map[string]string{"myparam": "paramvalue"}, + Context: map[string]string{"mycontext": "contextvalue"}, }} // Create the register request @@ -155,6 +159,11 @@ func TestCSIVolumeEndpoint_Register(t *testing.T) { } resp1 := &structs.CSIVolumeRegisterResponse{} err := msgpackrpc.CallWithCodec(codec, "CSIVolume.Register", req1, resp1) + require.Error(t, err, "expected validation error") + + // Fix the registration so that it passes validation + vols[0].AttachmentMode = structs.CSIVolumeAttachmentModeFilesystem + err = msgpackrpc.CallWithCodec(codec, "CSIVolume.Register", req1, resp1) require.NoError(t, err) require.NotEqual(t, uint64(0), resp1.Index) @@ -170,6 +179,10 @@ func TestCSIVolumeEndpoint_Register(t *testing.T) { require.NoError(t, err) require.Equal(t, resp1.Index, resp2.Index) require.Equal(t, vols[0].ID, resp2.Volume.ID) + require.Equal(t, "csi.CSISecrets(map[mysecret:[REDACTED]])", + resp2.Volume.Secrets.String()) + require.Equal(t, "csi.CSIOptions(FSType: ext4, MountFlags: [REDACTED])", + resp2.Volume.MountOptions.String()) // Registration does not update req1.Volumes[0].PluginID = "adam" diff --git a/nomad/structs/csi.go b/nomad/structs/csi.go index 30404344ddf1..6e455b7573eb 100644 --- a/nomad/structs/csi.go +++ b/nomad/structs/csi.go @@ -172,7 +172,7 @@ func (o *CSIMountOptions) Merge(p *CSIMountOptions) { } } -// VolumeMountOptions implements the Stringer and GoStringer interfaces to prevent +// CSIMountOptions implements the Stringer and GoStringer interfaces to prevent // accidental leakage of sensitive mount flags via logs. var _ fmt.Stringer = &CSIMountOptions{} var _ fmt.GoStringer = &CSIMountOptions{} @@ -578,6 +578,11 @@ func (v *CSIVolume) Validate() error { if v.AttachmentMode == "" { errs = append(errs, "missing attachment mode") } + if v.AttachmentMode == CSIVolumeAttachmentModeBlockDevice { + if v.MountOptions != nil { + errs = append(errs, "mount options not allowed for block-device") + } + } // TODO: Volume Topologies are optional - We should check to see if the plugin // the volume is being registered with requires them. From 6cd1dbec3dec7e172d10ae07d7f89c0fc553b3b6 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 8 Oct 2020 08:58:05 -0400 Subject: [PATCH 2/2] allow empty mount options --- nomad/structs/csi.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/nomad/structs/csi.go b/nomad/structs/csi.go index 6e455b7573eb..b0c5796cafb2 100644 --- a/nomad/structs/csi.go +++ b/nomad/structs/csi.go @@ -580,7 +580,12 @@ func (v *CSIVolume) Validate() error { } if v.AttachmentMode == CSIVolumeAttachmentModeBlockDevice { if v.MountOptions != nil { - errs = append(errs, "mount options not allowed for block-device") + if v.MountOptions.FSType != "" { + errs = append(errs, "mount options not allowed for block-device") + } + if v.MountOptions.MountFlags != nil && len(v.MountOptions.MountFlags) != 0 { + errs = append(errs, "mount options not allowed for block-device") + } } }