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

Support EncryptionAtRestWithPlatformAndCustomerKeys in disk encryption #14218

Merged
merged 4 commits into from
Nov 18, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions internal/services/compute/disk_encryption_set_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,17 @@ func resourceDiskEncryptionSet() *pluginsdk.Resource {
Optional: true,
},

"encryption_type": {
Type: pluginsdk.TypeString,
Optional: true,
ForceNew: true,
Default: compute.DiskEncryptionSetTypeEncryptionAtRestWithCustomerKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be a string:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for pointing out, updated

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

Choose a reason for hiding this comment

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

this should be case-sensitive:

Suggested change
}, true),
}, false),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

},

"identity": {
Type: pluginsdk.TypeList,
// whilst the API Documentation shows optional - attempting to send nothing returns:
Expand Down Expand Up @@ -132,6 +143,7 @@ func resourceDiskEncryptionSetCreate(d *pluginsdk.ResourceData, meta interface{}

location := azure.NormalizeLocation(d.Get("location").(string))
rotationToLatestKeyVersionEnabled := d.Get("auto_key_rotation_enabled").(bool)
encryptionType := d.Get("encryption_type").(string)
identityRaw := d.Get("identity").([]interface{})
t := d.Get("tags").(map[string]interface{})

Expand All @@ -145,6 +157,7 @@ func resourceDiskEncryptionSetCreate(d *pluginsdk.ResourceData, meta interface{}
},
},
RotationToLatestKeyVersionEnabled: utils.Bool(rotationToLatestKeyVersionEnabled),
EncryptionType: compute.DiskEncryptionSetType(encryptionType),
},
Identity: expandDiskEncryptionSetIdentity(identityRaw),
Tags: tags.Expand(t),
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to set default value

}

if err := d.Set("identity", flattenDiskEncryptionSetIdentity(resp.Identity)); err != nil {
Expand Down
59 changes: 59 additions & 0 deletions internal/services/compute/disk_encryption_set_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,30 @@ func TestAccDiskEncryptionSet_keyRotate(t *testing.T) {
})
}

func TestAccDiskEncryptionSet_withEncryptionType(t *testing.T) {
data := acceptance.BuildTestData(t, "azurerm_disk_encryption_set", "test")
r := DiskEncryptionSetResource{}

data.ResourceTest(t, r, []acceptance.TestStep{
{
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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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),

Copy link
Contributor Author

@myc2h6o myc2h6o Nov 18, 2021

Choose a reason for hiding this comment

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

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

Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
check.That(data.ResourceName).Key("encryption_type").HasValue("EncryptionAtRestWithPlatformAndCustomerKeys"),
),
},
data.ImportStep(),
})
}

func (DiskEncryptionSetResource) Exists(ctx context.Context, clients *clients.Client, state *pluginsdk.InstanceState) (*bool, error) {
id, err := parse.DiskEncryptionSetID(state.ID)
if err != nil {
Expand Down Expand Up @@ -292,3 +316,38 @@ resource "azurerm_disk_encryption_set" "test" {
}
`, r.dependencies(data), data.RandomInteger)
}

func (r DiskEncryptionSetResource) withEncryptionTypeDefault(data acceptance.TestData) string {
return fmt.Sprintf(`
%s

resource "azurerm_disk_encryption_set" "test" {
name = "acctestDES-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
key_vault_key_id = azurerm_key_vault_key.test.id

identity {
type = "SystemAssigned"
}
}
`, r.dependencies(data), data.RandomInteger)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same as Basic so can go:

Suggested change
func (r DiskEncryptionSetResource) withEncryptionTypeDefault(data acceptance.TestData) string {
return fmt.Sprintf(`
%s
resource "azurerm_disk_encryption_set" "test" {
name = "acctestDES-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
key_vault_key_id = azurerm_key_vault_key.test.id
identity {
type = "SystemAssigned"
}
}
`, r.dependencies(data), data.RandomInteger)
}


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

Choose a reason for hiding this comment

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

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

return fmt.Sprintf(`
%s

resource "azurerm_disk_encryption_set" "test" {
name = "acctestDES-%d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
key_vault_key_id = azurerm_key_vault_key.test.id
encryption_type = "EncryptionAtRestWithPlatformAndCustomerKeys"

identity {
type = "SystemAssigned"
}
}
`, r.dependencies(data), data.RandomInteger)
}
26 changes: 24 additions & 2 deletions internal/services/compute/managed_disk_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and moved to new method retrieveDiskEncryptionSetEncryptionType in disk_encryption_set.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)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in the new method

}

props.Encryption = &compute.Encryption{
Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey,
Type: compute.EncryptionType(resp.EncryptionType),
DiskEncryptionSetID: utils.String(diskEncryptionSetId),
}
}
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here as above

}

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)
Copy link
Member

Choose a reason for hiding this comment

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

Same here as above

}

diskUpdate.Encryption = &compute.Encryption{
Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey,
Type: compute.EncryptionType(resp.EncryptionType),
DiskEncryptionSetID: utils.String(diskEncryptionSetId),
}
} else {
Expand Down
2 changes: 2 additions & 0 deletions website/docs/r/disk_encryption_set.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Copy link
Member

Choose a reason for hiding this comment

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

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`.

Copy link
Contributor

Choose a reason for hiding this comment

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

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`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


* `identity` - (Required) An `identity` block as defined below.

* `tags` - (Optional) A mapping of tags to assign to the Disk Encryption Set.
Expand Down
2 changes: 2 additions & 0 deletions website/docs/r/managed_disk.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)


* `disk_iops_read_write` - (Optional) The number of IOPS allowed for this disk; only settable for UltraSSD disks. One operation can transfer between 4k and 256k bytes.

* `disk_mbps_read_write` - (Optional) The bandwidth allowed for this disk; only settable for UltraSSD disks. MBps means millions of bytes per second.
Expand Down