Skip to content

[Core][Profile] Support lighthouse multi-tenant subscription #11886

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

Merged
jiasli merged 16 commits into from
Feb 12, 2020
Merged

[Core][Profile] Support lighthouse multi-tenant subscription #11886

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
fb46c9c
support lighthouse
jiasli Jan 17, 2020
e9a5e3d
fix linter
jiasli Jan 17, 2020
6853ccb
re-run tests
jiasli Jan 20, 2020
35782af
Merge branch 'dev' into lighthouse
jiasli Jan 21, 2020
22513ee
update urllib3
jiasli Jan 21, 2020
538ad5f
Merge branch 'dev' into lighthouse
jiasli Jan 21, 2020
213a5f9
Merge branch 'dev' into lighthouse
jiasli Feb 4, 2020
c0e8699
Rename subscriptionTenantId to homeTenantId
jiasli Feb 4, 2020
6fff467
Add history
jiasli Feb 4, 2020
d45a50f
clean up
jiasli Feb 4, 2020
2f822f0
Merge remote-tracking branch 'origin/dev' into lighthouse
jiasli Feb 5, 2020
b67e4cf
re-run botservice tests
jiasli Feb 5, 2020
51e7f4a
Merge branch 'dev' into lighthouse
jiasli Feb 5, 2020
35197da
move history to PR description
jiasli Feb 6, 2020
baf6341
Merge branch 'dev' into lighthouse
jiasli Feb 11, 2020
7c61bf3
re-run vm tests
jiasli Feb 11, 2020
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
Next Next commit
support lighthouse
  • Loading branch information
@jiasli
jiasli committed Jan 17, 2020
commit fb46c9c4c28316aabec08b33e4eb99c0dfacc554
36 changes: 33 additions & 3 deletions src/azure-cli-core/azure/cli/core/_profile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
_SUBSCRIPTION_TENANT_ID = 'subscriptionTenantId'
_MANAGED_BY_TENANTS = 'managedByTenants'
_USER_ENTITY = 'user'
_USER_NAME = 'name'
_CLOUD_SHELL_ID = 'cloudShellID'
Expand Down Expand Up @@ -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)

consolidated.append({
subscription_dict = {
_SUBSCRIPTION_ID: s.id.rpartition('/')[2],
_SUBSCRIPTION_NAME: display_name,
_STATE: s.state.value,
Expand All @@ -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:
Copy link
Member

@qianwens qianwens Feb 6, 2020

Choose a reason for hiding this comment

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

_TENANT_LEVEL_ACCOUNT_NAME [](start = 56, length = 26)

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?

Copy link
Member Author

@jiasli jiasli Feb 7, 2020

Choose a reason for hiding this comment

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

If a subscription is a tenant level account, does it mean it only belong to one tenant?

In that case, the account is a tenant with no subscriptions, not "belong to". The naming of subscription_dict is inaccurate. It contains both subscription and tenant accounts.

In this case, the rest api response does not has homeTenentId?

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

def _build_tenant_level_accounts(self, tenants):
result = []
for t in tenants:
s = self._new_account()
s.id = '/subscriptions/' + t
s.subscription = t
s.tenant_id = t
s.display_name = _TENANT_LEVEL_ACCOUNT_NAME
result.append(s)
return result

Why do we need this check here?

It doesn't make sense to include home_tenant_id and managed_by_tenants (whichs are subscription account's attributes) for a tenant account.

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, 'subscription_tenant_id'):
Copy link
Member

@qianwens qianwens Jan 19, 2020

Choose a reason for hiding this comment

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

what is the difference between subscription_tenant_id and tenant_id?

Copy link
Member Author

@jiasli jiasli Jan 19, 2020

Choose a reason for hiding this comment

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

Explained at https://github.com/Azure/azure-cli/pull/11886/files#diff-e35be24f5c5a220ff6948bcd6f18150cR37-R41

Copy link
Member

@yungezz yungezz Jan 22, 2020

Choose a reason for hiding this comment

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

and what's difference between subscription_tenant_id and managed_by_tenants? in the pr comments example, managed_by_tenants is related with tenant_id. az login -t is switching tenant_id. subscription_tenant_id is readonly field?

Copy link
Member Author

@jiasli jiasli Jan 22, 2020

Choose a reason for hiding this comment

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

Good point. I also think these concepts are very confusing. I am following up with Lighthouse and our PMs trying to persuade them to change subscriptionTenantId to homeTenantId which is a read-only field indicating where the subscription is hosted. managedByTenants is a mapping from REST API's managedByTenants.

subscription_dict[_SUBSCRIPTION_TENANT_ID] = s.subscription_tenant_id
if hasattr(s, 'managed_by_tenants'):
Copy link
Member

@qianwens qianwens Jan 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@jiasli jiasli Jan 19, 2020

Choose a reason for hiding this comment

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

managed_by_tenants is only present since API version 2019-06-01. It is not possible to do conditional initialization according to python syntax. https://stackoverflow.com/a/14263905

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
Expand Down Expand Up @@ -855,7 +868,21 @@ def _find_using_common_tenant(self, access_token, resource):
subscriptions = self._find_using_specific_tenant(
tenant_id,
temp_credentials[_ACCESS_TOKEN])
all_subscriptions.extend(subscriptions)
Copy link
Member

@jsntcy jsntcy Feb 11, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@jiasli jiasli Feb 11, 2020

Choose a reason for hiding this comment

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

Yes. A subscription will only appear once since we are keying by subscriptionId. The old behavior is newly listed ones overwriting previously listed ones, while the new behavior is newly listed ones being discarded. Thus, we can make sure subscriptions from user's home tenant is preferred (home tenant is listed earlier).


# 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:
if sub_to_add.subscription_id == sub_to_compare.subscription_id:
logger.warning("Subscription {} '{}' can be accessed from tenants {}(default) and {}. "
"To select a specific tenant when accessing this subscription, "
"please include --tenant in 'az login'."
.format(sub_to_add.subscription_id, sub_to_add.display_name,
sub_to_compare.tenant_id, sub_to_add.tenant_id))
add_sub = False
break
Copy link
Contributor

@arrownj arrownj Feb 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Expand All @@ -867,6 +894,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 subscriptionTenantId
if hasattr(s, "tenant_id"):
setattr(s, 'subscription_tenant_id', s.tenant_id)
setattr(s, 'tenant_id', tenant)
all_subscriptions.append(s)
self.tenants.append(tenant)
Expand Down
2 changes: 1 addition & 1 deletion src/azure-cli-core/azure/cli/core/profiles/_shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def default_api_version(self):
ResourceType.MGMT_RESOURCE_LOCKS: '2016-09-01',
ResourceType.MGMT_RESOURCE_POLICY: '2019-09-01',
ResourceType.MGMT_RESOURCE_RESOURCES: '2019-07-01',
ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2016-06-01',
ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2019-06-01',
ResourceType.MGMT_NETWORK_DNS: '2018-05-01',
ResourceType.MGMT_KEYVAULT: '2018-02-14',
ResourceType.MGMT_AUTHORIZATION: SDKProfile('2018-09-01-preview', {
Expand Down
Loading