diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d894c9bc52..bca02e88909 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ IMPROVEMENTS: * cli: Added `-monitor` flag to `deployment status` command and automatically monitor deployments from `job run` command. [[GH-10661](https://github.com/hashicorp/nomad/pull/10661)] +* consul/connect: Validate Connect service upstream address uniqueness within task group [[GH-7833](https://github.com/hashicorp/nomad/issues/7833)] * docker: Tasks using `network.mode = "bridge"` that don't set their `network_mode` will receive a `/etc/hosts` file that includes the pause container's hostname and any `extra_hosts`. [[GH-10766](https://github.com/hashicorp/nomad/issues/10766)] BUG FIXES: diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index 9c319e10688..1fb961b9f07 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -127,10 +127,8 @@ func (jobConnectHook) Validate(job *structs.Job) ([]error, error) { var warnings []error for _, g := range job.TaskGroups { - if w, err := groupConnectValidate(g); err != nil { + if err := groupConnectValidate(g); err != nil { return nil, err - } else if w != nil { - warnings = append(warnings, w...) } } @@ -480,24 +478,49 @@ func newConnectSidecarTask(service string) *structs.Task { } } -func groupConnectValidate(g *structs.TaskGroup) (warnings []error, err error) { +func groupConnectValidate(g *structs.TaskGroup) error { for _, s := range g.Services { switch { case s.Connect.HasSidecar(): if err := groupConnectSidecarValidate(g, s); err != nil { - return nil, err + return err } case s.Connect.IsNative(): if err := groupConnectNativeValidate(g, s); err != nil { - return nil, err + return err } case s.Connect.IsGateway(): if err := groupConnectGatewayValidate(g); err != nil { - return nil, err + return err } } } - return nil, nil + + if err := groupConnectUpstreamsValidate(g.Name, g.Services); err != nil { + return err + } + + return nil +} + +func groupConnectUpstreamsValidate(group string, services []*structs.Service) error { + listeners := make(map[string]string) // address -> service + + for _, service := range services { + if service.Connect.HasSidecar() && service.Connect.SidecarService.Proxy != nil { + for _, up := range service.Connect.SidecarService.Proxy.Upstreams { + listener := fmt.Sprintf("%s:%d", up.LocalBindAddress, up.LocalBindPort) + if s, exists := listeners[listener]; exists { + return fmt.Errorf( + "Consul Connect services %q and %q in group %q using same address for upstreams (%s)", + service.Name, s, group, listener, + ) + } + listeners[listener] = service.Name + } + } + } + return nil } func groupConnectSidecarValidate(g *structs.TaskGroup, s *structs.Service) error { diff --git a/nomad/job_endpoint_hook_connect_test.go b/nomad/job_endpoint_hook_connect_test.go index 6b107163b6b..14d49aa071a 100644 --- a/nomad/job_endpoint_hook_connect_test.go +++ b/nomad/job_endpoint_hook_connect_test.go @@ -348,7 +348,7 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { // group and service name validation t.Run("non-connect service contains uppercase characters", func(t *testing.T) { - _, err := groupConnectValidate(&structs.TaskGroup{ + err := groupConnectValidate(&structs.TaskGroup{ Name: "group", Networks: structs.Networks{{Mode: "bridge"}}, Services: []*structs.Service{{ @@ -359,7 +359,7 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { }) t.Run("connect service contains uppercase characters", func(t *testing.T) { - _, err := groupConnectValidate(&structs.TaskGroup{ + err := groupConnectValidate(&structs.TaskGroup{ Name: "group", Networks: structs.Networks{{Mode: "bridge"}}, Services: []*structs.Service{{ @@ -370,7 +370,7 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { }) t.Run("non-connect group contains uppercase characters", func(t *testing.T) { - _, err := groupConnectValidate(&structs.TaskGroup{ + err := groupConnectValidate(&structs.TaskGroup{ Name: "Other-Group", Networks: structs.Networks{{Mode: "bridge"}}, Services: []*structs.Service{{ @@ -381,7 +381,7 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { }) t.Run("connect-group contains uppercase characters", func(t *testing.T) { - _, err := groupConnectValidate(&structs.TaskGroup{ + err := groupConnectValidate(&structs.TaskGroup{ Name: "Connect-Group", Networks: structs.Networks{{Mode: "bridge"}}, Services: []*structs.Service{{ @@ -392,7 +392,7 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { }) t.Run("connect group and service lowercase", func(t *testing.T) { - _, err := groupConnectValidate(&structs.TaskGroup{ + err := groupConnectValidate(&structs.TaskGroup{ Name: "connect-group", Networks: structs.Networks{{Mode: "bridge"}}, Services: []*structs.Service{{ @@ -401,6 +401,113 @@ func TestJobEndpointConnect_groupConnectSidecarValidate(t *testing.T) { }) require.NoError(t, err) }) + + t.Run("connect group overlap upstreams", func(t *testing.T) { + s1 := makeService("s1") + s2 := makeService("s2") + s1.Connect.SidecarService.Proxy = &structs.ConsulProxy{ + Upstreams: []structs.ConsulUpstream{{ + LocalBindPort: 8999, + }}, + } + s2.Connect.SidecarService.Proxy = &structs.ConsulProxy{ + Upstreams: []structs.ConsulUpstream{{ + LocalBindPort: 8999, + }}, + } + err := groupConnectValidate(&structs.TaskGroup{ + Name: "connect-group", + Networks: structs.Networks{{Mode: "bridge"}}, + Services: []*structs.Service{s1, s2}, + }) + require.EqualError(t, err, `Consul Connect services "s2" and "s1" in group "connect-group" using same address for upstreams (:8999)`) + }) +} + +func TestJobEndpointConnect_groupConnectUpstreamsValidate(t *testing.T) { + t.Run("no connect services", func(t *testing.T) { + err := groupConnectUpstreamsValidate("group", + []*structs.Service{{Name: "s1"}, {Name: "s2"}}) + require.NoError(t, err) + }) + + t.Run("connect services no overlap", func(t *testing.T) { + err := groupConnectUpstreamsValidate("group", + []*structs.Service{ + { + Name: "s1", + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{ + Upstreams: []structs.ConsulUpstream{{ + LocalBindAddress: "127.0.0.1", + LocalBindPort: 9001, + }, { + LocalBindAddress: "127.0.0.1", + LocalBindPort: 9002, + }}, + }, + }, + }, + }, + { + Name: "s2", + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{ + Upstreams: []structs.ConsulUpstream{{ + LocalBindAddress: "10.0.0.1", + LocalBindPort: 9001, + }, { + LocalBindAddress: "127.0.0.1", + LocalBindPort: 9003, + }}, + }, + }, + }, + }, + }) + require.NoError(t, err) + }) + + t.Run("connect services overlap port", func(t *testing.T) { + err := groupConnectUpstreamsValidate("group", + []*structs.Service{ + { + Name: "s1", + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{ + Upstreams: []structs.ConsulUpstream{{ + LocalBindAddress: "127.0.0.1", + LocalBindPort: 9001, + }, { + LocalBindAddress: "127.0.0.1", + LocalBindPort: 9002, + }}, + }, + }, + }, + }, + { + Name: "s2", + Connect: &structs.ConsulConnect{ + SidecarService: &structs.ConsulSidecarService{ + Proxy: &structs.ConsulProxy{ + Upstreams: []structs.ConsulUpstream{{ + LocalBindAddress: "127.0.0.1", + LocalBindPort: 9002, + }, { + LocalBindAddress: "127.0.0.1", + LocalBindPort: 9003, + }}, + }, + }, + }, + }, + }) + require.EqualError(t, err, `Consul Connect services "s2" and "s1" in group "group" using same address for upstreams (127.0.0.1:9002)`) + }) } func TestJobEndpointConnect_getNamedTaskForNativeService(t *testing.T) {