Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The subscription_id Module Parameter Is Used Contradictively #1223

Open
timway opened this issue Jul 27, 2023 · 4 comments
Open

The subscription_id Module Parameter Is Used Contradictively #1223

timway opened this issue Jul 27, 2023 · 4 comments
Labels
has_pr PR fixes have been made high_priority High priority work in In trying to solve, or in working with contributors

Comments

@timway
Copy link
Contributor

timway commented Jul 27, 2023

SUMMARY

Related to #1218 but broader. Most if not all azure.azcollection modules leverage the subscription_id module argument provided by AzureRMModuleBase class. It has a fallback to the environment variable AZURE_SUBSCRIPTION_ID that is commonly used to inject credentials into playbooks either via ansible-navigator, AWX, or Automation Controller.

https://github.com/ansible-collections/azure/blob/v1.16.0/plugins/module_utils/azure_rm_common.py#L602-L611

In the parse_resource_to_dict method provided by AzureRMModuleBase it looks only to subscription_id provided by AzureRMModuleBase which maps back to self.azure_auth.subscription_id with azure_auth being an instantiation of AzureRMAuth. This creates a binding between any module that has code that interacts with parse_resource_to_dict have the subscription ID used for authentication added to the resource ID it generates (assumes).

https://github.com/ansible-collections/azure/blob/v1.16.0/plugins/module_utils/azure_rm_common.py#L913-L915

On the other hand, modules that use get_mgmt_svc_client method provided by AzureRMModuleBase to get a follow-on management client instantiation. actually look at the module parameter for subscription_id. This creates some modules that are able to use subscription_id to target a module to a subscription not used for authentication and actually get resources created in that target subscription but follow-on plays that try to find that resource by name fail.

As stated below, my expectation coming to the collection and viewing how ansible as a whole functions is that any module parameter provided overrides its fallback method. This is not the case right now and I would disagree with @xuzhang3 opinion in #1218 that separate parameters should be leveraged for this use case. I don't view this bug as an enhancement request but it is possible that fixing it like I propose would have playbook breaking changes so it would warrant a v2.0.0 release in my opinion.

ISSUE TYPE
  • Bug Report
COMPONENT NAME

azure.plugins.module_utils.azure_rm_common.AzureRMModuleBase

ANSIBLE VERSION
ansible [core 2.15.2]
  config file = None
  configured module search path = ['/Users/abcd/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /private/var/folders/1m/3w2m5k795nq38zc8mfzz6p600000gn/T/tmp.KodK6p6K/ansible-navigator/lib/python3.9/site-packages/ansible
  ansible collection location = /Users/abcd/.ansible/collections:/usr/share/ansible/collections
  executable location = /var/folders/1m/3w2m5k795nq38zc8mfzz6p600000gn/T/tmp.KodK6p6K/ansible-navigator/bin/ansible
  python version = 3.9.6 (default, May  7 2023, 23:32:44) [Clang 14.0.3 (clang-1403.0.22.14.1)] (/private/var/folders/1m/3w2m5k795nq38zc8mfzz6p600000gn/T/tmp.KodK6p6K/ansible-navigator/bin/python)
  jinja version = 3.1.2
  libyaml = True
COLLECTION VERSION
Collection         Version   
------------------ ---------- 
azure.azcollection 1.16.0
CONFIGURATION
CONFIG_FILE() = None
GALAXY_SERVER_LIST(env: ANSIBLE_GALAXY_SERVER_LIST) = ['hub_published', 'hub_rhcertified']
OS / ENVIRONMENT

Mac OS on M1, Azure authentication done by fallback environment variables

STEPS TO REPRODUCE
  1. Have 2 subscription IDs
  • The first to use for authentication
  • The second to use for resource creation
  1. Setup to authenticate to Ansible using the fallback environment variables with the first subscription in AZURE_SUBSCRIPTION_ID
  2. Run the playbook below with --extra-vars "sub2=the-second-sub-id"

Set your environment to authenticate to Azure and use a

---
- name: Multiple subscription bugs
  hosts: localhost

  tasks:
    - name: Ensure a resource group exists in subscription 2
      azure.azcollection.azure_rm_resourcegroup:
        location: northeurope
        name: multiple-subscription-issue-rg
        subscription_id: "{{ sub2 }}"

    - name: Ensure a route table exists in subscription 2
      azure.azcollection.azure_rm_routetable:
        name: multiple-subscription-issue-rt
        resource_group: multiple-subscription-issue-rg
        subscription_id: "{{ sub2 }}"

    - name: Ensure a vnet exists for our subnet
      azure.azcollection.azure_rm_virtualnetwork:
        address_prefixes_cidr:
            - 192.0.2.0/24
        dns_servers:
            - 127.0.0.1
            - 127.0.0.2
        name: multiple-subscription-issue-vnet
        resource_group: multiple-subscription-issue-rg
        subscription_id: "{{ sub2 }}"

    - name: Ensure a subnet exists with our route table attached
      azure.azcollection.azure_rm_subnet:
        name: multiple-subscription-issue-snet
        resource_group: multiple-subscription-issue-rg
        route_table: multiple-subscription-issue-rt
        subscription_id: "{{ sub2 }}"
        virtual_network_name: multiple-subscription-issue-vnet
        address_prefixes_cidr:
          - 192.0.2.0/29
EXPECTED RESULTS

That the subscription_id module parameter is more preferred over the fallback of AZURE_SUBSCRIPTION_ID when provided when resolving names to identifiers.

In this particular case, the route_table parameter does accept an ID so I can specify that as a work-around. That said other module parameters like virtual_network_name exist in this module. It is my opinion that all _name parameters should be replaced by a module parameter that is only the resource name, providing an alias back to _name is acceptable. Then all the module parameters should pass through the parse_resource_to_dict method found in AzureRMModuleBase. That method should then be adjusted to respect the module parameter over the fallback for authentication.

ACTUAL RESULTS

The results contain subscription IDs but you will see that the it tries to find the route table in the first subscription, the one used for authentication. Despite it respecting the subscription_id module parameter in the earlier task and creating the route table in the second subscription.

@xuzhang3
Copy link
Collaborator

@timway custom subscription_id is not available for all kind types of authorization. You can custom the subscrioption_id with az CLI authorization but SPN auth cannot

@timway
Copy link
Contributor Author

timway commented Jul 28, 2023

@timway custom subscription_id is not available for all kind types of authorization. You can custom the subscrioption_id with az CLI authorization but SPN auth cannot

My example uses env via auto which my understanding is that's a service principal behind it. I'm able to use subscription_id currently to get resources in a different subscription in some modules but not others. It works that way today because of the code sections I highlighted in my issue. My opinion is that the subscription_id module parameter have the following behavior:

Provided by user directly -> Provided by ENV fallback -> Discovered and assigned as part of auth

Furthermore when referenced in code for doing things like constructing a resource ID it should follow that order of precedence.

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Jul 31, 2023
@grayzu grayzu added high_priority High priority and removed medium_priority Medium priority labels Jul 31, 2023
@Fred-sun Fred-sun added has_pr PR fixes have been made and removed work in In trying to solve, or in working with contributors labels Aug 2, 2023
@Fred-sun Fred-sun added work in In trying to solve, or in working with contributors and removed has_pr PR fixes have been made labels Nov 23, 2023
@Fred-sun
Copy link
Collaborator

@timway Do you want to use 'subscription_id' in the parameter first? Like PR #1380?

@timway
Copy link
Contributor Author

timway commented Jan 31, 2024

@timway Do you want to use 'subscription_id' in the parameter first? Like PR #1380?

Yes, in my opinion 'subscription_id' should act like other Ansible module parameters that have traditional ENV fallback values. If specified by a user in a task it should have precedence over the ENV variable.

I haven't fully reviewed the proposed solution in that PR though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has_pr PR fixes have been made high_priority High priority work in In trying to solve, or in working with contributors
Projects
None yet
Development

No branches or pull requests

4 participants