resource/aws_efs_file_system: Add support for encryption #1420

pavloos · 2017-08-15T11:33:06Z

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEFSFileSystem_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEFSFileSystem_ -timeout 120m
=== RUN   TestAccAWSEFSFileSystem_importBasic
--- PASS: TestAccAWSEFSFileSystem_importBasic (50.69s)
=== RUN   TestAccAWSEFSFileSystem_basic
--- PASS: TestAccAWSEFSFileSystem_basic (113.40s)
=== RUN   TestAccAWSEFSFileSystem_pagedTags
--- PASS: TestAccAWSEFSFileSystem_pagedTags (48.26s)
=== RUN   TestAccAWSEFSFileSystem_KmsKey
--- PASS: TestAccAWSEFSFileSystem_KmsKey (87.40s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	299.772s

Ninir

Hey @pjfbashton

Thanks for the work here!
Just left a few comments to address before merging.

Nice work for a first contribution! 👍

Ninir · 2017-08-16T08:58:20Z

aws/resource_aws_efs_file_system_test.go

+
+func testAccAWSEFSFileSystemConfigWithKmsKey(rInt int) string {
+	return fmt.Sprintf(`
+	resource "aws_kms_key" "foo" {


Could you stick lines to the left with an indentation of 2 spaces?

I actually dont think we need that policy at all, test runs fine without it so I removed it

Ninir · 2017-08-16T09:01:46Z

aws/resource_aws_efs_file_system.go

+
+	if v, ok := d.GetOk("kms_key_id"); ok {
+		createOpts.KmsKeyId = aws.String(v.(string))
+	}


We should check that Encrypted is true if KmsKeyId is set, and catch the related Exception if the API call is made (BadRequest perhaps?).

Looking into this now, thanks ;)

Ninir · 2017-08-16T09:04:30Z

aws/resource_aws_efs_file_system_test.go

+						"aws_efs_file_system.foo-with-kms",
+						"kms_key_id",
+						keyRegex,
+					),


We should also check that encrypted is well set here :)

Thanks, good call. Check added

Ninir · 2017-08-16T09:05:18Z

aws/resource_aws_efs_file_system_test.go

+	}
+
+	resource "aws_efs_file_system" "foo-with-kms" {
+		encrypted = true


encrypted should be aligned with kms_key_id

Actually I cannot see why would we need this policy, test runs fine without it

pavloos · 2017-08-17T11:18:56Z

@Ninir seems that KmsKeyId is ignored by sdk when Encrypted is false so I added a check that will throw an error if encrypted is not true when kms_key_id is specified. would that work?

Ninir

Hey @pjfbashton

Made a few modifications of your work, so that it can be merged. For the record, added another acceptance test that actually checks the error you implemented, when encrypted is false and the kms key is defined.

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEFSFileSystem_'   
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEFSFileSystem_ -timeout 120m
=== RUN   TestAccAWSEFSFileSystem_importBasic
--- PASS: TestAccAWSEFSFileSystem_importBasic (66.84s)
=== RUN   TestAccAWSEFSFileSystem_basic
--- PASS: TestAccAWSEFSFileSystem_basic (164.57s)
=== RUN   TestAccAWSEFSFileSystem_pagedTags
--- PASS: TestAccAWSEFSFileSystem_pagedTags (68.46s)
=== RUN   TestAccAWSEFSFileSystem_kmsKey
--- PASS: TestAccAWSEFSFileSystem_kmsKey (123.88s)
=== RUN   TestAccAWSEFSFileSystem_kmsConfigurationWithoutEncryption
--- PASS: TestAccAWSEFSFileSystem_kmsConfigurationWithoutEncryption (73.56s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	497.346s

Thanks for the work here, nice 1st contribution! 👍 :)

pavloos · 2017-08-18T08:37:26Z

That's great @Ninir ! Thanks for your help 👍 ;)

shezaan · 2017-09-08T18:30:25Z

Stupid question, which version of TF will this be added to? I am already on 0.10.4.
Documentation still doesn't reflect this if its already there.
Thanks in advance!

Ninir · 2017-09-08T20:05:08Z

Hi @shezaan !

Since the Terraform split, providers (AWS, Google, Azure, Kubernetes, etc...) handle their own releases.

The one including EFS encryption will then be in the next AWS release.

I can't provide any ETA of it yet, sadly. 'will try to get back to you on that as soon as possible! 👍

resource/aws_efs_file_system: Add support for encryption

ghost · 2020-04-11T17:20:52Z

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!

Pawel Fiuto added 4 commits August 15, 2017 12:12

Add support for creation of encrypted EFS

55b3732

Add EFS encryption support to EFS data source

7384565

Add test for EFS KMS

410e2d5

Update docs for EFS encryption

76f773c

grubernaut added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 15, 2017

stack72 mentioned this pull request Aug 16, 2017

Feature Request: encryption support for aws_efs_file_system #1422

Closed

Ninir suggested changes Aug 16, 2017

View reviewed changes

Pawel Fiuto added 3 commits August 17, 2017 09:19

r/efs: Check if encrypted is set when we use kms

856b9a0

r/efs: Fix code alignment

091a30c

r/efs: Remove unnecessary IAM policy from kms test

c99dfe3

Actually I cannot see why would we need this policy, test runs fine without it

Ninir added the waiting-response Maintainers are waiting on response from community or contributor. label Aug 17, 2017

Check if is true when kms_key_id is specified

cb13954

Added another test acceptance for encrypted EFS

a4a3c55

Ninir force-pushed the master branch from ae44990 to a4a3c55 Compare August 17, 2017 20:52

Ninir approved these changes Aug 17, 2017

View reviewed changes

Ninir changed the title ~~Add support for EFS encryption~~ resource/aws_efs_file_system: Add support for encryption Aug 17, 2017

Ninir merged commit fe3fabc into hashicorp:master Aug 17, 2017

Ninir removed the waiting-response Maintainers are waiting on response from community or contributor. label Aug 17, 2017

Ninir mentioned this pull request Aug 19, 2017

Feature Request: AWS EFS Encyption Option for aws_efs_file_system #1451

Closed

This was referenced Aug 28, 2017

enable encryption in aws efs #1514

Closed

Feature - EFS encryption data at rest now available #1536

Closed

Ninir mentioned this pull request Sep 20, 2017

Support for EFS encryption #1705

Closed

nbaztec pushed a commit to nbaztec/terraform-provider-aws that referenced this pull request Sep 26, 2017

Merge pull request hashicorp#1420 from pjfbashton/master

e77472d

resource/aws_efs_file_system: Add support for encryption

ghost locked and limited conversation to collaborators Apr 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/aws_efs_file_system: Add support for encryption #1420

resource/aws_efs_file_system: Add support for encryption #1420

pavloos commented Aug 15, 2017

Ninir left a comment

Ninir Aug 16, 2017

pavloos Aug 17, 2017

Ninir Aug 16, 2017

pavloos Aug 17, 2017

Ninir Aug 16, 2017

pavloos Aug 17, 2017

Ninir Aug 16, 2017

pavloos commented Aug 17, 2017

Ninir left a comment •

edited

Loading

pavloos commented Aug 18, 2017

shezaan commented Sep 8, 2017

Ninir commented Sep 8, 2017 •

edited

Loading

ghost commented Apr 11, 2020

resource/aws_efs_file_system: Add support for encryption #1420

resource/aws_efs_file_system: Add support for encryption #1420

Conversation

pavloos commented Aug 15, 2017

Ninir left a comment

Choose a reason for hiding this comment

Ninir Aug 16, 2017

Choose a reason for hiding this comment

pavloos Aug 17, 2017

Choose a reason for hiding this comment

Ninir Aug 16, 2017

Choose a reason for hiding this comment

pavloos Aug 17, 2017

Choose a reason for hiding this comment

Ninir Aug 16, 2017

Choose a reason for hiding this comment

pavloos Aug 17, 2017

Choose a reason for hiding this comment

Ninir Aug 16, 2017

Choose a reason for hiding this comment

pavloos commented Aug 17, 2017

Ninir left a comment • edited Loading

Choose a reason for hiding this comment

pavloos commented Aug 18, 2017

shezaan commented Sep 8, 2017

Ninir commented Sep 8, 2017 • edited Loading

ghost commented Apr 11, 2020

Ninir left a comment •

edited

Loading

Ninir commented Sep 8, 2017 •

edited

Loading