feat: Resource policy Iam, Vpc and Ip whitelist/blacklist support #1077
Conversation
|
Some things that are still needed to be done -
|
There was a problem hiding this comment.
Left an initial review. Like you said, this is of a much larger scope than we originally thought. Some comments:
Methods:
- Could you update/add docstrings in some of the methods to explain more clearly what they do?
Tests:
- Could you add a test or tests with multiple resource policies inside of them? I would like to see some common patterns, like pairing
IpDenyListandIpAllowListtogether. - Could you add a test that has multiple paths inside of it that all create resource policies for just those paths? Also use the
ANYmethod and path parameters ({id}{proxy+}) - Could you add a test that adds CloudFormation conditions into the paths? I talked about that in one of my comments, let me know if you'd like a sample template or more clarification about that.
samtranslator/swagger/swagger.py
Outdated
There was a problem hiding this comment.
Will need to look more into this- what if there's a CFN condition on one of the paths? Try adding a test that has a condition on a function with an api event and another function without a condition that also has an api event.
There was a problem hiding this comment.
I would need a sample template for this.
There was a problem hiding this comment.
Add to the top of the template:
Conditions:
C1:
Fn::Equals:
- true
- trueThen on any function, add under the logical ID:
Condition: C1There was a problem hiding this comment.
On adding conditions support, I noticed that though it worked when the ResourcePolicy property was defined on the function event itself, the behavior was different when defined in Globals. Given the inconsistency, I talked to @keetonian in person, and confirmed that APIGW does not validate the paths exist, so adding a non existent path will not make APIGW throw an error. Given this, my preferred approach is to proceed without condition support on ResourcePolicy for now. Adding it will require exposing the Function's condition property in Globals and API plugins.
|
@keetonian @jlhood this is ready for your review again. Tried to address most of the comments the best I could! |
Codecov Report
@@ Coverage Diff @@
## develop #1077 +/- ##
===========================================
- Coverage 94.8% 94.36% -0.45%
===========================================
Files 71 71
Lines 3486 3621 +135
Branches 680 713 +33
===========================================
+ Hits 3305 3417 +112
- Misses 93 104 +11
- Partials 88 100 +12
Continue to review full report at Codecov.
|
|
|
||
| # Api Event sources must "always" be paired with a Serverless::Api | ||
| 'RestApiId': PropertyType(True, is_str()), | ||
| 'Stage': PropertyType(False, is_str()), |
There was a problem hiding this comment.
Is this the only way to get the Stage here? This surfaces the Stage parameter to the template author, they can then override this value to be whatever they want.
There was a problem hiding this comment.
We don't modify the stage property here. Is there a better way?
There was a problem hiding this comment.
Maybe make it a private variable here instead of one of these variables?
There was a problem hiding this comment.
Just tested that adding it on the function event does not override the stage name.
4 participants
Issue #, if available:
#514
Description of changes:
Adding support in SAM to create resource policies for IAM/IP/VPC whitelist/blacklist.
Description of how you validated changes:
still WIP
Checklist:
make prpassesexamples/2016-10-31By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.