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

Added option for disabling MSI autodiscover feature in azure_keyvault_secret lookup plugin #956

Closed
wants to merge 82 commits into from
Closed

Added option for disabling MSI autodiscover feature in azure_keyvault_secret lookup plugin #956

wants to merge 82 commits into from

Conversation

nalle
Copy link
Contributor

@nalle nalle commented Aug 19, 2022

SUMMARY

Added an option to be able to disable MSI autodiscover feature.

The default for the module is to assume that the MSI metadata URL is available, which slows down the module considerably when not being able to access that IP.

This way the original intended functionality is preserved and we who want to use it without MSI available can do so without an almost 30s penalty.

Some things had to be moved around due to where the options kwarg is available.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure.azcollection.azure_keyvault_secret

ADDITIONAL INFORMATION

Below follows a paste from running with and without MSI autodiscover enabled
With MSI autodiscovery without MSI metadata host:

PLAY [localhost] ***************************************************************************************************************************************************************************

TASK [debug] *******************************************************************************************************************************************************************************
Friday 19 August 2022  09:35:14 +0200 (0:00:00.016)       0:00:00.016 *********
Your credentials class does not support session injection. Performance will not be at the maximum.
ok: [localhost] => {
    "msg": "the value of this secret is <redacted>"
}

PLAY RECAP *********************************************************************************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Friday 19 August 2022  09:35:42 +0200 (0:00:28.608)       0:00:28.624 *********
===============================================================================
debug ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ 28.61s
Playbook run took 0 days, 0 hours, 0 minutes, 28 seconds

Without MSI autodiscovery enabled:

PLAY [localhost] ***************************************************************************************************************************************************************************

TASK [debug] *******************************************************************************************************************************************************************************
Friday 19 August 2022  09:35:06 +0200 (0:00:00.019)       0:00:00.019 *********
Your credentials class does not support session injection. Performance will not be at the maximum.
ok: [localhost] => {
    "msg": "the value of this secret is <redacted>"
}

PLAY RECAP *********************************************************************************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

Friday 19 August 2022  09:35:07 +0200 (0:00:00.920)       0:00:00.939 *********
===============================================================================
debug ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 0.92s
Playbook run took 0 days, 0 hours, 0 minutes, 0 seconds

@nalle nalle changed the title Added option for disabling MSI autodiscover feature Added option for disabling MSI autodiscover feature in azure_keyvault_secret lookup plugin Aug 19, 2022
@Fred-sun
Copy link
Collaborator

@nalle Could you please help to provide test cases? It helps to recommend a PR merger. Thank you very much!

@Fred-sun Fred-sun added medium_priority Medium priority work in In trying to solve, or in working with contributors labels Sep 30, 2022
@nalle
Copy link
Contributor Author

nalle commented Sep 30, 2022

Is this something like what you're asking for?

Using MSI autodiscovery

  connection: local
  vars:
    azure_keyvault_url: https://something.vault.azure.net
    azure_client_id: xxxxxx-xxxx-xxxxx-xxxxx
    azure_secret: xxxxxx-xxx-xxxxx-xxxxxx
    azure_tenant_id: xxxxxxx-xxxx-xxxxx-xxxxx-
  tasks:
    - debug:
        msg: "{{ lookup('azure.azcollection.azure_keyvault_secret','secretname',vault_url=azure_keyvault_url, client_id=azure_client_id, secret=azure_secret, tenant_id=azure_tenant_id) }}

Without MSI autodiscovery

  connection: local
  vars:
    azure_keyvault_url: https://something.vault.azure.net
    azure_client_id: xxxxxx-xxxx-xxxxx-xxxxx
    azure_secret: xxxxxx-xxx-xxxxx-xxxxxx
    azure_tenant_id: xxxxxxx-xxxx-xxxxx-xxxxx-
  tasks:
    - debug:
        msg: "{{ lookup('azure.azcollection.azure_keyvault_secret','secretname',vault_url=azure_keyvault_url, client_id=azure_client_id, secret=azure_secret, tenant_id=azure_tenant_id, use_msi=False) }}

Fred-sun and others added 24 commits May 31, 2023 16:00
* Add the method parameter to specify how http gets information

* Change default value
…ugin (#1161)

* Added azure cli credential support in the azure_keyvault_secret.py plugin

* Fix sanity error

* Update azure_keyvault_secret.py
* Bump verison to v1.16.0

* update CHANGELOG.md

* update CHANGELOG.md

---------

Co-authored-by: xuzhang3 <Zhangxu894765>
Co-authored-by: xuzhang3 <Zhangxu894765>
…Assigned identities (#1177)

* Changed vm_identity parameter to support User Assigned Managed Identities

First version, still not complete. Working:
* change UserAssigned for SystemAssigned
* add UserAssigned to SystemAssigned
* Remove SystemAssigned and leave UserAssigned

* Added some functionality, not complete yet:

* Tested None vm_identity (remove identity)
* Checks pressence of 'user_assigned_identities' when UserAssigned 'type' is used

* Tests and append User Assigned Identities

* user_assigned_identities has two fields: id (list) and append (bool)
* you can append new managed identities to existing with append=True
* you can add SystemAssigned without removing current appended UserAssigned

* Append user assigned identity to another not working

* Working version with tests

Tasks to do
* Review and clean up
* Add comments
* Leave test main.yaml as it was
* Modify inventory to use a custom subnet

* Clean up and review

* Commented code
* Removed self.log sentences
* Set options defaults
* hosts of tests of azure_rm_virtualmachine changed back to 'all'

* Create managed identities during test

* Changed boolean comparisons from '=='  to 'is'

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>

* Apply suggestions from code review

Code style changes

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>

* Changed lines length as suggested by @Fred-sun

* Fixed line length

* Remove whitespace

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>

---------

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
* Update azure_rm_manageddisk.py

appears to be copy/paste error with storage/source for storage_account_id parameter 

Error creating the managed disk: (BadRequest) Required parameter 'storageAccountId' is missing (null).
Code: BadRequest
Message: Required parameter 'storageAccountId' is missing (null).

* Update azure_rm_multiplemanageddisks.py
* new change

* Update azure_rm_keyvaultsecret module

* Improve the azure_rm_keyvaultkey_info code

* new change

* Improve azure_rm_keyvaultkey.py code

* Improve azure_rm_keyvaultsecret_info.py

* Improve azure_rm_keyvaultsecret.py code

* Update lookup file azure_keyvault_secret.py

* fix sanity fail

* last sanity fail update
* Refs #1033 fix security profile for virtualimachine_info

* Update plugins/modules/azure_rm_virtualmachine_info.py

Fix indentation

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>

* Update plugins/modules/azure_rm_virtualmachine_info.py

Fix indentation

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>

---------

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
* Fixes #993 allow to set boot diagnostics storage account to managed

* Fix changing boot diagnostics if boot diagnostics are disabled

* fix blank space

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>

* Introduce new var type for boot diagnostics

* Fix type for default case of creating/preexisting storage account

* Fix managed boot diagnostics for vm creation -- include freds changes

* Adress review, add mutually exclusive description

* Fix argspec for mutually exvlusiveness inside a suboption

thanks to @flowerysong for explaining it :)

* boot_diagnostics.enabled default is undefined

* Fix sanity tests

* Adress freds review

* Add single newline at the end

---------

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
Co-authored-by: Marges, RSY (Rick) <rick.marges@achmea.nl>
* Improve Docs In `azure_rm_virtualnetwork`

* The DNS Servers should be a list of strings
* Remove the trailing comma in the `dict()` call for `dns_servers`
* The boolean values should default to a boolean value in the docs
* Clean up excess quotation in some of the examples

* Set the Argument Spec up to Validate List Element Typing

* Incorporate PR feedback from @Fred-sun correctly indicated that I didn't apply the elements parameter to the argument spec for `address_prefixes_cidr` and `dns_servers`
* This helps Ansible validate the list elements are the correct type for the 2 affected module parameters

* Remove Unneeded Ignores In Sanity

* As @Fred-sun pointed out a number of the sanity tests no longer need to ignore a failure condition with this PR
* Add new parameters to azure_rm_storageshare.py

* Add test case

* Fix sanity fail

* small change

* small change 02

* remove test case

* small change
Changed update_security_profle for update_security_profile
)

* use module sub_id if definied

* use module sub_id if definied

* lint
xuzhang3 and others added 7 commits November 14, 2023 13:14
…`Flexible` (#1331)

* orchestration_mode defaults to Flexible

* update VMSS test case
* Add vm_agent_version to output

* Apply suggestions from code review

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>

---------

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
* Add os_disk_encryption_set to azure_rm_virtualmachine

Add the parameter `os_disk_encryption_set` to the
`azure_rm_virtualmachine` module, making it possible to specify which
DES to use when encrypting the OS disk for a newly created VM.

* Improved checking update of DES for OSDisk

To ensure that we won't try to *add* or *change* DES on an existing VM,
the logic to check existing DES setting and comparing it to the desired
state is fixed to handle both cases.

The code is also polished to make it more readable.

* Add integration tests

Add a new kind of virtual machine tests
which creates VMs with encrypted OS disk
using the new property `os_disk_encryption_set`.

I also added the generated inventory to
`.gitignore` to avoid comitting it by mistake.

* Polish the code

* Fix os disk encryption in tests

Ensure that the OS disk is encrypted in the integration tests.

This requires the DES to be granted access to the KeyVault, and
that the KeyVault is protected against purges. To avoid that we
get conflicts with previously deleted, but unpurged KVs, they are
named with a date-based suffix.
)

* Add disk_encryption_set for data disks in azure_rm_virtualmachine

Add the parameter `data_disks.disk_encryption_set` to the
`azure_rm_virtualmachine` module, making it possible to specify which
DES to use when encrypting the data disk.

This is required when creating a VM (with data disks) from an encrypted
image which resides in another landing zone.

* Add disk_encryption_set to arg spec
… file (#1326)

* Move azure_service_principal_attribute.py to azure-collecitons lookup file

* remove common azure_service_principal_attribute.py

* fix sanity error

* fix sanity error -- 02

* fix sanity error -- 02

* fix sanity error --- 03

* Adjust test order

* Modify azure_service_principal_attribute.py

* fix sanity error

* fix sanity error 02

* Add parameters authority
* Delete trunck1 process

* small change

* Delete parameters is_track2

* Update requirements-azure.txt

* Delete  tests/integration/targets/azure_rm_virtualmachine/lookup_plugins/azure_service_principal_attribute.py
@TiTi
Copy link
Contributor

TiTi commented Nov 16, 2023

This would solve my #1297

Fred-sun and others added 10 commits November 17, 2023 11:27
* Bump version to v2.0.0

* update CHANGELOG.md

* update CHANGELOG.md

---------

Co-authored-by: xuzhang3 <xuzhang3@microsoft.com>
This adds a module to retrieve an access token for Azure.
* Update azure_rm_iotdevice*.py, Fix getting alias parameters

* Update the method corresponding to 'self.auth_method'

* Disable azure_rm_iothub test

* restore azure_rm_iothub.py test case
Add azure_rm_openshiftmanagedclusterkubeconfig_info module to fetch
kubeconfig from the ARO cluster.
The kubeconfig file could be fetched and registered or saved into a
specified file.

Example usage:
```
- name: Obtain kubeconfig file of ARO cluster
  azure_rm_openshiftmanagedclusterkubeconfig_info:
    name: myCluster
    resource_group: myResourceGroup
  register: kubeconf

- name: Print registered kubeconfig file
  debug:
    msg: "{{ kubeconf['kubeconfig'] }}"

- name: Fetch kubeconfig and save it as mycluster_kubeconfig filename
  azure_rm_openshiftmanagedclusterkubeconfig_info:
    name: myCluster
    resource_group: myResourceGroup
    path: ./files/mycluster_kubeconfig

- name: Fetch kubeconfig and save it to specified directory (file will be named as kubeconfig by default)
  azure_rm_openshiftmanagedclusterkubeconfig_info:
    name: myCluster
    resource_group: myResourceGroup
    path: ./files/
```
* Fix az login credentials error

* Fix sanity error

* Fix sanity error

* fix sanity error 02

* fix santiy error 03

* fix sanity error 04

* Add removed_from_collection and remove_in_version

* remove document define

* Modify deprecate version to v3.0.0

* New change for deprecated attribute

* small change

* restore change

* Fix sanity error

* Add missing ','
* Support azure_rm_sshpublickey module to managed VM public keys

* fix sanity fail

* update test data

* Modify version to v2.0.0

* Fix lint error

* Fix ansible-lint check errors

---------

Co-authored-by: Zooopx <xuzhang3@microsoft.com>
* Add large_file_shares_state to azure_rm_storageaccount

* small change
@Fred-sun
Copy link
Collaborator

@nalle There are some conflicts in this PR, can you help solve the conflicts? We will review and proceed with the merger as soon as possible. Thank you!

@Fred-sun Fred-sun added the question Further information is requested label Nov 24, 2023
tomaxsas and others added 2 commits November 24, 2023 16:33
Return response body if its not json
…ent (#1231)

* fix azure_rm_deployment.py bug

* small change
@Fred-sun
Copy link
Collaborator

Kindly ping!

@Fred-sun
Copy link
Collaborator

Fred-sun commented Dec 1, 2023

@nalle Are you still paying attention to this PR? There is a conflict in this PR, can you help solve the conflict? I will push forward the merger of this PR as soon as possible, thank you!

@nalle
Copy link
Contributor Author

nalle commented Dec 1, 2023

Eh.. I tried to update the fork and resolve the conflict but it looks like I didn't really get it right

@Fred-sun
Copy link
Collaborator

Fred-sun commented Dec 1, 2023

@nalle Thee're a lot of code changed, and if it doesn't work, what about closing this one and resubmitting it? Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority question Further information is requested work in In trying to solve, or in working with contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.