AuthParams.WithTenant should copy all AuthorityInfo values #364
Conversation
This file contains 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
chlowell
requested review from
rayluo,
jhendrixMSFT,
bgavrilMS and
keegan-caruso
|
Kudos, SonarCloud Quality Gate passed!
|
bgavrilMS
approved these changes
Dec 12, 2022
bgavrilMS
approved these changes
Dec 12, 2022
jhendrixMSFT
approved these changes
Dec 12, 2022
Merged
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.
This fixes an obscure bug that hasn't shipped yet: a client configured to use a regional endpoint sends requests for non-default tenants to the non-regional endpoint. That is to say, the per-request
WithTenantID()option negates the client's region configuration for that request.This happens because
AuthParams.WithTenant()must update authority URLs when the user specifies a new tenant. That entails constructing a newauthority.Info, whose constructor doesn't set itsRegionfield.AuthParams.WithTenant()thus effectively resetsRegionto its zero value, which means "no region".In this PR I've made the simplest fix and added a test that should prevent this problem from sneaking back in as
Infoexpands.Infoperhaps deserves another look from a design perspective to prevent this kind of thing but I chose not to get into that here because any improvement could require a large refactoring which may not be worth the effort--I don't have another example of the current design introducing risk.