From be3ac8faa402a20dd1b449bfc57a57cc8abb5773 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Thu, 4 Feb 2021 22:20:49 +0200 Subject: [PATCH 01/17] enabled hairpin mode --- client/allocrunner/networking_bridge_linux.go | 1 + 1 file changed, 1 insertion(+) diff --git a/client/allocrunner/networking_bridge_linux.go b/client/allocrunner/networking_bridge_linux.go index 37c504e41416..183f85a1ff33 100644 --- a/client/allocrunner/networking_bridge_linux.go +++ b/client/allocrunner/networking_bridge_linux.go @@ -149,6 +149,7 @@ const nomadCNIConfigTemplate = `{ "ipMasq": true, "isGateway": true, "forceAddress": true, + "hairpinMode": true, "ipam": { "type": "host-local", "ranges": [ From 4e8142dabb165030a066abb285e4473d85a91acb Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Mon, 8 Feb 2021 12:51:31 +0200 Subject: [PATCH 02/17] customized default sidecar checks --- client/allocrunner/networking_bridge_linux.go | 1 - command/agent/consul/connect.go | 18 +++++++++++++++--- command/agent/consul/service_client.go | 3 ++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/client/allocrunner/networking_bridge_linux.go b/client/allocrunner/networking_bridge_linux.go index 183f85a1ff33..37c504e41416 100644 --- a/client/allocrunner/networking_bridge_linux.go +++ b/client/allocrunner/networking_bridge_linux.go @@ -149,7 +149,6 @@ const nomadCNIConfigTemplate = `{ "ipMasq": true, "isGateway": true, "forceAddress": true, - "hairpinMode": true, "ipam": { "type": "host-local", "ranges": [ diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 68904070f6db..54ce30b11d48 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/structs" ) @@ -11,7 +12,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) (*api.AgentServiceConnect, error) { switch { case nc == nil: // no connect stanza means there is no connect service to register @@ -27,7 +28,7 @@ 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) + sidecarReg, err := connectSidecarRegistration(serviceId, serviceName, nc.SidecarService, networks) if err != nil { return nil, err } @@ -84,7 +85,7 @@ 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, serviceName string, css *structs.ConsulSidecarService, networks structs.Networks) (*api.AgentServiceRegistration, error) { if css == nil { // no sidecar stanza means there is no sidecar service to register return nil, nil @@ -105,6 +106,17 @@ func connectSidecarRegistration(serviceName string, css *structs.ConsulSidecarSe Port: cPort.Value, Address: cNet.IP, Proxy: proxy, + Checks: api.AgentServiceChecks{ + { + Name: "Connect Sidecar Listening", + TCP: ipaddr.FormatAddressPort(cNet.IP, cPort.Value), + Interval: "10s", + }, + { + Name: "Connect Sidecar Aliasing " + serviceId, + AliasService: serviceId, + }, + }, }, nil } diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index 762d3154c336..dcde80fbf594 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -866,7 +866,8 @@ 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) + sidecarId = id + sidecarSuffix + connect, err := newConnect(sidecarId, service.Name, service.Connect, workload.Networks) if err != nil { return nil, fmt.Errorf("invalid Consul Connect configuration for service %q: %v", service.Name, err) } From f43e153a6137bf7794ae9c9a52fe54a84ab97994 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Mon, 8 Feb 2021 12:59:17 +0200 Subject: [PATCH 03/17] fixed variable initialization --- command/agent/consul/service_client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index dcde80fbf594..dbf2b60d498c 100644 --- a/command/agent/consul/service_client.go +++ b/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. - sidecarId = id + sidecarSuffix + sidecarId := id + sidecarSuffix connect, err := newConnect(sidecarId, service.Name, service.Connect, workload.Networks) if err != nil { return nil, fmt.Errorf("invalid Consul Connect configuration for service %q: %v", service.Name, err) From b3323a562b0121e78d7032529cf8c76a1c938a00 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Mon, 8 Feb 2021 13:12:40 +0200 Subject: [PATCH 04/17] removed proxy suffix --- command/agent/consul/service_client.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index dbf2b60d498c..d3c4f1938f4d 100644 --- a/command/agent/consul/service_client.go +++ b/command/agent/consul/service_client.go @@ -866,8 +866,7 @@ func (c *ServiceClient) serviceRegs(ops *operations, service *structs.Service, w } // newConnect returns (nil, nil) if there's no Connect-enabled service. - sidecarId := id + sidecarSuffix - connect, err := newConnect(sidecarId, service.Name, service.Connect, workload.Networks) + connect, err := newConnect(id, service.Name, service.Connect, workload.Networks) if err != nil { return nil, fmt.Errorf("invalid Consul Connect configuration for service %q: %v", service.Name, err) } From 1d5981015a176ce6d37e7dae33ce0b00af3d43a2 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Mon, 8 Feb 2021 18:16:49 +0200 Subject: [PATCH 05/17] fixed tests --- command/agent/consul/connect.go | 4 ++-- command/agent/consul/connect_test.go | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 54ce30b11d48..7ec387c80818 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -100,7 +100,7 @@ func connectSidecarRegistration(serviceId string, serviceName string, css *struc if err != nil { return nil, err } - + return &api.AgentServiceRegistration{ Tags: helper.CopySliceString(css.Tags), Port: cPort.Value, @@ -108,7 +108,7 @@ func connectSidecarRegistration(serviceId string, serviceName string, css *struc Proxy: proxy, Checks: api.AgentServiceChecks{ { - Name: "Connect Sidecar Listening", + Name: "Connect Sidecar Listening", TCP: ipaddr.FormatAddressPort(cNet.IP, cPort.Value), Interval: "10s", }, diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index d42f639957cb..1e5df9424726 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -27,13 +27,13 @@ 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) 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) require.NoError(t, err) @@ -42,7 +42,7 @@ func TestConnect_newConnect(t *testing.T) { }) 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"}, @@ -68,20 +68,20 @@ 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) require.NoError(t, err) require.Nil(t, sidecarReg) }) t.Run("no service port", func(t *testing.T) { - _, err := connectSidecarRegistration("unknown", &structs.ConsulSidecarService{ + _, err := connectSidecarRegistration("unknown-id", "unknown", &structs.ConsulSidecarService{ // irrelevant }, testConnectNetwork) require.EqualError(t, err, `No Connect port defined for service "unknown"`) }) t.Run("bad proxy", func(t *testing.T) { - _, err := connectSidecarRegistration("redis", &structs.ConsulSidecarService{ + _, err := connectSidecarRegistration("redis-service-id", "redis", &structs.ConsulSidecarService{ Proxy: &structs.ConsulProxy{ Expose: &structs.ConsulExposeConfig{ Paths: []structs.ConsulExposePath{{ @@ -94,7 +94,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { }) t.Run("normal", func(t *testing.T) { - proxy, err := connectSidecarRegistration("redis", &structs.ConsulSidecarService{ + proxy, err := connectSidecarRegistration("redis-service-id", "redis", &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, Port: "sidecarPort", }, testConnectNetwork) From 93cd0d2e88f61157dc1ce6a40226d8cda33872e2 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Mon, 8 Feb 2021 21:54:21 +0200 Subject: [PATCH 06/17] fixed tests --- command/agent/consul/connect.go | 4 ++-- command/agent/consul/connect_test.go | 13 ++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 7ec387c80818..54ce30b11d48 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -100,7 +100,7 @@ func connectSidecarRegistration(serviceId string, serviceName string, css *struc if err != nil { return nil, err } - + return &api.AgentServiceRegistration{ Tags: helper.CopySliceString(css.Tags), Port: cPort.Value, @@ -108,7 +108,7 @@ func connectSidecarRegistration(serviceId string, serviceName string, css *struc Proxy: proxy, Checks: api.AgentServiceChecks{ { - Name: "Connect Sidecar Listening", + Name: "Connect Sidecar Listening", TCP: ipaddr.FormatAddressPort(cNet.IP, cPort.Value), Interval: "10s", }, diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index 1e5df9424726..11c80f3916b8 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -27,7 +27,7 @@ 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) require.NoError(t, err) require.Nil(t, asr) }) @@ -109,6 +109,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) }) } From 2d13826e22703078ca4ae58e08383af7340ba366 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Mon, 8 Feb 2021 22:13:32 +0200 Subject: [PATCH 07/17] fixed tests --- command/agent/consul/connect_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index 11c80f3916b8..af86be7e7601 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -60,6 +60,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) }) } From b860cc44a001f56c4bdc190ce55889f2f9f3e541 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 14:05:28 +0200 Subject: [PATCH 08/17] allocate sidecar task port on host_network interface --- .../taskrunner/envoy_bootstrap_hook.go | 3 +- command/agent/consul/connect.go | 41 +++++++++++-------- command/agent/consul/service_client.go | 8 ++-- nomad/job_endpoint_hook_connect.go | 6 ++- nomad/job_endpoint_hook_expose_check.go | 4 +- nomad/structs/structs.go | 36 +++++++--------- 6 files changed, 51 insertions(+), 47 deletions(-) diff --git a/client/allocrunner/taskrunner/envoy_bootstrap_hook.go b/client/allocrunner/taskrunner/envoy_bootstrap_hook.go index 0b286138dff8..eaa9f534c9e9 100644 --- a/client/allocrunner/taskrunner/envoy_bootstrap_hook.go +++ b/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 } } diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 54ce30b11d48..16ad8ce36e55 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -12,7 +12,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(serviceId string, 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 @@ -28,7 +28,10 @@ func newConnect(serviceId string, serviceName string, nc *structs.ConsulConnect, case nc.HasSidecar(): // must register the sidecar for this service - sidecarReg, err := connectSidecarRegistration(serviceId, 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 } @@ -85,31 +88,31 @@ func newConnectGateway(serviceName string, connect *structs.ConsulConnect) *api. return &api.AgentServiceConnectProxyConfig{Config: envoyConfig} } -func connectSidecarRegistration(serviceId string, 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: ipaddr.FormatAddressPort(cNet.IP, cPort.Value), + TCP: ipaddr.FormatAddressPort(cMapping.HostIP, cMapping.Value), Interval: "10s", }, { @@ -212,17 +215,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("invalid port %q: port label not found", portLabel) } - - return networks[0], port, nil + return mapping, nil } // connectExposePathPort returns the port for the exposed path for the exposed @@ -232,10 +237,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 } diff --git a/command/agent/consul/service_client.go b/command/agent/consul/service_client.go index d3c4f1938f4d..8af946c46f2d 100644 --- a/command/agent/consul/service_client.go +++ b/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(id, 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 diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index fbd444fa2757..20601a433b79 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/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 diff --git a/nomad/job_endpoint_hook_expose_check.go b/nomad/job_endpoint_hook_expose_check.go index e1bc6d1ee2cb..ad60f706a466 100644 --- a/nomad/job_endpoint_hook_expose_check.go +++ b/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 diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 5b3b8d2f9d15..17b2c31083c2 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -2602,24 +2602,6 @@ func (n *NetworkResource) PortLabels() map[string]int { return labelValues } -// ConnectPort returns the Connect port for the given service. Returns false if -// no port was found for a service with that name. -func (n *NetworkResource) PortForService(serviceName string) (Port, bool) { - label := fmt.Sprintf("%s-%s", ConnectProxyPrefix, serviceName) - for _, port := range n.ReservedPorts { - if port.Label == label { - return port, true - } - } - for _, port := range n.DynamicPorts { - if port.Label == label { - return port, true - } - } - - return Port{}, false -} - // Networks defined for a task on the Resources struct. type Networks []*NetworkResource @@ -2636,20 +2618,30 @@ func (ns Networks) Copy() Networks { } // Port assignment and IP for the given label or empty values. -func (ns Networks) Port(label string) (string, int) { +func (ns Networks) Port(label string) AllocatedPortMapping { for _, n := range ns { for _, p := range n.ReservedPorts { if p.Label == label { - return n.IP, p.Value + return AllocatedPortMapping{ + Label: label, + Value: p.Value, + To: p.To, + HostIP: n.IP, + } } } for _, p := range n.DynamicPorts { if p.Label == label { - return n.IP, p.Value + return AllocatedPortMapping{ + Label: label, + Value: p.Value, + To: p.To, + HostIP: n.IP, + } } } } - return "", 0 + return AllocatedPortMapping{} } func (ns Networks) NetIndex(n *NetworkResource) int { From b7d1aee6214c979c3c0fca8f0857ca0593be6398 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 14:46:13 +0200 Subject: [PATCH 09/17] tests fix --- command/agent/consul/connect_test.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index af86be7e7601..e9e63ad4aca5 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -21,13 +21,19 @@ 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) }) @@ -35,7 +41,7 @@ func TestConnect_newConnect(t *testing.T) { t.Run("native", func(t *testing.T) { asr, err := newConnect("", "", &structs.ConsulConnect{ Native: true, - }, nil) + }, nil, nil) require.NoError(t, err) require.True(t, asr.Native) require.Nil(t, asr.SidecarService) @@ -48,7 +54,7 @@ func TestConnect_newConnect(t *testing.T) { Tags: []string{"foo", "bar"}, Port: "sidecarPort", }, - }, testConnectNetwork) + }, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Equal(t, &api.AgentServiceRegistration{ Tags: []string{"foo", "bar"}, @@ -79,7 +85,7 @@ 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) }) @@ -87,7 +93,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { t.Run("no service port", func(t *testing.T) { _, err := connectSidecarRegistration("unknown-id", "unknown", &structs.ConsulSidecarService{ // irrelevant - }, testConnectNetwork) + }, testConnectNetwork, testConnectPorts) require.EqualError(t, err, `No Connect port defined for service "unknown"`) }) @@ -100,7 +106,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { }}, }, }, - }, testConnectNetwork) + }, testConnectNetwork, testConnectPorts) require.EqualError(t, err, `No port of label "badPort" defined`) }) @@ -108,7 +114,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { proxy, err := connectSidecarRegistration("redis-service-id", "redis", &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, Port: "sidecarPort", - }, testConnectNetwork) + }, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Equal(t, &api.AgentServiceRegistration{ Tags: []string{"foo", "bar"}, From 4ed7323c8d6f391c8569f659327741cd0c2f95d4 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 15:31:48 +0200 Subject: [PATCH 10/17] fixed connect port label --- command/agent/consul/connect_test.go | 8 ++++---- nomad/job_endpoint_hook_connect.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index e9e63ad4aca5..27fd09d8160b 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -22,10 +22,10 @@ var ( }, }} testConnectPorts = structs.AllocatedPorts{{ - Label = "connect-proxy-redis" - Value = 3000, - To = 3000, - HostIP = "192.168.30.1", + Label: "connect-proxy-redis", + Value: 3000, + To: 3000, + HostIP: "192.168.30.1", }} ) diff --git a/nomad/job_endpoint_hook_connect.go b/nomad/job_endpoint_hook_connect.go index 20601a433b79..1e215f6196aa 100644 --- a/nomad/job_endpoint_hook_connect.go +++ b/nomad/job_endpoint_hook_connect.go @@ -249,7 +249,7 @@ func groupConnectHook(job *structs.Job, g *structs.TaskGroup) error { // create a port for the sidecar task's proxy port portLabel := service.Connect.SidecarService.Port - if portLabel != "" { + if portLabel == "" { portLabel = fmt.Sprintf("%s-%s", structs.ConnectProxyPrefix, service.Name) } injectPort(g, portLabel) From 625bd989150b4e6c102b063f7514a9779b7a0628 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 15:53:57 +0200 Subject: [PATCH 11/17] fixed connect port label --- command/agent/consul/connect_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index 27fd09d8160b..0e726200fe16 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -85,20 +85,20 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { t.Parallel() t.Run("nil", func(t *testing.T) { - sidecarReg, err := connectSidecarRegistration("", "", nil, testConnectNetwork, testConnectPorts) + 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-id", "unknown", &structs.ConsulSidecarService{ + _, err := connectSidecarRegistration("unknown-id", &structs.ConsulSidecarService{ // irrelevant }, testConnectNetwork, testConnectPorts) - require.EqualError(t, err, `No Connect port defined for service "unknown"`) + require.EqualError(t, err, `No Connect port defined`) }) t.Run("bad proxy", func(t *testing.T) { - _, err := connectSidecarRegistration("redis-service-id", "redis", &structs.ConsulSidecarService{ + _, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ Proxy: &structs.ConsulProxy{ Expose: &structs.ConsulExposeConfig{ Paths: []structs.ConsulExposePath{{ @@ -111,7 +111,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { }) t.Run("normal", func(t *testing.T) { - proxy, err := connectSidecarRegistration("redis-service-id", "redis", &structs.ConsulSidecarService{ + proxy, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, Port: "sidecarPort", }, testConnectNetwork, testConnectPorts) @@ -363,31 +363,31 @@ func TestConnect_getConnectPort(t *testing.T) { }}}} t.Run("normal", func(t *testing.T) { - nr, port, err := connectPort("foo", networks) + nr, err := connectPort("foo", networks) 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) + }, nr.Value) + require.Equal(t, "192.168.30.1", nr.HostIP) }) t.Run("no such service", func(t *testing.T) { - _, _, err := connectPort("other", networks) + _, err := connectPort("other", networks) require.EqualError(t, err, `No Connect port defined for service "other"`) }) 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)") }) } From f6e875dcb86fac281940901a3cc75bcbbac7d84c Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 15:58:09 +0200 Subject: [PATCH 12/17] fixed connect port label --- command/agent/consul/connect_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index 0e726200fe16..cf17a7e410c1 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -92,9 +92,9 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { t.Run("no service port", func(t *testing.T) { _, err := connectSidecarRegistration("unknown-id", &structs.ConsulSidecarService{ - // irrelevant + Port: "unknown-label", }, testConnectNetwork, testConnectPorts) - require.EqualError(t, err, `No Connect port defined`) + require.EqualError(t, err, `invalid port unknown-label: port label not found`) }) t.Run("bad proxy", func(t *testing.T) { From 02aa923a5784925dd3ae613ee55c34acd51734aa Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 16:26:44 +0200 Subject: [PATCH 13/17] fixed tests --- command/agent/consul/connect_test.go | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index cf17a7e410c1..fa4a9cb67a80 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -362,19 +362,26 @@ 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, 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, - }, nr.Value) - require.Equal(t, "192.168.30.1", nr.HostIP) + 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) + _, err := connectPort("other", networks, ports) require.EqualError(t, err, `No Connect port defined for service "other"`) }) From 8e09d7c3947214864eb5ee660a6023794278bc56 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 16:38:44 +0200 Subject: [PATCH 14/17] fixed typo --- command/agent/consul/connect_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index fa4a9cb67a80..66464d78bac4 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -52,7 +52,7 @@ func TestConnect_newConnect(t *testing.T) { Native: false, SidecarService: &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, - Port: "sidecarPort", + Port: "connext-proxy-redis", }, }, testConnectNetwork, testConnectPorts) require.NoError(t, err) @@ -94,11 +94,12 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { _, err := connectSidecarRegistration("unknown-id", &structs.ConsulSidecarService{ Port: "unknown-label", }, testConnectNetwork, testConnectPorts) - require.EqualError(t, err, `invalid port unknown-label: port label not found`) + require.EqualError(t, err, `invalid port "unknown-label": port label not found`) }) t.Run("bad proxy", func(t *testing.T) { _, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ + Port: "connext-proxy-redis", Proxy: &structs.ConsulProxy{ Expose: &structs.ConsulExposeConfig{ Paths: []structs.ConsulExposePath{{ @@ -113,7 +114,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { t.Run("normal", func(t *testing.T) { proxy, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, - Port: "sidecarPort", + Port: "connext-proxy-redis", }, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Equal(t, &api.AgentServiceRegistration{ @@ -362,7 +363,7 @@ func TestConnect_getConnectPort(t *testing.T) { To: 23456, }}}} - ports = structs.AllocatedPorts{{ + ports := structs.AllocatedPorts{{ Label: "foo", Value: 23456, To: 23456, @@ -382,7 +383,7 @@ func TestConnect_getConnectPort(t *testing.T) { t.Run("no such service", func(t *testing.T) { _, err := connectPort("other", networks, ports) - require.EqualError(t, err, `No Connect port defined for service "other"`) + require.EqualError(t, err, `invalid port "other": port label not found`) }) t.Run("no network", func(t *testing.T) { From 501d27c9b5f63013148c78eff2835ed2feb6296d Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 17:04:35 +0200 Subject: [PATCH 15/17] fixed typo --- command/agent/consul/connect_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index 66464d78bac4..dd3332ecbee4 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -52,7 +52,7 @@ func TestConnect_newConnect(t *testing.T) { Native: false, SidecarService: &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, - Port: "connext-proxy-redis", + Port: "connect-proxy-redis", }, }, testConnectNetwork, testConnectPorts) require.NoError(t, err) @@ -99,7 +99,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { t.Run("bad proxy", func(t *testing.T) { _, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ - Port: "connext-proxy-redis", + Port: "connect-proxy-redis", Proxy: &structs.ConsulProxy{ Expose: &structs.ConsulExposeConfig{ Paths: []structs.ConsulExposePath{{ From 66e3e95aaf752bd8668d510f79fc3f6236da36e9 Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Tue, 9 Feb 2021 17:05:55 +0200 Subject: [PATCH 16/17] fixed typo --- command/agent/consul/connect_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index dd3332ecbee4..edefa3f88a97 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -114,7 +114,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { t.Run("normal", func(t *testing.T) { proxy, err := connectSidecarRegistration("redis-service-id", &structs.ConsulSidecarService{ Tags: []string{"foo", "bar"}, - Port: "connext-proxy-redis", + Port: "connect-proxy-redis", }, testConnectNetwork, testConnectPorts) require.NoError(t, err) require.Equal(t, &api.AgentServiceRegistration{ From 77ec3d1fabc95c7815a935a45afdfc8bbb76f96a Mon Sep 17 00:00:00 2001 From: AndrewChubatiuk Date: Sat, 13 Feb 2021 02:32:33 +0200 Subject: [PATCH 17/17] pr comments --- command/agent/consul/connect.go | 7 ++++--- command/agent/consul/connect_test.go | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/command/agent/consul/connect.go b/command/agent/consul/connect.go index 16ad8ce36e55..3825c0da9320 100644 --- a/command/agent/consul/connect.go +++ b/command/agent/consul/connect.go @@ -2,9 +2,10 @@ package consul import ( "fmt" + "net" + "strconv" "github.com/hashicorp/consul/api" - "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/nomad/structs" ) @@ -112,7 +113,7 @@ func connectSidecarRegistration(serviceId string, css *structs.ConsulSidecarServ Checks: api.AgentServiceChecks{ { Name: "Connect Sidecar Listening", - TCP: ipaddr.FormatAddressPort(cMapping.HostIP, cMapping.Value), + TCP: net.JoinHostPort(cMapping.HostIP, strconv.Itoa(cMapping.Value)), Interval: "10s", }, { @@ -225,7 +226,7 @@ func connectPort(portLabel string, networks structs.Networks, ports structs.Allo if mapping.Value > 0 { return mapping, nil } - return structs.AllocatedPortMapping{}, fmt.Errorf("invalid port %q: port label not found", portLabel) + return structs.AllocatedPortMapping{}, fmt.Errorf("No port of label %q defined", portLabel) } return mapping, nil } diff --git a/command/agent/consul/connect_test.go b/command/agent/consul/connect_test.go index edefa3f88a97..7d89c55eca4a 100644 --- a/command/agent/consul/connect_test.go +++ b/command/agent/consul/connect_test.go @@ -94,7 +94,7 @@ func TestConnect_connectSidecarRegistration(t *testing.T) { _, err := connectSidecarRegistration("unknown-id", &structs.ConsulSidecarService{ Port: "unknown-label", }, testConnectNetwork, testConnectPorts) - require.EqualError(t, err, `invalid port "unknown-label": port label not found`) + require.EqualError(t, err, `No port of label "unknown-label" defined`) }) t.Run("bad proxy", func(t *testing.T) { @@ -383,7 +383,7 @@ func TestConnect_getConnectPort(t *testing.T) { t.Run("no such service", func(t *testing.T) { _, err := connectPort("other", networks, ports) - require.EqualError(t, err, `invalid port "other": port label not found`) + require.EqualError(t, err, `No port of label "other" defined`) }) t.Run("no network", func(t *testing.T) {