feat(efs): import access point - fromAccessPointAttributes()

[DaWyz]

Cannot use efs.AccessPoint.fromAccessPointId() with lambda.FileSystem.fromEfsAccessPoint(). the former returns an IAccessPoint when the later expect an AccessPoint. I think following the CDK guidelines, lambda.FileSystem.fromEfsAccessPoint() should expect an IAccessPoint, not an AccessPoint.

Argument of type IAccessPoint is not assignable to parameter of type AccessPoint.

Solution

---

Add a new import method to the AccessPoint class called fromAccessPointAttributes() allowing to pass a fileSystem as an attribute.

Closes #10711.

---

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

[DaWyz]

I just looked into it again. I'm not sure it makes sense to me anymore. Also, the issue is still valid, the implementation I made doesn't seem like the right way to do it. If you think the same, I'm happy to discuss about the implementation and close this Pull Request.

I'm just confused about the FileSystem class in the @aws-cdk/aws-lambda and I'm wondering if it's not kind of redundant.

[shivlaks]

@DaWyz

awesome first pass!! thanks for getting the ball rolling on this. I have some thoughts that I'd love to work in before merging this in. The most important one is creating a class that represents an imported file system. Take a look at the example of how it's done in rds and let me know what you think!

Pull request has been modified.

[DaWyz]

@DaWyz

awesome first pass!! thanks for getting the ball rolling on this. I have some thoughts that I'd love to work in before merging this in. The most important one is creating a class that represents an imported file system. Take a look at the example of how it's done in rds and let me know what you think!

@shivlaks We could extract the existing imported file system class if it's what you mean. I can do the same for the AccessPoint as well.

About the IFileSystem property in the IAccessPoint, I made it mandatory because of the lambda efs implementation. This is the only place where the AccessPoint class is used and it needs the FileSystem to be able to setup the ingress rules.

So, at the moment, we cannot import an existing access point since it doesn't have the fileSystem property when using fromAccessPointId. I guess we could make the FileSystem property of fromAccessPointAttributes optional by updating the code in the lambda efs implementation to check for ap.fileSystem presence and make if fail with a nice error message in case it's missing.

Also, I dropped the existing fromAccessPointId method and I don't think I should. Making the fileSystem property optional would allow me to add fromAccessPointId method back.

Let me know what you think and I can update the code accordingly.

[gitpod-io]

[shivlaks]

@shivlaks We could extract the existing imported file system class if it's what you mean. I can do the same for the AccessPoint as well.

yep that is what I mean. extract to ImportedFileSystem or so that implements IFileSystem and we just return new ImportedFileSystem in the fromFileSystemAttributes method

About the IFileSystem property in the IAccessPoint, I made it mandatory because of the lambda efs implementation. This is the only place where the AccessPoint class is used and it needs the FileSystem to be able to setup the ingress rules.

got it, thanks for explaining! might be worth a comment.

So, at the moment, we cannot import an existing access point since it doesn't have the fileSystem property when using fromAccessPointId. I guess we could make the FileSystem property of fromAccessPointAttributes optional by updating the code in the lambda efs implementation to check for ap.fileSystem presence and make if fail with a nice error message in case it's missing.

It seems reasonable to me. Adding the fileSystem property would have to be optional as you've suggested because the alternative is a breaking change and the lambda module is stable.
I'd defer to @nija-at on whether that's an approach he's comfortable with as he's the primary maintainer of the module.

Also, I dropped the existing fromAccessPointId method and I don't think I should. Making the fileSystem property optional would allow me to add fromAccessPointId method back.

I agree, let's avoid dropping the method.

Let me know what you think and I can update the code accordingly.

[nija-at]

Some suggestions below.

[nija-at]

How about make this mandatory and throw an error in the implementation of fromAccessPointId()?

The error message can be "fileSystem is not available when 'fromAccessPointId()' is used. Use 'fromAccessPointAttributes()' instead"

[DaWyz]

@nija-at, that would also be a breaking change as well, right ?

I'm wondering if there is a use-case (outside of EFS for lambda) where we would need to import an AccessPoint without having the need for FileSystem ?

[nija-at]

efs is an experimental module. Breaking changes are acceptable as long as they are called out in the commit description.

I'm wondering if there is a use-case (outside of EFS for lambda) where we would need to import an AccessPoint without having the need for FileSystem ?

In such cases exist, the fromAccessPointId() API can be used.

[DaWyz]

I see. I didn't get the idea of how to implement this at start. I think I'm good now. I'm doing the same as in the rds imported cluster and throw the exception on the Getter when fileSystem is not present.. See below. Will update the PR.

class ImportedAccessPoint extends AccessPointBase {
  public readonly accessPointId: string;
  public readonly accessPointArn: string;
  private readonly _fileSystem?: IFileSystem;

  constructor(scope: Construct, id: string, attrs: AccessPointAttributes) {
    super(scope, id);

    if (!attrs.accessPointId) {
      if (!attrs.accessPointArn) {
        throw new Error('One of accessPointId or AccessPointArn is required!');
      }

      this.accessPointArn = attrs.accessPointArn;
      let maybeApId = Stack.of(scope).parseArn(attrs.accessPointArn).resourceName;

      if (!maybeApId) {
        throw new Error('ARN for AccessPoint must provide the resource name.');
      }

      this.accessPointId = maybeApId;
    } else {
      if (attrs.accessPointArn) {
        throw new Error('Only one of accessPointId or AccessPointArn can be provided!');
      }

      this.accessPointId = attrs.accessPointId;
      this.accessPointArn = Stack.of(scope).formatArn({
        service: 'elasticfilesystem',
        resource: 'access-point',
        resourceName: attrs.accessPointId,
      });
    }

    this._fileSystem = attrs.fileSystem;
  }

  public get fileSystem() {
    if (!this._fileSystem) {
      throw new Error("fileSystem is not available when 'fromAccessPointId()' is used. Use 'fromAccessPointAttributes()' instead");
    }

    return this._fileSystem;
  }
}

Pull request has been modified.

[shivlaks]

@DaWyz - changes look good to me!
i added a do-not-merge so @nija-at can also take a look

[shivlaks]

confirmed with @nija-at that we shouldn't wait on another review, thanks for the contribution @DaWyz !!

[mergify]

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]

AWS CodeBuild CI Report

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

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

[mergify]

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

[DaWyz]

confirmed with @nija-at that we shouldn't wait on another review, thanks for the contribution @DaWyz !!

No problem! Happy to help! I'm sure you guys are extremely busy with CDK so thank you for taking the time to review the PRs! Keep up the good work!