-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat: allow usage of existing service account #363
base: master
Are you sure you want to change the base?
Conversation
0ef820d
to
7d495fb
Compare
7d495fb
to
c127a42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments. I like the idea of adding a semantic value like this, but I feel the way it currently is introduces more confusion than clarity. I expanded upon this in one of the threads I opened below.
{{- if .Values.rbac.enabled }} | ||
{{- if and .Values.rbac.serviceAccount.enabled .Values.rbac.existingServiceAccountName }} | ||
{{- fail "Conflict: 'rbac.existingServiceAccountName' is set, but a new service account is being created. Please disable 'rbac.serviceAccount.enabled' or unset 'rbac.existingServiceAccountName'." }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear that this may just cause confusion when reading release values later down the line. I'd much rather have .Values.rbac.enabled
be changed to .Values.rbac.create
instead, but I realize that would be a breaking change. The way things are in this PR, I find it a bit confusing for a release using an existing service account:
.Values.rbac.serviceAccount.name
exists, but is completely empty, meaning default or disabled. The comments hint at the default value being{{ include "application.name" $ }}
..Values.rbac.serviceAccount.enabled
exists and is set to false. What that means is unclear unless we peek into the templates or make an assumption based on widely available charts..Values.rbac.existingServiceAccount
exists is finally what we were expecting. Its mere presence introduces branching in the line of code this thread highlights.
If instead we had .Values.rbac.serviceAccount.create
as a boolean defaulting to false
, .Values.rbac.serviceAccount.name
would always have the name of the mounted (when applicable) service account.
Do you believe this legibility tradeoff is enough to justify a breaking change? I find myself unable to make that judgment.
{{- if .Values.rbac.serviceAccount.enabled }} | ||
{{- default (include "application.name" .) .Values.rbac.serviceAccount.name }} | ||
{{- else }} | ||
{{- default "null" .Values.rbac.existingServiceAccountName }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edge case, may or may not be something to take into consideration: service accounts named null
. Also, we're assuming the result of this partial template to be interpreted as YAML rather than a raw string, that may create confusion.
{{- default "null" .Values.rbac.existingServiceAccountName }} | |
{{- $saName := .Values.rbac.existingServiceAccountName }} | |
{{- empty $saName | ternary (quote $saName) "null" }} |
@@ -68,3 +68,18 @@ reference: | |||
kind: Route | |||
name: {{ include "application.name" . }} | |||
{{- end }} | |||
|
|||
{{- define "application.service-account-name" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This partial template (or whatever replaces it) needs to be used in application/templates/rolebinding.yaml
as well and anywhere else where template "application.name" $
(+ other branches) was used to come up with a service account name.
Would be good to add test cases for the templates that use it, though that might lead to duplicated tests.
No description provided.