connect: canonicalize before adding sidecar

Fixes #6853

Canonicalize jobs first before adding any sidecars. This fixes a bug
where sidecar tasks were added without interpolated names and broke
validation. Sidecar tasks must be canonicalized independently.

Also adds a group network to the mock connect job because it wasn't a
valid connect job before!

nomad/job_endpoint.go

@@ -58,8 +58,8 @@ func NewJobEndpoints(s *Server) *Job {
 		srv:    s,
 		logger: s.logger.Named("job"),
 		mutators: []jobMutator{
-			jobConnectHook{},
 			jobCanonicalizer{},
+			jobConnectHook{},
 			jobImpliedConstraints{},
 		},
 		validators: []jobValidator{

nomad/job_endpoint_hook_connect.go

@@ -59,7 +59,7 @@ func (jobConnectHook) Mutate(job *structs.Job) (_ *structs.Job, warnings []error
 			continue
 		}
 
-		if err := groupConnectHook(g); err != nil {
+		if err := groupConnectHook(job, g); err != nil {
 			return nil, nil, err
 		}
 	}
@@ -96,15 +96,15 @@ func isSidecarForService(t *structs.Task, svc string) bool {
 	return t.Kind == structs.TaskKind(fmt.Sprintf("%s:%s", structs.ConnectProxyPrefix, svc))
 }
 
-func groupConnectHook(g *structs.TaskGroup) error {
+func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error {
 	for _, service := range g.Services {
 		if service.Connect.HasSidecar() {
 			// Check to see if the sidecar task already exists
 			task := getSidecarTaskForService(g, service.Name)
 
 			// If the task doesn't already exist, create a new one and add it to the job
 			if task == nil {
-				task = newConnectTask(service)
+				task = newConnectTask(service.Name)
 
 				// If there happens to be a task defined with the same name
 				// append an UUID fragment to the task name
@@ -121,6 +121,9 @@ func groupConnectHook(g *structs.TaskGroup) error {
 				service.Connect.SidecarTask.MergeIntoTask(task)
 			}
 
+			// Canonicalize task since this mutator runs after job canonicalization
+			task.Canonicalize(job, g)
+
 			// port to be added for the sidecar task's proxy port
 			port := structs.Port{
 				Label: fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name),
@@ -147,11 +150,11 @@ func groupConnectHook(g *structs.TaskGroup) error {
 	return nil
 }
 
-func newConnectTask(service *structs.Service) *structs.Task {
+func newConnectTask(serviceName string) *structs.Task {
 	task := &structs.Task{
 		// Name is used in container name so must start with '[A-Za-z0-9]'
-		Name:          fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name),
-		Kind:          structs.TaskKind(fmt.Sprintf("%s:%s", structs.ConnectProxyPrefix, service.Name)),
+		Name:          fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, serviceName),
+		Kind:          structs.TaskKind(fmt.Sprintf("%s:%s", structs.ConnectProxyPrefix, serviceName)),
 		Driver:        "docker",
 		Config:        connectDriverConfig,
 		ShutdownDelay: 5 * time.Second,

nomad/job_endpoint_hook_connect_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/hashicorp/nomad/helper/testlog"
+	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/stretchr/testify/require"
 )
@@ -49,7 +51,8 @@ func Test_isSidecarForService(t *testing.T) {
 
 func Test_groupConnectHook(t *testing.T) {
 	// Test that connect-proxy task is inserted for backend service
-	tgIn := &structs.TaskGroup{
+	job := mock.Job()
+	job.TaskGroups[0] = &structs.TaskGroup{
 		Networks: structs.Networks{
 			{
 				Mode: "bridge",
@@ -73,11 +76,16 @@ func Test_groupConnectHook(t *testing.T) {
 		},
 	}
 
-	tgOut := tgIn.Copy()
+	// Expected tasks
+	tgOut := job.TaskGroups[0].Copy()
 	tgOut.Tasks = []*structs.Task{
-		newConnectTask(tgOut.Services[0]),
-		newConnectTask(tgOut.Services[1]),
+		newConnectTask(tgOut.Services[0].Name),
+		newConnectTask(tgOut.Services[1].Name),
 	}
+
+	// Expect sidecar tasks to be properly canonicalized
+	tgOut.Tasks[0].Canonicalize(job, tgOut)
+	tgOut.Tasks[1].Canonicalize(job, tgOut)
 	tgOut.Networks[0].DynamicPorts = []structs.Port{
 		{
 			Label: fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, "backend"),
@@ -89,10 +97,31 @@ func Test_groupConnectHook(t *testing.T) {
 		},
 	}
 
-	require.NoError(t, groupConnectHook(tgIn))
-	require.Exactly(t, tgOut, tgIn)
+	require.NoError(t, groupConnectHook(job, job.TaskGroups[0]))
+	require.Exactly(t, tgOut, job.TaskGroups[0])
 
 	// Test that hook is idempotent
-	require.NoError(t, groupConnectHook(tgIn))
-	require.Exactly(t, tgOut, tgIn)
+	require.NoError(t, groupConnectHook(job, job.TaskGroups[0]))
+	require.Exactly(t, tgOut, job.TaskGroups[0])
+}
+
+// TestJobEndpoint_ConnectInterpolation asserts that when a Connect sidecar
+// proxy task is being created for a group service with an interpolated name,
+// the service name is interpolated *before the task is created.
+//
+// See https://github.com/hashicorp/nomad/issues/6853
+func TestJobEndpoint_ConnectInterpolation(t *testing.T) {
+	t.Parallel()
+
+	server := &Server{logger: testlog.HCLogger(t)}
+	jobEndpoint := NewJobEndpoints(server)
+
+	j := mock.ConnectJob()
+	j.TaskGroups[0].Services[0].Name = "${JOB}-api"
+	j, warnings, err := jobEndpoint.admissionMutators(j)
+	require.NoError(t, err)
+	require.Nil(t, warnings)
+
+	require.Len(t, j.TaskGroups[0].Tasks, 2)
+	require.Equal(t, "connect-proxy-my-job-api", j.TaskGroups[0].Tasks[1].Name)
 }

nomad/mock/mock.go

@@ -385,6 +385,11 @@ func MaxParallelJob() *structs.Job {
 func ConnectJob() *structs.Job {
 	job := Job()
 	tg := job.TaskGroups[0]
+	tg.Networks = []*structs.NetworkResource{
+		{
+			Mode: "bridge",
+		},
+	}
 	tg.Services = []*structs.Service{
 		{
 			Name:      "testconnect",

nomad/structs/structs.go

@@ -5552,6 +5552,8 @@ func (t *Task) Validate(ephemeralDisk *EphemeralDisk, jobType string, tgServices
 		if t.Leader {
 			mErr.Errors = append(mErr.Errors, fmt.Errorf("Connect proxy task must not have leader set"))
 		}
+
+		// Ensure the proxy task has a corresponding service entry
 		serviceErr := ValidateConnectProxyService(t.Kind.Value(), tgServices)
 		if serviceErr != nil {
 			mErr.Errors = append(mErr.Errors, serviceErr)
@@ -5746,15 +5748,28 @@ const ConnectProxyPrefix = "connect-proxy"
 // valid Connect config.
 func ValidateConnectProxyService(serviceName string, tgServices []*Service) error {
 	found := false
+	names := make([]string, 0, len(tgServices))
 	for _, svc := range tgServices {
-		if svc.Name == serviceName && svc.Connect != nil && svc.Connect.SidecarService != nil {
+		if svc.Connect == nil || svc.Connect.SidecarService == nil {
+			continue
+		}
+
+		if svc.Name == serviceName {
 			found = true
 			break
 		}
+
+		// Build up list of mismatched Connect service names for error
+		// reporting.
+		names = append(names, svc.Name)
 	}
 
 	if !found {
-		return fmt.Errorf("Connect proxy service name not found in services from task group")
+		if len(names) == 0 {
+			return fmt.Errorf("No Connect services in task group with Connect proxy (%q)", serviceName)
+		} else {
+			return fmt.Errorf("Connect proxy service name (%q) not found in Connect services from task group: %s", serviceName, names)
+		}
 	}
 
 	return nil

nomad/structs/structs_test.go

@@ -1599,20 +1599,20 @@ func TestTask_Validate_ConnectProxyKind(t *testing.T) {
 			Service: &Service{
 				Name: "redis",
 			},
-			ErrContains: "Connect proxy service name not found in services from task group",
+			ErrContains: `No Connect services in task group with Connect proxy ("redis:test")`,
 		},
 		{
 			Desc:        "Service name not found in group",
 			Kind:        "connect-proxy:redis",
-			ErrContains: "Connect proxy service name not found in services from task group",
+			ErrContains: `No Connect services in task group with Connect proxy ("redis")`,
 		},
 		{
 			Desc: "Connect stanza not configured in group",
 			Kind: "connect-proxy:redis",
 			TgService: []*Service{{
 				Name: "redis",
 			}},
-			ErrContains: "Connect proxy service name not found in services from task group",
+			ErrContains: `No Connect services in task group with Connect proxy ("redis")`,
 		},
 		{
 			Desc: "Valid connect proxy kind",
@@ -1640,12 +1640,8 @@ func TestTask_Validate_ConnectProxyKind(t *testing.T) {
 				// Ok!
 				return
 			}
-			if err == nil {
-				t.Fatalf("no error returned. expected: %s", tc.ErrContains)
-			}
-			if !strings.Contains(err.Error(), tc.ErrContains) {
-				t.Fatalf("expected %q but found: %v", tc.ErrContains, err)
-			}
+			require.Errorf(t, err, "no error returned. expected: %s", tc.ErrContains)
+			require.Containsf(t, err.Error(), tc.ErrContains, "expected %q but found: %v", tc.ErrContains, err)
 		})
 	}