Skip to content
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

Merged
merged 47 commits into from Apr 28, 2021

Conversation

michaelbrewer
Copy link
Contributor

@michaelbrewer michaelbrewer commented Mar 28, 2021

Issue #, if available:
#325

Description of changes:

Very basic / minimal http proxy event handler (ALB Events, Http api V1 and V2)

Changes:

  • feat(data-classes):Add path and http_method methods to BaseProxyEvent
  • feat(data-classes): Add json_body helper property to BaseProxyEvent that json parses body
  • docs: Add example ApiGatewayResolver event handler
  • tests: Add test coverage for ApiGatewayResolver
  • tests: Add shared test utils.load_event
  • feat(context): Add current_event and lambda_context
  • feat(decorators): Add shortcut decorators for get, post, patch, put, delete
  • feat(cors): Add basic CORS flag
  • feat(compress): Add gzip compression flag
  • feat(binary): Add isBase64Encoded support for binary content
  • feat(cache-control): Add cache_control option, which defaults to no-cache for non-200 responses
  • feat(rest): Add simplification for functions just returning a dict
  • feat(rest): Use shared JSONEncoder that can handle encoding Decimal
  • feat(response): Add Response class for fine grained control of the http headers returned.
  • feat(cors): Add CORSConfig class for higher control over access control headers
  • feat(cors): Built in preflight support

Features:

  • Routes - @app.get("/foo")
  • Path expressions - @app.delete("/delete/<uid>")
  • Cors - @app.post("/make_foo", cors=True) or via CORSConfig
  • Base64 encode binary - @app.get("/logo.png")
  • Gzip Compression - @app.get("/large-json", compression=True)
  • Cache-control - @app.get("/foo", cache_control="max-age=600")
  • Rest API simplification with function returns a Dict
  • Support function returns a Reponse object which give fine grained control of the headers
  • JSON encoding of Decimals
import json
from typing import Dict, Tuple

from aws_lambda_powertools.utilities.data_classes import APIGatewayProxyEvent
from aws_lambda_powertools.utilities.event_handler.api_gateway import ApiGatewayResolver, CORSConfig, ProxyEventType, Response

# proxy_type: "APIGatewayProxyEvent", "APIGatewayProxyEventV2", "ALBEvent"
app = ApiGatewayResolver(
    proxy_type=ProxyEventType.http_api_v1,
    cors=CORSConfig(
        allow_origin="https://www.foo.com/",  # Whitelisted site
        expose_headers=["foo1"],
        allow_headers=["x-custom-header"],
        max_age=100,
        allow_credentials=True,
    )
)

@app.get("/foo", compress=True)
def get_foo() -> Tuple[int, str, str]:
    # Matches on http GET and proxy path "/foo"
    return 200, "text/html", "Hello"

@app.get("/logo.png")
def get_logo() -> Tuple[int, str, bytes]:
    # Base64 encode 
    logo: bytes = load_logo()
    return 200, "image/png", logo

@app.post("/make_foo", cors=True)
def make_foo() -> Tuple[int, str, str]:
    # Matches on http POST and proxy path "/make_foo"
    post_data: dict = app. current_event.json_body
    return 200, "application/json", json.dumps(post_data["value"])

@app.delete("/delete/<uid>")
def delete_foo(uid: str) -> Tuple[int, str, str]:
    # Matches on http DELETE and proxy path starting with "/delete/"
    assert isinstance(app.current_event, APIGatewayProxyEvent)
    assert app.current_event.request_context.authorizer.claims is not None
    assert app.current_event.request_context.authorizer.claims["username"] == "Mike"
    return 200, "application/json", json.dumps({"id": uid})

@app.get("/hello/<username>")
def hello_user(username: str) -> Tuple[int, str, str]:
    return 200, "text/html", f"Hello {username}!"

@app.get("/rest")
def rest_fun() -> Dict:
    # Returns a statusCode: 200, Content-Type: application/json and json.dumps dict
    return {"message": "Example"}

@app.get(“/foo3”)
def foo3() -> Response:
    return Response(
        status_code=200,
        content_type="application/json",
        body=json.dumps({“message”: “Foo3”}),
        headers={“custom-header”: “value”}, 
    )

Unlikely bonus features:

  • Brotli Compression - via an extra install option
  • OpenAPI support - to generate swagger docs
  • Rendering of html templates - to allow for html templates using Python 3 string templates or Jinja (extra deps)
  • Multiple method mapping for a single func ie: @app.route(methods=["GET", "POST"],...
  • Build in idempotency support

Checklist

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-io
Copy link

codecov-io commented Mar 28, 2021

Codecov Report

Merging #369 (85b5ff8) into develop (0edec8e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           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           
Impacted Files Coverage Δ
...bda_powertools/utilities/data_classes/alb_event.py 100.00% <ø> (ø)
aws_lambda_powertools/event_handler/api_gateway.py 100.00% <100.00%> (ø)
.../utilities/data_classes/api_gateway_proxy_event.py 100.00% <100.00%> (ø)
...lambda_powertools/utilities/data_classes/common.py 100.00% <100.00%> (ø)

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 9be19b2...85b5ff8. Read the comment docs.

@michaelbrewer
Copy link
Contributor Author

@heitorlessa an ultralight handler for http proxy lambdas

@michaelbrewer
Copy link
Contributor Author

@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

@michaelbrewer
Copy link
Contributor Author

@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)

@michaelbrewer michaelbrewer changed the title feat(event-handler): Add http ProxyEvent handler feat(event-handler): add http ProxyEvent handler Apr 26, 2021
@heitorlessa heitorlessa self-assigned this Apr 26, 2021
Copy link
Contributor

@heitorlessa heitorlessa left a 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
@heitorlessa
Copy link
Contributor

Cc @marcioemiranda your feedback on this would be great before we merge and release this week.

What are your thoughts on CORS UX?

@michaelbrewer
Copy link
Contributor Author

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.

Copy link
Contributor

@pcolazurdo pcolazurdo left a 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

@michaelbrewer
Copy link
Contributor Author

Adding some comments about CORS best practices

Sure we can add some of these to the docs. I was aiming to all for an cors=True shortcut like in Chalice (and other frameworks), they got you up and running quickly. For those that want full control, they should use CORSConfig.

Copy link
Contributor

@heitorlessa heitorlessa left a 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

@heitorlessa
Copy link
Contributor

heitorlessa commented Apr 27, 2021

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:

  • Set CORS once and toggle cors=True as on/off. Perhaps, a single method to add default headers that might be needed over and over, in which CORS could also be added works too.
    • Since CORS would have to be defined via a method, you could leave cors=True as the default per method, if it's not set then it wouldn't have any effect, and when it is you have one less step to do.
  • Remove preflight (OPTIONS) since that will have to happen at the API Gateway/ALB resource level not at the integration
  • [Optional] Docstrings are missing for Resolvers and CORS. These can be added in the next PR concerning docs too.

I'm approving now so I can either merge or wait for these two last changes.

@michaelbrewer
Copy link
Contributor Author

@heitorlessa I made the code changes. Just the docstrings need to be added.

@marcioemiranda
Copy link

Cc @marcioemiranda your feedback on this would be great before we merge and release this week.

What are your thoughts on CORS UX?

@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.
@michaelbrewer great job on this feature. I will want to use it asap.

Anyways, my initial thought was having something like this @app.post("/make_foo", cors=True).
However, it's likely that one will enable cors for all methods, so the centralized config makes sense.

I set the following headers in my functions for all responses, both success and error:
headers = {
"Access-Control-Allow-Origin": "*",
"Access-Control-Expose-Headers": "x-amzn-ErrorType"
}

Some additional comments:

  • Does it make sense to assume a default 200 | application/json response and assume the returned value is the response model? I would say this would represent the majority of the usage, so writing the tuple for every method seemed cumbersome.
  • Looking at one of the examples I wonder if it would make sense to have an additional decorator for authorization.

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.

@heitorlessa
Copy link
Contributor

heitorlessa commented Apr 28, 2021

Alright... tested on CORS, here's the confusion clarification at least on my part:

  1. CORS Pre-flight can be handled by API GW at the resource level by using a MOCK response on OPTIONS method. You can also forward to a Lambda function, if you'd like to handle it (/*, ANY).
  2. When running locally, pre-flight requests might be handled by a local emulator like SAM CLI if CORS configuration is configured - This can be tricky as local endpoints will be different thus having people changing to * that can lead to security issues in prod

Verdict: We must handle CORS pre-flight requests because a) people might configure ANY method and /* to a single Lambda (not recommended but can happen), and b) running locally can be error prone with CORS so we should handle that to improve the experience (why this lib exist).

API GW handling pre-flight CORS response at the resource level, when you don't setup ANY method

image

image

@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

Copy link
Contributor

@heitorlessa heitorlessa left a 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 !

@heitorlessa heitorlessa merged commit e24a985 into aws-powertools:develop Apr 28, 2021
@michaelbrewer michaelbrewer deleted the feat-event-handler-apigw branch April 28, 2021 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants