From 44df2644526244004746be01d6c08f1d46bc2ca4 Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 8 Jul 2020 10:19:36 -0500 Subject: [PATCH] consul/connect: infer task name in service if possible Before, the service definition for a Connect Native service would always require setting the `service.task` parameter. Now, that parameter is automatically inferred when there is only one task in the task group. Fixes #8274 --- nomad/job_endpoint_hook_connect.go | 39 ++++++++------ nomad/job_endpoint_hook_connect_test.go | 54 +++++++++++-------- nomad/structs/services.go | 3 +- .../pages/docs/job-specification/service.mdx | 4 +- 4 files changed, 57 insertions(+), 43 deletions(-) diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index 73b264006f8..14c3186a606 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" + "github.com/pkg/errors" ) var ( @@ -97,13 +98,23 @@ func isSidecarForService(t *structs.Task, svc string) bool { return t.Kind == structs.TaskKind(fmt.Sprintf("%s:%s", structs.ConnectProxyPrefix, svc)) } -func getNamedTaskForNativeService(tg *structs.TaskGroup, taskName string) *structs.Task { +// getNamedTaskForNativeService retrieves the Task with the name specified in the +// group service definition. If the task name is empty and there is only one task +// in the group, infer the name from the only option. +func getNamedTaskForNativeService(tg *structs.TaskGroup, serviceName, taskName string) (*structs.Task, error) { + if taskName == "" { + if len(tg.Tasks) == 1 { + return tg.Tasks[0], nil + } + return nil, errors.Errorf("task for Consul Connect Native service %s->%s is ambiguous and must be set", tg.Name, serviceName) + } + for _, t := range tg.Tasks { if t.Name == taskName { - return t + return t, nil } } - return nil + return nil, errors.Errorf("task %s named by Consul Connect Native service %s->%s does not exist", taskName, tg.Name, serviceName) } // probably need to hack this up to look for checks on the service, and if they @@ -155,11 +166,13 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { // create a port for the sidecar task's proxy port makePort(fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name)) } else if service.Connect.IsNative() { + // find the task backing this connect native service and set the kind nativeTaskName := service.TaskName - if t := getNamedTaskForNativeService(g, nativeTaskName); t != nil { - t.Kind = structs.NewTaskKind(structs.ConnectNativePrefix, service.Name) + if t, err := getNamedTaskForNativeService(g, service.Name, nativeTaskName); err != nil { + return err } else { - return fmt.Errorf("native task %s named by %s->%s does not exist", nativeTaskName, g.Name, service.Name) + t.Kind = structs.NewTaskKind(structs.ConnectNativePrefix, service.Name) + service.TaskName = t.Name // in case the task was inferred } } } @@ -220,16 +233,8 @@ func groupConnectSidecarValidate(g *structs.TaskGroup) error { func groupConnectNativeValidate(g *structs.TaskGroup, s *structs.Service) error { // note that network mode is not enforced for connect native services - // a native service must have the task identified in the service definition. - if len(s.TaskName) == 0 { - return fmt.Errorf("Consul Connect Native service %q requires task name", s.Name) + if _, err := getNamedTaskForNativeService(g, s.Name, s.TaskName); err != nil { + return err } - - // also make sure that task actually exists - for _, task := range g.Tasks { - if s.TaskName == task.Name { - return nil - } - } - return fmt.Errorf("Consul Connect Native service %q requires undefined task %q in group %q", s.Name, s.TaskName, g.Name) + return nil } diff --git a/nomad/job_endpoint_hook_connect_test.go b/nomad/job_endpoint_hook_connect_test.go index 931554ed145..87bc2218e55 100644 --- a/nomad/job_endpoint_hook_connect_test.go +++ b/nomad/job_endpoint_hook_connect_test.go @@ -157,32 +157,40 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { }) } -func TestJobEndpointConnect_groupConnectNativeValidate(t *testing.T) { - t.Run("no task in service", func(t *testing.T) { - require.EqualError(t, groupConnectNativeValidate(&structs.TaskGroup{ - Name: "g1", - }, &structs.Service{ - Name: "s1", - TaskName: "", - }), `Consul Connect Native service "s1" requires task name`) +func TestJobEndpointConnect_getNamedTaskForNativeService(t *testing.T) { + t.Run("named exists", func(t *testing.T) { + task, err := getNamedTaskForNativeService(&structs.TaskGroup{ + Name: "g1", + Tasks: []*structs.Task{{Name: "t1"}, {Name: "t2"}}, + }, "s1", "t2") + require.NoError(t, err) + require.Equal(t, "t2", task.Name) }) - t.Run("no task for service", func(t *testing.T) { - require.EqualError(t, groupConnectNativeValidate(&structs.TaskGroup{ - Name: "g2", - }, &structs.Service{ - Name: "s2", - TaskName: "t1", - }), `Consul Connect Native service "s2" requires undefined task "t1" in group "g2"`) + t.Run("infer exists", func(t *testing.T) { + task, err := getNamedTaskForNativeService(&structs.TaskGroup{ + Name: "g1", + Tasks: []*structs.Task{{Name: "t2"}}, + }, "s1", "") + require.NoError(t, err) + require.Equal(t, "t2", task.Name) }) - t.Run("native okay", func(t *testing.T) { - require.NoError(t, groupConnectNativeValidate(&structs.TaskGroup{ - Name: "g2", - Tasks: []*structs.Task{{Name: "t0"}, {Name: "t1"}, {Name: "t3"}}, - }, &structs.Service{ - Name: "s2", - TaskName: "t1", - })) + t.Run("infer ambiguous", func(t *testing.T) { + task, err := getNamedTaskForNativeService(&structs.TaskGroup{ + Name: "g1", + Tasks: []*structs.Task{{Name: "t1"}, {Name: "t2"}}, + }, "s1", "") + require.EqualError(t, err, "task for Consul Connect Native service g1->s1 is ambiguous and must be set") + require.Nil(t, task) + }) + + t.Run("named absent", func(t *testing.T) { + task, err := getNamedTaskForNativeService(&structs.TaskGroup{ + Name: "g1", + Tasks: []*structs.Task{{Name: "t1"}, {Name: "t2"}}, + }, "s1", "t3") + require.EqualError(t, err, "task t3 named by Consul Connect Native service g1->s1 does not exist") + require.Nil(t, task) }) } diff --git a/nomad/structs/services.go b/nomad/structs/services.go index 5367e273159..8e3ad01f65b 100644 --- a/nomad/structs/services.go +++ b/nomad/structs/services.go @@ -481,7 +481,8 @@ func (s *Service) Validate() error { mErr.Errors = append(mErr.Errors, err) } - // if service is connect native, service task must be set + // if service is connect native, service task must be set (which may + // happen implicitly in a job mutation if there is only one task) if s.Connect.IsNative() && len(s.TaskName) == 0 { mErr.Errors = append(mErr.Errors, fmt.Errorf("Service %s is Connect Native and requires setting the task", s.Name)) } diff --git a/website/pages/docs/job-specification/service.mdx b/website/pages/docs/job-specification/service.mdx index d3148b570b2..c38910797f2 100644 --- a/website/pages/docs/job-specification/service.mdx +++ b/website/pages/docs/job-specification/service.mdx @@ -149,8 +149,8 @@ Connect][connect] integration. - `task` `(string: "")` - Specifies the name of the Nomad Task associated with this service definition. Only available on group services. Must be set if this - service definition represents a Consul Connect Native service. The Nomad Task - must exist in the same Task Group. + service definition represents a Consul Connect Native service and there is more + than one task in the task group. - `meta` ([Meta][]: nil) - Specifies a key-value map that annotates the Consul service with user-defined metadata.