From d87e4acb4d1902c55ef69e62fbe64effde0d75c0 Mon Sep 17 00:00:00 2001 From: Derek Menteer <105233703+hashi-derek@users.noreply.github.com> Date: Thu, 16 Feb 2023 09:22:41 -0600 Subject: [PATCH] Fix mesh gateways incorrectly matching peer locality. (#16257) Fix mesh gateways incorrectly matching peer locality. This fixes an issue where local mesh gateways use an incorrect address when attempting to forward traffic to a peered datacenter. Prior to this change it would use the lan address instead of the wan if the locality matched. This should never be done for peering, since we must route all traffic through the remote mesh gateway. --- .changelog/16257.txt | 3 +++ agent/proxycfg/testing_mesh_gateway.go | 8 ++++---- agent/proxycfg/testing_upstreams.go | 4 ++-- agent/structs/testing_catalog.go | 27 ++++++++++++++++++-------- agent/xds/endpoints.go | 4 +++- 5 files changed, 31 insertions(+), 15 deletions(-) create mode 100644 .changelog/16257.txt diff --git a/.changelog/16257.txt b/.changelog/16257.txt new file mode 100644 index 000000000..8e98530c4 --- /dev/null +++ b/.changelog/16257.txt @@ -0,0 +1,3 @@ +```release-note:bug +peering: Fix issue where mesh gateways would use the wrong address when contacting a remote peer with the same datacenter name. +``` diff --git a/agent/proxycfg/testing_mesh_gateway.go b/agent/proxycfg/testing_mesh_gateway.go index 039807ed6..f6b463f9d 100644 --- a/agent/proxycfg/testing_mesh_gateway.go +++ b/agent/proxycfg/testing_mesh_gateway.go @@ -659,8 +659,8 @@ func TestConfigSnapshotPeeredMeshGateway(t testing.T, variant string, nsFn func( CorrelationID: "peering-connect-service:peer-a:db", Result: &structs.IndexedCheckServiceNodes{ Nodes: structs.CheckServiceNodes{ - structs.TestCheckNodeServiceWithNameInPeer(t, "db", "peer-a", "10.40.1.1", false), - structs.TestCheckNodeServiceWithNameInPeer(t, "db", "peer-a", "10.40.1.2", false), + structs.TestCheckNodeServiceWithNameInPeer(t, "db", "dc1", "peer-a", "10.40.1.1", false), + structs.TestCheckNodeServiceWithNameInPeer(t, "db", "dc1", "peer-a", "10.40.1.2", false), }, }, }, @@ -668,8 +668,8 @@ func TestConfigSnapshotPeeredMeshGateway(t testing.T, variant string, nsFn func( CorrelationID: "peering-connect-service:peer-b:alt", Result: &structs.IndexedCheckServiceNodes{ Nodes: structs.CheckServiceNodes{ - structs.TestCheckNodeServiceWithNameInPeer(t, "alt", "peer-b", "10.40.2.1", false), - structs.TestCheckNodeServiceWithNameInPeer(t, "alt", "peer-b", "10.40.2.2", true), + structs.TestCheckNodeServiceWithNameInPeer(t, "alt", "remote-dc", "peer-b", "10.40.2.1", false), + structs.TestCheckNodeServiceWithNameInPeer(t, "alt", "remote-dc", "peer-b", "10.40.2.2", true), }, }, }, diff --git a/agent/proxycfg/testing_upstreams.go b/agent/proxycfg/testing_upstreams.go index ed2e65d8d..cb38a3de1 100644 --- a/agent/proxycfg/testing_upstreams.go +++ b/agent/proxycfg/testing_upstreams.go @@ -94,7 +94,7 @@ func setupTestVariationConfigEntriesAndSnapshot( events = append(events, UpdateEvent{ CorrelationID: "upstream-peer:db?peer=cluster-01", Result: &structs.IndexedCheckServiceNodes{ - Nodes: structs.CheckServiceNodes{structs.TestCheckNodeServiceWithNameInPeer(t, "db", "cluster-01", "10.40.1.1", false)}, + Nodes: structs.CheckServiceNodes{structs.TestCheckNodeServiceWithNameInPeer(t, "db", "dc1", "cluster-01", "10.40.1.1", false)}, }, }) case "redirect-to-cluster-peer": @@ -112,7 +112,7 @@ func setupTestVariationConfigEntriesAndSnapshot( events = append(events, UpdateEvent{ CorrelationID: "upstream-peer:db?peer=cluster-01", Result: &structs.IndexedCheckServiceNodes{ - Nodes: structs.CheckServiceNodes{structs.TestCheckNodeServiceWithNameInPeer(t, "db", "cluster-01", "10.40.1.1", false)}, + Nodes: structs.CheckServiceNodes{structs.TestCheckNodeServiceWithNameInPeer(t, "db", "dc2", "cluster-01", "10.40.1.1", false)}, }, }) case "failover-through-double-remote-gateway-triggered": diff --git a/agent/structs/testing_catalog.go b/agent/structs/testing_catalog.go index 113169051..f7f2a7e71 100644 --- a/agent/structs/testing_catalog.go +++ b/agent/structs/testing_catalog.go @@ -55,11 +55,13 @@ func TestNodeServiceWithName(t testing.T, name string) *NodeService { const peerTrustDomain = "1c053652-8512-4373-90cf-5a7f6263a994.consul" -func TestCheckNodeServiceWithNameInPeer(t testing.T, name, peer, ip string, useHostname bool) CheckServiceNode { +func TestCheckNodeServiceWithNameInPeer(t testing.T, name, dc, peer, ip string, useHostname bool) CheckServiceNode { service := &NodeService{ - Kind: ServiceKindTypical, - Service: name, - Port: 8080, + Kind: ServiceKindTypical, + Service: name, + // We should not see this port number appear in most xds golden tests, + // because the WAN addr should typically be used. + Port: 9090, PeerName: peer, Connect: ServiceConnect{ PeerMeta: &PeeringServiceMeta{ @@ -72,6 +74,13 @@ func TestCheckNodeServiceWithNameInPeer(t testing.T, name, peer, ip string, useH Protocol: "tcp", }, }, + // This value should typically be seen in golden file output, since this is a peered service. + TaggedAddresses: map[string]ServiceAddress{ + TaggedAddressWAN: { + Address: ip, + Port: 8080, + }, + }, } if useHostname { @@ -89,10 +98,12 @@ func TestCheckNodeServiceWithNameInPeer(t testing.T, name, peer, ip string, useH return CheckServiceNode{ Node: &Node{ - ID: "test1", - Node: "test1", - Address: ip, - Datacenter: "cloud-dc", + ID: "test1", + Node: "test1", + // We should not see this address appear in most xds golden tests, + // because the WAN addr should typically be used. + Address: "1.23.45.67", + Datacenter: dc, }, Service: service, } diff --git a/agent/xds/endpoints.go b/agent/xds/endpoints.go index dcdbb4d2f..d117f8dd8 100644 --- a/agent/xds/endpoints.go +++ b/agent/xds/endpoints.go @@ -449,7 +449,9 @@ func (s *ResourceGenerator) makeEndpointsForOutgoingPeeredServices( la := makeLoadAssignment( clusterName, groups, - cfgSnap.Locality, + // Use an empty key here so that it never matches. This will force the mesh gateway to always + // reference the remote mesh gateway's wan addr. + proxycfg.GatewayKey{}, ) resources = append(resources, la) }