feat: support reading service account tokens from CSI secrets field for Kubernetes 1.35+#2945
feat: support reading service account tokens from CSI secrets field for Kubernetes 1.35+#2945aramase wants to merge 1 commit intokubernetes-sigs:masterfrom
Conversation
k8s-ci-robot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aramase The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
@andyzhangx what's the best way to handle this in the helm charts? do you create a new version of chart for every Kubernetes release? for 1.35+, we should have |
There was a problem hiding this comment.
Pull request overview
This PR implements support for reading service account tokens from the CSI secrets field as specified in Kubernetes KEP-5538, enabling forward compatibility with Kubernetes 1.35+ while maintaining backward compatibility with the existing volumeContext approach.
Changes:
- Added
getServiceAccountTokenshelper function to check secrets field first, then fall back to volumeContext - Updated
NodePublishVolumeandNodeStageVolumeto use the new helper function for retrieving service account tokens - Added comprehensive unit tests for the new helper function covering all scenarios including nil maps and priority ordering
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/azurefile/nodeserver.go | Implements the new getServiceAccountTokens helper function and integrates it into NodePublishVolume and NodeStageVolume to support reading service account tokens from the secrets field |
| pkg/azurefile/nodeserver_test.go | Adds unit tests for the getServiceAccountTokens function covering new behavior, backward compatibility, and edge cases with nil maps |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…or Kubernetes 1.35+ Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
aramase
force-pushed
the
aramase/c/fallback_sa_logic
branch
from
80901e9 to
c42356a
Compare
| serviceAccountToken = v | ||
| // Only use reqContext value if not already found in secrets (backward compatibility) | ||
| if serviceAccountToken == "" { | ||
| serviceAccountToken = v |
There was a problem hiding this comment.
is this assignment already covered by L822: serviceAccountToken = getValueInMap(secrets, serviceAccountTokenField)?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, err := d.NodeStageVolume(ctx, &csi.NodeStageVolumeRequest{ | ||
| StagingTargetPath: target, | ||
| VolumeContext: context, | ||
| VolumeCapability: volCap, | ||
| VolumeId: volumeID, | ||
| Secrets: secrets, | ||
| }) |
There was a problem hiding this comment.
NodePublishVolume now forwards req.Secrets to NodeStageVolume only in the service-account-token branch, but the ephemeral volume path still calls NodeStageVolume without Secrets. With Kubernetes 1.35+ token delivery via request.Secrets, this can cause NodeStageVolume to treat the token as missing (and potentially skip staging when clientID is set). Consider passing the same secrets map in the ephemeral NodeStageVolumeRequest as well to keep behavior consistent.
| // Check secrets first for service account token (new behavior in K8s 1.35+) | ||
| serviceAccountToken = getValueInMap(secrets, serviceAccountTokenField) | ||
|
|
There was a problem hiding this comment.
The token-precedence change (prefer Secrets over volumeContext) in GetAccountInfo isn’t covered by existing TestGetAccountInfo cases in pkg/azurefile/azurefile_test.go. Add a test that sets serviceAccountTokenField in secrets and a different value in reqContext to ensure secrets wins, plus a fallback case when secrets doesn’t include the token.
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/kind feature
implementing changes for KEP: kubernetes/enhancements#5538
see https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/5538-csi-sa-tokens-secrets-field/README.md#driver-migration-example for why we're doing this.