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.

Sign up for GitHub

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

Jump to bottom

azure_rm_virtualmachine: better support for User Assigned and System Assigned identities #1177

Merged
merged 13 commits into from
Jun 15, 2023
Merged

azure_rm_virtualmachine: better support for User Assigned and System Assigned identities #1177

merged 13 commits into from
Jun 15, 2023

Conversation

cpiment
Copy link
Contributor

@cpiment cpiment commented Jun 2, 2023 •
edited

SUMMARY

Allows to set/unset User and System managed identities to Virtual Machines

Fixes #1173

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm_virtualmachine

ADDITIONAL INFORMATION

Before

- name: Create minimal VM with UserAssigned Identity 
  azure_rm_virtualmachine:
    allocated: no
    resource_group: "{{ resource_group }}"
    name: "{{ vm_name }}"
    vm_identity: 'SystemAssigned'
    admin_username: "testuser"
    ssh_password_enabled: false    
    public_ip_allocation_method: Disabled
    location: westeurope
    network_interface_names:
      - name: "{{ interface_name }}-identity"
        resource_group: "{{ resource_group }}"
    ssh_public_keys:
      - path: /home/testuser/.ssh/authorized_keys
        key_data: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDfoYlIV4lTPZTv7hXaVwQQuqBgGs4yeNRX0SPo2+HQt9u4X7IGwrtXc0nEUm6LfaCikMH58bOL8f20NTGz285kxdFHZRcBXtqmnMz2rXwhK9gwq5h1khc+GzHtdcJXsGA4y0xuaNcidcg04jxAlN/06fwb/VYwwWTVbypNC0gpGEpWckCNm8vlDlA55sU5et0SZ+J0RKVvEaweUOeNbFZqckGPA384imfeYlADppK/7eAxqfBVadVvZG8IJk4yvATgaIENIFj2cXxqu2mQ/Bp5Wr45uApvJsFXmi+v/nkiOEV1QpLOnEwAZo6EfFS4CCQtsymxJCl1PxdJ5LD4ZOtP xiuxi.sun@qq.com"
    vm_size: Standard_D4s_v3
    virtual_network: "{{ network_name }}-identity"
    image:
      offer: UbuntuServer
      publisher: Canonical
      sku: 16.04-LTS
      version: latest

Now

- name: Create minimal VM with UserAssigned Identity 
  azure_rm_virtualmachine:
    allocated: no
    resource_group: "{{ resource_group }}"
    name: "{{ vm_name }}"
    vm_identity:
      type: UserAssigned
      user_assigned_identities:
        id:
        - "{{ user_identity_1 }}"
    admin_username: "testuser"
    ssh_password_enabled: false    
    public_ip_allocation_method: Disabled
    location: westeurope
    network_interface_names:
      - name: "{{ interface_name }}-identity"
        resource_group: "{{ resource_group }}"
    ssh_public_keys:
      - path: /home/testuser/.ssh/authorized_keys
        key_data: "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDfoYlIV4lTPZTv7hXaVwQQuqBgGs4yeNRX0SPo2+HQt9u4X7IGwrtXc0nEUm6LfaCikMH58bOL8f20NTGz285kxdFHZRcBXtqmnMz2rXwhK9gwq5h1khc+GzHtdcJXsGA4y0xuaNcidcg04jxAlN/06fwb/VYwwWTVbypNC0gpGEpWckCNm8vlDlA55sU5et0SZ+J0RKVvEaweUOeNbFZqckGPA384imfeYlADppK/7eAxqfBVadVvZG8IJk4yvATgaIENIFj2cXxqu2mQ/Bp5Wr45uApvJsFXmi+v/nkiOEV1QpLOnEwAZo6EfFS4CCQtsymxJCl1PxdJ5LD4ZOtP xiuxi.sun@qq.com"
    vm_size: Standard_D4s_v3
    virtual_network: "{{ network_name }}-identity"
    image:
      offer: UbuntuServer
      publisher: Canonical
      sku: 16.04-LTS
      version: latest

@cpiment
Changed vm_identity parameter to support User Assigned Managed Identi…
6615a97 
…ties

First version, still not complete. Working:
* change UserAssigned for SystemAssigned
* add UserAssigned to SystemAssigned
* Remove SystemAssigned and leave UserAssigned
@cpiment
Added some functionality, not complete yet:
fe79727 
* Tested None vm_identity (remove identity)
* Checks pressence of 'user_assigned_identities' when UserAssigned 'type' is used
@cpiment
Tests and append User Assigned Identities
aeeb1d8 
* 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
@cpiment
Append user assigned identity to another not working
a8be2db
@cpiment
Working version with tests
4650a19 
Tasks to do
* Review and clean up
* Add comments
* Leave test main.yaml as it was
* Modify inventory to use a custom subnet
@cpiment
Clean up and review
4c569ba 
* Commented code
* Removed self.log sentences
* Set options defaults
* hosts of tests of azure_rm_virtualmachine changed back to 'all'
@cpiment
Merge branch 'ansible-collections:dev' into dev
920e5c2
@cpiment
Create managed identities during test
42cfb09
@cpiment
Copy link
Contributor Author

cpiment commented Jun 5, 2023

I have found out that the user managed identities can be created during the tests using azure_rm_resource module, so I have modified the tests and edited the PR to remove the prerrequisites for the testing to succeed.

Fred-sun
Fred-sun reviewed Jun 6, 2023
View reviewed changes
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
@Fred-sun
Copy link
Collaborator

Fred-sun commented Jun 6, 2023

@cpiment Please! Thanks you very much!

@Fred-sun Fred-sun added medium_priority Medium priority new_feature New feature requirments labels Jun 6, 2023
@cpiment @Fred-sun
Changed boolean comparisons from '==' to 'is'
840933c 
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
@cpiment
Copy link
Contributor Author

cpiment commented Jun 6, 2023

@Fred-sun I have just commited your changes, thank you so much!

Fred-sun
Fred-sun reviewed Jun 6, 2023
View reviewed changes
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated Show resolved Hide resolved
plugins/modules/azure_rm_virtualmachine.py Outdated
elif 'UserAssigned' in self.vm_identity.get('type') and len(self.vm_identity.get('user_assigned_identities',{}).get('id',[])) == 0:
# Fail if append is False
if vm_identity_user_assigned_append is False:
self.fail("UserAssigned specified but no User Assigned IDs provided and no UserAssigned identities are currently assigned to the VM")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too long (165 characters)

Suggested change
self.fail("UserAssigned specified but no User Assigned IDs provided and no UserAssigned identities are currently assigned to the VM")
self.fail("UserAssigned specified but no User Assigned IDs provided and no UserAssigned identities are currently assigned to the VM")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have divided this line in two. Do you need me to rephrase the error message? If code reaches this point, the module arguments specify that a User Assigned Managed Identity has to be appended to the VM, but there aren't identities in the module arguments nor in the current VM configuration

plugins/modules/azure_rm_virtualmachine.py Outdated
# Fail if append is False
if vm_identity_user_assigned_append is False:
self.fail("UserAssigned specified but no User Assigned IDs provided and no UserAssigned identities are currently assigned to the VM")
# If append is true, user is changing from 'UserAssigned' to 'SystemAssigned, UserAssigned' and wants to keep current UserAssigned identities
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too long (169 characters)

Suggested change
# If append is true, user is changing from 'UserAssigned' to 'SystemAssigned, UserAssigned' and wants to keep current UserAssigned identities
# If append is true, user is changing from 'UserAssigned' to 'SystemAssigned, UserAssigned' and wants to keep current UserAssigned identities

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have divided this line in two

cpiment and others added 2 commits June 6, 2023 19:00
@cpiment @Fred-sun
Apply suggestions from code review
904a73d 
Code style changes

Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
@cpiment
Changed lines length as suggested by @Fred-sun
34822c6
Fred-sun
Fred-sun reviewed Jun 7, 2023
View reviewed changes
plugins/modules/azure_rm_virtualmachine.py Outdated
@@ -1813,6 +1897,49 @@ def exec_module(self, **kwargs):
if self.license_type is not None:
vm_resource.license_type = self.license_type

if self.vm_identity is not None:
# If 'append' is set to True save current user assigned managed identities to use later
if self.vm_identity.get('user_assigned_identities', {}) is not None and self.vm_identity.get('user_assigned_identities', {}).get('append', False) is True:
Copy link
Collaborator

Choose a reason for hiding this comment

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

line too long (178 > 160 characters)

Suggested change
if self.vm_identity.get('user_assigned_identities', {}) is not None and self.vm_identity.get('user_assigned_identities', {}).get('append', False) is True:
if self.vm_identity.get('user_assigned_identities', {}) is not None and self.vm_identity.get('user_assigned_identities', {}).get('append', False) is True:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have splitted this line in two

@cpiment @Fred-sun
Remove whitespace
3e4fac0 
Co-authored-by: Fred-sun <37327967+Fred-sun@users.noreply.github.com>
@cpiment
Copy link
Contributor Author

cpiment commented Jun 12, 2023

Hi @Fred-sun

Is there anything else to review?

Thanks!

@Fred-sun
Copy link
Collaborator

@cpiment Perfact! I will push for merged! Thank you very much!

@Fred-sun Fred-sun added the ready_for_review The PR has been modified and can be reviewed and merged label Jun 13, 2023
@xuzhang3
Copy link
Collaborator

@cpiment LGTM

@xuzhang3 xuzhang3 merged commit ef944d9 into ansible-collections:dev Jun 15, 2023
@cpiment cpiment mentioned this pull request Jun 19, 2023
Merged
@arughrat arughrat mentioned this pull request Sep 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fred-sun Fred-sun Fred-sun left review comments

@xuzhang3 xuzhang3 xuzhang3 approved these changes

Assignees

@xuzhang3 xuzhang3

Labels
medium_priority Medium priority new_feature New feature requirments ready_for_review The PR has been modified and can be reviewed and merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

azure_rm_virtualmachine: Ability to modify/unset User Assigned and System Assigned identities on VMs
3 participants
@cpiment @Fred-sun @xuzhang3