From 3cfea70273e1cfe7673c4220a8356a884ce69c17 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Thu, 1 Sep 2022 14:03:35 -0700 Subject: [PATCH] Use proxy address for default check (#14433) When a sidecar proxy is registered, a check is automatically added. Previously, the address this check used was the underlying service's address instead of the proxy's address, even though the check is testing if the proxy is up. This worked in most cases because the proxy ran on the same IP as the underlying service but it's not guaranteed and so the proper default address should be the proxy's address. --- .changelog/14433.txt | 3 + agent/agent_endpoint_test.go | 2 +- agent/sidecar_service.go | 24 ++++-- agent/sidecar_service_test.go | 135 ++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 9 deletions(-) create mode 100644 .changelog/14433.txt diff --git a/.changelog/14433.txt b/.changelog/14433.txt new file mode 100644 index 000000000..25167320c --- /dev/null +++ b/.changelog/14433.txt @@ -0,0 +1,3 @@ +```release-note:bug +checks: If set, use proxy address for automatically added sidecar check instead of service address. +``` diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 67850f9eb..d380d0d93 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -3764,7 +3764,7 @@ func testAgent_RegisterService_TranslateKeys(t *testing.T, extraHCL string) { fmt.Println("TCP Check:= ", v) } if hasNoCorrectTCPCheck { - t.Fatalf("Did not find the expected TCP Healtcheck '%s' in %#v ", tt.expectedTCPCheckStart, a.checkTCPs) + t.Fatalf("Did not find the expected TCP Healthcheck '%s' in %#v ", tt.expectedTCPCheckStart, a.checkTCPs) } require.Equal(t, sidecarSvc, gotSidecar) }) diff --git a/agent/sidecar_service.go b/agent/sidecar_service.go index e0cb24a0e..a41d73d80 100644 --- a/agent/sidecar_service.go +++ b/agent/sidecar_service.go @@ -127,9 +127,20 @@ func (a *Agent) sidecarServiceFromNodeService(ns *structs.NodeService, token str if err != nil { return nil, nil, "", err } - // Setup default check if none given + // Setup default check if none given. if len(checks) < 1 { - checks = sidecarDefaultChecks(ns.ID, sidecar.Proxy.LocalServiceAddress, sidecar.Port) + // The check should use the sidecar's address because it makes a request to the sidecar. + // If the sidecar's address is empty, we fall back to the address of the local service, as set in + // sidecar.Proxy.LocalServiceAddress, in the hope that the proxy is also accessible on that address + // (which in most cases it is because it's running as a sidecar in the same network). + // We could instead fall back to the address of the service as set by (ns.Address), but I've kept it using + // sidecar.Proxy.LocalServiceAddress so as to not change things too much in the + // process of fixing #14433. + checkAddress := sidecar.Address + if checkAddress == "" { + checkAddress = sidecar.Proxy.LocalServiceAddress + } + checks = sidecarDefaultChecks(ns.ID, checkAddress, sidecar.Port) } return sidecar, checks, token, nil @@ -202,14 +213,11 @@ func (a *Agent) sidecarPortFromServiceID(sidecarCompoundServiceID structs.Servic return sidecarPort, nil } -func sidecarDefaultChecks(serviceID string, localServiceAddress string, port int) []*structs.CheckType { - // Setup default check if none given +func sidecarDefaultChecks(serviceID string, address string, port int) []*structs.CheckType { return []*structs.CheckType{ { - Name: "Connect Sidecar Listening", - // Default to localhost rather than agent/service public IP. The checks - // can always be overridden if a non-loopback IP is needed. - TCP: ipaddr.FormatAddressPort(localServiceAddress, port), + Name: "Connect Sidecar Listening", + TCP: ipaddr.FormatAddressPort(address, port), Interval: 10 * time.Second, }, { diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go index f095670ff..39ab854a6 100644 --- a/agent/sidecar_service_test.go +++ b/agent/sidecar_service_test.go @@ -215,6 +215,141 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) { token: "foo", wantErr: "reserved for internal use", }, + { + name: "uses proxy address for check", + sd: &structs.ServiceDefinition{ + ID: "web1", + Name: "web", + Port: 1111, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Address: "123.123.123.123", + Proxy: &structs.ConnectProxyConfig{ + LocalServiceAddress: "255.255.255.255", + }, + }, + }, + Address: "255.255.255.255", + }, + token: "foo", + wantNS: &structs.NodeService{ + EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), + Kind: structs.ServiceKindConnectProxy, + ID: "web1-sidecar-proxy", + Service: "web-sidecar-proxy", + Port: 2222, + Address: "123.123.123.123", + LocallyRegisteredAsSidecar: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web1", + LocalServiceAddress: "255.255.255.255", + LocalServicePort: 1111, + }, + }, + wantChecks: []*structs.CheckType{ + { + Name: "Connect Sidecar Listening", + TCP: "123.123.123.123:2222", + Interval: 10 * time.Second, + }, + { + Name: "Connect Sidecar Aliasing web1", + AliasService: "web1", + }, + }, + wantToken: "foo", + }, + { + name: "uses proxy.local_service_address for check if proxy address is empty", + sd: &structs.ServiceDefinition{ + ID: "web1", + Name: "web", + Port: 1111, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Address: "", // Proxy address empty. + Proxy: &structs.ConnectProxyConfig{ + LocalServiceAddress: "1.2.3.4", + }, + }, + }, + Address: "", // Service address empty. + }, + token: "foo", + wantNS: &structs.NodeService{ + EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), + Kind: structs.ServiceKindConnectProxy, + ID: "web1-sidecar-proxy", + Service: "web-sidecar-proxy", + Port: 2222, + Address: "", + LocallyRegisteredAsSidecar: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web1", + LocalServiceAddress: "1.2.3.4", + LocalServicePort: 1111, + }, + }, + wantChecks: []*structs.CheckType{ + { + Name: "Connect Sidecar Listening", + TCP: "1.2.3.4:2222", + Interval: 10 * time.Second, + }, + { + Name: "Connect Sidecar Aliasing web1", + AliasService: "web1", + }, + }, + wantToken: "foo", + }, + { + name: "uses 127.0.0.1 for check if proxy and proxy.local_service_address are empty", + sd: &structs.ServiceDefinition{ + ID: "web1", + Name: "web", + Port: 1111, + Connect: &structs.ServiceConnect{ + SidecarService: &structs.ServiceDefinition{ + Address: "", + Proxy: &structs.ConnectProxyConfig{ + LocalServiceAddress: "", + }, + }, + }, + Address: "", + }, + token: "foo", + wantNS: &structs.NodeService{ + EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(), + Kind: structs.ServiceKindConnectProxy, + ID: "web1-sidecar-proxy", + Service: "web-sidecar-proxy", + Port: 2222, + Address: "", + LocallyRegisteredAsSidecar: true, + Proxy: structs.ConnectProxyConfig{ + DestinationServiceName: "web", + DestinationServiceID: "web1", + LocalServiceAddress: "127.0.0.1", + LocalServicePort: 1111, + }, + }, + wantChecks: []*structs.CheckType{ + { + Name: "Connect Sidecar Listening", + TCP: "127.0.0.1:2222", + Interval: 10 * time.Second, + }, + { + Name: "Connect Sidecar Aliasing web1", + AliasService: "web1", + }, + }, + wantToken: "foo", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {