{Compute} Set audience when creating LogsQueryClient
#30787
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
jiasli
changed the title
audience when creating LogsQueryClientaudience when creating LogsQueryClient
| api_version = 'v1' | ||
| return LogsQueryClient(cred, endpoint=cli_ctx.cloud.endpoints.log_analytics_resource_id + '/' + api_version) | ||
| return LogsQueryClient(cred, endpoint=cli_ctx.cloud.endpoints.log_analytics_resource_id + '/' + api_version, | ||
| audience=cli_ctx.cloud.endpoints.log_analytics_resource_id) |
There was a problem hiding this comment.
The naming of audience doesn't align with Track 2 mgmt SDKs where its equivalent is called credential_scopes.
Also, according to azure.monitor.query._helpers.get_authentication_policy
def get_authentication_policy(credential: TokenCredential, audience: str) -> BearerTokenCredentialPolicy:
"""Returns the correct authentication policy"""
if credential is None:
raise ValueError("Parameter 'credential' must not be None.")
scope = audience.rstrip("/") + "/.default"the value of scopes is a str without the /.default suffix, unlike credential_scopes's value which is a list[str].
| api_version = 'v1' | ||
| return LogsQueryClient(cred, endpoint=cli_ctx.cloud.endpoints.log_analytics_resource_id + '/' + api_version) | ||
| return LogsQueryClient(cred, endpoint=cli_ctx.cloud.endpoints.log_analytics_resource_id + '/' + api_version, | ||
| audience=cli_ctx.cloud.endpoints.log_analytics_resource_id) |
There was a problem hiding this comment.
When audience is not provided, it falls back to the domain of the URL according to azure.monitor.query._logs_query_client.LogsQueryClient.__init__
def __init__(self, credential: TokenCredential, **kwargs: Any) -> None:
endpoint = kwargs.pop("endpoint", "https://api.loganalytics.io/v1")
if not endpoint.startswith("https://") and not endpoint.startswith("http://"):
endpoint = "https://" + endpoint
parsed_endpoint = urlparse(endpoint)
audience = kwargs.pop("audience", f"{parsed_endpoint.scheme}://{parsed_endpoint.netloc}")
self._endpoint = endpoint
auth_policy = kwargs.pop("authentication_policy", None)
self._client = MonitorQueryClient(
credential=credential,
authentication_policy=auth_policy or get_authentication_policy(credential, audience),So passing audience is not really necessary.
There was a problem hiding this comment.
Shall we still add audience on client creation? Maybe we can just remove resource parameter when calling Profile.get_login_credentials()
There was a problem hiding this comment.
Setting audience may not be necessary, but it will be necessary if the URL's domain is not the same as the audience in certain clouds.
For example, for Dogfood cloud, ARM's resource ID is https://management.core.windows.net/, but its domain is https://api-dogfood.resources.windows-int.net/.
| cred, _, _ = profile.get_login_credentials( | ||
| resource=cli_ctx.cloud.endpoints.log_analytics_resource_id) | ||
| cred, _, _ = profile.get_login_credentials() |
There was a problem hiding this comment.
I currently lack some domain knowledge about Auth, may I ask why the resource attribute need to be removed for profile.get_login_credentials?
There was a problem hiding this comment.
This is a very good question. I wrote a detailed explanation at #29631 (comment).
There was a problem hiding this comment.
Great! thanks for the detailed explanation
jiasli
requested review from
jsntcy,
wangzelin007 and
yanzhudd
as code owners
Related command
Description
The migration to
azure-monitor-querySDK (#28199) is incomplete - it doesn't setaudiencewhen creatingLogsQueryClient.