fix(s3): cannot deploy multiple replication source buckets (under feature flag)

[badmintoncryer]

Issue # (if applicable)

Closes #33355.

Reason for this change

We cannot deploy multiple source buckets for object replication due to the explicitly set replication role name.

Description of changes

Set replication role name by PhysicalName.GENERATE_IF_NEEDED.

Describe any new or updated permissions being added

None

Description of how you validated changes

Update both unit and integ test.

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (dd0d62f) to head (a564674).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33360   +/-   ##
=======================================
  Coverage   82.21%   82.21%           
=======================================
  Files         119      119           
  Lines        6876     6876           
  Branches     1162     1162           
=======================================
  Hits         5653     5653           
  Misses       1120     1120           
  Partials      103      103           

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

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

Components	Coverage Δ
packages/aws-cdk	∅ <ø> (∅)
packages/aws-cdk-lib/core	82.21% <ø> (ø)

[jochemd]

How does this work in a scenario where multiple stages are deployed to 1 account? So we have 2 buckets from the TEST stage being replicated and 2 buckets from the PRD stage being replicated? Will they use the same role? And what if the replication is cross-account?

I was more thinking along the lines of adding an optional replicationRole: Role property to the ReplicationRule interface and use that role if it is passed in. That gives the control to the user on when to use the same and when to use different roles. The destination bucket already allows the user to specify the role (ARN) in the addReplicationPolicy method. Or possibly refactor renderReplicationConfiguration so that the part that creates the policy for the source bucket becomes a public method to allow people to set up permissions easily when they cutomize their stack (similar to how the addReplicationPolicy method is public for the destination bucket).

BTW, https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L2853 appears to be confusing the buckets: addReplicationPolicy() can be used to set permissions on the destination bucket.

[badmintoncryer]

@jochemd
I'm sorry for my late reply. Thank you for your feedback!

How does this work in a scenario where multiple stages are deployed to 1 account? So we have 2 buckets from the TEST stage being replicated and 2 buckets from the PRD stage being replicated? Will they use the same role? And what if the replication is cross-account?

One replication role is generated for each source bucket, regardless of stage differences or whether cross-account access is involved.
In the case of cross-account access, an explicit role name is specified, but there are no other differences.

I was more thinking along the lines of adding an optional replicationRole: Role property to the ReplicationRule interface and use that role if it is passed in.

Since one replication role executes replication according to multiple replication rules, it seems difficult to simply add a role property to the ReplicationRule interface. However, since I think the idea itself is excellent, how about posting it as a separate issue?
For this PR, I think it's better to focus on fixing the bug where multiple source buckets cannot be defined in the first place.

BTW, https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L2853 appears to be confusing the buckets: addReplicationPolicy() can be used to set permissions on the destination bucket.

Oh! I've not realized this. I'll resolve this problem in another PR.

[badmintoncryer]

@jochemd
I noticed that in the original issue you suggested making the role receivable as an argument.

From my perspective, since the current implementation is problematic, I think it would be best to first merge this PR to remove the hardcoded role name, and then create a new PR as a new feature to accept an optional replication role.

[jochemd]

Sounds good to me. Unfortunately I am not able to give an actual review or serious testing of this PR as when I try to go through the steps in the contributing guide I get errors on the linking step.

[GavinZZ]

Just a nit comment. The changes look good to me.

[GavinZZ]

Thank you as always for contributing!

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[badmintoncryer]

@GavinZZ Thank you always too!

[phuhung273]

Guys is it weird the PR stuck too long while being the 1st in queue ?

[badmintoncryer]

@mergify update

[mergify]

update

☑️ Nothing to do

• queue-position = -1 [📌 update requirement]
• #commits-behind > 0 [📌 update requirement]
• -closed [📌 update requirement]
• -conflict [📌 update requirement]

[badmintoncryer]

@mergify dequeue

[mergify]

dequeue

❌ Command disallowed due to command restrictions in the Mergify configuration.

• sender-permission >= write

[badmintoncryer]

@GavinZZ Could you please resolve this mergify problem??

[mergify]

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #33360 has been dequeued. Mergify failed to merge the pull request. GitHub can't merge the pull request after 15 retries.
Base branch was modified in the meantime

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

[badmintoncryer]

@mergify requeue

[mergify]

requeue

❌ Command disallowed due to command restrictions in the Mergify configuration.

• sender-permission >= write

[badmintoncryer]

@mergify update

[mergify]

update

❌ Base branch update has failed

expected head sha didn’t match current head ref.

[GavinZZ]

@badmintoncryer somehow I can't update latest main branch to this PR.. The PR seems to stuck at Checking for the ability to merge automatically.... I'll keep an eye out for this but we may need to create a new PR instead.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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