[Core][Profile] Support lighthouse multi-tenant subscription by jiasli · Pull Request #11886 · Azure/azure-cli

jiasli · 2020-01-17T06:52:50Z

History Notes:

[Profile] Preview: Add new attributes homeTenantId and managedByTenants to subscription accounts. Please re-run az login for the changes to take effect
[Profile] az login: Show a warning when a subscription is listed from more than one tenants and default to the first one. To select a specific tenant when accessing this subscription, please include --tenant in az login

Update on 2020-02-04

Rename:

subscriptionTenantId    > homeTenantId
subscription_tenant_id  > home_tenant_id
_SUBSCRIPTION_TENANT_ID > _HOME_TENANT_ID

This PR does these tasks:

Bump resource subscription client to API version 2019-06-01
Display homeTenantId and managedByTenants for subscription accounts while not changing the fields of bare tenant accounts generated by specifying --allow-no-subscriptions
Show a warning when a subscription is listed from more than one tenants
Add corresponding tests for both 2016-06-01 API and 2019-06-01 API

To test:

Run az login which will show a warning when a subscription is listed from more than one tenants:

Subscription 9535 'Visual Studio Enterprise' can be accessed from tenants 72f9(default) and 2b8e. To select a specific tenant when accessing this subscription, please include --tenant in 'az login'.

In the response homeTenantId and managedByTenants are added. homeTenantId maps to tenantId of Subscriptions - List REST API (GUID is trimmed for simplicity):

{
    "cloudName": "AzureCloud",
    "id": "9535",
    "isDefault": false,
    "managedByTenants": [
        {
        "tenantId": "2b8e"
        },
        {
        "tenantId": "d6ad"
        }
    ],
    "name": "Visual Studio Enterprise",
    "state": "Disabled",
    "homeTenantId": "72f9",          // maps to tenantId from REST API
    "tenantId": "72f9",              // this is the tenant ID of the token
    "user": {
        "name": "@microsoft.com",
        "type": "user"
}

Then login with az login -t 2b8e, this subscription is now as below. Notice that tenantId has changed to reflect the token tenant ID.

{
    "cloudName": "AzureCloud",
    "id": "9535",
    "isDefault": false,
    "managedByTenants": [
        {
        "tenantId": "2b8e"
        }
    ],
    "name": "Visual Studio Enterprise",
    "state": "Disabled",
    "homeTenantId": "72f9",
    "tenantId": "2b8e",              // tenant ID is switched to the one specified by -t
    "user": {
        "name": "@microsoft.com",
        "type": "user"
    }
}

This allows the user to operate on subscription 9535 from tenant 2b8e.

For backward compatibility, switch profile with az cloud set -n AzureCloud --profile 2019-03-01-hybrid and login again az login -t 2b8e

{
    "cloudName": "AzureCloud",
    "id": "9535",
    "isDefault": false,
    "name": "Visual Studio Enterprise",
    "state": "Disabled",
    "tenantId": "2b8e",
    "user": {
        "name": "@microsoft.com",
        "type": "user"
    }
}

This time, everything is the same as before. To switch cloud back, run az cloud set -n AzureCloud --profile latest

qianwens · 2020-01-19T03:10:48Z

src/azure-cli-core/azure/cli/core/_profile.py

+            }
+            # for Subscriptions - List REST API 2019-06-01's subscription account
+            if subscription_dict[_SUBSCRIPTION_NAME] != _TENANT_LEVEL_ACCOUNT_NAME:
+                if hasattr(s, 'subscription_tenant_id'):


what is the difference between subscription_tenant_id and tenant_id?

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

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?

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.

qianwens · 2020-01-19T03:13:00Z

src/azure-cli-core/azure/cli/core/_profile.py

+            if subscription_dict[_SUBSCRIPTION_NAME] != _TENANT_LEVEL_ACCOUNT_NAME:
+                if hasattr(s, 'subscription_tenant_id'):
+                    subscription_dict[_SUBSCRIPTION_TENANT_ID] = s.subscription_tenant_id
+                if hasattr(s, 'managed_by_tenants'):


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?

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

src/azure-cli-core/azure/cli/core/_profile.py

Juliehzl · 2020-01-19T03:30:35Z

src/azure-cli-core/azure/cli/core/tests/test_profile.py

 from adal import AdalError
 from azure.mgmt.resource.subscriptions.models import \
-    (SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit)
+    (SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit, ManagedByTenant)


Because subscription models have api contraint, please use get_models() here.

This test is only for latest. For specific test, it is in test_profile_v2016_06_01.py.

qianwens · 2020-01-19T03:32:11Z

src/azure-cli-core/azure/cli/core/_profile.py

+                add_sub = True
+                for sub_to_compare in all_subscriptions:
+                    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. "


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.

We SHOULD and WILL display them all. But CLI is currently keying on subscriptionId in the account list which makes showing all of them impossible. This requires an overhaul of the account persistence mechanism.

Juliehzl · 2020-01-19T03:33:57Z

In addition, changing api version will influence many other commands. Please make sure all the tests pass and re-record related test files.

jiasli · 2020-01-19T09:42:38Z

@Juliehzl, thanks for the notification. I was planning to do that but don't have time yet.

# Conflicts: # src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/export_features_prop.json

jiasli · 2020-01-21T08:32:00Z

This test failure

2020-01-21T05:00:05.1483290Z FAIL: test_dls_file_mgmt (azure.cli.command_modules.dls.tests.latest.test_dls_commands.DataLakeStoreFileScenarioTest)
2020-01-21T05:00:05.1484511Z ----------------------------------------------------------------------
2020-01-21T05:00:05.1491248Z Traceback (most recent call last):
2020-01-21T05:00:05.1492359Z   File "/opt/hostedtoolcache/Python/3.8.0/x64/lib/python3.8/site-packages/azure_devtools/scenario_tests/preparers.py", line 50, in _preparer_wrapper
2020-01-21T05:00:05.1493019Z     fn(test_class_instance, **kwargs)
2020-01-21T05:00:05.1494298Z   File "/opt/hostedtoolcache/Python/3.8.0/x64/lib/python3.8/site-packages/azure/cli/command_modules/dls/tests/latest/test_dls_commands.py", line 285, in test_dls_file_mgmt
2020-01-21T05:00:05.1495634Z     self.cmd('dls fs list -n {dls} --path "{folder1}"', checks=[
2020-01-21T05:00:05.1496572Z   File "/opt/hostedtoolcache/Python/3.8.0/x64/lib/python3.8/site-packages/azure/cli/testsdk/base.py", line 161, in cmd
2020-01-21T05:00:05.1497635Z     return execute(self.cli_ctx, command, expect_failure=expect_failure).assert_with_checks(checks)
2020-01-21T05:00:05.1499168Z   File "/opt/hostedtoolcache/Python/3.8.0/x64/lib/python3.8/site-packages/azure/cli/testsdk/base.py", line 201, in __init__
2020-01-21T05:00:05.1499518Z     self._in_process_execute(cli_ctx, command, expect_failure=expect_failure)
2020-01-21T05:00:05.1499997Z   File "/opt/hostedtoolcache/Python/3.8.0/x64/lib/python3.8/site-packages/azure/cli/testsdk/base.py", line 257, in _in_process_execute
2020-01-21T05:00:05.1500189Z     raise AssertionError(ex)
2020-01-21T05:00:05.1500731Z AssertionError: Can't overwrite existing cassette ('/opt/hostedtoolcache/Python/3.8.0/x64/lib/python3.8/site-packages/azure/cli/command_modules/dls/tests/latest/recordings/test_dls_file_mgmt.yaml') in your current record mode ('once').
2020-01-21T05:00:05.1501286Z No match for the request (<Request (GET) https://cliadls000002.azuredatalakestore.net/webhdfs/v1/?OP=LISTSTATUS&api-version=2018-09-01&listSize=4000>) was found.
2020-01-21T05:00:05.1501550Z Found 5 similar requests with 1 different matcher(s) :
...
2020-01-21T05:00:05.1509663Z 5 - (<Request (GET) https://cliadls000002.azuredatalakestore.net/webhdfs/v1/.?OP=LISTSTATUS&api-version=2018-09-01&listSize=4000>).
2020-01-21T05:00:05.1510350Z Matchers succeeded : ['method', 'scheme', 'host', 'port', '_custom_request_query_matcher']
2020-01-21T05:00:05.1510574Z Matchers failed :
2020-01-21T05:00:05.1510938Z path - assertion failure :
2020-01-21T05:00:05.1511149Z /webhdfs/v1/ != /webhdfs/v1/.

is caused by urllib3's breaking change urllib3/urllib3#1647 at util/url.py#L253-L255.

urllib3 1.24 keeps the trailing dot ., so the url /webhdfs/v1/. generated by dls is not modified. However the CI is using the latest urllib3==1.25.7 which trims the trailing . and fails due to the mismatch.

We need to record with at least urllib3==1.25.7. To update urllib3, run

pip install -U urllib3

# Conflicts: # src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/export_features.json

jiasli · 2020-01-21T10:24:47Z

For botservice, the recorded YAML files are manually edited to replace 2016-06-01 with 2019-06-01 due to the issue discussed at #8384 (comment).

jiasli · 2020-02-03T09:36:26Z

Per discussion with Lighthouse team, closing as subscriptionTenantId will be changed to homeTenantId.

# Conflicts: # src/azure-cli/azure/cli/command_modules/botservice/tests/latest/recordings/test_botservice_create_should_remove_invalid_char_from_name_when_registration.yaml # src/azure-cli/azure/cli/command_modules/botservice/tests/latest/recordings/test_botservice_create_should_remove_invalid_char_from_name_when_webapp.yaml

# Conflicts: # src/azure-cli/HISTORY.rst

qianwens · 2020-02-06T12:50:03Z

src/azure-cli-core/azure/cli/core/_profile.py

-            })
+            }
+            # for Subscriptions - List REST API 2019-06-01's subscription account
+            if subscription_dict[_SUBSCRIPTION_NAME] != _TENANT_LEVEL_ACCOUNT_NAME:


_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?

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.

revoking review

arrownj · 2020-02-11T09:27:26Z

Hi @jiasli , all the logic implement here is to set homeTenantTd and managedByTenants value. I'm wondering, where do we use these properties? Just used to show to end user ?

arrownj · 2020-02-11T09:23:46Z

src/azure-cli-core/azure/cli/core/_profile.py

+                                       sub_to_add.subscription_id, sub_to_add.display_name,
+                                       sub_to_compare.tenant_id, sub_to_add.tenant_id)
+                        add_sub = False
+                        break


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 ?

# Conflicts: # src/azure-cli/azure/cli/command_modules/vm/tests/latest/recordings/test_vm_create_existing_ids_options.yaml # src/azure-cli/azure/cli/command_modules/vm/tests/latest/recordings/test_vm_create_existing_options.yaml # src/azure-cli/azure/cli/command_modules/vm/tests/latest/recordings/test_vmss_create_existing_ids_options.yaml # src/azure-cli/azure/cli/command_modules/vm/tests/latest/recordings/test_vmss_create_existing_options.yaml

qwordy · 2020-02-11T11:01:33Z

There are conflicting files

Juliehzl

No more concern for me if all the tests pass.

jsntcy · 2020-02-11T14:30:15Z

src/azure-cli-core/azure/cli/core/_profile.py

            subscriptions = self._find_using_specific_tenant(
                tenant_id,
                temp_credentials[_ACCESS_TOKEN])
-            all_subscriptions.extend(subscriptions)


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?

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

jsntcy

support lighthouse

fb46c9c

jiasli requested review from Juliehzl, fengzhou-msft, haroldrandom and qianwens as code owners January 17, 2020 06:52

jiasli self-assigned this Jan 17, 2020

fix linter

e9a5e3d

qianwens reviewed Jan 19, 2020

View reviewed changes

Juliehzl reviewed Jan 19, 2020

View reviewed changes

src/azure-cli-core/azure/cli/core/_profile.py Show resolved Hide resolved

Juliehzl reviewed Jan 19, 2020

View reviewed changes

src/azure-cli-core/azure/cli/core/_profile.py Show resolved Hide resolved

Juliehzl reviewed Jan 19, 2020

View reviewed changes

qianwens reviewed Jan 19, 2020

View reviewed changes

qianwens previously approved these changes Jan 19, 2020

View reviewed changes

jiasli added the do-not-merge label Jan 19, 2020

jiasli mentioned this pull request Jan 20, 2020

BotService: 0.1.5 release #8384

Merged

2 tasks

re-run tests

6853ccb

jiasli requested review from lewu-msft, mmyyrroonn and qwordy as code owners January 21, 2020 03:44

Merge branch 'dev' into lighthouse

35782af

# Conflicts: # src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/export_features_prop.json

jiasli added 2 commits January 21, 2020 16:40

update urllib3

22513ee

Merge branch 'dev' into lighthouse

538ad5f

# Conflicts: # src/azure-cli/azure/cli/command_modules/appconfig/tests/latest/export_features.json

jiasli closed this Feb 3, 2020

Add history

6fff467

jiasli requested a review from yonzhan February 4, 2020 07:42

jiasli added 5 commits February 4, 2020 16:11

clean up

d45a50f

Merge remote-tracking branch 'origin/dev' into lighthouse

2f822f0

re-run botservice tests

b67e4cf

Merge branch 'dev' into lighthouse

51e7f4a

# Conflicts: # src/azure-cli/HISTORY.rst

move history to PR description

35197da

jiasli requested review from arrownj and jsntcy February 6, 2020 05:38

qianwens reviewed Feb 6, 2020

View reviewed changes

qianwens approved these changes Feb 11, 2020

View reviewed changes

arrownj reviewed Feb 11, 2020

View reviewed changes

haroldrandom approved these changes Feb 11, 2020

View reviewed changes

qwordy approved these changes Feb 11, 2020

View reviewed changes

re-run vm tests

7c61bf3

Juliehzl approved these changes Feb 11, 2020

View reviewed changes

jsntcy reviewed Feb 11, 2020

View reviewed changes

lewu-msft approved these changes Feb 11, 2020

View reviewed changes

jsntcy approved these changes Feb 12, 2020

View reviewed changes

jiasli merged commit 34549d2 into Azure:dev Feb 12, 2020

jiasli mentioned this pull request Feb 14, 2020

{azdev} Remove urllib3==1.24.2 from requirements.txt #12211

Merged

jiasli removed the do-not-merge label Feb 19, 2020

jiasli mentioned this pull request Mar 24, 2020

{Profile} az login: Refine warning messages #12726

Merged

jiasli deleted the lighthouse branch April 16, 2020 16:19

jiasli mentioned this pull request Sep 1, 2020

[Enhancement Proposal] User and tenant isolation #15005

Open

jiasli mentioned this pull request Mar 31, 2021

{Core} Quick patch for managed_by_tenants missing from response #17509

Closed

	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

Conversation

jiasli commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qianwens Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Juliehzl commented Jan 19, 2020

Uh oh!

jiasli commented Jan 19, 2020

Uh oh!

jiasli commented Jan 21, 2020

Uh oh!

jiasli commented Jan 21, 2020

Uh oh!

jiasli commented Feb 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arrownj commented Feb 11, 2020

Uh oh!

arrownj Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwordy commented Feb 11, 2020

Uh oh!

Juliehzl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsntcy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jiasli commented Jan 17, 2020 •

edited

Loading

qianwens Jan 19, 2020 •

edited

Loading

arrownj Feb 11, 2020 •

edited

Loading