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

csi/server.statefulset: custom security context #767

Merged
merged 5 commits into from
Aug 8, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ CHANGES:
* Deprecated `injector.externalVaultAddr`. Added `global.externalVaultAddr`, which applies to both the Injector and the CSI Provider. [GH-745](https://github.com/hashicorp/vault-helm/pull/745)
* CSI Provider pods now set the `VAULT_ADDR` environment variable to either the internal Vault service or the configured external address. [GH-745](https://github.com/hashicorp/vault-helm/pull/745)
* Deprecated `injector.uid` and `injector.gid`. Replaced with `injector.securityContext.pod`. [GH-750](https://github.com/hashicorp/vault-helm/pull/750)
Copy link
Member

Choose a reason for hiding this comment

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

We should probably remove this line, since this PR effectively un-deprecates injector.uid and injector.gid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

* injector: Allow setting securityContext for pod and container to be objects or YAML strings. [GH-767](https://github.com/hashicorp/vault-helm/pull/767)
Copy link
Member

Choose a reason for hiding this comment

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

We could probably move this down/combine it with the entry on line 14 (just mention both PRs in the entry) since GH-750 hasn't been released yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


Features:
* server: Add `server.statefulSet.securityContext` to override pod and container `securityContext`. [GH-767](https://github.com/hashicorp/vault-helm/pull/767)
* csi: Add `csi.daemonSet.securityContext` to override pod and container `securityContext`. [GH-767](https://github.com/hashicorp/vault-helm/pull/767)
* Add `server.service.activeNodePort` and `server.service.standbyNodePort` to specify the `nodePort` for active and standby services. [GH-610](https://github.com/hashicorp/vault-helm/pull/610)
* Support for setting annotations on the injector's serviceAccount [GH-753](https://github.com/hashicorp/vault-helm/pull/753)
* injector: Support setting both pod and container securityContext [GH-750](https://github.com/hashicorp/vault-helm/pull/750)
Expand Down
94 changes: 88 additions & 6 deletions templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -474,14 +474,20 @@ Sets extra injector service annotations
securityContext for the injector pod level.
*/}}
{{- define "injector.securityContext.pod" -}}
{{- if or (.Values.injector.uid) (.Values.injector.gid) }}
{{- if .Values.injector.securityContext.pod }}
securityContext:
{{- $tp := typeOf .Values.injector.securityContext.pod }}
{{- if eq $tp "string" }}
{{- tpl .Values.injector.securityContext.pod . | nindent 8 }}
{{- else }}
{{- toYaml .Values.injector.securityContext.pod | nindent 8 }}
{{- end }}
{{- else }}
securityContext:
runAsNonRoot: true
runAsGroup: {{ .Values.injector.gid | default 1000 }}
runAsUser: {{ .Values.injector.uid | default 100 }}
{{- else if .Values.injector.securityContext.pod }}
securityContext:
{{- toYaml .Values.injector.securityContext.pod | nindent 8 }}
fsGroup: {{ .Values.injector.gid | default 1000 }}
Copy link
Member

Choose a reason for hiding this comment

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

Should we move the openshift check into the helper's else clause similar to the server and csi securityContext helpers? That way folks can set a securityContext if they so choose on openshift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

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

Expand All @@ -491,9 +497,54 @@ securityContext for the injector container level.
{{- define "injector.securityContext.container" -}}
{{- if .Values.injector.securityContext.container}}
securityContext:
{{- toYaml .Values.injector.securityContext.container | nindent 12 }}
{{- $tp := typeOf .Values.injector.securityContext.container }}
{{- if eq $tp "string" }}
{{- tpl .Values.injector.securityContext.container . | nindent 12 }}
Comment on lines +501 to +502
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to add another test that sets this as a multi-line string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

{{- else }}
{{- toYaml .Values.injector.securityContext.container | nindent 12 }}
{{- end }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Should we move the openshift check into the helper here to match server and csi? i.e.

Suggested change
{{- end }}
{{- else if not .Values.global.openshift }}

And remove the check from injector-deployment.yaml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but wouldn't the else here be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, nevermind -- I see, we should move the default out of values.yaml anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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

{{/*
securityContext for the statefulset pod template.
*/}}
{{- define "server.statefulSet.securityContext.pod" -}}
{{- if .Values.server.statefulSet.securityContext.pod }}
securityContext:
{{- $tp := typeOf .Values.server.statefulSet.securityContext.pod }}
{{- if eq $tp "string" }}
{{- tpl .Values.server.statefulSet.securityContext.pod . | nindent 8 }}
{{- else }}
{{- toYaml .Values.server.statefulSet.securityContext.pod | nindent 8 }}
{{- end }}
{{- else if not .Values.global.openshift }}
securityContext:
runAsNonRoot: true
runAsGroup: {{ .Values.server.gid | default 1000 }}
runAsUser: {{ .Values.server.uid | default 100 }}
fsGroup: {{ .Values.server.gid | default 1000 }}
{{- end }}
{{- end -}}

{{/*
securityContext for the statefulset vault container
*/}}
{{- define "server.statefulSet.securityContext.container" -}}
{{- if .Values.server.statefulSet.securityContext.container }}
securityContext:
{{- $tp := typeOf .Values.server.statefulSet.securityContext.container }}
{{- if eq $tp "string" }}
{{- tpl .Values.server.statefulSet.securityContext.container . | nindent 12 }}
{{- else }}
{{- toYaml .Values.server.statefulSet.securityContext.container | nindent 12 }}
{{- end }}
{{- else if not .Values.global.openshift }}
securityContext:
allowPrivilegeEscalation: false
{{- end }}
{{- end -}}


{{/*
Sets extra injector service account annotations
Expand Down Expand Up @@ -731,6 +782,37 @@ Sets extra CSI daemonset annotations
{{- end }}
{{- end -}}

{{/*
Sets CSI daemonset securityContext for pod template
*/}}
{{- define "csi.daemonSet.securityContext.pod" -}}
{{- if .Values.csi.daemonSet.securityContext.pod }}
securityContext:
{{- $tp := typeOf .Values.csi.daemonSet.securityContext.pod }}
{{- if eq $tp "string" }}
{{- tpl .Values.csi.daemonSet.securityContext.pod . | nindent 8 }}
{{- else }}
{{- toYaml .Values.csi.daemonSet.securityContext.pod | nindent 8 }}
{{- end }}
{{- end }}
{{- end -}}

{{/*
Sets CSI daemonset securityContext for container
*/}}
{{- define "csi.daemonSet.securityContext.container" -}}
{{- if .Values.csi.daemonSet.securityContext.container }}
securityContext:
{{- $tp := typeOf .Values.csi.daemonSet.securityContext.container }}
{{- if eq $tp "string" }}
{{- tpl .Values.csi.daemonSet.securityContext.container . | nindent 12 }}
{{- else }}
{{- toYaml .Values.csi.daemonSet.securityContext.container | nindent 12 }}
{{- end }}
{{- end }}
{{- end -}}


{{/*
Sets the injector toleration for pod placement
*/}}
Expand Down
2 changes: 2 additions & 0 deletions templates/csi-daemonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ spec:
{{- end -}}
{{ template "csi.pod.annotations" . }}
spec:
{{ template "csi.daemonSet.securityContext.pod" . }}
{{- if .Values.csi.priorityClassName }}
priorityClassName: {{ .Values.csi.priorityClassName }}
{{- end }}
Expand All @@ -42,6 +43,7 @@ spec:
containers:
- name: {{ include "vault.name" . }}-csi-provider
{{ template "csi.resources" . }}
{{ template "csi.daemonSet.securityContext.container" . }}
image: "{{ .Values.csi.image.repository }}:{{ .Values.csi.image.tag }}"
imagePullPolicy: {{ .Values.csi.image.pullPolicy }}
args:
Expand Down
13 changes: 2 additions & 11 deletions templates/server-statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,7 @@ spec:
{{ if .Values.server.shareProcessNamespace }}
shareProcessNamespace: true
{{ end }}
{{- if not .Values.global.openshift }}
securityContext:
runAsNonRoot: true
runAsGroup: {{ .Values.server.gid | default 1000 }}
runAsUser: {{ .Values.server.uid | default 100 }}
fsGroup: {{ .Values.server.gid | default 1000 }}
{{- end }}
{{- template "server.statefulSet.securityContext.pod" . }}
volumes:
{{ template "vault.volumes" . }}
- name: home
Expand All @@ -72,10 +66,7 @@ spec:
- "/bin/sh"
- "-ec"
args: {{ template "vault.args" . }}
{{- if not .Values.global.openshift }}
securityContext:
allowPrivilegeEscalation: false
{{- end }}
{{- template "server.statefulSet.securityContext.container" . }}
env:
- name: HOST_IP
valueFrom:
Expand Down
56 changes: 56 additions & 0 deletions test/unit/csi-daemonset.bats
Original file line number Diff line number Diff line change
Expand Up @@ -592,3 +592,59 @@ load _helpers
yq -r 'map(select(.name=="VAULT_ADDR")) | .[] .value' | tee /dev/stderr)
[ "${value}" = "http://vault-outside" ]
}

#--------------------------------------------------------------------
# securityContext

@test "csi/daemonset: default csi.daemonSet.securityContext.pod" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.securityContext' | tee /dev/stderr)
[ "${actual}" = "null" ]
}

@test "csi/daemonset: default csi.daemonSet.securityContext.container" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.enabled=true' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.containers[0].securityContext' | tee /dev/stderr)
[ "${actual}" = "null" ]
}

@test "csi/daemonset: specify csi.daemonSet.securityContext.pod yaml" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.enabled=true' \
--set 'csi.daemonSet.securityContext.pod.foo=bar' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.securityContext.foo' | tee /dev/stderr)
[ "${actual}" = "bar" ]
}

@test "csi/daemonset: specify csi.daemonSet.securityContext.container yaml" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.enabled=true' \
--set 'csi.daemonSet.securityContext.container.foo=bar' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.containers[0].securityContext.foo' | tee /dev/stderr)
[ "${actual}" = "bar" ]
}

@test "csi/daemonset: specify csi.daemonSet.securityContext.container yaml string" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/csi-daemonset.yaml \
--set 'csi.enabled=true' \
--set 'csi.daemonSet.securityContext.container=foo: bar' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.containers[0].securityContext.foo' | tee /dev/stderr)
[ "${actual}" = "bar" ]
}
61 changes: 61 additions & 0 deletions test/unit/server-statefulset.bats
Original file line number Diff line number Diff line change
Expand Up @@ -1723,3 +1723,64 @@ load _helpers
yq -r -c '.spec.template.spec.containers[0].env[] | select(.name == "VAULT_LICENSE_PATH")' | tee /dev/stderr)
[ "${actual}" = '' ]
}

#--------------------------------------------------------------------
# securityContext

@test "server/standalone-StatefulSet: default statefulSet.securityContext.pod" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/server-statefulset.yaml \
. | tee /dev/stderr |
yq -r '.spec.template.spec.securityContext' | tee /dev/stderr)
[ ! "${actual}" = "null" ]
}

@test "server/standalone-StatefulSet: default statefulSet.securityContext.container" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/server-statefulset.yaml \
. | tee /dev/stderr |
yq -r '.spec.template.spec.containers[0].securityContext' | tee /dev/stderr)
[ ! "${actual}" = "null" ]
}

@test "server/standalone-StatefulSet: specify statefulSet.securityContext.pod yaml" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/server-statefulset.yaml \
--set 'server.statefulSet.securityContext.pod.foo=bar' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.securityContext.foo' | tee /dev/stderr)
[ "${actual}" = "bar" ]
}

@test "server/standalone-StatefulSet: specify statefulSet.securityContext.container yaml" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/server-statefulset.yaml \
--set 'server.statefulSet.securityContext.container.foo=bar' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.containers[0].securityContext.foo' | tee /dev/stderr)
[ "${actual}" = "bar" ]
}

@test "server/standalone-StatefulSet: specify statefulSet.securityContext.pod yaml string" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/server-statefulset.yaml \
--set 'server.statefulSet.securityContext.pod=foo: bar' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.securityContext.foo' | tee /dev/stderr)
[ "${actual}" = "bar" ]
}

@test "server/standalone-StatefulSet: specify statefulSet.securityContext.container yaml string" {
cd `chart_dir`
local actual=$(helm template \
--show-only templates/server-statefulset.yaml \
--set 'server.statefulSet.securityContext.container=foo: bar' \
. | tee /dev/stderr |
yq -r '.spec.template.spec.containers[0].securityContext.foo' | tee /dev/stderr)
[ "${actual}" = "bar" ]
}
44 changes: 42 additions & 2 deletions values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,23 @@
"providersDir": {
"type": "string"
},
"securityContext": {
"type": "object",
"properties": {
"container": {
"type": [
"object",
"string"
]
},
"pod": {
"type": [
"object",
"string"
]
}
}
},
"updateStrategy": {
"type": "object",
"properties": {
Expand Down Expand Up @@ -366,10 +383,16 @@
"type": "object",
"properties": {
"container": {
"type": "object"
"type": [
"object",
"string"
]
},
"pod": {
"type": "object"
"type": [
"object",
"string"
]
}
}
},
Expand Down Expand Up @@ -905,6 +928,23 @@
"object",
"string"
]
},
"securityContext": {
"type": "object",
"properties": {
"container": {
"type": [
"object",
"string"
]
},
"pod": {
"type": [
"object",
"string"
]
}
}
}
}
},
Expand Down
Loading