Repository navigation
Conversation
…IAM mode Signed-off-by: Sarvesh Srivastava <sarvesh.srivastava@blackrock.com> Signed-off-by: Sarvesh Srivastava <sarveshsrivastava72@gmail.com>
CI commandsComment any of these to rerun checks on this pull request:
The |
|
Related PRs applying the same credential selection pattern across the ecosystem:
|
|
/assign @sarveshls |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #449 +/- ##
=======================================
Coverage 84.09% 84.10%
=======================================
Files 225 225
Lines 31732 31734 +2
=======================================
+ Hits 26686 26689 +3
+ Misses 3775 3773 -2
- Partials 1271 1272 +1
🚀 New features to boost your workflow:
|
This PR adds a client-secret credential path for Azure IAM auth; the way that path is selected can silently switch to a different identity or break working Workload Identity setups, and the new test does not exercise the new branch. Medium
(raised by xiaocai2333, xaxys)
(raised by xiaocai2333, xaxys)
(raised by xiaocai2333, xaxys) Low
(raised by xaxys) |
|
The SPN support is useful. I suggest the following changes before merging: 1. Preserve the existing Workload Identity priority and use an explicit SPN credential. Currently, the PR checks For backward compatibility, please use WI → SPN → MI. In the SPN branch, validate all three required variables and use For example, the credential-selection block could be: if os.Getenv("AZURE_FEDERATED_TOKEN_FILE") != "" {
// Preserve the existing Workload Identity priority.
cred, credErr = azidentity.NewWorkloadIdentityCredential(
&azidentity.WorkloadIdentityCredentialOptions{
ClientID: os.Getenv("AZURE_CLIENT_ID"),
TenantID: os.Getenv("AZURE_TENANT_ID"),
TokenFilePath: os.Getenv("AZURE_FEDERATED_TOKEN_FILE"),
},
)
} else if secret := os.Getenv("AZURE_CLIENT_SECRET"); secret != "" {
tenantID := os.Getenv("AZURE_TENANT_ID")
clientID := os.Getenv("AZURE_CLIENT_ID")
if tenantID == "" || clientID == "" {
return nil, errors.New(
"SPN authentication requires AZURE_TENANT_ID, AZURE_CLIENT_ID and AZURE_CLIENT_SECRET",
)
}
cred, credErr = azidentity.NewClientSecretCredential(
tenantID, clientID, secret, nil,
)
} else {
opts := &azidentity.ManagedIdentityCredentialOptions{}
if clientID := os.Getenv("AZURE_CLIENT_ID"); clientID != "" {
opts.ID = azidentity.ClientID(clientID)
}
cred, credErr = azidentity.NewManagedIdentityCredential(opts)
}
if credErr != nil {
return nil, credErr
}This priority is a compatibility choice for Woodpecker; Azure's DAC uses Environment credentials first. See the Azure Identity credential chain guidance. 2. Add tests that verify credential selection and configuration errors. Please cover WI, SPN, and MI selection, missing tenant/client IDs in the SPN branch, and WI taking priority when both WI and secret variables are present. The new test only checks client construction and its URL with Please also isolate the relevant Azure environment variables with 3. Document the supported authentication scenarios and selection order. Please add configuration examples explaining when to use each option:
Please state that these paths require |
When
AZURE_CLIENT_SECRETis set,newAzureObjectStorageClientnow selectsazidentity.NewDefaultAzureCredentialbefore trying Workload Identity orManaged Identity. This enables SPN auth using AZURE_CLIENT_ID +
AZURE_CLIENT_SECRET + AZURE_TENANT_ID without requiring a pod identity.
Priority mirrors DefaultAzureCredential's own chain order (EnvironmentCredential first).
Existing paths are unchanged.
A unit test verifies the client is built correctly when the three SPN vars are set.
/kind improvement