-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 logging to credentials #12319
Add logging to credentials #12319
Changes from all commits
d26328c
0290c6e
c72c411
0fe67d7
4ff5632
b1ea850
3f4bf1a
9625cb3
9d9c771
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -102,7 +102,6 @@ def __init__(self, **kwargs): | |
) | ||
credentials.append(shared_cache) | ||
except Exception as ex: # pylint:disable=broad-except | ||
# transitive dependency pywin32 doesn't support 3.8 (https://github.com/mhammond/pywin32/issues/1431) | ||
_LOGGER.info("Shared token cache is unavailable: '%s'", ex) | ||
if not exclude_visual_studio_code_credential: | ||
credentials.append(VSCodeCredential()) | ||
|
@@ -124,6 +123,10 @@ def get_token(self, *scopes, **kwargs): | |
`message` attribute listing each authentication attempt and its error message. | ||
""" | ||
if self._successful_credential: | ||
return self._successful_credential.get_token(*scopes, **kwargs) | ||
token = self._successful_credential.get_token(*scopes, **kwargs) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call must succeed, right? Then maybe we can just do _LOGGER.info(
"%s acquired a token from %s", self.__class__.__name__, self._successful_credential.__class__.__name__
)
return self._successful_credential.get_token(*scopes, **kwargs) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call could fail. Maybe the credential requests a new token and something goes wrong. |
||
_LOGGER.info( | ||
"%s acquired a token from %s", self.__class__.__name__, self._successful_credential.__class__.__name__ | ||
) | ||
return token | ||
|
||
return super(DefaultAzureCredential, self).get_token(*scopes, **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,13 @@ | |
# Copyright (c) Microsoft Corporation. | ||
# Licensed under the MIT License. | ||
# ------------------------------------ | ||
import logging | ||
import os | ||
|
||
|
||
from .. import CredentialUnavailableError | ||
from .._constants import EnvironmentVariables | ||
from .._internal.decorators import log_get_token | ||
from .certificate import CertificateCredential | ||
from .client_secret import ClientSecretCredential | ||
from .user_password import UsernamePasswordCredential | ||
|
@@ -22,6 +25,8 @@ | |
|
||
EnvironmentCredentialTypes = Union["CertificateCredential", "ClientSecretCredential", "UsernamePasswordCredential"] | ||
|
||
_LOGGER = logging.getLogger(__name__) | ||
|
||
|
||
class EnvironmentCredential(object): | ||
"""A credential configured by environment variables. | ||
|
@@ -76,6 +81,21 @@ def __init__(self, **kwargs): | |
**kwargs | ||
) | ||
|
||
if self._credential: | ||
_LOGGER.info("Environment is configured for %s", self._credential.__class__.__name__) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a question since I'm not familiar enough with how the credentials are set in python, do you log environment configurations for other credentials that are affected when there are environment variables present? For instance, in managed identity, if client ID is set, that affects the credential. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, in this PR only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we did settle on the fact that this is an important logging scenario, since env vars do impact the request for credentials when they are present. I would say that if possible we should only log them once per total log output. In Go, I have not quite reached that, I log them once per credential that is instantiated. |
||
else: | ||
expected_variables = set( | ||
EnvironmentVariables.CERT_VARS | ||
+ EnvironmentVariables.CLIENT_SECRET_VARS | ||
+ EnvironmentVariables.USERNAME_PASSWORD_VARS | ||
) | ||
set_variables = [v for v in expected_variables if v in os.environ] | ||
if set_variables: | ||
_LOGGER.warning("Incomplete environment configuration. Set variables: %s", ", ".join(set_variables)) | ||
else: | ||
_LOGGER.info("No environment configuration found.") | ||
|
||
@log_get_token("EnvironmentCredential") | ||
def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument | ||
# type: (*str, **Any) -> AccessToken | ||
"""Request an access token for `scopes`. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
# ------------------------------------ | ||
# Copyright (c) Microsoft Corporation. | ||
# Licensed under the MIT License. | ||
# ------------------------------------ | ||
import functools | ||
import logging | ||
|
||
from six import raise_from | ||
from azure.core.exceptions import ClientAuthenticationError | ||
|
||
_LOGGER = logging.getLogger(__name__) | ||
|
||
|
||
def log_get_token(class_name): | ||
"""Adds logging around get_token calls. | ||
|
||
:param str class_name: required for the sake of Python 2.7, which lacks an easy way to get the credential's class | ||
name from the decorated function | ||
""" | ||
|
||
def decorator(fn): | ||
qualified_name = class_name + ".get_token" | ||
|
||
@functools.wraps(fn) | ||
def wrapper(*args, **kwargs): | ||
try: | ||
token = fn(*args, **kwargs) | ||
_LOGGER.info("%s succeeded", qualified_name) | ||
return token | ||
except Exception as ex: | ||
_LOGGER.warning("%s failed: %s", qualified_name, ex, exc_info=_LOGGER.isEnabledFor(logging.DEBUG)) | ||
raise | ||
|
||
return wrapper | ||
|
||
return decorator | ||
|
||
|
||
def wrap_exceptions(fn): | ||
"""Prevents leaking exceptions defined outside azure-core by raising ClientAuthenticationError from them.""" | ||
|
||
@functools.wraps(fn) | ||
def wrapper(*args, **kwargs): | ||
try: | ||
return fn(*args, **kwargs) | ||
except ClientAuthenticationError: | ||
raise | ||
except Exception as ex: # pylint:disable=broad-except | ||
auth_error = ClientAuthenticationError(message="Authentication failed: {}".format(ex)) | ||
raise_from(auth_error, ex) | ||
|
||
return wrapper |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is chained credential, should this be a warning? Or just info?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design guidelines stipulate exceptions be logged as warnings.