Fix ansible can use azure-cli credentials #31871

yuwzho · 2017-10-18T08:05:13Z

SUMMARY

Ansible can use Azure-CLI's credential to manage azure resources.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

azure_rm_common

ANSIBLE VERSION

2.4.0.0

ADDITIONAL INFORMATION

User can login the AzureCLI and select the subscription via the following command. Then if the playbook hasn't specific the credentials, it will use AzureCLI's credential to manage the Azure resources.

az login
az account set -s <subscriptionId>

zikalino · 2017-10-18T08:11:07Z

lib/ansible/utils/module_docs_fragments/azure.py

-    - For authentication with Azure you can pass parameters, set environment variables or use a profile stored
-      in ~/.azure/credentials. Authentication is possible using a service principal or Active Directory user.
+    - For authentication with Azure you can pass parameters, set environment variables, use a profile stored
+      in ~/.azure/credentials or login your AzureCLI. 


"login your Azure CLI" -> "login with Azure CLI"

zikalino · 2017-10-18T08:11:45Z

lib/ansible/utils/module_docs_fragments/azure.py

@@ -82,4 +83,5 @@ class ModuleDocFragment(object):
      a [default] section and the following keys: subscription_id, client_id, secret and tenant or
      subscription_id, ad_user and password. It is also possible to add additional profiles. Specify the profile
      by passing profile or setting AZURE_PROFILE in the environment."
+    - Use 'az login' to login your AzureCLI.


AzureCLI -> Azure CLI

zikalino · 2017-10-18T08:20:34Z

lib/ansible/module_utils/azure_rm_common.py

+        elif self.credentials.get('credentials') is not None:
+            self.azure_credentials = self.credentials.get('credentials')
+
+        if not self.azure_credentials:
            self.fail("Failed to authenticate with provided credentials. Some attributes were missing. "
                      "Credentials must include client_id, secret and tenant or ad_user and password or "
                      "be logged using AzureCLI.")


I think still here we should change to "be logged in using Azure CLI"

zikalino · 2017-10-18T08:28:25Z

lib/ansible/utils/module_docs_fragments/azure.py

@@ -82,4 +83,5 @@ class ModuleDocFragment(object):
      a [default] section and the following keys: subscription_id, client_id, secret and tenant or
      subscription_id, ad_user and password. It is also possible to add additional profiles. Specify the profile
      by passing profile or setting AZURE_PROFILE in the environment."
+    - Use 'az login' to login your Azure CLI.


and here "your" -> "with"

gundalow · 2017-10-18T09:44:52Z

lib/ansible/utils/module_docs_fragments/azure.py

-    - For authentication with Azure you can pass parameters, set environment variables or use a profile stored
-      in ~/.azure/credentials. Authentication is possible using a service principal or Active Directory user.
+    - For authentication with Azure you can pass parameters, set environment variables, use a profile stored
+      in ~/.azure/credentials or login with Azure CLI.


C(~/.azure/credentials) will give formatted text

http://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#formatting-options

Could you please update the other docs, for example C(AZURE_SUBSCRIPTION_ID), etc?

Thanks in advance

ansibot · 2017-10-18T10:28:01Z

cc @haroldwongms @nitzmahone @tstringer
click here for bot help

yuwzho · 2017-10-19T06:20:24Z

@gundalow Thanks for your review. I have format the option value in the document.

yuwzho · 2017-10-20T10:29:38Z

@gundalow

gundalow · 2017-10-23T06:42:41Z

lib/ansible/utils/module_docs_fragments/azure.py

@@ -67,19 +67,27 @@ class ModuleDocFragment(object):
              C(AzureUSGovernment)), or a metadata discovery endpoint URL (required for Azure Stack). Can also be set via credential file profile or
              the C(AZURE_CLOUD_ENVIRONMENT) environment variable.
        default: AzureCloud
+    cli_default_profile:
+        description:
+            - Set to C(true), when login with Azure CLI


This description doesn't make sense to me.

How about Whether login with Azure CLI?

Whether login with Azure CLI sounds better.
What happens if this is false?
Forgive my basic questions, I've not used Azure

false is the same as NONE, this is a nullable option

Has updated

gundalow · 2017-10-23T06:43:28Z

lib/ansible/utils/module_docs_fragments/azure.py

+    cli_default_profile:
+        description:
+            - Set to C(true), when login with Azure CLI
+        required: false


If this is a new option then please add
version_added: "2.5"

It is a bugfix, the code has this option already, but the document haven't mention this option before.

ah, OK. Thanks for clarification

ansibot · 2017-10-24T03:25:26Z

@yuwzho this PR contains the following merge commits:

d123f5d

Please rebase your branch to remove these commits.

click here for bot help

* Make the ansible use azure-cli credentials * move the setValue out

ansibot · 2017-10-30T03:18:46Z

The test ansible-test sanity --test rstcheck [?] failed with the following errors:

docs/docsite/rst/guide_azure.rst:76:0: (bash) `$ az account set -s <your-subscription-id>'
docs/docsite/rst/guide_azure.rst:76:0: (bash) syntax error near unexpected token `newline'

click here for bot help

yuwzho · 2017-10-31T11:35:54Z

Hi @haroldwongms @nitzmahone @tstringer , please help take a look at this PR. Thanks very much!

haroldwongms · 2017-10-31T20:23:44Z

This is not working for me. I hit the error check in lines 245 - 248 when getting credentials.

nitzmahone · 2017-10-31T22:16:34Z

I missed zapping the UI element for this from 2.4 because it wasn't fully implemented. We definitely want this functionality, but we're talking about a slightly different way to expose it that will be consistent across Ansible cloud modules (probably a new credential_source arg that will default to auto, which would support the current behavior (and we'd probably just add cli as the lowest value in the chain), but also support explicit values like env,module,profile, etc.

haroldwongms · 2017-10-31T23:01:18Z

@nitzmahone You bring up a good point in terms of order of preference. I'm thinking cli should be the highest priority in the chain.

nitzmahone · 2017-10-31T23:07:12Z

That's not typically the way Ansible does it though- usually we go from closest/most-explicit definition out (some are arguably at the same level, so we choose arbitrarily in those cases and try to be consistent across providers). So module args > env > credential file on disk > CLI would probably be how I'd do it at first blush. For a typical intro cloud-shell user that's using ambient CLI auth, it'll still "just work" because that'd be all they have specified, but that way if someone needs to take more control at a different level (without explicitly specifying the source in the task), they still can.

yuwzho · 2017-11-01T02:04:12Z

@haroldwongms I think you are not set the cli_default_profile and meet such error. As currently behavior, the priority is CLI > profile > env var > auth file, so that using CLI should explicitly tell ansible use the CLI auth.

@nitzmahone
This PR is only for fixing login with Azure CLI's functional work. Since current error message tells user can login with CLI, but it is not work. That is why I treat this PR as a bug fix.

As for the ansible cloud modules' auth priority, it is another thing we need to think twice and make all the cloud modules consistent.

ansibot · 2017-11-01T02:04:48Z

cc @xscript @yaweiw @zikalino
click here for bot help

trstringer · 2017-11-01T21:08:59Z

If I'm understanding @nitzmahone correctly, I'm in agreement with him. Environment variables typically by convention take precedence in Ansible or otherwise. If the end user sets them it should be assumed those are the credentials that you want to use. It's quite intentional. If we were to have CLI profile credentials take precedence then we would run into really bad behaviour if the user explicitly sets env vars for connection and credentials. Best case, that's annoying. Worst case, that's disastrous as they could mutate Azure resources in an unintended subscription.

haroldwongms · 2017-11-01T21:50:17Z

@tstringer Totally understand.

@yuwzho I did set both options and neither option worked for me.

ansibot · 2018-01-20T23:08:49Z

cc @trstringer
click here for bot help

nitzmahone · 2018-01-23T07:44:13Z

Superseded by #35213 (the new auth_source arg)- since cli_default_profile was never fully hooked up or documented, we just removed it completely sans deprecation.

zikalino reviewed Oct 18, 2017

View reviewed changes

gundalow requested changes Oct 18, 2017

View reviewed changes

gundalow added azure cloud and removed needs_triage Needs a first human triage before being processed. labels Oct 18, 2017

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 18, 2017

gundalow reviewed Oct 23, 2017

View reviewed changes

ansibot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 24, 2017

yuwzho added 8 commits October 24, 2017 11:27

Make the ansible can use azure-cli credentials (#2)

3437c64

* Make the ansible use azure-cli credentials * move the setValue out

update document

90b3387

Update azure.py

f50c659

Update azure.py

93dca08

update doc

e53b960

lint

4f2ba99

add option value format

b639b77

Update azure.py

5478450

ansibot added the docs_pull_request label Oct 30, 2017

ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 30, 2017

yuwzho added 2 commits October 30, 2017 11:42

Update guide_azure.rst

53f6f08

Update guide_azure.rst

9d6390e

ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 30, 2017

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed new_contributor This PR is the first contribution by a new community member. labels Nov 9, 2017

ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 20, 2018

nitzmahone closed this Jan 23, 2018

yuwzho deleted the yuwzho-cred branch January 23, 2018 08:08

yuwzho restored the yuwzho-cred branch January 23, 2018 08:08

yuwzho deleted the yuwzho-cred branch February 1, 2018 12:06

ansibot added docs This issue/PR relates to or includes documentation. bug This issue/PR relates to a bug. and removed docs_pull_request labels Mar 4, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ansible can use azure-cli credentials #31871

Fix ansible can use azure-cli credentials #31871

yuwzho commented Oct 18, 2017

zikalino Oct 18, 2017

zikalino Oct 18, 2017

zikalino Oct 18, 2017

zikalino Oct 18, 2017

gundalow Oct 18, 2017

ansibot commented Oct 18, 2017

yuwzho commented Oct 19, 2017

yuwzho commented Oct 20, 2017

gundalow Oct 23, 2017

yuwzho Oct 23, 2017

gundalow Oct 23, 2017

yuwzho Oct 24, 2017

yuwzho Oct 24, 2017

gundalow Oct 23, 2017

yuwzho Oct 23, 2017

gundalow Oct 23, 2017

ansibot commented Oct 24, 2017

ansibot commented Oct 30, 2017

yuwzho commented Oct 31, 2017

haroldwongms commented Oct 31, 2017

nitzmahone commented Oct 31, 2017 •

edited

haroldwongms commented Oct 31, 2017

nitzmahone commented Oct 31, 2017

yuwzho commented Nov 1, 2017 •

edited

ansibot commented Nov 1, 2017

trstringer commented Nov 1, 2017

haroldwongms commented Nov 1, 2017

ansibot commented Jan 20, 2018

nitzmahone commented Jan 23, 2018

Fix ansible can use azure-cli credentials #31871

Fix ansible can use azure-cli credentials #31871

Conversation

yuwzho commented Oct 18, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Oct 18, 2017

yuwzho commented Oct 19, 2017

yuwzho commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Oct 24, 2017

ansibot commented Oct 30, 2017

yuwzho commented Oct 31, 2017

haroldwongms commented Oct 31, 2017

nitzmahone commented Oct 31, 2017 • edited

haroldwongms commented Oct 31, 2017

nitzmahone commented Oct 31, 2017

yuwzho commented Nov 1, 2017 • edited

ansibot commented Nov 1, 2017

trstringer commented Nov 1, 2017

haroldwongms commented Nov 1, 2017

ansibot commented Jan 20, 2018

nitzmahone commented Jan 23, 2018

nitzmahone commented Oct 31, 2017 •

edited

yuwzho commented Nov 1, 2017 •

edited