Skip to content

Commit

Permalink
consul/connect: Validate uniqueness of Connect upstreams within task …
Browse files Browse the repository at this point in the history 
…group

This PR adds validation during job submission that Connect proxy upstreams
within a task group are using different listener addresses. Otherwise, a
duplicate envoy listener will be created and not be able to bind.

Closes #7833
  • Loading branch information
@shoenig
shoenig committed Jun 18, 2021
1 parent 6dcada4 commit adcbcc1
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
39 changes: 31 additions & 8 deletions nomad/job_endpoint_hook_connect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
}
}

Expand Down Expand Up @@ -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 {
Expand Down
117 changes: 112 additions & 5 deletions nomad/job_endpoint_hook_connect_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{{
Expand All @@ -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{{
Expand All @@ -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{{
Expand All @@ -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{{
Expand All @@ -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{{
Expand All @@ -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) {
Expand Down

0 comments on commit adcbcc1

Please sign in to comment.