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

Jump to bottom

feat: Helm - custom service account creation and management #17880

Merged
merged 7 commits into from
Jan 4, 2022
Merged

feat: Helm - custom service account creation and management #17880

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fa02860
feat: Custom service account creation and management
wiktor2200 Dec 28, 2021
be299eb
bump helm chart version
wiktor2200 Dec 28, 2021
372fed1
add custom service account in init-job
wiktor2200 Dec 29, 2021
45d05ff
service account creation template
wiktor2200 Dec 29, 2021
9bf581b
Merge branch 'master' into feature-custom-service-account-management
wiktor2200 Dec 29, 2021
dedd6d5
changed service account creation template
wiktor2200 Dec 30, 2021
f023c40
add license
wiktor2200 Jan 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion helm/superset/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ maintainers:
- name: craig-rueda
email: craig@craigrueda.com
url: https://github.com/craig-rueda
version: 0.5.1
version: 0.5.2
dependencies:
- name: postgresql
version: 10.2.0
Expand Down
11 changes: 11 additions & 0 deletions helm/superset/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,17 @@ If release name contains chart name it will be used as a full name.
{{- end -}}
{{- end -}}

{{/*
Create the name of the service account to use
*/}}
{{- define "superset.serviceAccountName" -}}
{{- if .Values.serviceAccount.create -}}
{{- default (include "superset.fullname" .) .Values.serviceAccountName -}}
{{- else -}}
{{- default "default" .Values.serviceAccountName -}}
{{- end -}}
{{- end -}}

{{/*
Create chart name and version as used by the chart label.
*/}}
Expand Down
4 changes: 2 additions & 2 deletions helm/superset/templates/deployment-worker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ spec:
app: {{ template "superset.name" . }}-worker
release: {{ .Release.Name }}
spec:
{{- if .Values.serviceAccountName }}
serviceAccountName: {{ .Values.serviceAccountName }}
{{- if or (.Values.serviceAccount.create) (.Values.serviceAccountName) }}
serviceAccountName: {{ template "superset.serviceAccountName" . }}
{{- end }}
securityContext:
runAsUser: {{ .Values.runAsUser }}
Expand Down
4 changes: 2 additions & 2 deletions helm/superset/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ spec:
app: {{ template "superset.name" . }}
release: {{ .Release.Name }}
spec:
{{- if .Values.serviceAccountName }}
serviceAccountName: {{ .Values.serviceAccountName }}
{{- if or (.Values.serviceAccount.create) (.Values.serviceAccountName) }}
serviceAccountName: {{ template "superset.serviceAccountName" . }}
{{- end }}
securityContext:
runAsUser: {{ .Values.runAsUser }}
Expand Down
3 changes: 3 additions & 0 deletions helm/superset/templates/init-job.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ spec:
{{ toYaml .Values.init.podAnnotations | nindent 8 }}
{{- end }}
spec:
{{- if or (.Values.serviceAccount.create) (.Values.serviceAccountName) }}
serviceAccountName: {{ template "superset.serviceAccountName" . }}
{{- end }}
securityContext:
runAsUser: {{ .Values.runAsUser }}
{{- if .Values.init.initContainers }}
Expand Down
15 changes: 15 additions & 0 deletions helm/superset/templates/service-account.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{{- if .Values.serviceAccount.create -}}
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ if .Values.serviceAccountName }}{{ .Values.serviceAccountName }}{{ else }}{{ include "superset.fullname" . }}{{ end }}
wiktor2200 marked this conversation as resolved.
Show resolved Hide resolved
labels:
app.kubernetes.io/name: {{ include "superset.name" . }}
helm.sh/chart: {{ include "superset.chart" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- if semverCompare "> 1.6" .Capabilities.KubeVersion.GitVersion }}
kubernetes.io/cluster-service: "true"
{{- end }}
addonmanager.kubernetes.io/mode: Reconcile
{{- end -}}
5 changes: 5 additions & 0 deletions helm/superset/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ replicaCount: 1
# Runn containers as root is not recommended in production. Change this to another UID - e.g. 1000 to be more secure
runAsUser: 0

# Create custom service account for Superset. If create: true and name is not provided, superset.fullname will be used.
# serviceAccountName: superset
serviceAccount:
create: false
Copy link

Choose a reason for hiding this comment

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

How about:

  1. Remove "serviceAccountName"
  2. creating serviceAccount sestion as you did with two properties:
  • create as you did
  • name

According to: https://helm.sh/docs/chart_best_practices/rbac/#yaml-configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've thought about it. That was even my first choice, but then I've noticed that, there was ServiceAccountName variable added in the past #15340, so I've implemented new feature as enhancement beside the current solution.
I don't want to destroy any configs that can exist somewhere.
@mvoitko and @craig-rueda can you give some more feedback about this?

Copy link

Choose a reason for hiding this comment

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

Yeps you are right, we would break backward compatibility but on the other site, we could maintain both for sometime and prepare a communication about that right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If someone have blocked version on helm in their definition it's OK, but when someone is using "latest" it will brake down deployment (or dependent components).
Supporting two solutions for a while should be quite easy (I'll just add one more conditions in _helpers) but this could cause some caveats such as overwriting values if serviceAccountName and serviceAccount.name would be defined in the same time. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I think it LGTM. If people are using "latest" in any env that matters, that's generally a bad practice. Using the default of create: false seems reasonable to me.


# Install additional packages and do any other bootstrap configuration in this script
# For production clusters it's recommended to build own image with this step done in CI
bootstrapScript: |
Expand Down