[NET-5217] [OSS] Derive sidecar proxy locality from parent service (#…

…18437) * Add logging to locality policy application In OSS, this is currently a no-op. * Inherit locality when registering sidecars When sidecar locality is not explicitly configured, inherit locality from the proxied service.
hashicorp · Aug 10, 2023 · 05604ee · 05604ee
1 parent bee12c6
commit 05604ee
Show file tree

Hide file tree

Showing 10 changed files with 273 additions and 99 deletions.
diff --git a/.changelog/18437.txt b/.changelog/18437.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+Inherit locality from services when registering sidecar proxies.
+```
diff --git a/agent/sidecar_service.go b/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.

diff --git a/agent/sidecar_service_test.go b/agent/sidecar_service_test.go
@@ -134,25 +134,78 @@ 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{},
 				},
 			},
 			wantNS: &structs.NodeService{
-				EnterpriseMeta:             *structs.DefaultEnterpriseMetaInDefaultPartition(),
-				Kind:                       structs.ServiceKindConnectProxy,
-				ID:                         "web1-sidecar-proxy",
-				Service:                    "web-sidecar-proxy",
-				Port:                       0,
-				Tags:                       []string{"foo"},
-				Meta:                       map[string]string{"foo": "bar"},
+				EnterpriseMeta: *structs.DefaultEnterpriseMetaInDefaultPartition(),
+				Kind:           structs.ServiceKindConnectProxy,
+				ID:             "web1-sidecar-proxy",
+				Service:        "web-sidecar-proxy",
+				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",

diff --git a/agent/xds/endpoints.go b/agent/xds/endpoints.go
@@ -6,6 +6,7 @@ package xds
 import (
 	"errors"
 	"fmt"
+	"github.com/hashicorp/go-hclog"
 	"strconv"
 
 	envoy_cluster_v3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
@@ -136,6 +137,7 @@ func (s *ResourceGenerator) endpointsFromSnapshotConnectProxy(cfgSnap *proxycfg.
 		endpoints, ok := cfgSnap.ConnectProxy.PreparedQueryEndpoints[uid]
 		if ok {
 			la := makeLoadAssignment(
+				s.Logger,
 				cfgSnap,
 				clusterName,
 				nil,
@@ -161,6 +163,7 @@ func (s *ResourceGenerator) endpointsFromSnapshotConnectProxy(cfgSnap *proxycfg.
 			endpoints, ok := cfgSnap.ConnectProxy.DestinationGateways.Get(uid)
 			if ok {
 				la := makeLoadAssignment(
+					s.Logger,
 					cfgSnap,
 					name,
 					nil,
@@ -229,6 +232,7 @@ func (s *ResourceGenerator) endpointsFromSnapshotMeshGateway(cfgSnap *proxycfg.C
 			clusterName := connect.GatewaySNI(key.Datacenter, key.Partition, cfgSnap.Roots.TrustDomain)
 
 			la := makeLoadAssignment(
+				s.Logger,
 				cfgSnap,
 				clusterName,
 				nil,
@@ -246,6 +250,7 @@ func (s *ResourceGenerator) endpointsFromSnapshotMeshGateway(cfgSnap *proxycfg.C
 
 			clusterName := cfgSnap.ServerSNIFn(key.Datacenter, "")
 			la := makeLoadAssignment(
+				s.Logger,
 				cfgSnap,
 				clusterName,
 				nil,
@@ -418,6 +423,7 @@ func (s *ResourceGenerator) endpointsFromServicesAndResolvers(
 		for subsetName, groups := range clusterEndpoints {
 			clusterName := connect.ServiceSNI(svc.Name, subsetName, svc.NamespaceOrDefault(), svc.PartitionOrDefault(), cfgSnap.Datacenter, cfgSnap.Roots.TrustDomain)
 			la := makeLoadAssignment(
+				s.Logger,
 				cfgSnap,
 				clusterName,
 				nil,
@@ -455,6 +461,7 @@ func (s *ResourceGenerator) makeEndpointsForOutgoingPeeredServices(
 			groups := []loadAssignmentEndpointGroup{{Endpoints: serviceGroup.Nodes, OnlyPassing: false}}
 
 			la := makeLoadAssignment(
+				s.Logger,
 				cfgSnap,
 				clusterName,
 				nil,
@@ -619,6 +626,7 @@ func (s *ResourceGenerator) makeUpstreamLoadAssignmentForPeerService(
 			return la, nil
 		}
 		la = makeLoadAssignment(
+			s.Logger,
 			cfgSnap,
 			clusterName,
 			nil,
@@ -641,6 +649,7 @@ func (s *ResourceGenerator) makeUpstreamLoadAssignmentForPeerService(
 		return nil, nil
 	}
 	la = makeLoadAssignment(
+		s.Logger,
 		cfgSnap,
 		clusterName,
 		nil,
@@ -773,6 +782,7 @@ func (s *ResourceGenerator) endpointsFromDiscoveryChain(
 			}
 
 			la := makeLoadAssignment(
+				s.Logger,
 				cfgSnap,
 				clusterName,
 				ti.PrioritizeByLocality,
@@ -861,7 +871,7 @@ type loadAssignmentEndpointGroup struct {
 	OverrideHealth envoy_core_v3.HealthStatus
 }
 
-func makeLoadAssignment(cfgSnap *proxycfg.ConfigSnapshot, clusterName string, policy *structs.DiscoveryPrioritizeByLocality, endpointGroups []loadAssignmentEndpointGroup, localKey proxycfg.GatewayKey) *envoy_endpoint_v3.ClusterLoadAssignment {
+func makeLoadAssignment(logger hclog.Logger, cfgSnap *proxycfg.ConfigSnapshot, clusterName string, policy *structs.DiscoveryPrioritizeByLocality, endpointGroups []loadAssignmentEndpointGroup, localKey proxycfg.GatewayKey) *envoy_endpoint_v3.ClusterLoadAssignment {
 	cla := &envoy_endpoint_v3.ClusterLoadAssignment{
 		ClusterName: clusterName,
 		Endpoints:   make([]*envoy_endpoint_v3.LocalityLbEndpoints, 0, len(endpointGroups)),
@@ -878,7 +888,7 @@ func makeLoadAssignment(cfgSnap *proxycfg.ConfigSnapshot, clusterName string, po
 	var priority uint32
 
 	for _, endpointGroup := range endpointGroups {
-		endpointsByLocality, err := groupedEndpoints(cfgSnap.ServiceLocality, policy, endpointGroup.Endpoints)
+		endpointsByLocality, err := groupedEndpoints(logger, cfgSnap.ServiceLocality, policy, endpointGroup.Endpoints)
 
 		if err != nil {
 			continue

diff --git a/agent/xds/endpoints_test.go b/agent/xds/endpoints_test.go
@@ -4,6 +4,7 @@
 package xds
 
 import (
+	"github.com/hashicorp/go-hclog"
 	"path/filepath"
 	"sort"
 	"testing"
@@ -213,6 +214,7 @@ func Test_makeLoadAssignment(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got := makeLoadAssignment(
+				hclog.NewNullLogger(),
 				&proxycfg.ConfigSnapshot{ServiceLocality: tt.locality},
 				tt.clusterName,
 				nil,
@@ -223,6 +225,7 @@ func Test_makeLoadAssignment(t *testing.T) {
 
 			if tt.locality == nil {
 				got := makeLoadAssignment(
+					hclog.NewNullLogger(),
 					&proxycfg.ConfigSnapshot{ServiceLocality: &structs.Locality{Region: "us-west-1", Zone: "us-west-1a"}},
 					tt.clusterName,
 					nil,

diff --git a/agent/xds/locality_policy.go b/agent/xds/locality_policy.go
@@ -5,16 +5,18 @@ package xds
 
 import (
 	"fmt"
+	"github.com/hashicorp/go-hclog"
 
 	"github.com/hashicorp/consul/agent/structs"
 )
 
-func groupedEndpoints(locality *structs.Locality, policy *structs.DiscoveryPrioritizeByLocality, csns structs.CheckServiceNodes) ([]structs.CheckServiceNodes, error) {
+func groupedEndpoints(logger hclog.Logger, locality *structs.Locality, policy *structs.DiscoveryPrioritizeByLocality, csns structs.CheckServiceNodes) ([]structs.CheckServiceNodes, error) {
 	switch {
 	case policy == nil || policy.Mode == "" || policy.Mode == "none":
 		return []structs.CheckServiceNodes{csns}, nil
 	case policy.Mode == "failover":
-		return prioritizeByLocalityFailover(locality, csns), nil
+		log := logger.Named("locality")
+		return prioritizeByLocalityFailover(log, locality, csns), nil
 	default:
 		return nil, fmt.Errorf("unexpected priortize-by-locality mode %q", policy.Mode)
 	}

diff --git a/agent/xds/locality_policy_oss.go b/agent/xds/locality_policy_oss.go
@@ -8,8 +8,9 @@ package xds
 
 import (
 	"github.com/hashicorp/consul/agent/structs"
+	"github.com/hashicorp/go-hclog"
 )
 
-func prioritizeByLocalityFailover(locality *structs.Locality, csns structs.CheckServiceNodes) []structs.CheckServiceNodes {
+func prioritizeByLocalityFailover(_ hclog.Logger, _ *structs.Locality, _ structs.CheckServiceNodes) []structs.CheckServiceNodes {
 	return nil
 }