Add delete marker replication to v2 s3 config #19323

mkrant · 2021-05-12T01:55:49Z

Noted by issue 16250

This adds in the option to Enable or Disable delete marker replication when working with v2 s3 replication config.

This change makes the delete_marker_replication block required when using v2 replication configuration. I could not find a better approach without requiring the object.

Note that the v1 replication config does not support the ability to toggle delete marker replication, it is always on by default.

Example usage:

replication_configuration {
  role = aws_iam_role.role.arn

  rules {
    id     = "foobar"
    status = "Enabled"

    filter {
      prefix = "foo"
    }

    delete_marker_replication_status = "Enabled"

    destination {
      bucket        = aws_s3_bucket.destination.arn
      storage_class = "STANDARD"
    }
  }
}

Updated (July 12, 2021): Config above has changed to reflect new argument implementation.

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #16250

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSS3Bucket_ReplicationSchemaV2'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSS3Bucket_ReplicationSchemaV2 -timeout 180m
=== RUN   TestAccAWSS3Bucket_ReplicationSchemaV2
=== PAUSE TestAccAWSS3Bucket_ReplicationSchemaV2
=== CONT  TestAccAWSS3Bucket_ReplicationSchemaV2
--- PASS: TestAccAWSS3Bucket_ReplicationSchemaV2 (272.12s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	273.949s

github-actions

Welcome @mkrant 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

mkrant · 2021-05-14T05:28:29Z

I could not find a better approach to keep this backwards compatible for V2 configurations due to the way the state is getting saved and diffed on subsequent terraform plan runs. Providing a default value when V2 config is used would cause a change in the output of a plan every time. If there is a better way to do this then please let me know or make the change!

This v1 versus v2 replication issue gets a little awkward given the different scenarios as described in the AWS docs. The following behaviors with this change are outlined below.

V1 Configuration

The delete_marker_replication block is not supported in V1 configuration. Delete marker replication is enabled by default and is not configurable.

If using V1 configuration, including a delete_marker_replication block will result in an error:

Error putting S3 replication configuration: InvalidRequest: DeleteMarkerReplication cannot be used for this version of Cross Region Replication configuration schema.

V2 Configuration

This change makes the delete_marker_replication block required when using V2 config. This would mean any existing terraform using v2 replication would need to add in the block. To maintain existing delete marker functionality, the status field should be set to Disabled or omitted (since the default value of status is Disabled).

If delete_marker_replication is not provided, it will result in an error:

Error putting S3 replication configuration: InvalidRequest: DeleteMarkerReplication must be specified for this version of Cross Region Replication configuration schema

If the tags field in the filter object is not empty, then the delete marker replication status must be set to Disabled or left blank. This is a limitation of AWS and can be seen in the docs link above.

obobrova · 2021-05-17T20:46:26Z

If the tags field in the filter object is not empty, then the delete marker replication status must be set to Disabled or left blank. This is a limitation of AWS and can be seen in the docs link above.

This is not correct as of November 2020. Replication status can be set to "Enabled" in V2. Please refer to the documentation: https://docs.aws.amazon.com/AmazonS3/latest/userguide/delete-marker-replication.html

Here's a link to the initial announcement: https://aws.amazon.com/about-aws/whats-new/2020/11/amazon-s3-replication-adds-support-for-replicating-delete-markers/

mkrant · 2021-05-18T06:07:47Z

Unfortunately it does looks like tag-based replication is not supported. From the article:
Delete marker replication is not supported for tag-based replication rules.

And double checking when running locally with tags and delete_marker_replication enabled, you get this error:

Error: Error putting S3 replication configuration: InvalidRequest: Delete marker replication is not supported if any Tag filter is specified

obobrova · 2021-05-18T18:57:25Z

Unfortunately it does looks like tag-based replication is not supported.

Oh, yes, you are absolutely right. I read your initial comment wrong: I was thinking of the filter field only as a way to distinguish between V1 and V2.

jonjohnston · 2021-05-27T18:25:42Z

I need this feature so I can deploy bucket changes. What is the hold up?

rodo-hf

+1

slava-inozemtsev · 2021-06-11T09:07:27Z

What needs to be done to make this PR merged?

ipnextgen · 2021-07-07T17:21:17Z

+1 , Please prioritise this one, Its kinda a security feature we want to implement.

Noted by issue 16250. This adds the option to Enable or Disable delete marker replication when working with v2 S3 replication config. Note that the v1 replication config does not support the ability to toggle delete marker replication; it is always on by default.

YakDriver · 2021-07-12T18:52:28Z

@mkrant I changed the implementation to avoid this being a breaking change. A breaking change would require waiting until v4.0.0. Now, instead, use delete_marker_replication_status = "Enabled" or omit the argument to not enable. That way, all the existing configurations out there will continue with the previous behavior.

Also, I've broken down the s3 bucket acceptance tests into 6 subcategories:

Subcategory	Example
Basic	TestAccAWSS3Bucket_Basic_acceleration
Manage	TestAccAWSS3Bucket_Manage_lifecycleBasic
Replication	TestAccAWSS3Bucket_Replication_schemaV2
Security	TestAccAWSS3Bucket_Security_logging
Tags	TestAccAWSS3Bucket_Tags_basic
Web	TestAccAWSS3Bucket_Web_simple

This allows easier testing of individual areas.

YakDriver

Looks good! 🎉

Output from acceptance tests (us-west-2):


--- PASS: TestAccAWSS3Bucket_Basic_acceleration (138.19s)
--- PASS: TestAccAWSS3Bucket_Basic_basic (35.65s)
--- PASS: TestAccAWSS3Bucket_Basic_emptyString (57.26s)
--- PASS: TestAccAWSS3Bucket_Basic_forceDestroy (67.05s)
--- PASS: TestAccAWSS3Bucket_Basic_forceDestroyWithEmptyPrefixes (58.40s)
--- PASS: TestAccAWSS3Bucket_Basic_forceDestroyWithObjectLockEnabled (70.77s)
--- PASS: TestAccAWSS3Bucket_Basic_generatedName (75.56s)
--- PASS: TestAccAWSS3Bucket_Basic_keyEnabled (84.51s)
--- PASS: TestAccAWSS3Bucket_Basic_namePrefix (63.89s)
--- PASS: TestAccAWSS3Bucket_Basic_requestPayer (122.71s)
--- PASS: TestAccAWSS3Bucket_Basic_shouldFailNotFound (46.28s)
--- PASS: TestAccAWSS3Bucket_Manage_lifecycleBasic (216.47s)
--- PASS: TestAccAWSS3Bucket_Manage_lifecycleExpireMarkerOnly (152.20s)
--- PASS: TestAccAWSS3Bucket_Manage_lifecycleRuleAbortIncompleteMultipartUploadDaysNoExpiration (27.88s)
--- PASS: TestAccAWSS3Bucket_Manage_lifecycleRuleExpirationEmptyConfigurationBlock (62.57s)
--- PASS: TestAccAWSS3Bucket_Manage_objectLock (138.68s)
--- PASS: TestAccAWSS3Bucket_Manage_versioning (182.56s)
--- PASS: TestAccAWSS3Bucket_Replication_basic (291.70s)
--- PASS: TestAccAWSS3Bucket_Replication_configurationRuleDestinationAccessControlTranslation (181.22s)
--- PASS: TestAccAWSS3Bucket_Replication_configurationRuleDestinationAddAccessControlTranslation (179.38s)
--- PASS: TestAccAWSS3Bucket_Replication_expectVersioningValidationError (23.68s)
--- PASS: TestAccAWSS3Bucket_Replication_multipleDestinationsEmptyFilter (106.88s)
--- PASS: TestAccAWSS3Bucket_Replication_multipleDestinationsNonEmptyFilter (106.46s)
--- PASS: TestAccAWSS3Bucket_Replication_schemaV2 (321.86s)
--- PASS: TestAccAWSS3Bucket_Replication_schemaV2SameRegion (103.40s)
--- PASS: TestAccAWSS3Bucket_Replication_twoDestination (105.07s)
--- PASS: TestAccAWSS3Bucket_Replication_withoutPrefix (104.26s)
--- PASS: TestAccAWSS3Bucket_Replication_withoutStorageClass (103.84s)
--- PASS: TestAccAWSS3Bucket_Security_ACLToGrant (106.82s)
--- PASS: TestAccAWSS3Bucket_Security_corsDelete (63.52s)
--- PASS: TestAccAWSS3Bucket_Security_corsEmptyOrigin (28.70s)
--- PASS: TestAccAWSS3Bucket_Security_corsUpdate (149.34s)
--- PASS: TestAccAWSS3Bucket_Security_disableDefaultEncryptionWhenDefaultEncryptionIsEnabled (141.29s)
--- PASS: TestAccAWSS3Bucket_Security_enableDefaultEncryptionWhenAES256IsUsed (77.43s)
--- PASS: TestAccAWSS3Bucket_Security_enableDefaultEncryptionWhenTypical (85.97s)
--- PASS: TestAccAWSS3Bucket_Security_GrantToACL (111.13s)
--- PASS: TestAccAWSS3Bucket_Security_logging (101.73s)
--- PASS: TestAccAWSS3Bucket_Security_policy (128.88s)
--- PASS: TestAccAWSS3Bucket_Security_updateACL (101.50s)
--- PASS: TestAccAWSS3Bucket_Security_updateGrant (201.50s)
--- PASS: TestAccAWSS3Bucket_Tags_basic (79.92s)
--- PASS: TestAccAWSS3Bucket_Tags_ignoreTags (103.56s)
--- PASS: TestAccAWSS3Bucket_Tags_withNoSystemTags (148.48s)
--- PASS: TestAccAWSS3Bucket_Tags_withSystemTags (192.13s)
--- PASS: TestAccAWSS3Bucket_Web_redirect (145.03s)
--- PASS: TestAccAWSS3Bucket_Web_routingRules (140.28s)
--- PASS: TestAccAWSS3Bucket_Web_simple (172.93s)

Output from acceptance tests (GovCloud):

Failure is unrelated to these changes (see #18511).

--- FAIL: TestAccAWSS3Bucket_Manage_lifecycleBasic (106.62s) // unrelated failure
--- PASS: TestAccAWSS3Bucket_Basic_basic (104.11s)
--- PASS: TestAccAWSS3Bucket_Basic_emptyString (56.33s)
--- PASS: TestAccAWSS3Bucket_Basic_forceDestroy (72.96s)
--- PASS: TestAccAWSS3Bucket_Basic_forceDestroyWithEmptyPrefixes (73.59s)
--- PASS: TestAccAWSS3Bucket_Basic_forceDestroyWithObjectLockEnabled (77.85s)
--- PASS: TestAccAWSS3Bucket_Basic_generatedName (102.78s)
--- PASS: TestAccAWSS3Bucket_Basic_keyEnabled (108.50s)
--- PASS: TestAccAWSS3Bucket_Basic_namePrefix (106.02s)
--- PASS: TestAccAWSS3Bucket_Basic_requestPayer (177.04s)
--- PASS: TestAccAWSS3Bucket_Basic_shouldFailNotFound (59.50s)
--- PASS: TestAccAWSS3Bucket_Manage_lifecycleExpireMarkerOnly (71.29s)
--- PASS: TestAccAWSS3Bucket_Manage_lifecycleRuleAbortIncompleteMultipartUploadDaysNoExpiration (62.48s)
--- PASS: TestAccAWSS3Bucket_Manage_lifecycleRuleExpirationEmptyConfigurationBlock (63.85s)
--- PASS: TestAccAWSS3Bucket_Manage_objectLock (135.36s)
--- PASS: TestAccAWSS3Bucket_Manage_versioning (240.26s)
--- PASS: TestAccAWSS3Bucket_Replication_basic (171.58s)
--- PASS: TestAccAWSS3Bucket_Replication_configurationRuleDestinationAccessControlTranslation (143.51s)
--- PASS: TestAccAWSS3Bucket_Replication_configurationRuleDestinationAddAccessControlTranslation (104.42s)
--- PASS: TestAccAWSS3Bucket_Replication_expectVersioningValidationError (40.57s)
--- PASS: TestAccAWSS3Bucket_Replication_schemaV2 (211.41s)
--- PASS: TestAccAWSS3Bucket_Replication_schemaV2SameRegion (96.91s)
--- PASS: TestAccAWSS3Bucket_Replication_twoDestination (100.33s)
--- PASS: TestAccAWSS3Bucket_Replication_withoutPrefix (88.99s)
--- PASS: TestAccAWSS3Bucket_Replication_withoutStorageClass (86.39s)
--- PASS: TestAccAWSS3Bucket_Security_ACLToGrant (131.14s)
--- PASS: TestAccAWSS3Bucket_Security_corsDelete (37.30s)
--- PASS: TestAccAWSS3Bucket_Security_corsEmptyOrigin (55.32s)
--- PASS: TestAccAWSS3Bucket_Security_corsUpdate (177.60s)
--- PASS: TestAccAWSS3Bucket_Security_disableDefaultEncryptionWhenDefaultEncryptionIsEnabled (174.39s)
--- PASS: TestAccAWSS3Bucket_Security_enableDefaultEncryptionWhenAES256IsUsed (107.63s)
--- PASS: TestAccAWSS3Bucket_Security_enableDefaultEncryptionWhenTypical (107.23s)
--- PASS: TestAccAWSS3Bucket_Security_GrantToACL (151.27s)
--- PASS: TestAccAWSS3Bucket_Security_logging (66.27s)
--- PASS: TestAccAWSS3Bucket_Security_policy (237.23s)
--- PASS: TestAccAWSS3Bucket_Security_updateACL (151.13s)
--- PASS: TestAccAWSS3Bucket_Security_updateGrant (239.74s)
--- PASS: TestAccAWSS3Bucket_Tags_basic (91.77s)
--- PASS: TestAccAWSS3Bucket_Tags_ignoreTags (47.68s)
--- PASS: TestAccAWSS3Bucket_Tags_withNoSystemTags (187.23s)
--- PASS: TestAccAWSS3Bucket_Tags_withSystemTags (297.32s)
--- PASS: TestAccAWSS3Bucket_Web_redirect (239.80s)
--- PASS: TestAccAWSS3Bucket_Web_routingRules (177.38s)
--- PASS: TestAccAWSS3Bucket_Web_simple (239.61s)
--- SKIP: TestAccAWSS3Bucket_Basic_acceleration (1.31s)
--- SKIP: TestAccAWSS3Bucket_Replication_multipleDestinationsEmptyFilter (43.23s)
--- SKIP: TestAccAWSS3Bucket_Replication_multipleDestinationsNonEmptyFilter (42.33s)

github-actions · 2021-07-15T23:44:55Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

github-actions · 2021-08-15T02:05:34Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

mkrant requested a review from a team as a code owner May 12, 2021 01:55

ghost added size/S Managed by automation to categorize the size of a PR. service/s3 Issues and PRs that pertain to the s3 service. labels May 12, 2021

github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label May 12, 2021

github-actions bot reviewed May 12, 2021

View reviewed changes

mkrant force-pushed the f-aws_s3_bucket_replication branch from 5a2c5b1 to 64b37e3 Compare May 12, 2021 02:03

ghost added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/S Managed by automation to categorize the size of a PR. labels May 14, 2021

ghost added the documentation Introduces or discusses updates to documentation. label May 14, 2021

mkrant force-pushed the f-aws_s3_bucket_replication branch 2 times, most recently from fc86652 to a991d47 Compare May 14, 2021 06:31

mkrant changed the title ~~[WIP] Add delete marker replication to v2 s3 config~~ Add delete marker replication to v2 s3 config May 14, 2021

mkrant mentioned this pull request May 14, 2021

S3 Bucket Replication add Delete Markers #16250

Closed

rodo-hf approved these changes May 28, 2021

View reviewed changes

YakDriver force-pushed the f-aws_s3_bucket_replication branch from a991d47 to 8f969a1 Compare July 9, 2021 20:24

YakDriver self-assigned this Jul 9, 2021

YakDriver removed the needs-triage Waiting for first response or review from a maintainer. label Jul 9, 2021

YakDriver added 3 commits July 12, 2021 14:41

r/s3_bucket: Add delete_marker_replication_status argument

5bb7088

tests/r/s3_bucket: Add delete_marker_replication_status argument

74b57c2

docs/r/s3_bucket: Add delete_marker_replication_status argument

fa38dd5

github-actions bot added 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 Jul 12, 2021

YakDriver added 3 commits July 12, 2021 14:44

r/s3_bucket: Update changelog

9cb7da5

r/s3_bucket: Remove errant newline

7520f9a

r/s3_bucket: Linty McLinterface

af79ce0

YakDriver added this to the v3.50.0 milestone Jul 12, 2021

YakDriver approved these changes Jul 12, 2021

View reviewed changes

YakDriver merged commit fcae320 into hashicorp:main Jul 12, 2021

github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add delete marker replication to v2 s3 config #19323

Add delete marker replication to v2 s3 config #19323

mkrant commented May 12, 2021 •

edited by YakDriver

Loading

github-actions bot left a comment

mkrant commented May 14, 2021 •

edited

Loading

obobrova commented May 17, 2021

mkrant commented May 18, 2021

obobrova commented May 18, 2021

jonjohnston commented May 27, 2021

rodo-hf left a comment

slava-inozemtsev commented Jun 11, 2021

ipnextgen commented Jul 7, 2021 •

edited

Loading

YakDriver commented Jul 12, 2021 •

edited

Loading

YakDriver left a comment •

edited

Loading

github-actions bot commented Jul 15, 2021

github-actions bot commented Aug 15, 2021

Add delete marker replication to v2 s3 config #19323

Add delete marker replication to v2 s3 config #19323

Conversation

mkrant commented May 12, 2021 • edited by YakDriver Loading

Community Note

github-actions bot left a comment

Choose a reason for hiding this comment

mkrant commented May 14, 2021 • edited Loading

V1 Configuration

V2 Configuration

obobrova commented May 17, 2021

mkrant commented May 18, 2021

obobrova commented May 18, 2021

jonjohnston commented May 27, 2021

rodo-hf left a comment

Choose a reason for hiding this comment

slava-inozemtsev commented Jun 11, 2021

ipnextgen commented Jul 7, 2021 • edited Loading

YakDriver commented Jul 12, 2021 • edited Loading

YakDriver left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 15, 2021

github-actions bot commented Aug 15, 2021

mkrant commented May 12, 2021 •

edited by YakDriver

Loading

mkrant commented May 14, 2021 •

edited

Loading

ipnextgen commented Jul 7, 2021 •

edited

Loading

YakDriver commented Jul 12, 2021 •

edited

Loading

YakDriver left a comment •

edited

Loading