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 2 commits
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
29 changes: 29 additions & 0 deletions internal/services/compute/disk_encryption_set.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package compute

import (
"context"
"fmt"

"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-07-01/compute"
"github.com/hashicorp/terraform-provider-azurerm/internal/services/compute/parse"
)

// retrieveDiskEncryptionSetEncryptionType returns encryption type of the disk encryption set
func retrieveDiskEncryptionSetEncryptionType(ctx context.Context, client *compute.DiskEncryptionSetsClient, diskEncryptionSetId string) (*string, error) {
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 retrieveDiskEncryptionSetEncryptionType(ctx context.Context, client *compute.DiskEncryptionSetsClient, diskEncryptionSetId string) (*string, error) {
func retrieveDiskEncryptionSetEncryptionType(ctx context.Context, client *compute.DiskEncryptionSetsClient, diskEncryptionSetId string) (*compute.EncryptionType, error) {

diskEncryptionSet, err := parse.DiskEncryptionSetID(diskEncryptionSetId)
if err != nil {
return nil, err
}

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

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

if properties := resp.EncryptionSetProperties; properties != nil {
encryptionType := string(properties.EncryptionType)
return &encryptionType, nil
} else {
return nil, fmt.Errorf("could not get EncryptionSetProperties")
}
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 become:

Suggested change
if properties := resp.EncryptionSetProperties; properties != nil {
encryptionType := string(properties.EncryptionType)
return &encryptionType, nil
} else {
return nil, fmt.Errorf("could not get EncryptionSetProperties")
}
var encryptionType *compute.EncryptionType
if props := resp.EncryptionSetProperties; props != nil && string(props.EncryptionType) != "" {
encryptionType = &properties.EncryptionType
}
if encryptionType == nil {
return nil, fmt.Errorf("retrieving %s: EncryptionType was nil", *diskEncryptionSet)
}
return encryptionType, nil

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.
Besides the above change, also converted the return value from compute.DiskEncryptionSetType to compute.EncryptionType.

}
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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

}

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

update := compute.DiskUpdate{
DiskUpdateProperties: &compute.DiskUpdateProperties{
Encryption: &compute.Encryption{
Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey,
Type: compute.EncryptionType(*encryptionType),
DiskEncryptionSetID: utils.String(diskEncryptionSetId),
},
},
Expand Down
16 changes: 13 additions & 3 deletions internal/services/compute/managed_disk_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func resourceManagedDisk() *pluginsdk.Resource {
// TODO: make this case-sensitive once this bug in the Azure API has been fixed:
// https://github.com/Azure/azure-rest-api-specs/issues/8132
DiffSuppressFunc: suppress.CaseDifference,
ValidateFunc: azure.ValidateResourceID,
ValidateFunc: validate.DiskEncryptionSetID,
},

"encryption_settings": encryptionSettingsSchema(),
Expand Down Expand Up @@ -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)
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

}

props.Encryption = &compute.Encryption{
Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey,
Type: compute.EncryptionType(*encryptionType),
Copy link
Contributor

Choose a reason for hiding this comment

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

why doesn't the method above return *compute.EncryptionType instead, which would remove the need for this cast?

Suggested change
Type: compute.EncryptionType(*encryptionType),
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.

Return type updated

DiskEncryptionSetID: utils.String(diskEncryptionSetId),
}
}
Expand Down Expand Up @@ -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)
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

}

diskUpdate.Encryption = &compute.Encryption{
Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey,
Type: compute.EncryptionType(*encryptionType),
Copy link
Contributor

Choose a reason for hiding this comment

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

same here:

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

DiskEncryptionSetID: utils.String(diskEncryptionSetId),
}
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
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

}

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

update := compute.DiskUpdate{
DiskUpdateProperties: &compute.DiskUpdateProperties{
Encryption: &compute.Encryption{
Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey,
Type: compute.EncryptionType(*encryptionType),
Copy link
Contributor

Choose a reason for hiding this comment

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

same here:

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

DiskEncryptionSetID: utils.String(diskEncryptionSetId),
},
},
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