Skip to content

Support App Service managed identity 2019-08-01#13053

Merged
chlowell merged 13 commits intoAzure:masterfrom
chlowell:app-service-mi
Aug 17, 2020
Merged

Support App Service managed identity 2019-08-01#13053
chlowell merged 13 commits intoAzure:masterfrom
chlowell:app-service-mi

Conversation

@chlowell
Copy link
Copy Markdown
Member

This adds a new internal credential, AppServiceCredential, handling both versions of App Service managed identity (closes #11346). I did this as a new credential because I think it will be easier to manage the growing list of supported platforms if each has its own credential class. I also added a class for mixing proactive refreshing into credentials. This is only used by the new AppServiceCredential here; refactoring other credentials to use it, or the new ManagedIdentityClient, is work for future PRs.

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Aug 11, 2020
@chlowell chlowell requested a review from xiangyan99 August 11, 2020 23:26
@chlowell chlowell requested a review from schaabs as a code owner August 11, 2020 23:26
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This classname seems a bit misleading since it will not proactively refresh anything unless a request is made. Proactive makes it seem like it will refresh regardless of whether a token was requested.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions welcome 😉

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case that multiple credential share AuthnClient, should _last_request_time information be saved in the AuthnClient?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think not, because refreshing is scoped to credential instances. But it's a thought experiment today because credential instances don't share clients. Also re AuthnClient, it's now being used for managed identity only, and I want to replace it with the ManagedIdentityClient I'm adding here.

@chlowell chlowell requested a review from xiangyan99 August 13, 2020 17:04
Copy link
Copy Markdown
Member

@xiangyan99 xiangyan99 Aug 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have a _parse_app_service_expires_on in authn_client? Do you want to deprecate that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my plan is to delete AuthnClient once everything is using ManagedIdentityClient instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"client_id" works in "2019-08-01" while "clientid" works in "2017-09-01"?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why here we overwrite identity_config but not updating it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is correct, 2017's "clientid" is now "client_id". I overwrite identity_config here to prefer client_id when someone passes client_id and identity_config, which is at best redundant. I'll change it to update identity_config instead. If we make identity_config part of the public API, I think it should be mutually exclusive with client_id, but we can handle that once it's decided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that IDENTITY_ENDPOINT, IDENTITY_HEADER, MSI_ENDPOINT, MSI_SECRET all exist?

And what's the behavior for this case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in fact that's to be expected because App Service instances which support the new version still support the old one for compatibility's sake. Behavior is what you see here: prefer the more recent version, fall back to the prior one if necessary. The fallback behavior is required because the new API isn't supported for all apps, for example Linux Functions apps on a consumption plan don't support it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all of them exist, will we try "2017-09-01" APIs if "2019-08-01" does not work?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand your question but if you're asking whether we will follow a failed request to version 2019-08-01 with a request to 2017-09-01, no, we won't.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. You understand my question correctly. My question was do we want to make it behave like chained credential. if "2019-08-01" cannot get a valid token, we will try "2017-09-01". Thanks for the answer.

Comment thread sdk/identity/azure-identity/azure/identity/_credentials/app_service.py Outdated
@chlowell chlowell merged commit fe980e9 into Azure:master Aug 17, 2020
@chlowell chlowell deleted the app-service-mi branch August 17, 2020 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Identity Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use App Service managed identity version 2019-08-01

3 participants