Support disabling TCP checks for connect sidecar services

hashicorp · May 7, 2021 · d8e4060 · d8e4060
1 parent 72bab51
commit d8e4060
Show file tree

Hide file tree

Showing 11 changed files with 150 additions and 65 deletions.
diff --git a/api/services.go b/api/services.go
@@ -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() {

diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go
@@ -105,22 +105,27 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ
 		return nil, err
 	}
 
+	// if the service has a TCP check that's failing, we need an alias to
+	// ensure service discovery excludes this sidecar from queries
+	// (ex. in the case of Connect upstreams)
+	checks := api.AgentServiceChecks{{
+		Name:         "Connect Sidecar Aliasing " + serviceId,
+		AliasService: serviceId,
+	}}
+	if !css.DisableDefaultTCPCheck {
+		checks = append(checks, &api.AgentServiceCheck{
+			Name:     "Connect Sidecar Listening",
+			TCP:      net.JoinHostPort(cMapping.HostIP, strconv.Itoa(cMapping.Value)),
+			Interval: "10s",
+		})
+	}
+
 	return &api.AgentServiceRegistration{
 		Tags:    helper.CopySliceString(css.Tags),
 		Port:    cMapping.Value,
 		Address: cMapping.HostIP,
 		Proxy:   proxy,
-		Checks: api.AgentServiceChecks{
-			{
-				Name:     "Connect Sidecar Listening",
-				TCP:      net.JoinHostPort(cMapping.HostIP, strconv.Itoa(cMapping.Value)),
-				Interval: "10s",
-			},
-			{
-				Name:         "Connect Sidecar Aliasing " + serviceId,
-				AliasService: serviceId,
-			},
-		},
+		Checks:  checks,
 	}, nil
 }
 

diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go
@@ -67,11 +67,40 @@ func TestConnect_newConnect(t *testing.T) {
 				},
 			},
 			Checks: api.AgentServiceChecks{
+				{
+					Name:         "Connect Sidecar Aliasing redis-service-id",
+					AliasService: "redis-service-id",
+				},
 				{
 					Name:     "Connect Sidecar Listening",
 					TCP:      "192.168.30.1:3000",
 					Interval: "10s",
 				},
+			},
+		}, 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",
@@ -128,15 +157,15 @@ func TestConnect_connectSidecarRegistration(t *testing.T) {
 				},
 			},
 			Checks: api.AgentServiceChecks{
+				{
+					Name:         "Connect Sidecar Aliasing redis-service-id",
+					AliasService: "redis-service-id",
+				},
 				{
 					Name:     "Connect Sidecar Listening",
 					TCP:      "192.168.30.1:3000",
 					Interval: "10s",
 				},
-				{
-					Name:         "Connect Sidecar Aliasing redis-service-id",
-					AliasService: "redis-service-id",
-				},
 			},
 		}, proxy)
 	})

diff --git a/command/agent/job_endpoint.go b/command/agent/job_endpoint.go
@@ -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,
 	}
 }
 

diff --git a/command/agent/job_endpoint_test.go b/command/agent/job_endpoint_test.go
@@ -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,
 							},
 						},
 					},
@@ -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,
 							},
 						},
 					},

diff --git a/jobspec/parse_service.go b/jobspec/parse_service.go
@@ -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 {

diff --git a/jobspec/parse_test.go b/jobspec/parse_test.go
@@ -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
@@ -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{
@@ -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)
-		}
+		})
 	}
 }
 

diff --git a/jobspec/test-fixtures/tg-service-connect-sidecar_disablecheck.hcl b/jobspec/test-fixtures/tg-service-connect-sidecar_disablecheck.hcl
@@ -0,0 +1,15 @@
+job "sidecar_disablecheck" {
+  type = "service"
+
+  group "group" {
+    service {
+      name = "example"
+
+      connect {
+        sidecar_service {
+          disable_default_tcp_check = true
+        }
+      }
+    }
+  }
+}
diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go
@@ -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",
@@ -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)
 			}
 		})
 	}
@@ -5766,6 +5771,14 @@ func TestTaskDiff(t *testing.T) {
 									{
 										Type: DiffTypeAdded,
 										Name: "SidecarService",
+										Fields: []*FieldDiff{
+											{
+												Type: DiffTypeAdded,
+												Name: "DisableDefaultTCPCheck",
+												Old:  "",
+												New:  "false",
+											},
+										},
 									},
 								},
 							},
@@ -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)
 			}
 		})
 	}

diff --git a/nomad/structs/services.go b/nomad/structs/services.go
@@ -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
@@ -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,
 	}
 }
 
@@ -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
 	}

diff --git a/vendor/github.com/hashicorp/nomad/api/services.go b/vendor/github.com/hashicorp/nomad/api/services.go
-Original file line number
+Diff line change
@@ Expand Up @@
     		"port",
     		"proxy",
     		"tags",
+    		"disable_default_tcp_check",
     	}
     	if err := checkHCLKeys(o.Val, valid); err != nil {
@@ Expand Down @@