feat(efs): create EFS file systems #6373

yashda · 2020-02-19T20:22:21Z

100% unit test coverage.
This was tested by creating an EFS using this construct in a cdk application.
A instance was also created in this app, which successfully mounted it.

import {
    AmazonLinuxGeneration,
    AmazonLinuxImage,
    Instance,
    InstanceClass,
    InstanceSize,
    InstanceType,
    SubnetType,
    Vpc
} from '@aws-cdk/aws-ec2';
import * as efs from '@aws-cdk/aws-efs';
import * as cdk from '@aws-cdk/core';

export class TestEfscdkStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
        super(scope, id, props);

        // The code that defines your stack goes here

        const vpc = Vpc.fromLookup(this, 'VPC', {
          isDefault: true
        });

        const fileSystem = new efs.EfsFileSystem(this, 'EfsFileSystem', {
            vpc,
            encrypted: true,
            fileSystemTags: [new cdk.Tag("key1", "value1")],
            lifecyclePolicy: efs.EfsLifecyclePolicyProperty.AFTER_14_DAYS,
            performanceMode: efs.EfsPerformanceMode.GENERAL_PURPOSE,
            throughputMode: efs.EfsThroughputMode.BURSTING
        });

        const inst = new Instance(this, 'inst', {
            instanceType: InstanceType.of(InstanceClass.T2, InstanceSize.LARGE),
            machineImage: new AmazonLinuxImage({
                generation: AmazonLinuxGeneration.AMAZON_LINUX_2
            }),
            vpc,
            vpcSubnets: {
                subnetType: SubnetType.PUBLIC,
            },
        });

        fileSystem.connections.allowDefaultPortFrom(inst);

        inst.userData.addCommands("yum check-update -y",
            "yum upgrade -y",
            "apt-get -y update",
            "apt-get -y upgrade",
            "yum install -y amazon-efs-utils",
            "apt-get -y install amazon-efs-utils",
            "yum install -y nfs-utils",
            "apt-get -y install nfs-common",
            "file_system_id_1=" + fileSystem.fileSystemID,
            "efs_mount_point_1=/mnt/efs/fs1",
            "mkdir -p \"${efs_mount_point_1}\"",
            "test -f \"/sbin/mount.efs\" && echo \"${file_system_id_1}:/ ${efs_mount_point_1} efs defaults,_netdev\" >> /etc/fstab || " +
            "echo \"${file_system_id_1}.efs." + cdk.Stack.of(this).region + ".amazonaws.com:/ ${efs_mount_point_1} nfs4 nfsvers=4.1,rsize=1048576,wsize=1048576,hard,timeo=600,retrans=2,noresvport,_netdev 0 0\" >> /etc/fstab",
            "mount -a -t efs,nfs4 defaults");
  }
}

closes #6286

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

* 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes aws#6286

yashda · 2020-02-19T20:44:54Z

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

+/**
+ * Interface to implement AWS File Systems.
+ */
+export interface IFileSystem extends IResource, ec2.IConnectable {


The intention of adding this interface is to have a common interface for all AWS file systems (FSx windows, lustre). This can be moved to aws-core once we have construct for another type of file system.

For now, don't do this yet. Just make this an IEfsFileSystem interface. We can always add the superinterface later if necessary, and they should probably have different names in any case.

aws-cdk-automation · 2020-02-19T21:16:05Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9b208a6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

pahud · 2020-02-20T02:24:26Z

@yashda Good job!

One question - Any tools to help you with 100% unit test coverage?

I have been working on my recent PR for cloud9 L2 but can't have enough coverage. Any advice would be highly appreciated.

yashda · 2020-02-20T16:58:53Z

@yashda Good job!

One question - Any tools to help you with 100% unit test coverage?

I have been working on my recent PR for cloud9 L2 but can't have enough coverage. Any advice would be highly appreciated.

@pahud istanbul is already integrated with CDK, so whenever you run unit tests, it generates the coverage report at path aws-cdk\packages\@aws-cdk\aws-<module>\coverage. All the statements, branches and lines which are not covered in the UTs can be seen highlighted in index.html. Based on this report, more tests can be added to the code to increase coverage.
Does that answers your question?

rix0rrr · 2020-02-24T10:54:37Z

packages/@aws-cdk/aws-efs/README.md

+
+const myVpc = new ec2.Vpc(this, 'VPC');
+const fileSystem = new efs.EfsFileSystem(this, 'MyEfsFileSystem', {
+    vpc: myVpc,


Something very trivial, but can you make sure to change indentation to 2 spaces everywhere?

Example still shows up as 4 spaces for me.

packages/@aws-cdk/aws-efs/README.md

rix0rrr · 2020-02-24T10:56:55Z

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

+     *
+     * @default - no tags will be added
+     */
+    readonly fileSystemTags?: Tag[];


Tagging is done centrally (https://docs.aws.amazon.com/cdk/latest/guide/tagging.html) so doesn't need to be added here.

rix0rrr · 2020-02-24T10:58:24Z

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

+/**
+ * Interface to implement AWS File Systems.
+ */
+export interface IFileSystem extends IResource, ec2.IConnectable {


For now, don't do this yet. Just make this an IEfsFileSystem interface. We can always add the superinterface later if necessary, and they should probably have different names in any case.

rix0rrr · 2020-02-24T10:58:29Z

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

+     *
+     * @attribute
+     */
+    readonly fileSystemID: string;


Id. This will be come file_system_i_d in Python.

rix0rrr · 2020-02-24T11:00:05Z

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

+    /**
+     * Import an existing File System from the given properties.
+     */
+    public static fromEfsFileSystemAttributes(scope: Construct, id: string, attrs: EfsFileSystemAttributes): IFileSystem {


Can a filesystem imported in this way still be mounted? What would that look like?

The mount script in the user data just needs the file systems Id, which we are setting in the object here. We additionally need to update the connections to allow connection for a different resource.
Since we are setting both these properties here while importing an existing file system, it will be mounted using the same same way.

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

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

rix0rrr · 2020-02-24T11:10:11Z

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

+/**
+ * EFS Lifecycle Policy, if a file is not accessed for given days, it will move to EFS Infrequent Access.
+ *
+ * @see http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-efs-filesystem.html#cfn-efs-filesystem-performancemode


Wrong link.

rix0rrr · 2020-02-24T11:10:24Z

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

+ */
+export enum EfsLifecyclePolicyProperty {
+    /**
+     * After 7 days of inaccessibility.


Not "inaccessibility", but "not being accessed"

Pull request has been modified.

aws-cdk-automation · 2020-02-27T20:02:23Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 188fec0
Result: FAILED
Build Logs (available for 30 days)

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

* 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes aws#6286

aws-cdk-automation · 2020-02-27T21:41:24Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 6d080bc
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-02-27T22:28:32Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b78a698
Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2020-03-02T13:19:47Z

(Regular-merging master back to get rid of unrelated changes)

rix0rrr · 2020-03-02T13:20:41Z

packages/@aws-cdk/aws-efs/README.md

+
+const myVpc = new ec2.Vpc(this, 'VPC');
+const fileSystem = new efs.EfsFileSystem(this, 'MyEfsFileSystem', {
+    vpc: myVpc,


Example still shows up as 4 spaces for me.

rix0rrr · 2020-03-02T13:24:20Z

packages/@aws-cdk/aws-efs/README.md

+    },
+});
+
+inst.userData.addCommands("yum check-update -y",


I don't like putting in commands that will fail on one of the platforms and then silently ignoring the errors. Be prepared for UserData scripts to be executed under bash -e.

In the example you are creating an Amazon Linux instance, so no real need to add the Ubuntu commands though. If you want to show the commands for multiple platforms (admirable) then show the difference in comments:

inst.userData.addCommands( 'yum check-update -y', // Ubuntu: apt-get -y update ...);

Also this example is incomplete, it doesn't show creation of the EFS filesystem and how to allow connections.

rix0rrr · 2020-03-02T13:25:14Z

packages/@aws-cdk/aws-efs/README.md

+    "apt-get -y install amazon-efs-utils",
+    "yum install -y nfs-utils",
+    "apt-get -y install nfs-common",
+    "file_system_id_1=" + fileSystem.fileSystemID,


Changed to fileSystemId if I'm not mistaken.

rix0rrr · 2020-03-02T13:27:59Z

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

+/**
+ * Interface to implement AWS File Systems.
+ */
+export interface IEfsFileSystem extends IResource, ec2.IConnectable {


Wait, IResource doesn't need to be in here. Sorry missed that on the first go-around.

Just for my understanding, why don't we need this interface to be implemented here? I believe that the file system act as a cloud formation resource.

Nevermind, I figured that we are already extending Resource which implements IResource.

rix0rrr · 2020-03-02T13:29:36Z

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

+      }
+    }
+
+    this.efsFileSystem = new CfnFileSystem(this, "FileSystem", {


Construct id must be "Resource".

rix0rrr · 2020-03-02T13:32:47Z

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

+          subnetId
+        });
+      this.mountTargets.push(efsMountTarget);
+      this.mountTargetIdentifiers.push(efsMountTarget.ref);


I see we're now holding on to even more attributes of the mount targets, but why would you need these?

The UserData script you gave uses only the ID and nothing else. Why do we need the IDs and IP addresses?

The file system can be mounted using the IP addresses as well. The UserData in the example creates the domain name from file system and uses it for mounting. Some users may need the subnet based on IP address to mount it, hence exposing the IP address of all the mount targets.

Exposing Ids is not required, but I took the reference from aws-rds that we do expose both the identifiers and ip address of all the instances, hence exposing to the user.

Should I remove these for now?

This is probably fine, although in practice people are going to want to use the mount targets in the same AZs as their instances (to cut on cross-AZ traffic fees).

Can the construct accomodate that in its current state?

Oh I see, using the domain name you don't have to: https://docs.aws.amazon.com/efs/latest/ug/mounting-fs-mount-cmd-general.html

Given that I feel the IP addresses are useless without subnet information to identify them, I vote we don't expose any of the mount target information right now and wait for people to come to us with use cases where they need IP addresses. Then they can tell us how they expect to find the IP address corresponding to their AZ.

Removing the mount targets for now.
I can think of a way for exposing the IP address along with the subnet information. Will create another PR for the same.

aws-cdk-automation · 2020-03-02T14:00:21Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: a36a011
Result: SUCCEEDED
Build Logs (available for 30 days)

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

Pull request has been modified.

aws-cdk-automation · 2020-03-03T04:26:02Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 2caba56
Result: SUCCEEDED
Build Logs (available for 30 days)

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

rix0rrr · 2020-03-03T10:21:12Z

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

+          subnetId
+        });
+      this.mountTargets.push(efsMountTarget);
+      this.mountTargetIdentifiers.push(efsMountTarget.ref);


This is probably fine, although in practice people are going to want to use the mount targets in the same AZs as their instances (to cut on cross-AZ traffic fees).

Can the construct accomodate that in its current state?

rix0rrr · 2020-03-03T10:23:28Z

packages/@aws-cdk/aws-efs/README.md

+
+fileSystem.connections.allowDefaultPortFrom(inst);
+
+inst.userData.addCommands("yum check-update -y",    // Ubuntu: apt-get -y update


Do we need to worry about this at all? I don't see IAM permissions mentioned anywhere in your example:

https://docs.aws.amazon.com/efs/latest/ug/iam-access-control-nfs-efs.html

The default EFS file system policy grants full access to any NFS client that can connect to the file system.

In this example, EFS was not created using any profile permissions, the instance is mounting the file system and hence does require any specific IAM role assigned to it.

Also, the IAM feature was recently introduced to EFS. Since I started working on the PR before this feature's launch, I haven't added it here and will add this feature in subsequent PR.

rix0rrr · 2020-03-03T10:25:52Z

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

+          subnetId
+        });
+      this.mountTargets.push(efsMountTarget);
+      this.mountTargetIdentifiers.push(efsMountTarget.ref);


Oh I see, using the domain name you don't have to: https://docs.aws.amazon.com/efs/latest/ug/mounting-fs-mount-cmd-general.html

Given that I feel the IP addresses are useless without subnet information to identify them, I vote we don't expose any of the mount target information right now and wait for people to come to us with use cases where they need IP addresses. Then they can tell us how they expect to find the IP address corresponding to their AZ.

Pull request has been modified.

aws-cdk-automation · 2020-03-04T23:41:22Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f972d4b
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-03-05T09:59:04Z

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

aws-cdk-automation · 2020-03-05T10:53:47Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8cdc403
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-03-05T10:54:09Z

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

* feat(aws-efs): adding construct library for creating EFS * 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes #6286 * addressing review comments * setting correct ipaddress for mount targets * feat(aws-efs): adding construct library for creating EFS * 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes #6286 * addressing review comments * setting correct ipaddress for mount targets * address review comments v2 * removing mount targets info Co-authored-by: Rico Huijbers <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

* feat(aws-efs): adding construct library for creating EFS * 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes aws#6286 * addressing review comments * setting correct ipaddress for mount targets * feat(aws-efs): adding construct library for creating EFS * 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes aws#6286 * addressing review comments * setting correct ipaddress for mount targets * address review comments v2 * removing mount targets info Co-authored-by: Rico Huijbers <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

feat(aws-efs): adding construct library for creating EFS

9b208a6

* 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes aws#6286

yashda requested a review from rix0rrr February 19, 2020 20:31

yashda commented Feb 19, 2020

View reviewed changes

SomayaB assigned rix0rrr Feb 20, 2020

rix0rrr previously requested changes Feb 24, 2020

View reviewed changes

addressing review comments

188fec0

yashda added 4 commits February 27, 2020 14:40

setting correct ipaddress for mount targets

6d080bc

feat(aws-efs): adding construct library for creating EFS

8916e92

* 100% unit test coverage. * This was tested by creating an EFS using this construct in a cdk application. A instance was also created in this app, which successfully mounted it. closes aws#6286

addressing review comments

95983ac

setting correct ipaddress for mount targets

ff0e30c

rebasing the branch

b78a698

Merge branch 'master' into yashda/efs-L2-construct

a36a011

rix0rrr previously requested changes Mar 2, 2020

View reviewed changes

address review comments v2

2caba56

rix0rrr previously requested changes Mar 3, 2020

View reviewed changes

removing mount targets info

f972d4b

rix0rrr changed the title ~~feat(aws-efs): adding construct library for creating EFS~~ feat(efs): create EFS file systems Mar 5, 2020

rix0rrr approved these changes Mar 5, 2020

View reviewed changes

Merge branch 'master' into yashda/efs-L2-construct

8cdc403

mergify bot merged commit a0fb518 into aws:master Mar 5, 2020

snyk-bot mentioned this pull request Apr 10, 2020

[Snyk] Upgrade aws-cdk from 1.28.0 to 1.30.0 ammobinDOTca/ammobin-cdk#2

Closed


		fileSystem.connections.allowDefaultPortFrom(inst);

		inst.userData.addCommands("yum check-update -y", // Ubuntu: apt-get -y update

feat(efs): create EFS file systems #6373

feat(efs): create EFS file systems #6373

Conversation

yashda commented Feb 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 19, 2020

AWS CodeBuild CI Report

pahud commented Feb 20, 2020

yashda commented Feb 20, 2020

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

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

aws-cdk-automation commented Feb 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Feb 27, 2020

AWS CodeBuild CI Report

rix0rrr commented Mar 2, 2020

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

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

aws-cdk-automation commented Mar 2, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 3, 2020

AWS CodeBuild CI Report

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

aws-cdk-automation commented Mar 4, 2020

AWS CodeBuild CI Report

mergify bot commented Mar 5, 2020

aws-cdk-automation commented Mar 5, 2020

AWS CodeBuild CI Report

mergify bot commented Mar 5, 2020