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

@@ -0,0 +1,3 @@
+```release-note:bug
+checks: If set, use proxy address for automatically added sidecar check instead of service address.
+```

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)
 		})

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),
+			TCP:      ipaddr.FormatAddressPort(address, port),
 			Interval: 10 * time.Second,
 		},
 		{

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) {