feat(s3): replicating objects #30966

badmintoncryer · 2024-07-28T14:00:19Z

Issue # (if applicable)

Closes #1680.

Reason for this change

AWS S3 supports configuring object replication , but the s3.Bucket construct does not support it.

Description of changes

Added replicationRules to BucketProps.

Replication configuration version

There are two versions of replication configuration. This PR uses only the V2 replication configuration to enable the specification of the Filter element and S3 Replication Time Control (S3 RTC).

To use V2 replication configuration, this PR explicitly specifies Filter.Prefix property.

        const prefix = rule.prefixFilter ?? '';
        const filter = isAndFilter ? {
          and: {
            prefix,
            tagFilters: rule.tagFilter,
          },
        } : {
          prefix,
        };

V2 replication configuration has some restriction:

Must specify DeleteMarkerReplication

ReplicationStack | 4/7 | 9:22:08 PM | CREATE_FAILED        | AWS::S3::Bucket  | SourceBucket (SourceBucketDDD2130A) Resource handler returned message:
Delete marker replication is not supported if any Tag filter is specified. Please refer to S3 Developer Guide for more information. (Service: S3, Status Code: 400, Request ID: XXX, Extended Request ID: XXX)

Must specify Priority

ReplicationStack | 4/7 | 9:12:08 PM | CREATE_FAILED        | AWS::S3::Bucket  | SourceBucket (SourceBucketDDD2130A) Resource handler returned message:
Priority must be specified for this version of Cross Region Replication configuration schema. Please refer to S3 Developer Guide for more information. (Service: S3, Status Code: 400, Request ID: XXX, Extended Request ID: XXX)

These restriction is not documented but there are some posts about these points.

https://repost.aws/questions/QUiEc8wFE_Q16fX5WG-YWnrA/cloudformation-support-for-s3-replication-to-multiple-destination-buckets

To resolve these problems,I made the priority required and explicitly set the deleteMarkerReplication.

       const prefix = rule.prefixFilter ?? ''; // set empty string to use V2 replication configuration
        const filter = isAndFilter ? {
          and: {
            prefix,
            tagFilters: rule.tagFilter,
          },
        } : {
          prefix,
        };

        return {
          id: rule.id,
          priority: rule.priority,
          status: 'Enabled',
          destination: {
            bucket: rule.destination.bucket.bucketArn,
            account: rule.destination.account,
            storageClass: rule.storageClass?.toString(),
            accessControlTranslation: rule.destination.accessControlTransition ? {
              owner: 'Destination',
            } : undefined,
            encryptionConfiguration: rule.kmsKey ? {
              replicaKmsKeyId: rule.kmsKey.keyArn,
            } : undefined,
            replicationTime: rule.replicationTimeControl !== undefined ? {
              status: rule.replicationTimeControl ? 'Enabled' : 'Disabled',
              time: {
                minutes: 15,
              },
            } : undefined,
            metrics: rule.replicationTimeControlMetrics !== undefined ? {
              status: rule.replicationTimeControlMetrics ? 'Enabled' : 'Disabled',
              eventThreshold: {
                minutes: 15,
              },
            } : undefined,
          },
          filter,
          // To avoid deploy error when there are multiple replication rules with undefined deleteMarkerReplication,
          // CDK explicitly set the deleteMarkerReplication if it is undefined.
          deleteMarkerReplication: {
            status: rule.deleteMarkerReplication ? 'Enabled' : 'Disabled',
          },
          sourceSelectionCriteria,
        };

IAM permission

There is a documentation to setup IAM permissions for service role.

{
   "Version":"2012-10-17",
   "Statement":[
      {
         "Effect":"Allow",
         "Action":[
            "s3:GetReplicationConfiguration",
            "s3:ListBucket"
         ],
         "Resource":[
            "arn:aws:s3:::SRC-BUCKET"
         ]
      },
      {
         "Effect":"Allow",
         "Action":[
            "s3:GetObjectVersionForReplication",
            "s3:GetObjectVersionAcl",
            "s3:GetObjectVersionTagging"
         ],
         "Resource":[
            "arn:aws:s3:::SRC-BUCKET/*"
         ]
      },
      {
         "Effect":"Allow",
         "Action":[
            "s3:ReplicateObject",
            "s3:ReplicateDelete",
            "s3:ReplicateTags"
         ],
         "Resource":"arn:aws:s3:::DST-BUCKET/*"
      }
   ]
}

However, there are discrepancies between the automatically generated IAM policies in the management console and the IAM policies in the documentation.

Generated Policy:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "s3:ListBucket",
                "s3:GetReplicationConfiguration",
                "s3:GetObjectVersionForReplication",
                "s3:GetObjectVersionAcl",
                "s3:GetObjectVersionTagging",
                "s3:GetObjectRetention",
                "s3:GetObjectLegalHold"
            ],
            "Effect": "Allow",
            "Resource": [
                "arn:aws:s3:::SRC-BUCKET",
                "arn:aws:s3:::SRC-BUCKET/*"
            ]
        },
        {
            "Action": [
                "s3:ReplicateObject",
                "s3:ReplicateDelete",
                "s3:ReplicateTags",
                "s3:GetObjectVersionTagging",
                "s3:ObjectOwnerOverrideToBucketOwner"
            ],
            "Effect": "Allow",
            "Condition": {
                "StringLikeIfExists": {
                    "s3:x-amz-server-side-encryption": [
                        "aws:kms",
                        "aws:kms:dsse",
                        "AES256"
                    ]
                }
            },
            "Resource": [
                "arn:aws:s3:::DST-BUCKET/*"
            ]
        },
        {
            "Action": [
                "kms:Decrypt"
            ],
            "Effect": "Allow",
            "Condition": {
                "StringLike": {
                    "kms:ViaService": "s3.ap-northeast-1.amazonaws.com",
                    "kms:EncryptionContext:aws:s3:arn": [
                        "arn:aws:s3:::SRC-BUCKET/*"
                    ]
                }
            },
            "Resource": [
                "arn:aws:kms:ap-northeast-1:123456789012:key/hogehuga"
            ]
        },
        {
            "Action": [
                "kms:Encrypt"
            ],
            "Effect": "Allow",
            "Condition": {
                "StringLike": {
                    "kms:ViaService": [
                        "s3.ap-northeast-1.amazonaws.com"
                    ],
                    "kms:EncryptionContext:aws:s3:arn": [
                        "arn:aws:s3:::DST-BUCKET*"
                    ]
                }
            },
            "Resource": [
                "arn:aws:kms:ap-northeast-1:123456789012:key/hogefuga"
            ]
        }
    ]
}

I adopted the policy from the document. I look forward to hearing your thoughts on this matter.

Description of how you validated changes

Added both unit and integ tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

shikha372 · 2024-08-15T22:37:55Z

Hi @badmintoncryer , Thank you for submitting this PR, as this feature is adding some additional pemissions for s3, we'll need to have an internal security review while we go through this PR. Will keep you posted !!

shikha372 · 2024-09-10T17:17:06Z

Hi @badmintoncryer, to update had the first security review for this PR, currently concern is around populating correct permissions for opt-in regions, will let you know if any changes are required to address it.

rix0rrr · 2024-09-24T10:04:47Z

packages/aws-cdk-lib/aws-s3/README.md

+  replicationRules: [
+    {
+      // The destination bucket for the replication rule.
+      destination: s3.ReplicationDestination.sameAccount(destinationBucket1),


Why a ReplicationDestination? Shouldn't the destination just be an IBucket ?

All resources will have a resourceEnv property you can use to get account and region from.

I have changed it to the IBucket type. Thank you for suggesting the use of ResourceEnv.

rix0rrr · 2024-09-24T10:07:09Z

packages/aws-cdk-lib/aws-s3/README.md

+    {
+      destination: s3.ReplicationDestination.sameAccount(destinationBucket2),
+      priority: 2,
+      // Whether to specify S3 Replication Time Control (S3 RTC).


Please spare a sentence to explain what enabling RTC means.

According to the docs it has something to do with metrics, but metrics is a separate field here, so now I don't understand what this does.

I've updated like this.

// Whether to specify S3 Replication Time Control (S3 RTC). // S3 RTC replicates most objects that you upload to Amazon S3 in seconds, // and 99.99 percent of those objects within specified time. replicationTimeControl: s3.ReplicationTimeValue.FIFTEEN_MINUTES, // Whether to enable replication metrics about S3 RTC. // If set, metrics will be output to indicate whether replication by S3 RTC took longer than the configured time. metrics: s3.ReplicationTimeValue.FIFTEEN_MINUTES,

packages/aws-cdk-lib/aws-s3/README.md

rix0rrr · 2024-09-24T10:10:09Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

+   * A container specifying replication metrics-related settings enabling replication metrics and events.
+   *
+   * @default - Replication metrics are not enabled
+   */
+  readonly metrics?: ReplicationTimeValue;


The 15 minute value is used to log events that say something like "SLA missed", right? Might want to explain that here.

I've added below description.

When a value is set, metrics will be output to indicate whether the replication took longer than the specified time.

I'm not very confident in writing English, so please feel free to improve my sentences, not just in this context.

rix0rrr · 2024-09-24T10:11:26Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

+   * Specifies whether Amazon S3 replicates delete markers.
+   *
+   * @default false


What's the implication of this being false ? That replicated objects in versioned buckets don't get deleted?

False means do not replicate delete marker to the destination bucket.
https://docs.aws.amazon.com/AmazonS3/latest/userguide/delete-marker-replication.html

I've updated JSDoc.

/** * Specifies whether Amazon S3 replicates delete markers. * * @see https://docs.aws.amazon.com/AmazonS3/latest/userguide/delete-marker-replication.html * * @default - delete markers in source bucket is not replicated to destination bucket */ readonly deleteMarkerReplication?: boolean;

rix0rrr · 2024-09-24T10:13:15Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

+   * @param accessControlTransition whether to want to change replica ownership to the AWS account that owns the destination bucket. The replicas are owned by same AWS account that owns the source object by default.
+   */
+  public static crossAccount(bucket: IBucket, account: string, accessControlTransition?: boolean): ReplicationDestination {
+    return new ReplicationDestination(bucket, account, accessControlTransition);


I think I'd also prefer the bucket policy be added automatically. It is the CDK thing to do.

We can add the annotation if addToBucketPolicy(): boolean returns false, indicating that we have a referenced bucket.

Co-authored-by: Rico Hermans <[email protected]>

Pull request has been modified.

badmintoncryer · 2024-09-28T22:44:37Z

@rix0rrr Thank you for your review! I've addressed all of your comments.

shikha372 · 2024-10-16T20:50:44Z

Hello @badmintoncryer ,
Apologies to keep you waiting, have completed the feature review for this PR. Current ask is if we can add integ test for bidirectional replication and for cross-account replication(if possible) as well before final approval.

badmintoncryer · 2024-10-16T21:37:40Z

@shikha372 Thank you for your information.

add integ test for bidirectional replication

What is the meaning of bidirectional replication? Are you suggesting that we prepare Bucket A and Bucket B, and designate each other as their respective replication destinations?

　cross-account replication(if possible)

I don't have two AWS account...　Would it be better to create a new account and run the tests?

shikha372 · 2024-10-24T16:25:53Z

What is the meaning of bidirectional replication? Are you suggesting that we prepare Bucket A and Bucket B, and designate each other as their respective replication destinations?

Yes, since this can also be supported using new feature, we'll need a integ test for this case.

I don't have two AWS account...　Would it be better to create a new account and run the tests?

Got it, in that case I will help adding integ test for this one

badmintoncryer · 2024-10-27T11:36:50Z

I considered bidirectional replication. First, I added an addReplicationRules() method to allow the destination bucket to be set lazily.

  public addReplicationRules(rules: ReplicationRule[]): void {
    this.replicationRules.push(...rules);
  }

However, it seems that the circular reference between the source and destination buckets cannot be resolved.

Do you have any suggestions on the implementation?

shikha372 · 2024-11-18T19:51:04Z

I considered bidirectional replication. First, I added an addReplicationRules() method to allow the destination bucket to be set lazily.
  public addReplicationRules(rules: ReplicationRule[]): void {
    this.replicationRules.push(...rules);
  }
However, it seems that the circular reference between the source and destination buckets cannot be resolved.

Do you have any suggestions on the implementation?

Thanks @badmintoncryer , working on adding cross-account integ test will give it a try at my end for bi-directional.

badmintoncryer · 2024-11-18T23:11:37Z

Thank you very much @shikha372

codecov · 2024-11-27T14:14:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.46%. Comparing base (be000a2) to head (5a25de6).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #30966   +/-   ##
=======================================
  Coverage   77.46%   77.46%           
=======================================
  Files         105      105           
  Lines        7168     7168           
  Branches     1314     1314           
=======================================
  Hits         5553     5553           
  Misses       1433     1433           
  Partials      182      182

Flag	Coverage Δ
suite.unit	`77.46% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`77.46% <ø> (ø)`

aws-cdk-automation · 2024-11-27T14:34:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5a25de6
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

badmintoncryer added 6 commits July 25, 2024 23:23

feat: replicationConfiguration

b64f9c6

temp

1e9ca53

add role

a7f8ba0

integ test

6d53e35

unit test

593f6c6

add unit test

8149782

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Jul 28, 2024

aws-cdk-automation requested a review from a team July 28, 2024 14:00

github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Jul 28, 2024

aws-cdk-automation previously requested changes Jul 28, 2024

View reviewed changes

badmintoncryer added 3 commits August 2, 2024 00:19

add snapshot

7f1015e

docs

f81fc01

updatet readme

b1d8b44

badmintoncryer added 5 commits August 8, 2024 21:18

fix readme

bba1cd6

update docs

d5d1450

fix readme

198150b

Merge branch 'main' into 1680-s3Replication

e2a1e22

fix

83cff2c

badmintoncryer marked this pull request as ready for review August 8, 2024 14:26

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 8, 2024

Merge branch 'main' into 1680-s3Replication

609d945

shikha372 self-assigned this Aug 13, 2024

Merge branch 'main' into 1680-s3Replication

c4559e0

shikha372 added SECURITY needs-security-review Related to feature or issues that needs security review and removed SECURITY labels Aug 15, 2024

Merge branch 'main' into 1680-s3Replication

7bd51fa

rix0rrr previously requested changes Sep 24, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 24, 2024

badmintoncryer and others added 3 commits September 27, 2024 21:46

make destination as IBucket

7e22542

refactor integ

cfe7ccc

Update packages/aws-cdk-lib/aws-s3/README.md

566f644

Co-authored-by: Rico Hermans <[email protected]>

badmintoncryer added 8 commits September 28, 2024 12:33

add bucket policy

f69eb1c

configure bucket policy for a cross-account replication

6a0eb21

update unit test

1daff03

Merge remote-tracking branch 'origin/main' into 1680-s3Replication

e58b2ab

remove featureflag

2ed77f8

update readme

1aa47da

fix readme

68e8e98

fix readme

a1f3054

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 28, 2024

badmintoncryer added 2 commits October 26, 2024 21:48

Merge remote-tracking branch 'origin/main' into 1680-s3Replication

a56361f

fix file format

b2fe790

paulhcsun and others added 2 commits November 22, 2024 13:58

Merge branch 'main' into 1680-s3Replication

04d08a2

Merge branch 'main' into 1680-s3Replication

5a25de6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s3): replicating objects #30966

feat(s3): replicating objects #30966

badmintoncryer commented Jul 28, 2024 •

edited

Loading

aws-cdk-automation left a comment

shikha372 commented Aug 15, 2024 •

edited

Loading

shikha372 commented Sep 10, 2024

rix0rrr Sep 24, 2024

badmintoncryer Sep 27, 2024

rix0rrr Sep 24, 2024

badmintoncryer Sep 28, 2024

rix0rrr Sep 24, 2024

badmintoncryer Sep 28, 2024

rix0rrr Sep 24, 2024

badmintoncryer Sep 28, 2024

rix0rrr Sep 24, 2024

badmintoncryer commented Sep 28, 2024

shikha372 commented Oct 16, 2024 •

edited

Loading

badmintoncryer commented Oct 16, 2024 •

edited

Loading

shikha372 commented Oct 24, 2024 •

edited

Loading

badmintoncryer commented Oct 27, 2024 •

edited

Loading

shikha372 commented Nov 18, 2024

badmintoncryer commented Nov 18, 2024

codecov bot commented Nov 27, 2024 •

edited

Loading

aws-cdk-automation commented Nov 27, 2024

feat(s3): replicating objects #30966

Are you sure you want to change the base?

feat(s3): replicating objects #30966

Conversation

badmintoncryer commented Jul 28, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Replication configuration version

IAM permission

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

shikha372 commented Aug 15, 2024 • edited Loading

shikha372 commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badmintoncryer commented Sep 28, 2024

shikha372 commented Oct 16, 2024 • edited Loading

badmintoncryer commented Oct 16, 2024 • edited Loading

shikha372 commented Oct 24, 2024 • edited Loading

badmintoncryer commented Oct 27, 2024 • edited Loading

shikha372 commented Nov 18, 2024

badmintoncryer commented Nov 18, 2024

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

aws-cdk-automation commented Nov 27, 2024

AWS CodeBuild CI Report

badmintoncryer commented Jul 28, 2024 •

edited

Loading

shikha372 commented Aug 15, 2024 •

edited

Loading

shikha372 commented Oct 16, 2024 •

edited

Loading

badmintoncryer commented Oct 16, 2024 •

edited

Loading

shikha372 commented Oct 24, 2024 •

edited

Loading

badmintoncryer commented Oct 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading