Enable OAuth-only/Acct Key Lookup on the testing framework for static resources #3275
Conversation
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
adreed-msft
requested review from
dphulkar-msft,
gapra-msft,
seanmcc-msft,
souravgupta-msft,
tanyasethi-msft,
vibhansa-msft and
wonwuakpa-msft
as code owners
adreed-msft
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for OAuth-based authentication as a fallback mechanism for Azure storage accounts in the e2e testing framework. When account keys are unavailable or not provided, the system can now authenticate using OAuth tokens, enabling more flexible test configurations.
- Adds
AvailableAuthTypes()method to account resource managers to query supported authentication types - Implements account key lookup functionality via Azure Resource Manager for static storage accounts
- Refactors OAuth cache to support multi-scope token management with UUID-based tracking
- Updates ARM client architecture to use a ParentSubject pattern for better type safety
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| e2etest/zt_newe2e_basic_functionality_test.go | Extracts source location to variable to avoid duplicate resolution calls |
| e2etest/stress_generators/acct_rm.go | Implements AvailableAuthTypes for OAuth account manager and fixes field naming |
| e2etest/newe2e_task_runazcopy.go | Adds authentication type compatibility checking with fallback logic |
| e2etest/newe2e_resource_managers_file.go | Renames Llocation field to InternalLocation for consistency |
| e2etest/newe2e_resource_manager_mock.go | Adds AvailableAuthTypes stub for mock account manager |
| e2etest/newe2e_resource_manager_interface.go | Adds AvailableAuthTypes to AccountResourceManager interface |
| e2etest/newe2e_resource_manager_azstorage.go | Implements OAuth fallback for client creation and adds AvailableAuthTypes logic |
| e2etest/newe2e_oauth_cache.go | Refactors token caching to support multiple scopes with UUID-based tracking |
| e2etest/newe2e_generic_wrangling.go | Adds list comparison utility functions |
| e2etest/newe2e_config.go | Makes account keys optional and adds account key lookup configuration |
| e2etest/newe2e_arm_test_hooks.go | Updates ARM client setup to support static resources with OAuth |
| e2etest/newe2e_arm_subscription.go | Refactors to use ParentSubject pattern and adds helper methods |
| e2etest/newe2e_arm_storage_account.go | Updates to use ParentSubject pattern |
| e2etest/newe2e_arm_resource_group.go | Updates to use ParentSubject pattern and adds factory methods |
| e2etest/newe2e_arm_requests.go | Moves common ARM request logic to central location |
| e2etest/newe2e_arm_managed_disk.go | Updates to use ParentSubject pattern |
| e2etest/newe2e_arm_interfaces.go | Defines new ARM subject interfaces and ParentSubject type |
| e2etest/newe2e_arm_client.go | Refactors to use new interface pattern and adds factory method |
| e2etest/newe2e_account_registry.go | Implements account key lookup and OAuth fallback during registration |
Comments suppressed due to low confidence (2)
e2etest/newe2e_resource_manager_azstorage.go:1
- Missing error check for the OAuth-based BlobFS client creation. The error
errfrom line 215 is not checked before returning the service manager, unlike the Blob and File cases which havea.NoError(\"Create client\", err)checks.
package e2etest
e2etest/newe2e_arm_requests.go:90
- This definition of err is never used.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func (o *OAuthCache) GetAccessToken(scope string) (*AzCoreAccessToken, error) { | ||
| // at this point, operates on backwards compat | ||
| tok, err := o.GetToken(ctx, policy.TokenRequestOptions{ |
There was a problem hiding this comment.
The variable ctx is used but not defined in this file. It appears to be a package-level variable from helpers.go. Either pass context.Context as a parameter to GetAccessToken, or use context.Background() directly here.
Suggested change
| tok, err := o.GetToken(ctx, policy.TokenRequestOptions{ | |
| tok, err := o.GetToken(context.Background(), policy.TokenRequestOptions{ |
| var client *blobfsservice.Client | ||
| if acct.InternalAccountKey != "" { | ||
| var sharedKey *blobfscommon.SharedKeyCredential | ||
| sharedKey, err = blobfscommon.NewSharedKeyCredential(acct.InternalAccountName, acct.InternalAccountKey) |
There was a problem hiding this comment.
This definition of err is never used.
Suggested change
| sharedKey, err = blobfscommon.NewSharedKeyCredential(acct.InternalAccountName, acct.InternalAccountKey) | |
| sharedKey, err = blobfscommon.NewSharedKeyCredential(acct.InternalAccountName, acct.InternalAccountKey) | |
| a.NoError("Create BlobFS shared key", err) |
| a.NoError("Create BlobFS client", err) | ||
| } else { | ||
| client, err = blobfsservice.NewClient(uri, PrimaryOAuthCache, nil) | ||
| } |
There was a problem hiding this comment.
This definition of err is never used.
Suggested change
| } | |
| a.NoError("Create BlobFS client", err) |
wonwuakpa-msft
approved these changes
Dec 1, 2025
| AccountName: DerefOrDefault(opts.CustomName, "azcopynewe2e") + uuidSegments[len(uuidSegments)-1], | ||
| } | ||
| accountARMClient := CommonARMResourceGroup. | ||
| NewStorageAccountARMClient(DerefOrDefault(opts.CustomName, "azcopynewe2e") + uuidSegments[len(uuidSegments)-1]) |
Member
There was a problem hiding this comment.
nitpic: Are we reducing flexibility by using a factory method to create the ARM client instead of using the struct?
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.
Description
Type of Change
How Has This Been Tested?
Manually. This feature is only enabled for static resources on the testing framework, so it shouldn't negatively affect the core product, or automated runs of the test suite.
Some test breaks should be expected for OAuth-only/fallback mode, since some tests were designed with the use of public access and/or account keys in mind. Lookup mode is available for that.