Add VPC support to lambda functions #837

jamesls · 2018-05-15T17:27:21Z

This adds VPC support to lambda functions in chalice. It pulls in #673 and fleshes out the rest of the deployer support for VPC. Notably it makes a few changes:

The VPC configuration can be added per lambda function and per stage.
The role management is now moved into the deployer and based on the models. In order to do this I had to add a traits flag to the AutogenPolicy which flags which policies it needs to add.
Update the awsclient to handle removal of VPC configuration (that is, if you configure a function for VPC and then remove it from your config.json, it will appropriately remove VPC configuration).
Added a few more examples and docs on how to configure VPC.

This PR also triggered a few linter warnings so I cleaned up some of the code (as a separate commit). There was a whole class (with tests) that was no longer used. It was a part of the legacy deployer.

Thanks to @lime-green for the original PR.

Had to do some minor refactoring to get things passing.

This was an artifact of the old deployer.

codecov-io · 2018-05-15T17:31:55Z

Codecov Report

Merging #837 into master will increase coverage by 0.01%.
The diff coverage is 98.61%.

@@            Coverage Diff            @@
##           master    #837      +/-   ##
=========================================
+ Coverage   94.79%   94.8%   +0.01%     
=========================================
  Files          21      21              
  Lines        3707    3735      +28     
  Branches      478     486       +8     
=========================================
+ Hits         3514    3541      +27     
  Misses        130     130              
- Partials       63      64       +1

Impacted Files	Coverage Δ
chalice/app.py	`96.86% <ø> (ø)`	⬆️
chalice/deploy/planner.py	`94.01% <ø> (ø)`	⬆️
chalice/deploy/deployer.py	`98.48% <100%> (-0.04%)`	⬇️
chalice/package.py	`100% <100%> (ø)`	⬆️
chalice/deploy/models.py	`99.15% <100%> (+0.02%)`	⬆️
chalice/policy.py	`92.1% <100%> (+0.21%)`	⬆️
chalice/constants.py	`100% <100%> (ø)`	⬆️
chalice/config.py	`97.98% <100%> (+0.05%)`	⬆️
chalice/awsclient.py	`92.14% <95.83%> (+0.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46cbb73...30ba203. Read the comment docs.

stealthycoin

Looks pretty good to me, just a few questions.

stealthycoin · 2018-05-15T17:36:48Z

CHANGELOG.rst

@@ -11,6 +11,7 @@ CHANGELOG
 * Update ``policies.json`` file
  (`#817 <https://github.com/aws/chalice/issues/817>`__)



Needs a changelog entry

stealthycoin · 2018-05-15T18:15:20Z

docs/source/topics/configfile.rst

+will be applied to that function.  These are the configuration options
+that can be applied per function:
+
+* ``iam_policy_file``


Might be nice to have their default values listed somewhere if they are omitted.

I added a reference back to the stage config, these are described in the section above.

stealthycoin · 2018-05-15T18:45:55Z

chalice/deploy/deployer.py

-            resource.document = self._policy_gen.generate_policy(config)
+            policy = self._policy_gen.generate_policy(config)
+            if models.RoleTraits.VPC_NEEDED in resource.traits:
+                policy['Statement'].append(VPC_ATTACH_POLICY)


What do you think about replacing the * in VPC_ATTACH_POLICY resources section with all the specific ARNs from the config?

They're different ARNs. The config ARNs are for the subnet/sg ids, but the VPC policy is to CRUD ENIs.

jamesls · 2018-05-15T20:51:43Z

@stealthycoin Feedback incorporated.

kyleknap

Looks good. A couple of things that I ran into while playing around with this. I may have some more...

We should add validation that both subnet_ids and security_group_ids are present in the config. Otherwise, chalice will it ignore it and deploy the lambda function without letting the user know about the issue.
Trying it out I think there is an eventual consistency issue where: if you deploy the lambda function once (with no VPC configured), and then again (waiting a minute or so) it will error out with the following error:

ChaliceDeploymentError: ERROR - While deploying your chalice application, received the following error:

 An error occurred (InvalidParameterValueException) when calling the 
 UpdateFunctionConfiguration operation: The provided execution role does not 
 have permissions to call CreateNetworkInterface on EC2

However running deploy again succeeds.

jamesls · 2018-05-16T01:18:19Z

So (1) ends up being a little tricker than I thought and there's a number of edge cases. I was initially adding validation to the chalice.deploy.validate module. It seemed better to fast fail before we even make it to the deployer. However, it didn't seem like the right thing to do. For example, what about this:

        {
            'app_name': 'myapp',
            'security_group_ids': ['sg-1'],
            'stages': {
                'dev': {
                    'lambda_functions': {
                        'foo': {'subnet_ids': ['sn-1']},
                        'bar': {'subnet_ids': ['sn-1']},
                    }
                },
                "prod": {},
            }
        }

Deploying to dev is fine, but the prod stage would "fail" because there's no subnet_ids defined. But it seems wrong to fail config validation because this is only an issue if you deploy to prod. The dev stage is actually fine so I would expect chalice deploy --stage dev to just work. Also, what if I had:

        {
            'app_name': 'myapp',
            'security_group_ids': ['sg-1'],
            'stages': {
                'dev': {
                    'lambda_functions': {
                        'foo': {'subnet_ids': ['sn-1']},
                        'bar': {},
                    }
                }
            }
        }

but my actual app only has a foo function:

@app.lambda_function()
def foo(...): pass

It doesn't seem like we should say the config is invalid because you chalice deploy will actually do what we want: deploy the lambda function in a VPC.

I think what I'm going to do is just make this a deployer error. If we try to create a models.LambdaFunction and we're missing a subnet id or a sg then we'll fail. The error messages won't be as good but I don't want to fail validation when it doesn't actually matter.

What do you think?

This was initially done in chalice.deploy.validate, but it produces too many false positives. This way, we only fail if we try to construct a resource with invalid params. This does mean we might have latent issues that we'll only find about when we try to deploy our app, but that seems better than failing validation when it wouldn't actually affect a deploy.

jamesls · 2018-05-16T01:47:20Z

@kyleknap feedback incorporated. I ended up just adding the validation in the AppGraphBuilder. That way you can catch the error with chalice package and you don't actually have to do a deploy. The error message you get is:

ChaliceBuildError: Invalid VPC params for function 'foo', in order to configure VPC for a Lambda function, you must provide the subnet_ids as well as the security_group_ids, got subnet_ids: [u'subnet-1234', u'subnet-2345'], security_group_ids: []

kyleknap

Looks good. Just had a small comment and a follow up question. So if the validation errors were in chalice.deploy.validate instead, what specific features would you get these false positives? Like is it related to getting an error when running chalice local for these misconfigured stages/functions?

kyleknap · 2018-05-16T17:48:53Z

chalice/deploy/deployer.py

@@ -559,9 +581,10 @@ def _build_lambda_function(self,
            config.app_name, config.chalice_stage, name)
        security_group_ids = config.security_group_ids
        subnet_ids = config.subnet_ids
-        if security_group_ids is None or subnet_ids is None:
+        if security_group_ids is None and subnet_ids is None:


Is this if statement still needed? It looks like it gets handled in _get_vpc_params and is not necessarily needed.

jamesls · 2018-05-16T19:13:48Z

@kyleknap The two examples I gave in my previous comments give you false positives. One was for an invalid stage that you're not deploying to and one was config for a non-existent function (defined in config but not in app). By false positive, I mean the "config validation" would fail, but if you actually were to somehow turn off validation and just let chalice deploy (or whatever command) go through it would actually deploy correctly.
This happens anytime the config is loaded too which is just about every command, local, package, deploy, etc.

kyleknap

Ahh that makes more sense. Besides that I had a comment that looks like it got hidden so I posted it again. Otherwise, 🚢

kyleknap · 2018-05-16T19:35:55Z

chalice/deploy/deployer.py

@@ -555,7 +579,13 @@ def _build_lambda_function(self,
        # type: (...) -> models.LambdaFunction
        function_name = '%s-%s-%s' % (
            config.app_name, config.chalice_stage, name)
-        return models.LambdaFunction(
+        security_group_ids = config.security_group_ids


Mentioned this before but I think we can remove lines 582-586 because they are handled in the _get_vpc_params() method.

This is fixed now.

kyleknap · 2018-05-16T19:39:20Z

Thanks. 🚢 again

PR #837 * lime-green-vpc-support: Remove unused ifcheck, not needed Retry lambda errors related to VPC permissions Raise validation error on invalid VPC params Extract validate tests into separate module Add a reference back to the stage config in the lambda config Add feature to changelog Update docs with more config examples Remove unused ApplicationPolicyHandler Flesh out the rest of the deployer Security groups vary per function Put back original linter values Add VPC support

Joshua Doncaster-Marsiglio and others added 6 commits May 14, 2018 19:17

Add VPC support

21286dc

Put back original linter values

bfe2ede

Had to do some minor refactoring to get things passing.

Security groups vary per function

43e68de

Flesh out the rest of the deployer

960d9a1

Remove unused ApplicationPolicyHandler

f676492

This was an artifact of the old deployer.

Update docs with more config examples

e588e4d

jamesls requested review from stealthycoin and kyleknap May 15, 2018 17:27

stealthycoin reviewed May 15, 2018

View reviewed changes

jamesls added 2 commits May 15, 2018 13:49

Add feature to changelog

3faa8de

Add a reference back to the stage config in the lambda config

a7cd61e

jamesls force-pushed the lime-green-vpc-support branch from 6d6cdab to a7cd61e Compare May 15, 2018 20:50

stealthycoin approved these changes May 15, 2018

View reviewed changes

kyleknap reviewed May 15, 2018

View reviewed changes

Extract validate tests into separate module

5e24f5e

jamesls added 2 commits May 15, 2018 18:34

Retry lambda errors related to VPC permissions

3c1f3c7

kyleknap reviewed May 16, 2018

View reviewed changes

kyleknap approved these changes May 16, 2018

View reviewed changes

Remove unused ifcheck, not needed

30ba203

jamesls merged commit 30ba203 into aws:master May 17, 2018

This was referenced May 17, 2018

Vpc support #673

Closed

Add support for VPC, SubNet, and Security Groups to Configuration #413

Closed

yahavb mentioned this pull request Aug 23, 2018

Configuring VPC and SGs in Chalice Deployment #939

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VPC support to lambda functions #837

Add VPC support to lambda functions #837

jamesls commented May 15, 2018

codecov-io commented May 15, 2018 •

edited

Loading

stealthycoin left a comment

stealthycoin May 15, 2018

stealthycoin May 15, 2018

jamesls May 15, 2018

stealthycoin May 15, 2018

jamesls May 15, 2018

jamesls commented May 15, 2018

kyleknap left a comment

jamesls commented May 16, 2018

jamesls commented May 16, 2018

kyleknap left a comment

kyleknap May 16, 2018

jamesls commented May 16, 2018

kyleknap left a comment

kyleknap May 16, 2018

jamesls May 16, 2018

kyleknap commented May 16, 2018

		@@ -11,6 +11,7 @@ CHANGELOG
		* Update ``policies.json`` file
		(`#817 <https://github.com/aws/chalice/issues/817>`__)

Add VPC support to lambda functions #837

Add VPC support to lambda functions #837

Conversation

jamesls commented May 15, 2018

codecov-io commented May 15, 2018 • edited Loading

Codecov Report

stealthycoin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls commented May 15, 2018

kyleknap left a comment

Choose a reason for hiding this comment

jamesls commented May 16, 2018

jamesls commented May 16, 2018

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls commented May 16, 2018

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap commented May 16, 2018

codecov-io commented May 15, 2018 •

edited

Loading