Skip to content

Commit

Permalink
Rename flag for peering token server addresses. (#1426)
Browse files Browse the repository at this point in the history 
- The flag was initially named `server-address` but the `server-address` flag has been used across the project to imply ONLY the address of the external server when enabled. This meaning is overloaded in the context of the server address used for generating the peering token. This leads to errors (specifically in the agentless context), where the deployment must explicitly know the list of external servers.
  • Loading branch information
@thisisnotashwin @curtbushko
thisisnotashwin authored and curtbushko committed Aug 19, 2022
1 parent 4e7bc84 commit c0de455
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 19 deletions.
4 changes: 2 additions & 2 deletions charts/consul/templates/connect-inject-deployment.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,14 @@ spec:
{{- if .Values.externalServers.enabled }}
{{- $port := .Values.externalServers.grpcPort }}
{{- range $h := .Values.externalServers.hosts }}
-server-address="{{ $h }}:{{ $port }}" \
-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 }}
-server-address="{{ $addr }}" \
-token-server-address="{{ $addr }}" \
{{- end }}
{{- end }}
{{- end }}
Expand Down
26 changes: 13 additions & 13 deletions charts/consul/test/unit/connect-inject-deployment.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1937,7 +1937,7 @@ EOF
[[ "$output" =~ "global.peering.tokenGeneration.serverAddresses.source must be one of empty string, 'consul' or 'static'" ]]
}

@test "connectInject/Deployment: -read-server-expose-service and -server-address is not set when global.peering.tokenGeneration.serverAddresses.source is consul" {
@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 \
Expand All @@ -1950,11 +1950,11 @@ EOF
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("-server-address"))' | tee /dev/stderr)
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 -server-address flags with hosts" {
@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 \
Expand All @@ -1967,10 +1967,10 @@ EOF
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command')

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"1.2.3.4:8503\""))' | tee /dev/stderr)
local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"1.2.3.4:8503\""))' | tee /dev/stderr)
[ "${actual}" = "true" ]

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"2.2.3.4:8503\""))' | tee /dev/stderr)
local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"2.2.3.4:8503\""))' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

Expand All @@ -1988,14 +1988,14 @@ EOF
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command')

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"1.2.3.4:1234\""))' | tee /dev/stderr)
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("-server-address=\"2.2.3.4:1234\""))' | tee /dev/stderr)
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 -server-address flags with static addresses" {
@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 \
Expand All @@ -2007,14 +2007,14 @@ EOF
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command')

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"1.2.3.4:1234\""))' | tee /dev/stderr)
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("-server-address=\"2.2.3.4:2234\""))' | tee /dev/stderr)
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 -server-address flags with static addresses, not externalServers.hosts" {
@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 \
Expand All @@ -2030,10 +2030,10 @@ EOF
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].command')

local actual=$(echo $command | jq -r ' . | any(contains("-server-address=\"1.2.3.4:1234\""))' | tee /dev/stderr)
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("-server-address=\"2.2.3.4:2234\""))' | tee /dev/stderr)
local actual=$(echo $command | jq -r ' . | any(contains("-token-server-address=\"2.2.3.4:2234\""))' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

Expand Down
8 changes: 4 additions & 4 deletions control-plane/subcommand/inject-connect/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ type Command struct {

// Server address flags.
flagReadServerExposeService bool
flagServerAddresses []string
flagTokenServerAddresses []string

// Transparent proxy flags.
flagDefaultEnableTransparentProxy bool
Expand Down Expand Up @@ -200,8 +200,8 @@ func (c *Command) init() {
"Enable or disable JSON output format for logging.")
c.flagSet.BoolVar(&c.flagReadServerExposeService, "read-server-expose-service", false,
"Enables polling the Consul servers' external service for its IP(s).")
c.flagSet.Var((*flags.AppendSliceValue)(&c.flagServerAddresses), "server-address",
"An address of the Consul server(s), formatted host:port, where host may be an IP or DNS name and port must be a gRPC port. May be specified multiple times for multiple addresses.")
c.flagSet.Var((*flags.AppendSliceValue)(&c.flagTokenServerAddresses), "token-server-address",
"An address of the Consul server(s) as saved in the peering token, formatted host:port, where host may be an IP or DNS name and port must be a gRPC port. May be specified multiple times for multiple addresses.")

// Proxy sidecar resource setting flags.
c.flagSet.StringVar(&c.flagDefaultSidecarProxyCPURequest, "default-sidecar-proxy-cpu-request", "", "Default sidecar proxy CPU request.")
Expand Down Expand Up @@ -457,7 +457,7 @@ func (c *Command) Run(args []string) int {
ConsulClient: c.consulClient,
ExposeServersServiceName: c.flagResourcePrefix + "-expose-servers",
ReadServerExternalService: c.flagReadServerExposeService,
TokenServerAddresses: c.flagServerAddresses,
TokenServerAddresses: c.flagTokenServerAddresses,
ReleaseNamespace: c.flagReleaseNamespace,
Log: ctrl.Log.WithName("controller").WithName("peering-acceptor"),
Scheme: mgr.GetScheme(),
Expand Down

0 comments on commit c0de455

Please sign in to comment.