CSI: enforce single access mode at validation time (#12337)

A volume that has single-use access mode is feasibility checked during
scheduling to ensure that only a single reader or writer claim
exists. However, because feasibility checking is done one alloc at a
time before the plan is written, a job that's misconfigured to have
count > 1 that mounts one of these volumes will pass feasibility
checking.

Enforce the check at validation time instead to prevent us from even
trying to evaluation a job that's misconfigured this way.

.changelog/12337.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+csi: Fixed a bug where single-use access modes were not enforced during validation
+```

nomad/job_endpoint_test.go

@@ -756,7 +756,7 @@ func TestJobEndpoint_Register_ACL(t *testing.T) {
 				Type:           structs.VolumeTypeCSI,
 				Source:         "prod-db",
 				AttachmentMode: structs.CSIVolumeAttachmentModeBlockDevice,
-				AccessMode:     structs.CSIVolumeAccessModeSingleNodeWriter,
+				AccessMode:     structs.CSIVolumeAccessModeMultiNodeMultiWriter,
 			},
 		}
 

nomad/structs/csi_test.go

@@ -1002,7 +1002,7 @@ func TestDeleteNodeForType_Monolith_NilController(t *testing.T) {
 
 func TestDeleteNodeForType_Monolith_NilNode(t *testing.T) {
 	ci.Parallel(t)
-	
+
 	plug := NewCSIPlugin("foo", 1000)
 
 	plug.Nodes["n0"] = nil

nomad/structs/structs.go

@@ -6396,7 +6396,7 @@ func (tg *TaskGroup) Validate(j *Job) error {
 		canaries = tg.Update.Canary
 	}
 	for name, volReq := range tg.Volumes {
-		if err := volReq.Validate(canaries); err != nil {
+		if err := volReq.Validate(tg.Count, canaries); err != nil {
 			mErr.Errors = append(mErr.Errors, fmt.Errorf(
 				"Task group volume validation for %s failed: %v", name, err))
 		}

nomad/structs/volume_test.go

@@ -0,0 +1,91 @@
+package structs
+
+import (
+	"testing"
+
+	"github.com/hashicorp/nomad/ci"
+	"github.com/stretchr/testify/require"
+)
+
+func TestVolumeRequest_Validate(t *testing.T) {
+	ci.Parallel(t)
+
+	testCases := []struct {
+		name           string
+		expected       []string
+		canariesCount  int
+		taskGroupCount int
+		req            *VolumeRequest
+	}{
+		{
+			name:     "host volume with empty source",
+			expected: []string{"volume has an empty source"},
+			req: &VolumeRequest{
+				Type: VolumeTypeHost,
+			},
+		},
+		{
+			name: "host volume with CSI volume config",
+			expected: []string{
+				"host volumes cannot have an access mode",
+				"host volumes cannot have an attachment mode",
+				"host volumes cannot have mount options",
+				"host volumes do not support per_alloc",
+			},
+			req: &VolumeRequest{
+				Type:           VolumeTypeHost,
+				ReadOnly:       false,
+				AccessMode:     CSIVolumeAccessModeSingleNodeReader,
+				AttachmentMode: CSIVolumeAttachmentModeBlockDevice,
+				MountOptions: &CSIMountOptions{
+					FSType:     "ext4",
+					MountFlags: []string{"ro"},
+				},
+				PerAlloc: true,
+			},
+		},
+		{
+			name: "CSI volume multi-reader-single-writer access mode",
+			expected: []string{
+				"volume with multi-node-single-writer access mode allows only one writer",
+			},
+			taskGroupCount: 2,
+			req: &VolumeRequest{
+				Type:       VolumeTypeCSI,
+				AccessMode: CSIVolumeAccessModeMultiNodeSingleWriter,
+			},
+		},
+		{
+			name: "CSI volume single reader access mode",
+			expected: []string{
+				"volume with single-node-reader-only access mode allows only one reader",
+			},
+			taskGroupCount: 2,
+			req: &VolumeRequest{
+				Type:       VolumeTypeCSI,
+				AccessMode: CSIVolumeAccessModeSingleNodeReader,
+				ReadOnly:   true,
+			},
+		},
+		{
+			name:          "CSI volume per-alloc with canaries",
+			expected:      []string{"volume cannot be per_alloc when canaries are in use"},
+			canariesCount: 1,
+			req: &VolumeRequest{
+				Type:     VolumeTypeCSI,
+				PerAlloc: true,
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		tc = tc
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.req.Validate(tc.taskGroupCount, tc.canariesCount)
+			for _, expected := range tc.expected {
+				require.Contains(t, err.Error(), expected)
+			}
+		})
+	}
+
+}

nomad/structs/volumes.go

@@ -102,54 +102,81 @@ type VolumeRequest struct {
 	PerAlloc       bool
 }
 
-func (v *VolumeRequest) Validate(canaries int) error {
+func (v *VolumeRequest) Validate(taskGroupCount, canaries int) error {
 	if !(v.Type == VolumeTypeHost ||
 		v.Type == VolumeTypeCSI) {
 		return fmt.Errorf("volume has unrecognized type %s", v.Type)
 	}
 
 	var mErr multierror.Error
-	if v.Type == VolumeTypeHost && v.AttachmentMode != CSIVolumeAttachmentModeUnknown {
-		mErr.Errors = append(mErr.Errors,
-			fmt.Errorf("host volumes cannot have an attachment mode"))
+	addErr := func(msg string, args ...interface{}) {
+		mErr.Errors = append(mErr.Errors, fmt.Errorf(msg, args...))
 	}
-	if v.Type == VolumeTypeHost && v.AccessMode != CSIVolumeAccessModeUnknown {
-		mErr.Errors = append(mErr.Errors,
-			fmt.Errorf("host volumes cannot have an access mode"))
-	}
-	if v.Type == VolumeTypeHost && v.MountOptions != nil {
-		mErr.Errors = append(mErr.Errors,
-			fmt.Errorf("host volumes cannot have mount options"))
-	}
-	if v.Type == VolumeTypeCSI && v.AttachmentMode == CSIVolumeAttachmentModeUnknown {
-		mErr.Errors = append(mErr.Errors,
-			fmt.Errorf("CSI volumes must have an attachment mode"))
-	}
-	if v.Type == VolumeTypeCSI && v.AccessMode == CSIVolumeAccessModeUnknown {
-		mErr.Errors = append(mErr.Errors,
-			fmt.Errorf("CSI volumes must have an access mode"))
+
+	if v.Source == "" {
+		addErr("volume has an empty source")
 	}
 
-	if v.AccessMode == CSIVolumeAccessModeSingleNodeReader || v.AccessMode == CSIVolumeAccessModeMultiNodeReader {
-		if !v.ReadOnly {
-			mErr.Errors = append(mErr.Errors,
-				fmt.Errorf("%s volumes must be read-only", v.AccessMode))
+	switch v.Type {
+
+	case VolumeTypeHost:
+		if v.AttachmentMode != CSIVolumeAttachmentModeUnknown {
+			addErr("host volumes cannot have an attachment mode")
+		}
+		if v.AccessMode != CSIVolumeAccessModeUnknown {
+			addErr("host volumes cannot have an access mode")
+		}
+		if v.MountOptions != nil {
+			addErr("host volumes cannot have mount options")
+		}
+		if v.PerAlloc {
+			addErr("host volumes do not support per_alloc")
 		}
-	}
 
-	if v.AttachmentMode == CSIVolumeAttachmentModeBlockDevice && v.MountOptions != nil {
-		mErr.Errors = append(mErr.Errors,
-			fmt.Errorf("block devices cannot have mount options"))
-	}
+	case VolumeTypeCSI:
 
-	if v.PerAlloc && canaries > 0 {
-		mErr.Errors = append(mErr.Errors,
-			fmt.Errorf("volume cannot be per_alloc when canaries are in use"))
-	}
+		switch v.AttachmentMode {
+		case CSIVolumeAttachmentModeUnknown:
+			addErr("CSI volumes must have an attachment mode")
+		case CSIVolumeAttachmentModeBlockDevice:
+			if v.MountOptions != nil {
+				addErr("block devices cannot have mount options")
+			}
+		}
+
+		switch v.AccessMode {
+		case CSIVolumeAccessModeUnknown:
+			addErr("CSI volumes must have an access mode")
+		case CSIVolumeAccessModeSingleNodeReader:
+			if !v.ReadOnly {
+				addErr("%s volumes must be read-only", v.AccessMode)
+			}
+			if taskGroupCount > 1 && !v.PerAlloc {
+				addErr("volume with %s access mode allows only one reader", v.AccessMode)
+			}
+		case CSIVolumeAccessModeSingleNodeWriter:
+			// note: we allow read-only mount of this volume, but only one
+			if taskGroupCount > 1 && !v.PerAlloc {
+				addErr("volume with %s access mode allows only one reader or writer", v.AccessMode)
+			}
+		case CSIVolumeAccessModeMultiNodeReader:
+			if !v.ReadOnly {
+				addErr("%s volumes must be read-only", v.AccessMode)
+			}
+		case CSIVolumeAccessModeMultiNodeSingleWriter:
+			if !v.ReadOnly && taskGroupCount > 1 && !v.PerAlloc {
+				addErr("volume with %s access mode allows only one writer", v.AccessMode)
+			}
+		case CSIVolumeAccessModeMultiNodeMultiWriter:
+			// note: we intentionally allow read-only mount of this mode
+		}
+
+		if v.PerAlloc && canaries > 0 {
+			addErr("volume cannot be per_alloc when canaries are in use")
+		}
 
-	if v.Source == "" {
-		mErr.Errors = append(mErr.Errors, fmt.Errorf("volume has an empty source"))
 	}
+
 	return mErr.ErrorOrNil()
 }