Backport of [NET-5217] [OSS] Derive sidecar proxy locality from parent service into release/1.16.x (#18438)

backport of commit 05604eeec1186a4c8a6156e7519a579171659c1a

Partial backport to include fix without logging changes (logging targets
code not included in 1.16.x).

Co-authored-by: Michael Zalimeni <michael.zalimeni@hashicorp.com>

.changelog/18437.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+Inherit locality from services when registering sidecar proxies.
+```

agent/sidecar_service.go

@@ -81,6 +81,12 @@ func sidecarServiceFromNodeService(ns *structs.NodeService, token string) (*stru
 		sidecar.Tags = append(sidecar.Tags, ns.Tags...)
 	}
 
+	// Copy the locality from the original service if locality was not provided
+	if sidecar.Locality == nil && ns.Locality != nil {
+		tmp := *ns.Locality
+		sidecar.Locality = &tmp
+	}
+
 	// Flag this as a sidecar - this is not persisted in catalog but only needed
 	// in local agent state to disambiguate lineage when deregistering the parent
 	// service later.

agent/sidecar_service_test.go

@@ -134,13 +134,17 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 			wantToken: "custom-token",
 		},
 		{
-			name: "inherit tags and meta",
+			name: "inherit locality, tags and meta",
 			sd: &structs.ServiceDefinition{
 				ID:   "web1",
 				Name: "web",
 				Port: 1111,
 				Tags: []string{"foo"},
 				Meta: map[string]string{"foo": "bar"},
+				Locality: &structs.Locality{
+					Region: "us-east-1",
+					Zone:   "us-east-1a",
+				},
 				Connect: &structs.ServiceConnect{
 					SidecarService: &structs.ServiceDefinition{},
 				},
@@ -153,6 +157,55 @@ func TestAgent_sidecarServiceFromNodeService(t *testing.T) {
 				Port:           0,
 				Tags:           []string{"foo"},
 				Meta:           map[string]string{"foo": "bar"},
+				Locality: &structs.Locality{
+					Region: "us-east-1",
+					Zone:   "us-east-1a",
+				},
+				LocallyRegisteredAsSidecar: true,
+				Proxy: structs.ConnectProxyConfig{
+					DestinationServiceName: "web",
+					DestinationServiceID:   "web1",
+					LocalServiceAddress:    "127.0.0.1",
+					LocalServicePort:       1111,
+				},
+			},
+			wantChecks: nil,
+		},
+		{
+			name: "retain locality, tags and meta if explicitly configured",
+			sd: &structs.ServiceDefinition{
+				ID:   "web1",
+				Name: "web",
+				Port: 1111,
+				Tags: []string{"foo"},
+				Meta: map[string]string{"foo": "bar"},
+				Locality: &structs.Locality{
+					Region: "us-east-1",
+					Zone:   "us-east-1a",
+				},
+				Connect: &structs.ServiceConnect{
+					SidecarService: &structs.ServiceDefinition{
+						Tags: []string{"bar"},
+						Meta: map[string]string{"baz": "qux"},
+						Locality: &structs.Locality{
+							Region: "us-east-2",
+							Zone:   "us-east-2a",
+						},
+					},
+				},
+			},
+			wantNS: &structs.NodeService{
+				EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(),
+				Kind:           structs.ServiceKindConnectProxy,
+				ID:             "web1-sidecar-proxy",
+				Service:        "web-sidecar-proxy",
+				Port:           0,
+				Tags:           []string{"bar"},
+				Meta:           map[string]string{"baz": "qux"},
+				Locality: &structs.Locality{
+					Region: "us-east-2",
+					Zone:   "us-east-2a",
+				},
 				LocallyRegisteredAsSidecar: true,
 				Proxy: structs.ConnectProxyConfig{
 					DestinationServiceName: "web",

command/connect/envoy/envoy.go

@@ -344,15 +344,16 @@ func (c *cmd) run(args []string) int {
 		}
 	}
 
+	var svcForSidecar api.AgentService
 	if c.proxyID == "" {
 		switch {
 		case c.sidecarFor != "":
-			proxyID, err := proxyCmd.LookupProxyIDForSidecar(c.client, c.sidecarFor)
+			svcForSidecar, err := proxyCmd.LookupServiceForSidecar(c.client, c.sidecarFor)
 			if err != nil {
 				c.UI.Error(err.Error())
 				return 1
 			}
-			c.proxyID = proxyID
+			c.proxyID = svcForSidecar.ID
 
 		case c.gateway != "" && !c.register:
 			gatewaySvc, err := proxyCmd.LookupGatewayProxy(c.client, c.gatewayKind)
@@ -394,77 +395,13 @@ func (c *cmd) run(args []string) int {
 			return 1
 		}
 
-		taggedAddrs := make(map[string]api.ServiceAddress)
+		svc, err := c.proxyRegistration(&svcForSidecar)
-		lanAddr := c.lanAddress.Value()
+		if err != nil {
-		if lanAddr.Address != "" {
+			c.UI.Error(err.Error())
-			taggedAddrs[structs.TaggedAddressLAN] = lanAddr
-		}
-
-		wanAddr := c.wanAddress.Value()
-		if wanAddr.Address != "" {
-			taggedAddrs[structs.TaggedAddressWAN] = wanAddr
-		}
-
-		tcpCheckAddr := lanAddr.Address
-		if tcpCheckAddr == "" {
-			// fallback to localhost as the gateway has to reside in the same network namespace
-			// as the agent
-			tcpCheckAddr = "127.0.0.1"
-		}
-
-		var proxyConf *api.AgentServiceConnectProxyConfig
-		if len(c.bindAddresses.value) > 0 {
-			// override all default binding rules and just bind to the user-supplied addresses
-			proxyConf = &api.AgentServiceConnectProxyConfig{
-				Config: map[string]interface{}{
-					"envoy_gateway_no_default_bind": true,
-					"envoy_gateway_bind_addresses":  c.bindAddresses.value,
-				},
-			}
-		} else if canBind(lanAddr) && canBind(wanAddr) {
-			// when both addresses are bindable then we bind to the tagged addresses
-			// for creating the envoy listeners
-			proxyConf = &api.AgentServiceConnectProxyConfig{
-				Config: map[string]interface{}{
-					"envoy_gateway_no_default_bind":       true,
-					"envoy_gateway_bind_tagged_addresses": true,
-				},
-			}
-		} else if !canBind(lanAddr) && lanAddr.Address != "" {
-			c.UI.Error(fmt.Sprintf("The LAN address %q will not be bindable. Either set a bindable address or override the bind addresses with -bind-address", lanAddr.Address))
 			return 1
 		}
 
-		var meta map[string]string
+		if err := c.client.Agent().ServiceRegister(svc); err != nil {
-		if c.exposeServers {
-			meta = map[string]string{structs.MetaWANFederationKey: "1"}
-		}
-
-		// API gateways do not have a default listener or ready endpoint,
-		// so adding any check to the registration will fail
-		var check *api.AgentServiceCheck
-		if c.gatewayKind != api.ServiceKindAPIGateway {
-			check = &api.AgentServiceCheck{
-				Name:                           fmt.Sprintf("%s listening", c.gatewayKind),
-				TCP:                            ipaddr.FormatAddressPort(tcpCheckAddr, lanAddr.Port),
-				Interval:                       "10s",
-				DeregisterCriticalServiceAfter: c.deregAfterCritical,
-			}
-		}
-
-		svc := api.AgentServiceRegistration{
-			Kind:            c.gatewayKind,
-			Name:            c.gatewaySvcName,
-			ID:              c.proxyID,
-			Address:         lanAddr.Address,
-			Port:            lanAddr.Port,
-			Meta:            meta,
-			TaggedAddresses: taggedAddrs,
-			Proxy:           proxyConf,
-			Check:           check,
-		}
-
-		if err := c.client.Agent().ServiceRegister(&svc); err != nil {
 			c.UI.Error(fmt.Sprintf("Error registering service %q: %s", svc.Name, err))
 			return 1
 		}
@@ -542,6 +479,85 @@ func (c *cmd) run(args []string) int {
 	return 0
 }
 
+func (c *cmd) proxyRegistration(svcForSidecar *api.AgentService) (*api.AgentServiceRegistration, error) {
+	taggedAddrs := make(map[string]api.ServiceAddress)
+	lanAddr := c.lanAddress.Value()
+	if lanAddr.Address != "" {
+		taggedAddrs[structs.TaggedAddressLAN] = lanAddr
+	}
+
+	wanAddr := c.wanAddress.Value()
+	if wanAddr.Address != "" {
+		taggedAddrs[structs.TaggedAddressWAN] = wanAddr
+	}
+
+	tcpCheckAddr := lanAddr.Address
+	if tcpCheckAddr == "" {
+		// fallback to localhost as the gateway has to reside in the same network namespace
+		// as the agent
+		tcpCheckAddr = "127.0.0.1"
+	}
+
+	var proxyConf *api.AgentServiceConnectProxyConfig
+	if len(c.bindAddresses.value) > 0 {
+		// override all default binding rules and just bind to the user-supplied addresses
+		proxyConf = &api.AgentServiceConnectProxyConfig{
+			Config: map[string]interface{}{
+				"envoy_gateway_no_default_bind": true,
+				"envoy_gateway_bind_addresses":  c.bindAddresses.value,
+			},
+		}
+	} else if canBind(lanAddr) && canBind(wanAddr) {
+		// when both addresses are bindable then we bind to the tagged addresses
+		// for creating the envoy listeners
+		proxyConf = &api.AgentServiceConnectProxyConfig{
+			Config: map[string]interface{}{
+				"envoy_gateway_no_default_bind":       true,
+				"envoy_gateway_bind_tagged_addresses": true,
+			},
+		}
+	} else if !canBind(lanAddr) && lanAddr.Address != "" {
+		return nil, fmt.Errorf("The LAN address %q will not be bindable. Either set a bindable address or override the bind addresses with -bind-address", lanAddr.Address)
+	}
+
+	var meta map[string]string
+	if c.exposeServers {
+		meta = map[string]string{structs.MetaWANFederationKey: "1"}
+	}
+
+	// API gateways do not have a default listener or ready endpoint,
+	// so adding any check to the registration will fail
+	var check *api.AgentServiceCheck
+	if c.gatewayKind != api.ServiceKindAPIGateway {
+		check = &api.AgentServiceCheck{
+			Name:                           fmt.Sprintf("%s listening", c.gatewayKind),
+			TCP:                            ipaddr.FormatAddressPort(tcpCheckAddr, lanAddr.Port),
+			Interval:                       "10s",
+			DeregisterCriticalServiceAfter: c.deregAfterCritical,
+		}
+	}
+
+	// If registering a sidecar for an existing service, inherit the
+	// locality of that service if it was explicitly configured.
+	var locality *api.Locality
+	if c.sidecarFor != "" {
+		locality = svcForSidecar.Locality
+	}
+
+	return &api.AgentServiceRegistration{
+		Kind:            c.gatewayKind,
+		Name:            c.gatewaySvcName,
+		ID:              c.proxyID,
+		Address:         lanAddr.Address,
+		Port:            lanAddr.Port,
+		Meta:            meta,
+		TaggedAddresses: taggedAddrs,
+		Proxy:           proxyConf,
+		Check:           check,
+		Locality:        locality,
+	}, nil
+}
+
 var errUnsupportedOS = errors.New("envoy: not implemented on this operating system")
 
 func (c *cmd) findBinary() (string, error) {

command/connect/envoy/envoy_test.go

@@ -1399,6 +1399,83 @@ func TestEnvoy_GatewayRegistration(t *testing.T) {
 	}
 }
 
+func TestEnvoy_proxyRegistration(t *testing.T) {
+	t.Parallel()
+
+	type args struct {
+		svcForProxy api.AgentService
+		cmdFn       func(*cmd)
+	}
+
+	cases := []struct {
+		name   string
+		args   args
+		testFn func(*testing.T, args, *api.AgentServiceRegistration)
+	}{
+		{
+			"locality is inherited from proxied service if configured and using sidecarFor",
+			args{
+				svcForProxy: api.AgentService{
+					ID: "my-svc",
+					Locality: &api.Locality{
+						Region: "us-east-1",
+						Zone:   "us-east-1a",
+					},
+				},
+				cmdFn: func(c *cmd) {
+					c.sidecarFor = "my-svc"
+				},
+			},
+			func(t *testing.T, args args, r *api.AgentServiceRegistration) {
+				assert.NotNil(t, r.Locality)
+				assert.Equal(t, args.svcForProxy.Locality, r.Locality)
+			},
+		},
+		{
+			"locality is not inherited if not using sidecarFor",
+			args{
+				svcForProxy: api.AgentService{
+					ID: "my-svc",
+					Locality: &api.Locality{
+						Region: "us-east-1",
+						Zone:   "us-east-1a",
+					},
+				},
+			},
+			func(t *testing.T, args args, r *api.AgentServiceRegistration) {
+				assert.Nil(t, r.Locality)
+			},
+		},
+		{
+			"locality is not set if not configured for proxied service",
+			args{
+				svcForProxy: api.AgentService{},
+				cmdFn: func(c *cmd) {
+					c.sidecarFor = "my-svc"
+				},
+			},
+			func(t *testing.T, args args, r *api.AgentServiceRegistration) {
+				assert.Nil(t, r.Locality)
+			},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			ui := cli.NewMockUi()
+			c := New(ui)
+
+			if tc.args.cmdFn != nil {
+				tc.args.cmdFn(c)
+			}
+
+			result, err := c.proxyRegistration(&tc.args.svcForProxy)
+			assert.NoError(t, err)
+			tc.testFn(t, tc.args, result)
+		})
+	}
+}
+
 // testMockAgent combines testMockAgentProxyConfig and testMockAgentSelf,
 // routing /agent/service/... requests to testMockAgentProxyConfig,
 // routing /catalog/node-services/... requests to testMockCatalogNodeServiceList

command/connect/proxy/proxy.go

@@ -215,40 +215,42 @@ func (c *cmd) Run(args []string) int {
 	return 0
 }
 
-func (c *cmd) lookupProxyIDForSidecar(client *api.Client) (string, error) {
+func (c *cmd) lookupServiceForSidecar(client *api.Client) (*api.AgentService, error) {
-	return LookupProxyIDForSidecar(client, c.sidecarFor)
+	return LookupServiceForSidecar(client, c.sidecarFor)
 }
 
-// LookupProxyIDForSidecar finds candidate local proxy registrations that are a
+// LookupServiceForSidecar finds candidate local proxy registrations that are a
-// sidecar for the given service. It will return an ID if and only if there is
+// sidecar for the given service. It will return that service if and only if there
-// exactly one registered connect proxy with `Proxy.DestinationServiceID` set to
+// is exactly one registered connect proxy with `Proxy.DestinationServiceID` set to
 // the specified service ID.
 //
 // This is exported to share it with the connect envoy command.
-func LookupProxyIDForSidecar(client *api.Client, sidecarFor string) (string, error) {
+func LookupServiceForSidecar(client *api.Client, sidecarFor string) (*api.AgentService, error) {
 	svcs, err := client.Agent().Services()
 	if err != nil {
-		return "", fmt.Errorf("Failed looking up sidecar proxy info for %s: %s",
+		return nil, fmt.Errorf("Failed looking up sidecar proxy info for %s: %s",
 			sidecarFor, err)
 	}
 
-	var proxyIDs []string
+	var matched []*api.AgentService
+	var matchedProxyIDs []string
 	for _, svc := range svcs {
 		if svc.Kind == api.ServiceKindConnectProxy && svc.Proxy != nil &&
 			strings.EqualFold(svc.Proxy.DestinationServiceID, sidecarFor) {
-			proxyIDs = append(proxyIDs, svc.ID)
+			matched = append(matched, svc)
+			matchedProxyIDs = append(matchedProxyIDs, svc.ID)
 		}
 	}
 
-	if len(proxyIDs) == 0 {
+	if len(matched) == 0 {
-		return "", fmt.Errorf("No sidecar proxy registered for %s", sidecarFor)
+		return nil, fmt.Errorf("No sidecar proxy registered for %s", sidecarFor)
 	}
-	if len(proxyIDs) > 1 {
+	if len(matched) > 1 {
-		return "", fmt.Errorf("More than one sidecar proxy registered for %s.\n"+
+		return nil, fmt.Errorf("More than one sidecar proxy registered for %s.\n"+
 			"    Start proxy with -proxy-id and one of the following IDs: %s",
-			sidecarFor, strings.Join(proxyIDs, ", "))
+			sidecarFor, strings.Join(matchedProxyIDs, ", "))
 	}
-	return proxyIDs[0], nil
+	return matched[0], nil
 }
 
 // LookupGatewayProxy finds the gateway service registered with the local
@@ -285,10 +287,11 @@ func (c *cmd) configWatcher(client *api.Client) (proxyImpl.ConfigWatcher, error)
 		// Running as a sidecar, we need to find the proxy-id for the requested
 		// service
 		var err error
-		c.proxyID, err = c.lookupProxyIDForSidecar(client)
+		svc, err := c.lookupServiceForSidecar(client)
 		if err != nil {
 			return nil, err
 		}
+		c.proxyID = svc.ID
 
 		c.UI.Info("Configuration mode: Agent API")
 		c.UI.Info(fmt.Sprintf("    Sidecar for ID: %s", c.sidecarFor))