Adds the S3 Replication AccessControlTranslation and Account arguments #3577

jrstarke · 2018-03-01T06:22:21Z

This allows the S3 Replication to translate the owner of the objects on replication (GH-3575). This is particularly useful when you're replicating to a separate AWS Account, because otherwise, objects are by default not accessible beyond the original account. With translation, or replication, the replica account becomes the owner of the replica objects.

I'm new to Go and contributing to the Terraform Providers, so I'd love some feedback/guidance. But this feature is important enough to me that I felt it was worth the effort to learn.

I'm unsure how to set up an acceptance test for this feature, because it is, by design, cross account.

I tried running the acceptance tests, without modification, and they didn't seem to run as I would have expected

make testacc TESTARGS="-run 'TestAccAWSS3Bucket_'"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	0.879s [no tests to run]
make: *** No rule to make target `'TestAccAWSS3Bucket_''.  Stop.

I would appreciate whatever help or guidance you can provide, as I'd love to see this feature complete.

seanscofield · 2018-05-08T19:50:18Z

Just expressing my interest in this as well. I run a bunch of EC2 instances that continually write stuff to S3 (which then get replicated to another account), and the lack of this feature means that I have to pause my cluster and let things drain out for a few hours before I can deploy my terraform; deploying breaks the replication configuration, at which point I need to use aws cli to reset it

caarl · 2018-05-18T16:20:39Z

A +1 from me for this feature too. I'm in a similar position to @seanscofield with data continually being written to a bucket. Unfortunately in my scenario I cannot pause the data coming in which leaves me with an issue if Terraform resets the rules and data is replicated before reapplying outside of it.

Can anyone give an indication on when this feature is likely to become available @radeksimko ?

mdlavin · 2018-06-01T01:26:35Z

If anybody is interested in testing out this feature in a patched v1.21.0 version, I've made some Alpine Linux x64 binaries available here: https://github.com/lifeomic/terraform-provider-aws/releases/tag/v1.21.0_patched_5f7d0def

mdlavin · 2018-09-07T12:49:55Z

@radeksimko is there anything I can do to help this get merged into an official release?

jrstarke · 2018-09-07T13:07:13Z

I'd be happy to make any changes necessary to get this in too. I'd especially love some feedback on the code and tests

jrstarke · 2018-09-10T05:15:03Z

$ make testacc TESTARGS="-run 'TestAccAWSS3Bucket_Replication'"
=== RUN   TestAccAWSS3Bucket_Replication
--- PASS: TestAccAWSS3Bucket_Replication (266.92s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (238.43s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (38.94s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	544.363s

jrstarke · 2018-09-10T14:53:33Z

Ok, I run the whole set of s3 bucket acceptance tests. They seem to be inconsistent.

$ make testacc TESTARGS="-run 'TestAccAWSS3Bucket_'"
Enter MFA code for arn:aws:iam::972822414733:mfa/jamie-cli:
==> 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 (18.00s)
=== RUN   TestAccAWSS3Bucket_importWithPolicy
--- PASS: TestAccAWSS3Bucket_importWithPolicy (18.91s)
=== RUN   TestAccAWSS3Bucket_basic
--- PASS: TestAccAWSS3Bucket_basic (28.70s)
=== RUN   TestAccAWSS3Bucket_namePrefix
--- PASS: TestAccAWSS3Bucket_namePrefix (15.34s)
=== RUN   TestAccAWSS3Bucket_generatedName
--- PASS: TestAccAWSS3Bucket_generatedName (14.99s)
=== RUN   TestAccAWSS3Bucket_region
--- PASS: TestAccAWSS3Bucket_region (43.15s)
=== RUN   TestAccAWSS3Bucket_acceleration
--- PASS: TestAccAWSS3Bucket_acceleration (40.76s)
=== RUN   TestAccAWSS3Bucket_RequestPayer
--- PASS: TestAccAWSS3Bucket_RequestPayer (26.65s)
=== RUN   TestAccAWSS3Bucket_Policy
--- PASS: TestAccAWSS3Bucket_Policy (36.72s)
=== RUN   TestAccAWSS3Bucket_UpdateAcl
--- PASS: TestAccAWSS3Bucket_UpdateAcl (27.86s)
=== RUN   TestAccAWSS3Bucket_Website_Simple
--- PASS: TestAccAWSS3Bucket_Website_Simple (39.43s)
=== RUN   TestAccAWSS3Bucket_WebsiteRedirect
--- FAIL: TestAccAWSS3Bucket_WebsiteRedirect (28.45s)
    testing.go:527: Step 1 error: After applying this step and refreshing, the plan was not empty:

        DIFF:

        UPDATE: aws_s3_bucket.bucket
          website.0.redirect_all_requests_to: "hashicorp.com?my=query" => "https://hashicorp.com?my=query"

        STATE:

        aws_s3_bucket.bucket:
          ID = tf-test-bucket-8128057372481047653
          provider = provider.aws
          acceleration_status =
          acl = public-read
          arn = arn:aws:s3:::tf-test-bucket-8128057372481047653
          bucket = tf-test-bucket-8128057372481047653
          bucket_domain_name = tf-test-bucket-8128057372481047653.s3.amazonaws.com
          bucket_regional_domain_name = tf-test-bucket-8128057372481047653.s3.us-west-2.amazonaws.com
          cors_rule.# = 0
          force_destroy = false
          hosted_zone_id = Z3BJ6K6RIION7M
          logging.# = 0
          region = us-west-2
          replication_configuration.# = 0
          request_payer = BucketOwner
          server_side_encryption_configuration.# = 0
          tags.% = 0
          versioning.# = 1
          versioning.0.enabled = false
          versioning.0.mfa_delete = false
          website.# = 1
          website.0.error_document =
          website.0.index_document =
          website.0.redirect_all_requests_to = hashicorp.com?my=query
          website.0.routing_rules =
          website_domain = s3-website-us-west-2.amazonaws.com
          website_endpoint = tf-test-bucket-8128057372481047653.s3-website-us-west-2.amazonaws.com
=== RUN   TestAccAWSS3Bucket_WebsiteRoutingRules
--- PASS: TestAccAWSS3Bucket_WebsiteRoutingRules (28.88s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenTypical (37.99s)
=== RUN   TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed
--- PASS: TestAccAWSS3Bucket_enableDefaultEncryption_whenAES256IsUsed (15.08s)
=== RUN   TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled
--- PASS: TestAccAWSS3Bucket_disableDefaultEncryption_whenDefaultEncryptionIsEnabled (28.00s)
=== RUN   TestAccAWSS3Bucket_shouldFailNotFound
--- PASS: TestAccAWSS3Bucket_shouldFailNotFound (9.82s)
=== RUN   TestAccAWSS3Bucket_Versioning
--- FAIL: TestAccAWSS3Bucket_Versioning (36.19s)
    testing.go:527: Step 2 error: Check failed: Check 2/2 error: bad error versioning status, expected: Suspended, got Enabled
=== RUN   TestAccAWSS3Bucket_Cors
--- FAIL: TestAccAWSS3Bucket_Cors (51.75s)
    testing.go:527: Step 1 error: Check failed: Check 2/2 error: bad error cors rule, expected: [{
          AllowedHeaders: ["*"],
          AllowedMethods: ["PUT","POST"],
          AllowedOrigins: ["https://www.example.com"],
          ExposeHeaders: ["x-amz-server-side-encryption","ETag"],
          MaxAgeSeconds: 3000
        }], got [{
          AllowedHeaders: ["*"],
          AllowedMethods: ["GET"],
          AllowedOrigins: ["https://www.example.com"]
        }]
    testing.go:527: Step 0 error: Check failed: Check 2/2 error: NoSuchBucket: The specified bucket does not exist
          status code: 404, request id: 05A7772F9FB4A77D, host id: 2bhuZ1A6X5EqXFQq/jNdO2j/l8NDFdNh6K/CBWFHrCkOdw5LLKsuGrZZK4uaz/7oVfze41l7/BI=
=== RUN   TestAccAWSS3Bucket_Logging
--- PASS: TestAccAWSS3Bucket_Logging (22.99s)
=== RUN   TestAccAWSS3Bucket_Lifecycle
--- PASS: TestAccAWSS3Bucket_Lifecycle (39.53s)
=== RUN   TestAccAWSS3Bucket_LifecycleExpireMarkerOnly
--- PASS: TestAccAWSS3Bucket_LifecycleExpireMarkerOnly (26.65s)
=== RUN   TestAccAWSS3Bucket_Replication
--- FAIL: TestAccAWSS3Bucket_Replication (80.26s)
    testing.go:527: Step 1 error: Check failed: Check 2/6 error: aws_s3_bucket.bucket: Attribute 'replication_configuration.#' expected "1", got "0"
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (57.88s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (37.13s)
FAIL
FAIL  github.com/terraform-providers/terraform-provider-aws/aws 811.156s
make: *** [testacc] Error 1
$ make testacc TESTARGS="-run 'TestAccAWSS3Bucket_WebsiteRedirect'"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run 'TestAccAWSS3Bucket_WebsiteRedirect' -timeout 120m
?     github.com/terraform-providers/terraform-provider-aws [no test files]
=== RUN   TestAccAWSS3Bucket_WebsiteRedirect
--- PASS: TestAccAWSS3Bucket_WebsiteRedirect (41.64s)
PASS
ok    github.com/terraform-providers/terraform-provider-aws/aws 41.676s
$ make testacc TESTARGS="-run 'TestAccAWSS3Bucket_Versioning'"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run 'TestAccAWSS3Bucket_Versioning' -timeout 120m
?     github.com/terraform-providers/terraform-provider-aws [no test files]
=== RUN   TestAccAWSS3Bucket_Versioning
--- PASS: TestAccAWSS3Bucket_Versioning (38.33s)
PASS
ok    github.com/terraform-providers/terraform-provider-aws/aws 38.362s
$ make testacc TESTARGS="-run 'TestAccAWSS3Bucket_Cors'"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run 'TestAccAWSS3Bucket_Cors' -timeout 120m
?     github.com/terraform-providers/terraform-provider-aws [no test files]
=== RUN   TestAccAWSS3Bucket_Cors
--- PASS: TestAccAWSS3Bucket_Cors (61.15s)
PASS
ok    github.com/terraform-providers/terraform-provider-aws/aws 61.187s
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 (212.57s)
=== RUN   TestAccAWSS3Bucket_ReplicationWithoutStorageClass
--- PASS: TestAccAWSS3Bucket_ReplicationWithoutStorageClass (75.38s)
=== RUN   TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError
--- PASS: TestAccAWSS3Bucket_ReplicationExpectVersioningValidationError (36.19s)
PASS
ok    github.com/terraform-providers/terraform-provider-aws/aws 324.205s

mdlavin · 2018-09-13T20:22:41Z

If anybody is interested in testing out this feature in a patched v1.36.0 version, I've made some Alpine Linux x64 binaries available here: https://github.com/lifeomic/terraform-provider-aws/releases/tag/v1.36.0_patched_f2d0f833c

aws/resource_aws_s3_bucket.go

ewbankkit · 2018-10-10T13:46:51Z

aws/resource_aws_s3_bucket.go

@@ -377,6 +377,10 @@ func resourceAwsS3Bucket() *schema.Resource {
 										Set:      destinationHash,
 										Elem: &schema.Resource{
 											Schema: map[string]*schema.Schema{
+												"account": {


I'd suggest renaming this to account_id to match similar places in the code base.
Also can add ValidateFunc: validateAwsAccountId,.

Both these seem reasonable and will be updated on merge.

aws/resource_aws_s3_bucket.go

* Switch replication rule destination access_control_translation from TypeSet to TypeList * Add replication rule destination account validation * Add replication rule destination access_control_translation owner validation * Split replication rule destination access_control_translation acceptance testing into its own function and add non-encrypted configuration TestStep * Ensure testAccCheckAWSS3BucketReplicationRules can translate HIL references in Rule.Destination.Account

bflad · 2018-10-10T15:04:28Z

@ewbankkit thanks for the feedback here - I have some in-flight changes already and will try to incorporate some of your feedback.

bflad

Hi @jrstarke 👋 Thank you so much for submitting this and your patience! Overall it was in fantastic shape. I hope you don't mind I made a few minor adjustments in a followup commit on merge (a9c5649) based on mine and @ewbankkit's feedback below just so we could get this released today.

LGTM after running the acceptance testing multiple passes. 🚀

bflad · 2018-10-10T15:19:27Z

aws/resource_aws_s3_bucket_test.go

+							{
+								ID: aws.String("foobar"),
+								Destination: &s3.Destination{
+									Account:      aws.String("${data.aws_caller_identity.current.account_id}"),


I'm not sure how this was passing previously, but I needed to add the HIL translation for this parameter in testAccCheckAWSS3BucketReplicationRules for the testing to pass for me. 😄

if account := dest.Account; account != nil && strings.HasPrefix(aws.StringValue(dest.Account), "${") { resourceReference := strings.Replace(aws.StringValue(dest.Account), "${", "", 1) resourceReference = strings.Replace(resourceReference, "}", "", 1) resourceReferenceParts := strings.Split(resourceReference, ".") resourceAttribute := resourceReferenceParts[len(resourceReferenceParts)-1] resourceName := strings.Join(resourceReferenceParts[:len(resourceReferenceParts)-1], ".") value := s.RootModule().Resources[resourceName].Primary.Attributes[resourceAttribute] dest.Account = aws.String(value) }

We should probably move this logic to its own function so it can be reused in other testing. 😅

bflad · 2018-10-10T15:21:06Z

aws/resource_aws_s3_bucket_test.go

@@ -731,6 +731,42 @@ func TestAccAWSS3Bucket_Cors_EmptyOrigin(t *testing.T) {
 					),
 				),
 			},
+			{


We generally prefer to create new, separate acceptance test functions for new attributes 👍

bflad · 2018-10-10T15:48:32Z

aws/resource_aws_s3_bucket.go

+			}
+			if v.Destination.AccessControlTranslation != nil {
+				rdt := make(map[string]interface{})
+				rdt["owner"] = *v.Destination.AccessControlTranslation.Owner


To prevent potential panics, we prefer to use the AWS Go SDK provided methods for dereferencing, e.g. aws.StringValue(v.Destination.AccessControlTranslation.Owner) 👍

* Switch replication rule destination access_control_translation from TypeSet to TypeList * Change replication rule destination account attribute naming to account_id * Add replication rule destination account_id validation * Add replication rule destination access_control_translation owner validation * Split replication rule destination access_control_translation acceptance testing into its own function and add non-encrypted configuration TestStep * Ensure testAccCheckAWSS3BucketReplicationRules can translate HIL references in Rule.Destination.Account

bflad · 2018-10-10T19:58:29Z

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

ghost · 2020-04-02T17:58:40Z

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!

ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 1, 2018

radeksimko added enhancement Requests to existing resources that expand the functionality or scope. service/s3 Issues and PRs that pertain to the s3 service. labels Mar 1, 2018

jrstarke force-pushed the s3AclTranslation branch from 3f4370c to f0e9511 Compare March 12, 2018 20:24

ghost added size/M Managed by automation to categorize the size of a PR. size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Mar 12, 2018

jrstarke changed the title ~~[WIP] Adds the S3 Replication AccessControlTranslation and Account arguments~~ Adds the S3 Replication AccessControlTranslation and Account arguments Sep 7, 2018

jrstarke force-pushed the s3AclTranslation branch from 9370b16 to 677035f Compare September 8, 2018 02:54

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

jrstarke mentioned this pull request Sep 19, 2018

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

Closed

bflad mentioned this pull request Sep 20, 2018

r/aws_s3_bucket: Support S3 Cross-Region Replication filtering based on S3 object tags #5940

Closed

ewbankkit mentioned this pull request Sep 20, 2018

r/aws_s3_bucket: Add support for S3 Cross-Region Replication ACL Overwrite #5447

Closed

Jamie Starke added 2 commits October 1, 2018 14:31

Adds the AccessControlTranslation and Account arguments

039a132

Add basic test for Access Control Translation

4518e09

jrstarke force-pushed the s3AclTranslation branch from 78d828a to 4518e09 Compare October 1, 2018 21:31

ghost added documentation Introduces or discusses updates to documentation. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 1, 2018

ewbankkit reviewed Oct 10, 2018

View reviewed changes

aws/resource_aws_s3_bucket.go Show resolved Hide resolved

ewbankkit reviewed Oct 10, 2018

View reviewed changes

aws/resource_aws_s3_bucket.go Show resolved Hide resolved

bflad added this to the v1.40.0 milestone Oct 10, 2018

bflad approved these changes Oct 10, 2018

View reviewed changes

bflad merged commit 4518e09 into hashicorp:master Oct 10, 2018

bflad added a commit that referenced this pull request Oct 10, 2018

Update CHANGELOG for #3577

f952a9e

ghost locked and limited conversation to collaborators Apr 2, 2020

jrstarke deleted the s3AclTranslation branch October 8, 2020 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the S3 Replication AccessControlTranslation and Account arguments #3577

Adds the S3 Replication AccessControlTranslation and Account arguments #3577

jrstarke commented Mar 1, 2018 •

edited

Loading

seanscofield commented May 8, 2018

caarl commented May 18, 2018

mdlavin commented Jun 1, 2018

mdlavin commented Sep 7, 2018

jrstarke commented Sep 7, 2018

jrstarke commented Sep 10, 2018

jrstarke commented Sep 10, 2018

mdlavin commented Sep 13, 2018

ewbankkit Oct 10, 2018

bflad Oct 10, 2018

bflad commented Oct 10, 2018

bflad left a comment

bflad Oct 10, 2018

bflad Oct 10, 2018

bflad Oct 10, 2018

bflad commented Oct 10, 2018

ghost commented Apr 2, 2020

@@ @@ -731,6 +731,42 @@ func TestAccAWSS3Bucket_Cors_EmptyOrigin(t *testing.T) { @@
               					),
               				),
               			},
+              			{

Adds the S3 Replication AccessControlTranslation and Account arguments #3577

Adds the S3 Replication AccessControlTranslation and Account arguments #3577

Conversation

jrstarke commented Mar 1, 2018 • edited Loading

seanscofield commented May 8, 2018

caarl commented May 18, 2018

mdlavin commented Jun 1, 2018

mdlavin commented Sep 7, 2018

jrstarke commented Sep 7, 2018

jrstarke commented Sep 10, 2018

jrstarke commented Sep 10, 2018

mdlavin commented Sep 13, 2018

ewbankkit Oct 10, 2018

Choose a reason for hiding this comment

bflad Oct 10, 2018

Choose a reason for hiding this comment

bflad commented Oct 10, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Oct 10, 2018

Choose a reason for hiding this comment

bflad Oct 10, 2018

Choose a reason for hiding this comment

bflad Oct 10, 2018

Choose a reason for hiding this comment

bflad commented Oct 10, 2018

ghost commented Apr 2, 2020

jrstarke commented Mar 1, 2018 •

edited

Loading