fix(lambda): custom resource fails to connect to efs filesystem by ddneilson · Pull Request #14431 · aws/aws-cdk

ddneilson · 2021-04-28T19:50:27Z

Fixes: #14430

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

gitpod-io · 2021-04-28T19:50:29Z

nija-at

Thanks for submitting this PR! @ddneilson

Please see my comments below.

packages/@aws-cdk/aws-lambda/lib/function.ts

nija-at · 2021-05-06T14:19:35Z

packages/@aws-cdk/aws-lambda/lib/function.ts

    }

+    if (!props.vpc && props.filesystem) {
+      throw new Error('Cannot configurea \'filesystem\' without configuring a VPC.');


Suggested change

throw new Error('Cannot configurea \'filesystem\' without configuring a VPC.');

throw new Error("Cannot configure 'filesystem' without configuring a VPC.");

Fixed in next revision.

nija-at · 2021-05-06T14:20:13Z

packages/@aws-cdk/aws-lambda/lib/function.ts

+    if (!props.vpc && props.filesystem) {
+      throw new Error('Cannot configurea \'filesystem\' without configuring a VPC.');
+    }


Move this validation into the if (props.filesystem) conditional as well.

That would be an error, I'm afraid. It would put the check after:

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 859 in 46a1631

if (!props.vpc) { return undefined; }

which would cause the condition to never be true.

I'm not sure I understand the error. Can you explain?

Mmm. Maybe a miscommunication stemming from ambiguity. I am assuming that you mean this conditional in the same function:

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 884 in 71c61e8

if (props.filesystem) {

That would be the error I outlined.

But, perhaps you mean one of these conditionals in the constructor?

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 570 in 71c61e8

if (props.filesystem) {

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 618 in 71c61e8

if (props.filesystem) {

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 703 in 71c61e8

if (props.filesystem) {

Checking for the vpc property in one of those, and throwing an error, is redundant. There is an unconditional call to configureVpc() in the same constructor:

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 645 in 71c61e8

vpcConfig: this.configureVpc(props),

So, the only way to not hit the code that I've added here would be to have already thrown an error due to the securityGroup check, which has the same resolution as the error that I have added:

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 856 in 71c61e8

throw new Error('Cannot configure \'securityGroup\' or \'allowAllOutbound\' without configuring a VPC');

Perhaps it's better to just modify:

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 855 in 71c61e8

if ((props.securityGroup || props.allowAllOutbound !== undefined) && !props.vpc) {

to

if ((props.securityGroup || props.allowAllOutbound !== undefined || props.filesystem) && !props.vpc) { throw new Error('Cannot configure \'securityGroup\', \'allowAllOutbound\', or \'filesystem\' without configuring a VPC'); }

?

For clarity, I meant adding the conditional here right next to where you are adding the new dependencies -

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Line 570 in 71c61e8

if (props.filesystem) {

There is an unconditional call to configureVpc() in the same constructor. So, the only way to not hit the code that I've added here would be to have already thrown an error due to the securityGroup check, which has the same resolution as the error that I have added.

I'm finding a bit difficult to understand what you're saying here. It's not clear to me why the configureVpc() method has anything to do with this.

Perhaps I didn't explain the intention behind my comment - I wanted to keep the validation around filesystem, in the same place(s) as where the filesystem is being configured. configureVpc() feels like the wrong place.

At a later time (i.e., not in this PR), I'd like to bring all filesystem related code into a single (or a few) methods instead of it littered in the constructor.

Let me know if you're still having difficulty.

I'm finding a bit difficult to understand what you're saying here. It's not clear to me why the configureVpc() method has anything to do with this.

The code in question is within configureVpc().... but I digress. Sure, I'll move the check & error to the conditional where the dependencies are being added.

Perhaps I didn't explain the intention behind my comment - I wanted to keep the validation around filesystem, in the same place(s) as where the filesystem is being configured. configureVpc() feels like the wrong place.

Yes. Starting with that would have been useful. :-)

gitpod-io · 2021-05-06T18:53:56Z

Pull request has been modified.

ddneilson · 2021-05-10T14:49:05Z

@nija-at Are we good to merge? I do not see any remaining comments that have not been addressed.

mergify · 2021-05-11T08:23:16Z

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-05-11T09:09:21Z

AWS CodeBuild CI Report

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

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

mergify · 2021-05-11T09:09:22Z

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

@BenChaimberg

* chore(cloudfront): remove the use of calculateFunctionHash (#14583) `calculateFunctionHash()` was used to compute the 'refresh token' of the custom resource for the EdgeFunction construct. This method is private to the lambda module and is deemed to be changed. Instead, use the lambda function version's logical id. The logical id of the version includes computing the function hash (among others) and is a more reliable determinant of whether the underlying function version changed. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cloudwatch): validate parameters for a metric dimensions (closes #3116) (#14365) As per #3116, the changes in this PR validate metric dimension values (length, and checking if the value is null or undefined) and throw errors if the values are not valid. I've also corrected a comment in the metric-types.ts to use the correct method name * feat(appmesh): change HealthChecks to use protocol-specific union-like classes (#14432) BREAKING CHANGE: HealthChecks require use of static factory methods fixes #11640 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(msk): Cluster L2 Construct (#9908) L2 Construct for a MSK Cluster. I wrote this for internal use and thought I'd share it. I tried to follow the [example resource](https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/example-construct-library/lib/example-resource.ts) and [design guidelines](https://github.com/aws/aws-cdk/blob/master/DESIGN_GUIDELINES.md) as much as I could. Default properties were chosen either based on defaults when creating a cluster in the console or defaults set from CloudFormation. Closes #9603 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(kms): allow specifying key spec and key usage (#14478) This allows specifying key spec and key usage, so you can create asymmetric keys. closes #5639 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: add `@types/jest` to a package that was missing it (#14609) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: issue template for bugs to have an SSCCE example (#14615) Ask customers to provide reproducible code snippets to reduce the amount of triage time required. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * fix(cli): synth fails if there was an error when synthesizing the stack (#14613) All stacks created inside a pipeline stage will be flagged for validation. After synth is done, the CLI will validate all flagged stacks plus the stacks that were explicitly specified. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: annotate `aws-lambda-go` with `docker` requirement (#14618) The `nozem` build tool needs to know that `docker` is required to build/test this particular package. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(elbv2): preserveClientIp for NetworkTargetGroup (#14589) Allows users to configure client IP preservation for network target groups. See: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-elasticloadbalancingv2-targetgroup-targetgroupattribute.html ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore: npm-check-updates && yarn upgrade (#14620) Ran npm-check-updates and yarn upgrade to keep the `yarn.lock` file up-to-date. * chore(mergify): add @BenChaimberg to team roster * chore(release): 1.103.0 * chore: mark "otaviomacedo" as core contributor (#14619) Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com> * chore(cli): add npm command to upgrade notice (#14621) A colleague had to go look up the command to update the CDK CLI. It occurred to me that is probably common with developers who don't work with NPM on a daily basis, such as anyone who isn't developing in TypeScript or JavaScript. Put the necessary command right in the upgrade notice. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * fix(lambda): custom resource fails to connect to efs filesystem (#14431) Fixes: #14430 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * feat(cfnspec): cloudformation spec v35.2.0 (#14610) * feat: cloudformation spec v35.2.0 * add spec patches Co-authored-by: AWS CDK Team <aws-cdk@amazon.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * fix(lambda-nodejs): handler filename missing from error message (#14564) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* * chore(build): clarify prerequisites in CONTRIBUTING and verify before build (#14642) The build script at the top level of the repository performs a prerequisites check before beginning the build. This verification is not complete as it does not include the required checks for .NET and Python executables. Further, the prerequisites documentation in the contributing guide does not note the Docker requirement. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored-by: Niranjan Jayakar <nija@amazon.com> Co-authored-by: OksanaH <34384274+OksanaH@users.noreply.github.com> Co-authored-by: Dominic Fezzie <fezzid@amazon.com> Co-authored-by: Curtis <curtis.eppel@cultureamp.com> Co-authored-by: Shinya Tsuda <shinya@dacci.org> Co-authored-by: Rico Huijbers <rix0rrr@gmail.com> Co-authored-by: Otavio Macedo <otaviomacedo@protonmail.com> Co-authored-by: Griffin Byatt <byatt.griffin@gmail.com> Co-authored-by: Ben Chaimberg <chaimber@amazon.com> Co-authored-by: AWS CDK Team <aws-cdk@amazon.com> Co-authored-by: Mitchell Valine <valinm@amazon.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Jerry Kindall <52084730+Jerry-AWS@users.noreply.github.com> Co-authored-by: Daniel Neilson <53624638+ddneilson@users.noreply.github.com> Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

…14431) Fixes: aws#14430 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot assigned iliapolo Apr 28, 2021

github-actions bot added @aws-cdk/aws-efs Related to Amazon Elastic File System @aws-cdk/aws-lambda Related to AWS Lambda labels Apr 28, 2021

github-actions bot assigned nija-at Apr 28, 2021

Daniel Neilson added 2 commits April 29, 2021 15:33

fix(lambda): adds missing deploy-time deps for lambda with efs

93a8d74

fix integ test failure

7081f4d

ddneilson force-pushed the lambda_efs_deps branch from 708c3bd to 7081f4d Compare April 29, 2021 16:57

iliapolo removed their assignment May 2, 2021

iliapolo removed the @aws-cdk/aws-efs Related to Amazon Elastic File System label May 2, 2021

nija-at previously requested changes May 6, 2021

View reviewed changes

Daniel Neilson added 2 commits May 6, 2021 18:52

Minor typo fix in an error string

6c0d68a

Merge branch 'master' into lambda_efs_deps

8558c0e

Daniel Neilson added 2 commits May 10, 2021 17:19

Move check that ensures vpc when providing filesystem

9e80a7b

Merge branch 'master' into lambda_efs_deps

d082c7b

nija-at changed the title ~~fix(lambda): adds missing deploy-time deps for lambda with efs~~ fix(lambda): lambda based custom resource fails to connect to efs filesystem May 11, 2021

nija-at changed the title ~~fix(lambda): lambda based custom resource fails to connect to efs filesystem~~ fix(lambda): custom resource fails to connect to efs filesystem May 11, 2021

github-actions bot added the @aws-cdk/aws-efs Related to Amazon Elastic File System label May 11, 2021

github-actions bot assigned iliapolo May 11, 2021

nija-at approved these changes May 11, 2021

View reviewed changes

Merge branch 'master' into lambda_efs_deps

12286fb

nija-at added effort/small Small work item – less than a day of effort p1 p2 and removed p1 labels May 11, 2021

mergify bot merged commit 10a633c into aws:master May 11, 2021

ddneilson mentioned this pull request May 17, 2021

chore(deps): bumping cdk from 1.102.0 to 1.104.0 aws/aws-rfdk#429

Merged

	throw new Error('Cannot configurea \'filesystem\' without configuring a VPC.');
	throw new Error("Cannot configure 'filesystem' without configuring a VPC.");

Conversation

ddneilson commented Apr 28, 2021

Uh oh!

gitpod-io bot commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nija-at left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

nija-at May 10, 2021 • 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!

gitpod-io bot commented May 6, 2021

Uh oh!

ddneilson commented May 10, 2021

Uh oh!

mergify bot commented May 11, 2021

Uh oh!

aws-cdk-automation commented May 11, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gitpod-io bot commented Apr 28, 2021 •

edited

Loading

nija-at May 10, 2021 •

edited

Loading