feat(AWS Backup): adding S3 bucket support to AWS backup and exposing backup selection role

[PeterBaker0]

---

All Submissions:

• [x] Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• [ ] This PR adds new unconventional dependencies following the process described here

New Features

• [x] Have you added the new feature to an integration test?
  • [x] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

This is my first contribution to the wonderful CDK - so apologies if I've missed something.

This PR is a non breaking change which adds support for S3 buckets to the AWS Backup L2 constructs.

This was realised by

• a fromS3Bucket static method on the BackupResource construct
• a check for the bucket type in the Backupable Resources Collector Aspect

When using this construct for a project, I found two other issues

1. The BackupSelection construct does not add the S3 backup/restore managed policies and has no means to do so
2. The BackupSelection construct does not expose the IAM role (at least directly - it was previously set as the grantPrincipal)

So this PR also:

1. Adds allowS3Backup and allowS3Restores flags which add the AWS Backup for S3 managed policies for backup/restore respectively
2. Exposes the role in the BackupSelection construct

Additionally, I found that in using this construct, it was not possible to provide a fully customised role to the construct, it would always add, at least, the managed policy for backup. So I added a customRole flag which can be provided to disable the addition of any managed policies, meaning that a user could provide a completely custom role, if desired. Annotation warnings for the construct were added in the case where both the customRole flag and any of the allowX flags for managed policies were both true.

An integration test was added which makes sure that selections both using the fromS3Bucket and fromConstruct resource static methods function. The allowS3Backup and allowS3Restore flags were true to ensure that the managed policy names were correct.

Added unit tests for the S3 support (static resource methods, managed policies), the IAM role property, and the customRole flag behaviour.

[gitpod-io[bot]]

[PeterBaker0]

The most pressing one I will write here, please add unit tests, especially since there are misconfiguration cases.

I did add a few unit tests in the selections test - this covered the fromS3Bucket, fromConstruct, as well as the changes to the roles. Wasn't sure how to handle the unit test case for misconfiguration - how do you enforce that there should be annotation warnings?

[PeterBaker0]

@Naumel thanks for your review.

I think the customRole interface was confusing so changed it to include a allowBackup flag which defaults to true. This means that there is no breaking change, however users can easily specify allowBackup as false if they need to use a custom role which doesn't include this managed policy.

As mentioned above - I did add a few unit tests and update relevant existing ones - could you clarify which features are missing from the unit test coverage if you would like more? Thanks.

I also added another new integration test to verify that the various combinations of policies are all valid and deploy properly.

Readme was updated and streamlined. Added a table to map the allowX fields to the managed policies, and supplied a link to relevant section in AWS docs.

Cheers.

[Naumel]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[PeterBaker0]

I like the renaming, makes reading easier for sure.

If you decide to add even more functionality, I look forward to the submission(s)!

Thanks!

There was a stale reference in the parameter comments so have corrected that now - apologies if that breaks your PR approval!

[PeterBaker0]

@Naumel I have read the contribution guide but couldn't find any advice on the next steps. Do I need to take any more actions to get this merged or will happen automatically? Cheers.

[mergify[bot]]

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).

[TheRealAmazonKendra]

@Mergifyio unqueue

[mergify[bot]]

unqueue

☑️ The pull request is not queued

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

One quick question from me: is there an issue for this feature? If so, please link it so it will get closed when this is merged. If not, no worries.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[kaizencc]

@PeterBaker0 are you still working on this PR? I see some 👍 on my previous comments but no action besides that. If you need more time, that's fine! If you need to drop and let someone else pick this up, that's fine too. Just let me know.

[PeterBaker0]

@PeterBaker0 are you still working on this PR? I see some +1 on my previous comments but no action besides that. If you need more time, that's fine! If you need to drop and let someone else pick this up, that's fine too. Just let me know.

Hey Kaizen - I'd like to do this but I don't see myself having time in the next few weeks.

Not sure of the process to defer this - I'm happy to close this PR then maybe we can make an issue to track the progress so far and what was missing from this PR, so someone could come and pick it up (or me if I get time)?

[corymhall]

Not sure of the process to defer this - I'm happy to close this PR then maybe we can make an issue to track the progress so far and what was missing from this PR, so someone could come and pick it up (or me if I get time)?

Thanks for letting us know @PeterBaker0! I'll go ahead and close this issue for now. If you ever want to come back to it you can just re-open this PR.

[johnmarcou]

@corymhall There is any in-progress PR on this subject?

Thank you!

[PeterBaker0]

@johnmarcou This was closed as the PR required some more in depth changes which I didn't have time to continue.

I am unsure if someone has picked up or is working on this.

You're welcome to continue from where I got to if you wish.