From b7075f04fd4418f1d9ee5402c9a9554f71d0988b Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Wed, 23 Mar 2022 09:21:26 -0400 Subject: [PATCH] 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 | 3 + nomad/job_endpoint_test.go | 2 +- nomad/structs/csi_test.go | 2 +- nomad/structs/structs.go | 2 +- nomad/structs/volume_test.go | 91 +++++++++++++++++++++++++++++++ nomad/structs/volumes.go | 103 ++++++++++++++++++++++------------- 6 files changed, 162 insertions(+), 41 deletions(-) create mode 100644 .changelog/12337.txt create mode 100644 nomad/structs/volume_test.go diff --git a/.changelog/12337.txt b/.changelog/12337.txt new file mode 100644 index 000000000..b6ef664d3 --- /dev/null +++ b/.changelog/12337.txt @@ -0,0 +1,3 @@ +```release-note:bug +csi: Fixed a bug where single-use access modes were not enforced during validation +``` diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go index 3cf238b35..a94d2ab59 100644 --- a/nomad/job_endpoint_test.go +++ b/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, }, } diff --git a/nomad/structs/csi_test.go b/nomad/structs/csi_test.go index 9ef5a7f8b..5da73a78b 100644 --- a/nomad/structs/csi_test.go +++ b/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 diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index c4ac6a652..698512bad 100644 --- a/nomad/structs/structs.go +++ b/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)) } diff --git a/nomad/structs/volume_test.go b/nomad/structs/volume_test.go new file mode 100644 index 000000000..cbe9ed6fe --- /dev/null +++ b/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) + } + }) + } + +} diff --git a/nomad/structs/volumes.go b/nomad/structs/volumes.go index 5fe523873..0f8b040de 100644 --- a/nomad/structs/volumes.go +++ b/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")) - } - 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.AccessMode == CSIVolumeAccessModeSingleNodeReader || v.AccessMode == CSIVolumeAccessModeMultiNodeReader { - if !v.ReadOnly { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("%s volumes must be read-only", v.AccessMode)) - } - } - - if v.AttachmentMode == CSIVolumeAttachmentModeBlockDevice && v.MountOptions != nil { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("block devices cannot have mount options")) - } - - if v.PerAlloc && canaries > 0 { - mErr.Errors = append(mErr.Errors, - fmt.Errorf("volume cannot be per_alloc when canaries are in use")) + addErr := func(msg string, args ...interface{}) { + mErr.Errors = append(mErr.Errors, fmt.Errorf(msg, args...)) } if v.Source == "" { - mErr.Errors = append(mErr.Errors, fmt.Errorf("volume has an empty source")) + addErr("volume has an empty source") } + + 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") + } + + case VolumeTypeCSI: + + 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") + } + + } + return mErr.ErrorOrNil() }