Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(aws-lambda): allow placing Lambda in VPC #598
feat(aws-lambda): allow placing Lambda in VPC #598
Changes from 9 commits
6ba0614
cfefb7e
e807777
d4ef24c
5f9e0f6
66111d7
d078fec
66900d3
5cf128f
37bd186
9fe130f
d11ffb0
305da53
30f78dc
4eb363a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow I am bummed out by the fact this seems to expect anything to know up-front whether a function is in-VPC or not... At lease should provide a property to verify this (
public get isVpcBound(): boolean
or something?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't you know since you were building the model yourself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message should tell the user how to fix the problem? (i.e. how to add the lambda to a VPC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rational of exporting the security group ID of the Lambda? Is it in order to be able to use connections on an imported lambda? Maybe worth explaining in the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"all private subnets"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh? I thought Lambdas could only be in private subnets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would that be? I mean, it might be true, but since it's just an ENI I'm guessing you can place it anywhere.
How would such a restriction even work? If the routing table points to an IGW instead of an NGW, the Lambda will not work?
We also still have isolated subnets that need to be selectable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This blog post seems to imply you need to do something different for public and private subnets, thereby implying it could be in a public subnet: https://aws.amazon.com/premiumsupport/knowledge-center/internet-access-lambda-function/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe unsolicited feedback but I've been browsing the PRs to learn some CDK aspects..
While Lambda can technically be in a public subnet, invocations will fail if the function attempts to reach any publicly routable IP (ie 0.0.0.0 -> VPC internet gateway).
Therefore, our recommendation is to always add functions in a Private Subnet with NAT Gateway should they need to reach any publicly routable IP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification @heitorlessa. I guess I was wrong.
That still leaves the opportunity to have
Private
(NAT routed) andIsolated
(nonrouted) subnets, so we still need a placement argument, but now complicated by the fact that selecting public subnets is an error, and we might not know a priori what the selection is going to match.For the moment, I'll propose to fix this with docs. Everyone okay with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it with a post-selection check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be a bit more descriptive here: "If the function is placed within a VPC and a security group is not specified, a dedicated security group will be created for this function"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
role.addManagedPolicy('service-role/xxx')
would be nicer?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but you can have your own managed policies that you can attach, and they'll have a different account ('12345' vs 'aws'). Let's use ARNs to be safe.
Ultimately, this should be just "the object", not "the ARN", but I just wanted to make this copy/pasted code slighly better, not introduce a new modeling of ManagedPolicies as part of this CR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to
renderVpcConfig
or something like thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's private, and it does more than just render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So maybe
configureVpc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention what are the legal options (private/internal) and also why is this not possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't mention Internal because in this PR support for selecting internal subnets hasn't landed yet.
Also I'm told that my understanding of this is incomplete (by someone from the Lambda team), AND they're changing it early next year, AND we never explicate the reason on any other of our public documentation pages, we just say "no public for you". I don't want us to be the single source of truth for a factoid that's going to become wrong in a short while.