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

Reduce redundant logging during ChainedTokenCredential.get_token() #19283

Merged
merged 3 commits into from
Jul 20, 2021

Conversation

chlowell
Copy link
Member

@chlowell chlowell commented Jun 16, 2021

This PR closes #18972 by reducing redundant logging during ChainedTokenCredential.get_token() and, by inheritance, DefaultAzureCredential.get_token(). The rationale is that when get_token() fails for a credential chain, it logs an aggregate of the inner credentials' error messages, making those credentials' logging of same redundant. Similarly, when get_token() succeeds, users will seldom find error messages from unavailable inner credentials interesting. On the contrary, those messages can be confusing: "Why do I see half a dozen error messages when authentication succeeds?"

Here's the diff for DefaultAzureCredential failure given typical logging configuration (filtering DEBUG messages):

[INFO] No environment configuration found.
[INFO] ManagedIdentityCredential will use IMDS
-[WARNING] EnvironmentCredential.get_token failed: EnvironmentCredential authentication unavailable. Environment variables are not fully configured.
-[INFO] DefaultAzureCredential - EnvironmentCredential is unavailable
[INFO] No response from the IMDS endpoint.
-[WARNING] ImdsCredential.get_token failed: ManagedIdentityCredential authentication unavailable, no managed identity endpoint found.
-[WARNING] ManagedIdentityCredential.get_token failed: ManagedIdentityCredential authentication unavailable, no managed identity endpoint found.
-[INFO] DefaultAzureCredential - ManagedIdentityCredential is unavailable
-[WARNING] SharedTokenCacheCredential.get_token failed: SharedTokenCacheCredential authentication unavailable. No accounts were found in the cache.
-[INFO] DefaultAzureCredential - SharedTokenCacheCredential is unavailable
-[WARNING] VisualStudioCodeCredential.get_token failed: Failed to get Azure user details from Visual Studio Code.
-[INFO] DefaultAzureCredential - VisualStudioCodeCredential is unavailable
-[WARNING] AzureCliCredential.get_token failed: Please run 'az login' to set up an account
-[INFO] DefaultAzureCredential - AzureCliCredential is unavailable
-[WARNING] AzurePowerShellCredential.get_token failed: Please run "Connect-AzAccount" to set up account
-[INFO] DefaultAzureCredential - AzurePowerShellCredential is unavailable
[WARNING] DefaultAzureCredential failed to retrieve a token from the included credentials.
Attempted credentials:
        EnvironmentCredential: EnvironmentCredential authentication unavailable. Environment variables are not fully configured.
        ManagedIdentityCredential: ManagedIdentityCredential authentication unavailable, no managed identity endpoint found.
        SharedTokenCacheCredential: SharedTokenCacheCredential authentication unavailable. No accounts were found in the cache.
        VisualStudioCodeCredential: Failed to get Azure user details from Visual Studio Code.
        AzureCliCredential: Please run 'az login' to set up an account
        AzurePowerShellCredential: Please run "Connect-AzAccount" to set up account

Similarly, DefaultAzureCredential success:

[INFO] No environment configuration found.
[INFO] ManagedIdentityCredential will use IMDS
-[WARNING] EnvironmentCredential.get_token failed: EnvironmentCredential authentication unavailable. Environment variables are not fully configured.
-[INFO] DefaultAzureCredential - EnvironmentCredential is unavailable
[INFO] No response from the IMDS endpoint.
-[WARNING] ImdsCredential.get_token failed: ManagedIdentityCredential authentication unavailable, no managed identity endpoint found.
-[WARNING] ManagedIdentityCredential.get_token failed: ManagedIdentityCredential authentication unavailable, no managed identity endpoint found.
-[INFO] DefaultAzureCredential - ManagedIdentityCredential is unavailable
-[WARNING] SharedTokenCacheCredential.get_token failed: SharedTokenCacheCredential authentication unavailable. No accounts were found in the cache.
-[INFO] DefaultAzureCredential - SharedTokenCacheCredential is unavailable
-[INFO] DefaultAzureCredential - VisualStudioCodeCredential is unavailable
-[WARNING] AzureCliCredential.get_token failed: Please run 'az login' to set up an account
-[INFO] DefaultAzureCredential - AzureCliCredential is unavailable
-[INFO] AzurePowerShellCredential.get_token succeeded
[INFO] DefaultAzureCredential acquired a token from AzurePowerShellCredential

Inner credentials still log these messages, however they do so at DEBUG level when they're invoked by ChainedTokenCredential, using a ContextVar to determine whether that's the case. Unfortunately, ContextVar was added in Python 3.7. For Python 3.6, we have a backport. The nearest equivalent available to 2.7 is threading.local, which isn't concurrency-safe. Another alternative that works everywhere is to pass around an internal keyword argument, but 🤮

In the end, because this isn't a functional or API change, I don't mind excluding 2.7. Similarly, if the backport is unacceptable, I observe that 3.6 will be EOL in 6 months. But that's just my opinion, please feel free to change my mind or propose alternative solutions 😄

@chlowell chlowell force-pushed the chain-contextvar branch from 469f2df to 8c6be31 Compare July 8, 2021 20:46
@chlowell chlowell marked this pull request as ready for review July 8, 2021 20:46
@chlowell chlowell requested a review from schaabs as a code owner July 8, 2021 20:46
Copy link
Member

@mccoyp mccoyp 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 is a reasonable solution -- at least, I can't think of a better one. I feel bad excluding 2.7, but I don't think the necessary work to support it would be justifiable when this is a cosmetic change, albeit a really nice one.

I do wonder if it would be good to include a note in the changelog that these changes are available on 3.6 if you pip install the backport

@chlowell
Copy link
Member Author

It turns out the backport doesn't work correctly in all cases and likely never will given the impending 3.6 EOL (see MagicStack/contextvars/issues/2). I'll stick with 3.7+ here and recommend upgrading Python to anyone running 3.6 who really wants this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DefaultAzureCredential prints get_token failed even when it succeeds
2 participants