feat(custom-resource): allow AwsCustomResource to be placed in vpc by laurelmay · Pull Request #21357 · aws/aws-cdk

laurelmay · 2022-07-28T11:54:17Z

This will allow for users with a need for all Lambda functions to be
placed in a VPC to more easily adopt the use of AwsCustomResource.
This is implemented by accepting a vpc and vpcSubnets property and
passing those up to the created SingletonFunction. Then tests are
added to ensure that it builds correctly and that errors are thrown in
the expected circumstances (in the same situations that
lambda.Function does).

This mimics the setup already used by Provider in the same package.

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

laurelmay · 2022-07-28T11:54:22Z

While this is a feature and may need a README update, I wasn't sure how to go about documenting that naturally; most of the docs go over the features more unique to Provider and AwsCustomResource and there aren't really examples of all the possible options for configuration.

gitpod-io · 2022-07-28T11:54:23Z

TheRealAmazonKendra · 2022-07-28T16:07:24Z

While this is a feature and may need a README update, I wasn't sure how to go about documenting that naturally; most of the docs go over the features more unique to Provider and AwsCustomResource and there aren't really examples of all the possible options for configuration.

https://github.com/aws/aws-cdk/tree/main/packages/%40aws-cdk/custom-resources#customizing-the-lambda-function-implementing-the-custom-resource discusses configuration a bit. Seems like a good place to add documentation for this.

TheRealAmazonKendra · 2022-07-28T16:15:59Z

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

+  /**
+   * Which subnets from the VPC to place the lambda function in.
+   *
+   * Only used if 'vpc' is supplied. Note: internet access for Lambdas


The comments here are doing a lot of heavy lifting. Rather than commenting that this can only be supplied if VPC is supplied, I think we need the contract/code to enforce this. Based off this I would be unclear if vpcSubnets is also required when vpc is supplied.

I think like a lot of situations with lambda.Function this may be tricky? Because I think vpc.selectSubnets()/vpc.selectSubnets(undefined) will return private subnets if you only have private subnets and may return other values if that's not the case. It's a little hard to validate since ec2.SubnetSelection can have such a variety of options. The base construct seems to be pretty good at catching obviously-broken scenarios.

So I think the comments are just documenting some of the synth-time failures that can occur if the expected behavior isn't happening, even though those checks happen in the Lambda package and not here.

Right, there's validation happening elsewhere that will catch some of this. Given that, there should be test cases to ensure that it fails how we expect it to fail in those scenarios.

TheRealAmazonKendra · 2022-07-28T16:17:02Z

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

+   * Which subnets from the VPC to place the lambda function in.
+   *
+   * Only used if 'vpc' is supplied. Note: internet access for Lambdas
+   * requires a NAT gateway, so picking Public subnets is not allowed.


Another thing that needs to be enforced in code. What happens if someone picks public subnets? Will this fail at deployment time or at synth?

I believe synth time.

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

Lines 1131 to 1136 in 17f1ec8

for (const subnetId of subnetIds) {

if (publicSubnetIds.has(subnetId) && !allowPublicSubnet) {

throw new Error('Lambda Functions in a public subnet can NOT access the internet. ' +

'If you are aware of this limitation and would still like to place the function in a public subnet, set `allowPublicSubnet` to true');

}

}

This doesn't allow for passing the allowPublicSubnets prop.

TheRealAmazonKendra · 2022-07-28T16:20:50Z

packages/@aws-cdk/custom-resources/lib/aws-custom-resource/aws-custom-resource.ts

+   *
+   * @default - the Vpc default strategy if not specified
+   */
+  readonly vpcSubnets?: ec2.SubnetSelection;


What happens if this is supplied when VPC is not? Does it fail silently/fail at deployment/fail at synth?

I think it's just ignored? Which seems to be a pattern (here, with lambda.Function, and customresources.Provider). It's not especially ideal but it seems to be semi-predictable.

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

Line 1129 in 17f1ec8

const { subnetIds } = props.vpc.selectSubnets(props.vpcSubnets);

is the only use of props.vpcSubnets I see in the Function construct, so no validation seems to be performed there to guard against it being passed senselessly.

If something is done to validate this prop here, it'd probably also need to work for the Provider construct.

aws-cdk/packages/@aws-cdk/custom-resources/lib/provider-framework/provider.ts

Lines 205 to 219 in 17f1ec8

const fn = new lambda.Function(this, `framework-${entrypoint}`, {

code: lambda.Code.fromAsset(RUNTIME_HANDLER_PATH, {

exclude: ['*.ts'],

}),

description: `AWS CDK resource provider framework - ${entrypoint} (${this.node.path})`.slice(0, 256),

runtime: lambda.Runtime.NODEJS_14_X,

handler: `framework.${entrypoint}`,

timeout: FRAMEWORK_HANDLER_TIMEOUT,

logRetention: this.logRetention,

vpc: this.vpc,

vpcSubnets: this.vpcSubnets,

securityGroups: this.securityGroups,

role: this.role,

functionName: name,

});

Having a value that just gets ignored is not a pattern I think we should be perpetuating. It's unfortunate that we haven't been validating this already because adding it to the right place (in lambda) could potentially break someone's code. We should, however, have enough test coverage added so that we know the answer to if/how this will fail.

I agree. I've opened #21369 to at least have the conversation about throwing an Error. And maybe at least it could fall back to an Annotations.of(this).addWarning() either here or there. But I'll save the specific of that discussion for that PR.

Pull request has been modified.

laurelmay · 2022-07-28T16:58:29Z

I've added an example to the README. Thanks for the suggestion on location. I added an additional example to be able to talk more about the vpcSubnets and NAT requirements without over-complicating the existing example. But happy to merge them if that'd be better.

And as mentioned in replies to individual line comments, it seems like lambda.Function errs on the side of doing validation so that things mostly work as expected by requiring private subnets. I agree that some of the wording around the documentation isn't quite ideal.

Maybe there'd be value in adding a unit test to ensure that something like vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC } does result in an error during synthesis? Because these attributes are basically just directly passed to the SingletonFunction, I didn't really test that here because it seems to already be tested in

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

Lines 316 to 325 in 17f1ec8

    
             expect(() => { 
        
               new lambda.Function(stack, 'PublicLambda', { 
        
                 code: new lambda.InlineCode('foo'), 
        
                 handler: 'index.handler', 
        
                 runtime: lambda.Runtime.NODEJS_14_X, 
        
                 vpc, 
        
                 vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC }, 
        
               }); 
        
             }).toThrow(/Lambda Functions in a public subnet/); 
        
           });

TheRealAmazonKendra · 2022-07-28T18:50:14Z

I've added an example to the README. Thanks for the suggestion on location. I added an additional example to be able to talk more about the vpcSubnets and NAT requirements without over-complicating the existing example. But happy to merge them if that'd be better.

And as mentioned in replies to individual line comments, it seems like lambda.Function errs on the side of doing validation so that things mostly work as expected by requiring private subnets. I agree that some of the wording around the documentation isn't quite ideal.

Maybe there'd be value in adding a unit test to ensure that something like vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC } does result in an error during synthesis? Because these attributes are basically just directly passed to the SingletonFunction, I didn't really test that here because it seems to already be tested in

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

Lines 316 to 325 in 17f1ec8

expect(() => {

new lambda.Function(stack, 'PublicLambda', {

code: new lambda.InlineCode('foo'),

handler: 'index.handler',

runtime: lambda.Runtime.NODEJS_14_X,

vpc,

vpcSubnets: { subnetType: ec2.SubnetType.PUBLIC },

});

}).toThrow(/Lambda Functions in a public subnet/);

});

I commented on the comments above before seeing this one. I always err on the side of more tests so even though we have that test in lambda, I think we should add it here as well. Basically, for anything where we only think we know what the output will be, but we don't know for sure, should have a test case to ensure that output is what we expect.

I also acknowledge that this is a higher test coverage than we've probably asked for in the past, but our test coverage is something that I'd like to improve going forward.

TheRealAmazonKendra

As noted in in-line comments, please add some more test coverage to verify validation is working as expected and that it fails in the right ways.

Pull request has been modified.

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. This is motivated by the feedback received during review of aws#21357.

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

I always err on the side of more tests so even though we have that test in lambda, I think we should add it here as well. Basically, for anything where we only think we know what the output will be, but we don't know for sure, should have a test case to ensure that output is what we expect.

I also acknowledge that this is a higher test coverage than we've probably asked for in the past, but our test coverage is something that I'd like to improve going forward.

Yeah, I think that makes a lot of sense. So I've added tests for the following configurations:

Subnet Configuration	`vpcSubnets`	Expected Result
Default VPC config	Not specified	Works, selects all private/NAT subnets
Default VPC config	`{ subnetType: ec2.SubnetType.PRIVATE_NAT }`	Works, selects all private/NAT subnets
Default VPC config	`{ subnetType: ec2.SubnetType.PUBLIC }`	Lambda constructor throws error
Only public subnets	Not specified	Lambda constructor throws error
Only isolated subnets	Not specified	Works, selects all isolated subnets

These feel like most of the typical cases. It's not possible to construct a VPC with only PRIVATE_NAT (since NAT requires a PUBLIC, and that basically mimics the default config). I think it shows that if you'd accidentally put in Public subnets, it'll fail. And that if you want to filter, it works; and that usually it chooses the best viable configuration.

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. This is motivated by the feedback received during review of aws#21357.

TheRealAmazonKendra · 2022-07-29T19:21:25Z

So, I've been thinking about this and, after taking a look at the linked issue that was resolved, I feel unclear about what the value add here is. Given that I'm still relatively new, I've asked @rix0rrr to take a look at this.

laurelmay · 2022-07-30T15:01:29Z

So, I've been thinking about this and, after taking a look at the linked issue that was resolved, I feel unclear about what the value add here is. Given that I'm still relatively new, I've asked @rix0rrr to take a look at this.

Thanks for the review and the feedback that you have given so far! For clarification, if you mean the issue on cdklabs/cdk-nag, that was just about telling that tool how to ignore this problem. For compliance reasons, I'd still really like to be able to actually put AwsCustomResource into a VPC without needing to fallback to using Provider and writing my own handler (because this uses SingletonFunction, it's hard to use a regular override; I guess an Aspect may be option though...). This way private endpoints can be used, Flow Logs can be captured, and egress rules can be applied.

But in any case, AwsCustomResource is a really convenient tool for being able to make API calls as a custom resource. It would be even more helpful if it's interface more closely matched that of Provider and if I could use it in situations where compliance requirements mandate placing all resources in a VPC. If this PR isn't the best way to do that, I'm happy to open an issue instead to discuss other options. But because the AwsCustomResource doesn't expose the underlying SingletonFunction nor allow generically passing props to it, this felt like the simplest modification to make this easier for users with compliance requirements.

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: * [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*

laurelmay · 2022-08-05T01:34:19Z

I've also updated the tests now that #21369 has been merged to follow the pattern of expecting various error conditions.

laurelmay · 2022-08-08T05:04:34Z

Going to convert this to draft -- I accidentally checked in snapshots from a dry run and I don't think this can be properly implemented until #21348 is resolved.

This will allow for users with a need for all Lambda functions to be placed in a VPC to more easily adopt the use of `AwsCustomResource`. This is implemented by accepting a `vpc` and `vpcSubnets` property and passing those up to the created `SingletonFunction`. Then tests are added to ensure that it builds correctly and that errors are thrown in the expected circumstances (in the same situations that `lambda.Function` does).

laurelmay · 2022-08-23T02:29:30Z

Okay, I think this is ready for review again now that #21369 (and #21572) and #21495 have been merged that fix various related issues with this. Overall, the functionality remains the same. I ran into some issues with the merge after this so I rebased, which I know may make review of the incremental changes a little harder (but honestly whatever I did to the merge this time wouldn't have helped and it's been awhile since the last change on this branch anyway).

After the rebase, I re-ran the new integration test (without --dry-run this time) and it worked successfully. The existing integration test (without VPC) was unchanged but I ran that as well.

I've also updated the PR opening message with more clarification on the purpose for this change.

I'll wait for the build bot to come back with results (hopefully all green) before formally marking this as ready for review.

mergify · 2022-09-03T06:57:12Z

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

laurelmay · 2022-09-03T15:02:56Z

Result: FAILED

Oops! Looks like #21856. I'll re-run the new integration test.

This is due to changes in the default setup for integration tests, not anything changed in this package specifically.

Pull request has been modified.

mergify · 2022-09-03T19:18:07Z

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-09-03T19:56:04Z

AWS CodeBuild CI Report

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

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

mergify · 2022-09-03T19:56:22Z

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

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

aws-cdk-automation requested a review from a team July 28, 2022 11:54

laurelmay mentioned this pull request Jul 28, 2022

bug: Cannot ignore AwsCustomResource for NIST.800.53.R4-LambdaInsideVPC cdklabs/cdk-nag#959

Closed

TheRealAmazonKendra previously requested changes Jul 28, 2022

View reviewed changes

laurelmay mentioned this pull request Jul 29, 2022

fix(lambda): Function allows specifying vpcSubnets without vpc #21369

Merged

4 tasks

laurelmay mentioned this pull request Aug 8, 2022

(ec2): default Vpc structure results in broken networking during create/delete #21348

Closed

laurelmay marked this pull request as draft August 8, 2022 05:03

laurelmay force-pushed the custom-resource-vpc branch from 985d55d to 976bd50 Compare August 23, 2022 02:05

laurelmay marked this pull request as ready for review August 23, 2022 02:42

TheRealAmazonKendra previously approved these changes Sep 3, 2022

View reviewed changes

Merge branch 'main' into custom-resource-vpc

1affa80

Update integration tests

8517baf

This is due to changes in the default setup for integration tests, not anything changed in this package specifically.

TheRealAmazonKendra approved these changes Sep 3, 2022

View reviewed changes

Merge branch 'main' into custom-resource-vpc

ba4bd76

mergify bot merged commit 62d7bf8 into aws:main Sep 3, 2022

	for (const subnetId of subnetIds) {
	if (publicSubnetIds.has(subnetId) && !allowPublicSubnet) {
	throw new Error('Lambda Functions in a public subnet can NOT access the internet. ' +
	'If you are aware of this limitation and would still like to place the function in a public subnet, set `allowPublicSubnet` to true');
	}
	}

	const fn = new lambda.Function(this, `framework-${entrypoint}`, {
	code: lambda.Code.fromAsset(RUNTIME_HANDLER_PATH, {
	exclude: ['*.ts'],
	}),
	description: `AWS CDK resource provider framework - ${entrypoint} (${this.node.path})`.slice(0, 256),
	runtime: lambda.Runtime.NODEJS_14_X,
	handler: `framework.${entrypoint}`,
	timeout: FRAMEWORK_HANDLER_TIMEOUT,
	logRetention: this.logRetention,
	vpc: this.vpc,
	vpcSubnets: this.vpcSubnets,
	securityGroups: this.securityGroups,
	role: this.role,
	functionName: name,
	});

Conversation

laurelmay commented Jul 28, 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!

laurelmay commented Jul 28, 2022

Uh oh!

gitpod-io bot commented Jul 28, 2022

Uh oh!

TheRealAmazonKendra commented Jul 28, 2022

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!

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!

laurelmay commented Jul 28, 2022

Uh oh!

TheRealAmazonKendra commented Jul 28, 2022

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

laurelmay commented Jul 29, 2022

Uh oh!

TheRealAmazonKendra commented Jul 29, 2022

Uh oh!

laurelmay commented Jul 30, 2022

Uh oh!

laurelmay commented Aug 5, 2022

Uh oh!

laurelmay commented Aug 8, 2022

Uh oh!

laurelmay commented Aug 23, 2022

Uh oh!

mergify bot commented Sep 3, 2022

Uh oh!

laurelmay commented Sep 3, 2022

Uh oh!

mergify bot commented Sep 3, 2022

Uh oh!

aws-cdk-automation commented Sep 3, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Sep 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

laurelmay commented Jul 28, 2022 •

edited

Loading