Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Mar 31, 2021

Description

Fix #17509

  1. When latest or 2019-06-01 is used for Azure Stack /subscription, the REST response doesn't have managedByTenants. However, because the REST spec defines it, the Subscription object returned by SDK will have managed_by_tenants as None. In such case, the behavior of this PR differs from the patch provided in {Core} Quick patch for managed_by_tenants missing from response #17509:

  2. When 2019-03-01-hybrid or 2016-06-01 is used for Azure Stack /subscription, the REST response doesn't have managedByTenants. Because the REST spec doesn't define it, the Subscription object returned by SDK doesn't have managed_by_tenants, so CLI ignores it. This is the same as {Core} Quick patch for managed_by_tenants missing from response #17509.

Additional Context

  1. When SDK object has managed_by_tenants as None, CLI can't distinguish between

    • the REST API doesn't return managedByTenants
    • the REST API returns managedByTenants as null (Luckily, this has never been observed.)
  2. By disabling this check in the gatekeeper az login, CLI may fail even deeper in other commands because the profile / API version is incorrect after all. The handling of unsupported API will be more difficult, but Azure Stack team confirms this is acceptable.

@jiasli jiasli changed the base branch from release to dev March 31, 2021 10:49
@jiasli jiasli changed the title {Core} Hotfix: Remove the check on managed_by_tenants {Core} Remove the check on managed_by_tenants Mar 31, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 31, 2021

Remove check on managed_by_tenants

@yonzhan yonzhan added this to the S185 milestone Mar 31, 2021
Copy link
Collaborator

@yonzhan yonzhan left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +1379 to +1380
if s.managed_by_tenants is None:
s_dict[_MANAGED_BY_TENANTS] = None
Copy link
Member Author

@jiasli jiasli Jan 30, 2023

Choose a reason for hiding this comment

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

Consider following steps:

  1. ARM REST API response omits managedByTenants (or returns "managedByTenants": null, though never observed)
  2. Python SDK will still have managed_by_tenants: None
  3. As a consequence, Azure CLI output will have "managedByTenants": null

This behavior is a breaking change and doesn't comply with swagger as empty array [] becomes null (null is no longer an array), so users assuming managedByTenants to be an array may also face failure.

https://github.com/Azure/azure-rest-api-specs/blob/78ec1b99699a4bf44869bd13f1b0ed7d92a99c27/specification/resources/resource-manager/Microsoft.Resources/stable/2019-11-01/subscriptions.json#L438-L439

        "managedByTenants": {
          "type": "array",
          "items": {
            "$ref": "#/definitions/ManagedByTenant"
          },
          "description": "An array containing the tenants managing the subscription."
        },

Actually, there is no way to perfectly handle all kinds of REST API changes or behaviors in the gray area. In my humble opinion, making API consistent and comply with the swagger is the best and ultimate solution.

In this case,

  1. managedByTenants property MUST exist
  2. managedByTenants MUST be an array, not null

https://learn.microsoft.com/en-us/rest/api/resources/subscriptions/list?tabs=HTTP

image

@jiasli
Copy link
Member Author

jiasli commented Jan 30, 2023

This PR has been shipped in Azure CLI 2.22.0.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants