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

modax · 2017-12-11T12:56:24Z

It would be great to get this merged soon. Thanks!

make testacc TESTARGS="-run 'TestAccAWSS3Bucket_'"                                                   
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run 'TestAccAWSS3Bucket_' -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSS3Bucket_importBasic
--- PASS: TestAccAWSS3Bucket_importBasic (79.48s)
=== RUN   TestAccAWSS3Bucket_importWithPolicy
--- PASS: TestAccAWSS3Bucket_importWithPolicy (86.50s)
=== RUN   TestAccAWSS3Bucket_basic
--- PASS: TestAccAWSS3Bucket_basic (65.44s)
=== RUN   TestAccAWSS3Bucket_namePrefix
--- PASS: TestAccAWSS3Bucket_namePrefix (65.88s)
=== RUN   TestAccAWSS3Bucket_generatedName
--- PASS: TestAccAWSS3Bucket_generatedName (66.26s)
=== RUN   TestAccAWSS3Bucket_region
--- PASS: TestAccAWSS3Bucket_region (39.46s)
=== RUN   TestAccAWSS3Bucket_acceleration
--- PASS: TestAccAWSS3Bucket_acceleration (73.89s)
=== RUN   TestAccAWSS3Bucket_RequestPayer
--- PASS: TestAccAWSS3Bucket_RequestPayer (123.23s)
=== RUN   TestAccAWSS3Bucket_Policy
--- PASS: TestAccAWSS3Bucket_Policy (168.01s)
=== RUN   TestAccAWSS3Bucket_UpdateAcl
--- PASS: TestAccAWSS3Bucket_UpdateAcl (120.25s)
=== RUN   TestAccAWSS3Bucket_Website_Simple
--- PASS: TestAccAWSS3Bucket_Website_Simple (183.89s)
=== RUN   TestAccAWSS3Bucket_WebsiteRedirect
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (185.42s)
=== RUN   TestAccAWSS3Bucket_WebsiteRoutingRules
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (128.49s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (109.67s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed (67.26s)
=== RUN   TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (124.96s)
=== RUN   TestAccAWSS3Bucket_shouldFailNotFound
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (36.49s)
=== RUN   TestAccAWSS3Bucket_Versioning
--- PASS: TestAccAWSS3Bucket_Versioning (178.60s)
=== RUN   TestAccAWSS3Bucket_Cors
--- PASS: TestAccAWSS3Bucket_Cors (131.24s)
=== RUN   TestAccAWSS3Bucket_Logging
--- PASS: TestAccAWSS3Bucket_Logging (103.32s)
=== RUN   TestAccAWSS3Bucket_Lifecycle
--- PASS: TestAccAWSS3Bucket_Lifecycle (176.16s)
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (254.52s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (98.22s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (50.31s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       2716.962s

modax · 2017-12-18T23:22:41Z

Glad to see a release with #2472. This one would have made a nice pairing (and it still does)!

johvet · 2018-01-13T16:41:11Z

We'd also love to see this coming soon.

handlerbot · 2018-01-19T13:37:02Z

@radeksimko Any chance this can make the 1.8.0 milestone? (Or is that one full up? 🙏)

handlerbot · 2018-01-23T01:05:36Z

@bflad @Ninir Any chance this can make the 1.8.0 milestone? 🔮 :-)

cornfeedhobo · 2018-01-27T02:37:08Z

@jen20 @radeksimko any hope here?

cornfeedhobo · 2018-02-06T21:51:03Z

can haz merge?

cornfeedhobo · 2018-02-13T16:55:33Z

@modax Looks like you might have to take another pass here. Thanks for owning this!

jrstarke · 2018-02-15T22:29:23Z

@modax: looks like this has conflicts now

jrstarke · 2018-02-16T17:33:40Z

Looks like there are some conflicts. If we get the conflicts resolved, can we get this merged?

Given that we are deep with TypeSets here in Destination with a dynamic key (i.e. destination bucket name), we cannot use simple resource.TestCheckResourceAttr anymore due to hard to predict set hash. Therefore use S3 API object matching instead (for replication rules). This iead was stolen from testAccCheckAWSS3BucketWebsiteRoutingRules.

modax · 2018-02-18T19:18:30Z

Fixed conflicts (removed two commits addressing other related problems that already been fixed in master), reran acc tests.

However, it is really unfortunate that PR merging cycle takes such a long time...

tombuildsstuff

hey @modax

Thanks for this PR (and rebasing it) - apologies for the delayed review here!

I've taken a look through and raised one comment around the hash function which needs @radeksimko or @bflad to confirm is ok, but this otherwise LGTM 👍 I'll kick off the test suite now to confirm the tests are still good after the rebase

Thanks!

tombuildsstuff · 2018-02-20T01:52:29Z

aws/resource_aws_s3_bucket.go

@@ -2086,6 +2154,12 @@ func rulesHash(v interface{}) int {
 	if v, ok := m["status"]; ok {
 		buf.WriteString(fmt.Sprintf("%s-", v.(string)))
 	}
+	if v, ok := m["destination"].(*schema.Set); ok && v.Len() > 0 {
+		buf.WriteString(fmt.Sprintf("%d-", destinationHash(v.List()[0])))
+	}


Given this is adding an existing field to the hashcode function - this will appear as a diff to existing resources, I believe that should be fine, but @radeksimko / @bflad can confirm for sure

In that case I think we should add state migration, so the diff doesn't appear for existing users who just upgraded and not use any new functionality.

hi, @radeksimko,

I have just confirmed that neither of these:

terraform apply

terraform apply -refresh=false

terraform refresh

report any changes after provider upgrade. Rule hash value changes in the state file after all of them though. Frankly, I have also expected 2) to report changes but for some reason it does not happen (must be related to how set hash is compared probably, i.e. always upfront or something).

Here is the tf code if you want to test yourself: https://gist.github.com/modax/9e13db22565ab2f1a26821e5ee5995e5

confirmed that scenario ^ - LGTM 👍

tombuildsstuff · 2018-02-20T02:38:28Z

Tests pass:

This was done in the spirit of current testAccCheckAWSS3BucketExistsWithProvider. Previous provider for loop has been deprecated as of late.

modax · 2018-02-20T04:43:15Z

@tombuildsstuff Thanks for review. Sorry for the late change, but I've just realized that my testAccCheckAWSS3BucketReplicationRules was still using old "provider for loop" which deemed to be deprecated and was refactored in testAccCheckAWSS3BucketExistsWithProvider with specific provider function. So I have refactored my func in the spirit of the new testAccCheckAWSS3BucketExistsWithProvider as well: 2b643af

Hopefully this does not delay merge...

$ make testacc TESTARGS="-run 'TestAccAWSS3Bucket_Replication'" 
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run 'TestAccAWSS3Bucket_Replication' -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (271.27s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (104.66s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (54.79s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       430.744s

bflad · 2018-02-27T13:03:10Z

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

cornfeedhobo · 2018-02-28T01:00:41Z

This seriously might make my quarter. Thank you @modax for getting this in!!

jrstarke · 2018-03-01T00:21:01Z

Oh man, this has me super bummed. I thought that this was exactly what I needed, but it turns out I also need the Account and AccessControlTranslation arguments. Without those, all of the files getting replicated to my replication account are un-usable, unless the owner account explicitly grants access to all of them.

https://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketPUTreplication.html

jnoss · 2018-09-18T16:52:27Z

+1 for #3575 to get this working for cross-account replication, too

ghost · 2020-04-03T17:34:33Z

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!

modax force-pushed the r/aws-s3-bucket-sse-kms-replication branch 2 times, most recently from 61b9b4d to bc018da Compare December 11, 2017 17:46

modax mentioned this pull request Dec 12, 2017

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

Closed

3 tasks

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

modax force-pushed the r/aws-s3-bucket-sse-kms-replication branch from bc018da to 55a00de Compare December 18, 2017 11:08

bflad added the service/s3 Issues and PRs that pertain to the s3 service. label Jan 28, 2018

handlerbot mentioned this pull request Jan 30, 2018

Don't mangle DNS aliases: Trim{Prefix,Suffix} not Trim{Left,Right}. #3187

Merged

vladholubiev approved these changes Jan 31, 2018

View reviewed changes

tfjaeckel approved these changes Jan 31, 2018

View reviewed changes

cornfeedhobo mentioned this pull request Feb 16, 2018

AWS S3 cross region replication using AWS-KMS terraform code #2226

Closed

modax force-pushed the r/aws-s3-bucket-sse-kms-replication branch from 55a00de to 5696b4a Compare February 18, 2018 11:41

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 18, 2018

modax force-pushed the r/aws-s3-bucket-sse-kms-replication branch from 5696b4a to 2b42802 Compare February 18, 2018 19:13

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 18, 2018

Support S3 replication configuration with SS3-KMS.

2c8906e

modax force-pushed the r/aws-s3-bucket-sse-kms-replication branch from 2b42802 to 2c8906e Compare February 18, 2018 19:20

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 18, 2018

tombuildsstuff approved these changes Feb 20, 2018

View reviewed changes

Improve testAccCheckAWSS3BucketReplicationRules to use specifc provider.

2b643af

This was done in the spirit of current testAccCheckAWSS3BucketExistsWithProvider. Previous provider for loop has been deprecated as of late.

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 20, 2018

tombuildsstuff merged commit fda88a7 into hashicorp:master Feb 22, 2018

tombuildsstuff added a commit that referenced this pull request Feb 22, 2018

Updating to include #2625

a78d419

bflad added this to the v1.10.0 milestone Feb 27, 2018

jrstarke mentioned this pull request Mar 1, 2018

S3 Cross-Account Cross-Region Replication with KMS #3575

Closed

modax deleted the r/aws-s3-bucket-sse-kms-replication branch March 8, 2018 12:27

ghost locked and limited conversation to collaborators Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

modax commented Dec 11, 2017 •

edited

Loading

modax commented Dec 18, 2017

johvet commented Jan 13, 2018

handlerbot commented Jan 19, 2018

handlerbot commented Jan 23, 2018

cornfeedhobo commented Jan 27, 2018

cornfeedhobo commented Feb 6, 2018

cornfeedhobo commented Feb 13, 2018

jrstarke commented Feb 15, 2018

jrstarke commented Feb 16, 2018

modax commented Feb 18, 2018

tombuildsstuff left a comment

tombuildsstuff Feb 20, 2018

radeksimko Feb 20, 2018

modax Feb 20, 2018 •

edited

Loading

tombuildsstuff Feb 22, 2018

tombuildsstuff commented Feb 20, 2018

modax commented Feb 20, 2018

bflad commented Feb 27, 2018

cornfeedhobo commented Feb 28, 2018

jrstarke commented Mar 1, 2018 •

edited

Loading

jnoss commented Sep 18, 2018

ghost commented Apr 3, 2020

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

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

Conversation

modax commented Dec 11, 2017 • edited Loading

modax commented Dec 18, 2017

johvet commented Jan 13, 2018

handlerbot commented Jan 19, 2018

handlerbot commented Jan 23, 2018

cornfeedhobo commented Jan 27, 2018

cornfeedhobo commented Feb 6, 2018

cornfeedhobo commented Feb 13, 2018

jrstarke commented Feb 15, 2018

jrstarke commented Feb 16, 2018

modax commented Feb 18, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Feb 20, 2018

Choose a reason for hiding this comment

radeksimko Feb 20, 2018

Choose a reason for hiding this comment

modax Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

tombuildsstuff Feb 22, 2018

Choose a reason for hiding this comment

tombuildsstuff commented Feb 20, 2018

modax commented Feb 20, 2018

bflad commented Feb 27, 2018

cornfeedhobo commented Feb 28, 2018

jrstarke commented Mar 1, 2018 • edited Loading

jnoss commented Sep 18, 2018

ghost commented Apr 3, 2020

modax commented Dec 11, 2017 •

edited

Loading

modax Feb 20, 2018 •

edited

Loading

jrstarke commented Mar 1, 2018 •

edited

Loading