Add new resource & data source: azurerm_disk_encryption_set #5249

ArcturusZhang · 2019-12-25T02:46:01Z

This is part of update of the SSE-CMK feature, adding a new resource and a data source: azurerm_disk_encryption_set.

This feature requires Keyvault to enable enable_soft_delete and enable_purge_protection, which are both not implemented in terraform. And the creation of azurerm_disk_encryption_set depends on those two switches, otherwise the service will return errors complaining about this.
Therefore I did some workaround in the test of azurerm_disk_encryption_set. I will change the workaround back when these two features are implemented in azurerm_key_vault.

Currently the fields that could accept user input in a DiskEncryptionSet is all required, therefore I do not put a complete test, since all the fields are covered in the basic test.

tombuildsstuff

hi @ArcturusZhang

Thanks for this PR - I've taken a look through and left some comments inline but this is off to a good start - if we can fix those up then we should be able to run the acceptance tests and take another look :)

Thanks!

azurerm/internal/services/compute/data_source_disk_encryption_set.go

azurerm/internal/services/compute/resource_arm_disk_encryption_set.go

website/docs/r/disk_encryption_set.html.markdown

tombuildsstuff · 2019-12-26T08:31:56Z

azurerm/internal/services/compute/data_source_disk_encryption_set.go

+			return fmt.Errorf("Error setting `previous_keys`: %+v", err)
+		}
+	}
+	if identity := resp.Identity; identity != nil {


we need to ensure that identity is set in all circumstances, so we can move this check inside the flatten function

azurerm/internal/services/compute/resource_arm_disk_encryption_set.go

tombuildsstuff · 2019-12-26T08:34:50Z

website/docs/r/disk_encryption_set.html.markdown

+
+Manage an Azure DiskEncryptionSet instance.
+
+-> **NOTE:** The DiskEncryptionSet service is currently in public preview and are only available in a limited set of regions. 


can we make the phrasing consistent with other resources:

Suggested change

-> **NOTE:** The DiskEncryptionSet service is currently in public preview and are only available in a limited set of regions.

-> **NOTE:** Disk Encryption Sets are currently in Public Preview and are only available in a limited set of regions.

how do users opt-into this?

Let me ask the service team and then fill this information in.

tombuildsstuff · 2019-12-26T08:37:32Z

website/docs/r/disk_encryption_set.html.markdown

+Manage an Azure DiskEncryptionSet instance.
+
+-> **NOTE:** The DiskEncryptionSet service is currently in public preview and are only available in a limited set of regions. 
+


since this requires that the Key Vault is configured in a particular way - can we also document that here:

-> **NOTE:** At this time the Key Vault used to store the Active Key for this Disk Encryption Set must have both Soft Delete & Purge Protection enabled - which are not yet supported by Terraform - instead you can configure this using [a provisioner](https://www.terraform.io/docs/provisioners/local-exec.html) or [the `azurerm_template_deployment` resource](https://www.terraform.io/docs/providers/azurerm/r/template_deployment.html).

Added. And do you think we should add the provisioner command in the example?

👍 - no this isn't something we'd recommend doing until it's natively supported (but that's blocked on the API issue mentioned above)

tombuildsstuff

hi @ArcturusZhang

Thanks for this PR - I've taken a look through and left some comments inline but this is off to a good start - if we can fix those up then we should be able to run the acceptance tests and take another look :)

Thanks!

tombuildsstuff · 2019-12-26T08:45:43Z

Sorry for the double review - Github bug 🙃

ArcturusZhang · 2019-12-27T06:20:37Z

Hi @tombuildsstuff I have resolved some comments, and left some replies. Please have a look.
Additionally, I cannot pass the acctest on my side now. It works well when I am submitting this PR.
It reports:

=== RUN   TestAccAzureRMDiskEncryptionSet_basic
=== PAUSE TestAccAzureRMDiskEncryptionSet_basic
=== CONT  TestAccAzureRMDiskEncryptionSet_basic
--- FAIL: TestAccAzureRMDiskEncryptionSet_basic (0.07s)
    testing.go:569: Step 0 error: Error initializing context: 2 problems:

        - Could not satisfy plugin requirements:
        Plugin reinitialization required. Please run "terraform init".

        Plugins are external binaries that Terraform uses to access and manipulate
        resources. The configuration provided requires plugins which can't be located,
        don't satisfy the version constraints, or are otherwise incompatible.

        Terraform automatically discovers provider requirements from your
        configuration, including providers used in child modules. To see the
        requirements and constraints from each module, run "terraform providers".

        - provider "azurerm" is not available

Could you please help me on this? What is possibly causing this error? Or is there some kinds of caches terraform are using during the acctest?

ArcturusZhang · 2019-12-31T07:44:06Z

Hi @tombuildsstuff I have fixed the acc-tests:

=== RUN   TestAccAzureRMDiskEncryptionSet_basic
=== PAUSE TestAccAzureRMDiskEncryptionSet_basic
=== CONT  TestAccAzureRMDiskEncryptionSet_basic
--- PASS: TestAccAzureRMDiskEncryptionSet_basic (367.77s)
PASS
=== RUN   TestAccDataSourceAzureRMDiskEncryptionSet_basic
=== PAUSE TestAccDataSourceAzureRMDiskEncryptionSet_basic
=== CONT  TestAccDataSourceAzureRMDiskEncryptionSet_basic
--- PASS: TestAccDataSourceAzureRMDiskEncryptionSet_basic (428.47s)
PASS

tombuildsstuff · 2020-01-09T15:03:24Z

@ArcturusZhang hope you don't mind but I've rebased this on top of master to fix the merge conflicts

ArcturusZhang · 2020-01-10T03:28:15Z

@ArcturusZhang hope you don't mind but I've rebased this on top of master to fix the merge conflicts

Sure, thanks.
But the CI is failing on website-lint, how do I fix this? Because i did not really catch any useful information on the log

ArcturusZhang · 2020-01-10T06:00:38Z

Hi @tombuildsstuff I should have fixed the CI failure, please have a look 🚀

tombuildsstuff

hey @ArcturusZhang

Thanks for pushing those changes - I've taken a look through and left some extra comments inline here. Since this is a Preview feature I've taken a closer look at this and noticed that the API Documentation is incorrect for the identity field - insofar as this is Required rather than Optional.

As such I hope you don't mind but so that we can get this merged I'm going to push a couple of commits to resolve these comments (and also hard-code the test location for the moment to work around the limited regions supported by the Preview).

Thanks!

azurerm/internal/services/compute/data_source_disk_encryption_set.go

azurerm/internal/services/compute/resource_arm_disk_encryption_set.go

tombuildsstuff · 2020-01-07T13:02:01Z

azurerm/internal/services/compute/resource_arm_disk_encryption_set.go

+	if location := resp.Location; location != nil {
+		d.Set("location", azure.NormalizeLocation(*location))
+	}
+	if encryptionSetProperties := resp.EncryptionSetProperties; encryptionSetProperties != nil {


minor to match everything else can we use props here

azurerm/internal/services/compute/resource_arm_disk_encryption_set.go

azurerm/internal/services/compute/tests/resource_arm_disk_encryption_set_test.go

website/docs/r/disk_encryption_set.html.markdown

tombuildsstuff · 2020-01-10T10:53:52Z

Tests pass:

ArcturusZhang · 2020-01-10T14:12:59Z

Thanks @tombuildsstuff !

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

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:49Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

ghost added size/XXL documentation labels Dec 25, 2019

ArcturusZhang force-pushed the DiskEncryptionSet branch from 433b732 to a497b8e Compare December 25, 2019 04:16

tombuildsstuff added new-resource service/disks preview labels Dec 26, 2019

tombuildsstuff suggested changes Dec 26, 2019

View reviewed changes

tombuildsstuff added the waiting-response label Dec 26, 2019

This was referenced Dec 26, 2019

r/(linux|windows)_virtual_machine_scale_set - support for Disk Encryption Sets #5252

Merged

Update ManagedDisks for SSE-CMK feature #5250

Merged

ghost added size/XL and removed size/XXL labels Dec 27, 2019

ArcturusZhang force-pushed the DiskEncryptionSet branch from 83e5b2c to 9d819ae Compare December 27, 2019 06:12

ghost removed the waiting-response label Dec 27, 2019

ArcturusZhang force-pushed the DiskEncryptionSet branch from 9d819ae to 22a999e Compare December 31, 2019 02:31

ArcturusZhang requested a review from tombuildsstuff December 31, 2019 02:31

tombuildsstuff self-assigned this Jan 2, 2020

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

tombuildsstuff mentioned this pull request Jan 7, 2020

Key Vault: Support for Disabling Purge Protection Azure/azure-rest-api-specs#8075

Closed

ArcturusZhang added 7 commits January 9, 2020 16:02

Add new resource and data source DiskEncryptionSet

10a2859

Fix CI failure

0edcc3b

Resolve comments

acbaad7

Rebased to master to conform some new changes on test

f3d4333

Fixed acctest

aec2a14

Resolve more comments

e21506f

Fix CI

77c4d4c

ArcturusZhang added 4 commits January 9, 2020 16:02

Modify docs a little bit

d6cb640

Use vaultsClient instead or ArmClient

d729603

Goimports

420ac7a

Add validation test for disk encryption set name

3f2b59c

tombuildsstuff force-pushed the DiskEncryptionSet branch from 55d4bc3 to 3f2b59c Compare January 9, 2020 15:03

r/disk_encryption_set: fixing the tests to use testdata

61c92b3

r/disk_encryption_set: fixing up the pr comments

b762f5a

tombuildsstuff approved these changes Jan 10, 2020

View reviewed changes

tombuildsstuff force-pushed the DiskEncryptionSet branch from 74ce628 to b762f5a Compare January 10, 2020 10:31

ghost added size/XXL and removed size/XL labels Jan 10, 2020

tombuildsstuff added 2 commits January 10, 2020 12:13

r/disk_encryption_set: fixing the messages

1dffac7

linting: fixing the website

011bf54

tombuildsstuff merged commit f553c07 into hashicorp:master Jan 10, 2020

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

updating to include #5249

99fb416

ArcturusZhang deleted the DiskEncryptionSet branch January 10, 2020 14:12

ghost locked and limited conversation to collaborators Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new resource & data source: azurerm_disk_encryption_set #5249

Add new resource & data source: azurerm_disk_encryption_set #5249

ArcturusZhang commented Dec 25, 2019 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Dec 26, 2019

tombuildsstuff Dec 26, 2019

ArcturusZhang Dec 27, 2019

tombuildsstuff Dec 26, 2019

ArcturusZhang Dec 27, 2019

tombuildsstuff Jan 7, 2020

tombuildsstuff left a comment

tombuildsstuff commented Dec 26, 2019

ArcturusZhang commented Dec 27, 2019

ArcturusZhang commented Dec 31, 2019

tombuildsstuff commented Jan 9, 2020

ArcturusZhang commented Jan 10, 2020 •

edited

Loading

ArcturusZhang commented Jan 10, 2020

tombuildsstuff left a comment

tombuildsstuff Jan 7, 2020

tombuildsstuff commented Jan 10, 2020

ArcturusZhang commented Jan 10, 2020

ghost commented Jan 16, 2020

ghost commented Mar 28, 2020


		Manage an Azure DiskEncryptionSet instance.

		-> NOTE: The DiskEncryptionSet service is currently in public preview and are only available in a limited set of regions.

	-> NOTE: The DiskEncryptionSet service is currently in public preview and are only available in a limited set of regions.
	-> NOTE: Disk Encryption Sets are currently in Public Preview and are only available in a limited set of regions.

Add new resource & data source: azurerm_disk_encryption_set #5249

Add new resource & data source: azurerm_disk_encryption_set #5249

Conversation

ArcturusZhang commented Dec 25, 2019 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Dec 26, 2019

Choose a reason for hiding this comment

tombuildsstuff Dec 26, 2019

Choose a reason for hiding this comment

ArcturusZhang Dec 27, 2019

Choose a reason for hiding this comment

tombuildsstuff Dec 26, 2019

Choose a reason for hiding this comment

ArcturusZhang Dec 27, 2019

Choose a reason for hiding this comment

tombuildsstuff Jan 7, 2020

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Dec 26, 2019

ArcturusZhang commented Dec 27, 2019

ArcturusZhang commented Dec 31, 2019

tombuildsstuff commented Jan 9, 2020

ArcturusZhang commented Jan 10, 2020 • edited Loading

ArcturusZhang commented Jan 10, 2020

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Jan 7, 2020

Choose a reason for hiding this comment

tombuildsstuff commented Jan 10, 2020

ArcturusZhang commented Jan 10, 2020

ghost commented Jan 16, 2020

ghost commented Mar 28, 2020

ArcturusZhang commented Dec 25, 2019 •

edited

Loading

ArcturusZhang commented Jan 10, 2020 •

edited

Loading