feat: support reading service account tokens from CSI secrets field for Kubernetes 1.35+#2305
feat: support reading service account tokens from CSI secrets field for Kubernetes 1.35+#2305aramase wants to merge 1 commit into
Conversation
…or Kubernetes 1.35+ Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
k8s-ci-robot
added
the
kind/feature
|
@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 |
|
[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
size/M
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
andyzhangx
removed
the
lifecycle/stale
There was a problem hiding this comment.
Pull request overview
Adds Kubernetes 1.35+ compatibility for the CSI “serviceAccountTokenInSecrets” migration by preferring service account tokens from the CSI request Secrets field while keeping backward compatibility with VolumeContext.
Changes:
- Add
getServiceAccountTokens()helper to preferNode(Stage|Publish)VolumeRequest.SecretsoverVolumeContext. - Update NodePublishVolume/NodeStageVolume workload-identity gating logic to use the helper.
- Add unit tests covering helper precedence and nil-map behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/blob/nodeserver.go | Reads SA tokens from req.Secrets (preferred) with fallback to VolumeContext, and threads Secrets into an internal NodeStageVolume call. |
| pkg/blob/nodeserver_test.go | Adds unit tests for getServiceAccountTokens() precedence and nil handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| secrets := req.GetSecrets() | ||
|
|
||
| mountPermissions := d.mountPermissions | ||
| context := req.GetVolumeContext() | ||
| serviceAccountTokens := getServiceAccountTokens(secrets, context) |
There was a problem hiding this comment.
NodePublishVolume now uses getServiceAccountTokens(req.Secrets, req.VolumeContext) to decide whether to take the workload-identity path, but the token is not guaranteed to be present in VolumeContext anymore (Kubernetes 1.35+ moves it to Secrets). Unless NodeStageVolume / GetAuthEnv also reads serviceAccountTokenField from Secrets, this will still fail later when the token is parsed from volumeContext only.
| func TestGetServiceAccountTokens(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| secrets map[string]string | ||
| volumeContext map[string]string |
There was a problem hiding this comment.
This test validates the precedence logic of getServiceAccountTokens, but it doesn’t cover the actual workload-identity auth flow when Kubernetes supplies serviceAccountTokenField only via Node(Stage|Publish)VolumeRequest.Secrets (the 1.35+ behavior). Adding a test that exercises NodeStageVolume/GetAuthEnv with the token present only in secrets would catch regressions where the token is still read exclusively from volumeContext.
| volumeMountGroup := req.GetVolumeCapability().GetMount().GetVolumeMountGroup() | ||
| attrib := req.GetVolumeContext() | ||
| secrets := req.GetSecrets() | ||
| serviceAccountTokens := getServiceAccountTokens(secrets, attrib) |
There was a problem hiding this comment.
serviceAccountTokens is computed from req.Secrets, but the token is still consumed downstream via GetAuthEnv/parseServiceAccountToken from attrib (volumeContext) only (see pkg/blob/blob.go switch on serviceAccountTokenField). In Kubernetes 1.35+ with serviceAccountTokenInSecrets, mounts that rely on workload identity will fail unless the secrets token is propagated into the data consumed by GetAuthEnv (either read from secrets there, or inject into the map passed to it).
| serviceAccountTokens := getServiceAccountTokens(secrets, attrib) | |
| serviceAccountTokens := getServiceAccountTokens(secrets, attrib) | |
| if serviceAccountTokens != "" { | |
| attribWithServiceAccountToken := make(map[string]string, len(attrib)+1) | |
| for k, v := range attrib { | |
| attribWithServiceAccountToken[k] = v | |
| } | |
| attribWithServiceAccountToken[serviceAccountTokenField] = serviceAccountTokens | |
| attrib = attribWithServiceAccountToken | |
| } |
|
Should we enable the feature in this PR by setting |
we should! |
Discussed with @andyzhangx - you can add the |
/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.