Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
5 changes: 3 additions & 2 deletions .flake8
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ ignore =
per-file-ignores =
# module level import not at top of file, elevate timer to measure import time
src/azure-cli/azure/cli/__main__.py:E402
exclude =
exclude =
azure_bdist_wheel.py
build
tools
scripts
doc
build_scripts
*/grammar/
*/grammar/
vendored_sdks
59 changes: 42 additions & 17 deletions src/azure-cli-core/azure/cli/core/_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@

_AZ_LOGIN_MESSAGE = "Please run 'az login' to setup account."

_USE_VENDORED_SUBSCRIPTION_SDK = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to change this to False in the future ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not. azure-cli-core uses only very limited functionalities from this SDK, so we can always use the fixed version.



def load_subscriptions(cli_ctx, all_clouds=False, refresh=False):
profile = Profile(cli_ctx=cli_ctx)
Expand Down Expand Up @@ -258,7 +260,7 @@ def _normalize_properties(self, user, subscriptions, is_service_principal, cert_
subscription_dict = {
_SUBSCRIPTION_ID: s.id.rpartition('/')[2],
_SUBSCRIPTION_NAME: display_name,
_STATE: s.state.value,
_STATE: s.state,
_USER_ENTITY: {
_USER_NAME: user,
_USER_TYPE: _SERVICE_PRINCIPAL if is_service_principal else _USER
Expand Down Expand Up @@ -303,11 +305,18 @@ def _build_tenant_level_accounts(self, tenants):
return result

def _new_account(self):
from azure.cli.core.profiles import ResourceType, get_sdk
SubscriptionType, StateType = get_sdk(self.cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS, 'Subscription',
'SubscriptionState', mod='models')
"""Build an empty Subscription which will be used as a tenant account.
API version doesn't matter as only specified attributes are preserved by _normalize_properties."""
if _USE_VENDORED_SUBSCRIPTION_SDK:
from azure.cli.core.vendored_sdks.subscriptions.models import Subscription
SubscriptionType = Subscription
else:
from azure.cli.core.profiles import ResourceType, get_sdk
SubscriptionType, StateType = get_sdk(self.cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS,
'Subscription',
'SubscriptionState', mod='models')
s = SubscriptionType()
s.state = StateType.enabled
s.state = 'Enabled'
Copy link
Member Author

@jiasli jiasli Nov 16, 2020

Choose a reason for hiding this comment

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

Track 2 uses str instead of Enum SubscriptionState as the value when deserializing Subscription objects, so we do the same.

Track 1:

https://github.com/Azure/azure-sdk-for-python/blob/azure-mgmt-resource_10.2.0/sdk/resources/azure-mgmt-resource/azure/mgmt/resource/subscriptions/v2019_11_01/models/_models_py3.py#L390

'state': {'key': 'state', 'type': 'SubscriptionState'},

Track 2:

https://github.com/Azure/azure-sdk-for-python/blob/azure-mgmt-resource_15.0.0/sdk/resources/azure-mgmt-resource/azure/mgmt/resource/subscriptions/v2019_11_01/models/_models_py3.py#L453

'state': {'key': 'state', 'type': 'str'},

See email: A breaking change in Enum property's deserialization

return s

def find_subscriptions_in_vm_with_msi(self, identity_id=None, allow_no_subscriptions=None):
Expand Down Expand Up @@ -449,8 +458,7 @@ def _match_account(account, subscription_id, secondary_key_name, secondary_key_v

@staticmethod
def _pick_working_subscription(subscriptions):
from azure.mgmt.resource.subscriptions.models import SubscriptionState
s = next((x for x in subscriptions if x.get(_STATE) == SubscriptionState.enabled.value), None)
s = next((x for x in subscriptions if x.get(_STATE) == 'Enabled'), None)
Copy link
Member Author

Choose a reason for hiding this comment

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

return s or subscriptions[0]

def is_tenant_level_account(self):
Expand Down Expand Up @@ -821,18 +829,18 @@ def __init__(self, cli_ctx, auth_context_factory, adal_token_cache, arm_client_f
def create_arm_client_factory(credentials):
if arm_client_factory:
return arm_client_factory(credentials)
from azure.cli.core.profiles._shared import get_client_class
from azure.cli.core.profiles import ResourceType, get_api_version
from azure.cli.core.commands.client_factory import configure_common_settings
from azure.cli.core.azclierror import CLIInternalError
client_type = get_client_class(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS)
from azure.cli.core.commands.client_factory import _prepare_client_kwargs_track2

client_type = self._get_subscription_client_class()
if client_type is None:
raise CLIInternalError("Unable to get '{}' in profile '{}'"
.format(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS, cli_ctx.cloud.profile))
api_version = get_api_version(cli_ctx, ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS)
client_kwargs = _prepare_client_kwargs_track2(cli_ctx)
# We don't need to change credential_scopes as 'scopes' is ignored by BasicTokenCredential anyway
client = client_type(credentials, api_version=api_version,
base_url=self.cli_ctx.cloud.endpoints.resource_manager)
Comment on lines 839 to -834
Copy link
Contributor

Choose a reason for hiding this comment

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

What api-version of Subscription is used by azure-cli, the corresponding version of vendor SDK is required for azure-cli-core, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only 2019-11-01 and 2016-06-01 are used and they are included under vendored_sdks.

ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2019-11-01',

ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS: '2016-06-01',

Copy link
Contributor

@zhoxing-ms zhoxing-ms Nov 17, 2020

Choose a reason for hiding this comment

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

Yes, I know it~ I mean if a new api-version of Subscription is added to azure-cli in the feture, does the vendor SDK in azure-cli-core also need to change to this new api-version, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

MGMT_RESOURCE_SUBSCRIPTIONS is now only used by azure-cli-core. We can use it for now, assuming the corresponding api-version is vendored correctly.

configure_common_settings(cli_ctx, client)
base_url=self.cli_ctx.cloud.endpoints.resource_manager, **client_kwargs)
return client

self._arm_client_factory = create_arm_client_factory
Expand Down Expand Up @@ -916,16 +924,17 @@ def _create_auth_context(self, tenant, use_token_cache=True):

def _find_using_common_tenant(self, access_token, resource):
import adal
from msrest.authentication import BasicTokenAuthentication
from azure.cli.core.adal_authentication import BasicTokenCredential

all_subscriptions = []
empty_tenants = []
mfa_tenants = []
token_credential = BasicTokenAuthentication({'access_token': access_token})
token_credential = BasicTokenCredential(access_token)
client = self._arm_client_factory(token_credential)
tenants = client.tenants.list()
for t in tenants:
tenant_id = t.tenant_id
logger.debug("Finding subscriptions under tenant %s", tenant_id)
# display_name is available since /tenants?api-version=2018-06-01,
# not available in /tenants?api-version=2016-06-01
if not hasattr(t, 'display_name'):
Expand All @@ -934,6 +943,7 @@ def _find_using_common_tenant(self, access_token, resource):
t.display_name = t.additional_properties.get('displayName')
temp_context = self._create_auth_context(tenant_id)
try:
logger.debug("Acquiring a token with tenant=%s, resource=%s", tenant_id, resource)
temp_credentials = temp_context.acquire_token(resource, self.user_id, _CLIENT_ID)
except adal.AdalError as ex:
# because user creds went through the 'common' tenant, the error here must be
Expand Down Expand Up @@ -990,9 +1000,9 @@ def _find_using_common_tenant(self, access_token, resource):
return all_subscriptions

def _find_using_specific_tenant(self, tenant, access_token):
from msrest.authentication import BasicTokenAuthentication
from azure.cli.core.adal_authentication import BasicTokenCredential

token_credential = BasicTokenAuthentication({'access_token': access_token})
token_credential = BasicTokenCredential(access_token)
client = self._arm_client_factory(token_credential)
subscriptions = client.subscriptions.list()
all_subscriptions = []
Expand All @@ -1005,6 +1015,21 @@ def _find_using_specific_tenant(self, tenant, access_token):
self.tenants.append(tenant)
return all_subscriptions

def _get_subscription_client_class(self): # pylint: disable=no-self-use
"""Get the subscription client class. It can come from either the vendored SDK or public SDK, depending
on the design of architecture.
"""
if _USE_VENDORED_SUBSCRIPTION_SDK:
# Use vendered subscription SDK to decouple from `resource` command module
from azure.cli.core.vendored_sdks.subscriptions import SubscriptionClient
client_type = SubscriptionClient
else:
# Use the public SDK
from azure.cli.core.profiles import ResourceType
from azure.cli.core.profiles._shared import get_client_class
client_type = get_client_class(ResourceType.MGMT_RESOURCE_SUBSCRIPTIONS)
return client_type


class CredsCache:
'''Caches AAD tokena and service principal secrets, and persistence will
Expand Down
10 changes: 10 additions & 0 deletions src/azure-cli-core/azure/cli/core/adal_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,13 @@ def _try_scopes_to_resource(scopes):

# Exactly only one scope is provided
return scopes_to_resource(scopes)


class BasicTokenCredential:
# pylint:disable=too-few-public-methods
"""A Track 2 implementation of msrest.authentication.BasicTokenAuthentication."""
def __init__(self, access_token):
self.access_token = access_token

def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
return AccessToken(self.access_token, int(time.time() + 3600))
Copy link
Member

@jsntcy jsntcy Nov 18, 2020

Choose a reason for hiding this comment

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

Can we use a meaningful constant instead of a magic number (3600) so that reviewers can understand it easier?@[email protected]

Copy link
Member Author

Choose a reason for hiding this comment

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

As self.access_token is a fixed value, get_token cannot refresh the token, thus defeating the purpose of returning a valid expires_on. Using time.time() + 3600 will force SDK to use the provided token, regardless of whether the token is expired or not.

Also, Subscriptions - List and Tenants - List APIs are not long-running operations, so the time of expires_on will not be reached anyway.

21 changes: 13 additions & 8 deletions src/azure-cli-core/azure/cli/core/tests/test_profile.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,16 @@
from copy import deepcopy

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

from azure.cli.core._profile import (Profile, CredsCache, SubscriptionFinder,
ServicePrincipalAuth, _AUTH_CTX_FACTORY)
ServicePrincipalAuth, _AUTH_CTX_FACTORY, _USE_VENDORED_SUBSCRIPTION_SDK)
if _USE_VENDORED_SUBSCRIPTION_SDK:
from azure.cli.core.vendored_sdks.subscriptions.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit, ManagedByTenant)
else:
from azure.mgmt.resource.subscriptions.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit, ManagedByTenant)

from azure.cli.core.mock import DummyCli

from knack.util import CLIError
Expand Down Expand Up @@ -314,7 +319,7 @@ def test_subscription_finder_constructor(self, get_api_mock):
cli.cloud.endpoints.resource_manager = 'http://foo_arm'
finder = SubscriptionFinder(cli, None, None, arm_client_factory=None)
result = finder._arm_client_factory(mock.MagicMock())
self.assertEqual(result.config.base_url, 'http://foo_arm')
self.assertEqual(result._client._base_url, 'http://foo_arm')

@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
def test_subscription_finder_fail_on_arm_client_factory(self, get_client_class_mock):
Expand Down Expand Up @@ -1107,7 +1112,7 @@ def find_from_raw_token(self, tenant, token):
self.assertEqual(s['id'], self.id1.split('/')[-1])

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_system_assigned(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1145,7 +1150,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a')

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_no_subscriptions(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1183,7 +1188,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], self.test_msi_tenant)

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_client_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1269,7 +1274,7 @@ def set_token(self):
self.assertEqual(subscriptions[0]['user']['assignedIdentityInfo'], 'MSIObject-{}'.format(test_object_id))

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_res_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,17 @@
from copy import deepcopy

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

from azure.cli.core._profile import (Profile, CredsCache, SubscriptionFinder,
ServicePrincipalAuth, _AUTH_CTX_FACTORY)
ServicePrincipalAuth, _AUTH_CTX_FACTORY, _USE_VENDORED_SUBSCRIPTION_SDK)

if _USE_VENDORED_SUBSCRIPTION_SDK:
from azure.cli.core.vendored_sdks.subscriptions.v2016_06_01.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit)
else:
from azure.mgmt.resource.subscriptions.v2016_06_01.models import \
(SubscriptionState, Subscription, SubscriptionPolicies, SpendingLimit)

from azure.cli.core.mock import DummyCli

from knack.util import CLIError
Expand Down Expand Up @@ -294,7 +300,7 @@ def test_subscription_finder_constructor(self, get_api_mock):
cli.cloud.endpoints.resource_manager = 'http://foo_arm'
finder = SubscriptionFinder(cli, None, None, arm_client_factory=None)
result = finder._arm_client_factory(mock.MagicMock())
self.assertEqual(result.config.base_url, 'http://foo_arm')
self.assertEqual(result._client._base_url, 'http://foo_arm')

@mock.patch('adal.AuthenticationContext', autospec=True)
def test_get_auth_info_for_logged_in_service_principal(self, mock_auth_context):
Expand Down Expand Up @@ -934,7 +940,7 @@ def find_from_raw_token(self, tenant, token):
self.assertEqual(s['id'], self.id1.split('/')[-1])

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_system_assigned(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -972,7 +978,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a')

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_no_subscriptions(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1010,7 +1016,7 @@ def __init__(self, *args, **kwargs):
self.assertEqual(s['tenantId'], self.test_msi_tenant)

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_client_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down Expand Up @@ -1096,7 +1102,7 @@ def set_token(self):
self.assertEqual(subscriptions[0]['user']['assignedIdentityInfo'], 'MSIObject-{}'.format(test_object_id))

@mock.patch('requests.get', autospec=True)
@mock.patch('azure.cli.core.profiles._shared.get_client_class', autospec=True)
@mock.patch('azure.cli.core._profile.SubscriptionFinder._get_subscription_client_class', autospec=True)
def test_find_subscriptions_in_vm_with_msi_user_assigned_with_res_id(self, mock_get_client_class, mock_get):

class ClientStub:
Expand Down
7 changes: 7 additions & 0 deletions src/azure-cli-core/azure/cli/core/vendored_sdks/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# coding=utf-8
# --------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.
# Code generated by Microsoft (R) AutoRest Code Generator.
# Changes may cause incorrect behavior and will be lost if the code is regenerated.
# --------------------------------------------------------------------------
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# coding=utf-8
# --------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for license information.
# Code generated by Microsoft (R) AutoRest Code Generator.
# Changes may cause incorrect behavior and will be lost if the code is regenerated.
# --------------------------------------------------------------------------

from ._subscription_client import SubscriptionClient
__all__ = ['SubscriptionClient']

try:
from ._patch import patch_sdk # type: ignore
patch_sdk()
except ImportError:
pass
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# coding=utf-8
# --------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for
# license information.
#
# Code generated by Microsoft (R) AutoRest Code Generator.
# Changes may cause incorrect behavior and will be lost if the code is
# regenerated.
# --------------------------------------------------------------------------
from typing import Any

from azure.core.configuration import Configuration
from azure.core.pipeline import policies
from azure.mgmt.core.policies import ARMHttpLoggingPolicy

from ._version import VERSION


class SubscriptionClientConfiguration(Configuration):
"""Configuration for SubscriptionClient.

Note that all parameters used to create this instance are saved as instance
attributes.

:param credential: Credential needed for the client to connect to Azure.
:type credential: ~azure.core.credentials.TokenCredential
"""

def __init__(
self,
credential, # type: "TokenCredential"
**kwargs # type: Any
):
# type: (...) -> None
if credential is None:
raise ValueError("Parameter 'credential' must not be None.")
super(SubscriptionClientConfiguration, self).__init__(**kwargs)

self.credential = credential
self.credential_scopes = kwargs.pop('credential_scopes', ['https://management.azure.com/.default'])
kwargs.setdefault('sdk_moniker', 'azure-mgmt-resource/{}'.format(VERSION))
self._configure(**kwargs)

def _configure(
self,
**kwargs # type: Any
):
# type: (...) -> None
self.user_agent_policy = kwargs.get('user_agent_policy') or policies.UserAgentPolicy(**kwargs)
self.headers_policy = kwargs.get('headers_policy') or policies.HeadersPolicy(**kwargs)
self.proxy_policy = kwargs.get('proxy_policy') or policies.ProxyPolicy(**kwargs)
self.logging_policy = kwargs.get('logging_policy') or policies.NetworkTraceLoggingPolicy(**kwargs)
self.http_logging_policy = kwargs.get('http_logging_policy') or ARMHttpLoggingPolicy(**kwargs)
self.retry_policy = kwargs.get('retry_policy') or policies.RetryPolicy(**kwargs)
self.custom_hook_policy = kwargs.get('custom_hook_policy') or policies.CustomHookPolicy(**kwargs)
self.redirect_policy = kwargs.get('redirect_policy') or policies.RedirectPolicy(**kwargs)
self.authentication_policy = kwargs.get('authentication_policy')
if self.credential and not self.authentication_policy:
self.authentication_policy = policies.BearerTokenCredentialPolicy(self.credential, *self.credential_scopes, **kwargs)
Loading