Support azure cli credentials with multiple `subscription_id`s #195

UnwashedMeme · 2020-07-21T03:13:08Z

SUMMARY

This is very similar to #53 but taken a bit further:

rebased onto dev branch
Added more documentation
Tried to cleanup/simply azure_rm_common a bit (not very much).
- move some defaults out of __init__ onto class
- use python kwargs instead fo passing in a dict(...)
- use ansible's env_fallback for auth_source.

I'm hoping to get a bit further but wanted to put up the WIP and get any feedback.

Fred-sun · 2020-08-04T07:07:33Z

@UnwashedMeme Recently we have made some related changes to this repo. We need your help to rebase this PR. Once your rebase is completed, I will test and promote the merger of this PR. Thank you!

UnwashedMeme · 2020-08-05T14:19:34Z

@Fred-sun just rebased again. I'm also happy with Internetionals work in #53. I'm just trying to take it a bit further and do some more cleanups.

I haven't gotten a chance to test all the code paths (msi, profile, cli, sp). I'm hoping you have some tests in here for that and I've tried to make each change small for easier reviewing.

plugins/doc_fragments/azure.py

plugins/module_utils/azure_rm_common.py

Fred-sun · 2020-08-06T05:38:15Z

plugins/module_utils/azure_rm_common.py

@@ -1415,30 +1437,26 @@ def _get_env_credentials(self):

        return None

-    # TODO: use explicit kwargs instead of intermediate dict
-    def _get_credentials(self, params):
+


There is a line between functions (there are two blank lines)。

UnwashedMeme · 2020-08-07T02:19:41Z

@UnwashedMeme Could you please help answer the following questions?

When subscription_ID is specified in the parameter, this parameter has no effect. (When both sunbscription_ID and auth_source parameters exist, you do not specify a priority in use)

I'm afraid I don't understand the question. I'm going to restate it a bit differently and try to answer that. if I miss something, will you please clarify?

When the module parameters include subscription_id and auth_source what happens?

The auth_source previously and still does determine what authentication source will be used. The only time this isn't true is if auth_source is auto or not specified (default to auto). The docs already declare it "controls" which mechanism is used.

This PR reduces the number of cases in which subscription_id is unexpectedly ignored.

subscription_id is already common parameter for at least 2 authentication sources: service principal and msi.

I don't believe the docs currently say that subscription_id implies "only use service principal" in any of subscription_id, auth_source, or notes. The closest they get is "To authenticate via service principal, pass subscription_id, client_id, secret and tenant." To me that does not mean that subscrition_id only belongs to that set, especially when the docs also say, under msi, that "subscription_id or [envvar] can be used to identify the subscription ... if ...more than one"

That current behavior of tying subscription_id to service principal is from this if statement. I don't believe this was a good test because it is possible in the Azure api to authenticate with a service principal specifying the the client id, secret, and tenant without subscription_id. This test on L1464 doesn't ensure any of those important params are actually present.

The change on L480 of this PR improves this situation as it requires either client_id or ad_user to be present in the module params; both of those are better distinguishers of an authentication mechanism than subscription_id.

Fred-sun · 2020-08-07T08:01:13Z

@UnwashedMeme I have been busy with other more urgent things recently, I will watch this later. Thank you!

UnwashedMeme · 2020-08-14T18:07:35Z

rebased; @Fred-sun have you gotten a chance to look at this yet?

Fred-sun

@UnwashedMeme All task test PASS, but sanity fail, please update as follow comment and rebase again! Thank you very much!

plugins/doc_fragments/azure.py

If the `subscription_id` is specified as module parameter or in the environment then try to find that subscription in either the MSI (existing) or CLI credentials (new). This patch brings those two scenarios in line.

Just trying to make the `__init__` fn a bit slimmer and easier to reason about.

This is an isomorphic change, just using python syntax to accomplish the exact same thing.

Ansible modules have a pattern for looking up a module parameter in the environment with precedence of explicit param -> env -> default. Use this pattern to simplify our code here. This shouldn't change any behavior of `auth_source`, just using standard ansible patterns to accomplish it.

No semantic change, just wrapping a long line to be a bit more readable.

Somewhat frequently there is a lookup in the environment for the key that matches a module parameter. This simple helper just encapsulates that to make it a bit easier elsewhere-- lookup the same key in params, credentials, env

Fred-sun · 2020-08-25T01:59:19Z

@UnwashedMeme Could you please help answer the following questions?


1. When subscription_ID is specified in the parameter, this parameter has no effect. (When both sunbscription_ID and auth_source parameters exist, you do not specify a priority in use)

It's my mistake! Cancel this comment!

Fred-sun · 2020-08-25T07:50:46Z

plugins/module_utils/azure_rm_common.py

        if self.is_ad_resource:
            resource = 'https://graph.windows.net/'
-        credentials, subscription_id = get_azure_cli_credentials(resource)
+        subscription_id = subscription_id or self._get_env('subscription_id')


@UnwashedMeme Why do you catch the subscription_id of the environment variable here? auth_source='cli' uses the credentials of 'az loggin', so the environment variable subscription should not be used in this function, please update. Thank you!

why do you catch the subscription_id of the environment variable here?

Because that is a valid way of providing data to these modules and this PR reduces the number of cases in which a user provided subscription_id is unexpectedly ignored.

More broadly "The authentication mechanism (service principal, msi, cli) and parameter passing convention (module parameters, environment variables, profile file) should be as loosely coupled as possible."

I believe you have been looking at this as "There currently several distinct parameter sets that are entirely separate depending upon which auth_source is specified." I don't believe that is true in existing code except by accident and it certainly isn't stated in the module documentation.

The only places subscription_id should be required is when we don't have a mechanism for looking it up associated with the authentication. The service principal authentication code does not use the subscription_id. But since we need to have a subscription_id when creating the mgmt client it is required. When not set explicitly, some mechanisms like MSI and CLI can provide a default. If the module user provides a subscription id through any mechanism it should be respected.

Yes, but if you assign the subscription_id of the environment variable here, then auth_source= cli will not use the credentials of 'az logging', which is contrary to the designed auth_source parameter.

So I'm think here, if you set ’auth_source= cli‘， then you're going to use the 'az login' credentials, and if you can't catch the credentials or if the credentials are wrong, you're going to throw an exception,' az Login 'credentials have a problem. Thank you very much!

if you assign the subscription_id of the environment variable here, then auth_source= cli will not use the credentials of 'az logging'

This is not a problem because subscription_id is not part of the credentials from az login. Please recall that the credentials from az login can grant access to many subscriptions. The subscription_id module parameter or environment variable just allows the user to specify which one this module should actually target.

As with any auth_source, if the user specifies a subscription_id that the credentials aren't authorized for it won't work.

Yes, I'll also update my description, the subscription_id here should get the subscription_id of the playbook parameter, not the one in the environment variable. Thank you very much!

… of a Storage Account. (ansible-collections#108) * Add support for managing the 'Firewall and virtual networks' settings of a Storage Account. Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com> Update azure_rm_publicipaddress add test sample codebase cleanup: update format (ansible-collections#131) * Update format refine azure devops pipeline (ansible-collections#138) * refine azure devops pipeline enable single module test (ansible-collections#144) * enable single module test bug fixing in azure_rm_aks module(ansible-collections#170) * bug fixing in azure_rm_aks module For Test( 2.9 to 2.10) (ansible-collections#140) * adding more testing dimensions Add FileEndpoint to azure_rm_storageaccount_info (ansible-collections#102) * Add FileEndpoint to azure_rm_storageaccount_info Use hasattr instead of dir (ansible-collections#75) Add ephemeral OS disk for azure_rm_virtualmachine (ansible-collections#124) * Add ephemeral OS disk support for azure_rm_virtualmachine Add ephemeral OS disk for azure_rm_virtualmachinescaleset (ansible-collections#128) * Add ephemeral OS disk support for azure_rm_virtualmachinescaleset Change network_client api_version to match latest default api_version (ansible-collections#157) * #Fixes 156 Change network_client api_version to match latest default api_version * #Fixes 156 Update azure-mgmt-network to 10.2.0 Update 'latest' version listing and default version for Network Client to 2018-08-01 Add ability to remove all Subnet Service Endpoints when supplying an empty list. (ansible-collections#148) New azure_rm_privatednszone module with tests (ansible-collections#122) * New azure_rm_privatednszone module with tests Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com> Co-authored-by: Fred-sun <xiuxi.sun@qq.com> Add new azure active directory related modules (ansible-collections#179) * add ad related modules * release v0.2.0 preparation and minor ad module bug fixing fixing sanity testing errors in ad related modules (ansible-collections#182) * disable ad relate module test * fixing sanity testing errors in ad related modules Fixing issues in azure_rm_deployment_info.py (ansible-collections#180) - Fixing getting the template_link when it does not exist. - Feature: Return the correlation_id Co-authored-by: Steve Kieu <steve.kieu@xvt.com.au> azure_rm_virtualmachine: Protect against no diskSizeGB (ansible-collections#185) Disks `diskSizeGB` is not reported if the Virtual Machine is deallocated. Trying to fetch it is causing a `KeyError`. This guards against that KeyError and then nulls out the key so it isn't set incorrectly in the update call. Fix wrong module deprecate statement format (ansible-collections#176) * fix wrong module deprecate statement format update doc add batch upload feature in azure_rm_storageblob (ansible-collections#203) Co-authored-by: haiyuazhang <haiyuan.zhang1982@gmaile.com> fixing status code issue in azure_rm_deployment (ansible-collections#204) Co-authored-by: haiyuazhang <haiyuan.zhang1982@gmaile.com> fixing update check bug in azure_rm_adserviceprincipal (ansible-collections#205) Bump version to v0.3.0 update doc change service_url to service_uri (ansible-collections#212) Storageaccount tlsversion (ansible-collections#207) * add support for minimum_tls_version fix(vm): fix boot diagnistics option "no" caused an error (ansible-collections#200) Fix the issue ansible-collections#158 (ansible-collections#214) * fix issue ansible-collections#158 and add tests azure_rm_appgateway.py: Support Version 2 SKUs (ansible-collections#198) fix sanity test issues (ansible-collections#223) Bump version to v0.4.0 lift v0.4.0 to v1.0.0 update new pr-pipeline (ansible-collections#229) Add execution environment metadata (ansible-collections#220) Storage account allow blob public access parameter (ansible-collections#219) * Storage account allow blob public access parameter * Change name of test to match correct attribute. Update azure_rm_virtualmachine boot_diagnostics resource group config… (ansible-collections#208) * Update azure_rm_virtualmachine boot_diagnostics resource group configuration Update azure_rm_storageaccount.py file (ansible-collections#233) * Uneatable parameters do not need to set default values fix the problem of disk lun self-increment (ansible-collections#237) Fixing rule_type reference (ansible-collections#99) Reference to ev instead of item is preventing the expected rule_type values from functioning Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com> add load_balancer_sku option for aks (ansible-collections#199) * added load_balancer_sku option for aks Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com> Improve OS detection when VM has no osProfile (ansible-collections#197) We have VM without OS profile (checked on https://resources.azure.com/) and they appear as "unknown" os :-/ This appear to be a known issue: https://support.microsoft.com/en-ph/help/4018140/computer-names-of-specialized-virtual-machines-are-missing-or-blank-in Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com> Co-authored-by: haiyuan_zhang <haiyuan.zhang1982@gmail.com> Add IPv6 address for azure_rm_subnet (ansible-collections#240) * Add IPv6 address for azure_rm_subnet Get address_prefixes info from virtualnetwork (ansible-collections#239) * Add new parameter for get subnet IPv6 info Support azure cli credentials with multiple `subscription_id`s (ansible-collections#195) * feat: Support azure cli credentials with multiple `subscription_id`s If the `subscription_id` is specified as module parameter or in the environment then try to find that subscription in either the MSI (existing) or CLI credentials (new). This patch brings those two scenarios in line. * docs: Improve documentation on auth_source * refactor: Move defaults up to class Just trying to make the `__init__` fn a bit slimmer and easier to reason about. * refactor: Use python kwargs instead of passing dict This is an isomorphic change, just using python syntax to accomplish the exact same thing. * refactor: Use ansible builtin `env_fallback` for `auth_source` Ansible modules have a pattern for looking up a module parameter in the environment with precedence of explicit param -> env -> default. Use this pattern to simplify our code here. This shouldn't change any behavior of `auth_source`, just using standard ansible patterns to accomplish it. * style: Split long line No semantic change, just wrapping a long line to be a bit more readable. * refactor: helper fun _get_env Somewhat frequently there is a lookup in the environment for the key that matches a module parameter. This simple helper just encapsulates that to make it a bit easier elsewhere-- lookup the same key in params, credentials, env * fix: typo in log message Co-authored-by: Justin Ossevoort <justin.ossevoort@tesorion.nl> Add 10.3 version for azure_rm_mariadbserver module (ansible-collections#244) * add new version for azure_rm_mariadbserver Update azure_rm_storageaccount yaml (ansible-collections#226) * update azure_rm_storageaccount test * update pipeline file

This was referenced Jul 21, 2020

Modules ignore subscription_id when using CLI auth; resources get created in the wrong subscription. #181

Closed

Support azure cli credentials with multiple subscription_ids #53

Closed

UnwashedMeme force-pushed the subscription_ids branch 2 times, most recently from 76a1021 to 5b8c5f4 Compare August 5, 2020 14:17

UnwashedMeme changed the title ~~WIP: Support azure cli credentials with multiple subscription_ids~~ Support azure cli credentials with multiple subscription_ids Aug 5, 2020

Fred-sun reviewed Aug 6, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

UnwashedMeme force-pushed the subscription_ids branch 2 times, most recently from 64e135c to 61db0d0 Compare August 7, 2020 02:19

UnwashedMeme force-pushed the subscription_ids branch from 61db0d0 to 920e69d Compare August 14, 2020 18:07

Fred-sun reviewed Aug 21, 2020

View reviewed changes

plugins/doc_fragments/azure.py Outdated Show resolved Hide resolved

plugins/doc_fragments/azure.py Outdated Show resolved Hide resolved

plugins/doc_fragments/azure.py Outdated Show resolved Hide resolved

Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Aug 24, 2020

feat: Support azure cli credentials with multiple subscription_ids

de1e16b

If the `subscription_id` is specified as module parameter or in the environment then try to find that subscription in either the MSI (existing) or CLI credentials (new). This patch brings those two scenarios in line.

UnwashedMeme force-pushed the subscription_ids branch from 920e69d to 334405a Compare August 24, 2020 22:03

UnwashedMeme added 7 commits August 24, 2020 18:11

docs: Improve documentation on auth_source

95bfd29

refactor: Move defaults up to class

5cd48e2

Just trying to make the `__init__` fn a bit slimmer and easier to reason about.

refactor: Use python kwargs instead of passing dict

28d2aed

This is an isomorphic change, just using python syntax to accomplish the exact same thing.

style: Split long line

4f2f49a

No semantic change, just wrapping a long line to be a bit more readable.

refactor: helper fun _get_env

7b0ec64

Somewhat frequently there is a lookup in the environment for the key that matches a module parameter. This simple helper just encapsulates that to make it a bit easier elsewhere-- lookup the same key in params, credentials, env

fix: typo in log message

4073146

UnwashedMeme force-pushed the subscription_ids branch from 334405a to 4073146 Compare August 24, 2020 22:13

Fred-sun reviewed Aug 25, 2020

View reviewed changes

haiyuazhang merged commit 7f78fb7 into ansible-collections:dev Sep 1, 2020

victorbrca mentioned this pull request Mar 5, 2021

azure_rm does not honor 'subscription_id' or 'AZURE_SUBSCRIPTION_ID' #445

Closed

36roentgen mentioned this pull request Mar 5, 2021

subscription_id from selected modules doesn't override service princple subscription value #446

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support azure cli credentials with multiple `subscription_id`s #195

Support azure cli credentials with multiple `subscription_id`s #195

UnwashedMeme commented Jul 21, 2020

Fred-sun commented Aug 4, 2020

UnwashedMeme commented Aug 5, 2020

Fred-sun Aug 6, 2020

UnwashedMeme Aug 7, 2020

This comment has been minimized.

UnwashedMeme commented Aug 7, 2020

Fred-sun commented Aug 7, 2020

UnwashedMeme commented Aug 14, 2020

Fred-sun left a comment •

edited

Loading

Fred-sun commented Aug 25, 2020

Fred-sun Aug 25, 2020

UnwashedMeme Aug 25, 2020

Fred-sun Aug 26, 2020

Fred-sun Aug 26, 2020

UnwashedMeme Aug 28, 2020

Fred-sun Aug 28, 2020

Support azure cli credentials with multiple subscription_ids #195

Support azure cli credentials with multiple subscription_ids #195

Conversation

UnwashedMeme commented Jul 21, 2020

SUMMARY

Fred-sun commented Aug 4, 2020

UnwashedMeme commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

UnwashedMeme commented Aug 7, 2020

Fred-sun commented Aug 7, 2020

UnwashedMeme commented Aug 14, 2020

Fred-sun left a comment • edited Loading

Choose a reason for hiding this comment

Fred-sun commented Aug 25, 2020

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

Support azure cli credentials with multiple `subscription_id`s #195

Support azure cli credentials with multiple `subscription_id`s #195

Fred-sun left a comment •

edited

Loading