feat: support mount SMB Azure File with user-provided OAuth token through k8s secret#3100
Open
andyzhangx wants to merge 67 commits into
Open
feat: support mount SMB Azure File with user-provided OAuth token through k8s secret#3100andyzhangx wants to merge 67 commits into
andyzhangx wants to merge 67 commits into
Conversation
552cc56 to
c2bd547
Compare
4188373 to
18fe595
Compare
18fe595 to
fa39190
Compare
There was a problem hiding this comment.
Pull request overview
Adds support for mounting Azure File shares using a user-provided managed identity OAuth token (via Kubernetes Secret) to enable cross-tenant scenarios, integrating this as a new volume context parameter and wiring it into NodeStageVolume credential-cache setup.
Changes:
- Add
mountwithmanagedidentitytokenvolume context parameter and secret fieldazurestoragemanagedidentitytoken. - Extend credential-cache setup to support passing an OAuth token directly to
azfilesauthmanager set. - Update node staging logic to handle the new MI-token path and enforce mutual exclusivity with existing identity-based options.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/azurefile/utils.go | Extends setCredentialCache to accept either tokenFile-based WI flow or direct token flow; redacts token in logs. |
| pkg/azurefile/utils_test.go | Updates setCredentialCache call signature in tests. |
| pkg/azurefile/nodeserver.go | Adds parsing/validation for new volume context flag and implements setCredentialCacheWithMIToken called during SMB mount. |
| pkg/azurefile/nodeserver_test.go | Updates mutual exclusivity test/error message to include the new flag. |
| pkg/azurefile/azurefile.go | Adds new constants and extends GetStorageAccountFromSecret to return the MI token; recognizes the new mount mode in GetAccountInfo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6f727c1 to
89e66ed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5c07d5e to
927abaf
Compare
- Merge storeAccountKey=false and requiresSmbOAuth into a single conditional block for mountWithManagedIdentity/mountWithWIToken/mountWithOAuthToken - Remove duplicate early secretName validation in NodePublishVolume (already validated in the mountWithOAuthToken block below) - Add mountWithOAuthToken to shouldUseServiceAccountToken function
…to setCredentialCacheWithOAuthToken Move server resolution logic (from volumeContext, volumeID, or secret) and secretName validation into setCredentialCacheWithOAuthToken. The function now returns (server, error) and encapsulates: - secretName validation - server resolution (from context, volumeID parsing, or secret lookup) - oauthTokenSHAMap check (skip refresh if token unchanged) - credential cache refresh This removes duplicate server resolution code and makes both callers (pre-ensureMountPoint and mount path) simpler.
…age in mount path
OAuth token authentication uses a Kubernetes Secret, not a service account token. Including it in shouldUseServiceAccountToken caused NodeStageVolume to skip all validation and return early when serviceAccountToken was empty, making the NFS/secretName/createFolder checks unreachable.
…n NodePublishVolume Validation errors (missing secretName) are caught in NodeStageVolume. By the time NodePublishVolume runs, failures from setCredentialCacheWithOAuthToken are server-side issues (secret fetch, credential cache), not input validation.
…cret fetch - Return codes.InvalidArgument instead of codes.Internal when secretName is missing in volume context (input validation error, not driver error). - Fetch secret once upfront in setCredentialCacheWithOAuthToken and reuse parsed values to avoid duplicate Kubernetes API calls. Addresses copilot review feedback.
setCredentialCacheWithOAuthToken now returns proper gRPC status errors: - codes.InvalidArgument for missing secretName, empty server, missing token - plain errors for system failures (secret fetch, credential cache) The caller propagates the status code from the helper instead of only checking for empty secretName.
No need to propagate gRPC status codes from the internal helper - kubelet treats all mount failures the same regardless of code. Just return codes.Internal with the error message.
Use status.Convert to unwrap the error's code and message cleanly, avoiding nested 'rpc error' in the message while preserving the original status code (e.g. InvalidArgument for validation errors).
Use status.Errorf instead of fmt.Errorf so the error surfaces as codes.Internal rather than codes.Unknown through status.Convert.
Use status.Errorf instead of fmt.Errorf so the error surfaces as codes.Internal rather than codes.Unknown through status.Convert.
- CreateVolume: validate secretName is provided when mountWithOAuthToken is true, fail early with InvalidArgument instead of deferring failure to NodePublish - e2e: use shared oauthSecretName/oauthSecretNamespace constants instead of hardcoded strings in dynamic provisioning test - utils_test: add test case for mutually exclusive token and tokenFile
- CreateVolume: return InvalidArgument when mountWithOAuthToken is combined with NFS protocol, preventing provisioning shares that will fail at mount time - e2e: add trailing slash to IMDS resource URL (https://storage.azure.com/) to avoid invalid_resource rejection in some environments
NFS can be indicated via either protocol=nfs or fsType=nfs. The mountWithOAuthToken guard only checked protocol, so a PV with fsType: nfs could bypass the check. Reject both.
In CAPZ clusters, the OIDC JWKS document is served from Azure Blob Storage and may not be immediately available after cluster creation. When the CSI driver tries to mount with workload identity, it exchanges the SA token for an Azure OAuth token via AAD. If AAD cannot find the signing key in the OIDC issuer's JWKS metadata, it returns AADSTS7000272 / 400 Bad Request and the mount fails. Changes: - Add 120s wait for FIC and RBAC propagation after setup - Add waitForOIDCJWKS() that polls the issuer's JWKS endpoint until it returns at least one signing key (up to 5 min timeout)
…AuthToken Instead of wrapping the error with codes.Internal, return the error directly so that InvalidArgument and other specific status codes from setCredentialCacheWithOAuthToken are preserved.
…essages - Extract mountWithOAuthToken validation to shared validateMountWithOAuthToken() - Log secret name and namespace when using OAuth token - Improve error message: clientID must be provided when tokenFile is set
Member
Author
|
/test |
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add
mountWithOAuthTokenvolume context parameter to support cross-tenant scenarios where users provide their own OAuth token via a Kubernetes Secret.This enables first-party users who cannot use kubelet identity or workload identity (cross-tenant) to mount Azure File shares using an externally-managed OAuth token.
Changes:
mountWithOAuthTokenoauthtokensetCredentialCacheto support direct token passing toazfilesauthmanager setGetStorageAccountFromSecretto return OAuth token from secretsetCredentialCacheWithOAuthToken— reads OAuth token from K8s Secret and updates credential cachemountWithManagedIdentityandmountWithWorkloadIdentityTokensec=krb5mount)NodeStageVolume: performs initial mount with OAuth token credential cache setupNodePublishVolume: refreshes OAuth token credential cache on every publish (even when already mounted), with SHA256-based skip to avoid unnecessaryazfilesauthmanagercalls when the token is unchangedgetSecretNamespacehelper for consistent namespace resolution (secretNamespace → pvcNamespace → "default")secretNamerequired, empty server returnsInvalidArgumentUsage Example
1. Create Secret with OAuth Token
kubectl create secret generic azure-oauth-token-secret --from-literal=oauthtoken="<oauth-token>"2. Dynamic Provisioning (StorageClass)
3. PV (Static Provisioning)
Which issue(s) this PR fixes:
Fixes #3099
Does this PR introduce a user-facing change?