Update ManagedDisks for SSE-CMK feature #5250

ArcturusZhang · 2019-12-25T03:31:36Z

This is part of update of the SSE-CMK feature, adding new fields for azurerm_managed_disk.

tombuildsstuff · 2019-12-26T08:49:02Z

Dependent on #5249

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments inline. As discussed offline since this needed to be rebased on top of #5249 so that we can add some acceptance tests I hope you don't mind but I've pushed some commits to resolve the issues raised and add some tests.

Thanks!

azurerm/internal/services/compute/data_source_managed_disk.go

azurerm/internal/services/compute/resource_arm_managed_disk.go

azurerm/internal/services/compute/data_source_managed_disk.go

azurerm/internal/services/compute/resource_arm_managed_disk.go

tombuildsstuff · 2020-01-10T12:26:36Z

azurerm/internal/services/compute/resource_arm_managed_disk.go

+		}
+	}
+
+	createDisk.Encryption = &encryption


since this is optional we need to move this within the if statement

on reflection we could also infer the value for encryption_type - since the value's dependent on disk_encryption_set_id?

tombuildsstuff · 2020-01-10T12:49:56Z

website/docs/r/managed_disk.html.markdown

@@ -113,6 +113,12 @@ The following arguments are supported:

 * `encryption_settings` - (Optional) an `encryption_settings` block as defined below.

+* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Valid values are `EncryptionAtRestWithPlatformKey` or `EncryptionAtRestWithCustomerKey`. Default value is `EncryptionAtRestWithPlatformKey`. When set to `EncryptionAtRestWithPlatformKey`, the disk is encrypted with XStore managed key at rest. When set to `EncryptionAtRestWithCustomerKey`, the disk is encrypted with Customer managed key at rest, and the `managed_disk_encryption_set_id` must be set to a valid `azurerm_disk_encryption_set` ID.
+
+* `managed_disk_encryption_set_id` - (Optional) ID of the disk encryption set to use for enabling encryption at rest.


Suggested change

* `managed_disk_encryption_set_id` - (Optional) ID of the disk encryption set to use for enabling encryption at rest.

* `disk_encryption_set_id` - (Optional) ID of the Disk Encryption Set which should be used to Encrypt the Disk.

-> **NOTE:** This field can only be specified when `encryption_type` is set to `EncryptionAtRestWithCustomerKey`

this also wants a Preview note:

~> **NOTE:** Disk Encryption Sets are in Preview.

tombuildsstuff · 2020-01-10T13:57:06Z

azurerm/internal/services/compute/data_source_managed_disk.go

@@ -71,6 +71,16 @@ func dataSourceArmManagedDisk() *schema.Resource {
 				Computed: true,
 			},

+			"encryption_type": {


after investigating it appears we can set this field conditionally based on disk_encryption_set_id having a value - as such we can remove this

tombuildsstuff · 2020-01-10T13:57:17Z

azurerm/internal/services/compute/data_source_managed_disk.go

 		d.Set("disk_size_gb", props.DiskSizeGB)
 		d.Set("disk_iops_read_write", props.DiskIOPSReadWrite)
 		d.Set("disk_mbps_read_write", props.DiskMBpsReadWrite)
 		d.Set("os_type", props.OsType)
+		if encryption := props.Encryption; encryption != nil {
+			d.Set("encryption_type", string(encryption.Type))


after investigating it appears we can set this field conditionally based on disk_encryption_set_id having a value - as such we can remove this

Suggested change

d.Set("encryption_type", string(encryption.Type))

tombuildsstuff · 2020-01-10T13:58:33Z

website/docs/r/managed_disk.html.markdown

@@ -113,6 +113,12 @@ The following arguments are supported:

 * `encryption_settings` - (Optional) an `encryption_settings` block as defined below.

+* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Valid values are `EncryptionAtRestWithPlatformKey` or `EncryptionAtRestWithCustomerKey`. Default value is `EncryptionAtRestWithPlatformKey`. When set to `EncryptionAtRestWithPlatformKey`, the disk is encrypted with XStore managed key at rest. When set to `EncryptionAtRestWithCustomerKey`, the disk is encrypted with Customer managed key at rest, and the `managed_disk_encryption_set_id` must be set to a valid `azurerm_disk_encryption_set` ID.


we can remove this:

Suggested change

* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Valid values are `EncryptionAtRestWithPlatformKey` or `EncryptionAtRestWithCustomerKey`. Default value is `EncryptionAtRestWithPlatformKey`. When set to `EncryptionAtRestWithPlatformKey`, the disk is encrypted with XStore managed key at rest. When set to `EncryptionAtRestWithCustomerKey`, the disk is encrypted with Customer managed key at rest, and the `managed_disk_encryption_set_id` must be set to a valid `azurerm_disk_encryption_set` ID.

tombuildsstuff · 2020-01-10T13:58:40Z

website/docs/d/managed_disk.html.markdown

@@ -113,5 +113,7 @@ resource "azurerm_virtual_machine" "example" {
 * `disk_size_gb` - The size of the managed disk in gigabytes.
 * `disk_iops_read_write` - The number of IOPS allowed for this disk. One operation can transfer between 4k and 256k bytes.
 * `disk_mbps_read_write` - The bandwidth allowed for this disk. 
+* `encryption_type` - The type of key used to encrypt the data of the disk.


Suggested change

* `encryption_type` - The type of key used to encrypt the data of the disk.

mbfrahry

LGTM once tests are passing

ArcturusZhang · 2020-01-14T06:34:15Z

Thanks a lot @tombuildsstuff for the modification!
And I also made some changes in the VMSS PR to conform the changes here.

…t_id

…forcenew

Unfortunately it appears that this information is now required in the API as such we'll have to make this optional+required Fixes hashicorp#4994

tombuildsstuff · 2020-01-14T11:52:44Z

Tests pass:

ghost · 2020-01-16T12:19:03Z

This has been released in version 1.41.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.41.0"
}
# ... other configuration ...

ghost · 2020-03-28T15:28:14Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

ghost added size/M documentation labels Dec 25, 2019

tombuildsstuff added enhancement service/disks labels Dec 26, 2019

tombuildsstuff self-assigned this Jan 2, 2020

tombuildsstuff added this to the v1.41.0 milestone Jan 7, 2020

tombuildsstuff force-pushed the ManagedDisks branch from 45d873f to 2f904d1 Compare January 13, 2020 15:43

ghost added size/XL and removed size/M labels Jan 13, 2020

tombuildsstuff reviewed Jan 13, 2020

View reviewed changes

mbfrahry approved these changes Jan 13, 2020

View reviewed changes

ArcturusZhang and others added 8 commits January 14, 2020 11:18

Add SSE-CMK feature for managed disks

f33a8a4

d/managed_disk: resolving PR comments

5d4bd29

r/managed_disk: refactoring inferring based on the disk_encryption_se…

e6b8210

…t_id

r/managed_disk: adding a test using a disk encryption set

ee06f7c

r/managed_disk: adding a test case & making disk_encryption_set_id …

be4b771

…forcenew

r/managed_disks: adding an example for Encrypted Disks

12ca5ff

r/managed_disk: fixing the last-minute cleanup

74baefb

r/managed_disk: ensuring that disk_encryption_set_id is always set

189508e

tombuildsstuff force-pushed the ManagedDisks branch from 9c4cb43 to 189508e Compare January 14, 2020 10:18

tombuildsstuff added 3 commits January 14, 2020 11:51

r/managed_disk: adding a todo to fix disk_size_gb in 2.0

95838bd

r/managed_disk: ensuring we check for the vm not the disk

2bbe838

r/managed_disk: specifying storage_account_id during import

899e98d

Unfortunately it appears that this information is now required in the API as such we'll have to make this optional+required Fixes hashicorp#4994

tombuildsstuff merged commit 7e57289 into hashicorp:master Jan 14, 2020

tombuildsstuff added a commit that referenced this pull request Jan 14, 2020

updating to include #5250

afa4ef0

ArcturusZhang deleted the ManagedDisks branch January 17, 2020 02:23

ghost locked and limited conversation to collaborators Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ManagedDisks for SSE-CMK feature #5250

Update ManagedDisks for SSE-CMK feature #5250

ArcturusZhang commented Dec 25, 2019

tombuildsstuff commented Dec 26, 2019

tombuildsstuff left a comment

tombuildsstuff Jan 10, 2020

tombuildsstuff Jan 10, 2020

tombuildsstuff Jan 10, 2020

tombuildsstuff Jan 10, 2020

tombuildsstuff Jan 10, 2020

tombuildsstuff Jan 10, 2020

tombuildsstuff Jan 10, 2020

mbfrahry left a comment •

edited

Loading

ArcturusZhang commented Jan 14, 2020 •

edited

Loading

tombuildsstuff commented Jan 14, 2020

ghost commented Jan 16, 2020

ghost commented Mar 28, 2020

-* `managed_disk_encryption_set_id` - (Optional) ID of the disk encryption set to use for enabling encryption at rest.
+* `disk_encryption_set_id` - (Optional) ID of the Disk Encryption Set which should be used to Encrypt the Disk.
+-> **NOTE:** This field can only be specified when `encryption_type` is set to `EncryptionAtRestWithCustomerKey`

		@@ -113,6 +113,12 @@ The following arguments are supported:

		* `encryption_settings` - (Optional) an `encryption_settings` block as defined below.

		* `encryption_type` - (Optional) The type of key used to encrypt the data of the disk. Valid values are `EncryptionAtRestWithPlatformKey` or `EncryptionAtRestWithCustomerKey`. Default value is `EncryptionAtRestWithPlatformKey`. When set to `EncryptionAtRestWithPlatformKey`, the disk is encrypted with XStore managed key at rest. When set to `EncryptionAtRestWithCustomerKey`, the disk is encrypted with Customer managed key at rest, and the `managed_disk_encryption_set_id` must be set to a valid `azurerm_disk_encryption_set` ID.

Update ManagedDisks for SSE-CMK feature #5250

Update ManagedDisks for SSE-CMK feature #5250

Conversation

ArcturusZhang commented Dec 25, 2019

tombuildsstuff commented Dec 26, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Jan 10, 2020

Choose a reason for hiding this comment

tombuildsstuff Jan 10, 2020

Choose a reason for hiding this comment

tombuildsstuff Jan 10, 2020

Choose a reason for hiding this comment

tombuildsstuff Jan 10, 2020

Choose a reason for hiding this comment

tombuildsstuff Jan 10, 2020

Choose a reason for hiding this comment

tombuildsstuff Jan 10, 2020

Choose a reason for hiding this comment

tombuildsstuff Jan 10, 2020

Choose a reason for hiding this comment

mbfrahry left a comment • edited Loading

Choose a reason for hiding this comment

ArcturusZhang commented Jan 14, 2020 • edited Loading

tombuildsstuff commented Jan 14, 2020

ghost commented Jan 16, 2020

ghost commented Mar 28, 2020

mbfrahry left a comment •

edited

Loading

ArcturusZhang commented Jan 14, 2020 •

edited

Loading