Skip to content

Commit

Permalink
Support disabling TCP checks for connect sidecar services
Browse files Browse the repository at this point in the history
  • Loading branch information
Mahmood Ali committed May 6, 2021
1 parent 78f26c0 commit 23ffa84
Show file tree
Hide file tree
Showing 10 changed files with 135 additions and 57 deletions.
7 changes: 4 additions & 3 deletions api/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,10 @@ func (cc *ConsulConnect) Canonicalize() {
// ConsulSidecarService represents a Consul Connect SidecarService jobspec
// stanza.
type ConsulSidecarService struct {
Tags []string `hcl:"tags,optional"`
Port string `hcl:"port,optional"`
Proxy *ConsulProxy `hcl:"proxy,block"`
Tags []string `hcl:"tags,optional"`
Port string `hcl:"port,optional"`
Proxy *ConsulProxy `hcl:"proxy,block"`
DisableDefaultTCPCheck bool `mapstructure:"disable_default_tcp_check" hcl:"disable_default_tcp_check,optional"`
}

func (css *ConsulSidecarService) Canonicalize() {
Expand Down
27 changes: 20 additions & 7 deletions command/agent/consul/connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,12 +105,9 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ
return nil, err
}

return &api.AgentServiceRegistration{
Tags: helper.CopySliceString(css.Tags),
Port: cMapping.Value,
Address: cMapping.HostIP,
Proxy: proxy,
Checks: api.AgentServiceChecks{
var checks api.AgentServiceChecks
if !css.DisableDefaultTCPCheck {
checks = api.AgentServiceChecks{
{
Name: "Connect Sidecar Listening",
TCP: net.JoinHostPort(cMapping.HostIP, strconv.Itoa(cMapping.Value)),
Expand All @@ -120,7 +117,23 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ
Name: "Connect Sidecar Aliasing " + serviceId,
AliasService: serviceId,
},
},
}
} else {
// insert a NOOP check to avoid Consul inserting the default
checks = api.AgentServiceChecks{
{
Name: "Connect Sidecar Aliasing " + serviceId,
AliasService: serviceId,
},
}
}

return &api.AgentServiceRegistration{
Tags: helper.CopySliceString(css.Tags),
Port: cMapping.Value,
Address: cMapping.HostIP,
Proxy: proxy,
Checks: checks,
}, nil
}

Expand Down
29 changes: 29 additions & 0 deletions command/agent/consul/connect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,35 @@ func TestConnect_newConnect(t *testing.T) {
},
}, asr.SidecarService)
})

t.Run("with sidecar without TCP checks", func(t *testing.T) {
asr, err := newConnect("redis-service-id", "redis", &structs.ConsulConnect{
Native: false,
SidecarService: &structs.ConsulSidecarService{
Tags: []string{"foo", "bar"},
Port: "connect-proxy-redis",
DisableDefaultTCPCheck: true,
},
}, testConnectNetwork, testConnectPorts)
require.NoError(t, err)
require.Equal(t, &api.AgentServiceRegistration{
Tags: []string{"foo", "bar"},
Port: 3000,
Address: "192.168.30.1",
Proxy: &api.AgentServiceConnectProxyConfig{
Config: map[string]interface{}{
"bind_address": "0.0.0.0",
"bind_port": 3000,
},
},
Checks: api.AgentServiceChecks{
{
Name: "Connect Sidecar Aliasing redis-service-id",
AliasService: "redis-service-id",
},
},
}, asr.SidecarService)
})
}

func TestConnect_connectSidecarRegistration(t *testing.T) {
Expand Down
7 changes: 4 additions & 3 deletions command/agent/job_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -1499,9 +1499,10 @@ func apiConnectSidecarServiceToStructs(in *api.ConsulSidecarService) *structs.Co
return nil
}
return &structs.ConsulSidecarService{
Port: in.Port,
Tags: helper.CopySliceString(in.Tags),
Proxy: apiConnectSidecarServiceProxyToStructs(in.Proxy),
Port: in.Port,
Tags: helper.CopySliceString(in.Tags),
Proxy: apiConnectSidecarServiceProxyToStructs(in.Proxy),
DisableDefaultTCPCheck: in.DisableDefaultTCPCheck,
}
}

Expand Down
10 changes: 6 additions & 4 deletions command/agent/job_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2035,8 +2035,9 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
Connect: &api.ConsulConnect{
Native: false,
SidecarService: &api.ConsulSidecarService{
Tags: []string{"f", "g"},
Port: "9000",
Tags: []string{"f", "g"},
Port: "9000",
DisableDefaultTCPCheck: true,
},
},
},
Expand Down Expand Up @@ -2414,8 +2415,9 @@ func TestJobs_ApiJobToStructsJob(t *testing.T) {
Connect: &structs.ConsulConnect{
Native: false,
SidecarService: &structs.ConsulSidecarService{
Tags: []string{"f", "g"},
Port: "9000",
Tags: []string{"f", "g"},
Port: "9000",
DisableDefaultTCPCheck: true,
},
},
},
Expand Down
1 change: 1 addition & 0 deletions jobspec/parse_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ func parseSidecarService(o *ast.ObjectItem) (*api.ConsulSidecarService, error) {
"port",
"proxy",
"tags",
"disable_default_tcp_check",
}

if err := checkHCLKeys(o.Val, valid); err != nil {
Expand Down
52 changes: 33 additions & 19 deletions jobspec/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ package jobspec

import (
"path/filepath"
"reflect"
"strings"
"testing"
"time"

capi "github.com/hashicorp/consul/api"
"github.com/hashicorp/nomad/api"
"github.com/kr/pretty"
"github.com/stretchr/testify/require"
)

// consts copied from nomad/structs package to keep jobspec isolated from rest of nomad
Expand Down Expand Up @@ -1253,6 +1252,27 @@ func TestParse(t *testing.T) {
},
false,
},
{
"tg-service-connect-sidecar_disablecheck.hcl",
&api.Job{
ID: stringToPtr("sidecar_disablecheck"),
Name: stringToPtr("sidecar_disablecheck"),
Type: stringToPtr("service"),
TaskGroups: []*api.TaskGroup{{
Name: stringToPtr("group"),
Services: []*api.Service{{
Name: "example",
Connect: &api.ConsulConnect{
Native: false,
SidecarService: &api.ConsulSidecarService{
DisableDefaultTCPCheck: true,
},
},
}},
}},
},
false,
},
{
"tg-service-connect-proxy.hcl",
&api.Job{
Expand Down Expand Up @@ -1608,26 +1628,20 @@ func TestParse(t *testing.T) {
}

for _, tc := range cases {
t.Logf("Testing parse: %s", tc.File)

path, err := filepath.Abs(filepath.Join("./test-fixtures", tc.File))
if err != nil {
t.Fatalf("file: %s\n\n%s", tc.File, err)
continue
}
t.Run(tc.File, func(t *testing.T) {
t.Logf("Testing parse: %s", tc.File)

actual, err := ParseFile(path)
if (err != nil) != tc.Err {
t.Fatalf("file: %s\n\n%s", tc.File, err)
continue
}
path, err := filepath.Abs(filepath.Join("./test-fixtures", tc.File))
require.NoError(t, err)

if !reflect.DeepEqual(actual, tc.Result) {
for _, d := range pretty.Diff(actual, tc.Result) {
t.Logf(d)
actual, err := ParseFile(path)
if tc.Err {
require.Error(t, err)
} else {
require.NoError(t, err)
require.Equal(t, tc.Result, actual)
}
t.Fatalf("file: %s", tc.File)
}
})
}
}

Expand Down
37 changes: 22 additions & 15 deletions nomad/structs/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3044,6 +3044,12 @@ func TestTaskGroupDiff(t *testing.T) {
Type: DiffTypeAdded,
Name: "SidecarService",
Fields: []*FieldDiff{
{
Type: DiffTypeAdded,
Name: "DisableDefaultTCPCheck",
Old: "",
New: "false",
},
{
Type: DiffTypeAdded,
Name: "Port",
Expand Down Expand Up @@ -3884,8 +3890,7 @@ func TestTaskGroupDiff(t *testing.T) {
require.Error(t, err, "case %q expected error", c.TestCase)
case false:
require.NoError(t, err, "case %q expected no error", c.TestCase)
require.True(t, reflect.DeepEqual(result, c.Expected),
"case %q got\n%#v\nwant:\n%#v\n", c.TestCase, result, c.Expected)
require.Equal(t, c.Expected, result)
}
})
}
Expand Down Expand Up @@ -5766,6 +5771,14 @@ func TestTaskDiff(t *testing.T) {
{
Type: DiffTypeAdded,
Name: "SidecarService",
Fields: []*FieldDiff{
{
Type: DiffTypeAdded,
Name: "DisableDefaultTCPCheck",
Old: "",
New: "false",
},
},
},
},
},
Expand Down Expand Up @@ -7149,20 +7162,14 @@ func TestTaskDiff(t *testing.T) {

for i, c := range cases {
t.Run(c.Name, func(t *testing.T) {
actual, err := c.Old.Diff(c.New, c.Contextual)
if c.Error && err == nil {
t.Fatalf("case %d: expected errored", i+1)
} else if err != nil {
if !c.Error {
t.Fatalf("case %d: errored %#v", i+1, err)
} else {
return
}
}
t.Logf("running case: %d %v", i, c.Name)

if !reflect.DeepEqual(actual, c.Expected) {
t.Errorf("case %d: got:\n%#v\n want:\n%#v\n",
i+1, actual, c.Expected)
actual, err := c.Old.Diff(c.New, c.Contextual)
if c.Error {
require.Error(t, err)
} else {
require.NoError(t, err)
require.Equal(t, c.Expected, actual)
}
})
}
Expand Down
15 changes: 12 additions & 3 deletions nomad/structs/services.go
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,10 @@ type ConsulSidecarService struct {

// Proxy stanza defining the sidecar proxy configuration.
Proxy *ConsulProxy

// DisableDefaultTCPCheck, if true, instructs Nomad to avoid setting a
// default TCP check for the sidecar service.
DisableDefaultTCPCheck bool
}

// HasUpstreams checks if the sidecar service has any upstreams configured
Expand All @@ -897,9 +901,10 @@ func (s *ConsulSidecarService) Copy() *ConsulSidecarService {
return nil
}
return &ConsulSidecarService{
Tags: helper.CopySliceString(s.Tags),
Port: s.Port,
Proxy: s.Proxy.Copy(),
Tags: helper.CopySliceString(s.Tags),
Port: s.Port,
Proxy: s.Proxy.Copy(),
DisableDefaultTCPCheck: s.DisableDefaultTCPCheck,
}
}

Expand All @@ -913,6 +918,10 @@ func (s *ConsulSidecarService) Equals(o *ConsulSidecarService) bool {
return false
}

if s.DisableDefaultTCPCheck != o.DisableDefaultTCPCheck {
return false
}

if !helper.CompareSliceSetString(s.Tags, o.Tags) {
return false
}
Expand Down
7 changes: 4 additions & 3 deletions vendor/github.com/hashicorp/nomad/api/services.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 23ffa84

Please sign in to comment.