Merge pull request #9975 from AndrewChubatiuk/connect-host-networks-fix

Updated Default Consul Sidecar Healthcheck

client/allocrunner/taskrunner/envoy_bootstrap_hook.go

@@ -278,7 +278,8 @@ func buildEnvoyAdminBind(alloc *structs.Allocation, serviceName, taskName string
 	case "host":
 		for _, service := range tg.Services {
 			if service.Name == serviceName {
-				_, port = tg.Networks.Port(service.PortLabel)
+				mapping := tg.Networks.Port(service.PortLabel)
+				port = mapping.Value
 				break
 			}
 		}

command/agent/consul/connect.go

@@ -2,6 +2,8 @@ package consul
 
 import (
 	"fmt"
+	"net"
+	"strconv"
 
 	"github.com/hashicorp/consul/api"
 	"github.com/hashicorp/nomad/helper"
@@ -11,7 +13,7 @@ import (
 // newConnect creates a new Consul AgentServiceConnect struct based on a Nomad
 // Connect struct. If the nomad Connect struct is nil, nil will be returned to
 // disable Connect for this service.
-func newConnect(serviceName string, nc *structs.ConsulConnect, networks structs.Networks) (*api.AgentServiceConnect, error) {
+func newConnect(serviceId string, serviceName string, nc *structs.ConsulConnect, networks structs.Networks, ports structs.AllocatedPorts) (*api.AgentServiceConnect, error) {
 	switch {
 	case nc == nil:
 		// no connect stanza means there is no connect service to register
@@ -27,7 +29,10 @@ func newConnect(serviceName string, nc *structs.ConsulConnect, networks structs.
 
 	case nc.HasSidecar():
 		// must register the sidecar for this service
-		sidecarReg, err := connectSidecarRegistration(serviceName, nc.SidecarService, networks)
+		if nc.SidecarService.Port == "" {
+			nc.SidecarService.Port = fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, serviceName)
+		}
+		sidecarReg, err := connectSidecarRegistration(serviceId, nc.SidecarService, networks, ports)
 		if err != nil {
 			return nil, err
 		}
@@ -84,27 +89,38 @@ func newConnectGateway(serviceName string, connect *structs.ConsulConnect) *api.
 	return &api.AgentServiceConnectProxyConfig{Config: envoyConfig}
 }
 
-func connectSidecarRegistration(serviceName string, css *structs.ConsulSidecarService, networks structs.Networks) (*api.AgentServiceRegistration, error) {
+func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarService, networks structs.Networks, ports structs.AllocatedPorts) (*api.AgentServiceRegistration, error) {
 	if css == nil {
 		// no sidecar stanza means there is no sidecar service to register
 		return nil, nil
 	}
 
-	cNet, cPort, err := connectPort(serviceName, networks)
+	cMapping, err := connectPort(css.Port, networks, ports)
 	if err != nil {
 		return nil, err
 	}
 
-	proxy, err := connectSidecarProxy(css.Proxy, cPort.To, networks)
+	proxy, err := connectSidecarProxy(css.Proxy, cMapping.To, networks)
 	if err != nil {
 		return nil, err
 	}
 
 	return &api.AgentServiceRegistration{
 		Tags:    helper.CopySliceString(css.Tags),
-		Port:    cPort.Value,
-		Address: cNet.IP,
+		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,
+			},
+		},
 	}, nil
 }
 
@@ -200,17 +216,19 @@ func connectNetworkInvariants(networks structs.Networks) error {
 // connectPort returns the network and port for the Connect proxy sidecar
 // defined for this service. An error is returned if the network and port
 // cannot be determined.
-func connectPort(serviceName string, networks structs.Networks) (*structs.NetworkResource, structs.Port, error) {
+func connectPort(portLabel string, networks structs.Networks, ports structs.AllocatedPorts) (structs.AllocatedPortMapping, error) {
 	if err := connectNetworkInvariants(networks); err != nil {
-		return nil, structs.Port{}, err
+		return structs.AllocatedPortMapping{}, err
 	}
-
-	port, ok := networks[0].PortForService(serviceName)
+	mapping, ok := ports.Get(portLabel)
 	if !ok {
-		return nil, structs.Port{}, fmt.Errorf("No Connect port defined for service %q", serviceName)
+		mapping = networks.Port(portLabel)
+		if mapping.Value > 0 {
+			return mapping, nil
+		}
+		return structs.AllocatedPortMapping{}, fmt.Errorf("No port of label %q defined", portLabel)
 	}
-
-	return networks[0], port, nil
+	return mapping, nil
 }
 
 // connectExposePathPort returns the port for the exposed path for the exposed
@@ -220,10 +238,10 @@ func connectExposePathPort(portLabel string, networks structs.Networks) (string,
 		return "", 0, err
 	}
 
-	ip, port := networks.Port(portLabel)
-	if port == 0 {
+	mapping := networks.Port(portLabel)
+	if mapping.Value == 0 {
 		return "", 0, fmt.Errorf("No port of label %q defined", portLabel)
 	}
 
-	return ip, port, nil
+	return mapping.HostIP, mapping.Value, nil
 }

command/agent/consul/connect_test.go

@@ -21,34 +21,40 @@ var (
 			{Label: "connect-proxy-redis", Value: 3000, To: 3000},
 		},
 	}}
+	testConnectPorts = structs.AllocatedPorts{{
+		Label:  "connect-proxy-redis",
+		Value:  3000,
+		To:     3000,
+		HostIP: "192.168.30.1",
+	}}
 )
 
 func TestConnect_newConnect(t *testing.T) {
 	t.Parallel()
 
 	t.Run("nil", func(t *testing.T) {
-		asr, err := newConnect("", nil, nil)
+		asr, err := newConnect("", "", nil, nil, nil)
 		require.NoError(t, err)
 		require.Nil(t, asr)
 	})
 
 	t.Run("native", func(t *testing.T) {
-		asr, err := newConnect("", &structs.ConsulConnect{
+		asr, err := newConnect("", "", &structs.ConsulConnect{
 			Native: true,
-		}, nil)
+		}, nil, nil)
 		require.NoError(t, err)
 		require.True(t, asr.Native)
 		require.Nil(t, asr.SidecarService)
 	})
 
 	t.Run("with sidecar", func(t *testing.T) {
-		asr, err := newConnect("redis", &structs.ConsulConnect{
+		asr, err := newConnect("redis-service-id", "redis", &structs.ConsulConnect{
 			Native: false,
 			SidecarService: &structs.ConsulSidecarService{
 				Tags: []string{"foo", "bar"},
-				Port: "sidecarPort",
+				Port: "connect-proxy-redis",
 			},
-		}, testConnectNetwork)
+		}, testConnectNetwork, testConnectPorts)
 		require.NoError(t, err)
 		require.Equal(t, &api.AgentServiceRegistration{
 			Tags:    []string{"foo", "bar"},
@@ -60,6 +66,17 @@ func TestConnect_newConnect(t *testing.T) {
 					"bind_port":    3000,
 				},
 			},
+			Checks: api.AgentServiceChecks{
+				{
+					Name:     "Connect Sidecar Listening",
+					TCP:      "192.168.30.1:3000",
+					Interval: "10s",
+				},
+				{
+					Name:         "Connect Sidecar Aliasing redis-service-id",
+					AliasService: "redis-service-id",
+				},
+			},
 		}, asr.SidecarService)
 	})
 }
@@ -68,36 +85,37 @@ func TestConnect_connectSidecarRegistration(t *testing.T) {
 	t.Parallel()
 
 	t.Run("nil", func(t *testing.T) {
-		sidecarReg, err := connectSidecarRegistration("", nil, testConnectNetwork)
+		sidecarReg, err := connectSidecarRegistration("", nil, testConnectNetwork, testConnectPorts)
 		require.NoError(t, err)
 		require.Nil(t, sidecarReg)
 	})
 
 	t.Run("no service port", func(t *testing.T) {
-		_, err := connectSidecarRegistration("unknown", &structs.ConsulSidecarService{
-			// irrelevant
-		}, testConnectNetwork)
-		require.EqualError(t, err, `No Connect port defined for service "unknown"`)
+		_, err := connectSidecarRegistration("unknown-id", &structs.ConsulSidecarService{
+			Port: "unknown-label",
+		}, testConnectNetwork, testConnectPorts)
+		require.EqualError(t, err, `No port of label "unknown-label" defined`)
 	})
 
 	t.Run("bad proxy", func(t *testing.T) {
-		_, err := connectSidecarRegistration("redis", &structs.ConsulSidecarService{
+		_, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{
+			Port: "connect-proxy-redis",
 			Proxy: &structs.ConsulProxy{
 				Expose: &structs.ConsulExposeConfig{
 					Paths: []structs.ConsulExposePath{{
 						ListenerPort: "badPort",
 					}},
 				},
 			},
-		}, testConnectNetwork)
+		}, testConnectNetwork, testConnectPorts)
 		require.EqualError(t, err, `No port of label "badPort" defined`)
 	})
 
 	t.Run("normal", func(t *testing.T) {
-		proxy, err := connectSidecarRegistration("redis", &structs.ConsulSidecarService{
+		proxy, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{
 			Tags: []string{"foo", "bar"},
-			Port: "sidecarPort",
-		}, testConnectNetwork)
+			Port: "connect-proxy-redis",
+		}, testConnectNetwork, testConnectPorts)
 		require.NoError(t, err)
 		require.Equal(t, &api.AgentServiceRegistration{
 			Tags:    []string{"foo", "bar"},
@@ -109,6 +127,17 @@ func TestConnect_connectSidecarRegistration(t *testing.T) {
 					"bind_port":    3000,
 				},
 			},
+			Checks: api.AgentServiceChecks{
+				{
+					Name:     "Connect Sidecar Listening",
+					TCP:      "192.168.30.1:3000",
+					Interval: "10s",
+				},
+				{
+					Name:         "Connect Sidecar Aliasing redis-service-id",
+					AliasService: "redis-service-id",
+				},
+			},
 		}, proxy)
 	})
 }
@@ -334,32 +363,39 @@ func TestConnect_getConnectPort(t *testing.T) {
 			To:    23456,
 		}}}}
 
+	ports := structs.AllocatedPorts{{
+		Label:  "foo",
+		Value:  23456,
+		To:     23456,
+		HostIP: "192.168.30.1",
+	}}
+
 	t.Run("normal", func(t *testing.T) {
-		nr, port, err := connectPort("foo", networks)
+		nr, err := connectPort("foo", networks, ports)
 		require.NoError(t, err)
-		require.Equal(t, structs.Port{
-			Label: "connect-proxy-foo",
-			Value: 23456,
-			To:    23456,
-		}, port)
-		require.Equal(t, "192.168.30.1", nr.IP)
+		require.Equal(t, structs.AllocatedPortMapping{
+			Label:  "foo",
+			Value:  23456,
+			To:     23456,
+			HostIP: "192.168.30.1",
+		}, nr)
 	})
 
 	t.Run("no such service", func(t *testing.T) {
-		_, _, err := connectPort("other", networks)
-		require.EqualError(t, err, `No Connect port defined for service "other"`)
+		_, err := connectPort("other", networks, ports)
+		require.EqualError(t, err, `No port of label "other" defined`)
 	})
 
 	t.Run("no network", func(t *testing.T) {
-		_, _, err := connectPort("foo", nil)
+		_, err := connectPort("foo", nil, nil)
 		require.EqualError(t, err, "Connect only supported with exactly 1 network (found 0)")
 	})
 
 	t.Run("multi network", func(t *testing.T) {
-		_, _, err := connectPort("foo", append(networks, &structs.NetworkResource{
+		_, err := connectPort("foo", append(networks, &structs.NetworkResource{
 			Device: "eth1",
 			IP:     "10.0.10.0",
-		}))
+		}), nil)
 		require.EqualError(t, err, "Connect only supported with exactly 1 network (found 2)")
 	})
 }

command/agent/consul/service_client.go

@@ -866,7 +866,7 @@ func (c *ServiceClient) serviceRegs(ops *operations, service *structs.Service, w
 	}
 
 	// newConnect returns (nil, nil) if there's no Connect-enabled service.
-	connect, err := newConnect(service.Name, service.Connect, workload.Networks)
+	connect, err := newConnect(id, service.Name, service.Connect, workload.Networks, workload.Ports)
 	if err != nil {
 		return nil, fmt.Errorf("invalid Consul Connect configuration for service %q: %v", service.Name, err)
 	}
@@ -1500,9 +1500,9 @@ func getAddress(addrMode, portLabel string, networks structs.Networks, driverNet
 		// Check in Networks struct for backwards compatibility if not found
 		mapping, ok := ports.Get(portLabel)
 		if !ok {
-			ip, port := networks.Port(portLabel)
-			if port > 0 {
-				return ip, port, nil
+			mapping = networks.Port(portLabel)
+			if mapping.Value > 0 {
+				return mapping.HostIP, mapping.Value, nil
 			}
 
 			// If port isn't a label, try to parse it as a literal port number

nomad/job_endpoint_hook_connect.go

@@ -248,7 +248,11 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error {
 			task.Canonicalize(job, g)
 
 			// create a port for the sidecar task's proxy port
-			injectPort(g, fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name))
+			portLabel := service.Connect.SidecarService.Port
+			if portLabel == "" {
+				portLabel = fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name)
+			}
+			injectPort(g, portLabel)
 
 		case service.Connect.IsNative():
 			// find the task backing this connect native service and set the kind

nomad/job_endpoint_hook_expose_check.go

@@ -206,13 +206,15 @@ func exposePathForCheck(tg *structs.TaskGroup, s *structs.Service, check *struct
 	// The difference here is the address is predestined to be localhost since
 	// it is binding inside the namespace.
 	var port int
-	if _, port = tg.Networks.Port(s.PortLabel); port <= 0 { // try looking up by port label
+	if mapping := tg.Networks.Port(s.PortLabel); mapping.Value <= 0 { // try looking up by port label
 		if port, _ = strconv.Atoi(s.PortLabel); port <= 0 { // then try direct port value
 			return nil, errors.Errorf(
 				"unable to determine local service port for service check %s->%s->%s",
 				tg.Name, s.Name, check.Name,
 			)
 		}
+	} else {
+		port = mapping.Value
 	}
 
 	// The Path, Protocol, and PortLabel are just copied over from the service