feat(efs): replicating file systems by badmintoncryer · Pull Request #29347 · aws/aws-cdk

badmintoncryer · 2024-03-03T17:14:21Z

Issue # (if applicable)

Reason for this change

EFS supports replicating file systems but AWS CDK cannot configure it.

Description of changes

Add replicationConfiguration to FileSystemProps

declare const vpc: ec2.Vpc;
declare const kmsKey: kms.Key;

// auto generate a replication destination file system
new efs.FileSystem(this, 'ReplicationSourceFileSystem1', {
  vpc,
  replicationConfiguration: {
    kmsKey, // optional
    region: 'us-east-1', // optional
    availabilityZone: 'us-east-1a', // optional, Specifing the AZ means creating a One Zone file system as the replication destination
  }
});

// specify the replication destination file system
const destinationFileSystem = new efs.FileSystem(this, 'DestinationFileSystem', {
  vpc,
  // set as the read-only file system for use as a replication destination
  replicationOverwriteProtection: efs.ReplicationOverwriteProtection.DISABLED,
});

new efs.FileSystem(this, 'ReplicationSourceFileSystem2', {
  vpc,
  replicationConfiguration: {
    destinationFileSystem,
    // cannot configure other properties when destinationFileSystem is specified
  }
});

Description of how you validated changes

I have 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

lpizzinidev

Thanks 👍 I left some initial comments for some refactoring.
Please let me know if you have suggestions for further improvements.

lpizzinidev · 2024-03-05T07:11:22Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+   *
+   * @default - no replication
+   */
+  readonly replicationConfiguration?: ReplicationConfiguration;


Why aren't we allowing multiple destinations? (CFN declaration)

CloudFormation supports an array of destinations, but the maximum number of elements is restricted to 1. Therefore, I did not implement support for multiple destinations.

Should I add support for it to accommodate future expansion?

Tricky question.
I guess we can keep it like this for now and deprecate it in the future in favor of an array of configurations if necessary.

I've updated to receive replicationConfiguration as array.

I apologize for misunderstanding your opinion.. Which implementation will you adopt?

So what I would do here is actually make this an enum like class to future proof us against if/when the service team does allow for multiple regions. My suggestion would be to make ReplicationConfiguration a class with static functions like the Schedule class. For now, we'll only have one function that takes in one replicationConfiguration, but in the future we can add more functions for multiple.

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

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.

badmintoncryer · 2024-04-14T14:20:24Z

@gracelu0 I've updated my implementation and have a question. Could you please confirm it again?

TheRealAmazonKendra

Well shit. I have a bunch of review comments in pending. They're from a hot minute ago so some may be outdated.

TheRealAmazonKendra · 2024-03-21T17:21:49Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+   *
+   * @default - create regional file system for the replication destination
+   */
+  readonly availabilityZone?: string;


If the availability zone is only a valid field if the region is provided, these two should fields should be a single object.

Further down I'm also seeing a lot of checking for whether certain properties are compatible with one another. This tells me that the contract here is too loose. I get that you are working from a construct that uses a lot of optional props when they really shouldn't be but I'd like further changes to not follow that pattern.

Can we structure this in a way that enforces the dependent props in the contract and disallows the combinations you're checking for below?

TheRealAmazonKendra · 2024-03-21T17:47:56Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+   *
+   * @default - no replication
+   */
+  readonly replicationConfiguration?: ReplicationConfiguration;


So what I would do here is actually make this an enum like class to future proof us against if/when the service team does allow for multiple regions. My suggestion would be to make ReplicationConfiguration a class with static functions like the Schedule class. For now, we'll only have one function that takes in one replicationConfiguration, but in the future we can add more functions for multiple.

gracelu0

Final tiny docstring comments and then I'm good to approve :)

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

gracelu0 · 2024-04-24T01:26:27Z

packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

+   *
+   * @default - create a new file system for the replication


Suggested change

*

* @default - create a new file system for the replication

@gracelu0
I believe the @ default description is essential because the destinationFileSystem is optional. Removing this description could cause the CI to fail.

What do you think?

Ohh right, thanks for pointing that out! perhaps “None” or “No destinationFileSystem set” instead then?

Thanks! I've updated it.

Co-authored-by: Grace Luo <54298030+gracelu0@users.noreply.github.com>

gracelu0

LGTM, thank you for contributing!

mergify · 2024-04-24T17:19:36Z

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

aws-cdk-automation · 2024-04-24T17:48:03Z

AWS CodeBuild CI Report

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

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

mergify · 2024-04-24T17:48:24Z

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

github-actions bot added feature-request A feature should be added or improved. p2 labels Mar 3, 2024

aws-cdk-automation requested a review from a team March 3, 2024 17:14

github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Mar 3, 2024

badmintoncryer changed the title ~~feat(efs): replicationg file systems~~ feat(efs): replicating file systems Mar 3, 2024

badmintoncryer force-pushed the 21455-efsReplication branch from dd939f8 to a16bbbe Compare March 4, 2024 15:43

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 4, 2024

lpizzinidev suggested changes Mar 5, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Mar 5, 2024

badmintoncryer and others added 15 commits March 6, 2024 04:03

feat: configure replication

c426b12

fix: replication config correctly

6dcf485

fix: debug

b1361d8

test: add integ test

f31fc43

test: add unit test

2737c3f

fix: property name

9956a3b

chore: add comments

9a101c7

feat: add region validation

75dc9da

chore: format

c575735

fix: add comments

8537be0

feat: specify region when filesystem is passed

9cb4146

fix: remove unused import

2b664b2

fix: readme

2525202

Update efs-file-system.ts

e73da37

Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>

feat: remove enable property

41dcee4

badmintoncryer force-pushed the 21455-efsReplication branch from b545233 to 41dcee4 Compare March 5, 2024 19:03

badmintoncryer added 2 commits March 6, 2024 04:17

feat: replicationConfiguration is to be array

f77fb2c

test: remove integ test files

6be4342

aws-cdk-automation previously requested changes Mar 5, 2024

View reviewed changes

test: add integ test files

0fd10b0

badmintoncryer added 2 commits April 2, 2024 22:29

Merge branch 'main' into 21455-efsReplication

46f81da

Merge branch 'main' into 21455-efsReplication

6aa6666

TheRealAmazonKendra reviewed Apr 18, 2024

View reviewed changes

badmintoncryer added 4 commits April 22, 2024 18:21

chore: refactor

edf0c08

Merge branch 'main' into 21455-efsReplication

59e8cd4

fix: add export

7c04a23

fix: optional

c26a21f

gracelu0 reviewed Apr 24, 2024

View reviewed changes

badmintoncryer and others added 2 commits April 24, 2024 10:32

Update packages/aws-cdk-lib/aws-efs/lib/efs-file-system.ts

b806c5e

Co-authored-by: Grace Luo <54298030+gracelu0@users.noreply.github.com>

chore: update comments

7e3380f

gracelu0 approved these changes Apr 24, 2024

View reviewed changes

Merge branch 'main' into 21455-efsReplication

ecfd8c0

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

mergify bot merged commit a15dc93 into aws:main Apr 24, 2024

badmintoncryer deleted the 21455-efsReplication branch April 24, 2024 18:09

This was referenced May 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#98

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#99

Open

NOUIY mentioned this pull request May 30, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#101

Open

NOUIY mentioned this pull request Jun 7, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#102

Open

niravparikh05 mentioned this pull request Jun 13, 2024

[Snyk] Upgrade aws-cdk from 2.65.0 to 2.142.1 paralus/eks-blueprints-addon#3

Open

NOUIY mentioned this pull request Jun 14, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#103

Open

snyk-io bot mentioned this pull request Jun 20, 2024

[Snyk] Upgrade aws-cdk-lib from 2.138.0 to 2.143.1 Opetushallitus/heratepalvelu#280

Closed

NOUIY mentioned this pull request Jun 20, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#104

Open

snyk-io bot mentioned this pull request Jun 21, 2024

[Snyk] Upgrade aws-cdk-lib from 2.139.0 to 2.143.1 Opetushallitus/ovara#36

Open

This was referenced Jun 22, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#105

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#106

Open

Conversation

badmintoncryer commented Mar 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

lpizzinidev left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badmintoncryer Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

badmintoncryer commented Apr 14, 2024

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gracelu0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

badmintoncryer Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gracelu0 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Apr 24, 2024

Uh oh!

aws-cdk-automation commented Apr 24, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

badmintoncryer commented Mar 3, 2024 •

edited

Loading

lpizzinidev left a comment •

edited

Loading

badmintoncryer Mar 5, 2024 •

edited

Loading

badmintoncryer Apr 24, 2024 •

edited

Loading