Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add timeout to IMDS when used in DefaultAzureCredential #1016

Merged
merged 5 commits into from
Nov 29, 2022

Conversation

bmc-msft
Copy link
Contributor

@bmc-msft bmc-msft commented Aug 16, 2022

The DefaultAzureCredential iterates through multiple methods for obtaining a credential until one succeeds (or all fail).

Currently, the default is to check the Environment, IMDS, then the CLI.

In many non-Azure workloads (such as MSFT corp computers), the IMDS token request takes an exceedingly long time to fail. This makes using DefaultAzureCredential without disabling IMDS on corp machines painfully slow.

This PR adds a timeout of 1 second to IMDS, per research of @johnbatty (see below).

The DefaultAzureCredential iterates through multiple methods for obtaining a credential until one succeeds (or all fail).

Currently, the default is to check the Environment, IMDS, then the CLI.

In many non-Azure workloads (such as MSFT corp computers), the IMDS token request takes an exceedingly long time to fail.  This makes using DefaultAzureCredential without disabling IMDS on corp machines painfully slow.

This PR changes the order to Environment, CLI, then IMDS, which is beneficial for developers using the examples.
@bmc-msft bmc-msft marked this pull request as ready for review August 16, 2022 19:12
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I think this makes a lot of sense!

Copy link
Member

@cataggar cataggar left a comment

Choose a reason for hiding this comment

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

I’d like to review this more. The order should match other implementations lie .NET.

@cataggar
Copy link
Member

.NET Azure.Identity.DefaultAzureCredentials has this order:

Java com.azure.identity.DefaultAzureCredential has this order:

It would be nice if all credential options failed fast if unavailable. I think our ImdsManagedIdentityCredential is actually a combination of these two source types in .NET:

There are multiple sources for ManagedIdentity. I think we need to split them up.

        private static ManagedIdentitySource SelectManagedIdentitySource(ManagedIdentityClientOptions options)
        {
            return
                ServiceFabricManagedIdentitySource.TryCreate(options) ??
                AppServiceV2019ManagedIdentitySource.TryCreate(options) ??
                AppServiceV2017ManagedIdentitySource.TryCreate(options) ??
                CloudShellManagedIdentitySource.TryCreate(options) ??
                AzureArcManagedIdentitySource.TryCreate(options) ??
                TokenExchangeManagedIdentitySource.TryCreate(options) ??
                new ImdsManagedIdentitySource(options);
        }

FYI, the difference between the two AppService:

image

The AppServiceManagedIdentitySource validates the environment variables to see if the identity is available & fail fast if not:

        protected static bool TryValidateEnvVars(string msiEndpoint, string secret, out Uri endpointUri)
        {
            endpointUri = null;
            // if BOTH the env vars endpoint and secret values are null, this MSI provider is unavailable.
            // Also validate that IdentityServerThumbprint is null or empty to differentiate from Service Fabric.
            if (string.IsNullOrEmpty(msiEndpoint) || string.IsNullOrEmpty(secret))
            {
                return false;
            }

Perhaps we can make ImdsManagedIdentityCredential opt in if one of the environment variables is set or a combination of.

@yvespp
Copy link
Contributor

yvespp commented Aug 22, 2022

What's the timeout of the IMDS request? Maybe that could be shortened to a few seconds?

@johnbatty
Copy link
Contributor

johnbatty commented Nov 24, 2022

I also just hit this - using DefaultAzureCredential on my local dev machine hangs for over 2 mins (135 secs) attempting to access the IMDS endpoint, before then continuing and successfully getting a token via AzureCliCredential.

A bit of searching shows that the other SDKs seem to have a short default timeout when trying to access IMDS from within DefaultAzureCredential, but NOT if directly using ImdsManagedIdentityCredential:

Seems like we should align with the behaviour of the Go and .NET SDKs here, and add a 1 sec timeout for first connection when used via DefaultAzureCredential, rather than changing the ordering of the credential providers.

@bmc-msft bmc-msft changed the title make checking IMDS for tokens happen last in DefaultAzureCredential add timeout to IMDS when used in DefaultAzureCredential Nov 28, 2022
@bmc-msft
Copy link
Contributor Author

I've updated the PR to use futures-time to add a 1 second timeout to using IMS managed identity token fetch. which aligns with @johnbatty's analysis.

Rather than expanding our dependencies, this uses azure-core's sleep to provide the future used for timing out and lifts the future from futures-time.
@bmc-msft
Copy link
Contributor Author

Originally, the update was implemented using @yoshuawuyts 's futures-time crate, but rather than expanding on our dependencies, this reimplements the Timeout future from said crate and uses azure_core::sleep::Sleep for the timer.

@bmc-msft bmc-msft requested review from cataggar and rylev November 28, 2022 22:40
Copy link
Member

@cataggar cataggar left a comment

Choose a reason for hiding this comment

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

This looks like a good approach. Thank you both.

@rylev rylev merged commit 2a84232 into Azure:main Nov 29, 2022
@bmc-msft bmc-msft deleted the make-managed-identity-last branch November 29, 2022 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants