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.

Sign up for GitHub

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 externalTrafficPolicy support for services (excluding the UI service) #464

Closed
wants to merge 17 commits into from
Closed

Add externalTrafficPolicy support for services (excluding the UI service) #464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
46769ec
Add externalTrafficPolicy support for services
shayfisher Mar 1, 2021
d6bcefc
Fix the field data extraction
shayfisher Mar 2, 2021
fd79787
Update server-service.bats
shayfisher Mar 2, 2021
2a88f15
Update server-ha-active-service.bats
shayfisher Mar 3, 2021
8cd8a6b
Update server-ha-standby-service.bats
shayfisher Mar 3, 2021
b066e2d
Update server-service.bats
shayfisher Mar 3, 2021
de28081
Update server-ha-active-service.yaml
shayfisher Mar 3, 2021
7ce2c84
Update server-ha-standby-service.yaml
shayfisher Mar 3, 2021
67e4e7c
Update server-ha-standby-service.yaml
shayfisher Mar 3, 2021
44fdd05
Update server-ha-active-service.yaml
shayfisher Mar 3, 2021
3b05686
Update server-service.yaml
shayfisher Mar 3, 2021
a5cd8fc
Update server-ha-standby-service.bats
shayfisher Mar 24, 2021
799a6f1
Set externalTrafficPolicy=Local only for NodePort
shayfisher Mar 24, 2021
7ce27f3
change tests for the case where
shayfisher Mar 25, 2021
aaf500a
update according to suggestions by Ben
shayfisher Sep 29, 2021
31ff8b1
use the title function on externalTrafficPolicy
shayfisher Sep 29, 2021
d9cd4fc
first change case to lower
shayfisher Sep 29, 2021
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
3 changes: 3 additions & 0 deletions templates/server-ha-active-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ metadata:
annotations:
{{ template "vault.service.annotations" .}}
spec:
{{- if .Values.server.service.externalTrafficPolicy }}
externalTrafficPolicy: {{ .Values.server.service.externalTrafficPolicy | lower | title }}
benashz marked this conversation as resolved.
Show resolved Hide resolved
{{- end}}
{{- if .Values.server.service.type}}
type: {{ .Values.server.service.type }}
{{- end}}
Expand Down
5 changes: 5 additions & 0 deletions templates/server-ha-standby-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ metadata:
annotations:
{{ template "vault.service.annotations" .}}
spec:
{{- if and (.Values.server.service.externalTrafficPolicy ) (print .Values.server.service.externalTrafficPolicy | lower | title | eq "Local" )}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be a missing update here.

externalTrafficPolicy: {{ .Values.server.service.externalTrafficPolicy| lower | title | quote }}
{{- else }}
externalTrafficPolicy: {{ default "Cluster" | quote }}
{{- end}}
{{- if .Values.server.service.type}}
type: {{ .Values.server.service.type }}
{{- end}}
Expand Down
3 changes: 3 additions & 0 deletions templates/server-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ metadata:
annotations:
{{ template "vault.service.annotations" .}}
spec:
{{- if .Values.server.service.externalTrafficPolicy }}
externalTrafficPolicy: {{ .Values.server.service.externalTrafficPolicy | lower | title }}
{{- end}}
{{- if .Values.server.service.type}}
type: {{ .Values.server.service.type }}
{{- end}}
Expand Down
50 changes: 50 additions & 0 deletions test/unit/server-ha-active-service.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,53 @@ load _helpers
yq -r '.spec.ports | map(select(.port==8200)) | .[] .name' | tee /dev/stderr)
[ "${actual}" = "https" ]
}

@test "server/ha-active-Service: vault externalTrafficPolicy set to Local lowercase" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think just testing that the value provided is honoured is good enough here. The two cases are:

  • empty string ""
  • non empty value like Cluster

Copy link
Author

Choose a reason for hiding this comment

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

@benashz So you suggest adding a default value of "Cluster" for instance ?
I'm not handling the case where an empty string isn't provided.
This would probably fall back to K8S defaults (Which currently I'm not sure what is).

Copy link
Author

Choose a reason for hiding this comment

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

@benashz Just checked - made a new svc with a value not in the correct case - got the following error:
The Service "my-service" is invalid: spec.externalTrafficPolicy: Invalid value: "local": ExternalTrafficPolicy must be empty, Cluster or Local.

So - empty is just fine.
Non empty value would fail if it isn't Local or Cluster.

cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-active-service.yaml \
--set 'server.service.externalTrafficPolicy=local' \
--set 'server.service.type=NodePort' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Local" ]
}

@test "server/ha-active-Service: vault externalTrafficPolicy set to Local uppercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-active-service.yaml \
--set 'server.service.externalTrafficPolicy=LOCAL' \
--set 'server.service.type=NodePort' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Local" ]
}

@test "server/ha-active-Service: vault externalTrafficPolicy set to Cluster lowercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-active-service.yaml \
--set 'server.service.externalTrafficPolicy=cluster' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Cluster" ]
}

@test "server/ha-active-Service: vault externalTrafficPolicy set to Cluster uppercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-active-service.yaml \
--set 'server.service.externalTrafficPolicy=CLUSTER' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Cluster" ]
}
62 changes: 62 additions & 0 deletions test/unit/server-ha-standby-service.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,3 +168,65 @@ load _helpers
yq -r '.spec.ports | map(select(.port==8200)) | .[] .name' | tee /dev/stderr)
[ "${actual}" = "https" ]
}

@test "server/ha-standby-Service: vault externalTrafficPolicy set to Local lowercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-standby-service.yaml \
--set 'server.service.externalTrafficPolicy=local' \
--set 'server.service.type=NodePort' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Local" ]
}

@test "server/ha-standby-Service: vault externalTrafficPolicy set to Local uppercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-standby-service.yaml \
--set 'server.service.externalTrafficPolicy=LOCAL' \
--set 'server.service.type=NodePort' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Local" ]
}

@test "server/ha-standby-Service: vault externalTrafficPolicy set to Cluster lowercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-standby-service.yaml \
--set 'server.service.externalTrafficPolicy=cluster' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Cluster" ]
}

@test "server/ha-standby-Service: vault externalTrafficPolicy set to Cluster uppercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-standby-service.yaml \
--set 'server.service.externalTrafficPolicy=CLUSTER' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Cluster" ]
}

@test "server/ha-standby-Service: vault externalTrafficPolicy set to wrong values, fallback to Cluster" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-ha-standby-service.yaml \
--set 'server.service.externalTrafficPolicy=vault' \
--set 'server.ha.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Cluster" ]
}
46 changes: 46 additions & 0 deletions test/unit/server-service.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,3 +384,49 @@ load _helpers
yq -r '.spec.ports | map(select(.port==8200)) | .[] .name' | tee /dev/stderr)
[ "${actual}" = "https" ]
}

@test "server/Service: vault externalTrafficPolicy set to Local lowercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-service.yaml \
--set 'server.service.externalTrafficPolicy=local' \
--set 'server.service.type=NodePort' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Local" ]
}

@test "server/Service: vault externalTrafficPolicy set to Local uppercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-service.yaml \
--set 'server.service.externalTrafficPolicy=LOCAL' \
--set 'server.service.type=NodePort' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Local" ]
}

@test "server/Service: vault externalTrafficPolicy set to Cluster lowercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-service.yaml \
--set 'server.service.externalTrafficPolicy=cluster' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Cluster" ]
}

@test "server/Service: vault externalTrafficPolicy set to Cluster uppercase" {
cd `chart_dir`

local actual=$(helm template \
--show-only templates/server-service.yaml \
--set 'server.service.externalTrafficPolicy=CLUSTER' \
. | tee /dev/stderr |
yq -r '.spec.externalTrafficPolicy' | tee /dev/stderr)
[ "${actual}" = "Cluster" ]
}
8 changes: 8 additions & 0 deletions values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,14 @@ server:

# Enables a headless service to be used by the Vault Statefulset
service:
# The externalTrafficPolicy can be set to either Cluster or Local
shayfisher marked this conversation as resolved.
Show resolved Hide resolved
# when service type is NodePort, using Local will not forward packets
# to nodes that don't have an active endpoint but will allow preserving the
# source ip of the request. If Cluster is being used, whenever a packet
# is received on a node with no active endpoint it will be forwarded to
# a node which has one (replacing the source ip with internal ip address)
# for more information - https://kubernetes.io/docs/tutorials/services/source-ip/#source-ip-for-services-with-type-nodeport
externalTrafficPolicy: Cluster
enabled: true
# clusterIP controls whether a Cluster IP address is attached to the
# Vault service within Kubernetes. By default the Vault service will
Expand Down