Skip to content

Conversation

@gsmith-sas
Copy link

NOTE: This is the same change as PR #632. I abandoned that PR since it looks like I may have messed up the commit history while rebasing.

This change allows users to override the automountServiceAccountToken setting at the pod level. The Kubernetes API allows this property to be set at two levels:

  • ServiceAccount level - when set at the ServiceAccount level, the setting impacts all pods associated with the ServiceAccount. The default value for this property is "true" meaning that the API token of the ServiceAccount is automatically mounted to any pod associated with the ServiceAccount.

  • Pod level - when set at the Pod level, the setting only impacts that specific pod. When set at both levels, the Pod-level setting overrides the ServiceAccount level.

As described in PR #629, many sites require that this property be disabled for security reasons since the default value of "true" makes API tokens available by default. And, some security scans will flag this default behavior as potentially problematic. The change in PR #629 allows users to disable this setting and, when disabled, disables it at both the ServiceAccount and Pod levels.
However, there are situations where it is appropriate to disable this setting at the ServiceAccount level but enable it at the Pod level. For example, when configuring Fluent Bit to collect Kubernetes Events, access to the API tokens is required. The solution is to disable the setting at the ServiceAccount level (which PR#629 now permits) but enable it at the Pod level (which PR#629 does not permit). This PR is designed to allow users to enable this.

In this PR, I chose to name the new property automountServiceAccountToken even though that is very similr to the new property added in PR #629 (which added serviceAccount.automountServiceAccountToken). While this is potentially confusing, I think aligning the names of the Helm chart values with the properties they impact in the underlying Kubernetes resources is critical. This naming is consistent with other Helm charts I have reviewed (e.g. the Grafana Helm chart that support setting this property at both the ServiceAccount and Pod levels. I have also attempted to minimize confusion by adding comments immediately before both properties in the values.yaml file explaining the scope of each property.

@gsmith-sas
Copy link
Author

@stevehipwell This is the same change (#632) you were reviewing but with a cleaner commit history. Sorry for the churn/confusion.

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.

@gsmith-sas after further thought I think the existing value should be deprecated and replaced by new values that are clearer in their intent. The pattern I've suggested is backwards compatible, and while this chart is pre-release I think it's worth using a deprecation pattern.

@gsmith-sas
Copy link
Author

@stevehipwell Thanks for reviewing the PR and providing your feedback. However, I was a little confused after reading your comments and want to clarify things before making any changes. I believe a high-level summary of your suggestions is the following:

  • Rename the existing serviceAccount.automountServiceAccountToken key (introduced in PR [Helm Chart]: Add automountServiceAccountToken to pod spec #629) to serviceAccount.automountToken
  • To provide backwards compatibility, maintain support for the now deprecated serviceAccount.automountServiceAccountToken key in the ServiceAcccount template
  • Adjust wording of comments in values.yaml to clarify the above.

But, if so, I was also confused by a couple of comments in your suggested changes to values.yaml:

  • Line 36: # Explicit setting for if the Fluent Bit pods should have the service account token auto mounted.
    Is this comment referring to the serviceAccount.automountToken setting or the automountServiceAccountToken setting? If it is the former, it is incorrect since that setting applies to all pods associated with the ServiceAccount as described in the comment on Line 34. If it is the latter, adding some blank lines and/or moving it "down" would probably make that more clear.

  • Line 37: # Deprecated use the root automountServiceAccountToken value
    This comment seems to indicate the desire to deprecate the very feature this PR is trying to add: the new "root-level" setting (i.e. automountServiceAccountToken). As noted above, I think your suggestion was to deprecate serviceAccount.automountServiceAccountToken in favor of the more compact (and less potentially confusing) serviceAccount.automountToken. Please confirm what you are suggesting we deprecate.

  • Line 41: # Automatically mount the ServiceAccount token on all pods using this ServiceAccount unless they've explicitly opted out.
    This was your suggested comment to appear before the new automountServiceAccountToken setting; but it seems to describe the functionality of the serviceAccount.automountToken. Hmmm, thinking about it further, perhaps this is simply a case of having the comments reversed (i.e. the comments on Line 36 and Line 41 should be swapped). Please confirm or clarify.

Assuming I am correct in what your overall intent is, I will also need to make changes to the Pod template as well: adding support for serviceAccount.automountToken (in addition to continuing to support serviceAccount.automountServiceAccountToken for backwards compatibility).

And, finally, assuming I have things straight, I think that means the following things need to be included in the artifact.io/changes annotations:

  • addition of serviceAccount.automountToken
  • deprecation of serviceAccount.automountServiceAccountToken (in favor of the addition of serviceAccount.automountToken)
  • addition of automountServiceAccountToken

Sorry for the lengthy comment but I thought it would be best to sort this out before pushing changes. Thank you again for taking time to review this PR.

Cheers,
Greg

@stevehipwell
Copy link
Collaborator

@gsmith-sas the intent is as follows.

The existing value controlling both the sa and the pod is deprecated and is deprioritized; if it is set then it will only apply where the new more specific values haven't been set.

The new values are specific to the sa and the pod and override the deprecated value.

It's probably easiest to comment on iterative changes, otherwise the cognitive load to cross reference non-inline comments with the code is much too high.

@gsmith-sas
Copy link
Author

@stevehipwell Thank you for the clarification. I believe I have addressed all of your concerns in my latest commit.

Signed-off-by: Greg Smith <[email protected]>
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 making the changes, but I think we still need another change.

gsmith-sas and others added 2 commits October 8, 2025 12:22
Co-authored-by: Steve Hipwell <[email protected]>
Signed-off-by: Greg Smith <[email protected]>
Co-authored-by: Steve Hipwell <[email protected]>
Signed-off-by: Greg Smith <[email protected]>
@gsmith-sas
Copy link
Author

@stevehipwell Thanks for the (re)review and suggested code modifications. I have incorporated your suggestions which I think address all outstanding concerns. If so, I think this PR should be good to go. Cheers, Greg

@gsmith-sas
Copy link
Author

@stevehipwell I just noticed there were some merge conflicts due to other PRs being merged in recently. I've now taken care of those conflicts, so we should be good to go. Cheers, Greg

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 @gsmith-sas. I just tested this locally and spotted one more thing to fix and then we can get this merged.

Co-authored-by: Steve Hipwell <[email protected]>
Signed-off-by: Greg Smith <[email protected]>
@gsmith-sas
Copy link
Author

gsmith-sas commented Oct 21, 2025

@stevehipwell I accepted your change (rather than re-implementing it) and I still run into a failure when trying to deploy. I apologize for including the following lengthy chunks of console session output, but I wanted to be transparent about my testing process.

In this first snippet, you can see I've done the following:

  • created a fresh clone of the repo to eliminate the chance of accidently picking up some other random change
  • dumped the git log to confirm I have all of the changes (including your suggestion)
  • used the helm package command to create the helm chart tgz file
  • deployed the Fluent Bit Helm chart with no customizations
    • confirmed that worked as expected (one Fluent Bit pod per Kubernetes node)
  • removed the Helm release
  • attempted deploy again setting automountServiceAccountToken to 'true'
    • notice the Helm install command fails with the error I reported last week
  • attempted deploy again setting automountServiceAccountToken to 'false'
    • notice the Helm install command fails with the error I reported last week
[greg@gemini-m1 repos]$ git clone [email protected]:gsmith-sas/fluent-helm-charts.git
Cloning into 'fluent-helm-charts'...
remote: Enumerating objects: 3226, done.
remote: Counting objects: 100% (87/87), done.
remote: Compressing objects: 100% (67/67), done.
remote: Total 3226 (delta 59), reused 20 (delta 20), pack-reused 3139 (from 3)
Receiving objects: 100% (3226/3226), 1.45 MiB | 11.67 MiB/s, done.
Resolving deltas: 100% (1957/1957), done.

[greg@gemini-m1 fluent-helm-charts]$ git checkout podsatoken2
branch 'podsatoken2' set up to track 'origin/podsatoken2'.
Switched to a new branch 'podsatoken2'

[greg@gemini-m1 fluent-helm-charts]$ git log --oneline
9a8bd85 (HEAD -> podsatoken2, origin/podsatoken2) Update charts/fluent-bit/templates/_pod.tpl
6a43b17 Merge branch 'main' into podsatoken2
e0f34ec Correct fluent-bit version
7008e47 Bump fluent-bit chart version number
af4df3b (origin/main, origin/HEAD, main) build(deps): bump the github-actions group across 1 directory with 4 updates (#644)
8749b0a feat(fluent-bit): Update image to 4.1.0 (#639)
{snip...additional log lines omitted}

[greg@gemini-m1 fluent-helm-charts]$ cd charts/fluent-bit/

[greg@gemini-m1 fluent-bit]$ helm package .
Successfully packaged chart and saved it to: /home/greg/repos/fluent-helm-charts/charts/fluent-bit/fluent-bit-0.55.0.tgz

[greg@gemini-m1 fluent-bit]$ helm install fbtest fluent-bit-0.55.0.tgz
NAME: fbtest
LAST DEPLOYED: Mon Oct 20 18:55:51 2025
NAMESPACE: default
STATUS: deployed
REVISION: 1
NOTES:
Get Fluent Bit build information by running these commands:

export POD_NAME=$(kubectl get pods --namespace default -l "app.kubernetes.io/name=fluent-bit,app.kubernetes.io/instance=fbtest" -o jsonpath="{.items[0].metadata.name}")
kubectl --namespace default port-forward $POD_NAME 2020:2020
curl http://127.0.0.1:2020
[greg@gemini-m1 fluent-bit]$ kubectl get pods
NAME                           READY   STATUS    RESTARTS        AGE
fbtest-fluent-bit-jwjgz        1/1     Running   0               9s
fbtest-fluent-bit-mqn5z        1/1     Running   0               9s
fbtest-fluent-bit-w9htg        1/1     Running   0               9s
fbtest-fluent-bit-zdqvl        1/1     Running   0               9s
hello-world-6d54847684-p57gh   1/1     Running   8 (7h18m ago)   60d
hello-world-6d54847684-smw6p   1/1     Running   8 (7h18m ago)   60d

[greg@gemini-m1 fluent-bit]$ helm delete fbtest
release "fbtest" uninstalled

[greg@gemini-m1 fluent-bit]$ helm install --set automountServiceAccountToken=true fbtest fluent-bit-0.55.0.tgz
Error: INSTALLATION FAILED: YAML parse error on fluent-bit/templates/daemonset.yaml: error converting YAML to JSON: yaml: line 26: mapping values are not allowed in this context

[greg@gemini-m1 fluent-bit]$ helm install --set automountServiceAccountToken=false fbtest fluent-bit-0.55.0.tgz
Error: INSTALLATION FAILED: YAML parse error on fluent-bit/templates/daemonset.yaml: error converting YAML to JSON: yaml: line 26: mapping values are not allowed in this context

To confirm the issue was related to the most recent change, I then did the following:

  • I then checked out the last commit before your recent suggestion
  • used the helm package command to re-create the helm chart tgz file
  • attempted deploy again setting automountServiceAccountToken to 'false'
    • notice things deployed properly
  • attempted deploy again setting automountServiceAccountToken to 'true'
    • notice things deployed properly
[greg@gemini-m1 fluent-bit]$ git checkout 6a43b17
Note: switching to '6a43b17'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 6a43b17 Merge branch 'main' into podsatoken2

[greg@gemini-m1 fluent-bit]$ helm package .
Successfully packaged chart and saved it to: /home/greg/repos/fluent-helm-charts/charts/fluent-bit/fluent-bit-0.55.0.tgz

[greg@gemini-m1 fluent-bit]$ helm install --set automountServiceAccountToken=false fbtest fluent-bit-0.55.0.tgz
NAME: fbtest
LAST DEPLOYED: Mon Oct 20 20:55:14 2025
NAMESPACE: default
STATUS: deployed
REVISION: 1
NOTES:
Get Fluent Bit build information by running these commands:

export POD_NAME=$(kubectl get pods --namespace default -l "app.kubernetes.io/name=fluent-bit,app.kubernetes.io/instance=fbtest" -o jsonpath="{.items[0].metadata.name}")
kubectl --namespace default port-forward $POD_NAME 2020:2020
curl http://127.0.0.1:2020

[greg@gemini-m1 fluent-bit]$ kubectl get pods
NAME                           READY   STATUS    RESTARTS     AGE
fbtest-fluent-bit-4pv6w        1/1     Running   0            68s
fbtest-fluent-bit-bqpl6        1/1     Running   0            68s
fbtest-fluent-bit-m72g9        1/1     Running   0            68s
fbtest-fluent-bit-zp86m        1/1     Running   0            68s
hello-world-6d54847684-p57gh   1/1     Running   8 (9h ago)   60d
hello-world-6d54847684-smw6p   1/1     Running   8 (9h ago)   60d

[greg@gemini-m1 fluent-bit]$ helm install --set automountServiceAccountToken=true fbtest fluent-bit-0.55.0.tgz
NAME: fbtest
LAST DEPLOYED: Mon Oct 20 21:00:38 2025
NAMESPACE: default
STATUS: deployed
REVISION: 1
NOTES:
Get Fluent Bit build information by running these commands:

export POD_NAME=$(kubectl get pods --namespace default -l "app.kubernetes.io/name=fluent-bit,app.kubernetes.io/instance=fbtest" -o jsonpath="{.items[0].metadata.name}")
kubectl --namespace default port-forward $POD_NAME 2020:2020
curl http://127.0.0.1:2020

Based on all of that, I think I should revert the most recent suggested change. But if these same test-cases are working for you, we'll need to figure out where I'm going wrong or what is different in our two test environments that may be causing these problems.

Regards,
Greg

@gsmith-sas
Copy link
Author

@stevehipwell Have you had a chance to validate (or not) the two use-cases I identified where the proposed change seems to result in errors? I'll be out of office next week but hope we can finalize this PR when I return. Regards, Greg

@stevehipwell
Copy link
Collaborator

@gsmith-sas if you move the auto mount code in the pod template below the service account line and then remove the trailing - it should fix it.

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

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