From 3a4265758de2b506ceaf0b3db8333bbe76184426 Mon Sep 17 00:00:00 2001 From: Nitya Dhanushkodi Date: Wed, 27 Jul 2022 17:51:34 -0700 Subject: [PATCH] address review comments --- .../peering_connect_namespaces_test.go | 48 +++++++++++-------- .../tests/peering/peering_connect_test.go | 24 ++++++---- .../templates/connect-inject-deployment.yaml | 2 +- .../test/unit/connect-inject-deployment.bats | 29 +++++++---- charts/consul/values.yaml | 6 ++- .../peering_acceptor_controller.go | 4 +- .../subcommand/inject-connect/command.go | 6 +-- 7 files changed, 76 insertions(+), 43 deletions(-) diff --git a/acceptance/tests/peering/peering_connect_namespaces_test.go b/acceptance/tests/peering/peering_connect_namespaces_test.go index 44c5aa28a0..9989fd5016 100644 --- 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 index 075ced127a..eb6df7aac2 100644 --- 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 index 0e0e59bd86..6cf1f27a90 100644 --- 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 index d3beb7e051..2eded05672 100755 --- 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,12 +1853,12 @@ 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 \ @@ -1866,19 +1866,32 @@ 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}" = "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 index 209ea60a16..e800192d3a 100644 --- 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 index d1ff388468..e35d82be5c 100644 --- 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 index bc69df1be1..de35d78d0b 100644 --- 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(),