address review comments

hashicorp · Jul 28, 2022 · 3a42657 · 3a42657
1 parent 3e46335
commit 3a42657
Show file tree

Hide file tree

Showing 7 changed files with 76 additions and 43 deletions.
diff --git a/acceptance/tests/peering/peering_connect_namespaces_test.go b/acceptance/tests/peering/peering_connect_namespaces_test.go
@@ -55,18 +55,18 @@ func TestPeering_ConnectNamespaces(t *testing.T) {
 			false,
 			false,
 		},
-		//{
-		//	"single destination namespace",
-		//	staticServerNamespace,
-		//	false,
-		//	false,
-		//},
-		//{
-		//	"mirror k8s namespaces",
-		//	staticServerNamespace,
-		//	true,
-		//	false,
-		//},
+		{
+			"single destination namespace",
+			staticServerNamespace,
+			false,
+			false,
+		},
+		{
+			"mirror k8s namespaces",
+			staticServerNamespace,
+			true,
+			false,
+		},
 		{
 			"default destination namespace",
 			defaultNamespace,
@@ -115,16 +115,19 @@ func TestPeering_ConnectNamespaces(t *testing.T) {
 
 				"controller.enabled": "true",
 
-				"dns.enabled":            "true",
-				"dns.enableRedirection":  strconv.FormatBool(cfg.EnableTransparentProxy),
-				"server.replicas":        "3",
-				"server.bootstrapExpect": "3",
+				"dns.enabled":           "true",
+				"dns.enableRedirection": strconv.FormatBool(cfg.EnableTransparentProxy),
 			}
 
 			staticServerPeerHelmValues := map[string]string{
 				"global.datacenter": staticServerPeer,
 			}
 
+			if !cfg.UseKind {
+				staticServerPeerHelmValues["server.replicas"] = "3"
+				staticServerPeerHelmValues["server.bootstrapExpect"] = "3"
+			}
+
 			// On Kind, there are no load balancers but since all clusters
 			// share the same node network (docker bridge), we can use
 			// a NodePort service so that we can access node(s) in a different Kind cluster.
@@ -140,16 +143,21 @@ func TestPeering_ConnectNamespaces(t *testing.T) {
 
 			releaseName := helpers.RandomName()
 
-			helpers.MergeMaps(commonHelmValues, staticServerPeerHelmValues)
+			helpers.MergeMaps(staticServerPeerHelmValues, commonHelmValues)
 
 			// Install the first peer where static-server will be deployed in the static-server kubernetes context.
-			staticServerPeerCluster := consul.NewHelmCluster(t, commonHelmValues, staticServerPeerClusterContext, cfg, releaseName)
+			staticServerPeerCluster := consul.NewHelmCluster(t, staticServerPeerHelmValues, staticServerPeerClusterContext, cfg, releaseName)
 			staticServerPeerCluster.Create(t)
 
 			staticClientPeerHelmValues := map[string]string{
 				"global.datacenter": staticClientPeer,
 			}
 
+			if !cfg.UseKind {
+				staticClientPeerHelmValues["server.replicas"] = "3"
+				staticClientPeerHelmValues["server.bootstrapExpect"] = "3"
+			}
+
 			if cfg.UseKind {
 				staticClientPeerHelmValues["server.exposeGossipAndRPCPorts"] = "true"
 				staticClientPeerHelmValues["meshGateway.service.type"] = "NodePort"
@@ -160,10 +168,10 @@ func TestPeering_ConnectNamespaces(t *testing.T) {
 				staticServerPeerHelmValues["server.bootstrapExpect"] = "1"
 			}
 
-			helpers.MergeMaps(commonHelmValues, staticClientPeerHelmValues)
+			helpers.MergeMaps(staticClientPeerHelmValues, commonHelmValues)
 
 			// Install the second peer where static-client will be deployed in the static-client kubernetes context.
-			staticClientPeerCluster := consul.NewHelmCluster(t, commonHelmValues, staticClientPeerClusterContext, cfg, releaseName)
+			staticClientPeerCluster := consul.NewHelmCluster(t, staticClientPeerHelmValues, staticClientPeerClusterContext, cfg, releaseName)
 			staticClientPeerCluster.Create(t)
 
 			// Create the peering acceptor on the client peer.

diff --git a/acceptance/tests/peering/peering_connect_test.go b/acceptance/tests/peering/peering_connect_test.go
@@ -70,16 +70,19 @@ func TestPeering_Connect(t *testing.T) {
 
 				"controller.enabled": "true",
 
-				"dns.enabled":            "true",
-				"dns.enableRedirection":  strconv.FormatBool(cfg.EnableTransparentProxy),
-				"server.replicas":        "3",
-				"server.bootstrapExpect": "3",
+				"dns.enabled":           "true",
+				"dns.enableRedirection": strconv.FormatBool(cfg.EnableTransparentProxy),
 			}
 
 			staticServerPeerHelmValues := map[string]string{
 				"global.datacenter": staticServerPeer,
 			}
 
+			if !cfg.UseKind {
+				staticServerPeerHelmValues["server.replicas"] = "3"
+				staticServerPeerHelmValues["server.bootstrapExpect"] = "3"
+			}
+
 			// On Kind, there are no load balancers but since all clusters
 			// share the same node network (docker bridge), we can use
 			// a NodePort service so that we can access node(s) in a different Kind cluster.
@@ -95,16 +98,21 @@ func TestPeering_Connect(t *testing.T) {
 
 			releaseName := helpers.RandomName()
 
-			helpers.MergeMaps(commonHelmValues, staticServerPeerHelmValues)
+			helpers.MergeMaps(staticServerPeerHelmValues, commonHelmValues)
 
 			// Install the first peer where static-server will be deployed in the static-server kubernetes context.
-			staticServerPeerCluster := consul.NewHelmCluster(t, commonHelmValues, staticServerPeerClusterContext, cfg, releaseName)
+			staticServerPeerCluster := consul.NewHelmCluster(t, staticServerPeerHelmValues, staticServerPeerClusterContext, cfg, releaseName)
 			staticServerPeerCluster.Create(t)
 
 			staticClientPeerHelmValues := map[string]string{
 				"global.datacenter": staticClientPeer,
 			}
 
+			if !cfg.UseKind {
+				staticServerPeerHelmValues["server.replicas"] = "3"
+				staticServerPeerHelmValues["server.bootstrapExpect"] = "3"
+			}
+
 			if cfg.UseKind {
 				staticClientPeerHelmValues["server.exposeGossipAndRPCPorts"] = "true"
 				staticClientPeerHelmValues["meshGateway.service.type"] = "NodePort"
@@ -115,10 +123,10 @@ func TestPeering_Connect(t *testing.T) {
 				staticClientPeerHelmValues["server.bootstrapExpect"] = "1"
 			}
 
-			helpers.MergeMaps(commonHelmValues, staticClientPeerHelmValues)
+			helpers.MergeMaps(staticClientPeerHelmValues, commonHelmValues)
 
 			// Install the second peer where static-client will be deployed in the static-client kubernetes context.
-			staticClientPeerCluster := consul.NewHelmCluster(t, commonHelmValues, staticClientPeerClusterContext, cfg, releaseName)
+			staticClientPeerCluster := consul.NewHelmCluster(t, staticClientPeerHelmValues, staticClientPeerClusterContext, cfg, releaseName)
 			staticClientPeerCluster.Create(t)
 
 			// Create the peering acceptor on the client peer.

diff --git a/charts/consul/templates/connect-inject-deployment.yaml b/charts/consul/templates/connect-inject-deployment.yaml
@@ -142,7 +142,7 @@ spec:
                 -enable-peering=true \
                 {{- if (eq .Values.global.peering.tokenGeneration.serverAddresses.source "") }}
                 {{- if (and $serverEnabled $serverExposeServiceEnabled) }}
-                -poll-server-expose-service=true \
+                -read-server-expose-service=true \
                 {{- end }}
                 {{- end }}
                 {{- end }}

diff --git a/charts/consul/test/unit/connect-inject-deployment.bats b/charts/consul/test/unit/connect-inject-deployment.bats
@@ -1831,19 +1831,19 @@ EOF
   [[ "$output" =~ "setting global.peering.enabled to true requires connectInject.enabled to be true" ]]
 }
 
-@test "connectInject/Deployment: -poll-server-expose-service=true is set when global.peering.enabled is 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("-poll-server-expose-service=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: -poll-server-expose-service=true is set when servers are enabled and peering is enabled" {
+@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  \
@@ -1853,32 +1853,45 @@ EOF
       --set 'connectInject.enabled=true' \
       --set 'global.peering.enabled=true' \
       . | tee /dev/stderr |
-      yq '.spec.template.spec.containers[0].command | any(contains("-poll-server-expose-service=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: -poll-server-expose-service is not set when servers are disabled" {
+@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("-poll-server-expose-service=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: -poll-server-expose-service is not set when peering is disabled" {
+@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("-poll-server-expose-service=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 global.peering.tokenGeneration.serverAddresses.source is not equal to empty string" {
+  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="notempty"' \
+      . | tee /dev/stderr |
+      yq '.spec.template.spec.containers[0].command | any(contains("-read-server-expose-service=true"))' | tee /dev/stderr)
 
   [ "${actual}" = "false" ]
 }

diff --git a/charts/consul/values.yaml b/charts/consul/values.yaml
@@ -37,7 +37,11 @@ global:
     tokenGeneration:
       serverAddresses:
         # Source can be set to "" or "consul".
-        # "" is the default behavior.
+        #
+        # "" is the default source. If servers are enabled, it will check if server.exposeService is enabled, and read
+        # the addresses from that service to use as the peering token server addresses.
+        #
+        # "consul" will use the Consul advertise addresses in the peering token.
         source: ""
 
   # [Enterprise Only] Enabling `adminPartitions` allows creation of Admin Partitions in Kubernetes clusters.

diff --git a/control-plane/connect-inject/peering_acceptor_controller.go b/control-plane/connect-inject/peering_acceptor_controller.go
@@ -31,7 +31,7 @@ type PeeringAcceptorController struct {
 	// ConsulClient points at the agent local to the connect-inject deployment pod.
 	ConsulClient              *api.Client
 	ExposeServersServiceName  string
-	PollServerExternalService bool
+	ReadServerExternalService bool
 	ReleaseNamespace          string
 	Log                       logr.Logger
 	Scheme                    *runtime.Scheme
@@ -104,7 +104,7 @@ func (r *PeeringAcceptorController) Reconcile(ctx context.Context, req ctrl.Requ
 
 	// Scrape the address of the server service
 	var serverExternalAddresses []string
-	if r.PollServerExternalService {
+	if r.ReadServerExternalService {
 		addrs, err := r.getExposeServersServiceAddresses()
 		if err != nil {
 			r.updateStatusError(ctx, acceptor, KubernetesError, err)

diff --git a/control-plane/subcommand/inject-connect/command.go b/control-plane/subcommand/inject-connect/command.go
@@ -95,7 +95,7 @@ type Command struct {
 	flagInitContainerMemoryRequest string
 
 	// Server address flags.
-	flagPollServerExposeService bool
+	flagReadServerExposeService bool
 
 	// Transparent proxy flags.
 	flagDefaultEnableTransparentProxy          bool
@@ -192,7 +192,7 @@ func (c *Command) init() {
 			"%q, %q, %q, and %q.", zapcore.DebugLevel.String(), zapcore.InfoLevel.String(), zapcore.WarnLevel.String(), zapcore.ErrorLevel.String()))
 	c.flagSet.BoolVar(&c.flagLogJSON, "log-json", false,
 		"Enable or disable JSON output format for logging.")
-	c.flagSet.BoolVar(&c.flagPollServerExposeService, "poll-server-expose-service", false,
+	c.flagSet.BoolVar(&c.flagReadServerExposeService, "read-server-expose-service", false,
 		"Enables polling the Consul servers' external service for its IP(s).")
 
 	// Proxy sidecar resource setting flags.
@@ -448,7 +448,7 @@ func (c *Command) Run(args []string) int {
 			Client:                    mgr.GetClient(),
 			ConsulClient:              c.consulClient,
 			ExposeServersServiceName:  c.flagResourcePrefix + "-expose-servers",
-			PollServerExternalService: c.flagPollServerExposeService,
+			ReadServerExternalService: c.flagReadServerExposeService,
 			ReleaseNamespace:          c.flagReleaseNamespace,
 			Log:                       ctrl.Log.WithName("controller").WithName("peering-acceptor"),
 			Scheme:                    mgr.GetScheme(),