From 0c51bb02dd67231826f0ba38b77798ad996696a7 Mon Sep 17 00:00:00 2001
From: Tim Gross <tgross@hashicorp.com>
Date: Tue, 6 Apr 2021 13:54:53 -0400
Subject: [PATCH] refactor: move VolumeRequest validation to Validate method

---
 nomad/structs/structs.go      | 22 ++++++++--------------
 nomad/structs/structs_test.go |  8 ++++----
 nomad/structs/volumes.go      | 24 ++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go
index e92d42981..6f1b11040 100644
--- a/nomad/structs/structs.go
+++ b/nomad/structs/structs.go
@@ -6189,20 +6189,14 @@ func (tg *TaskGroup) Validate(j *Job) error {
 	}
 
 	// Validate the volume requests
-	for name, decl := range tg.Volumes {
-		if !(decl.Type == VolumeTypeHost ||
-			decl.Type == VolumeTypeCSI) {
-			mErr.Errors = append(mErr.Errors, fmt.Errorf("Volume %s has unrecognised type %s", name, decl.Type))
-			continue
-		}
-
-		if decl.PerAlloc && tg.Update != nil && tg.Update.Canary > 0 {
-			mErr.Errors = append(mErr.Errors,
-				fmt.Errorf("Volume %s cannot be per_alloc when canaries are in use", name))
-		}
-
-		if decl.Source == "" {
-			mErr.Errors = append(mErr.Errors, fmt.Errorf("Volume %s has an empty source", name))
+	var canaries int
+	if tg.Update != nil {
+		canaries = tg.Update.Canary
+	}
+	for name, volReq := range tg.Volumes {
+		if err := volReq.Validate(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/structs_test.go b/nomad/structs/structs_test.go
index e67def0da..b98ec2551 100644
--- a/nomad/structs/structs_test.go
+++ b/nomad/structs/structs_test.go
@@ -1088,7 +1088,7 @@ func TestTaskGroup_Validate(t *testing.T) {
 		},
 	}
 	err = tg.Validate(&Job{})
-	require.Contains(t, err.Error(), `Volume foo has unrecognised type nothost`)
+	require.Contains(t, err.Error(), `volume has unrecognised type nothost`)
 
 	tg = &TaskGroup{
 		Volumes: map[string]*VolumeRequest{
@@ -1104,7 +1104,7 @@ func TestTaskGroup_Validate(t *testing.T) {
 		},
 	}
 	err = tg.Validate(&Job{})
-	require.Contains(t, err.Error(), `Volume foo has an empty source`)
+	require.Contains(t, err.Error(), `volume has an empty source`)
 
 	tg = &TaskGroup{
 		Name: "group-a",
@@ -1125,8 +1125,8 @@ func TestTaskGroup_Validate(t *testing.T) {
 		},
 	}
 	err = tg.Validate(&Job{})
-	require.Contains(t, err.Error(), `Volume foo has an empty source`)
-	require.Contains(t, err.Error(), `Volume foo cannot be per_alloc when canaries are in use`)
+	require.Contains(t, err.Error(), `volume has an empty source`)
+	require.Contains(t, err.Error(), `volume cannot be per_alloc when canaries are in use`)
 
 	tg = &TaskGroup{
 		Volumes: map[string]*VolumeRequest{
diff --git a/nomad/structs/volumes.go b/nomad/structs/volumes.go
index ed6da1342..0a7e18d2a 100644
--- a/nomad/structs/volumes.go
+++ b/nomad/structs/volumes.go
@@ -1,5 +1,11 @@
 package structs
 
+import (
+	"fmt"
+
+	multierror "github.com/hashicorp/go-multierror"
+)
+
 const (
 	VolumeTypeHost = "host"
 )
@@ -94,6 +100,24 @@ type VolumeRequest struct {
 	PerAlloc     bool
 }
 
+func (v *VolumeRequest) Validate(canaries int) error {
+	if !(v.Type == VolumeTypeHost ||
+		v.Type == VolumeTypeCSI) {
+		return fmt.Errorf("volume has unrecognised type %s", v.Type)
+	}
+
+	var mErr multierror.Error
+	if v.PerAlloc && canaries > 0 {
+		mErr.Errors = append(mErr.Errors,
+			fmt.Errorf("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()
+}
+
 func (v *VolumeRequest) Copy() *VolumeRequest {
 	if v == nil {
 		return nil