feat(aws-efs): grant support on FileSystem by DaWyz · Pull Request #14999 · aws/aws-cdk

DaWyz · 2021-06-06T04:49:14Z

FileSystem now have the fileSystemArn attribute available. It is also possible to import an existing FileSystem by arn or id using the fromFileSystemAttributes method.

You can also grant permission to an existing grantee using the grant method.

See the example below giving an IAM Role permission to write to an imported file system:

const arn = stack.formatArn({
  service: 'elasticfilesystem',
  resource: 'file-system',
  resourceName: 'fs-12912923',
});

const importedFileSystem = efs.FileSystem.fromFileSystemAttributes(this, 'existingFS', {
  fileSystemArn: arn, // You can also use fileSystemArn instead of fileSystemId.
  securityGroup: ec2.SecurityGroup.fromSecurityGroupId(this, 'SG', 'sg-123456789', {
    allowAllOutbound: false,
  }),
});

const role = new iam.Role(this, 'Access Role', { assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com') });

importedFileSystem.grant(role, 'elasticfilesystem:ClientWrite');

Closes #14998.

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

gitpod-io · 2021-06-06T04:49:16Z

nija-at

Thanks for the PR @DaWyz. Looks mostly good. A few comments below.

nija-at · 2021-06-21T09:56:24Z

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

+    this.fileSystemArn = Stack.of(scope).formatArn({
+      service: 'elasticfilesystem',
+      resource: 'file-system',
+      resourceName: this.fileSystemId,
+    });


I believe you should just get the arn from CfnFileSystem.

Suggested change

this.fileSystemArn = Stack.of(scope).formatArn({

service: 'elasticfilesystem',

resource: 'file-system',

resourceName: this.fileSystemId,

});

this.fileSystemArn = filesystem.attrArn;

Good catch. Just updated the code.

nija-at · 2021-06-21T09:58:37Z

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

    super(scope, id);

-    this.fileSystemId = attrs.fileSystemId;
+    if (!attrs.fileSystemId && !attrs.fileSystemArn) {


Either we should change this so that only one of them is provided, or if we want to support providing both, we should validate (later) that they are not different.

I'm going to throw an error if both are provided (similar to how it's done for DynamoDB tables).

nija-at · 2021-06-21T10:01:52Z

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

+const role = new iam.Role(this, 'Role', {
+  assumedBy: new iam.AnyPrincipal(),
+});
+
+importedFileSystem.grant(role, 'elasticfilesystem:ClientWrite');


Document the grant API in a separate section.

Pull request has been modified.

nija-at

Thanks for making the changes. A few more comments below.

nija-at · 2021-06-22T04:50:20Z

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

+});
+```
+
+### Granting fileSystem permissions to resources


Suggested change

### Granting fileSystem permissions to resources

### Permissions

nija-at · 2021-06-22T04:51:13Z

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

+If you need to grant file system permissions to another resource, you can use the `.grant()` method.
+As an example, the following code gives ClientWrite permissions to an IAM role.


Suggested change

If you need to grant file system permissions to another resource, you can use the `.grant()` method.

As an example, the following code gives ClientWrite permissions to an IAM role.

If you need to grant file system permissions to another resource, you can use the `grant()` API.

As an example, the following code gives `elasticfilesystem:ClientWrite` permissions to an IAM role.

nija-at · 2021-06-22T04:56:18Z

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

+    if (!attrs.fileSystemId && !attrs.fileSystemArn) {
+      throw new Error('One of fileSystemId or fileSystemArn must be provided.');
+    }
+
+    if (attrs.fileSystemId && attrs.fileSystemArn) {
+      throw new Error('Only one of fileSystemId or fileSystemArn must be provided.');
+    }


I believe you can shorten this by

Suggested change

if (!attrs.fileSystemId && !attrs.fileSystemArn) {

throw new Error('One of fileSystemId or fileSystemArn must be provided.');

}

if (attrs.fileSystemId && attrs.fileSystemArn) {

throw new Error('Only one of fileSystemId or fileSystemArn must be provided.');

}

if (!!attrs.fileSystemId === !!attrs.fileSystemArn) {

throw new Error('One of fileSystemId or fileSystemArn, but not both, must be provided.');

}

nija-at · 2021-06-22T04:56:52Z

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

+    const parsedArn = Stack.of(scope).parseArn(this.fileSystemArn);
+
+    if (!parsedArn.resourceName) {
+      throw new Error('Invalid FileSystem Arn.');


Suggested change

throw new Error('Invalid FileSystem Arn.');

throw new Error(`Invalid FileSystem Arn ${this.filesystemArn}`);

nija-at · 2021-06-22T04:58:37Z

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

+      throw new Error('Invalid FileSystem Arn.');
+    }
+
+    this.fileSystemId = attrs.fileSystemId ?? Stack.of(scope).parseArn(this.fileSystemArn).resourceName!;


avoid the non-null assertion operator !. I believe you can now do -

Suggested change

this.fileSystemId = attrs.fileSystemId ?? Stack.of(scope).parseArn(this.fileSystemArn).resourceName!;

this.fileSystemId = attrs.fileSystemId ?? parsedArn.resourceName;

nija-at · 2021-06-22T05:00:14Z

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

+    if (!attrs.fileSystemId && !attrs.fileSystemArn) {
+      throw new Error('One of fileSystemId or fileSystemArn must be provided.');
+    }
+
+    if (attrs.fileSystemId && attrs.fileSystemArn) {
+      throw new Error('Only one of fileSystemId or fileSystemArn must be provided.');
+    }


Add unit tests for these

Pull request has been modified.

DaWyz · 2021-06-22T13:47:46Z

Updates the code following last recommendations. Also updated the pull request description.

nija-at

Awesome! Looks great.

mergify · 2021-06-22T16:15:34Z

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

nija-at

@DaWyz - can you take a look at the failing build with the latest changes.

It's likely because you need to update the tests to use the new assertion framework - 367fe7e

Pull request has been modified.

mergify · 2021-07-15T14:38:17Z

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 · 2021-07-15T15:16:49Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 936c769
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-07-15T15:17:18Z

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

`FileSystem` now have the `fileSystemArn` attribute available. It is also possible to import an existing `FileSystem` by `arn` or `id` using the `fromFileSystemAttributes` method. You can also grant permission to an existing grantee using the grant method. See the example below giving an IAM Role permission to write to an imported file system: ```ts const arn = stack.formatArn({ service: 'elasticfilesystem', resource: 'file-system', resourceName: 'fs-12912923', }); const importedFileSystem = efs.FileSystem.fromFileSystemAttributes(this, 'existingFS', { fileSystemArn: arn, // You can also use fileSystemArn instead of fileSystemId. securityGroup: ec2.SecurityGroup.fromSecurityGroupId(this, 'SG', 'sg-123456789', { allowAllOutbound: false, }), }); const role = new iam.Role(this, 'Access Role', { assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com') }); importedFileSystem.grant(role, 'elasticfilesystem:ClientWrite'); ``` Closes aws#14998. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

feat(aws-efs): Add arn attribute to FileSystem class

070c266

peterwoodworth assigned nija-at Jun 8, 2021

peterwoodworth added the @aws-cdk/aws-efs Related to Amazon Elastic File System label Jun 8, 2021

fixing README syntax for Rosetta to complile + replacing let by const

1d573da

nija-at changed the title ~~feat(aws-efs): Add arn attribute and grant method to FileSystem construct~~ feat(aws-efs): grant support on FileSystem Jun 21, 2021

nija-at previously requested changes Jun 21, 2021

View reviewed changes

DaWyz added 2 commits June 21, 2021 16:11

improving code and documentation

429d943

fixing test assertion

2436ad3

DaWyz requested a review from nija-at June 22, 2021 03:11

nija-at previously requested changes Jun 22, 2021

View reviewed changes

Improving doc, fixing nits and adding unit tests as per comments

13c32c1

DaWyz requested a review from nija-at June 22, 2021 15:01

nija-at approved these changes Jun 22, 2021

View reviewed changes

Merge branch 'master' into DaWyz/efs-file-system-arn-attribute

f5b0da9

nija-at previously requested changes Jun 23, 2021

View reviewed changes

fix test using new assertion mechanism

1c73145

DaWyz requested a review from nija-at June 24, 2021 06:16

nija-at approved these changes Jul 15, 2021

View reviewed changes

Merge branch 'master' into DaWyz/efs-file-system-arn-attribute

936c769

mergify bot merged commit 09591c6 into aws:master Jul 15, 2021

	### Granting fileSystem permissions to resources
	### Permissions

		If you need to grant file system permissions to another resource, you can use the `.grant()` method.
		As an example, the following code gives ClientWrite permissions to an IAM role.

	throw new Error('Invalid FileSystem Arn.');
	throw new Error(`Invalid FileSystem Arn ${this.filesystemArn}`);

	this.fileSystemId = attrs.fileSystemId ?? Stack.of(scope).parseArn(this.fileSystemArn).resourceName!;
	this.fileSystemId = attrs.fileSystemId ?? parsedArn.resourceName;

Conversation

DaWyz commented Jun 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Jun 6, 2021

Uh oh!

nija-at 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nija-at 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!

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!

DaWyz commented Jun 22, 2021

Uh oh!

nija-at left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jun 22, 2021

Uh oh!

nija-at left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jul 15, 2021

Uh oh!

aws-cdk-automation commented Jul 15, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DaWyz commented Jun 6, 2021 •

edited

Loading