-
Notifications
You must be signed in to change notification settings - Fork 0
Run workloads under dedicated ServiceAccounts without API tokens #49
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
base: pdb-topology-spread
Are you sure you want to change the base?
Changes from all commits
794718d
ccedfde
e6753a7
3dc0050
34f3da6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| {{- if .Values.serviceAccount.create }} | ||
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{ include "shopsys.serviceAccountName" . }} | ||
| labels: | ||
| {{- include "shopsys.labels" $ | nindent 4 }} | ||
| automountServiceAccountToken: {{ .Values.serviceAccount.automountToken }} | ||
| {{- end }} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| suite: dedicated service account | ||
| values: | ||
| - ./values/required.yaml | ||
| templates: | ||
| - templates/serviceaccount.yaml | ||
| - templates/deployment-webserver-php-fpm.yaml | ||
| - templates/hooks/job-migrate-application.yaml | ||
| tests: | ||
| - it: creates the chart ServiceAccount without token automount | ||
| template: templates/serviceaccount.yaml | ||
| asserts: | ||
| - equal: | ||
| path: metadata.name | ||
| value: shopsys-app | ||
| - equal: | ||
| path: automountServiceAccountToken | ||
| value: false | ||
|
|
||
| - it: assigns the ServiceAccount to workload pods | ||
| template: templates/deployment-webserver-php-fpm.yaml | ||
| asserts: | ||
| - equal: | ||
| path: spec.template.spec.serviceAccountName | ||
| value: shopsys-app | ||
| - equal: | ||
| path: spec.template.spec.automountServiceAccountToken | ||
| value: false | ||
|
|
||
| - it: honors a custom name | ||
| template: templates/serviceaccount.yaml | ||
| set: | ||
| serviceAccount: | ||
| name: my-account | ||
| asserts: | ||
| - equal: | ||
| path: metadata.name | ||
| value: my-account | ||
|
|
||
| - it: references an externally managed account when create is false | ||
| template: templates/deployment-webserver-php-fpm.yaml | ||
| set: | ||
| serviceAccount: | ||
| create: false | ||
| name: external-account | ||
| asserts: | ||
| - equal: | ||
| path: spec.template.spec.serviceAccountName | ||
| value: external-account | ||
|
|
||
| - it: renders no ServiceAccount when create is false | ||
| template: templates/serviceaccount.yaml | ||
| set: | ||
| serviceAccount: | ||
| create: false | ||
| asserts: | ||
| - hasDocuments: | ||
| count: 0 | ||
|
|
||
| - it: falls back to the default SA when create is false and no name is given | ||
| template: templates/deployment-webserver-php-fpm.yaml | ||
| set: | ||
| serviceAccount: | ||
| create: false | ||
| asserts: | ||
| - equal: | ||
| path: spec.template.spec.serviceAccountName | ||
| value: default | ||
| # pod-level automount still guarantees no token, whatever the SA itself mounts | ||
| - equal: | ||
| path: spec.template.spec.automountServiceAccountToken | ||
| value: false | ||
|
|
||
| - it: mounts the token at pod level when automountToken is enabled | ||
| template: templates/deployment-webserver-php-fpm.yaml | ||
| set: | ||
| serviceAccount: | ||
| automountToken: true | ||
| asserts: | ||
| - equal: | ||
| path: spec.template.spec.automountServiceAccountToken | ||
| value: true | ||
|
|
||
| - it: does not mount the default SA token into the migration hook | ||
| template: templates/hooks/job-migrate-application.yaml | ||
| asserts: | ||
| - equal: | ||
| path: spec.template.spec.automountServiceAccountToken | ||
| value: false | ||
| - notExists: | ||
| path: spec.template.spec.serviceAccountName |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -248,10 +248,23 @@ checksum/php-fpm: {{ printf "%s%s" ($root.Values.webserver.phpFpm.config | defau | |
| {{- end }} | ||
| {{- end }} | ||
|
|
||
| {{/* Name of the chart-scoped ServiceAccount workload pods run under. | ||
| Defaults to the chart name; with create=false an explicitly named (externally | ||
| managed) ServiceAccount is referenced, falling back to "default". */}} | ||
| {{- define "shopsys.serviceAccountName" -}} | ||
| {{- if .Values.serviceAccount.create -}} | ||
| {{- .Values.serviceAccount.name | default .Chart.Name -}} | ||
| {{- else -}} | ||
| {{- .Values.serviceAccount.name | default "default" -}} | ||
|
Comment on lines
+257
to
+258
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low: with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed the silent fallback was a real gap, but a |
||
| {{- end -}} | ||
| {{- end }} | ||
|
|
||
| {{/* Standard scheduling/security pod fields shared by every component. | ||
| ctx: (dict "root" $ "component" <component values>) | ||
| Rendered at zero indent — use `| nindent N` at the call site. */}} | ||
| {{- define "shopsys.podSettings" -}} | ||
| serviceAccountName: {{ include "shopsys.serviceAccountName" .root }} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion (Low): the no-token guarantee currently lives only on the chart-created SA object. Emitting the pod-level field here as well, e.g. serviceAccountName: {{ include "shopsys.serviceAccountName" .root }}
automountServiceAccountToken: {{ .root.Values.serviceAccount.automountToken }}would make it hold regardless of SA-level configuration (pod-level overrides SA-level), covering both the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in ccedfde exactly as suggested: |
||
| automountServiceAccountToken: {{ .root.Values.serviceAccount.automountToken }} | ||
| {{- with .component.nodeSelector }} | ||
| nodeSelector: | ||
| {{ toYaml . | indent 2 }} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| {{- if .Values.serviceAccount.create }} | ||
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{ include "shopsys.serviceAccountName" . }} | ||
| labels: | ||
| {{- include "shopsys.labels" $ | nindent 4 }} | ||
| automountServiceAccountToken: {{ .Values.serviceAccount.automountToken }} | ||
| {{- end }} |
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.
Info: worth a note here (or in
docs/values.md) thatautomountTokenonly takes effect whencreate: true— withcreate: falseit is silently ignored and the external SA's own automount setting wins. Also consider adding theserviceAccountblock tovalues.schema.jsonin a follow-up (create: boolean,name: string,automountToken: boolean) so typos fail at render time. — Claude Agent (CR)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.
The "only takes effect when create=true" caveat no longer exists after ccedfde:
automountTokennow also drives the pod-levelautomountServiceAccountTokenon every workload pod, so it is effective withcreate: falsetoo — documented in both values.yaml comments,docs/values.md, and deviation 23. Thevalues.schema.jsonaddition is deferred to a follow-up so it can cover thepdb/topologySpreadConstraintsblocks from #48 in the same change and keep the stack consistent — flagged to the maintainer.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.
Follow-up: per the maintainer's decision the schema coverage now ships inside the PRs themselves instead of a separate follow-up —
serviceAccountwas added to both charts'values.schema.jsonin this PR (34f3da6,additionalProperties: falseso typos likeautomountServiceAccountTokenfail the render), andpdb+topologySpreadConstraintswere added in #48 (f93671c).