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): improved support for headers and cookies in v2 #1455

Merged

Conversation

rubenfonseca
Copy link
Contributor

@rubenfonseca rubenfonseca commented Aug 15, 2022

Issue number: #1192, #1450

Summary

Changes

Please provide a summary of what's being changed

This change adds support for transparently serialize headers and cookies when using the Response object. Powertools will pick the correct payload format depending on the type of input event, and serialize headers and cookies accordingly.

carbon (11)

User experience

Please share what the user experience looks like before and after this change

Before this change, setting cookies was done manually:

Response(..., headers={"Set-Cookie": "SSID=12345"})

This had multiple problems:

  • no out-of-the-box support for setting multiple headers or cookies
  • no support for multiple cookies when using the API Gateway REST integration and the ALB integration, since they require the use of a multiValueHeaders key in the response payload.

After this change, users can set headers and cookies freely (single or multiple), and Powertools will serialize them correctly according to the type of input event:

Response(...,
  headers={
    "Content-Type": ["application/json"],
    "X-Transaction_Id": ["<uuid>"]
  },
  cookies=["<cookie-name>=<cookie-value>; Secure; Expires=<date>"]
)

Existing code will still work because we model header values as a union Union[str, List[str]].

API Gateway HTTP API integration output:

carbon (9)

ALB Integration output:

carbon (10)

This improves the usability and hides the implementation differences across the different integrations.

Caveat

There is a scenario where using the default ALB integration configuration (multiValueHeaders are disabled) while setting multiple cookies would fail. When we detect that scenario, a warning is printed and the last cookie/header is set.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

Yes, two breaking changes are introduced:

  • the type of the headers parameter in the Response object changed from Dict to the more explicit Dict[str, List[str]]
  • the output of the serialized response changes according to the input event type. This means that on some integrations (HTTP API, Lambda Function URL) we will see headers and cookies on the payload. But on other integrations (REST API, ALB) we will see multiValueHeaders with a Dict[str, List[str]] type. This could break existing customer tests.

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.


View rendered docs/core/event_handler/api_gateway.md

@boring-cyborg boring-cyborg bot added area/event_handlers documentation Improvements or additions to documentation tests labels Aug 15, 2022
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2022
@github-actions github-actions bot added the feature New feature or functionality label Aug 15, 2022
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 15, 2022
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 16, 2022
@rubenfonseca rubenfonseca changed the base branch from develop to v2 August 16, 2022 10:29
@heitorlessa heitorlessa linked an issue Aug 17, 2022 that may be closed by this pull request
2 tasks
@rubenfonseca rubenfonseca marked this pull request as ready for review August 17, 2022 14:00
@rubenfonseca rubenfonseca requested a review from a team as a code owner August 17, 2022 14:00
@rubenfonseca rubenfonseca requested review from am29d and heitorlessa and removed request for a team and am29d August 17, 2022 14:00
@heitorlessa
Copy link
Contributor

Thank you so much @rubenfonseca. Two QQ before I start reviewing the implementation:

1. Can customers add multiple value headers in Response? e.g. Response(headers={"Content-type": [...]}
2. Any chance we can add E2E for these to make sure ALB and API GW don't break?

@rubenfonseca
Copy link
Contributor Author

@heitorlessa

  1. I was thinking doing that change but pulled back because I thought it would be too big. But now that we're doing breaking changes, I think that is what makes most sense. I will change that.
  2. I will work on that :)

@rubenfonseca rubenfonseca marked this pull request as draft August 18, 2022 15:03
@pull-request-size pull-request-size bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 18, 2022
@boring-cyborg boring-cyborg bot added github-actions Pull requests that update Github_actions code internal Maintenance changes labels Aug 18, 2022
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 18, 2022
@rubenfonseca rubenfonseca changed the base branch from v2 to develop August 18, 2022 21:40
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 18, 2022
@heitorlessa heitorlessa added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 25, 2022
@heitorlessa
Copy link
Contributor

For some reason I can't run tests locally because it fails on apigw v2 alpha import. I've noticed CI didn't run (to confirm if it's on my laptop only) because it's the new v2 branch. I'll update CI to run on v2 too.

The PR is just fantastic. The only gut feeling I'm having is the UX of only supporting a list of values for headers - I'd like to run some tests locally to remind myself whether Response(headers={"custom_header": "value"}) would work or fail. If it does fail, I'm pretty sure many will be caught by surprise and cut tickets afterwards - we need to nail what trade-offs we'll make in this case.

@heitorlessa
Copy link
Contributor

heitorlessa commented Aug 28, 2022

For some reason I can't run tests locally because it fails on apigw v2 alpha import. I've noticed CI didn't run (to confirm if it's on my laptop only) because it's the new v2 branch. I'll update CI to run on v2 too.

The PR is just fantastic. The only gut feeling I'm having is the UX of only supporting a list of values for headers - I'd like to run some tests locally to remind myself whether Response(headers={"custom_header": "value"}) would work or fail. If it does fail, I'm pretty sure many will be caught by surprise and cut tickets afterwards - we need to nail what trade-offs we'll make in this case.

Asked on Discord, and two customers prefer values always being a list regardless - please disregard this comment. I'll merge on Monday - just need to double check docs on event handler code snippets output

@heitorlessa
Copy link
Contributor

heitorlessa commented Aug 29, 2022

As we discussed in the standup, we'll go with multiValueHeaders being the default and allow customers to pass a single string OR list of strings as header value(s). Next steps to merge it are:

  • [Ruben] Update doc snippets to use the new multiValueHeaders
  • [Heitor] Review whether parametrize tests can be simplified

@rubenfonseca
Copy link
Contributor Author

Ready to review.

@heitorlessa heitorlessa changed the title feat(event_handler): improved support for headers and cookies feat(event_handler): improved support for headers and cookies in v2 Aug 29, 2022
@heitorlessa
Copy link
Contributor

we can cleanup the leftovers from peer review on adding a more realistic cookie, and use POST instead of GET to make tests dynamic.

@heitorlessa heitorlessa merged commit 1696228 into aws-powertools:v2 Aug 29, 2022
@rubenfonseca rubenfonseca deleted the feat/api-gateway-cookies-v2 branch August 29, 2022 15:56
rubenfonseca added a commit that referenced this pull request Aug 31, 2022
rubenfonseca added a commit to rubenfonseca/aws-lambda-powertools-python that referenced this pull request Aug 31, 2022
rubenfonseca added a commit to rubenfonseca/aws-lambda-powertools-python that referenced this pull request Sep 2, 2022
Tankanow pushed a commit to Tankanow/aws-lambda-powertools-python that referenced this pull request Sep 13, 2022
Tankanow pushed a commit to Tankanow/aws-lambda-powertools-python that referenced this pull request Sep 13, 2022
rubenfonseca added a commit that referenced this pull request Sep 14, 2022
rubenfonseca added a commit that referenced this pull request Oct 14, 2022
if isinstance(values, str):
payload["headers"][key] = values
else:
if len(values) > 1:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@heitorlessa I'm seeing TypeError: object of type 'NoneType' has no len() when I have the following header set:

'Access-Control-Allow-Origin': None

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasmarc could you create a bug report issue on this please?

We can make a patch release as soon as we reproduce it.

Thanks a lot!!!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with my laptop now

While we await for a bug report and a fix by EOD for headers with None value, you can safely use an empty string instead of None.

Because we provide native support for CORS in Event Handler, you can do as follows:

Test URL: https://xwgyz6mo2m.execute-api.eu-west-1.amazonaws.com/Prod/hello/

from aws_lambda_powertools.event_handler import APIGatewayRestResolver, CORSConfig

cors = CORSConfig(allow_origin="")  # Create CORS configuration
app = APIGatewayRestResolver(cors=cors)  # Ensure CORS is properly configured for all routes


@app.get("/hello")
def hello():
    return {"hello": "world"}


def lambda_handler(event, context):
    return app.resolve(event, context)

If your intent is to set an explicit null in the Allow-Origin...

W3C doesn't recommend having an Allow Origin explicitly set to null due to potential data leak, since data: and file: schemes can accessnull origins - that's not the case with an empty Allow Origin (''). Unless I'm misunderstanding both your intent (bug report helps!) or W3C docs.


With an explicit null

import json

from aws_lambda_powertools.event_handler import (
    APIGatewayRestResolver,
    CORSConfig,
    Response,
)

# Not recommended due to data: and file: schemes also using `null` origin
# https://w3c.github.io/webappsec-cors-for-developers/#avoid-returning-access-control-allow-origin-null
cors = CORSConfig(allow_origin="null")
app = APIGatewayRestResolver(cors=cors)


# https://xwgyz6mo2m.execute-api.eu-west-1.amazonaws.com/Prod/hello/
@app.get("/hello")
def hello():
    return {"hello": "world"}


# chore: verify API Gateway REST Lambda integration behaviour with `None`
# API Gateway REST serializes`None` as an empty string (just like `null` in JS)
# need proper E2E to validate behaviour for API Gateway HTTP (v2), ALB, and Function URL resolvers

# https://xwgyz6mo2m.execute-api.eu-west-1.amazonaws.com/Prod/none/
@app.get("/none")
def hello():
    headers = {"X-Empty": None, "X-Null": "null"}
    response = {"hello": "world"}
    return Response(body=json.dumps(response), headers=headers, status_code=200)


def lambda_handler(event, context):
    return app.resolve(event, context)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leandrodamascena when you're back online, feel free to create a bug report if we don't hear from the customer within an hour. Leaving the tests and my endpoint above to save you from setting it all up besides E2E.


tests/functional/test_headers_serializer.py

These tests reproduce the issue. What needs to be done is an E2E for each resolver to confirm whether the final value becomes '' or fails (ALB is always the odd one) - this needs to be documented too later.

def test_http_api_headers_serializer_with_null_values():
    serializer = HttpApiHeadersSerializer()
    payload = serializer.serialize(headers={"Foo": None}, cookies=[])
    assert payload == {"headers": {}, "cookies": []}


def test_multi_value_headers_serializer_with_null_values():
    serializer = MultiValueHeadersSerializer()
    payload = serializer.serialize(headers={"Foo": None}, cookies=[])
    assert payload == {"headers": {}, "cookies": []}


def test_single_value_headers_serializer_with_null_values():
    serializer = SingleValueHeadersSerializer()
    payload = serializer.serialize(headers={"Foo": None}, cookies=[])
    assert payload["headers"] == {}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @heitorlessa sorry for the delay. Yes, I worked around it and I did also notice that it was not recommended, so I almost hesitated to bring it up.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have filed #1791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation feature New feature or functionality internal Maintenance changes size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tests
Projects
None yet
3 participants