Skip to content

[Helm Chart]: Add automountServiceAccountToken to pod spec#629

Merged
stevehipwell merged 1 commit intofluent:mainfrom
bianchi2:make-automount-sa-token-configurable
Aug 14, 2025
Merged

[Helm Chart]: Add automountServiceAccountToken to pod spec#629
stevehipwell merged 1 commit intofluent:mainfrom
bianchi2:make-automount-sa-token-configurable

Conversation

@bianchi2
Copy link
Contributor

@bianchi2 bianchi2 commented Aug 8, 2025

Setting automountServiceAccountToken to false is often enforced in regulated clusters. Adding it to pod spec with the default value true.

Copy link
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bianchi2. I've added some suggestions to tidy up the implementation.

Also I think that if the chart provides a serviceAccount.automountToken value then I'd expect this to primarily modify the ServiceAccount resource; so could you please add automountServiceAccountToken: {{ .Values.serviceAccount.automountToken }} to the service account template.

I think I'm happy that this value is also used for the pod spec.

@bianchi2
Copy link
Contributor Author

bianchi2 commented Aug 8, 2025

@stevehipwell thanks! Indeed, it's cleaner this way - no changes to existing deployments. Fixed now.

@bianchi2
Copy link
Contributor Author

@stevehipwell please let me know if there's anything else to do to have this one merged. That's the only thing that forces us fork this helm chart (or use kustomize post render) which we'd like to avoid. Thanks

Copy link
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

Also I think that if the chart provides a serviceAccount.automountToken value then I'd expect this to primarily modify the ServiceAccount resource; so could you please add automountServiceAccountToken: {{ .Values.serviceAccount.automountToken }} to the service account template.

A per my previous comment I'm waiting on an update to the ServiceAccount template, but using the with pattern as the default is null.

Also you will need to bump the chart version and update the annotations in Chart.yaml.

@bianchi2
Copy link
Contributor Author

bianchi2 commented Aug 13, 2025

@stevehipwell I have updated the PR. One thing though. When using:

{{- with .Values.serviceAccount.automountServiceAccountToken }}
automountServiceAccountToken: {{ . }}
{{- end }}

automountServiceAccountToken does not show up in the generated yamls at all when I set serviceAccount.automountServiceAccountToken to false (which is why we need it in the first place since it's implicitly true by default). That's the behavior of "with syntax" - if it's false, it skips it.

In helpers, I have defined the following, basically checking if it's not nil:

{{- define "fluent-bit.automountServiceAccountToken" -}}
{{- if ne .Values.serviceAccount.automountServiceAccountToken nil }}
automountServiceAccountToken: {{ .Values.serviceAccount.automountServiceAccountToken }}
{{- end }}
{{- end }}

So now, when unset (default behavior), nothing changes in the rendered templates, and when set the value is used in both pod and sa.

I hope it makes sense.

@bianchi2 bianchi2 force-pushed the make-automount-sa-token-configurable branch from f60cc14 to 6c187e7 Compare August 13, 2025 21:25
Copy link
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

I'd rather keep this simple and not create a template for a conditional check.

Signed-off-by: Yevhen Ivantsov <yivantsov@atlassian.com>
@bianchi2 bianchi2 force-pushed the make-automount-sa-token-configurable branch from ee7f751 to f5eb544 Compare August 14, 2025 10:08
Copy link
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants