[Identity] Remove version check for now, until other services support it#12164
Closed
sophiajt wants to merge 2 commits into
Closed
[Identity] Remove version check for now, until other services support it#12164sophiajt wants to merge 2 commits into
sophiajt wants to merge 2 commits into
Conversation
ghost
added
the
Contributor
|
This PR will fix this instead: #11976 |
Contributor
ghost
pushed a commit
that referenced
this pull request
Nov 4, 2020
#11976) This PR does the following: 1. Simplifies the workflow of ManagedIdentityCredential, fixing #11653 2. Implements caching on the managed credential so that we only verify which MSI is available once per instance of this class. There's no issue for this, but this is necessary to align with .Net 3. Adds Azure Arc support, fixing #10235 4. Adds Azure Fabric support, fixing #10238 ### Simplifies the workflow of ManagedIdentityCredential This is now how we pick what MSI credential is available: ```ts const MSIs = [appServiceMsi2019, appServiceMsi2017, arcMsi, cloudShellMsi, imdsMsi]; for (const msi of MSIs) { if (await msi.isAvailable(this.identityClient, resource, clientId, getTokenOptions)) { this.cachedMSI = msi; return msi; } } ``` Shows the order of verification more clearly. Reduces the complexity of the credential. ### Implements caching on the managed credential The managed credential was verifying the availability of the MSIs on each request - except for the IMDS one, which had a very weird flow with a stateful boolean value that was passed through from method to method. Instead of doing that, the first time we authenticate we define what MSI is available, and subsequent calls won't run any validation. This aligns with .Net. ### Adds Azure Arc support This PR also showcases the refactoring by how simple it is to add a new MSI to the main ManagedIdentityCredential class. The notes on how I was able to write and test the Arc MSI are here: https://gist.github.com/sadasant/888dc7e88543a21ee7061997984dd207 The change on the ManagedIdentityCredential consists of adding `arcMsi` to the array of MSIs used inside of the `cachedAvailableMSI` function. There's an important note on how I'm currently validating this environment here: [link](#11976 (comment)). --- I'll leave this PR as draft until we find the time to do an internal review, then I'll move it out of draft. Fixes #11653 Fixes #10235 **New:** Fixes #11595 Fixes #10238 Closes #12164 Fixes #12058
This pull request was closed.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
We'd like to support more versions for MSI, but this currently isn't supported broadly enough to roll out. We'll use the older API version for now until we get broader support.
Fixes #12058