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

…lity to pass external addresses to token generation endpoint (#1610)
hashicorp · Oct 31, 2022 · 540d74a · 540d74a
1 parent 73e9377
commit 540d74a
Show file tree

Hide file tree

Showing 14 changed files with 104 additions and 707 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,7 +13,8 @@ IMPROVEMENTS:
 ## 1.0.0-beta3 (October 12, 2022)
 
 FEATURES:
-* 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)]
 
 BREAKING CHANGES:
 * Helm:
@@ -28,7 +29,10 @@ IMPROVEMENTS:
 
 ## 1.0.0-beta2 (October 7, 2022)
 BREAKING CHANGES:
-* Peering: Rename `PeerName` to `Peer` in ExportedServices CRD. [[GH-1596](https://github.com/hashicorp/consul-k8s/pull/1596)]
+* Peering:
+  * Rename `PeerName` to `Peer` in ExportedServices CRD. [[GH-1596](https://github.com/hashicorp/consul-k8s/pull/1596)]
+  * Remove support for customizing the server addresses in peering token generation. Instead, mesh gateways should be used
+    to establish peering connections if the server pods are not directly reachable. [[GH-1610](https://github.com/hashicorp/consul-k8s/pull/1610)]
 * Helm 
   * `server.replicas` now defaults to `1`. Formerly, this defaulted to `3`. [[GH-1551](https://github.com/hashicorp/consul-k8s/pull/1551)]
   * `connectInject.enabled` now defaults to `true`. [[GH-1551](https://github.com/hashicorp/consul-k8s/pull/1551)]

diff --git a/acceptance/framework/k8s/deploy.go b/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}
 
 	args := []string{"exec", "deploy/" + sourceApp, "-c", sourceApp, "--", "curl", "-vvvsSf"}
 	args = append(args, curlArgs...)

diff --git a/acceptance/tests/fixtures/bases/mesh-peering/kustomization.yaml b/acceptance/tests/fixtures/bases/mesh-peering/kustomization.yaml
@@ -0,0 +1,2 @@
+resources:
+  - meshpeering.yaml
diff --git a/acceptance/tests/fixtures/bases/mesh-peering/meshpeering.yaml b/acceptance/tests/fixtures/bases/mesh-peering/meshpeering.yaml
@@ -0,0 +1,7 @@
+apiVersion: consul.hashicorp.com/v1alpha1
+kind: Mesh
+metadata:
+  name: mesh
+spec:
+  peering:
+    peerThroughMeshGateways: true
diff --git a/acceptance/tests/peering/peering_connect_namespaces_test.go b/acceptance/tests/peering/peering_connect_namespaces_test.go
@@ -135,8 +135,6 @@ func TestPeering_ConnectNamespaces(t *testing.T) {
 				staticServerPeerHelmValues["server.exposeGossipAndRPCPorts"] = "true"
 				staticServerPeerHelmValues["meshGateway.service.type"] = "NodePort"
 				staticServerPeerHelmValues["meshGateway.service.nodePort"] = "30100"
-				staticServerPeerHelmValues["server.exposeService.type"] = "NodePort"
-				staticServerPeerHelmValues["server.exposeService.nodePort.grpc"] = "30200"
 			}
 
 			releaseName := helpers.RandomName()
@@ -159,8 +157,6 @@ func TestPeering_ConnectNamespaces(t *testing.T) {
 				staticClientPeerHelmValues["server.exposeGossipAndRPCPorts"] = "true"
 				staticClientPeerHelmValues["meshGateway.service.type"] = "NodePort"
 				staticClientPeerHelmValues["meshGateway.service.nodePort"] = "30100"
-				staticClientPeerHelmValues["server.exposeService.type"] = "NodePort"
-				staticClientPeerHelmValues["server.exposeService.nodePort.grpc"] = "30200"
 			}
 
 			helpers.MergeMaps(staticClientPeerHelmValues, commonHelmValues)
@@ -169,6 +165,20 @@ func TestPeering_ConnectNamespaces(t *testing.T) {
 			staticClientPeerCluster := consul.NewHelmCluster(t, staticClientPeerHelmValues, staticClientPeerClusterContext, cfg, releaseName)
 			staticClientPeerCluster.Create(t)
 
+			// Create Mesh resource to use mesh gateways.
+			logger.Log(t, "creating mesh config")
+			kustomizeMeshDir := "../fixtures/bases/mesh-peering"
+
+			k8s.KubectlApplyK(t, staticServerPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() {
+				k8s.KubectlDeleteK(t, staticServerPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			})
+
+			k8s.KubectlApplyK(t, staticClientPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() {
+				k8s.KubectlDeleteK(t, staticClientPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			})
+
 			// Create the peering acceptor on the client peer.
 			k8s.KubectlApply(t, staticClientPeerClusterContext.KubectlOptions(t), "../fixtures/bases/peering/peering-acceptor.yaml")
 			helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() {

diff --git a/acceptance/tests/peering/peering_connect_test.go b/acceptance/tests/peering/peering_connect_test.go
@@ -73,6 +73,7 @@ func TestPeering_Connect(t *testing.T) {
 
 				"dns.enabled":           "true",
 				"dns.enableRedirection": strconv.FormatBool(cfg.EnableTransparentProxy),
+				"peering.tokenGeneration.serverAddresses.source": "consul",
 			}
 
 			staticServerPeerHelmValues := map[string]string{
@@ -90,8 +91,6 @@ 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"
-				staticServerPeerHelmValues["server.exposeService.nodePort.grpc"] = "30200"
 			}
 
 			releaseName := helpers.RandomName()
@@ -107,15 +106,13 @@ func TestPeering_Connect(t *testing.T) {
 			}
 
 			if !cfg.UseKind {
-				staticServerPeerHelmValues["server.replicas"] = "3"
+				staticClientPeerHelmValues["server.replicas"] = "3"
 			}
 
 			if cfg.UseKind {
 				staticClientPeerHelmValues["server.exposeGossipAndRPCPorts"] = "true"
 				staticClientPeerHelmValues["meshGateway.service.type"] = "NodePort"
 				staticClientPeerHelmValues["meshGateway.service.nodePort"] = "30100"
-				staticClientPeerHelmValues["server.exposeService.type"] = "NodePort"
-				staticClientPeerHelmValues["server.exposeService.nodePort.grpc"] = "30200"
 			}
 
 			helpers.MergeMaps(staticClientPeerHelmValues, commonHelmValues)
@@ -124,6 +121,20 @@ func TestPeering_Connect(t *testing.T) {
 			staticClientPeerCluster := consul.NewHelmCluster(t, staticClientPeerHelmValues, staticClientPeerClusterContext, cfg, releaseName)
 			staticClientPeerCluster.Create(t)
 
+			// Create Mesh resource to use mesh gateways.
+			logger.Log(t, "creating mesh config")
+			kustomizeMeshDir := "../fixtures/bases/mesh-peering"
+
+			k8s.KubectlApplyK(t, staticServerPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() {
+				k8s.KubectlDeleteK(t, staticServerPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			})
+
+			k8s.KubectlApplyK(t, staticClientPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() {
+				k8s.KubectlDeleteK(t, staticClientPeerClusterContext.KubectlOptions(t), kustomizeMeshDir)
+			})
+
 			// Create the peering acceptor on the client peer.
 			k8s.KubectlApply(t, staticClientPeerClusterContext.KubectlOptions(t), "../fixtures/bases/peering/peering-acceptor.yaml")
 			helpers.Cleanup(t, cfg.NoCleanupOnFailure, func() {
@@ -261,6 +272,7 @@ func TestPeering_Connect(t *testing.T) {
 			} else {
 				k8s.CheckStaticServerConnectionSuccessful(t, staticClientOpts, staticClientName, "http://localhost:1234")
 			}
+
 		})
 	}
 }
diff --git a/charts/consul/templates/connect-inject-deployment.yaml b/charts/consul/templates/connect-inject-deployment.yaml
@@ -4,8 +4,7 @@
 {{ template "consul.validateVaultWebhookCertConfiguration" . }}
 {{- template "consul.reservedNamesFailer" (list .Values.connectInject.consulNamespaces.consulDestinationNamespace "connectInject.consulNamespaces.consulDestinationNamespace") }}
 {{- $serverEnabled := (or (and (ne (.Values.server.enabled | toString) "-") .Values.server.enabled) (and (eq (.Values.server.enabled | toString) "-") .Values.global.enabled)) -}}
-{{- $serverExposeServiceEnabled := (or (and (ne (.Values.server.exposeService.enabled | toString) "-") .Values.server.exposeService.enabled) (and (eq (.Values.server.exposeService.enabled | toString) "-") (or .Values.global.peering.enabled .Values.global.adminPartitions.enabled))) -}}
-{{- if not (or (eq .Values.global.peering.tokenGeneration.serverAddresses.source "") (or (eq .Values.global.peering.tokenGeneration.serverAddresses.source "static") (eq .Values.global.peering.tokenGeneration.serverAddresses.source "consul"))) }}{{ fail "global.peering.tokenGeneration.serverAddresses.source must be one of empty string, 'consul' or 'static'" }}{{ end }}
+{{- $serverExposeServiceEnabled := (or (and (ne (.Values.server.exposeService.enabled | toString) "-") .Values.server.exposeService.enabled) (and (eq (.Values.server.exposeService.enabled | toString) "-") .Values.global.adminPartitions.enabled)) -}}
 {{- if and .Values.externalServers.enabled (not .Values.externalServers.hosts) }}{{ fail "externalServers.hosts must be set if externalServers.enabled is true" }}{{ end -}}
 {{ template "consul.validateRequiredCloudSecretsExist" . }}
 {{ template "consul.validateCloudSecretKeys" . }}
@@ -142,23 +141,6 @@ spec:
                 -enable-cni={{ .Values.connectInject.cni.enabled }} \
                 {{- if .Values.global.peering.enabled }}
                 -enable-peering=true \
-                {{- if (eq .Values.global.peering.tokenGeneration.serverAddresses.source "") }}
-                {{- if (and $serverEnabled $serverExposeServiceEnabled) }}
-                -read-server-expose-service=true \
-                {{- else }}
-                {{- if .Values.externalServers.enabled }}
-                {{- $port := .Values.externalServers.grpcPort }}
-                {{- range $h := .Values.externalServers.hosts }}
-                -token-server-address="{{ $h }}:{{ $port }}" \
-                {{- end }}
-                {{- end }}
-                {{- end }}
-                {{- end }}
-                {{- if (eq .Values.global.peering.tokenGeneration.serverAddresses.source "static") }}
-                {{- range $addr := .Values.global.peering.tokenGeneration.serverAddresses.static }}
-                -token-server-address="{{ $addr }}" \
-                {{- end }}
-                {{- end }}
                 {{- end }}
                 {{- if .Values.global.openshift.enabled }}
                 -enable-openshift \

diff --git a/charts/consul/templates/server-config-configmap.yaml b/charts/consul/templates/server-config-configmap.yaml
@@ -27,7 +27,12 @@ data:
       "data_dir": "/consul/data",
       "domain": "{{ .Values.global.domain }}",
       "ports": {
+        {{- if not .Values.global.tls.enabled }}
         "grpc": 8502,
+        {{- end }}
+        {{- if .Values.global.tls.enabled }}
+        "grpc_tls": 8502,
+        {{- end }}
         "serf_lan": {{ .Values.server.ports.serflan.port }}
       },
       "recursors": {{ .Values.global.recursors | toJson }},

diff --git a/charts/consul/test/unit/connect-inject-deployment.bats b/charts/consul/test/unit/connect-inject-deployment.bats
@@ -1527,182 +1527,6 @@ load _helpers
   [[ "$output" =~ "setting global.peering.enabled to true requires connectInject.enabled to be true" ]]
 }
 
-@test "connectInject/Deployment: -read-server-expose-service=true is set when global.peering.enabled is true and global.peering.tokenGeneration.serverAddresses.source is empty" {
-  cd `chart_dir`
-  local actual=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
-
-  [ "${actual}" = "true" ]
-}
-
-@test "connectInject/Deployment: -read-server-expose-service=true is set when servers are enabled and peering is enabled" {
-  cd `chart_dir`
-  local actual=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'global.enabled=false' \
-      --set 'server.enabled=true' \
-      --set 'client.enabled=true' \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
-
-  [ "${actual}" = "true" ]
-}
-
-@test "connectInject/Deployment: -read-server-expose-service is not set when servers are disabled" {
-  cd `chart_dir`
-  local actual=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'server.enabled=false' \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
-
-  [ "${actual}" = "false" ]
-}
-
-@test "connectInject/Deployment: -read-server-expose-service is not set when peering is disabled" {
-  cd `chart_dir`
-  local actual=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=false' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
-
-  [ "${actual}" = "false" ]
-}
-
-@test "connectInject/Deployment: -read-server-expose-service is not set when global.peering.tokenGeneration.serverAddresses.source is set to consul" {
-  cd `chart_dir`
-  local actual=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      --set 'global.peering.tokenGeneration.serverAddresses.source=consul' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
-
-  [ "${actual}" = "false" ]
-}
-
-@test "connectInject/Deployment: fails server address source is an invalid value" {
-  cd `chart_dir`
-  run helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      --set 'global.peering.tokenGeneration.serverAddresses.source=notempty' .
-  [ "$status" -eq 1 ]
-  [[ "$output" =~ "global.peering.tokenGeneration.serverAddresses.source must be one of empty string, 'consul' or 'static'" ]]
-}
-
-@test "connectInject/Deployment: -read-server-expose-service and -token-server-address is not set when global.peering.tokenGeneration.serverAddresses.source is consul" {
-  cd `chart_dir`
-  local command=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      --set 'global.peering.tokenGeneration.serverAddresses.source=consul' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command')
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
-  [ "${actual}" = "false" ]
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address"))' | tee /dev/stderr)
-  [ "${actual}" = "false" ]
-}
-
-@test "connectInject/Deployment: when servers are not enabled and externalServers.enabled=true, passes in -token-server-address flags with hosts" {
-  cd `chart_dir`
-  local command=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'server.enabled=false' \
-      --set 'externalServers.enabled=true' \
-      --set 'externalServers.hosts[0]=1.2.3.4' \
-      --set 'externalServers.hosts[1]=2.2.3.4' \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command')
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"1.2.3.4:8502\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"2.2.3.4:8502\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-}
-
-@test "connectInject/Deployment: externalServers.grpcPort can be customized" {
-  cd `chart_dir`
-  local command=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'server.enabled=false' \
-      --set 'externalServers.enabled=true' \
-      --set 'externalServers.hosts[0]=1.2.3.4' \
-      --set 'externalServers.hosts[1]=2.2.3.4' \
-      --set 'externalServers.grpcPort=1234' \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command')
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"1.2.3.4:1234\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"2.2.3.4:1234\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-}
-
-@test "connectInject/Deployment: when peering token generation source is static passes in -token-server-address flags with static addresses" {
-  cd `chart_dir`
-  local command=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'global.peering.tokenGeneration.serverAddresses.source=static' \
-      --set 'global.peering.tokenGeneration.serverAddresses.static[0]=1.2.3.4:1234' \
-      --set 'global.peering.tokenGeneration.serverAddresses.static[1]=2.2.3.4:2234' \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command')
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"1.2.3.4:1234\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"2.2.3.4:2234\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-}
-
-@test "connectInject/Deployment: when peering token generation source is static and externalHosts are set, passes in -token-server-address flags with static addresses, not externalServers.hosts" {
-  cd `chart_dir`
-  local command=$(helm template \
-      -s templates/connect-inject-deployment.yaml  \
-      --set 'server.enabled=false' \
-      --set 'global.peering.tokenGeneration.serverAddresses.source=static' \
-      --set 'global.peering.tokenGeneration.serverAddresses.static[0]=1.2.3.4:1234' \
-      --set 'global.peering.tokenGeneration.serverAddresses.static[1]=2.2.3.4:2234' \
-      --set 'externalServers.enabled=true' \
-      --set 'externalServers.hosts[0]=1.1.1.1' \
-      --set 'externalServers.hosts[1]=2.2.2.2' \
-      --set 'connectInject.enabled=true' \
-      --set 'global.peering.enabled=true' \
-      . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command')
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"1.2.3.4:1234\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-
-  local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"2.2.3.4:2234\""))' | tee /dev/stderr)
-  [ "${actual}" = "true" ]
-}
-
 #--------------------------------------------------------------------
 # openshift