r/aws_s3_bucket: support default server side encryption configuration #2472

trung · 2017-11-29T19:57:01Z

Fix #2217 and #2196

API: http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketPUTencryption.html

Modify resource_aws_s3_bucket
Write acceptance test for the resource
Documentation

Related to #2300

…e encryption configuration

radeksimko · 2017-12-11T15:12:41Z

Hey @trung
thanks for the PR. 😃

Do you need any help finishing this one off?

trung · 2017-12-12T00:52:41Z

yes @radeksimko, i made some changes but when the acceptance tests are failing... it takes nearly 15 min to rerun the whole suite ...
Happy to collaborate to push this one out

trung · 2017-12-12T04:22:01Z

Full test suite

❯ make testacc TESTARGS="-run=TestAccAWSS3Bucket_*"

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSS3Bucket_* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSS3BucketNotification_importBasic
--- PASS: TestAccAWSS3BucketNotification_importBasic (129.15s)
=== RUN   TestAccAWSS3Bucket_importBasic
--- PASS: TestAccAWSS3Bucket_importBasic (32.64s)
=== RUN   TestAccAWSS3Bucket_importWithPolicy
--- PASS: TestAccAWSS3Bucket_importWithPolicy (35.07s)
=== RUN   TestAccAWSS3Bucket_Notification
--- PASS: TestAccAWSS3Bucket_Notification (107.14s)
=== RUN   TestAccAWSS3Bucket_NotificationWithoutFilter
--- FAIL: TestAccAWSS3Bucket_NotificationWithoutFilter (28.37s)
	testing.go:503: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		UPDATE: aws_s3_bucket_notification.notification
		  topic.#:                   "0" => "1"
		  topic.0.events.#:          "0" => "2"
		  topic.0.events.3356830603: "" => "s3:ObjectCreated:*"
		  topic.0.events.652504163:  "" => "s3:ObjectRemoved:Delete"
		  topic.0.id:                "" => "notification-sns1"
		  topic.0.topic_arn:         "" => "arn:aws:sns:us-west-2:341947552535:terraform-test-topic-3866553050143056193"

		STATE:

		aws_s3_bucket.bucket:
		  ID = tf-test-bucket-3866553050143056193
		  acceleration_status =
		  acl = public-read
		  arn = arn:aws:s3:::tf-test-bucket-3866553050143056193
		  bucket = tf-test-bucket-3866553050143056193
		  bucket_domain_name = tf-test-bucket-3866553050143056193.s3.amazonaws.com
		  force_destroy = false
		  hosted_zone_id = Z3BJ6K6RIION7M
		  logging.# = 0
		  region = us-west-2
		  request_payer = BucketOwner
		  tags.% = 0
		  versioning.# = 1
		  versioning.0.enabled = false
		  versioning.0.mfa_delete = false
		  website.# = 0
		aws_s3_bucket_notification.notification:
		  ID = tf-test-bucket-3866553050143056193
		  bucket = tf-test-bucket-3866553050143056193
		  lambda_function.# = 0
		  queue.# = 0
		  topic.# = 0

		  Dependencies:
		    aws_s3_bucket.bucket
		    aws_sns_topic.topic
		aws_sns_topic.topic:
		  ID = arn:aws:sns:us-west-2:341947552535:terraform-test-topic-3866553050143056193
		  arn = arn:aws:sns:us-west-2:341947552535:terraform-test-topic-3866553050143056193
		  name = terraform-test-topic-3866553050143056193
		  policy = {"Statement":[{"Action":"SNS:Publish","Condition":{"ArnLike":{"aws:SourceArn":"arn:aws:s3:::tf-test-bucket-3866553050143056193"}},"Effect":"Allow","Principal":{"AWS":"*"},"Resource":"arn:aws:sns:*:*:terraform-test-topic-3866553050143056193","Sid":""}],"Version":"2012-10-17"}

		  Dependencies:
		    aws_s3_bucket.bucket
=== RUN   TestAccAWSS3BucketObject_source
--- PASS: TestAccAWSS3BucketObject_source (31.62s)
=== RUN   TestAccAWSS3BucketObject_content
--- PASS: TestAccAWSS3BucketObject_content (31.83s)
=== RUN   TestAccAWSS3BucketObject_withContentCharacteristics
--- PASS: TestAccAWSS3BucketObject_withContentCharacteristics (30.85s)
=== RUN   TestAccAWSS3BucketObject_updates
--- PASS: TestAccAWSS3BucketObject_updates (51.70s)
=== RUN   TestAccAWSS3BucketObject_updatesWithVersioning
--- PASS: TestAccAWSS3BucketObject_updatesWithVersioning (50.60s)
=== RUN   TestAccAWSS3BucketObject_kms
--- PASS: TestAccAWSS3BucketObject_kms (56.39s)
=== RUN   TestAccAWSS3BucketObject_sse
--- PASS: TestAccAWSS3BucketObject_sse (34.12s)
=== RUN   TestAccAWSS3BucketObject_acl
--- PASS: TestAccAWSS3BucketObject_acl (50.72s)
=== RUN   TestAccAWSS3BucketObject_storageClass
--- PASS: TestAccAWSS3BucketObject_storageClass (48.70s)
=== RUN   TestAccAWSS3BucketObject_tags
--- PASS: TestAccAWSS3BucketObject_tags (31.59s)
=== RUN   TestAccAWSS3BucketPolicy_basic
--- PASS: TestAccAWSS3BucketPolicy_basic (34.70s)
=== RUN   TestAccAWSS3BucketPolicy_policyUpdate
--- PASS: TestAccAWSS3BucketPolicy_policyUpdate (51.21s)
=== RUN   TestAccAWSS3Bucket_basic
--- PASS: TestAccAWSS3Bucket_basic (27.06s)
=== RUN   TestAccAWSS3Bucket_namePrefix
--- PASS: TestAccAWSS3Bucket_namePrefix (27.52s)
=== RUN   TestAccAWSS3Bucket_generatedName
--- PASS: TestAccAWSS3Bucket_generatedName (27.77s)
=== RUN   TestAccAWSS3Bucket_region
--- PASS: TestAccAWSS3Bucket_region (33.18s)
=== RUN   TestAccAWSS3Bucket_acceleration
--- FAIL: TestAccAWSS3Bucket_acceleration (58.68s)
	testing.go:503: Step 1 error: Check failed: Check 2/2 error: aws_s3_bucket.bucket: Attribute 'acceleration_status' expected "Suspended", got "Enabled"
=== RUN   TestAccAWSS3Bucket_RequestPayer
--- PASS: TestAccAWSS3Bucket_RequestPayer (54.99s)
=== RUN   TestAccAWSS3Bucket_Policy
--- PASS: TestAccAWSS3Bucket_Policy (70.83s)
=== RUN   TestAccAWSS3Bucket_UpdateAcl
--- PASS: TestAccAWSS3Bucket_UpdateAcl (52.67s)
=== RUN   TestAccAWSS3Bucket_Website_Simple
--- PASS: TestAccAWSS3Bucket_Website_Simple (78.95s)
=== RUN   TestAccAWSS3Bucket_WebsiteRedirect
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (74.60s)
=== RUN   TestAccAWSS3Bucket_WebsiteRoutingRules
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (57.81s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (58.27s)
=== RUN   TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (53.20s)
=== RUN   TestAccAWSS3Bucket_shouldFailNotFound
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (15.21s)
=== RUN   TestAccAWSS3Bucket_Versioning
--- FAIL: TestAccAWSS3Bucket_Versioning (42.28s)
	testing.go:503: Step 1 error: Check failed: Check 2/2 error: bad error versioning status, found nil, expected: Enabled
=== RUN   TestAccAWSS3Bucket_Cors
--- PASS: TestAccAWSS3Bucket_Cors (51.30s)
=== RUN   TestAccAWSS3Bucket_Logging
--- PASS: TestAccAWSS3Bucket_Logging (46.70s)
=== RUN   TestAccAWSS3Bucket_Lifecycle
--- PASS: TestAccAWSS3Bucket_Lifecycle (72.81s)
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (90.28s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- FAIL: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (42.75s)
	testing.go:503: Step 0 error: Check failed: Check 1/1 error: S3Bucket error: BucketRegionError: incorrect region, the bucket is not in 'eu-west-1' region
			status code: 301, request id: , host id:
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (335.90s)
FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	2209.207s
make: *** [testacc] Error 1

trung · 2017-12-12T04:32:14Z

I reran those failing tests and they all passed

❯ make testacc TESTARGS="-run=TestAccAWSS3Bucket_NotificationWithoutFilter"

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSS3Bucket_NotificationWithoutFilter -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSS3Bucket_NotificationWithoutFilter
--- PASS: TestAccAWSS3Bucket_NotificationWithoutFilter (35.86s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	35.889s

❯ make testacc TESTARGS="-run=TestAccAWSS3Bucket_acceleration"

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSS3Bucket_acceleration -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSS3Bucket_acceleration
--- PASS: TestAccAWSS3Bucket_acceleration (71.42s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	71.452s

❯ make testacc TESTARGS="-run=TestAccAWSS3Bucket_Versioning"

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSS3Bucket_Versioning -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSS3Bucket_Versioning
--- PASS: TestAccAWSS3Bucket_Versioning (72.55s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	72.586s

❯ make testacc TESTARGS="-run=TestAccAWSS3Bucket_ReplicationWithoutStorageClass"

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSS3Bucket_ReplicationWithoutStorageClass -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (62.47s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	62.505s

trung · 2017-12-12T04:33:13Z

@radeksimko once you are ok, i'll do the documentation

modax · 2017-12-12T07:57:39Z

@trung:

--- FAIL: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (42.75s)
	testing.go:503: Step 0 error: Check failed: Check 1/1 error: S3Bucket error: BucketRegionError: incorrect region, the bucket is not in 'eu-west-1' region
			status code: 301, request id: , host id:

This is a bug in acceptance tests which makes some replication tests fail randomly. I have (hopefully) fixed it in bc018da (PR #2625)

radeksimko · 2017-12-12T09:09:39Z

@trung Unfortunately there are some S3 tests which are intermittently failing. Some of those failures are bugs we could fix (outside of the scope of this PR), some are just caused by eventually consistent nature of S3 and there isn't much we can realistically do.

I have seen every one of the failures you posted above at least once - it's unrelated to your PR, so don't worry about these here. Feel free to carry on with documentation.

modax · 2017-12-12T13:41:01Z

@trung: as far as I can tell, there is no "refresh" support. I disabled default encryption via aws web console and tf did not detect it.

trung · 2017-12-13T01:44:16Z

@modax thanks for reporting. I've put a fix.

psyvision · 2017-12-13T17:11:25Z

@trung have you tried this with AES256? I note there is no unit test for it. The reason I ask is that I merged your PR into a custom branch I have. On using AES256 it told me that KMS key should not also be specified (obviously), yet none was specified. Don't have the exact error to hand at the moment.

trung · 2017-12-13T19:16:12Z

@psyvision good point, i will add that test case in. I think you are right, for AES256, i should not supply KMSKeyId along

jen20

This looks good to me, save for a couple of minor changes. I'll take this one over the line and get it committed. Thanks for all the work so far, @trung and @psyvision!

jen20 · 2017-12-14T16:23:05Z

aws/resource_aws_s3_bucket.go

+												},
+												"sse_algorithm": {
+													Type:     schema.TypeString,
+													Required: true,


I think we should consider adding a validation function here - the valid values are aws:kms or AES256. It's conceivable that this list will expand in the future, but additional values can easily be added. This will move a potential error from apply time to plan time, which is desirable.

jen20 · 2017-12-14T16:23:27Z

aws/resource_aws_s3_bucket.go

@@ -1739,6 +1868,25 @@ func resourceAwsS3BucketLifecycleUpdate(s3conn *s3.S3, d *schema.ResourceData) e
 	return nil
 }

+func flatternAwsS3ServerSideEncryptionConfiguration(c *s3.ServerSideEncryptionConfiguration) []map[string]interface{} {


Typo: flattern -> flatten

trung · 2017-12-14T16:42:32Z

@jen20 thanks for the review. I'll make some modifications per your comments

jen20 · 2017-12-14T16:48:00Z

@trung No need, I'll make them as I merge. Thanks!

jen20 · 2017-12-17T00:44:42Z

Thanks @trung, this landed in 019e6e0.

grimm26 · 2017-12-19T16:02:13Z

Thanks, but where I could make SSE toggle-able in a policy, I cannot with this implementation :(

shavo007 · 2017-12-21T03:22:50Z

great work guys. is there a label to define what version of aws provider you need to run this? i was on 1.5 and it complained that it could not interpret this resource. i nuked my dotfiles, reinstalled (with aws 1.6) and worked.

psyvision · 2017-12-21T09:23:59Z

@shavo007 It's been added to the change log under 1.6.0 improvements.

shavo007 · 2017-12-21T10:52:24Z

No worries. Is it possible to add a milestone label in future?

ghost · 2020-04-08T17:59: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!

trung added 2 commits November 29, 2017 14:53

hashicorp#2217: updated r/aws_s3_bucket to support default server sid…

897ac16

…e encryption configuration

hashicorp#2217: make fmt

c74d8af

radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 30, 2017

Merge branch 'master' into f-r-2217

f03a0a1

radeksimko mentioned this pull request Dec 11, 2017

[dirty] d/aws_s3_bucket: get default encryption configuration #2300

Closed

3 tasks

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 11, 2017

Merge branch 'master' into f-r-2217

3a3c6da

trung added 2 commits December 11, 2017 20:53

hashicorp#2217: wrote typical acceptance test

aeb4624

hashicorp#2217: wrote additional acceptance test

aaa2357

trung added 3 commits December 12, 2017 20:13

Merge branch 'master' into f-r-2217

1698f93

hashicorp#2217: re-enable default encryption after disabling in via UI

1f3fd42

hashicorp#2217: documentation

f19e809

trung changed the title ~~[WIP] r/aws_s3_bucket: support default server side encryption configuration~~ r/aws_s3_bucket: support default server side encryption configuration Dec 13, 2017

trung added 2 commits December 13, 2017 21:22

Merge branch 'master' into f-r-2217

bfa7843

hashicorp#2217: fixed when encryption is AES256

df3405e

jen20 reviewed Dec 14, 2017

View reviewed changes

jen20 closed this in 019e6e0 Dec 17, 2017

modax mentioned this pull request Dec 18, 2017

r/aws_s3_bucket: support SSE-KMS replication configuration #2625

Merged

This was referenced Dec 19, 2017

Feature: S3 bucket-wide default encryption #2196

Closed

Feature request: New S3 encryption support #2217

Closed

trung deleted the f-r-2217 branch December 21, 2017 15:11

ghost locked and limited conversation to collaborators Apr 8, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_s3_bucket: support default server side encryption configuration #2472

r/aws_s3_bucket: support default server side encryption configuration #2472

trung commented Nov 29, 2017 •

edited

Loading

radeksimko commented Dec 11, 2017

trung commented Dec 12, 2017

trung commented Dec 12, 2017

trung commented Dec 12, 2017

trung commented Dec 12, 2017

modax commented Dec 12, 2017 •

edited

Loading

radeksimko commented Dec 12, 2017

modax commented Dec 12, 2017

trung commented Dec 13, 2017

psyvision commented Dec 13, 2017

trung commented Dec 13, 2017

jen20 left a comment

jen20 Dec 14, 2017

jen20 Dec 14, 2017

trung commented Dec 14, 2017

jen20 commented Dec 14, 2017

jen20 commented Dec 17, 2017

grimm26 commented Dec 19, 2017

shavo007 commented Dec 21, 2017

psyvision commented Dec 21, 2017

shavo007 commented Dec 21, 2017

ghost commented Apr 8, 2020

r/aws_s3_bucket: support default server side encryption configuration #2472

r/aws_s3_bucket: support default server side encryption configuration #2472

Conversation

trung commented Nov 29, 2017 • edited Loading

radeksimko commented Dec 11, 2017

trung commented Dec 12, 2017

trung commented Dec 12, 2017

trung commented Dec 12, 2017

trung commented Dec 12, 2017

modax commented Dec 12, 2017 • edited Loading

radeksimko commented Dec 12, 2017

modax commented Dec 12, 2017

trung commented Dec 13, 2017

psyvision commented Dec 13, 2017

trung commented Dec 13, 2017

jen20 left a comment

Choose a reason for hiding this comment

jen20 Dec 14, 2017

Choose a reason for hiding this comment

jen20 Dec 14, 2017

Choose a reason for hiding this comment

trung commented Dec 14, 2017

jen20 commented Dec 14, 2017

jen20 commented Dec 17, 2017

grimm26 commented Dec 19, 2017

shavo007 commented Dec 21, 2017

psyvision commented Dec 21, 2017

shavo007 commented Dec 21, 2017

ghost commented Apr 8, 2020

trung commented Nov 29, 2017 •

edited

Loading

modax commented Dec 12, 2017 •

edited

Loading