[Identity] Azure Arc, Fabric and ManagedIdentityCredential refactoring by sadasant · Pull Request #11976 · Azure/azure-sdk-for-js

sadasant · 2020-10-21T21:31:08Z

This PR does the following:

Simplifies the workflow of ManagedIdentityCredential, fixing [Identity] Refactor Managed Identity Credential #11653
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
Adds Azure Arc support, fixing Azure.Identity Support for Managed Identity for Azure Arc Applications #10235
Adds Azure Fabric support, fixing Azure.Identity Support for Managed Identity for Service Fabric Applications #10238

Simplifies the workflow of ManagedIdentityCredential

This is now how we pick what MSI credential is available:

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.

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

sophiajt · 2020-10-23T01:48:49Z

@sadasant - this definitely feels complete enough to move to being a full PR and to pull in folks who have worked on Arc on other languages for their feedback.

Thanks for taking the time to work on the refactor. The code felt readable and I was able to track where this new functionality was being added and could see where it was related to existing functionality.

sadasant · 2020-10-30T22:06:06Z

This PR also fixes #11595. I'm not exactly clear why. I haven't had the time to dig through the previous code to see why. I tested as many times as I could think of with and without these changes and these changes definitely fix #11595.

sophiajt · 2020-11-01T18:59:02Z

This looks good. I commented that we're currently removing the 2019 MSI support because it seems to cause issues with some services (#12058)

cc @schaabs for confirmation.

Other than that, I think this is ready from my perspective.

sophiajt · 2020-11-02T21:28:15Z

    "sinon": "^9.0.2",
-    "@types/sinon": "^9.0.4"
+    "@types/sinon": "^9.0.4",
+    "fs-mock": "~1.2.1"


Called fs-mock here ^

sophiajt

Left some comments, but this looks pretty good

laiapat

Looking really good so far! This is a lot of great work 👍

all wonderful suggestions by McCoy! Co-authored-by: McCoy Patiño <39780829+mccoyp@users.noreply.github.com>

check-enforcer · 2020-11-03T21:37:01Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

laiapat

Service Fabric side looks good! 🎉

chlowell · 2020-11-03T23:51:26Z

+      return this.cachedMSI;
+    }
+
+    const MSIs = [fabricMsi, appServiceMsi2017, cloudShellMsi, arcMsi, imdsMsi];


Looks good to me.

chlowell · 2020-11-03T23:58:43Z

+      // If we received any response, the endpoint is available
+      logger.info(`IMDS endpoint is available`);
+
+      // IMDS MSI available!


Well, the endpoint is available. If it has no assigned identity, it will respond 400 to a token request. Looks like getToken would throw CredentialUnavailable in that case?

If getToken raises a 400 error, it will bubble up to this point: #11807 (comment)

Where we throw CredentialUnavailable!

How does that sound?

I'm asking more information about this through Teams. In any case, the changes related to any retry policy will happen through a separate issue and PR.

catalinaperalta · 2020-11-04T16:50:41Z

+  if (response.status !== 401) {
+    throw new AuthenticationError(
+      response.status,
+      "To authenticate with Azure Arc MSI, status code 401 is expected on the first request."


It might be helpful to users to return some info from the response from the service in the error. I'm wondering if it could have some useful information, for instance something like a malformed request or missing parameter.

I'll do that! thank you!

catalinaperalta

The Arc MSI support looks good to me!

ghost · 2020-11-04T22:23:24Z

Hello @sadasant!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

sadasant added 10 commits October 16, 2020 19:46

wip

26b6f44

last touches before showing this to jonathan

344b39c

comment and filter->find

286bfc4

no more prepareRequest

03fa5c5

async isAvailable and cached MSI

81356c3

Merge remote-tracking branch 'Azure/master' into identity/fix11653

8446f42

formatting

66d68cb

MSIs now have isAvailable and getToken, and a sketch of arcMsi

12b5b04

arcMsi code

ca2aba8

arc MSI is working

361e282

sadasant requested review from daviwil, schaabs and sophiajt October 21, 2020 21:31

sadasant self-assigned this Oct 21, 2020

ghost added the Azure.Identity label Oct 21, 2020

changelog entries

d8e2be0

sadasant commented Oct 21, 2020

View reviewed changes

Comment thread sdk/identity/identity/src/credentials/managedIdentityCredential/arcMsi.ts Outdated

sadasant added 3 commits October 22, 2020 13:23

Merge remote-tracking branch 'Azure/master' into identity/fix11653

c0f881b

fixed the tests, and added a test for the Arc MSI

a36eb3e

small lint fix

e6a45c8

Merge remote-tracking branch 'Azure/master' into identity/fix11653

b0999f9

sadasant mentioned this pull request Oct 30, 2020

DefaultAzureCredential fails on Azure App Service when using User-Assigned Identity #11595

Closed

6 tasks

pnpm lock updated

c2c50fc

sadasant marked this pull request as ready for review October 30, 2020 22:13

sadasant requested a review from bterlson as a code owner October 30, 2020 22:13

lint fix

d981b57

sadasant mentioned this pull request Oct 30, 2020

[Identity] Support for Managed Identity for Service Fabric Applications #12201

Closed

even less precission during the test

88e0907

sophiajt reviewed Nov 1, 2020

View reviewed changes

Comment thread sdk/identity/identity/src/credentials/managedIdentityCredential/appServiceMsi2019.ts Outdated

sadasant added 2 commits November 2, 2020 18:57

Merge remote-tracking branch 'Azure/master' into identity/fix11653

deda082

no more 2019 cred, arc fixes, and the fabric code

3488443

sadasant changed the title ~~[Identity] Azure Arc and ManagedIdentityCredential refactoring~~ [Identity] Azure Arc, Fabric and ManagedIdentityCredential refactoring Nov 2, 2020

sophiajt reviewed Nov 2, 2020

View reviewed changes

Comment thread sdk/identity/identity/test/internal/node/managedIdentityCredential.spec.ts

sophiajt reviewed Nov 2, 2020

View reviewed changes

sophiajt approved these changes Nov 2, 2020

View reviewed changes

sadasant added 2 commits November 2, 2020 21:59

mock-fs

559a2e7

CHANGELOG change

03b297a

sadasant mentioned this pull request Nov 2, 2020

[Identity] Remove version check for now, until other services support it #12164

Closed

sadasant requested review from catalinaperalta and laiapat November 3, 2020 00:07

laiapat reviewed Nov 3, 2020

View reviewed changes

Apply suggestions from code review

9b8e364

all wonderful suggestions by McCoy! Co-authored-by: McCoy Patiño <39780829+mccoyp@users.noreply.github.com>

sadasant added 2 commits November 3, 2020 22:23

arc fix based on recent feedback

378477d

Merge remote-tracking branch 'Azure/master' into identity/fix11653

2bd0ef3

sadasant requested a review from laiapat November 3, 2020 22:56

laiapat approved these changes Nov 3, 2020

View reviewed changes

chlowell reviewed Nov 4, 2020

View reviewed changes

catalinaperalta reviewed Nov 4, 2020

View reviewed changes

catalinaperalta approved these changes Nov 4, 2020

View reviewed changes

sadasant added 2 commits November 4, 2020 22:04

Catalinas feedback

6576924

Merge remote-tracking branch 'Azure/master' into identity/fix11653

ba453d6

sadasant added the auto-merge label Nov 4, 2020

ghost merged commit 3f6f5ed into Azure:master Nov 4, 2020

sadasant deleted the identity/fix11653 branch November 4, 2020 22:53

This pull request was closed.

Conversation

sadasant commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Simplifies the workflow of ManagedIdentityCredential

Implements caching on the managed credential

Adds Azure Arc support

Uh oh!

Uh oh!

sophiajt commented Oct 23, 2020

Uh oh!

sadasant commented Oct 30, 2020

Uh oh!

Uh oh!

sophiajt commented Nov 1, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophiajt left a comment

Choose a reason for hiding this comment

Uh oh!

laiapat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

check-enforcer Bot commented Nov 3, 2020

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

laiapat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catalinaperalta left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 4, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sadasant commented Oct 21, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.