EBS default encryption #8771

ewbankkit · 2019-05-24T19:39:55Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

FEATURES:

* New Resource: `aws_ebs_encryption_by_default`
* New Resource: `aws_ebs_default_kms_key`

Output from acceptance testing:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSEBSEncryptionByDefault_'==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAWSEBSEncryptionByDefault_ -timeout 120m
=== RUN   TestAccAWSEBSEncryptionByDefault_basic
=== PAUSE TestAccAWSEBSEncryptionByDefault_basic
=== CONT  TestAccAWSEBSEncryptionByDefault_basic
--- PASS: TestAccAWSEBSEncryptionByDefault_basic (24.04s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	24.079s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSEBSDefaultKmsKey_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAWSEBSDefaultKmsKey_ -timeout 120m
=== RUN   TestAccAWSEBSDefaultKmsKey_basic
=== PAUSE TestAccAWSEBSDefaultKmsKey_basic
=== CONT  TestAccAWSEBSDefaultKmsKey_basic
--- PASS: TestAccAWSEBSDefaultKmsKey_basic (56.43s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	56.464s

bflad

Thanks for this @ewbankkit! Overall nice work. Please reach out if you have any questions or do not have time for the items. 😄

bflad · 2019-06-06T20:05:11Z

aws/resource_aws_ebs_default_kms_key.go

+		Delete: resourceAwsEbsDefaultKmsKeyDelete,
+
+		Schema: map[string]*schema.Schema{
+			"key_id": {


Since the API requires the ARN instead of just the ID, it seems like we should prefer to naming this more clearly (even though its slightly off from the API itself 👍 )

Suggested change

"key_id": {

"key_arn": {

bflad · 2019-06-06T20:06:47Z

aws/resource_aws_ebs_default_kms_key.go

+		return fmt.Errorf("error creating EBS default KMS key: %s", err)
+	}
+
+	d.SetId(resource.UniqueId())


Should we set this to the key ARN/ID instead of a random identifier so its usable in downstream resources? 😄

Currently we are allowing the ARN to change and doing a resource update - If we go with using the ARN as the ID then we need to add ForceNew: true to the key_arn attribute and remove resourceAwsEbsDefaultKmsKeyUpdate(). IMHO this is actually a better way of modeling the resource.

bflad · 2019-06-06T20:14:16Z

aws/resource_aws_ebs_default_kms_key.go

+)
+
+func resourceAwsEbsDefaultKmsKey() *schema.Resource {
+	return &schema.Resource{


We should add import support from the get-go, e.g. via the key ARN as the ID 🚀

Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, },

In the testing:

{ ResourceName: resourceName, ImportState: true, ImportStateVerify: true, },

And the documentation:

## Import EBS Default KMS Key can be imported with the KMS Key ARN, e.g. ```console $ terraform import aws_ebs_default_kms_key.example arn:aws:kms:us-east-1:123456789012:key/abcd-1234 ```

Since resourceAwsEbsDefaultKmsKeyRead() doesn't actually use the resource's ID for the AWS API call (there's a single default EBS CMK per region per account) do we want to check that the KMS key ARN passed to terraform import is really the current default EBS CMK?

If the Terraform code corresponding to the imported resource has the same key_arn value as the ARN passed to terraform import then I think that having ForceNew on the key_arn attribute will cause the resource to be recreated.

bflad · 2019-06-06T20:15:48Z

aws/resource_aws_ebs_default_kms_key_test.go

+	ec2conn := testAccProvider.Meta().(*AWSClient).ec2conn
+	kmsconn := testAccProvider.Meta().(*AWSClient).kmsconn
+
+	alias, err := findKmsAliasByName(kmsconn, "alias/aws/ebs", nil)


bflad · 2019-06-06T20:17:15Z

aws/resource_aws_ebs_default_kms_key_test.go

+	return nil
+}
+
+func testAccCheckEbsDefaultKmsKey(name string) resource.TestCheckFunc {


Is it worth having this function do the inverse of the destroy check?

bflad · 2019-06-06T20:19:48Z

aws/resource_aws_ebs_encryption_by_default.go

+		Create: resourceAwsEbsEncryptionByDefaultCreate,
+		Read:   resourceAwsEbsEncryptionByDefaultRead,
+		Update: resourceAwsEbsEncryptionByDefaultUpdate,
+		Delete: resourceAwsEbsEncryptionByDefaultDelete,


If we are going to do nothing in the Delete function, this can use schema.Noop instead of a custom function.

Suggested change

Delete: resourceAwsEbsEncryptionByDefaultDelete,

Delete: schema.Noop,

The documentation in that case should also explicitly state that it is only removing Terraform's management of the setting.

Instead of an empty Delete function though, I think a better user experience would be do disable encryption when this resource is removed. 👍

bflad · 2019-06-06T20:21:25Z

aws/resource_aws_ebs_encryption_by_default_test.go

+func TestAccAWSEBSEncryptionByDefault_basic(t *testing.T) {
+	resourceName := "aws_ebs_encryption_by_default.test"
+
+	resource.ParallelTest(t, resource.TestCase{


If the Delete function is to remain empty, can you please add an explicit CheckDestroy: nil, to this TestCase?

Ideally though, this resource seems like it should enable encryption by default when its added and disable encryption when its removed. The CheckDestroy in that case would verify that encryption is disabled. 😄

bflad · 2019-06-06T20:22:01Z

website/docs/r/ebs_default_kms_key.html.markdown

+
+~> **NOTE:** Creating an `aws_ebs_default_kms_key` resource does not enable default EBS encryption. Use the [`aws_ebs_encryption_by_default`](ebs_encryption_by_default.html) to enable default EBS encryption.
+
+~> **NOTE:** Destroying this resource will reset the default CMK to the account's AWS-managed default CMK for EBS.


aws/resource_aws_ebs_encryption_by_default.go

website/docs/r/ebs_encryption_by_default.html.markdown

Code review changes. Co-Authored-By: Brian Flad <bflad417@gmail.com>

…rovider-aws into issue-8760

ewbankkit · 2019-06-09T22:15:30Z

Review comments addressed.
Re-ran acceptance tests:

$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSEBSEncryptionByDefault_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAWSEBSEncryptionByDefault_ -timeout 120m
=== RUN   TestAccAWSEBSEncryptionByDefault_basic
=== PAUSE TestAccAWSEBSEncryptionByDefault_basic
=== CONT  TestAccAWSEBSEncryptionByDefault_basic
--- PASS: TestAccAWSEBSEncryptionByDefault_basic (25.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	25.542s
$ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSEBSDefaultKmsKey_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAWSEBSDefaultKmsKey_ -timeout 120m
=== RUN   TestAccAWSEBSDefaultKmsKey_basic
=== PAUSE TestAccAWSEBSDefaultKmsKey_basic
=== CONT  TestAccAWSEBSDefaultKmsKey_basic
--- PASS: TestAccAWSEBSDefaultKmsKey_basic (46.26s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	46.284s

bflad

Looks great, thanks so much, @ewbankkit 🚀

--- PASS: TestAccAWSEBSEncryptionByDefault_basic (15.29s)
--- PASS: TestAccAWSEBSDefaultKmsKey_basic (40.03s)

bflad · 2019-06-20T22:09:25Z

This has been released in version 2.16.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2019-11-03T15:21:55Z

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. Thanks!

ewbankkit added 2 commits May 24, 2019 15:32

Add aws_ebs_encryption_by_default resource.

5d6355d

Add aws_ebs_default_kms_key resource.

00f6a10

ewbankkit force-pushed the issue-8760 branch from 68a390a to 7aaef34 Compare May 24, 2019 21:22

Correct documentation link.

6b028d1

ewbankkit force-pushed the issue-8760 branch from 7aaef34 to 6b028d1 Compare May 24, 2019 21:42

bflad added the new-resource Introduces a new resource. label Jun 6, 2019

bflad added this to the v2.15.0 milestone Jun 6, 2019

bflad reviewed Jun 6, 2019

View reviewed changes

ewbankkit and others added 5 commits June 9, 2019 17:55

r/aws_ebs_default_kms_key: Review changes.

0d6ce4c

Update website/docs/r/ebs_encryption_by_default.html.markdown

54ddb3f

Code review changes. Co-Authored-By: Brian Flad <bflad417@gmail.com>

Update aws/resource_aws_ebs_encryption_by_default.go

05b9633

Code review changes. Co-Authored-By: Brian Flad <bflad417@gmail.com>

Merge branch 'issue-8760' of https://github.com/ewbankkit/terraform-p…

753fb23

…rovider-aws into issue-8760

r/aws_ebs_encryption_by_default: Review changes.

84f451d

bflad self-assigned this Jun 11, 2019

bflad modified the milestones: v2.15.0, v2.16.0 Jun 13, 2019

bflad approved these changes Jun 20, 2019

View reviewed changes

bflad merged commit 3b751f1 into hashicorp:master Jun 20, 2019

bflad added a commit that referenced this pull request Jun 20, 2019

Update CHANGELOG for #8771

8f1c962

ewbankkit deleted the issue-8760 branch June 20, 2019 11:32

FernandoMiguel mentioned this pull request Jun 27, 2019

AWS Feature Release: Encrypt Root Volumes at Launch #8624

Closed

ghost locked and limited conversation to collaborators Nov 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EBS default encryption #8771

EBS default encryption #8771

ewbankkit commented May 24, 2019

bflad left a comment

bflad Jun 6, 2019

bflad Jun 6, 2019

ewbankkit Jun 9, 2019

bflad Jun 6, 2019

ewbankkit Jun 9, 2019

ewbankkit Jun 10, 2019

bflad Jun 6, 2019

bflad Jun 6, 2019

bflad Jun 6, 2019

bflad Jun 6, 2019

bflad Jun 6, 2019

ewbankkit commented Jun 9, 2019

bflad left a comment

bflad commented Jun 20, 2019

ghost commented Nov 3, 2019

	Delete: resourceAwsEbsEncryptionByDefaultDelete,
	Delete: schema.Noop,


		~> NOTE: Creating an `aws_ebs_default_kms_key` resource does not enable default EBS encryption. Use the [`aws_ebs_encryption_by_default`](ebs_encryption_by_default.html) to enable default EBS encryption.

		~> NOTE: Destroying this resource will reset the default CMK to the account's AWS-managed default CMK for EBS.

EBS default encryption #8771

EBS default encryption #8771

Conversation

ewbankkit commented May 24, 2019

Community Note

bflad 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

ewbankkit commented Jun 9, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jun 20, 2019

ghost commented Nov 3, 2019