feat(helm): Set automountServiceAccountToken on service accounts
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 +reviewableto 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.ylabels 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.
📝 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.
Comment @coderabbitai help to get the list of available commands and usage tips.
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?
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.
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)?
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.
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 beingtrue, 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.
Closing and reopening to see if CI gets unblocked