Make the mesh gateway changes to allow `local` mode for cluster peering data plane traffic #14817

erichaberkorn · 2022-09-30T16:07:31Z

Description

This PR creates Envoy clusters and envoys on mesh gateways that forward requests to a mesh gateway in another admin partition. These Envoy resources are used to enable mesh gateway local mode with cluster peering.

Testing & Reproduction steps

Manually set up cluster peering and verified that this works
Unit tests
Integration tests

PR Checklist

updated test coverage
external facing docs updated
not a security concern

erichaberkorn · 2022-10-03T13:24:49Z

agent/xds/clusters.go

+					return nil, fmt.Errorf("couldn't get SNI for peered service %s", sn.String())
+				}
+				clusterName = node.Service.Connect.PeerMeta.PrimarySNI()
+				isRemote = !cfgSnap.Locality.Matches(node.Node.Datacenter, node.Node.PartitionOrDefault())


Is this right? I copied the outline of this function from makeGatewayServiceClusters and am not 100% sure what to do here.

isRemote could just be set to true on L565 since we already know we're dialing a different cluster

Though an issue here and in endpoints.go is that the hostname handling is missing. We need to check whether the addresses attached to these services are hostnames.

Take a look at makeUpstreamLoadAssignmentForPeerService and makeUpstreamClusterForPeerService.

Since the remote addresses are mesh gateway addresses they're likely to have a hostname address (cloud provider LB) and we can't send hostnames via EDS. We instead attach them to a cluster with DNS as the discovery type.

agent/xds/testdata/clusters/ingress-with-defaults-service-max-connections.latest.golden

test/integration/connect/envoy/case-cross-peers/primary/service_s1.hcl

erichaberkorn · 2022-10-03T13:28:51Z

agent/structs/testing_catalog.go

@@ -56,18 +56,18 @@ func TestNodeServiceWithName(t testing.T, name string) *NodeService {
 const peerTrustDomain = "1c053652-8512-4373-90cf-5a7f6263a994.consul"

 func TestNodeServiceWithNameInPeer(t testing.T, name string, peer string) *NodeService {
-	service := "payments"


This change fixed some incorrect failover golden snapshots.

freddygv

Nice progress! I left some comments below.

My main request is that the cluster/endpoint generation needs some adjustment to account for hostname endpoints, since they can't be sent over EDS. Let me know if you have any questions!

test/integration/connect/envoy/case-cross-peers/primary/service_s1.hcl

agent/xds/testdata/clusters/ingress-with-defaults-service-max-connections.latest.golden

freddygv · 2022-10-04T23:33:25Z

agent/xds/endpoints.go

+	for _, serviceGroups := range cfgSnap.MeshGateway.PeeringServices {
+		for sn, serviceGroup := range serviceGroups {
+			var clusterName string
+			if len(serviceGroup) > 0 {


Doesn't seem like we should proceed if len(serviceGroup) == 0, since there won't be any endpoints and the clusterName won't be populated

freddygv · 2022-10-04T23:35:55Z

agent/xds/clusters.go

+		for sn, serviceGroup := range serviceGroups {
+			var clusterName string
+			var isRemote bool
+			if len(serviceGroup) > 0 {


Same as with endpoints, if we can't get a clusterName we probably shouldn't attempt to make a cluster

Could do:

if len(serviceGroup) == 0 { continue }

freddygv · 2022-10-04T23:42:46Z

agent/xds/clusters.go

+					return nil, fmt.Errorf("couldn't get SNI for peered service %s", sn.String())
+				}
+				clusterName = node.Service.Connect.PeerMeta.PrimarySNI()
+				isRemote = !cfgSnap.Locality.Matches(node.Node.Datacenter, node.Node.PartitionOrDefault())


isRemote could just be set to true on L565 since we already know we're dialing a different cluster

Though an issue here and in endpoints.go is that the hostname handling is missing. We need to check whether the addresses attached to these services are hostnames.

Take a look at makeUpstreamLoadAssignmentForPeerService and makeUpstreamClusterForPeerService.

Since the remote addresses are mesh gateway addresses they're likely to have a hostname address (cloud provider LB) and we can't send hostnames via EDS. We instead attach them to a cluster with DNS as the discovery type.

freddygv · 2022-10-04T23:54:03Z

test/integration/connect/envoy/case-cross-peers/primary/verify.bats

@@ -55,3 +55,7 @@ load helpers
 @test "s1 upstream made 1 connection to s2" {
  assert_envoy_metric_at_least 127.0.0.1:19000 "cluster.s2.default.primary-to-alpha.external.*cx_total" 1
 }
+
+@test "s1 upstream made 1 connection to s2 through the primary mesh gateway" {
+  assert_envoy_metric_at_least 127.0.0.1:19001 "cluster.s2.default.default.alpha-to-primary.external.*cx_total" 1


This seems unusual, why is alpha-to-primary here? That's the name that alpha uses to refer to primary, so why would the local primary gateway be aware of that?

Yeah, I was confused by this but was hoping there was some logic behind it. This matches the SNI created by the sidecar proxy 🤔

Ah I see what's going on. That's how the remote side can know that this traffic is for an exported service as opposed to the usual mesh gateway route for s2. See this comment: https://github.com/hashicorp/consul/blob/main/agent/xds/clusters.go#L1284-L1298

I'm a bit worried about whether there could be collisions on the dialing side though.

This SNI encodes: (service, namespace, partition, peerName, trustDomain) so I think there could be a collision if there is a separate peering with all the same data but different DC.

Actually that would require overriding the trust domain to make it the same on multiple non-WAN-federated DCs. We already need people not to do this because it would also lead to control-plane MGW collisions. So maybe just something to document.

…ng data plane traffic

…r peering data plane traffic

freddygv

erichaberkorn force-pushed the mesh-gw-dp-peer branch from 49df83c to c497ea4 Compare September 30, 2022 16:08

github-actions bot added the theme/envoy/xds Related to Envoy support label Sep 30, 2022

erichaberkorn force-pushed the mesh-gw-dp-peer branch from c497ea4 to 89033ea Compare October 3, 2022 13:29

erichaberkorn commented Oct 3, 2022

View reviewed changes

erichaberkorn marked this pull request as ready for review October 3, 2022 14:03

erichaberkorn requested a review from a team October 3, 2022 14:03

freddygv suggested changes Oct 4, 2022

View reviewed changes

Make the mesh gateway changes to allow local mode for cluster peeri…

e5aca17

…ng data plane traffic

erichaberkorn force-pushed the mesh-gw-dp-peer branch 2 times, most recently from edccae0 to ebe0a48 Compare October 5, 2022 16:26

fixup! Make the mesh gateway changes to allow local mode for cluste…

62b548f

…r peering data plane traffic

erichaberkorn force-pushed the mesh-gw-dp-peer branch from ebe0a48 to 62b548f Compare October 5, 2022 16:46

erichaberkorn requested a review from freddygv October 5, 2022 18:00

freddygv approved these changes Oct 5, 2022

View reviewed changes

erichaberkorn merged commit 1633cf2 into main Oct 6, 2022

erichaberkorn deleted the mesh-gw-dp-peer branch October 6, 2022 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the mesh gateway changes to allow `local` mode for cluster peering data plane traffic #14817

Make the mesh gateway changes to allow `local` mode for cluster peering data plane traffic #14817

erichaberkorn commented Sep 30, 2022

erichaberkorn Oct 3, 2022

freddygv Oct 4, 2022

erichaberkorn Oct 3, 2022

freddygv left a comment

freddygv Oct 4, 2022

freddygv Oct 4, 2022

freddygv Oct 4, 2022

freddygv Oct 4, 2022

erichaberkorn Oct 5, 2022

freddygv Oct 5, 2022 •

edited

Loading

freddygv Oct 5, 2022

freddygv left a comment

Make the mesh gateway changes to allow local mode for cluster peering data plane traffic #14817

Make the mesh gateway changes to allow local mode for cluster peering data plane traffic #14817

Conversation

erichaberkorn commented Sep 30, 2022

Description

Testing & Reproduction steps

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddygv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddygv Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freddygv left a comment

Choose a reason for hiding this comment

Make the mesh gateway changes to allow `local` mode for cluster peering data plane traffic #14817

Make the mesh gateway changes to allow `local` mode for cluster peering data plane traffic #14817

freddygv Oct 5, 2022 •

edited

Loading