[azidentity] Making ChainedTokenCredential re-use the first successful credential #16392

sadasant · 2021-12-02T20:18:17Z

This PR makes it so instances of ChainedTokenCredential will now re-use the first successful credential on GetToken calls.

Fixes #16268

Feedback always appreciated!

The purpose of this PR is explained in this or a referenced issue.
The PR does not update generated files.
- These files are managed by the codegen framework at Azure/autorest.go.
Tests are included and/or updated for code changes.
Updates to CHANGELOG.md are included.
MIT license headers are included in each file.

seankane-msft

Small comments

sdk/azidentity/chained_token_credential.go

sdk/azidentity/chained_token_credential_test.go

sdk/azidentity/chained_token_credential.go

Now I'm testing that the right credential is the one that appears as the successfulCredential, and that each credential is only called as many times as expected.

seankane-msft

LGTM, thanks @sadasant

sdk/azidentity/chained_token_credential.go

sdk/azidentity/CHANGELOG.md

sdk/azidentity/chained_token_credential.go

…mproved the tests

sdk/azidentity/CHANGELOG.md

sdk/azidentity/chained_token_credential.go

sdk/azidentity/CHANGELOG.md

sdk/azidentity/chained_token_credential.go

sdk/azidentity/CHANGELOG.md

Co-authored-by: Charles Lowell <[email protected]>

…ential

sdk/azidentity/chained_token_credential_test.go

sadasant · 2021-12-10T19:06:30Z

Is this CI error related to this PR? https://dev.azure.com/azure-sdk/public/_build/results?buildId=1242645&view=logs&j=b70e5e73-bbb6-5567-0939-8415943fadb9&t=bdeefc16-b669-5ebd-ad94-a2c19ade53b0

jhendrixMSFT · 2021-12-10T19:15:11Z

It's due to a recent check that was added in #16265. I will turn this off for now.

jhendrixMSFT · 2021-12-10T20:33:10Z

This has been fixed, please update/rebase to main and you should be good to go.

sdk/azidentity/chained_token_credential.go

chlowell · 2021-12-13T19:38:37Z

sdk/azidentity/chained_token_credential_test.go

+
+func TestChainedTokenCredential_RepeatedGetTokenWithSuccessfulCredential(t *testing.T) {
+	failedCredential := &TestCredential{responses: []TestCredentialResponse{
+		{err: newCredentialUnavailableError("MockCredential", "Mocking a credential unavailable error")},


The unavailableCredential type already in this file does this, so I suggest either using that or replacing it with the more general TestCredential you're adding. Also, the tests you're adding make the existing test which uses unavailableCredential redundant.

I’ll remove the unavailableCredential test 🤔

Updated here: 2080f96

chlowell · 2021-12-13T19:46:38Z

sdk/azidentity/chained_token_credential_test.go

+	if successfulCredential.getTokenCalls != 1 {
+		t.Fatalf("The successful credential getToken should have been called once")
+	}
+	tk2, err2 := cred.GetToken(context.Background(), policy.TokenRequestOptions{Scopes: []string{liveTestScope}})


Suggest a for loop rather than repeating all this code. I might also refactor these tests two functions into subtests, for example.

I don’t think a loop is the best approach for a test that has only two steps. What about abstracting out some of the checks into a function? I’ve pushed that idea here: 2080f96

chlowell · 2021-12-13T19:49:45Z

sdk/azidentity/chained_token_credential_test.go

+// Credential used for testing
+type TestCredential struct {
+	getTokenCalls int
+	responses     []TestCredentialResponse


As used, this always has duplicate entries. Should it simply be a static testCredentialResponse?

Limiting it to just one repeated response makes it way less useful I think 🤔

In principle, sure, but in fact that's the only way it's used 😜

Making GetToken return a static response seems to indicate that developers can alter that static response to change the way GetToken is answering which may end up being worse later.

sdk/azidentity/chained_token_credential_test.go

chlowell · 2021-12-13T21:13:53Z

sdk/azidentity/chained_token_credential.go

+	// When true, the credential will always request a token from each source in turn,
+	// stopping when one provides a token. When false, the credential requests a token
+	// only from the source that previously retrieved a token--it never again tries the sources which failed.
+	RetrySources bool


I doubt we'll find a perfect name for this, but I wonder whether we can come up with a more self-explanatory one. It controls whether GetToken always iterates over the sources, which has me thinking along the lines of AlwaysIterate, AlwaysStartWithFirstSource, or AlwaysStartAtBeginning. It would be nice if this option paired with the (opposite) option in other SDKs, so something like DoNot/PreferFirstSuccessfulSource or Forget/RememberFirstSuccessfulSource, but that's probably too high a bar. I'm just trying to start a brainstorm here.

What about cacheSuccessfulCredential ?

It would be good to name it in such a way that its zero-value (false) is the preferred behavior. Calling it CacheSuccessfulCredential would mean we'd need to set it to true by default. I think RetrySources is reasonable, however we could rename it before release.

Co-authored-by: Charles Lowell <[email protected]>

…e testCredentialResponse and added a re-usable function to test a valid token response

jhendrixMSFT

Let's wait for @chlowell to approve and merge.

sdk/azidentity/chained_token_credential_test.go

chlowell · 2022-01-08T00:59:27Z

sdk/azidentity/chained_token_credential_test.go

+	return response.token, response.err
+}
+
+func testGoodGetTokenResponse(t *testing.T, token *azcore.AccessToken, err error) {


This is almost the same as testGetTokenSuccess in live_test.go

(We talked over teams and we’ll keep this how it is in this PR at the moment)

Co-authored-by: Charles Lowell <[email protected]>

…l credential (#16392) This PR makes it so instances of `ChainedTokenCredential` will now re-use the first successful credential on `GetToken` calls. Fixed #16268

* Make azblob.BlobClient.downloadBlobToWriterAt public (#15899) * azblob.downloadBlobToWriterAt: initialDownloadResponse: remove as unused * azblob.DownloadBlobToWriterAt: make public * Replace the advancer (#16354) * Handle error response XML with whitespace (#16639) * [chunkwriting] Return original buffer to the pool (#16067) When the buffer isn't filled to capacity a new slice is created that's smaller than the original. In that case the smaller slice is returned to the pool which prevents the rest of the capacity from being used again. The solution is to pass the original slice through and attach the length. This allows the original slice to be returned to the pool once the operation is complete. This change also simplifies the `sendChunk` method, ensuring that the buffer is returned to the TransferManager even when no bytes were read from the reader. * UploadStreamToBlockBlob: only require body be io.Reader (#15958) This function only takes an io.Reader in azure-storage-blob-go, & wal-g abstracts over multiple upload mechanisms which all operate on io.Reader * uncomment o.Progress(progress) in getDownloadBlobOptions() (#16727) * Updated azblob README sample code (#16721) * Updated README sample code container name should be all lowercase * Fix typo GetAccountSASToken to GetSASToken I believe 'GetAccountSASToken' was written by mistake. It's inconsistent with what is described on line 97 and I don't see this method in the documentation. * [KeyVault] updating examples to use ClientSecretCredential, fix parsing keyID (#16689) * updating examples to use ClientSecretCredential, fix parsing keyID * fixing constructor test URL * adding test for no keyid * key version, not key ID, better error reporting * changes from working with maor and daniel * working for both hsm and non hsm, need to fix up for recorded tests * fixed implementation, i think... * working for hsm too * working challenge policy for hsm and non hsm * new recordings * adding all final recordings * reveerting back to DefaultAzCred * using streaming package from azcore * Add spell check warnings (#16656) * Add spell check warnings * Basic cspell.json * Ignore files in .vscode except cspell.json, new line before EOF in cspell.json * Spell check, ignore thyself * Adding Smoketests to nightly runs (#16226) * Adding Smoketests to nightly runs * updating location, fixes to script * starting go script * finishing script * updating yml file * formatting * adding snippet for finding go code * adding funcitonality for copying examples * trimming out unused funcs * fixed regexp, thanks benbp * fixing smoke test program to create go.mod file correctly, update powershell for nightly * removing need for argument in go program, updating yml and powershell to reflect * scripts not common * smoketests, plural not singular * finally got the right directory * fixed script locally, running into permissions issue on ci * updating script to exit properly, logging an error instead of panicing * manually set go111module to on * removing references to go111module * issue with duplicated function names... * updating to only pull examples from the service directory if one is provided * runs samples now too! * adding 'go run .' step to ps1, triggering for tables * adding step to analyze.yml file * adding debugging for ci * updating to work in ci * updating to specify go module name, removing print statements * updating scripts to fmt for prettier printing, find all environment variables * working on loading environment variables from file * removing env vars from example_test.go for testing * adding the environment variable portion to the generated main.go file * forgot to remove change to nightly script * adding import to the main file * cleaning up code, adding comments * don't import os if no env vars * small changes for checking all packages * removing _test suffix on copied files * converting to use cobra for better support * formatting * Sync eng/common directory with azure-sdk-tools for PR 2464 (#16730) * Support AAD graph and Microsoft Graph service principal APIs * Consolidate service principal wrapper creation Co-authored-by: Ben Broderick Phillips <[email protected]> * Make EndpointToScope resilient to subdomains (#16737) * Add user assigned managed identity example to docs (#16738) * [KeyVault] fixing broken live test (#16752) * fixing broken live test * coverage * forcing ci * Sync eng/common directory with azure-sdk-tools for PR 2484 (#16753) * Add weekly pipeline generation to prepare-pipelines template * Add succeeded condition to pipeline generation pipelines Co-authored-by: Ben Broderick Phillips <[email protected]> * Release v61.1.0 1641448664 (#16762) * Generated from specification/automation/resource-manager/readme.md tag package-2020-01-13-preview (commit hash: 3b9b0a930e29cbead33df69ae46c7080408e4c0f) * Generated from specification/compute/resource-manager/readme.md tag package-2021-08-01 (commit hash: 3b9b0a930e29cbead33df69ae46c7080408e4c0f) * v61.1.0 * Handle skipping docker build when PushImages is set and there is no dockerfile (#16555) Co-authored-by: Ben Broderick Phillips <[email protected]> * add new config (#16774) * Add offline test for Azure Arc managed identity (#16771) * Rename armmanagedapplications ci.yml files to avoid pipeline name definition collisions (#16770) * Refactor IMDS discovery to remove probing, stop caching failures (#16267) * Sync eng/common directory with azure-sdk-tools for PR 2500 (#16779) * Update pipeline generator tool feed to azure-sdk-for-net * Update pipeline generator tool version Co-authored-by: Ben Broderick Phillips <[email protected]> * feat: add generator cmd to generate or update readme.go.md file for track2 sdk param (#16780) * [azservicebus] Updating to handle breaking changes in azcore (#16776) Updating sb to handle breaking changes in azcore At the moment we're not using azcore's HTTP library, so we don't need to test for specific errors that come from azcore. * [Core] Bump version of internal (#16793)  - [ ] The purpose of this PR is explained in this or a referenced issue. - [ ] The PR does not update generated files. - These files are managed by the codegen framework at [Azure/autorest.go][]. - [ ] Tests are included and/or updated for code changes. - [ ] Updates to [CHANGELOG.md][] are included. - [ ] MIT license headers are included in each file. [Azure/autorest.go]: https://github.com/Azure/autorest.go [CHANGELOG.md]: https://github.com/Azure/azure-sdk-for-go/blob/main/CHANGELOG.md * [Core] Update Changelog.md (#16794) Update changelog for release * Update CHANGELOG.md (#16795) * Increment version for azcore releases (#16798) Increment package version after release of azcore * [azidentity] Making ChainedTokenCredential re-use the first successful credential (#16392) This PR makes it so instances of `ChainedTokenCredential` will now re-use the first successful credential on `GetToken` calls. Fixed #16268 * Align authentication errors with azcore.ResponseError (#16777) * Increment version for azidentity releases (#16799) Increment package version after release of azidentity * [KeyVault] release ready for keyvault keys (#16731) * release ready for keyvault keys * updating the api surface to latest azcore * updating to ResponseError * update with latest codegen * fixing ci * formatting * bumping azcore to v0.21.0 * updating azidentity and autorest version * updating go.sum * final upgrade for changelog Co-authored-by: Joel Hendrix <[email protected]> * [Tables] preparing tables for release (#16733) * preparing tables for release * prepping for release with latest azcore * update with latest code generator * formatting * updating to released azcore version * upgrading azidentity * updating autorest.go version * final changes to readme Co-authored-by: Joel Hendrix <[email protected]> * [KeyVault] prepare secrets for release (#16732) * prepare secrets for release * updating to latest azcore * update with latest codegen * fixing secret version issues * formatting * manual checkout of azkeys * updating to azcore v0.21.0 * updating autorest.go and azidentity * updating Changelog * updating changelog * updating changelog * modifying moduleVersion * undoing changes to keys and tables Co-authored-by: Joel Hendrix <[email protected]> * chore: pump codegen version in scripts (#16802) * Update azblob with the latest azcore (#16784) * Update azblob with the latest azcore This tactially resolves the small number of breaking changes in azcore. * clean-up Co-authored-by: Benoit Perrot <[email protected]> Co-authored-by: Mohit Sharma <[email protected]> Co-authored-by: adreed-msft <[email protected]> Co-authored-by: John Stairs <[email protected]> Co-authored-by: Philip Dubé <[email protected]> Co-authored-by: Brandon Kurtz <[email protected]> Co-authored-by: Kim Ying <[email protected]> Co-authored-by: Sean Kane <[email protected]> Co-authored-by: Daniel Jurek <[email protected]> Co-authored-by: Azure SDK Bot <[email protected]> Co-authored-by: Ben Broderick Phillips <[email protected]> Co-authored-by: Charles Lowell <[email protected]> Co-authored-by: Jiahui Peng <[email protected]> Co-authored-by: Dapeng Zhang <[email protected]> Co-authored-by: Chenjie Shi <[email protected]> Co-authored-by: Richard Park <[email protected]> Co-authored-by: Daniel Rodríguez <[email protected]>

sadasant added 2 commits December 2, 2021 18:50

wip

709222e

tests passing

fa5e663

sadasant added the Azure.Identity label Dec 2, 2021

sadasant requested review from RickWinter and chlowell December 2, 2021 20:18

sadasant self-assigned this Dec 2, 2021

sadasant requested a review from jhendrixMSFT as a code owner December 2, 2021 20:18

last -> first

0438cc4

seankane-msft reviewed Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

sadasant commented Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential_test.go Show resolved Hide resolved

sadasant commented Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential_test.go Show resolved Hide resolved

sadasant commented Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

sadasant commented Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

sadasant added 2 commits December 2, 2021 20:32

no need for an explicit pointer for successfulCredential

3f58cbc

better reference to successfulCredential

d5d1ea7

jhendrixMSFT reviewed Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

sadasant added 3 commits December 2, 2021 20:36

removed unnecessary else

3715c8a

moved formatError to be within the GetToken method

f55385d

Better tests

fa7d460

Now I'm testing that the right credential is the one that appears as the successfulCredential, and that each credential is only called as many times as expected.

seankane-msft approved these changes Dec 2, 2021

View reviewed changes

jhendrixMSFT reviewed Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

jhendrixMSFT reviewed Dec 2, 2021

View reviewed changes

sdk/azidentity/CHANGELOG.md Outdated Show resolved Hide resolved

RetryAllSources and related documentation

cf1ce47

jhendrixMSFT reviewed Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

jhendrixMSFT reviewed Dec 2, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

updated the logic so that successfulCredential is always populated. I…

a1bbdd1

…mproved the tests

sadasant commented Dec 3, 2021

View reviewed changes

sdk/azidentity/CHANGELOG.md Outdated Show resolved Hide resolved

sadasant commented Dec 3, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

Apply suggestions from code review

dab4067

RickWinter reviewed Dec 9, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

sadasant added 2 commits December 9, 2021 20:34

Merge remote-tracking branch 'Azure/main' into azidentity/fix16268

2183d96

RetryAllSources to RetrySources

e288440

chlowell reviewed Dec 9, 2021

View reviewed changes

sadasant and others added 2 commits December 9, 2021 17:20

Apply suggestions from code review

cdd4bb6

Co-authored-by: Charles Lowell <[email protected]>

Feedback from Charles and adding RetrySources to the DefaultAzureCred…

e2d1201

…ential

chlowell reviewed Dec 10, 2021

View reviewed changes

sdk/azidentity/chained_token_credential_test.go Outdated Show resolved Hide resolved

sdk/azidentity/chained_token_credential_test.go Outdated Show resolved Hide resolved

sdk/azidentity/chained_token_credential_test.go Outdated Show resolved Hide resolved

undoing changes to the DefaultAzureCredential

8e51b2c

This comment has been minimized.

Sign in to view

sadasant added 2 commits December 10, 2021 18:39

simplified tests with new test credentials

20197a9

Merge remote-tracking branch 'Azure/main' into azidentity/fix16268

02bb755

chlowell reviewed Dec 10, 2021

View reviewed changes

sdk/azidentity/chained_token_credential.go Outdated Show resolved Hide resolved

sadasant added 2 commits December 10, 2021 22:30

removed the formatError function

e14544a

Avoiding testing that .successfulCredential is set

a19c2f2

chlowell reviewed Dec 13, 2021

View reviewed changes

sadasant and others added 3 commits December 13, 2021 16:41

Apply suggestions from code review

af22a81

Co-authored-by: Charles Lowell <[email protected]>

removed some redundant tests, the unavailableCredential, unexposed th…

2080f96

…e testCredentialResponse and added a re-usable function to test a valid token response

formatting

ae92c66

jhendrixMSFT approved these changes Dec 17, 2021

View reviewed changes

chlowell approved these changes Jan 8, 2022

View reviewed changes

Update sdk/azidentity/chained_token_credential_test.go

0cac965

Co-authored-by: Charles Lowell <[email protected]>

sadasant merged commit 774c062 into Azure:main Jan 11, 2022

sadasant deleted the azidentity/fix16268 branch January 11, 2022 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[azidentity] Making ChainedTokenCredential re-use the first successful credential #16392

[azidentity] Making ChainedTokenCredential re-use the first successful credential #16392

sadasant commented Dec 2, 2021

seankane-msft left a comment

seankane-msft left a comment

This comment has been minimized.

sadasant commented Dec 10, 2021

jhendrixMSFT commented Dec 10, 2021

jhendrixMSFT commented Dec 10, 2021

chlowell Dec 13, 2021

sadasant Dec 13, 2021

sadasant Dec 13, 2021

chlowell Dec 13, 2021

sadasant Dec 13, 2021

chlowell Dec 13, 2021

sadasant Dec 13, 2021

chlowell Dec 13, 2021

sadasant Dec 13, 2021

chlowell Dec 13, 2021

sadasant Dec 13, 2021

jhendrixMSFT Dec 17, 2021

jhendrixMSFT left a comment

chlowell Jan 8, 2022

sadasant Jan 11, 2022

[azidentity] Making ChainedTokenCredential re-use the first successful credential #16392

[azidentity] Making ChainedTokenCredential re-use the first successful credential #16392

Conversation

sadasant commented Dec 2, 2021

seankane-msft left a comment

Choose a reason for hiding this comment

seankane-msft left a comment

Choose a reason for hiding this comment

This comment has been minimized.

sadasant commented Dec 10, 2021

jhendrixMSFT commented Dec 10, 2021

jhendrixMSFT commented Dec 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhendrixMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment