Add os_disk_encryption_set to azure_rm_virtualmachine #1306
Conversation
Fred-sun
added
medium_priority
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.
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.
ephracis
force-pushed
the
add-virtualmachine-os-encryption-set
branch
from
3b4a2e6
to
50869e6
Compare
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.
|
I added some integration tests to use the new disk encryption which works fine. |
tests/integration/targets/azure_rm_virtualmachine/tasks/setup_des.yml
Outdated
Show resolved
Hide resolved
| @@ -1559,6 +1565,11 @@ def exec_module(self, **kwargs): | |||
| vm_dict['os_profile']['linux_configuration']['disable_password_authentication']: | |||
| self.fail("(PropertyChangeNotAllowed) Changing property 'linuxConfiguration.disablePasswordAuthentication' is not allowed.") | |||
|
|
|||
| current_os_des_id = vm_dict['storage_profile'].get('os_disk', {}).get('managed_disk', {}).get('disk_encryption_set', {}).get('id', {}) | |||
| if self.os_disk_encryption_set is not None and current_os_des_id is not None: | |||
There was a problem hiding this comment.
| if self.os_disk_encryption_set is not None and current_os_des_id is not None: | |
| if self.os_disk_encryption_set is not None and current_os_des_id is not None and self.os_disk_encryption_set != current_os_des_id: |
@ephracis Yes, I see. But os_disk only works if managed_disk_type is not None（Please check lines 1704 through 1711）, and managed_disk_type is not specified in your use case, so it works fine. Thank you! |
Good catch! I've added
Unfortunately I don't see any Ansible module for doing this. Any suggestions? |
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.
|
Ok, I think I've managed to get the integration tests running. The problem was that I had to add the DES to the access list. Then I ran into a second issue where I had to enable purge protection in order to use the KV for disk encryption. This means that all KVs will reside deleted but unpurged for 90 days. So in order to not get naming conflicts with these keys, they are given a date-based suffix. Let me know if there's anything else that needs fixing. 😅 |
Fred-sun
added
ready_for_review
|
@ephracis LGTM |
SUMMARY
Add the parameter
os_disk_encryption_setto theazure_rm_virtualmachinemodule, making it possible to specify which DES to use when encrypting the OS disk for a newly created VM.ISSUE TYPE
COMPONENT NAME
azure_rm_virtualmachine
ADDITIONAL INFORMATION
This is required when deploying a VM into one landing zone, using an encrypted Compute Gallery Image from another landing zone. If this is not set it will try to use the DES of the image which will fail since it belongs to another LZ.