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

[laurelmay]

This adds the ability to specify the VPC and Subnets to use for the Lambda function backing the AwsCustomResource. This is mostly identical to how these attributes are supported for Provider.

---

All Submissions:

• [X] 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

• [X] 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]

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[bot]]

[TheRealAmazonKendra]

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.

[laurelmay]

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 https://github.com/aws/aws-cdk/blob/17f1ec881ba8fb300bd4cf8674a87640ab05c31a/packages/%40aws-cdk/aws-lambda/test/vpc-lambda.test.ts#L316-L325

[TheRealAmazonKendra]

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

https://github.com/aws/aws-cdk/blob/17f1ec881ba8fb300bd4cf8674a87640ab05c31a/packages/%40aws-cdk/aws-lambda/test/vpc-lambda.test.ts#L316-L325

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.

[laurelmay]

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.

[TheRealAmazonKendra]

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]

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.

[laurelmay]

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

[laurelmay]

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.

[laurelmay]

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[bot]]

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]

• Result: FAILED

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

[mergify[bot]]

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]

AWS CodeBuild CI Report

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

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

[mergify[bot]]

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