cleanup: clientID is not required when mountWithWorkloadIdentityToken #2271
Conversation
k8s-ci-robot
added
kind/cleanup
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
There was a problem hiding this comment.
Pull request overview
This PR refactors the workload identity authentication logic in the blob driver to make clientID optional when mountWithWorkloadIdentityToken is enabled. The change restructures the control flow so that when workload identity token mounting is requested, the code will fall back to using UserAssignedIdentityID from the cloud config if no explicit clientID is provided, rather than requiring clientID to be explicitly specified.
Key changes:
- The workload identity token mounting logic is now handled first (before the explicit
clientIDcheck), enabling the fallback behavior - When
mountWithWorkloadIdentityTokenis true andclientIDis empty, the code now usesd.cloud.Config.AzureAuthConfig.UserAssignedIdentityIDas a fallback - The explicit
clientIDcheck has been moved below the workload identity block to handle service account token authentication separately
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if mountWithWIToken { | ||
| if clientID == "" { | ||
| clientID = d.cloud.Config.AzureAuthConfig.UserAssignedIdentityID | ||
| } | ||
| klog.V(2).Infof("mountWithWorkloadIdentity is specified, use workload identity auth mount, clientID: %s, tenantID: %s", clientID, tenantID) | ||
|
|
||
| authEnv = append(authEnv, "AZURE_STORAGE_SPN_CLIENT_ID="+clientID) | ||
| if tenantID != "" { | ||
| authEnv = append(authEnv, "AZURE_STORAGE_SPN_TENANT_ID="+tenantID) | ||
| } | ||
| authEnv = append(authEnv, "AZURE_OAUTH_TOKEN_FILE="+azureOAuthTokenFile) | ||
| klog.V(2).Infof("workload identity auth: %v", authEnv) | ||
| workloadIdentityToken, err := parseServiceAccountToken(serviceAccountToken) | ||
| if err != nil { | ||
| return rgName, accountName, accountKey, containerName, authEnv, err | ||
| } | ||
| azureOAuthTokenFile := filepath.Join(defaultAzureOAuthTokenDir, clientID+accountName) | ||
| if err := os.WriteFile(azureOAuthTokenFile, []byte(workloadIdentityToken), 0600); err != nil { | ||
| return rgName, accountName, accountKey, containerName, authEnv, fmt.Errorf("failed to write workload identity token file %s: %v", azureOAuthTokenFile, err) | ||
| } | ||
|
|
||
| authEnv = append(authEnv, "AZURE_STORAGE_SPN_CLIENT_ID="+clientID) | ||
| if tenantID != "" { | ||
| authEnv = append(authEnv, "AZURE_STORAGE_SPN_TENANT_ID="+tenantID) | ||
| } | ||
| authEnv = append(authEnv, "AZURE_OAUTH_TOKEN_FILE="+azureOAuthTokenFile) | ||
| klog.V(2).Infof("workload identity auth: %v", authEnv) | ||
| return rgName, accountName, accountKey, containerName, authEnv, err | ||
| } |
There was a problem hiding this comment.
The new workload identity authentication flow when mountWithWIToken is true lacks test coverage. Consider adding tests to verify the behavior when: 1) clientID is provided, 2) clientID is empty and falls back to UserAssignedIdentityID, and 3) both clientID and UserAssignedIdentityID are empty. This is especially important given that other auth flows in this file have comprehensive test coverage in blob_test.go.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
andyzhangx
force-pushed
the
clientID-not-required
branch
from
b7d5f19 to
d9e0752
Compare
fix log add check
andyzhangx
force-pushed
the
clientID-not-required
branch
from
d9e0752 to
d7231a6
Compare
k8s-ci-robot
added
size/L
2 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
cleanup: clientID is not required when mountWithWorkloadIdentityToken
Which issue(s) this PR fixes:
Fixes #2272
Requirements:
Special notes for your reviewer:
Release note: