Add optional VPC configuration for all Lambda functions

[alexcasalboni]

Implement #168

To do:

• [x] Update documentation
• [x] Proper testing

[tenjaa]

I am pretty sure the policy has to be AWSLambdaVPCAccessExecutionRole instead of AWSLambdaBasicExecutionRole when running in a vpc, doesn't it? But when trying it, it works perfectly fine... 🙈 Guess I need to reread that documentation.

So yes, looks good to me!

[alexcasalboni]

@tenjaa I'll dive deeper and run some more tests, but it sounds like you're correct :)

As for the documentation:

AWSLambdaVPCAccessExecutionRole grants permissions to manage ENIs within an Amazon VPC and write to CloudWatch Logs.

Here are the allowed actions:

[
  "logs:CreateLogGroup",    <<< also included in AWSLambdaBasicExecutionRole
  "logs:CreateLogStream",   <<< also included in AWSLambdaBasicExecutionRole
  "logs:PutLogEvents",      <<< also included in AWSLambdaBasicExecutionRole
  "ec2:CreateNetworkInterface",
  "ec2:DescribeNetworkInterfaces",
  "ec2:DeleteNetworkInterface",
   "ec2:AssignPrivateIpAddresses",
  "ec2:UnassignPrivateIpAddresses"
  ]

I believe my tests so far worked fine only because I was not interacting with anything network-related.

[tenjaa]

But will it ever interact with anything network related? It will just call AWS APIs via the VPC endpoints.

Do you need anything else from my side?

[ldcorentin]

If you guys could have a look at it please, I need this feature as well 🙏 @alexcasalboni

[alexcasalboni]

@tenjaa after a few tests I've realized that AWSLambdaVPCAccessExecutionRole is automatically attached to the Function's role when you include a VPCConfig. That's why everything worked fine.

This doesn't seem to be documented anywhere and I couldn't find any reference to it even in the SAM CLI implementation. So it could be some new magic done by CloudFormation to avoid random IAM issues when deploying Lambda functions to a VPC.

I'd like to dive deeper and figure out if we can count on this behavior, before merging this PR.

Anyways, if we want to be hypercautious we could just add this to every function:

Policies:
  - AWSLambdaBasicExecutionRole # Only logs
  - !If [UseVPCConfig, VPCAccessPolicy: {}, !Ref AWS::NoValue]

[alexcasalboni]

@ldcorentin do you need this in both SAM/CloudFormation and Terraform?

This PR is only updating the CloudFormation template. We'll need to implement the same logic in Terraform.

[tenjaa]

@alexcasalboni I gave some feedback to the AWS docs team. Let's see what they say, I heard they are usually very quick to reply!

Anyways, if we want to be hypercautious we could just add this to every function

I wouldn't add it. Even though it might not be documented (yet) I don't see this feature getting removed. Too many people rely on it. Even here in this issue we did initially :D

Also for me only CloudFormation would be enough at the moment.

[ldcorentin]

@alexcasalboni I would only need it for Terraform me :D

[ldcorentin]

I will add the Terraform code

[alexcasalboni]

@tenjaa I've found it: https://github.com/aws/serverless-application-model/blob/develop/samtranslator/model/sam_resources.py#L599

I can confirm that SAM automatically includes AWSLambdaVPCAccessExecutionRole when a VPC Config is provided :) I'll double-check the PR and try to merge it later this week.

[alexcasalboni]

Ok, this works fine when you provide securityGroupIds or subnetIds.

Unfortunately, the deployment fails if you provide none (which is the most common case):

Parameter validation failed: parameter value for parameter name securityGroupIds does not exist,
parameter value for parameter name subnetIds does not exist. Rollback requested by user.

It's possible that I've messed something up in the UseVPCConfig condition, but I can't see what exactly. Will need to do some debugging :)

[darioackermann]

Hey! Thanks for your work so far. I wanted to ask if you found something in the mean-time? This feature would be extremly useful for us.

[alexcasalboni]

@darioackermann thanks for the reminder :) I didn't find the time to debug this in the last few weeks - will try harder later this week!

[alexcasalboni]

@darioackermann @tenjaa apologies for the delay in merging this 🙏

AWS re:Invent is coming soon and I'm planning to merge all open PRs between Dec 12th and Dec 22nd 🎄

[dcai-icfi]

@alexcasalboni any updates on this? Thanks!

[alexcasalboni]

@dcai-icfi @darioackermann @tenjaa I gave this PR another try today.

Good news: I managed to find a workaround for the default case where no VPC Config is need. Unfortunately we can't use the AWS-specific types for this, as they can't be optional. So I ended up using CommaDelimitedList instead. This is not ideal if you deploy via SAR or Cfn Console because you don't get autocompletion, but it's better than nothing.

It took me a while to test end-to-end because of all the VPC configuration required to enable API calls to the Lambda service. But I can confirm that the VPC Config is successfully applied to all functions and it should work fine as long as your VPC is set up correctly.

I just want to make sure we're all ok with the current implementation giving for granted that you've already configured your VPC correctly (Security Groups, Subnets, Route Tables, NAT Gateway or VPC Endpoint).

[alexcasalboni]

I'm going to merge this on Monday (3/20), if no comment/concern comes up :)

[alexcasalboni]

Sorry for the commit spam :) I really wanted to implement GHA integration tests before moving forward with this.

For now it only works on the current branch, but I'm going to build more accurate integration tests to make sure all these new deployment parameters work without regressions.

Will merge this PR now 🚀

[tenjaa]

A long vacation made me miss the merge :(

But better late than no feedback: It works perfectly for us! Thanks a lot!

[alexcasalboni]

FYI this is now published on SAR as version 4.3.1 👌