peering: support peering establishment over mesh gateways, remove ability to pass external addresses to token generation endpoint #1610

ndhanushkodi · 2022-10-11T16:18:28Z

Changes proposed in this PR:

In Consul 1.13, the way to establish a peering across 2 different networks was to use the expose-servers service to make the Consul servers reachable for peering establishment, and pass the externally reachable addresses of that service into the generate-token endpoint. This way, the peering token would contain these externally reachable server addresses rather than the Consul server pod ips.

Now, with support for peering establishment over mesh gateways, we can use the mesh gateways to reach the servers instead. So this PR removes support entirely for using the expose-servers service for peering. However, the service itself should remain since that will be used for admin partitions.

Now, the peering test uses mesh gateways for peering establishment by configuring the mesh config entry.

How I've tested this PR:

Ran the test locally, and verified via logs that peerings are being established both through the local and remote mesh gateways.

Ran the acceptance tests against all the clouds, passing build: https://app.circleci.com/pipelines/github/hashicorp/consul-k8s/8588/workflows/8fdab6f0-0fc6-48d4-abbc-5d86535ce55b

How I expect reviewers to test this PR:
👀

Checklist:

Tests added
CHANGELOG entry added

HashiCorp engineers only, community PRs should not add a changelog entry.
Entries should use present tense (e.g. Add support for...)

…rom expose service and use mesh gw image

peering tests

when running with these images: go test -run TestPeering_Connect -p 1 -timeout 15m -kubeconfig=/Users/nitya/.kube/gke-peering-mesh-1 -secondary-kubeconfig=/Users/nitya/.kube/gke-peering-mesh-2 -enable-enterprise -enable-multi-cluster -consul-k8s-image=ndhanushkodi/consul-k8s-dev:pmgw1 -consul-image=ndhanushkodi/consul-dev:peermeshgw2 -failfast -no-cleanup-on-failure on clusters peering-mesh-1 and peering-mesh-2

…to the right ones

ndhanushkodi · 2022-10-20T02:44:12Z

charts/consul/templates/mesh-gateway-deployment.yaml

@@ -237,7 +237,7 @@ spec:
            {{- if .Values.global.adminPartitions.enabled }}
            -service-partition={{ .Values.global.adminPartitions.name }} \
            {{- end }}
-            -log-level={{ default .Values.global.logLevel }} \
+            -log-level="trace" \


will remove

…ervices are being received only around 3 min into trying out the static server connection

ndhanushkodi · 2022-10-20T16:29:22Z

acceptance/framework/k8s/deploy.go

@@ -96,7 +96,7 @@ func CheckStaticServerConnectionMultipleFailureMessages(t *testing.T, options *k
 		expectedOutput = expectedSuccessOutput
 	}

-	retrier := &retry.Timer{Timeout: 160 * time.Second, Wait: 2 * time.Second}
+	retrier := &retry.Timer{Timeout: 320 * time.Second, Wait: 2 * time.Second}


This was because I noticed that by the time the peering is established and the exported service is actually replicated to the other side, this curl was timing out. It is taking a bit longer for the peering to get set up-- things that cause this are dialing a follower node and needing to retry the peering, a delta xds bug that causes the mesh gateways to take extra time to get their config.

kschoche · 2022-10-20T17:15:19Z

acceptance/tests/peering/peering_connect_test.go

@@ -53,10 +53,14 @@ func TestPeering_Connect(t *testing.T) {

 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
+			fmt.Println("starting test")


Should we replace the Println's with Logger.Log or t.Log or something?

whoops thanks i forgot to remove those, I was just using those locally so i could see what was going on while running the test!

kschoche · 2022-10-20T17:17:30Z

acceptance/tests/peering/peering_connect_test.go

@@ -90,14 +95,13 @@ func TestPeering_Connect(t *testing.T) {
 				staticServerPeerHelmValues["server.exposeGossipAndRPCPorts"] = "true"
 				staticServerPeerHelmValues["meshGateway.service.type"] = "NodePort"
 				staticServerPeerHelmValues["meshGateway.service.nodePort"] = "30100"
-				staticServerPeerHelmValues["server.exposeService.type"] = "NodePort"


QQ: why are we removing the grpc port configs for each of these?

We no longer need the exposeService at all for peering connections, so this removes any configuration of it.

acceptance/tests/peering/peering_connect_test.go

kschoche · 2022-10-20T17:27:35Z

charts/consul/templates/mesh-gateway-deployment.yaml

@@ -237,7 +237,7 @@ spec:
            {{- if .Values.global.adminPartitions.enabled }}
            -service-partition={{ .Values.global.adminPartitions.name }} \
            {{- end }}
-            -log-level={{ default .Values.global.logLevel }} \


was this intentional?

Nitya mentioned above that she will remove it before merge. I have not idea why it didn't show up in the diffs for review. 🤷

curtbushko

Looks fantastic! I just have a few comments but nothing breaking/blocking!

curtbushko · 2022-10-20T17:24:23Z

acceptance/tests/peering/peering_connect_test.go

 			staticServerPeerClusterContext := env.DefaultContext(t)
 			staticClientPeerClusterContext := env.Context(t, environment.SecondaryContextName)

 			commonHelmValues := map[string]string{
+				"global.imageK8S": "ndhanushkodi/consul-k8s-dev:pmgw1",


Don't forget these before merge!

curtbushko · 2022-10-20T17:28:59Z

CHANGELOG.md

-* Peering: Add support for `PeerThroughMeshGateways` in Mesh CRD. [[GH-1478](https://github.com/hashicorp/consul-k8s/pull/1478)]
+* Peering:
+  * Add support for `PeerThroughMeshGateways` in Mesh CRD. [[GH-1478](https://github.com/hashicorp/consul-k8s/pull/1478)]
+  * move support for customizing the server addresses in peering token generation. Instead, mesh gateways should be used


I am not sure but should the token changes from the helm chart be listed as a breaking change?

Yup added it to breaking instead!

charts/consul/templates/server-config-configmap.yaml

kschoche

This looks great! I have not had a chance to test it yet but assuming we're 🍏 everywhere I don't have any blocking comments beyond seconding Curt's comment about probably marking this as a breaking change in the changelog!

…lity to pass external addresses to token generation endpoint (#1610)

ndhanushkodi added 12 commits October 11, 2022 09:02

start adding grpc-tls port

2f52a5e

try running with grpc tls port and explicitly disable getting addrs f…

e002fe4

…rom expose service and use mesh gw image

the tls port shouldn't be separate, and peering should require tls

908a585

not sure if tls and acls must both be enabled

3caf84a

use mesh resource so conn goes through mesh gateways and run only

e696145

peering tests

failing test run with backwards compatible grpc tls port

bfc0bf0

this works if you remove the lb sourceRange

eefa004

remove the static ip, put back in the test cases, and set the images …

42994da

…to the right ones

bring back ent test and set the right replicas

2fde3bf

remove server.exposeService from peering tests

e66b034

remove external address support from generate token endpoint

abd53e1

ndhanushkodi commented Oct 20, 2022

View reviewed changes

ndhanushkodi added 5 commits October 19, 2022 19:49

add bats test

cefc722

remove more unused code

5ea134f

run all tests with latest consul image and add changelog

fb6d811

try with my image to compare to consul dev image

688bf18

run latest image, peering tests, with longer timeout since exported s…

f3782ae

…ervices are being received only around 3 min into trying out the static server connection

ndhanushkodi changed the title ~~changes to support mesh gw~~ peering: support peering establishment over mesh gateways, remove ability to pass external addresses to token generation endpoint Oct 20, 2022

ndhanushkodi marked this pull request as ready for review October 20, 2022 16:05

ndhanushkodi requested review from a team, curtbushko and kschoche and removed request for a team October 20, 2022 16:06

ndhanushkodi commented Oct 20, 2022

View reviewed changes

set ci back to normal

6a97224

kschoche reviewed Oct 20, 2022

View reviewed changes

acceptance/tests/peering/peering_connect_test.go Outdated Show resolved Hide resolved

kschoche reviewed Oct 20, 2022

View reviewed changes

curtbushko requested changes Oct 20, 2022

View reviewed changes

kschoche reviewed Oct 20, 2022

View reviewed changes

charts/consul/templates/server-config-configmap.yaml Show resolved Hide resolved

kschoche approved these changes Oct 20, 2022

View reviewed changes

curtbushko approved these changes Oct 20, 2022

View reviewed changes

ndhanushkodi added 4 commits October 20, 2022 15:04

try with xds fix

da61b7d

address review comments

5e4f6c8

can't use xds image since it's oss only

aa7391a

revert log level

06a9ec7

ndhanushkodi merged commit 5668d4c into main Oct 20, 2022

ndhanushkodi deleted the nd/peering-mesh-gw branch October 20, 2022 23:56

t-eckert pushed a commit that referenced this pull request Oct 31, 2022

peering: support peering establishment over mesh gateways, remove abi…

540d74a

…lity to pass external addresses to token generation endpoint (#1610)

natitomattis mentioned this pull request Jun 20, 2023

INFRA 11282 team-telnyx/consul-k8s#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

peering: support peering establishment over mesh gateways, remove ability to pass external addresses to token generation endpoint #1610

peering: support peering establishment over mesh gateways, remove ability to pass external addresses to token generation endpoint #1610

ndhanushkodi commented Oct 11, 2022 •

edited

ndhanushkodi Oct 20, 2022

ndhanushkodi Oct 20, 2022

kschoche Oct 20, 2022 •

edited

ndhanushkodi Oct 20, 2022

kschoche Oct 20, 2022

ndhanushkodi Oct 20, 2022

kschoche Oct 20, 2022

curtbushko Oct 20, 2022

kschoche Oct 20, 2022

curtbushko left a comment

curtbushko Oct 20, 2022

ndhanushkodi Oct 20, 2022

curtbushko Oct 20, 2022

ndhanushkodi Oct 20, 2022

kschoche left a comment

peering: support peering establishment over mesh gateways, remove ability to pass external addresses to token generation endpoint #1610

peering: support peering establishment over mesh gateways, remove ability to pass external addresses to token generation endpoint #1610

Conversation

ndhanushkodi commented Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschoche Oct 20, 2022 • edited

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

Choose a reason for hiding this comment

curtbushko 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

kschoche left a comment

Choose a reason for hiding this comment

ndhanushkodi commented Oct 11, 2022 •

edited

kschoche Oct 20, 2022 •

edited