Skip to content

{Core} az login: Capture managed_by_tenants error#15550

Merged
jiasli merged 7 commits intoAzure:devfrom
jiasli:login-stack
Nov 4, 2020
Merged

{Core} az login: Capture managed_by_tenants error#15550
jiasli merged 7 commits intoAzure:devfrom
jiasli:login-stack

Conversation

@jiasli
Copy link
Copy Markdown
Member

@jiasli jiasli commented Oct 16, 2020 •
edited
Loading

Fix #14969

Description

If profile latest is used for Azure Stack cloud, az login will fail:

> az cloud register -n redmond --endpoint-resource-manager "https://management.redmond.azurestack.corp.microsoft.com/" 
> az cloud set -n redmond
> az login
...
  File "d:\cli\azure-cli\src\azure-cli-core\azure\cli\core\_profile.py", line 276, in _normalize_properties
    subscription_dict[_MANAGED_BY_TENANTS] = [{_TENANT_ID: t.tenant_id} for t in s.managed_by_tenants]
TypeError: 'NoneType' object is not iterable

This is because latest should not be accepted by the service, and the service should return Not Supported. On the contrary, the service does accept the request and returns an invalid JSON which lacks managedByTenants. This behavior doesn't comply with the spec (Azure/azure-rest-api-specs#9567), causing the parsing logic of CLI to fail.

Ideally, the user should run az cloud set/update -n redmond --profile 2019-03-01-hybrid to set the profile for the cloud before az login.

This PR wraps the raw exception with better instructions:

UserFault: Invalid profile is used for cloud 'redmond'. To configure the cloud profile, 
run `az cloud set --name redmond --profile <profile>(e.g. 2019-03-01-hybrid)`. For more 
information about using Azure CLI with Azure Stack, see https://docs.microsoft.com/en-us
/azure/azure-stack/user/azure-stack-version-profiles-azurecli2#connect-to-azure-stack

@jiasli jiasli requested a review from houk-ms October 16, 2020 03:15
houk-ms
houk-ms reviewed Oct 16, 2020
View reviewed changes
src/azure-cli-core/azure/cli/core/_profile.py Outdated
Comment on lines +280 to +281
from azure.cli.core.azclierror import UserFault
raise UserFault("Invalid profile is used for cloud '{cloud_name}'. "
Copy link
Copy Markdown
Contributor

@houk-ms houk-ms Oct 16, 2020

Choose a reason for hiding this comment

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

Can we define an error sub-class may be InvalidProfileError(UserFault) and raise the specific error? UserFault is designed as an abstract class and is not recommended to be raised directly.

Copy link
Copy Markdown
Member Author

@jiasli jiasli Oct 16, 2020

Choose a reason for hiding this comment

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

This is only a temporary mitigation to the service bug, it should actually be an Unsupported API version error.

Copy link
Copy Markdown
Contributor

@arrownj arrownj Oct 23, 2020

Choose a reason for hiding this comment

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

@houk-ms , you can use ABC and abstractmethod (from abc import ABC, abstractmethod) to define your class and make the parent class can't be instantiated.

Copy link
Copy Markdown
Contributor

@houk-ms houk-ms Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@arrownj sure, we can do that. Maybe in next CLI release

Copy link
Copy Markdown
Contributor

@houk-ms houk-ms Oct 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@jiasli In this case, cosider using from azure.cli.core.azclierror import UnknownError to replace UserFault if you don't define a new error type

Copy link
Copy Markdown
Member Author

@jiasli jiasli Oct 23, 2020

Choose a reason for hiding this comment

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

Let's use CLIError for now until the error can be classified as UnsupportedAPIError after the service is fixed.

@yonzhan yonzhan added this to the S177 milestone Oct 16, 2020
@yonzhan
Copy link
Copy Markdown
Collaborator

yonzhan commented Oct 16, 2020

Core

@jiasli jiasli changed the title {Core} az login: Wrap managed_by_tenants error {Core} az login: Capture managed_by_tenants error Oct 23, 2020
fengzhou-msft
fengzhou-msft reviewed Oct 23, 2020
View reviewed changes
jiasli
jiasli commented Oct 23, 2020
View reviewed changes
@yonzhan yonzhan modified the milestones: S177, S178 Oct 24, 2020
@yonzhan yonzhan requested a review from evelyn-ys October 24, 2020 15:06
@jiasli
Copy link
Copy Markdown
Member Author

jiasli commented Oct 27, 2020

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Oct 27, 2020

Azure Pipelines successfully started running 2 pipeline(s).

@jiasli jiasli merged commit 24fe6f6 into Azure:dev Nov 4, 2020
@jiasli jiasli deleted the login-stack branch November 4, 2020 05:16
This was referenced Mar 26, 2021
Closed
Closed
Closed
Closed
Closed
@egodigitus egodigitus mentioned this pull request Mar 30, 2021
Closed
@dhmoto17 dhmoto17 mentioned this pull request Mar 30, 2021
Closed
@jiasli
Copy link
Copy Markdown
Member Author

jiasli commented Jan 31, 2023

This PR has been shipped in Azure CLI 2.15.0.

@jiasli jiasli mentioned this pull request Jun 12, 2025
Draft
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Juliehzl Juliehzl Juliehzl approved these changes

@haroldrandom haroldrandom Awaiting requested review from haroldrandom

@jsntcy jsntcy Awaiting requested review from jsntcy

@mmyyrroonn mmyyrroonn Awaiting requested review from mmyyrroonn

@evelyn-ys evelyn-ys Awaiting requested review from evelyn-ys

@dbradish-microsoft dbradish-microsoft Awaiting requested review from dbradish-microsoft

@chenlomis chenlomis Awaiting requested review from chenlomis

+3 more reviewers

@arrownj arrownj arrownj approved these changes

@houk-ms houk-ms houk-ms left review comments

@fengzhou-msft fengzhou-msft fengzhou-msft approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jiasli jiasli

Labels

None yet

Projects

None yet

Milestone

S178

Development

Successfully merging this pull request may close these issues.

az login into the AzureStackUser cloud fails

6 participants

@jiasli @yonzhan @arrownj @Juliehzl @fengzhou-msft @houk-ms