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.

Sign up for GitHub

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

{Compute} Set audience when creating LogsQueryClient #30787

Merged
merged 1 commit into from
Feb 12, 2025
Merged

{Compute} Set audience when creating LogsQueryClient #30787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/azure-cli/azure/cli/command_modules/vm/_client_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,10 @@ def cf_log_analytics_data_plane(cli_ctx, _):
from azure.monitor.query import LogsQueryClient
from azure.cli.core._profile import Profile
profile = Profile(cli_ctx=cli_ctx)
cred, _, _ = profile.get_login_credentials(
resource=cli_ctx.cloud.endpoints.log_analytics_resource_id)
cred, _, _ = profile.get_login_credentials()
Copy link
Contributor

@zhoxing-ms zhoxing-ms Feb 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I currently lack some domain knowledge about Auth, may I ask why the resource attribute need to be removed for profile.get_login_credentials?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very good question. I wrote a detailed explanation at #29631 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! thanks for the detailed explanation

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)
Copy link
Member Author

Choose a reason for hiding this comment

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

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].

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Shall we still add audience on client creation? Maybe we can just remove resource parameter when calling Profile.get_login_credentials()

Copy link
Member Author

Choose a reason for hiding this comment

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

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/.



def cf_disk_encryption_set(cli_ctx, _):
Expand Down
6 changes: 3 additions & 3 deletions src/azure-cli/azure/cli/command_modules/vm/azure_stack/_client_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,10 @@ def cf_log_analytics_data_plane(cli_ctx, _):
from azure.monitor.query import LogsQueryClient
from azure.cli.core._profile import Profile
profile = Profile(cli_ctx=cli_ctx)
cred, _, _ = profile.get_login_credentials(
resource=cli_ctx.cloud.endpoints.log_analytics_resource_id)
cred, _, _ = profile.get_login_credentials()
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)


def cf_disk_encryption_set(cli_ctx, _):
Expand Down