Skip to content
Merged
16 changes: 16 additions & 0 deletions src/azure-cli-core/azure/cli/core/_debug.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,22 @@ def change_ssl_cert_verification(client):
return client


def change_ssl_cert_verification_track2():
client_kwargs = {}
if should_disable_connection_verify():
logger.warning("Connection verification disabled by environment variable %s",
DISABLE_VERIFY_VARIABLE_NAME)
os.environ[ADAL_PYTHON_SSL_NO_VERIFY] = '1'
client_kwargs['connection_verify'] = False
elif REQUESTS_CA_BUNDLE in os.environ:
ca_bundle_file = os.environ[REQUESTS_CA_BUNDLE]
if not os.path.isfile(ca_bundle_file):
raise CLIError('REQUESTS_CA_BUNDLE environment variable is specified with an invalid file path')
logger.debug("Using CA bundle file at '%s'.", ca_bundle_file)
client_kwargs['connection_verify'] = ca_bundle_file
return client_kwargs


def allow_debug_adal_connection():
if should_disable_connection_verify():
os.environ[ADAL_PYTHON_SSL_NO_VERIFY] = '1'
22 changes: 18 additions & 4 deletions src/azure-cli-core/azure/cli/core/adal_authentication.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
# Licensed under the MIT License. See License.txt in the project root for license information.
# --------------------------------------------------------------------------------------------

import time
import requests
import adal

from msrest.authentication import Authentication

from azure.core.credentials import AccessToken
from azure.cli.core.util import in_cloud_console

from knack.util import CLIError
Expand All @@ -19,11 +20,10 @@ def __init__(self, token_retriever, external_tenant_token_retriever=None):
self._token_retriever = token_retriever
self._external_tenant_token_retriever = external_tenant_token_retriever

def signed_session(self, session=None): # pylint: disable=arguments-differ
session = session or super(AdalAuthentication, self).signed_session()
def _get_token(self):
external_tenant_tokens = None
try:
scheme, token, _ = self._token_retriever()
scheme, token, full_token = self._token_retriever()
if self._external_tenant_token_retriever:
external_tenant_tokens = self._external_tenant_token_retriever()
except CLIError as err:
Expand Down Expand Up @@ -55,6 +55,20 @@ def signed_session(self, session=None): # pylint: disable=arguments-differ
except requests.exceptions.ConnectionError as err:
raise CLIError('Please ensure you have network connection. Error detail: ' + str(err))

return scheme, token, full_token, external_tenant_tokens

# This method is exposed for Azure Core.
def get_token(self, *scopes, **kwargs): # pylint:disable=unused-argument
_, token, full_token, _ = self._get_token()

return AccessToken(token, int(full_token['expiresIn'] + time.time()))

# This method is exposed for msrest.
def signed_session(self, session=None): # pylint: disable=arguments-differ
session = session or super(AdalAuthentication, self).signed_session()

scheme, token, _, external_tenant_tokens = self._get_token()

header = "{} {}".format(scheme, token)
session.headers['Authorization'] = header
if external_tenant_tokens:
Expand Down
15 changes: 11 additions & 4 deletions src/azure-cli-core/azure/cli/core/commands/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -421,15 +421,21 @@ def _get_operation():
return _get_operation()


def cached_put(cmd_obj, operation, parameters, *args, **kwargs):

def cached_put(cmd_obj, operation, parameters, *args, setter_arg_name='parameters', **kwargs):
"""
setter_arg_name: The name of the argument in the setter which corresponds to the object being updated.
In track2, unknown kwargs will raise, so we should not pass 'parameters" for operation when the name of the argument
in the setter which corresponds to the object being updated is not 'parameters'.
"""
def _put_operation():
result = None
if args:
extended_args = args + (parameters,)
result = operation(*extended_args)
elif kwargs is not None:
result = operation(parameters=parameters, **kwargs)
kwargs[setter_arg_name] = parameters
result = operation(**kwargs)
del kwargs[setter_arg_name]
return result

# early out if the command does not use the cache
Expand Down Expand Up @@ -1067,7 +1073,8 @@ def _is_paged(obj):
and not isinstance(obj, list) \
and not isinstance(obj, dict):
from msrest.paging import Paged
return isinstance(obj, Paged)
from azure.core.paging import ItemPaged as AzureCorePaged
return isinstance(obj, (AzureCorePaged, Paged))
return False


Expand Down
2 changes: 1 addition & 1 deletion src/azure-cli-core/azure/cli/core/commands/arm.py
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ def handler(args): # pylint: disable=too-many-branches,too-many-statements
if setter_arg_name == 'parameters':
result = cached_put(cmd, setter, **setterargs)
else:
result = cached_put(cmd, setter, setterargs[setter_arg_name], **setterargs)
result = cached_put(cmd, setter, setterargs[setter_arg_name], setter_arg_name=setter_arg_name, **setterargs)

if supports_no_wait and no_wait_enabled:
return None
Expand Down
36 changes: 34 additions & 2 deletions src/azure-cli-core/azure/cli/core/commands/client_factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from azure.cli.core.extension import EXTENSIONS_MOD_PREFIX
from azure.cli.core.profiles._shared import get_client_class, SDKProfile
from azure.cli.core.profiles import ResourceType, CustomResourceType, get_api_version, get_sdk
from azure.cli.core.util import get_az_user_agent
from azure.cli.core.util import get_az_user_agent, is_track2

from knack.log import get_logger
from knack.util import CLIError
Expand Down Expand Up @@ -102,6 +102,34 @@ def configure_common_settings(cli_ctx, client):
client.config.generate_client_request_id = 'x-ms-client-request-id' not in cli_ctx.data['headers']


def configure_common_settings_track2(cli_ctx):
client_kwargs = {}

client_kwargs.update(_debug.change_ssl_cert_verification_track2())

client_kwargs['logging_enable'] = True
client_kwargs['user_agent'] = get_az_user_agent()

try:
command_ext_name = cli_ctx.data['command_extension_name']
if command_ext_name:
client_kwargs['user_agent'] += "CliExtension/{}".format(command_ext_name)
except KeyError:
pass

headers = dict(cli_ctx.data['headers'])
command_name_suffix = ';completer-request' if cli_ctx.data['completer_active'] else ''
headers['CommandName'] = "{}{}".format(cli_ctx.data['command'], command_name_suffix)
if cli_ctx.data.get('safe_params'):
headers['ParameterSetName'] = ' '.join(cli_ctx.data['safe_params'])
client_kwargs['headers'] = headers

if 'x-ms-client-request-id' in cli_ctx.data['headers']:
client_kwargs['request_id'] = cli_ctx.data['headers']['x-ms-client-request-id']
Copy link
Contributor

@Juliehzl Juliehzl Apr 13, 2020

Choose a reason for hiding this comment

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

Some questions here:

  1. request_id is required for client?
  2. Should we use x-ms-request-id?
    x-ms-request-id: This is a common response header that contains a unique identifier for the current operation, service generated.
    x-ms-client-request-id: Optional. Contains a unique ID provided by the client to identify the specific request.
    It looks x-ms-client--request-id is right. #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually request_id will be set to "x-ms-client-request-id" in request header, you can check the code in azure core below:

def on_request(self, request):
# type: (PipelineRequest) -> None
"""Updates with the given request id before sending the request to the next policy.

    :param request: The PipelineRequest object
    :type request: ~azure.core.pipeline.PipelineRequest
    """
    request_id = unset = object()
    if 'request_id' in request.context.options:
        request_id = request.context.options.pop('request_id')
        if request_id is None:
            return
    elif self._request_id is None:
        return
    elif self._request_id is not _Unset:
        request_id = self._request_id   # type: ignore
    elif self._auto_request_id:
        request_id = str(uuid.uuid1())  # type: ignore
    if request_id is not unset:
        header = {"x-ms-client-request-id": request_id}

In reply to: 407376852 [](ancestors = 407376852)


return client_kwargs


def _get_mgmt_service_client(cli_ctx,
client_type,
subscription_bound=True,
Expand Down Expand Up @@ -131,12 +159,16 @@ def _get_mgmt_service_client(cli_ctx,
if kwargs:
client_kwargs.update(kwargs)

if is_track2(client_type):
client_kwargs.update(configure_common_settings_track2(cli_ctx))

if subscription_bound:
client = client_type(cred, subscription_id, **client_kwargs)
else:
client = client_type(cred, **client_kwargs)

configure_common_settings(cli_ctx, client)
if not is_track2(client):
configure_common_settings(cli_ctx, client)

return client, subscription_id

Expand Down
20 changes: 16 additions & 4 deletions src/azure-cli-core/azure/cli/core/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@
import logging

from six.moves.urllib.request import urlopen # pylint: disable=import-error

from azure.common import AzureException
from azure.core.exceptions import AzureError
from knack.log import get_logger
from knack.util import CLIError, to_snake_case
from azure.common import AzureException
from inspect import getfullargspec as get_arg_spec

logger = get_logger(__name__)

Expand Down Expand Up @@ -51,7 +54,7 @@ def handle_exception(ex): # pylint: disable=too-many-return-statements
logger.error("To learn more about --query, please visit: "
"https://docs.microsoft.com/cli/azure/query-azure-cli?view=azure-cli-latest")
return 1
if isinstance(ex, (CLIError, CloudError, AzureException)):
if isinstance(ex, (CLIError, CloudError, AzureException, AzureError)):
logger.error(ex.args[0])
try:
for detail in ex.args[0].error.details:
Expand Down Expand Up @@ -365,6 +368,14 @@ def get_arg_list(op):
return sig.args


def is_track2(client_class):
""" IS this client a autorestv3/track2 one?.
Could be refined later if necessary.
"""
args = get_arg_spec(client_class.__init__).args
return "credential" in args


DISABLE_VERIFY_VARIABLE_NAME = "AZURE_CLI_DISABLE_CONNECTION_VERIFICATION"


Expand All @@ -376,7 +387,8 @@ def should_disable_connection_verify():
def poller_classes():
from msrestazure.azure_operation import AzureOperationPoller
from msrest.polling.poller import LROPoller
return (AzureOperationPoller, LROPoller)
from azure.core.polling import LROPoller as AzureCoreLROPoller
return (AzureOperationPoller, LROPoller, AzureCoreLROPoller)


def augment_no_wait_handler_args(no_wait_enabled, handler, handler_args):
Expand All @@ -394,7 +406,7 @@ def augment_no_wait_handler_args(no_wait_enabled, handler, handler_args):

def sdk_no_wait(no_wait, func, *args, **kwargs):
if no_wait:
kwargs.update({'raw': True, 'polling': False})
kwargs.update({'polling': False})
return func(*args, **kwargs)


Expand Down
1 change: 1 addition & 0 deletions src/azure-cli-core/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
'six~=1.12',
'wheel==0.30.0',
'azure-mgmt-resource==8.0.1',
'azure-mgmt-core==1.0.0'
]

TESTS_REQUIRE = [
Expand Down
3 changes: 1 addition & 2 deletions src/azure-cli/azure/cli/command_modules/advisor/custom.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
# --------------------------------------------------------------------------------------------

import uuid
from azure.cli.core.util import sdk_no_wait


def list_recommendations(client, ids=None, resource_group_name=None,
Expand Down Expand Up @@ -132,7 +131,7 @@ def _parse_recommendation_uri(recommendation_uri):
def _generate_recommendations(client):
from msrestazure.azure_exceptions import CloudError

response = sdk_no_wait(True, client.generate)
response = client.generate(raw=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

so. Is sdk_not_wait useless?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, sdk_not_wait is still useful for LRO as it will set "polling" to false if no_wait is true.
But here it should not use sdk_no_wait as it's not a LRO (I checked the SDK method.)
'raw' is another story which is different from LRO, so it should not be set in sdk_no_wait.

All other places I replace 'sdk_no_wait' with 'raw=True' actually are bug fix for those modules.


In reply to: 406657030 [](ancestors = 406657030)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can split the migration of advisor and backup to a different PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can split it to another PR.


In reply to: 406657692 [](ancestors = 406657692)

Copy link
Member Author

@jsntcy jsntcy Apr 13, 2020

Choose a reason for hiding this comment

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

@Prasanna-Padmanabhan,
Here we pass raw=True in method instead of using sdk_no_wait (actually it's not correct to use sdk_no_wait before as client.generate is not a long running operation.), please help confirm this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have the necessary knowledge to sign off on this change. If all existing tests are passing, then you can go ahead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Prasanna-Padmanabhan, thanks for your confirm. All existing tests are passing, so we'll commit this change.

location = response.headers['Location']
operation_id = _parse_operation_id(location)

Expand Down
Loading