Merge pull request #14033 from hashicorp/f-plumb-task-names

core: automatically plumb task name into task-level services and checks

nomad/structs/services.go

@@ -193,7 +193,7 @@ func (sc *ServiceCheck) Equals(o *ServiceCheck) bool {
 	return true
 }
 
-func (sc *ServiceCheck) Canonicalize(serviceName string) {
+func (sc *ServiceCheck) Canonicalize(serviceName, taskName string) {
 	// Ensure empty maps/slices are treated as null to avoid scheduling
 	// issues when using DeepEquals.
 	if len(sc.Args) == 0 {
@@ -216,6 +216,11 @@ func (sc *ServiceCheck) Canonicalize(serviceName string) {
 		sc.Name = fmt.Sprintf("service: %q check", serviceName)
 	}
 
+	// Set task name if not already set
+	if sc.TaskName == "" && taskName != "group" {
+		sc.TaskName = taskName
+	}
+
 	// Ensure OnUpdate defaults to require_healthy (i.e. healthiness check)
 	if sc.OnUpdate == "" {
 		sc.OnUpdate = OnUpdateRequireHealthy
@@ -528,9 +533,10 @@ type Service struct {
 	Name string
 
 	// Name of the Task associated with this service.
-	//
-	// Currently only used to identify the implementing task of a Consul
-	// Connect Native enabled service.
+	// Group services do not have a task name, unless they are a connect native
+	// service specifying the task implementing the service.
+	// Task-level services automatically have the task name plumbed through
+	// down to checks for convenience.
 	TaskName string
 
 	// PortLabel is either the numeric port number or the `host:port`.
@@ -626,6 +632,11 @@ func (s *Service) Canonicalize(job, taskGroup, task, jobNamespace string) {
 		s.TaggedAddresses = nil
 	}
 
+	// Set the task name if not already set
+	if s.TaskName == "" && task != "group" {
+		s.TaskName = task
+	}
+
 	s.Name = args.ReplaceEnv(s.Name, map[string]string{
 		"JOB":       job,
 		"TASKGROUP": taskGroup,
@@ -634,7 +645,7 @@ func (s *Service) Canonicalize(job, taskGroup, task, jobNamespace string) {
 	})
 
 	for _, check := range s.Checks {
-		check.Canonicalize(s.Name)
+		check.Canonicalize(s.Name, s.TaskName)
 	}
 
 	// Set the provider to its default value. The value of consul ensures this

nomad/structs/services_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/hashicorp/nomad/ci"
 	"github.com/hashicorp/nomad/helper"
 	"github.com/shoenig/test/must"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -67,7 +66,7 @@ func TestServiceCheck_Canonicalize(t *testing.T) {
 			Method:   "",
 			OnUpdate: "",
 		}
-		sc.Canonicalize("MyService")
+		sc.Canonicalize("MyService", "task1")
 		must.Nil(t, sc.Args)
 		must.Nil(t, sc.Header)
 		must.Eq(t, `service: "MyService" check`, sc.Name)
@@ -79,7 +78,7 @@ func TestServiceCheck_Canonicalize(t *testing.T) {
 		sc := &ServiceCheck{
 			Name: "Some Check",
 		}
-		sc.Canonicalize("MyService")
+		sc.Canonicalize("MyService", "task1")
 		must.Eq(t, "Some Check", sc.Name)
 	})
 
@@ -87,7 +86,7 @@ func TestServiceCheck_Canonicalize(t *testing.T) {
 		sc := &ServiceCheck{
 			OnUpdate: OnUpdateIgnore,
 		}
-		sc.Canonicalize("MyService")
+		sc.Canonicalize("MyService", "task1")
 		must.Eq(t, OnUpdateIgnore, sc.OnUpdate)
 	})
 }
@@ -1675,24 +1674,24 @@ func TestService_Validate(t *testing.T) {
 		name      string
 	}{
 		{
+			name: "base service",
 			input: &Service{
 				Name: "testservice",
 			},
 			expErr: false,
-			name:   "base service",
 		},
 		{
+			name: "Native Connect without task name",
 			input: &Service{
 				Name: "testservice",
 				Connect: &ConsulConnect{
 					Native: true,
 				},
 			},
-			expErr:    true,
-			expErrStr: "Connect Native and requires setting the task",
-			name:      "Native Connect without task name",
+			expErr: false, // gets set automatically
 		},
 		{
+			name: "Native Connect with task name",
 			input: &Service{
 				Name:     "testservice",
 				TaskName: "testtask",
@@ -1701,9 +1700,9 @@ func TestService_Validate(t *testing.T) {
 				},
 			},
 			expErr: false,
-			name:   "Native Connect with task name",
 		},
 		{
+			name: "Native Connect with Sidecar",
 			input: &Service{
 				Name:     "testservice",
 				TaskName: "testtask",
@@ -1714,9 +1713,9 @@ func TestService_Validate(t *testing.T) {
 			},
 			expErr:    true,
 			expErrStr: "Consul Connect must be exclusively native",
-			name:      "Native Connect with Sidecar",
 		},
 		{
+			name: "provider nomad with checks",
 			input: &Service{
 				Name:      "testservice",
 				Provider:  "nomad",
@@ -1738,9 +1737,9 @@ func TestService_Validate(t *testing.T) {
 				},
 			},
 			expErr: false,
-			name:   "provider nomad with checks",
 		},
 		{
+			name: "provider nomad with invalid check type",
 			input: &Service{
 				Name:     "testservice",
 				Provider: "nomad",
@@ -1752,9 +1751,9 @@ func TestService_Validate(t *testing.T) {
 				},
 			},
 			expErr: true,
-			name:   "provider nomad with invalid check type",
 		},
 		{
+			name: "provider nomad with connect",
 			input: &Service{
 				Name:     "testservice",
 				Provider: "nomad",
@@ -1764,15 +1763,14 @@ func TestService_Validate(t *testing.T) {
 			},
 			expErr:    true,
 			expErrStr: "Service with provider nomad cannot include Connect blocks",
-			name:      "provider nomad with connect",
 		},
 		{
+			name: "provider nomad valid",
 			input: &Service{
 				Name:     "testservice",
 				Provider: "nomad",
 			},
 			expErr: false,
-			name:   "provider nomad valid",
 		},
 	}
 
@@ -1781,10 +1779,10 @@ func TestService_Validate(t *testing.T) {
 			tc.input.Canonicalize("testjob", "testgroup", "testtask", "testnamespace")
 			err := tc.input.Validate()
 			if tc.expErr {
-				assert.Error(t, err)
-				assert.Contains(t, err.Error(), tc.expErrStr)
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tc.expErrStr)
 			} else {
-				assert.NoError(t, err)
+				require.NoError(t, err)
 			}
 		})
 	}

nomad/structs/structs.go

@@ -6675,10 +6675,19 @@ func (tg *TaskGroup) validateServices() error {
 
 		for _, service := range task.Services {
 
-			// Ensure no task-level checks specify a task
+			// Ensure no task-level service can only specify the task it belongs to.
+			if service.TaskName != "" && service.TaskName != task.Name {
+				mErr.Errors = append(mErr.Errors,
+					fmt.Errorf("Service %s is invalid: may only specify task the service belongs to, got %q", service.Name, service.TaskName),
+				)
+			}
+
+			// Ensure no task-level checks can only specify the task they belong to.
 			for _, check := range service.Checks {
-				if check.TaskName != "" {
-					mErr.Errors = append(mErr.Errors, fmt.Errorf("Check %s is invalid: only task group service checks can be assigned tasks", check.Name))
+				if check.TaskName != "" && check.TaskName != task.Name {
+					mErr.Errors = append(mErr.Errors,
+						fmt.Errorf("Check %s is invalid: may only specify task the check belongs to, got %q", check.Name, check.TaskName),
+					)
 				}
 			}
 
@@ -7924,7 +7933,7 @@ type AllocState struct {
 // they are assigned to is down, their state is migrated to the replacement
 // allocation.
 //
-//  Minimal set of fields from plugins/drivers/task_handle.go:TaskHandle
+// Minimal set of fields from plugins/drivers/task_handle.go:TaskHandle
 type TaskHandle struct {
 	// Version of driver state. Used by the driver to gracefully handle
 	// plugin upgrades.

nomad/structs/structs_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/hashicorp/nomad/ci"
 	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/helper/uuid"
-
 	"github.com/kr/pretty"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -3431,6 +3430,7 @@ func TestService_Canonicalize(t *testing.T) {
 				Name:      "redis-db",
 				Provider:  "consul",
 				Namespace: "default",
+				TaskName:  "redis",
 			},
 			name: "interpolate task in name",
 		},
@@ -3446,6 +3446,7 @@ func TestService_Canonicalize(t *testing.T) {
 				Name:      "db",
 				Provider:  "consul",
 				Namespace: "default",
+				TaskName:  "redis",
 			},
 			name: "no interpolation in name",
 		},
@@ -3461,6 +3462,7 @@ func TestService_Canonicalize(t *testing.T) {
 				Name:      "example-cache-redis-db",
 				Provider:  "consul",
 				Namespace: "default",
+				TaskName:  "redis",
 			},
 			name: "interpolate job, taskgroup and task in name",
 		},
@@ -3476,6 +3478,7 @@ func TestService_Canonicalize(t *testing.T) {
 				Name:      "example-cache-redis-db",
 				Provider:  "consul",
 				Namespace: "default",
+				TaskName:  "redis",
 			},
 			name: "interpolate base in name",
 		},
@@ -3492,6 +3495,7 @@ func TestService_Canonicalize(t *testing.T) {
 				Name:      "db",
 				Provider:  "nomad",
 				Namespace: "platform",
+				TaskName:  "redis",
 			},
 			name: "nomad provider",
 		},