From a0820fbf3fbcfc19d1f0d3719e0aa5eec3927b50 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 15 Oct 2021 16:27:08 -0400 Subject: [PATCH] Feat: add externalTrafficPolicy support - externalTrafficPolicy can be set for both the ui and server services. It is only supported for NodePort or LoadBalancer service types. --- templates/_helpers.tpl | 35 +++++++++++++++++ templates/server-ha-active-service.yaml | 1 + templates/server-ha-standby-service.yaml | 3 +- templates/server-service.yaml | 1 + templates/ui-service.yaml | 12 +----- test/unit/server-ha-active-service.bats | 40 +++++++++++++++++++ test/unit/server-ha-standby-service.bats | 40 +++++++++++++++++++ test/unit/server-service.bats | 40 +++++++++++++++++++ test/unit/ui-service.bats | 50 ++++++++++++++++++++++++ values.yaml | 14 ++++++- 10 files changed, 224 insertions(+), 12 deletions(-) diff --git a/templates/_helpers.tpl b/templates/_helpers.tpl index 3e936f77e..731119a91 100644 --- a/templates/_helpers.tpl +++ b/templates/_helpers.tpl @@ -655,3 +655,38 @@ imagePullSecrets: {{- end -}} {{- end -}} {{- end -}} + +{{/* +externalTrafficPolicy sets a Service's externalTrafficPolicy if applicable. +Supported inputs are Values.server.service and Values.ui +*/}} +{{- define "service.externalTrafficPolicy" -}} +{{- $type := "" -}} +{{- if .serviceType -}} +{{- $type = .serviceType -}} +{{- else if .type -}} +{{- $type = .type -}} +{{- end -}} +{{- if and .externalTrafficPolicy (or (eq $type "LoadBalancer") (eq $type "NodePort")) }} + externalTrafficPolicy: {{ .externalTrafficPolicy }} +{{- else }} +{{- end }} +{{- end -}} + +{{/* +loadBalancer configuration for the the UI service. +Supported inputs are Values.ui +*/}} +{{- define "service.loadBalancer" -}} +{{- if eq (.serviceType | toString) "LoadBalancer" }} +{{- if .loadBalancerIP }} + loadBalancerIP: {{ .loadBalancerIP }} +{{- end }} +{{- with .loadBalancerSourceRanges }} + loadBalancerSourceRanges: +{{- range . }} + - {{ . }} +{{- end }} +{{- end -}} +{{- end }} +{{- end -}} diff --git a/templates/server-ha-active-service.yaml b/templates/server-ha-active-service.yaml index 74fca41d7..c2a4f0227 100644 --- a/templates/server-ha-active-service.yaml +++ b/templates/server-ha-active-service.yaml @@ -21,6 +21,7 @@ spec: {{- if .Values.server.service.clusterIP }} clusterIP: {{ .Values.server.service.clusterIP }} {{- end }} + {{- include "service.externalTrafficPolicy" .Values.server.service }} publishNotReadyAddresses: true ports: - name: {{ include "vault.scheme" . }} diff --git a/templates/server-ha-standby-service.yaml b/templates/server-ha-standby-service.yaml index 9213b7452..fef92a1b2 100644 --- a/templates/server-ha-standby-service.yaml +++ b/templates/server-ha-standby-service.yaml @@ -21,6 +21,7 @@ spec: {{- if .Values.server.service.clusterIP }} clusterIP: {{ .Values.server.service.clusterIP }} {{- end }} + {{- include "service.externalTrafficPolicy" .Values.server.service }} publishNotReadyAddresses: true ports: - name: {{ include "vault.scheme" . }} @@ -38,4 +39,4 @@ spec: component: server vault-active: "false" {{- end }} -{{- end }} \ No newline at end of file +{{- end }} diff --git a/templates/server-service.yaml b/templates/server-service.yaml index 6f82e3862..00996aa25 100644 --- a/templates/server-service.yaml +++ b/templates/server-service.yaml @@ -21,6 +21,7 @@ spec: {{- if .Values.server.service.clusterIP }} clusterIP: {{ .Values.server.service.clusterIP }} {{- end }} + {{- include "service.externalTrafficPolicy" .Values.server.service }} # We want the servers to become available even if they're not ready # since this DNS is also used for join operations. publishNotReadyAddresses: true diff --git a/templates/ui-service.yaml b/templates/ui-service.yaml index 9e90af4bb..ea27de282 100644 --- a/templates/ui-service.yaml +++ b/templates/ui-service.yaml @@ -30,16 +30,8 @@ spec: nodePort: {{ .Values.ui.serviceNodePort }} {{- end }} type: {{ .Values.ui.serviceType }} - {{- if and (eq (.Values.ui.serviceType | toString) "LoadBalancer") (.Values.ui.loadBalancerSourceRanges) }} - loadBalancerSourceRanges: - {{- range $cidr := .Values.ui.loadBalancerSourceRanges }} - - {{ $cidr }} - {{- end }} - {{- end }} - {{- if and (eq (.Values.ui.serviceType | toString) "LoadBalancer") (.Values.ui.loadBalancerIP) }} - loadBalancerIP: {{ .Values.ui.loadBalancerIP }} - {{- end }} + {{- include "service.externalTrafficPolicy" .Values.ui }} + {{- include "service.loadBalancer" .Values.ui }} {{- end -}} - {{- end }} {{- end }} diff --git a/test/unit/server-ha-active-service.bats b/test/unit/server-ha-active-service.bats index be3060d64..a835c9d9c 100755 --- a/test/unit/server-ha-active-service.bats +++ b/test/unit/server-ha-active-service.bats @@ -157,3 +157,43 @@ load _helpers yq -r '.spec.ports | map(select(.port==8200)) | .[] .name' | tee /dev/stderr) [ "${actual}" = "https" ] } + +# duplicated in server-service.bats +@test "server/ha-active-Service: NodePort assert externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-ha-active-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=NodePort' \ + --set 'server.service.externalTrafficPolicy=Foo' \ + . | tee /dev/stderr | + yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "Foo" ] +} + +# duplicated in server-service.bats +@test "server/ha-active-Service: NodePort assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-ha-active-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=NodePort' \ + --set 'server.service.externalTrafficPolicy=' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] +} + +# duplicated in server-service.bats +@test "server/ha-active-Service: ClusterIP assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-ha-active-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=ClusterIP' \ + --set 'server.service.externalTrafficPolicy=Foo' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] +} + diff --git a/test/unit/server-ha-standby-service.bats b/test/unit/server-ha-standby-service.bats index e164cde1c..7dfd5d7fd 100755 --- a/test/unit/server-ha-standby-service.bats +++ b/test/unit/server-ha-standby-service.bats @@ -168,3 +168,43 @@ load _helpers yq -r '.spec.ports | map(select(.port==8200)) | .[] .name' | tee /dev/stderr) [ "${actual}" = "https" ] } + +# duplicated in server-service.bats +@test "server/ha-standby-Service: NodePort assert externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-ha-standby-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=NodePort' \ + --set 'server.service.externalTrafficPolicy=Foo' \ + . | tee /dev/stderr | + yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "Foo" ] +} + +# duplicated in server-service.bats +@test "server/ha-standby-Service: NodePort assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-ha-standby-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=NodePort' \ + --set 'server.service.externalTrafficPolicy=' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] +} + +# duplicated in server-service.bats +@test "server/ha-standby-Service: ClusterIP assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-ha-standby-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=ClusterIP' \ + --set 'server.service.externalTrafficPolicy=Foo' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] +} + diff --git a/test/unit/server-service.bats b/test/unit/server-service.bats index 7922f0ff3..4695f2fff 100755 --- a/test/unit/server-service.bats +++ b/test/unit/server-service.bats @@ -384,3 +384,43 @@ load _helpers yq -r '.spec.ports | map(select(.port==8200)) | .[] .name' | tee /dev/stderr) [ "${actual}" = "https" ] } + +# duplicated in server-ha-active-service.bats +@test "server/Service: NodePort assert externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=NodePort' \ + --set 'server.service.externalTrafficPolicy=Foo' \ + . | tee /dev/stderr | + yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "Foo" ] +} + +# duplicated in server-ha-active-service.bats +@test "server/ha-active-Service: NodePort assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=NodePort' \ + --set 'server.service.externalTrafficPolicy=' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] +} + +# duplicated in server-ha-active-service.bats +@test "server/Service: ClusterIP assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/server-service.yaml \ + --set 'server.ha.enabled=true' \ + --set 'server.service.type=ClusterIP' \ + --set 'server.service.externalTrafficPolicy=Foo' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] +} + diff --git a/test/unit/ui-service.bats b/test/unit/ui-service.bats index 9dade3db3..0603303cd 100755 --- a/test/unit/ui-service.bats +++ b/test/unit/ui-service.bats @@ -135,6 +135,16 @@ load _helpers . | tee /dev/stderr | yq -r '.spec.type' | tee /dev/stderr) [ "${actual}" = "LoadBalancer" ] + + local actual=$(helm template \ + --show-only templates/ui-service.yaml \ + --set 'server.standalone.enabled=true' \ + --set 'ui.serviceType=LoadBalancer' \ + --set 'ui.externalTrafficPolicy=Local' \ + --set 'ui.enabled=true' \ + . | tee /dev/stderr | + yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "Local" ] } @test "ui/Service: LoadBalancerIP set if specified and serviceType == LoadBalancer" { @@ -183,6 +193,19 @@ load _helpers [ "${actual}" = "null" ] } +@test "ui/Service: ClusterIP assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/ui-service.yaml \ + --set 'server.standalone.enabled=true' \ + --set 'ui.serviceType=ClusterIP' \ + --set 'ui.externalTrafficPolicy=Foo' \ + --set 'ui.enabled=true' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] +} + @test "ui/Service: specify annotations" { cd `chart_dir` local actual=$(helm template \ @@ -323,3 +346,30 @@ load _helpers yq -r '.spec.ports[0].nodePort' | tee /dev/stderr) [ "${actual}" = "123" ] } + +@test "ui/Service: LoadBalancer assert externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/ui-service.yaml \ + --set 'ui.enabled=true' \ + --set 'server.standalone.enabled=true' \ + --set 'ui.serviceType=LoadBalancer' \ + --set 'ui.externalTrafficPolicy=Foo' \ + . | tee /dev/stderr | + yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "Foo" ] +} + +@test "ui/Service: LoadBalancer assert no externalTrafficPolicy" { + cd `chart_dir` + local actual=$(helm template \ + --show-only templates/ui-service.yaml \ + --set 'ui.enabled=true' \ + --set 'server.standalone.enabled=true' \ + --set 'ui.serviceType=LoadBalancer' \ + --set 'ui.externalTrafficPolicy=' \ + . | tee /dev/stderr | + yq '.spec.externalTrafficPolicy' | tee /dev/stderr) + [ "${actual}" = "null" ] + +} diff --git a/values.yaml b/values.yaml index 32c59c438..48b413acf 100644 --- a/values.yaml +++ b/values.yaml @@ -483,6 +483,12 @@ server: # or NodePort. #type: ClusterIP + # The externalTrafficPolicy can be set to either Cluster or Local + # and is only valid for LoadBalancer and NodePort service types. + # The default value is Cluster. + # ref: https://kubernetes.io/docs/concepts/services-networking/service/#external-traffic-policy + externalTrafficPolicy: Cluster + # If type is set to "NodePort", a specific nodePort value can be configured, # will be random if left blank. #nodePort: 30000 @@ -704,7 +710,13 @@ ui: externalPort: 8200 targetPort: 8200 - # loadBalancerSourceRanges: + # The externalTrafficPolicy can be set to either Cluster or Local + # and is only valid for LoadBalancer and NodePort service types. + # The default value is Cluster. + # ref: https://kubernetes.io/docs/concepts/services-networking/service/#external-traffic-policy + externalTrafficPolicy: Cluster + + #loadBalancerSourceRanges: # - 10.0.0.0/16 # - 1.78.23.3/32