crossplane icon indicating copy to clipboard operation
crossplane copied to clipboard

feat(helm): Set automountServiceAccountToken on service accounts

Open bradyz7 opened this issue 5 months ago • 7 comments

Description of your changes

It is considered best practice (see: CIS EKS Benchmark 4.1.6) to only mount the service account token when necessary. In the case of the crossplane charts, the API token is needed as there is are RBAC role bindings to the service accounts. Since automountServiceAccountToken defaults to true, there is no functional change - this only adds clarity to the chart.

Fixes

I have:

  • [x] Read and followed Crossplane's contribution process.
  • [x] Run earthly +reviewable to ensure this PR is ready for review.
  • [ ] ~Added or updated unit tests.~
  • [ ] ~Added or updated e2e tests.~
  • [ ] ~Linked a PR or a docs tracking issue to document this change.~
  • [ ] Added backport release-x.y labels to auto-backport this PR.
  • [ ] ~Followed the API promotion workflow if this PR introduces, removes, or promotes an API.~

Need help with this checklist? See the cheat sheet.

bradyz7 avatar Oct 29 '25 17:10 bradyz7

📝 Walkthrough

Walkthrough

Two Crossplane Helm ServiceAccount templates are modified to add automountServiceAccountToken: true, enabling automatic mounting of service account tokens in pods using these service accounts.

Changes

Cohort / File(s) Summary
Crossplane ServiceAccount configurations
cluster/charts/crossplane/templates/rbac-manager-serviceaccount.yaml, cluster/charts/crossplane/templates/serviceaccount.yaml
Added automountServiceAccountToken: true field to both ServiceAccount resource definitions, changing the default behavior to automatically mount the service account token in pods.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Pre-merge checks

✅ Passed checks (4 passed)
Check name Status Explanation
Breaking Changes ✅ Passed The custom check for breaking changes specifies that it "Fails if files under 'apis/' or 'cmd/' remove or rename public fields/flags, add new required public fields/flags, or remove behavior without label 'breaking-change'." The shell script results confirm that the two modified files in this PR are located at cluster/charts/crossplane/templates/rbac-manager-serviceaccount.yaml and cluster/charts/crossplane/templates/serviceaccount.yaml, both of which show "NO MATCH" when checking against the apis/** or cmd/** directory patterns. Since these files fall outside the scope of directories that the breaking changes check monitors, the PR does not trigger the failure condition.
Feature Gate Requirement ✅ Passed This PR explicitly sets automountServiceAccountToken: true on Crossplane service accounts in Helm charts. The default behavior in Kubernetes is for this setting to be true, so this change does not introduce new experimental features or alter behavior—it only makes an existing default explicit for clarity and security compliance. The modification is limited to Helm chart templates and does not affect any code in the apis/** directory, nor does it implement or introduce new experimental functionality that would require feature flags.
Title Check ✅ Passed The pull request title "feat(helm): Set automountServiceAccountToken on service accounts" is 64 characters, which is well under the 72-character limit. The title is highly descriptive and directly reflects the main changes in the pull request: adding the automountServiceAccountToken field to service account templates in the Crossplane Helm charts. It accurately captures the core intent of the changeset and uses the conventional commit format appropriately, making it clear and informative for reviewers.
Description Check ✅ Passed The PR description is directly related to the changeset and provides clear context for the changes. The author explains that the change adds explicit automountServiceAccountToken: true to service accounts in the Helm charts, citing CIS EKS Benchmark 4.1.6 best practices as rationale and noting that Crossplane requires the API token due to RBAC role bindings. The description accurately reflects the modifications made to both the rbac-manager-serviceaccount.yaml and serviceaccount.yaml templates, and appropriately clarifies that this is a non-functional change intended for clarity.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 29 '25 17:10 coderabbitai[bot]

Do we really need these changes, is it a standard now to set this value explicitly in ServiceAccounts?

As its true by default, and IMO its something folks assume that is true by default, does it really help with clarity?

lsviben avatar Dec 05 '25 14:12 lsviben

It is normal for organizations with policy engines like Gatekeeper to require it. Since Gatekeeper cannot distinguish between the defaulting being an intentional setting versus Kubernetes defaulting, it is helpful to show explicit intent to ensure compliance.

bradyz7 avatar Dec 08 '25 21:12 bradyz7

It is normal for organizations with policy engines like Gatekeeper to require it. Since Gatekeeper cannot distinguish between the defaulting being an intentional setting versus Kubernetes defaulting, it is helpful to show explicit intent to ensure compliance.

Ok I see the use case. I guess this change does not hurt and could help someone.

Could we instead of just setting it to false in the template expose this as a option in the helm chart (default true)?

lsviben avatar Dec 10 '25 13:12 lsviben

I attempted to run Crossplane with automountServiceAccountToken: false, but setting that for the service accounts causes containers to fail to start, since it needs access to the API server. Since it wouldn't function properly without the setting being true, I elected not to expose it in the Helm chart.

bradyz7 avatar Dec 10 '25 14:12 bradyz7

I attempted to run Crossplane with automountServiceAccountToken: false, but setting that for the service accounts causes containers to fail to start, since it needs access to the API server. Since it wouldn't function properly without the setting being true, I elected not to expose it in the Helm chart.

Yeah you would need to mount the token yourself to make it work. Maybe adding the option to helm is to nitpicky and we dont really need it until somebody has a use case.

IMO this PR is ok and we can move forward with merging it if a maintainer agrees.

lsviben avatar Dec 12 '25 10:12 lsviben

Closing and reopening to see if CI gets unblocked

phisco avatar Dec 12 '25 11:12 phisco