[Core][Profile] Support lighthouse multi-tenant subscription #11886
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,12 @@ | |
| _IS_DEFAULT_SUBSCRIPTION = 'isDefault' | ||
| _SUBSCRIPTION_ID = 'id' | ||
| _SUBSCRIPTION_NAME = 'name' | ||
| # Tenant of the token which is used to list the subscription | ||
| _TENANT_ID = 'tenantId' | ||
| # Home tenant of the subscription, which maps to tenantId in 'Subscriptions - List REST API' | ||
| # https://docs.microsoft.com/en-us/rest/api/resources/subscriptions/list | ||
| _HOME_TENANT_ID = 'homeTenantId' | ||
| _MANAGED_BY_TENANTS = 'managedByTenants' | ||
| _USER_ENTITY = 'user' | ||
| _USER_NAME = 'name' | ||
| _CLOUD_SHELL_ID = 'cloudShellID' | ||
|
|
@@ -248,7 +253,7 @@ def _normalize_properties(self, user, subscriptions, is_service_principal, cert_ | |
| except (UnicodeEncodeError, UnicodeDecodeError): # mainly for Python 2.7 with ascii as the default encoding | ||
| display_name = re.sub(r'[^\x00-\x7f]', lambda x: '?', display_name) | ||
|
|
||
| subscription_dict = { | ||
| _SUBSCRIPTION_ID: s.id.rpartition('/')[2], | ||
| _SUBSCRIPTION_NAME: display_name, | ||
| _STATE: s.state.value, | ||
|
|
@@ -259,7 +264,15 @@ def _normalize_properties(self, user, subscriptions, is_service_principal, cert_ | |
| _IS_DEFAULT_SUBSCRIPTION: False, | ||
| _TENANT_ID: s.tenant_id, | ||
| _ENVIRONMENT_NAME: self.cli_ctx.cloud.name | ||
| } | ||
| # for Subscriptions - List REST API 2019-06-01's subscription account | ||
| if subscription_dict[_SUBSCRIPTION_NAME] != _TENANT_LEVEL_ACCOUNT_NAME: | ||
|
Member
There was a problem hiding this comment.
If a subscription is a tenant level account, does it mean it only belong to one tenant? In this case, the rest api response does not has homeTenentId? why do we need this check here?
Member
Author
There was a problem hiding this comment.
In that case, the account is a tenant with no subscriptions, not "belong to". The naming of
This tenant account is not generated from REST, but manually built up: azure-cli/src/azure-cli-core/azure/cli/core/_profile.py Lines 270 to 279 in 944c0c3
It doesn't make sense to include In fact, this is all due to the faulty original design that mixes subscription and tenant in the same list. This will and must be changed in the future to reflect the hierarchy. Tenant is a one-level-higher concept than subscription. |
||
| if hasattr(s, 'home_tenant_id'): | ||
| subscription_dict[_HOME_TENANT_ID] = s.home_tenant_id | ||
| if hasattr(s, 'managed_by_tenants'): | ||
|
Member
There was a problem hiding this comment. The code seems a little complicated here because of the check like if hasattr(s, 'managed_by_tenants'), is it possible that managed_by_tenants attribute does not exist? Can we move code like subscription_dict[_SUBSCRIPTION_TENANT_ID] = s.subscription_tenant_id to the initialization code of subscription_dict = {...} to make code more clean?
Member
Author
There was a problem hiding this comment.
|
||
| subscription_dict[_MANAGED_BY_TENANTS] = [{_TENANT_ID: t.tenant_id} for t in s.managed_by_tenants] | ||
|
|
||
| consolidated.append(subscription_dict) | ||
|
|
||
| if cert_sn_issuer_auth: | ||
| consolidated[-1][_USER_ENTITY][_SERVICE_PRINCIPAL_CERT_SN_ISSUER_AUTH] = True | ||
|
|
@@ -866,7 +879,21 @@ def _find_using_common_tenant(self, access_token, resource): | |
| subscriptions = self._find_using_specific_tenant( | ||
| tenant_id, | ||
| temp_credentials[_ACCESS_TOKEN]) | ||
|
Member
There was a problem hiding this comment. When are all_subscriptions different between old behavior and your new logic? when a subscription exists in multiple tenants? for example, it exists in two tenants, will it only show once in all_subscriptions?
Member
Author
There was a problem hiding this comment. Yes. A subscription will only appear once since we are keying by |
||
|
|
||
| # When a subscription can be listed by multiple tenants, only the first appearance is retained | ||
| for sub_to_add in subscriptions: | ||
| add_sub = True | ||
| for sub_to_compare in all_subscriptions: | ||
Juliehzl marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if sub_to_add.subscription_id == sub_to_compare.subscription_id: | ||
| logger.warning("Subscription %s '%s' can be accessed from tenants %s(default) and %s. " | ||
|
Member
There was a problem hiding this comment. For these new output formats, do we have some spec or context for this new multi-tenant behavior? I ask this because I want to learn if we should only output one subscription while it exists multi tenants or if we should output all of them and mark one of them as default one.
Member
Author
There was a problem hiding this comment. We SHOULD and WILL display them all. But CLI is currently keying on |
||
| "To select a specific tenant when accessing this subscription, " | ||
| "please include --tenant in 'az login'.", | ||
| sub_to_add.subscription_id, sub_to_add.display_name, | ||
| sub_to_compare.tenant_id, sub_to_add.tenant_id) | ||
| add_sub = False | ||
| break | ||
|
Contributor
There was a problem hiding this comment. if there are n subscriptions belong to multi tenants, we will write n lines of such similar warning here ? Can we just use a common message to combine these similar messages ? |
||
| if add_sub: | ||
| all_subscriptions.append(sub_to_add) | ||
|
|
||
| return all_subscriptions | ||
|
|
||
|
|
@@ -878,6 +905,9 @@ def _find_using_specific_tenant(self, tenant, access_token): | |
| subscriptions = client.subscriptions.list() | ||
| all_subscriptions = [] | ||
| for s in subscriptions: | ||
| # map tenantId from REST API to homeTenantId | ||
| if hasattr(s, "tenant_id"): | ||
| setattr(s, 'home_tenant_id', s.tenant_id) | ||
| setattr(s, 'tenant_id', tenant) | ||
| all_subscriptions.append(s) | ||
| self.tenants.append(tenant) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.