Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add externalServers.skipServerWatch to helm for working with load balancers #1686

Merged
merged 4 commits into from
Nov 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions charts/consul/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,10 @@ Consul server environment variables for consul-k8s commands.
value: {{ .Values.externalServers.tlsServerName }}
{{- end }}
{{- end }}
{{- if and .Values.externalServers.enabled .Values.externalServers.skipServerWatch }}
- name: CONSUL_SKIP_SERVER_WATCH
value: "true"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a nit but can we unqoute the "true"

{{- end }}
{{- end -}}

{{/*
Expand Down
1 change: 1 addition & 0 deletions charts/consul/templates/connect-inject-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
{{- $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) "-") .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 -}}
{{- if and .Values.externalServers.skipServerWatch (not .Values.externalServers.enabled) }}{{ fail "externalServers.enabled must be set if externalServers.skipServerWatch is true" }}{{ end -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting the fail condition here so that it is obvious that it pairs with the checks in connect-inject-deployment.bats.

A similar thing is done for externalServers.hosts and _helpers.tpl. The checks are here in connect-inject-deployment.yaml.

{{ template "consul.validateRequiredCloudSecretsExist" . }}
{{ template "consul.validateCloudSecretKeys" . }}
# The deployment for running the Connect sidecar injector
Expand Down
3 changes: 3 additions & 0 deletions charts/consul/templates/ingress-gateways-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,9 @@ spec:
{{- if (and $root.Values.global.metrics.enabled $root.Values.global.metrics.enableGatewayMetrics) }}
-telemetry-prom-scrape-path="/metrics"
{{- end }}
{{- if and $root.Values.externalServers.enabled $root.Values.externalServers.skipServerWatch }}
-server-watch-disabled=true
{{- end }}
livenessProbe:
tcpSocket:
port: 21000
Expand Down
3 changes: 3 additions & 0 deletions charts/consul/templates/mesh-gateway-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ spec:
{{- if (and .Values.global.metrics.enabled .Values.global.metrics.enableGatewayMetrics) }}
-telemetry-prom-scrape-path="/metrics"
{{- end }}
{{- if and .Values.externalServers.enabled .Values.externalServers.skipServerWatch }}
-server-watch-disabled=true
{{- end }}
livenessProbe:
tcpSocket:
port: {{ .Values.meshGateway.containerPort }}
Expand Down
3 changes: 3 additions & 0 deletions charts/consul/templates/terminating-gateways-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,9 @@ spec:
{{- if (and $root.Values.global.metrics.enabled $root.Values.global.metrics.enableGatewayMetrics) }}
-telemetry-prom-scrape-path="/metrics"
{{- end }}
{{- if and $root.Values.externalServers.enabled $root.Values.externalServers.skipServerWatch }}
-server-watch-disabled=true
{{- end }}
livenessProbe:
tcpSocket:
port: 8443
Expand Down
29 changes: 29 additions & 0 deletions charts/consul/test/unit/connect-inject-deployment.bats
Original file line number Diff line number Diff line change
Expand Up @@ -2033,6 +2033,35 @@ reservedNameTest() {
[ "${actual}" = "" ]
}

@test "connectInject/Deployment: fails if externalServers.skipServerWatch is not provided when externalServers.enabled is true" {
cd `chart_dir`
run helm template \
-s templates/connect-inject-deployment.yaml \
--set 'connectInject.enabled=true' \
--set 'server.enabled=false' \
--set 'externalServers.skipServerWatch=true' \
.
[ "$status" -eq 1 ]
[[ "$output" =~ "externalServers.enabled must be set if externalServers.skipServerWatch is true" ]]
}

@test "connectInject/Deployment: configures the sidecar-injector env to skip server watch" {
cd `chart_dir`
local env=$(helm template \
-s templates/connect-inject-deployment.yaml \
--set 'connectInject.enabled=true' \
--set 'server.enabled=false' \
--set 'externalServers.enabled=true' \
--set 'externalServers.hosts[0]=consul' \
--set 'externalServers.skipServerWatch=true' \
. | tee /dev/stderr |
yq '.spec.template.spec.containers[0].env[]' | tee /dev/stderr)\

local actual=$(echo "$env" |
jq -r '. | select( .name == "CONSUL_SKIP_SERVER_WATCH").value' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

#--------------------------------------------------------------------
# global.cloud

Expand Down
21 changes: 21 additions & 0 deletions charts/consul/test/unit/ingress-gateways-deployment.bats
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,27 @@ load _helpers
[ "${actual}" = "null" ]
}

#--------------------------------------------------------------------
# externalServers.skipServerWatch

@test "ingressGateways/Deployment: sets server-watch-disabled flag when externalServers.enabled and externalServers.skipServerWatch is true" {
cd `chart_dir`
local object=$(helm template \
-s templates/ingress-gateways-deployment.yaml \
--set 'ingressGateways.enabled=true' \
--set 'connectInject.enabled=true' \
--set 'global.tls.enabled=false' \
--set 'server.enabled=false' \
--set 'externalServers.enabled=true' \
--set 'externalServers.hosts[0]=consul' \
--set 'externalServers.skipServerWatch=true' \
. | tee /dev/stderr |
yq -s -r '.[0].spec.template.spec.containers[0].command[2]' | tee /dev/stderr)

local actual=$(echo $object | yq -r '. | contains("-server-watch-disabled")' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

#--------------------------------------------------------------------
# replicas

Expand Down
21 changes: 21 additions & 0 deletions charts/consul/test/unit/mesh-gateway-deployment.bats
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,27 @@ key2: value2' \
[ "${actual}" = "null" ]
}

#--------------------------------------------------------------------
# externalServers.skipServerWatch

@test "meshGateway/Deployment: sets server-watch-disabled flag when externalServers.enabled and externalServers.skipServerWatch is true" {
cd `chart_dir`
local object=$(helm template \
-s templates/ingress-gateways-deployment.yaml \
--set 'ingressGateways.enabled=true' \
--set 'connectInject.enabled=true' \
--set 'global.tls.enabled=false' \
--set 'server.enabled=false' \
--set 'externalServers.enabled=true' \
--set 'externalServers.hosts[0]=consul' \
--set 'externalServers.skipServerWatch=true' \
. | tee /dev/stderr |
yq -s -r '.[0].spec.template.spec.containers[0].command[2]' | tee /dev/stderr)

local actual=$(echo $object | yq -r '. | contains("-server-watch-disabled")' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

#--------------------------------------------------------------------
# replicas

Expand Down
21 changes: 21 additions & 0 deletions charts/consul/test/unit/terminating-gateways-deployment.bats
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,27 @@ load _helpers
[ "${actual}" = "null" ]
}

#--------------------------------------------------------------------
# externalServers.skipServerWatch

@test "terminatingGateways/Deployment: sets server-watch-disabled flag when externalServers.enabled and externalServers.skipServerWatch is true" {
cd `chart_dir`
local object=$(helm template \
-s templates/ingress-gateways-deployment.yaml \
--set 'ingressGateways.enabled=true' \
--set 'connectInject.enabled=true' \
--set 'global.tls.enabled=false' \
--set 'server.enabled=false' \
--set 'externalServers.enabled=true' \
--set 'externalServers.hosts[0]=consul' \
--set 'externalServers.skipServerWatch=true' \
. | tee /dev/stderr |
yq -s -r '.[0].spec.template.spec.containers[0].command[2]' | tee /dev/stderr)

local actual=$(echo $object | yq -r '. | contains("-server-watch-disabled")' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

#--------------------------------------------------------------------
# replicas

Expand Down
4 changes: 4 additions & 0 deletions charts/consul/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,10 @@ externalServers:
# @type: string
k8sAuthMethodHost: null

# If true, setting this prevents the consul-dataplane and consul-k8s components from watching the Consul servers for changes. This is
# useful for situations where Consul servers are behind a load balancer.
skipServerWatch: false

# Values that configure running a Consul client on Kubernetes nodes.
client:
# If true, the chart will install all
Expand Down
4 changes: 4 additions & 0 deletions control-plane/connect-inject/consul_dataplane_sidecar.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ func (w *MeshWebhook) getContainerSidecarCommand(namespace corev1.Namespace, mpi
"-envoy-concurrency=" + strconv.Itoa(envoyConcurrency),
}

if w.SkipServerWatch {
cmd = append(cmd, "-server-watch-disabled=true")
}

if w.AuthMethod != "" {
cmd = append(cmd,
"-credential-type=login",
Expand Down
6 changes: 6 additions & 0 deletions control-plane/connect-inject/consul_dataplane_sidecar_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,12 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
},
additionalExpCmdArgs: " -tls-disabled",
},
"skip server watch enabled": {
webhookSetupFunc: func(w *MeshWebhook) {
w.SkipServerWatch = true
},
additionalExpCmdArgs: " -server-watch-disabled=true -tls-disabled",
},
}

for name, c := range cases {
Expand Down
4 changes: 4 additions & 0 deletions control-plane/connect-inject/mesh_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ type MeshWebhook struct {
// those containers to be created otherwise.
EnableOpenShift bool

// SkipServerWatch prevents consul-dataplane from consuming the server update stream. This is useful
// for situations where Consul servers are behind a load balancer.
SkipServerWatch bool

// ReleaseNamespace is the Kubernetes namespace where this webhook is running.
ReleaseNamespace string

Expand Down
11 changes: 11 additions & 0 deletions control-plane/subcommand/flags/consul.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ const (
LoginNamespaceEnvVar = "CONSUL_LOGIN_NAMESPACE"
LoginMetaEnvVar = "CONSUL_LOGIN_META"

SkipServerWatchEnvVar = "CONSUL_SKIP_SERVER_WATCH"

APITimeoutEnvVar = "CONSUL_API_TIMEOUT"
)

Expand All @@ -52,6 +54,8 @@ type ConsulFlags struct {
Partition string
Datacenter string

SkipServerWatch bool

ConsulTLSFlags
ConsulACLFlags
}
Expand Down Expand Up @@ -87,6 +91,7 @@ func (f *ConsulFlags) Flags() *flag.FlagSet {
grpcPort, _ := strconv.Atoi(os.Getenv(GRPCPortEnvVar))
httpPort, _ := strconv.Atoi(os.Getenv(HTTPPortEnvVar))
useTLS, _ := strconv.ParseBool(os.Getenv(UseTLSEnvVar))
skipServerWatch, _ := strconv.ParseBool(os.Getenv(SkipServerWatchEnvVar))
consulLoginMetaFromEnv := os.Getenv(LoginMetaEnvVar)
if consulLoginMetaFromEnv != "" {
// Parse meta from env var.
Expand Down Expand Up @@ -168,6 +173,8 @@ func (f *ConsulFlags) Flags() *flag.FlagSet {
"may be specified multiple times to set multiple meta fields.")
fs.DurationVar(&f.APITimeout, "api-timeout", defaultAPITimeout,
"The time in seconds that the consul API client will wait for a response from the API before cancelling the request.")
fs.BoolVar(&f.SkipServerWatch, "skip-server-watch", skipServerWatch, "If true, skip watching server upstream."+
"This can also be specified via the CONSUL_SKIP_SERVER_WATCH environment variable.")
return fs
}

Expand Down Expand Up @@ -223,6 +230,10 @@ func (f *ConsulFlags) ConsulServerConnMgrConfig() (discovery.Config, error) {
cfg.Credentials.Static.Token = string(token)
}

if f.SkipServerWatch {
cfg.ServerWatchDisabled = true
}

return cfg, nil
}

Expand Down
14 changes: 14 additions & 0 deletions control-plane/subcommand/flags/consul_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func TestConsulFlags_Flags(t *testing.T) {
LoginPartitionEnvVar: "other-test-partition",
LoginNamespaceEnvVar: "other-test-ns",
LoginMetaEnvVar: "key1=value1,key2=value2",
SkipServerWatchEnvVar: "true",
},
expFlags: &ConsulFlags{
Addresses: "consul.address",
Expand Down Expand Up @@ -66,6 +67,7 @@ func TestConsulFlags_Flags(t *testing.T) {
Meta: map[string]string{"key1": "value1", "key2": "value2"},
},
},
SkipServerWatch: true,
},
},
"defaults": {
Expand Down Expand Up @@ -211,6 +213,18 @@ func TestConsulFlags_ConsulServerConnMgrConfig(t *testing.T) {
},
},
},
"skip server watch to server watch disabled": {
flags: ConsulFlags{
Addresses: "consul.address",
GRPCPort: 8502,
SkipServerWatch: true,
},
expConfig: discovery.Config{
Addresses: "consul.address",
GRPCPort: 8502,
ServerWatchDisabled: true,
},
},
}

for name, c := range cases {
Expand Down
1 change: 1 addition & 0 deletions control-plane/subcommand/inject-connect/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,7 @@ func (c *Command) Run(args []string) int {
ConsulCACert: string(caCertPem),
TLSEnabled: c.consul.UseTLS,
ConsulAddress: c.consul.Addresses,
SkipServerWatch: c.consul.SkipServerWatch,
ConsulTLSServerName: c.consul.TLSServerName,
DefaultProxyCPURequest: sidecarProxyCPURequest,
DefaultProxyCPULimit: sidecarProxyCPULimit,
Expand Down