Support `EncryptionAtRestWithPlatformAndCustomerKeys` in disk encryption #14218

myc2h6o · 2021-11-17T06:52:40Z

r\disk_encrytpion_set: Add new property encryption_type
- By default the value is EncryptionAtRestWithCustomerKey, and user can specify EncryptionAtRestWithPlatformAndCustomerKeys to enable double encryption, which uses both customer key and platform key.
- This property cannot be updated although it is an settable in update api, thus setting ForceNew
r\managed_disk, r\linux_virtual_machine, r\windows_virtual_machine: Auto-detect encryptionType from disk_encryption_set_id
- Earlier encryptionType is always EncryptionAtRestWithCustomerKey, which throws an error when the encryption type of the corresponding encryption set is EncryptionAtRestWithPlatformAndCustomerKeys. In this change, when disk_encryption_set_id is specified, the encryptionType will be retrieved from the encryption set so that it can support both encryption types.
r\managed_disk: Update ValidateFunc from azure.ValidateResourceID to validate.DiskEncryptionSetID
Test result
Tested with TestAccDiskEncryptionSet_|TestAccManagedDisk_|TestAccLinuxVirtualMachine_|TestAccWindowsVirtualMachine_. 196/200 tests pass, failed tests fail at main as well

…tWithPlatformAndCustomerKeys`

stephybun

Hi @myc2h6o, thanks for this PR! Overall this look goods, I have a few suggestions though.

stephybun · 2021-11-17T08:13:34Z

internal/services/compute/managed_disk_resource.go

@@ -344,8 +344,19 @@ func resourceManagedDiskCreate(d *pluginsdk.ResourceData, meta interface{}) erro
 	}

 	if diskEncryptionSetId := d.Get("disk_encryption_set_id").(string); diskEncryptionSetId != "" {
+		diskEncryptionSet, err := parse.DiskEncryptionSetID(diskEncryptionSetId)
+		if err != nil {
+			return fmt.Errorf("`disk_encryption_set_id` is not a valid disk encryption set id")


The error returned from parse.DiskEncryptionSetID contains some really useful information on why the id couldn't be parsed so I think it would be more helpful to return that in addition/instead of an error message

Suggested change

return fmt.Errorf("`disk_encryption_set_id` is not a valid disk encryption set id")

return err

Updated and moved to new method retrieveDiskEncryptionSetEncryptionType in disk_encryption_set.go

stephybun · 2021-11-17T08:18:55Z

internal/services/compute/managed_disk_resource.go

+		diskEncryptionSetClient := meta.(*clients.Client).Compute.DiskEncryptionSetsClient
+		resp, err := diskEncryptionSetClient.Get(ctx, diskEncryptionSet.ResourceGroup, diskEncryptionSet.Name)
+		if err != nil {
+			return fmt.Errorf("reading Disk Encryption Set %q (Resource Group %q): %+v", diskEncryptionSet.Name, diskEncryptionSet.ResourceGroup, err)


We could shorten the error message here since that info is contained within the parsed resource ID

Suggested change

return fmt.Errorf("reading Disk Encryption Set %q (Resource Group %q): %+v", diskEncryptionSet.Name, diskEncryptionSet.ResourceGroup, err)

return fmt.Errorf("reading %s: %+v", *diskEncryptionSet, err)

Updated in the new method

stephybun · 2021-11-17T08:20:27Z

internal/services/compute/managed_disk_resource.go

@@ -537,8 +548,19 @@ func resourceManagedDiskUpdate(d *pluginsdk.ResourceData, meta interface{}) erro
 	if d.HasChange("disk_encryption_set_id") {
 		shouldShutDown = true
 		if diskEncryptionSetId := d.Get("disk_encryption_set_id").(string); diskEncryptionSetId != "" {
+			diskEncryptionSet, err := parse.DiskEncryptionSetID(diskEncryptionSetId)
+			if err != nil {
+				return fmt.Errorf("`disk_encryption_set_id` is not a valid disk encryption set id")


Same comment here as above

stephybun · 2021-11-17T08:20:48Z

internal/services/compute/managed_disk_resource.go

+			diskEncryptionSetClient := meta.(*clients.Client).Compute.DiskEncryptionSetsClient
+			resp, err := diskEncryptionSetClient.Get(ctx, diskEncryptionSet.ResourceGroup, diskEncryptionSet.Name)
+			if err != nil {
+				return fmt.Errorf("reading Disk Encryption Set %q (Resource Group %q): %+v", diskEncryptionSet.Name, diskEncryptionSet.ResourceGroup, err)


Same here as above

stephybun · 2021-11-17T08:21:25Z

website/docs/r/disk_encryption_set.html.markdown

@@ -107,6 +107,8 @@ The following arguments are supported:

 * `auto_key_rotation_enabled` - (Optional) Boolean flag to specify whether Azure Disk Encryption Set automatically rotates encryption Key to latest version. Defaults to `false`.

+* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Allowed values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.


Suggested change

* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Allowed values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.

* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Accepted values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.

We've been using Possible in most cases fwiw:

Suggested change

* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Allowed values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.

* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Possible values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.

stephybun · 2021-11-17T08:59:24Z

internal/services/compute/disk_encryption_set.go

+
+	resp, err := client.Get(ctx, diskEncryptionSet.ResourceGroup, diskEncryptionSet.Name)
+	if err != nil {
+		return nil, err


I think it makes sense to specify an error occurred while attempting to retrieve a disk encryption set

Suggested change

return nil, err

return nil, fmt.Errorf("retrieving %s: %+v", *diskEncryptionSet, err)

yes make sense, adding retrieving %s to the error message

stephybun · 2021-11-17T09:00:16Z

internal/services/compute/linux_virtual_machine_resource.go

@@ -1089,12 +1089,17 @@ func resourceLinuxVirtualMachineUpdate(d *pluginsdk.ResourceData, meta interface
 			diskName := d.Get("os_disk.0.name").(string)
 			log.Printf("[DEBUG] Updating encryption settings of OS Disk %q for Linux Virtual Machine %q (Resource Group %q) to %q..", diskName, id.Name, id.ResourceGroup, diskEncryptionSetId)

+			encryptionType, err := retrieveDiskEncryptionSetEncryptionType(ctx, meta.(*clients.Client).Compute.DiskEncryptionSetsClient, diskEncryptionSetId)
+			if err != nil {
+				return fmt.Errorf("retrieving encryption type from disk encryption set %q: %+v", diskEncryptionSetId, err)


And then here to just return the error

Suggested change

return fmt.Errorf("retrieving encryption type from disk encryption set %q: %+v", diskEncryptionSetId, err)

return err

stephybun · 2021-11-17T09:05:09Z

internal/services/compute/managed_disk_resource.go

@@ -344,8 +344,13 @@ func resourceManagedDiskCreate(d *pluginsdk.ResourceData, meta interface{}) erro
 	}

 	if diskEncryptionSetId := d.Get("disk_encryption_set_id").(string); diskEncryptionSetId != "" {
+		encryptionType, err := retrieveDiskEncryptionSetEncryptionType(ctx, meta.(*clients.Client).Compute.DiskEncryptionSetsClient, diskEncryptionSetId)
+		if err != nil {
+			return fmt.Errorf("retrieving encryption type from disk encryption set %q: %+v", diskEncryptionSetId, err)


stephybun · 2021-11-17T09:06:28Z

internal/services/compute/managed_disk_resource.go

@@ -537,8 +542,13 @@ func resourceManagedDiskUpdate(d *pluginsdk.ResourceData, meta interface{}) erro
 	if d.HasChange("disk_encryption_set_id") {
 		shouldShutDown = true
 		if diskEncryptionSetId := d.Get("disk_encryption_set_id").(string); diskEncryptionSetId != "" {
+			encryptionType, err := retrieveDiskEncryptionSetEncryptionType(ctx, meta.(*clients.Client).Compute.DiskEncryptionSetsClient, diskEncryptionSetId)
+			if err != nil {
+				return fmt.Errorf("retrieving encryption type from disk encryption set %q: %+v", diskEncryptionSetId, err)


stephybun · 2021-11-17T09:06:39Z

internal/services/compute/windows_virtual_machine_resource.go

@@ -1126,12 +1126,17 @@ func resourceWindowsVirtualMachineUpdate(d *pluginsdk.ResourceData, meta interfa
 			diskName := d.Get("os_disk.0.name").(string)
 			log.Printf("[DEBUG] Updating encryption settings of OS Disk %q for Windows Virtual Machine %q (Resource Group %q) to %q..", diskName, id.Name, id.ResourceGroup, diskEncryptionSetId)

+			encryptionType, err := retrieveDiskEncryptionSetEncryptionType(ctx, meta.(*clients.Client).Compute.DiskEncryptionSetsClient, diskEncryptionSetId)
+			if err != nil {
+				return fmt.Errorf("retrieving encryption type from disk encryption set %q: %+v", diskEncryptionSetId, err)


tombuildsstuff

hi @myc2h6o

Thanks for this PR.

I've taken a look through and left some comments line in addition to @stephybun's review - if we can fix up both sets of comments then we should be able to take another look.

Thanks!

tombuildsstuff · 2021-11-17T09:40:47Z

internal/services/compute/disk_encryption_set_resource.go

+				ValidateFunc: validation.StringInSlice([]string{
+					string(compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey),
+					string(compute.DiskEncryptionSetTypeEncryptionAtRestWithPlatformAndCustomerKeys),
+				}, true),


this should be case-sensitive:

Suggested change

}, true),

}, false),

tombuildsstuff · 2021-11-17T09:42:00Z

internal/services/compute/disk_encryption_set_resource.go

@@ -203,6 +216,7 @@ func resourceDiskEncryptionSetRead(d *pluginsdk.ResourceData, meta interface{})
 		}
 		d.Set("key_vault_key_id", keyVaultKeyId)
 		d.Set("auto_key_rotation_enabled", props.RotationToLatestKeyVersionEnabled)
+		d.Set("encryption_type", props.EncryptionType)


this won't be returned from the API for older resources - so we'll need to default that in code:

Suggested change

d.Set("encryption_type", props.EncryptionType)

encryptionType := string(compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey)

if props.EncryptionType != "" {

encryptionType = string(props.EncryptionType)

}

d.Set("encryption_type", encryptionType)

Updated to set default value

tombuildsstuff · 2021-11-17T09:42:07Z

internal/services/compute/disk_encryption_set_resource.go

+				Type:     pluginsdk.TypeString,
+				Optional: true,
+				ForceNew: true,
+				Default:  compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey,


this should be a string:

Suggested change

Default: compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey,

Default: string(compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey),

thanks for pointing out, updated

tombuildsstuff · 2021-11-17T10:53:45Z

internal/services/compute/disk_encryption_set_resource_test.go

+		{
+			Config: r.withEncryptionTypeDefault(data),
+			Check: acceptance.ComposeTestCheckFunc(
+				check.That(data.ResourceName).ExistsInAzure(r),
+				check.That(data.ResourceName).Key("encryption_type").HasValue("EncryptionAtRestWithCustomerKey"),
+			),
+		},
+		data.ImportStep(),
+		{
+			Config: r.withEncryptionTypeUpdated(data),


this field is ForceNew, so this won't update - it'll destroy and recreate the resource. Since the first step is the same as Basic we can remove this, and move the assertion up into the Basic test

Suggested change

{

Config: r.withEncryptionTypeDefault(data),

Check: acceptance.ComposeTestCheckFunc(

check.That(data.ResourceName).ExistsInAzure(r),

check.That(data.ResourceName).Key("encryption_type").HasValue("EncryptionAtRestWithCustomerKey"),

),

},

data.ImportStep(),

{

Config: r.withEncryptionTypeUpdated(data),

{

Config: r.customerAndPlatformKey(data),

yes make sense, removed the update part in the test and add the default value check to the basic test

tombuildsstuff · 2021-11-17T10:54:35Z

internal/services/compute/disk_encryption_set_resource_test.go

+`, r.dependencies(data), data.RandomInteger)
+}
+
+func (r DiskEncryptionSetResource) withEncryptionTypeUpdated(data acceptance.TestData) string {


Suggested change

func (r DiskEncryptionSetResource) withEncryptionTypeUpdated(data acceptance.TestData) string {

func (r DiskEncryptionSetResource) customerAndPlatformKey(data acceptance.TestData) string {

tombuildsstuff · 2021-11-17T10:58:52Z

internal/services/compute/windows_virtual_machine_resource.go

 			disksClient := meta.(*clients.Client).Compute.DisksClient

 			update := compute.DiskUpdate{
 				DiskUpdateProperties: &compute.DiskUpdateProperties{
 					Encryption: &compute.Encryption{
-						Type:                compute.EncryptionTypeEncryptionAtRestWithCustomerKey,
+						Type:                compute.EncryptionType(*encryptionType),


same here:

Suggested change

Type: compute.EncryptionType(*encryptionType),

Type: *encryptionType,

tombuildsstuff · 2021-11-17T10:59:21Z

website/docs/r/disk_encryption_set.html.markdown

@@ -107,6 +107,8 @@ The following arguments are supported:

 * `auto_key_rotation_enabled` - (Optional) Boolean flag to specify whether Azure Disk Encryption Set automatically rotates encryption Key to latest version. Defaults to `false`.

+* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Allowed values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.


We've been using Possible in most cases fwiw:

Suggested change

* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Allowed values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.

* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Possible values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.

tombuildsstuff · 2021-11-17T11:00:46Z

website/docs/r/linux_virtual_machine.html.markdown

@@ -248,6 +248,8 @@ A `os_disk` block supports the following:

 -> **NOTE:** The Disk Encryption Set must have the `Reader` Role Assignment scoped on the Key Vault - in addition to an Access Policy to the Key Vault

+-> **NOTE:** Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)


this fields configured on the Disk Encryption Set rather than the VM, so this comment isn't really necessary:

Suggested change

-> **NOTE:** Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)

tombuildsstuff · 2021-11-17T11:00:51Z

website/docs/r/managed_disk.html.markdown

@@ -97,6 +97,8 @@ The following arguments are supported:

 ~> **NOTE:** Disk Encryption Sets are in Public Preview in a limited set of regions

+-> **NOTE:** Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)


this fields configured on the Disk Encryption Set rather than the VM, so this comment isn't really necessary:

Suggested change

-> **NOTE:** Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)

tombuildsstuff · 2021-11-17T11:00:54Z

website/docs/r/windows_virtual_machine.html.markdown

@@ -241,6 +241,8 @@ A `os_disk` block supports the following:

 -> **NOTE:** The Disk Encryption Set must have the `Reader` Role Assignment scoped on the Key Vault - in addition to an Access Policy to the Key Vault

+-> **NOTE:** Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)


this fields configured on the Disk Encryption Set rather than the VM, so this comment isn't really necessary:

Suggested change

-> **NOTE:** Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)

stephybun

Thanks for adopting those changes @myc2h6o. The tests are passing too so this LGTM 🚀

github-actions · 2021-11-19T06:17:34Z

This functionality has been released in v2.86.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2021-12-21T02:10:35Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

managed_disk,disk_encryption_set: Add support of `EncryptionAtRes…

426cb2b

…tWithPlatformAndCustomerKeys`

github-actions bot added documentation size/M labels Nov 17, 2021

myc2h6o added 2 commits November 17, 2021 16:47

Add encryption type discovery to virtual machine

5521373

Update doc

10e9b6d

stephybun reviewed Nov 17, 2021

View reviewed changes

tombuildsstuff suggested changes Nov 17, 2021

View reviewed changes

tombuildsstuff added enhancement service/disks waiting-response labels Nov 17, 2021

Resolve comments

e728022

myc2h6o force-pushed the double_encryption branch from 9d35598 to e728022 Compare November 18, 2021 05:19

myc2h6o requested review from stephybun and tombuildsstuff November 18, 2021 05:24

stephybun approved these changes Nov 18, 2021

View reviewed changes

stephybun merged commit 6cdd818 into hashicorp:main Nov 18, 2021

github-actions bot added this to the v2.86.0 milestone Nov 18, 2021

stephybun added a commit that referenced this pull request Nov 18, 2021

Update for #14218

36d6cfa

myc2h6o deleted the double_encryption branch November 19, 2021 01:51

github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 2021

	return fmt.Errorf("`disk_encryption_set_id` is not a valid disk encryption set id")
	return err

	return fmt.Errorf("reading Disk Encryption Set %q (Resource Group %q): %+v", diskEncryptionSet.Name, diskEncryptionSet.ResourceGroup, err)
	return fmt.Errorf("reading %s: %+v", *diskEncryptionSet, err)

		@@ -107,6 +107,8 @@ The following arguments are supported:

		* `auto_key_rotation_enabled` - (Optional) Boolean flag to specify whether Azure Disk Encryption Set automatically rotates encryption Key to latest version. Defaults to `false`.

		* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Allowed values are `EncryptionAtRestWithCustomerKey` and `EncryptionAtRestWithPlatformAndCustomerKeys`. Defaults to `EncryptionAtRestWithCustomerKey`.

	return nil, err
	return nil, fmt.Errorf("retrieving %s: %+v", *diskEncryptionSet, err)

	return fmt.Errorf("retrieving encryption type from disk encryption set %q: %+v", diskEncryptionSetId, err)
	return err

-		d.Set("encryption_type", props.EncryptionType)
+		encryptionType := string(compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey)
+		if props.EncryptionType != "" {
+			encryptionType = string(props.EncryptionType)
+		}
+		d.Set("encryption_type", encryptionType)

	Default: compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey,
	Default: string(compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey),

	func (r DiskEncryptionSetResource) withEncryptionTypeUpdated(data acceptance.TestData) string {
	func (r DiskEncryptionSetResource) customerAndPlatformKey(data acceptance.TestData) string {

	Type: compute.EncryptionType(*encryptionType),
	Type: *encryptionType,

		@@ -248,6 +248,8 @@ A `os_disk` block supports the following:

		-> NOTE: The Disk Encryption Set must have the `Reader` Role Assignment scoped on the Key Vault - in addition to an Access Policy to the Key Vault

		-> NOTE: Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)

		@@ -97,6 +97,8 @@ The following arguments are supported:

		~> NOTE: Disk Encryption Sets are in Public Preview in a limited set of regions

		-> NOTE: Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)

		@@ -241,6 +241,8 @@ A `os_disk` block supports the following:

		-> NOTE: The Disk Encryption Set must have the `Reader` Role Assignment scoped on the Key Vault - in addition to an Access Policy to the Key Vault

		-> NOTE: Encryption type of the key will be decided by the disk encryption set. [More info on encryption type](https://docs.microsoft.com/en-us/azure/virtual-machines/disk-encryption)

Support EncryptionAtRestWithPlatformAndCustomerKeys in disk encryption #14218

Support EncryptionAtRestWithPlatformAndCustomerKeys in disk encryption #14218

Conversation

myc2h6o commented Nov 17, 2021 • edited Loading

stephybun left a 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

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

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

tombuildsstuff left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myc2h6o Nov 18, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2021

github-actions bot commented Dec 21, 2021

Support `EncryptionAtRestWithPlatformAndCustomerKeys` in disk encryption #14218

Support `EncryptionAtRestWithPlatformAndCustomerKeys` in disk encryption #14218

myc2h6o commented Nov 17, 2021 •

edited

Loading

myc2h6o Nov 18, 2021 •

edited

Loading