fix(lambda): Function allows specifying vpcSubnets without vpc by laurelmay · Pull Request #21369 · aws/aws-cdk

laurelmay · 2022-07-29T02:22:41Z

This is almost certainly never something that a user would intend.
As-is, vpcSubnets does nothing if vpc is unspecified, so the Lambda
Function would have no interfaces placed in a VPC. This is unlikely to be what
a user would have expected if they passed a set of private subnets. This
should be an error to prevent users from accidentally thinking they've
put a Lambda Function into a VPC.

If throwing an Error is a breaking change and not acceptable, then
perhaps at least a Warning Annotation should be added in this case.

This is motivated by the feedback received during review of #21357.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-07-29T02:22:47Z

This is almost certainly never something that a user would intend. As-is, `vpcSubnets` does nothing if `vpc` is unspecified, so the Lambda Function would have no interfaces placed in a VPC. This is unlikely what a user would have expected if they passed a set of private subnets. This should be an error to prevent users from accidentally thinking they've put a Lambda Function into a VPC. If throwing an Error is a breaking change and not acceptable, then perhaps at least a Warning Annotation should be added in this case.

corymhall · 2022-08-03T19:08:10Z

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

    if (props.vpc) {
      // Policy that will have ENI creation permissions
      managedPolicies.push(iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaVPCAccessExecutionRole'));
+    } else if (props.vpcSubnets) {


I think I'm fine with this change given that it does not cause any changes to the deployed infra.

One nit - can you put this check in the configureVpc method? It feels like it should go there since that is where the other checks are.

Pull request has been modified.

laurelmay · 2022-08-03T23:43:14Z

Hoping the failure on ec5cd4f is fixed by #21374 and therefore the merge from main.

TheRealAmazonKendra

Just a couple of stylistic suggestions.

TheRealAmazonKendra · 2022-08-04T01:31:10Z

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

+    if (!props.vpc) {
+      if (props.vpcSubnets) {


Suggested change

if (!props.vpc) {

if (props.vpcSubnets) {

if (!props.vpc && props.vpcSubnets) {

I thought about doing this, but there's the return undefined; after the inner if. So it'd have to be rewritten as:

if (!props.vpc && props.vpcSubnets) { throw new Error("..."); } else if (!props.vpc) { return undefined }

which felt a little messy and maybe more confusing to repeat half of the condition?

Actually mean to include a note/question about this when I pushed the new commit but got distracted by the failing build 😄 So I'm happy to making the change but just want to make sure that this is the desired outcome?

No need for you to revise because of a small stylistic difference. I only made the suggestion because I was also making the suggestion to change the error message. Otherwise, I would have just plain approved since @corymhall is also in favor.

But also, I didn't notice that the error message above was infinitely better than the one I suggested. So, sorry, one more suggested change to that.

All good! And I meant "meant to" instead of "mean to" -- oops, sorta changes the tone. But in any case, thanks for the feedback on the error message. I read https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#error-messages and I think ended up writing something too wordy; this one is much clearer!

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

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Pull request has been modified.

TheRealAmazonKendra

I should have looked at the code directly above and made my suggestion to align to that. Sorry for the extra back and forth!

TheRealAmazonKendra · 2022-08-04T04:48:41Z

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

+    if (!props.vpc) {
+      if (props.vpcSubnets) {


But also, I didn't notice that the error message above was infinitely better than the one I suggested. So, sorry, one more suggested change to that.

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

packages/@aws-cdk/aws-lambda/test/vpc-lambda.test.ts

TheRealAmazonKendra

Aaand then I hit the wrong button...

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Pull request has been modified.

laurelmay · 2022-08-04T12:42:45Z

Not sure what's up with the CodeBuild failure this time around -- it doesn't really feel related to anything that's changed and running the integ-runner tests locally doesn't result in a failure. I think I'm just going to hope it's a one-off issue and that re-triggering a build fixes it? But I don't know how to ask CodeBuild to rebuild without a new commit so I'll just click the Update branch button? 🤷🏻

@aws-cdk/integ-runner: FAIL test/runner/snapshot-test-runner.test.js
@aws-cdk/integ-runner:   ● IntegTest runSnapshotTests › with defaults no diff
@aws-cdk/integ-runner:     Cannot read integ manifest 'test/test-data/test-with-snapshot.integ.snapshot/manifest.json': Unexpected end of JSON input
@aws-cdk/integ-runner:       32 |
@aws-cdk/integ-runner:       33 |     } catch (e) {
@aws-cdk/integ-runner:     > 34 |       throw new Error(`Cannot read integ manifest '${fileName}': ${e.message}`);
@aws-cdk/integ-runner:          |             ^
@aws-cdk/integ-runner:       35 |     }
@aws-cdk/integ-runner:       36 |   }
@aws-cdk/integ-runner:       37 |
@aws-cdk/integ-runner:       at Function.fromFile (lib/runner/private/cloud-assembly.ts:34:13)
@aws-cdk/integ-runner:       at Function.fromPath (lib/runner/private/cloud-assembly.ts:51:37)
@aws-cdk/integ-runner:       at IntegSnapshotRunner.readAssembly (lib/runner/snapshot-test-runner.ts:223:45)
@aws-cdk/integ-runner:       at IntegSnapshotRunner.testSnapshot (lib/runner/snapshot-test-runner.ts:55:33)
@aws-cdk/integ-runner:       at Object.<anonymous> (test/runner/snapshot-test-runner.test.ts:55:31)

mergify · 2022-08-04T14:13:53Z

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

aws-cdk-automation · 2022-08-04T15:30:09Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 6512acb
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-08-04T15:30:41Z

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

Recently, the Function construct was changed to raise an error if `vpcSubnets` was specified but `vpc` wasn't; however, this change was not properly reflected in the documentation for the function. The change itself was made in aws#21369. Closes aws#21565

Recently, the Function construct was changed to raise an error if `vpcSubnets` was specified but `vpc` wasn't; however, this change was not properly reflected in the documentation for the function. The change itself was made in #21369. Closes #21565 ---- ### All Submissions: * [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p2 label Jul 29, 2022

aws-cdk-automation requested a review from a team July 29, 2022 02:22

laurelmay mentioned this pull request Jul 29, 2022

feat(custom-resource): allow AwsCustomResource to be placed in vpc #21357

Merged

4 tasks

laurelmay force-pushed the add-warning-on-vpcsubnets-without-vpc branch from 50d0061 to ad050ac Compare July 29, 2022 02:55

laurelmay force-pushed the add-warning-on-vpcsubnets-without-vpc branch from ad050ac to 6e52f64 Compare July 29, 2022 03:08

corymhall previously requested changes Aug 3, 2022

View reviewed changes

Perform validation in the configureVpc method

ec5cd4f

Merge branch 'main' into add-warning-on-vpcsubnets-without-vpc

f07cbaf

TheRealAmazonKendra previously requested changes Aug 4, 2022

View reviewed changes

Improve error message text

fc723fc

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Update expected message to match error text

dafe583

TheRealAmazonKendra approved these changes Aug 4, 2022

View reviewed changes

TheRealAmazonKendra added the pr/do-not-merge This PR should not be merged at this time. label Aug 4, 2022

TheRealAmazonKendra previously requested changes Aug 4, 2022

View reviewed changes

TheRealAmazonKendra removed the pr/do-not-merge This PR should not be merged at this time. label Aug 4, 2022

Describe error condition better

d97aedd

Co-authored-by: Kendra Neil <53584728+TheRealAmazonKendra@users.noreply.github.com>

Merge branch 'main' into add-warning-on-vpcsubnets-without-vpc

131d9ea

TheRealAmazonKendra approved these changes Aug 4, 2022

View reviewed changes

Merge branch 'main' into add-warning-on-vpcsubnets-without-vpc

6512acb

mergify bot merged commit e9233fa into aws:main Aug 4, 2022

laurelmay deleted the add-warning-on-vpcsubnets-without-vpc branch August 4, 2022 15:35

laurelmay mentioned this pull request Aug 12, 2022

docs(lambda): Document recent changes to vpcSubnets property #21572

Merged

4 tasks

	if (!props.vpc) {
	if (props.vpcSubnets) {
	if (!props.vpc && props.vpcSubnets) {

Conversation

laurelmay commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jul 29, 2022

Uh oh!

corymhall Aug 3, 2022

Choose a reason for hiding this comment

Uh oh!

laurelmay commented Aug 3, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

laurelmay Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

laurelmay Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

laurelmay commented Aug 4, 2022

Uh oh!

mergify bot commented Aug 4, 2022

Uh oh!

aws-cdk-automation commented Aug 4, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

laurelmay commented Jul 29, 2022 •

edited

Loading