Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add ability to use EFS access points #339

Merged
merged 1 commit into from
Mar 12, 2021
Merged

feat: add ability to use EFS access points #339

merged 1 commit into from
Mar 12, 2021

Conversation

jusiskin
Copy link
Contributor

@jusiskin jusiskin commented Mar 9, 2021 •
edited
Loading

Summary

This change modifies the MountableEfs class to accept an optional accessPoint property. When specified, the construct will generate user data that mounts the EFS file-system using the access point.

This allows the process accessing the file-system to use a UID/GID different than what UID/GID of the user running the process on the file-system mount.

Additional Changes

The AWS-all-in-basic examples were modified to use EFS access points.

Testing

  • Both of the Python and TypeScript examples were tested with Deadline Docker recipes where the container UID/GID would not have POSIX permissions to the EFS without an access point. When using EFS access points, the container is able to read/write to the EFS as expected.
  • Ran the RFDK integration tests on this branch and they all passed

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

@jusiskin jusiskin force-pushed the efs_access_points branch 4 times, most recently from 85a14a5 to 5c417b3 Compare March 10, 2021 16:18
@jericht jericht self-requested a review March 10, 2021 19:55
ddneilson
ddneilson requested changes Mar 11, 2021
View reviewed changes
packages/aws-rfdk/lib/core/lib/mountable-efs.ts Show resolved Hide resolved
packages/aws-rfdk/lib/core/lib/mountable-efs.ts Outdated
@@ -75,6 +98,23 @@ export class MountableEfs implements IMountableLinuxFilesystem {
throw new Error('Target instance must be Linux.');
}

target.node.addDependency(this.props.filesystem.mountTargetsAvailable);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work if it's target.node?. ? Or, alternatively, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwnProperty ?

Then we don't have to change the definition of IMountingInstance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think MountableEfs already requires a construct, unless I'm mistaken. It needs to be able to modify the IMountingInstance's user data, right? On that basis, there should be no harm in this addition. Perhaps I'm missing some corner case though...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember there was a reason that I didn't put IConstruct in the original definition. I don't recall what that reason was... some corner case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I should be able to use Construct.isConstruct(...). Changing to that....

jericht
jericht requested changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@jericht jericht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one inconsistency in an upgrade doc example, otherwise it looks good to me!

packages/aws-rfdk/docs/upgrade/upgrading-0.27.md Outdated Show resolved Hide resolved
packages/aws-rfdk/docs/upgrade/upgrading-0.27.md Outdated Show resolved Hide resolved
@jusiskin
feat: add ability to use EFS access points
8735690 
- Allows instances to access EFS with a specified UID/GID
BREAKING CHANGE: Repository constructs supplied with an EFS file-system must also pass an EFS Access Point
- If your application provides an EFS file-system to a Repository construct, it must now also pass an
  EFS Access Point to work properly with the Deadline container images.
- Consult https://github.com/aws/aws-rfdk/blob/v0.27.0/packages/aws-rfdk/docs/upgrade/upgrading-0.27.md
  for detailed instructions on how to upgrade
@jusiskin jusiskin marked this pull request as ready for review March 12, 2021 05:24
@jusiskin jusiskin mentioned this pull request Mar 12, 2021
Open
ddneilson
ddneilson approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@ddneilson ddneilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks Josh!

@jusiskin jusiskin merged commit 544496c into aws:mainline Mar 12, 2021
@jusiskin jusiskin deleted the efs_access_points branch March 12, 2021 16:50
@jusiskin jusiskin added the contribution/core This is a PR that came from AWS. label Mar 12, 2021
@jusiskin jusiskin mentioned this pull request Mar 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jericht jericht jericht approved these changes

@ddneilson ddneilson ddneilson approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jusiskin @ddneilson @jericht