From 2782645f751f633ea05e9bc9983eca899d4efddf Mon Sep 17 00:00:00 2001
From: Tim Gross <tgross@hashicorp.com>
Date: Wed, 7 Oct 2020 14:52:14 -0400
Subject: [PATCH] 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               |  2 ++
 nomad/csi_endpoint_test.go | 17 +++++++++++++++--
 nomad/structs/csi.go       |  7 ++++++-
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index aeee24203b50..6123c3a8457f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,8 +14,10 @@ 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:
 
  * core: Fixed a bug where blocking queries would not include the query's maximum wait time when calculating whether it was safe to retry. [[GH-8921](https://github.com/hashicorp/nomad/issues/8921)]
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.