feat: support mount smb file share with workload identity token by andyzhangx · Pull Request #2902 · kubernetes-sigs/azurefile-csi-driver

andyzhangx · 2025-12-17T02:59:34Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
feat: support mount smb file share with workload identity token

Which issue(s) this PR fixes:

Fixes #

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

none

Copilot

Pull request overview

This PR adds support for mounting Azure SMB file shares using workload identity tokens instead of storage account keys. This enhances security by allowing Kubernetes workloads to authenticate using federated identity credentials rather than long-lived secrets.

Introduces mountWithWITokenField parameter to enable workload identity token-based authentication
Extends GetAccountInfo to return tenantID and tokenFilePath for workload identity flows
Adds token parsing, validation, and file-based caching for service account tokens

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 19 comments.

Show a summary per file

File	Description
pkg/azurefile/azurefile.go	Core implementation: adds workload identity token handling in GetAccountInfo, including token parsing, validation, and file-based storage; extends return signature with tenantID and tokenFilePath
pkg/azurefile/utils.go	Updates setCredentialCache to support both managed identity and workload identity modes via azfilesauthmanager; adds isValidTokenFileName helper for token file validation
pkg/azurefile/nodeserver.go	Implements workload identity logic in NodeStageVolume and NodePublishVolume; adds useWorkloadIdentity helper to determine authentication method; configures Kerberos mount options for workload identity
pkg/azurefile/controllerserver.go	Handles mountWithWITokenField parameter in CreateVolume; maps to IsSmbOAuthEnabled flag; updates GetAccountInfo call sites for new signature
pkg/azurefile/azurefile_test.go	Adds test case for invalid mountWithWITokenField value validation
pkg/azurefile/utils_test.go	Updates TestSetCredentialCache for new parameter handling; adds TestIsValidTokenFileName with comprehensive validation cases
pkg/azurefile/nodeserver_test.go	Adds TestUseWorkloadIdentity to verify workload identity detection logic
pkg/azurefile/controllerserver_test.go	Adds test for invalid mountWithWIToken parameter; includes mountWithWITokenField in comprehensive parameter test
pkg/azurefileplugin/Dockerfile	Installs azfilesauth package for Kerberos authentication support via azfilesauthmanager
deploy/csi-azurefile-driver.yaml	Enables requiresRepublish and configures token requests with 1-hour expiration for workload identity
test/e2e/testsuites/*.go	Updates e2e tests for GetAccountInfo signature changes (adds tenantID and tokenFilePath return values)

Comments suppressed due to low confidence (2)

pkg/azurefile/utils_test.go:1398

The TestSetCredentialCache test only covers error cases and doesn't test the successful execution paths. It should include test cases for: (1) successful call with clientID but no token (managed identity mode), and (2) successful call with both clientID and tokenFile (workload identity mode). These tests would verify that the correct command arguments are constructed for each authentication method.

func TestSetCredentialCache(t *testing.T) {
	tests := []struct {
		desc          string
		server        string
		clientID      string
		token         string
		expectedError string
	}{
		{
			desc:          "empty server",
			server:        "",
			clientID:      "test-client-id",
			expectedError: "server must be provided",
		},
		{
			desc:          "empty clientID and token",
			server:        "test.file.core.windows.net",
			clientID:      "",
			token:         "",
			expectedError: "either clientID or tokenFile must be provided",
		},
		{
			desc:          "both empty",
			server:        "",
			clientID:      "",
			expectedError: "server must be provided",
		},
	}

	for _, test := range tests {
		_, err := setCredentialCache(test.server, test.clientID, "", test.token)
		if test.expectedError != "" {
			if err == nil {
				t.Errorf("test[%s]: expected error containing %q, got nil", test.desc, test.expectedError)
			} else if !strings.Contains(err.Error(), test.expectedError) {
				t.Errorf("test[%s]: expected error containing %q, got %v", test.desc, test.expectedError, err)
			}
		}
	}
}

pkg/azurefileplugin/Dockerfile:26

The azfilesauth.deb package is being downloaded from a personal GitHub repository (andyzhangx/demo) instead of an official release or trusted package repository. This creates security and reliability risks: the package could be modified or become unavailable, and there's no verification of its integrity. Consider hosting this package in an official location with proper versioning and checksums, or building it from source in the Dockerfile.

    && curl -Lso /tmp/azfilesauth.deb https://raw.githubusercontent.com/andyzhangx/demo/refs/heads/master/aks/azfilesauth_1.0-8_amd64.deb \

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/azurefile/utils.go

deploy/csi-azurefile-driver.yaml

pkg/azurefile/azurefile.go

pkg/azurefile/utils.go

pkg/azurefile/nodeserver.go

pkg/azurefile/controllerserver.go

pkg/azurefile/nodeserver.go

pkg/azurefile/azurefile.go

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (2)

pkg/azurefile/utils_test.go:1398

The test only covers error cases for setCredentialCache, but doesn't test any success scenarios. Add test cases for: (1) successful call with clientID only (for managed identity), and (2) successful call with tokenFile path (for workload identity). While these might require mocking the command execution, at minimum verify that the function constructs the correct arguments without returning an error when valid inputs are provided.

func TestSetCredentialCache(t *testing.T) {
	tests := []struct {
		desc          string
		server        string
		clientID      string
		token         string
		expectedError string
	}{
		{
			desc:          "empty server",
			server:        "",
			clientID:      "test-client-id",
			expectedError: "server must be provided",
		},
		{
			desc:          "empty clientID and token",
			server:        "test.file.core.windows.net",
			clientID:      "",
			token:         "",
			expectedError: "either clientID or tokenFile must be provided",
		},
		{
			desc:          "both empty",
			server:        "",
			clientID:      "",
			expectedError: "server must be provided",
		},
	}

	for _, test := range tests {
		_, err := setCredentialCache(test.server, test.clientID, "", test.token)
		if test.expectedError != "" {
			if err == nil {
				t.Errorf("test[%s]: expected error containing %q, got nil", test.desc, test.expectedError)
			} else if !strings.Contains(err.Error(), test.expectedError) {
				t.Errorf("test[%s]: expected error containing %q, got %v", test.desc, test.expectedError, err)
			}
		}
	}
}

pkg/azurefileplugin/Dockerfile:26

This Dockerfile downloads a .deb package from a personal GitHub repository (andyzhangx/demo) rather than from an official or verified source. This poses security and reliability risks: the package could be modified, deleted, or become unavailable, causing build failures in production. Consider hosting this package in an official repository, using a checksum verification, or at minimum using a specific commit hash rather than the branch reference "refs/heads/master" to ensure build reproducibility.

    && curl -Lso /tmp/azfilesauth.deb https://raw.githubusercontent.com/andyzhangx/demo/refs/heads/master/aks/azfilesauth_1.0-8_amd64.deb \

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/azurefileplugin/Dockerfile

deploy/csi-azurefile-driver.yaml

pkg/azurefile/azurefile.go

deploy/csi-azurefile-driver.yaml

pkg/azurefile/utils.go

andyzhangx · 2025-12-17T05:12:11Z

/retest

pkg/azurefile/azurefile.go

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/azurefile/nodeserver.go

pkg/azurefile/azurefile_test.go

Copilot · 2025-12-25T07:52:48Z

@andyzhangx I've opened a new pull request, #2919, to work on those changes. Once the pull request is ready, I'll request review from you.

fix workload idenitty token setting getToken from service account token feat: use new azfilesauth version fix fix ut add check add token file name check fix refresh WI token set expirationSeconds: 3600 fix arm64 image build break

fix

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

pkg/azurefile/azurefile.go

pkg/azurefile/nodeserver.go

deploy/csi-azurefile-driver.yaml

pkg/azurefile/azurefile.go

pkg/azurefile/controllerserver.go

pkg/azurefile/azurefile.go

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-11T15:23:51Z

pkg/azurefile/nodeserver.go

 	return false
 }
+
+// shouldUseServiceAccountToken determines whether a service account token


The function documentation is incomplete. It should fully describe what the function does. The comment should be: "shouldUseServiceAccountToken determines whether a service account token should be used for authentication based on the volume context attributes."

Suggested change

// shouldUseServiceAccountToken determines whether a service account token

// shouldUseServiceAccountToken determines whether a service account token should be used for authentication based on the volume context attributes.

fix fix: copilot comments fix

mittachaitu

Are we going to add e2e test in different PR?

k8s-ci-robot · 2026-01-13T07:34:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, mittachaitu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [andyzhangx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mittachaitu · 2026-01-13T07:36:56Z

Can we add README on how to use WorkloadIdentity?

andyzhangx · 2026-02-04T01:21:21Z

Are we going to add e2e test in different PR?

@mittachaitu can you explore how to add such e2e test in aks RP?

andyzhangx requested a review from Copilot December 17, 2025 02:59

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 17, 2025

k8s-ci-robot requested review from cvvz and gnufied December 17, 2025 02:59

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 17, 2025

Copilot started reviewing on behalf of andyzhangx December 17, 2025 03:00 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

andyzhangx requested a review from Copilot December 17, 2025 03:51

Copilot started reviewing on behalf of andyzhangx December 17, 2025 03:51 View session

Copilot AI reviewed Dec 17, 2025

View reviewed changes

mittachaitu reviewed Dec 19, 2025

View reviewed changes

pkg/azurefile/azurefile.go Show resolved Hide resolved

andyzhangx force-pushed the wi-token branch from 39c9939 to 8823fee Compare December 21, 2025 08:33

andyzhangx changed the title ~~[WIP] feat: support mount smb file share with workload identity token~~ feat: support mount smb file share with workload identity token Dec 21, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2025

andyzhangx requested a review from Copilot December 24, 2025 15:31

Copilot started reviewing on behalf of andyzhangx December 24, 2025 15:32 View session

Copilot AI reviewed Dec 24, 2025

View reviewed changes

pkg/azurefile/nodeserver.go Outdated Show resolved Hide resolved

pkg/azurefile/azurefile_test.go Show resolved Hide resolved

Copilot AI mentioned this pull request Dec 25, 2025

Add positive test case for workload identity token flow in TestGetAccountInfo #2919

Closed

4 tasks

andyzhangx added 2 commits December 25, 2025 13:31

revert Dockerfile change

c5bc1b1

fix

andyzhangx force-pushed the wi-token branch from 8823fee to 953ae54 Compare December 25, 2025 13:31

andyzhangx requested a review from Copilot December 25, 2025 13:32

Copilot started reviewing on behalf of andyzhangx December 25, 2025 13:33 View session

Copilot AI reviewed Dec 25, 2025

View reviewed changes

andyzhangx requested a review from Copilot January 11, 2026 15:14

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 11, 2026

Copilot started reviewing on behalf of andyzhangx January 11, 2026 15:15 View session

Copilot AI reviewed Jan 11, 2026

View reviewed changes

fix copilot comments

be9306a

fix fix: copilot comments fix

andyzhangx force-pushed the wi-token branch from 6a80b63 to be9306a Compare January 12, 2026 02:51

mittachaitu approved these changes Jan 13, 2026

View reviewed changes

andyzhangx merged commit 4ce659a into master Feb 4, 2026
27 of 28 checks passed

RomanBednar mentioned this pull request Feb 16, 2026

STOR-2761: Rebase to upstream v1.35.0 for OCP 4.22 openshift/azure-file-csi-driver#116

Merged

	// shouldUseServiceAccountToken determines whether a service account token
	// shouldUseServiceAccountToken determines whether a service account token should be used for authentication based on the volume context attributes.

Conversation

andyzhangx commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyzhangx commented Dec 17, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

Uh oh!

mittachaitu left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 13, 2026

Uh oh!

mittachaitu commented Jan 13, 2026

Uh oh!

Uh oh!

andyzhangx commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andyzhangx commented Feb 4, 2026 •

edited

Loading