-
Notifications
You must be signed in to change notification settings - Fork 403
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(event-handler): add http ProxyEvent handler #369
feat(event-handler): add http ProxyEvent handler #369
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #369 +/- ##
========================================
Coverage 99.94% 99.94%
========================================
Files 98 99 +1
Lines 3688 3783 +95
Branches 174 184 +10
========================================
+ Hits 3686 3781 +95
Partials 2 2
Continue to review full report at Codecov.
|
@heitorlessa an ultralight handler for http proxy lambdas |
@heitorlessa maybe i can do the same for AppSync handler and include the current event in the “app” rather than a method parameter (which might run into shadowing issues0 |
@heitorlessa - as an example i added CORS support, the only things i would need regularly would be compression (and therefore also binay support via bas64encoding) |
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.
Looking great and slim ;) Loved it! Added some comments on CORS not being complete, and whether we want to take into account ALB tight limits like 1MB response etc
NOTE: Some of this is based on the cors behavior of Chalice, except where we actually return the preflight response
Cc @marcioemiranda your feedback on this would be great before we merge and release this week. What are your thoughts on CORS UX? |
Outstanding questions i have are around exceptions. Should we allow for it to ripple up, or should we always handle them and return an appropriate http response code? And in the case of CORS, also include the relavant http headers. |
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.
Adding some comments about CORS best practices
Sure we can add some of these to the docs. I was aiming to all for an |
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.
Two small CORS changes to ensure minimum security and for package not to be later flagged for insecure practices.
These are making allow origin a required argument, and credentials set to False by default.
It's not the easiest UX but a balance we must meet
Let me know whether you want me to merge and make these changes in a separate PR, or if you'd like to make them now so we can merge it clean:
I'm approving now so I can either merge or wait for these two last changes. |
@heitorlessa I made the code changes. Just the docstrings need to be added. |
@heitorlessa yesterday I posted a feature request that is totally related to this one (Sorry, I haven't reviewed the current Pull Requests, just the issues). Please review if any of that makes sense here. Anyways, my initial thought was having something like this @app.post("/make_foo", cors=True). I set the following headers in my functions for all responses, both success and error: Some additional comments:
Regarding exceptions I would say always handle them and map to proper status codes. Expose the exceptions to developers so that they can use them in the code. For example, a ResourceNotFoundError could map to status code 404. A ServiceUnavailableError, triggered when a downstream service is down, could be mapped to another status code. The CORS headers should be included in the error responses as well. |
Alright... tested on CORS, here's the confusion clarification at least on my part:
Verdict: We must handle CORS pre-flight requests because a) people might configure ANY method and API GW handling pre-flight CORS response at the resource level, when you don't setup ANY method @michaelbrewer could you add pre-flight CORS back and I'll merge it, please? It'll be easier to revert that as you have it locally than me creating another PR just to add that back in. I'll add the docstrings, examples, and the official docs in the next PR. Thanks a lot everyone for the help here |
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.
Merging this now so we can start the docs in a separate PR - As agreed, we'll release this as Beta by Friday EOD, so we can iterate on UX.
I always find that as soon as we write docs we find some good areas to improve, then more customers using it they can give us feedback on areas to iron out before we take a 1-way door decision.
Thanks a lot again Michael, and everyone, for the help on getting this out !
Issue #, if available:
#325
Description of changes:
Very basic / minimal http proxy event handler (ALB Events, Http api V1 and V2)
Changes:
path
andhttp_method
methods toBaseProxyEvent
json_body
helper property toBaseProxyEvent
that json parsesbody
ApiGatewayResolver
event handlerApiGatewayResolver
utils
.load_event
current_event
andlambda_context
get
,post
,patch
,put
,delete
isBase64Encoded
support for binary contentcache_control
option, which defaults tono-cache
for non-200 responsesDecimal
Features:
@app.get("/foo")
@app.delete("/delete/<uid>")
@app.post("/make_foo", cors=True)
or via CORSConfig@app.get("/logo.png")
@app.get("/large-json", compression=True)
@app.get("/foo", cache_control="max-age=600")
Reponse
object which give fine grained control of the headersUnlikely bonus features:
@app.route(methods=["GET", "POST"],...
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.