APIGW NG implement response template #11144

cloutierMat · 2024-07-05T04:08:25Z

Motivation

Completes the core of the Response Integration handler with implementation of response templates. The response template is used to modify the body along with allowing to override headers and response status code.
https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-override-request-response-parameters.html

Selecting the right template is a bit trickier than for the integration request as aws does not simply select a matching template or passthrough, instead it will select a template if there are templates created. The documentation states that if no templates are matched, it will select the first. But there is no mention as to what this first is. It does seem to favor application/json no matter if it was created first or not. Over my tests with the console this is the best I could figure out. There might be some extra logic I missed.

Changes

Template selection
VTL rendering for the response template
Apply overrides to headers and status code
Add the response body.

Other bug fixes

Terminate chain in exception handler. to prevent integration response from executing
Provide default if requestTemplates=None to prevent NoneType Exception
Enforce shape of ContextVarsResponseOverride
Fix method request reading from path instead of path_parameters

TODO

What's left to do:

write unit tests

github-actions · 2024-07-05T04:25:17Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 23m 53s ⏱️ - 1h 11m 23s
662 tests - 2 517 617 ✅ - 2 167 45 💤 - 350 0 ❌ ±0
664 runs - 2 517 617 ✅ - 2 167 47 💤 - 350 0 ❌ ±0

Results for commit 66ce1f4. ± Comparison against base commit acbbd3f.

This pull request removes 2517 tests.

tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

bentsku

Nice work! 🚀 lots of validation went into this, it's quite apparent 😄

I mostly have a comment regarding the code paths in the handler if we have a response template or not. Not quite sure what is the right solution, and I'd like your opinion on it too!

Also, great catch for the exception handler!

Thank you so much for adding this, I can't wait to try to run the pipeline with all the open PRs merged!

...ck-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_response.py

localstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/method_request.py

localstack-core/localstack/services/apigateway/next_gen/execute_api/template_mapping.py

tests/unit/services/apigateway/test_handler_integration_response.py

bentsku

LGTM! Great job! And great investigation, we're pushing far into parity here 🚀 I believe most of the comments could be addressed in follow-ups.

The remapping of headers is super interesting!

I've actually looked the remapped headers now, and they are, for once, documented:
https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-known-issues.html#api-gateway-known-issues-rest-apis

I found some gems here, some we hit our heads against:

The test invocation of a method uses the default content type of application/json and ignores specifications of any other content types.

So yes, we have a bit of work regarding the remapping, but I'm sure we can nicely generalize it and make it usable both in the integration response here and directly in the _PROXY integration as well. It might make the integration response handler a bit smaller as well, removing the headers list from it (we might need to declare them somewhere else, TBD 😄) as there are multiple dimensions to it, dropped/remapped/passthrough.

Great job, thanks for pushing through and finding where we were wrong!

bentsku · 2024-07-10T11:10:12Z

...ck-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_response.py

-        if mapped_headers := response_data_mapping.get("header"):
-            response.headers.update(mapped_headers)
+        # Those headers are passed through from the response headers, there might be more?
+        passthrough_headers = ("connection", "content-type", "content-length")


maybe Content-Type might not come from the response but is set here, depending on the template? We'll need to try with an integration that returns XML or HTML but the response template is JSON or there is no response template 🤔 or maybe you've done it already 😅

API Gateway includes a Content-Type header for all integration responses. By default, the content type is application/json.

bentsku · 2024-07-10T11:21:23Z

...ck-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_response.py

+            return ""
+
+        # The invocation request header is used to find the right response templated
+        accept = request["headers"].get("accept")


I also found this in the documentation:

When a request contains multiple media types in its Accept header, API Gateway only honors the first Accept media type.

I guess it depends it's a proper header list, in that case get might get the first, or if we need to parse the value and split on ,. Maybe we could write a quick test for it?

There will be a follow up pr with it that will include a fix for the headers

bentsku · 2024-07-10T11:28:19Z

...ck-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_response.py

+        # When trying to override certain headers, they will instead be remapped
+        # There may be other headers, but these have been confirmed on aws
+        remapped_headers = (
+            "connection",
+            "content-length",
+            "date",
+            "x-amzn-requestid",
+            "content-type",
+        )
+        for header in remapped_headers:
+            if value := headers.get(header):
+                headers[f"x-amzn-remapped-{header}"] = value
+                headers.remove(header)


This is a really great find! It's also very similar to what is done in the HTTP_PROXY integration.
The big table in the documentation shares more insights, and it seems it can be different from each integration... this is painful.

I think we should tackle this in a follow up, I wonder how we can generalize this. Maybe a util? Because I was thinking of a handler, but in the integration response step, this is very specific, it happens after mapping/overriding but before passing through, unlike the proxy where it just happens on the response directly. This is going to be fun to test 😅 and we also need to do it for requests, it seems a few are dropped, nothing remapped.
Could also be some constants we re-use, with a base list and depending on the integration type. As the way we use it depends a lot, we don't always have the same data types as input/output.
(side-note: the AWS_PROXY also drops some request headers from the payload sent to lambda, but those are marked passthrough in the documentation, which makes sense in the sense of the table, but not for the context... 😅)
(edit2: looks like the table isn't super precise, as it says User-Agent is Passthrough.. would need to check different scenarios but we know it overrides it for HTTP)

bentsku · 2024-07-10T11:42:32Z

tests/unit/services/apigateway/test_handler_integration_response.py

+        integration_response_handler(default_context, response)
+        assert response.data == b"json"
+
+    def test_remapped_headers(


nice new test, made it very clear 💯

cloutierMat added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Jul 5, 2024

cloutierMat added this to the 3.6 milestone Jul 5, 2024

cloutierMat self-assigned this Jul 5, 2024

cloutierMat force-pushed the apigw-ng-response-template-mapping branch from 4528d3d to 0e15837 Compare July 5, 2024 19:02

cloutierMat changed the base branch from master to apigw-ng-impl-http-integration July 5, 2024 22:20

cloutierMat force-pushed the apigw-ng-response-template-mapping branch from 790884b to 6515404 Compare July 5, 2024 23:19

cloutierMat changed the base branch from apigw-ng-impl-http-integration to master July 5, 2024 23:20

cloutierMat force-pushed the apigw-ng-response-template-mapping branch from 6515404 to 3cd1daf Compare July 5, 2024 23:24

cloutierMat mentioned this pull request Jul 5, 2024

make next-gen APIGW provider default #11035

Merged

2 tasks

cloutierMat marked this pull request as ready for review July 5, 2024 23:45

cloutierMat requested a review from bentsku as a code owner July 5, 2024 23:45

cloutierMat mentioned this pull request Jul 5, 2024

Apigw ng implement mock integration #11148

Merged

bentsku reviewed Jul 8, 2024

View reviewed changes

bentsku mentioned this pull request Jul 8, 2024

APIGW NG add HTTP and HTTP_PROXY integration #11143

Merged

1 task

cloutierMat added 10 commits July 9, 2024 16:28

implement response template

36ff6b2

fixed exisisting tests

276ce6f

Fixed various bugs

4611771

Fixed tests

4da276d

added tests

e0e38d2

move the flag to terminate the chain on top of the handler call

e2183d5

fixed test

4bcfa5a

removed comments in test file

476b9f6

Update integration response logic

6e3a200

added unit test

f238894

cloutierMat force-pushed the apigw-ng-response-template-mapping branch from 3cd1daf to f238894 Compare July 10, 2024 01:46

cloutierMat added 3 commits July 9, 2024 19:50

added comment

ff15b68

Clarify logic around template selection

5646c08

updated integration response key in tests

ac27fe9

Updated the vars with same naming as render_request

bb4a09e

cloutierMat requested a review from bentsku July 10, 2024 02:21

bentsku approved these changes Jul 10, 2024

View reviewed changes

bentsku changed the title ~~implement response template~~ APIGW NG implement response template Jul 10, 2024

pr comments

66ce1f4

cloutierMat merged commit d9e0cb0 into master Jul 11, 2024
32 checks passed

cloutierMat deleted the apigw-ng-response-template-mapping branch July 11, 2024 03:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

APIGW NG implement response template #11144

APIGW NG implement response template #11144

Uh oh!

cloutierMat commented Jul 5, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jul 5, 2024 •

edited

Loading

Uh oh!

bentsku left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bentsku left a comment

Uh oh!

bentsku Jul 10, 2024

Uh oh!

bentsku Jul 10, 2024

Uh oh!

cloutierMat Jul 11, 2024

Uh oh!

bentsku Jul 10, 2024 •

edited

Loading

Uh oh!

bentsku Jul 10, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

APIGW NG implement response template #11144

APIGW NG implement response template #11144

Uh oh!

Conversation

cloutierMat commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

TODO

Uh oh!

github-actions bot commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

bentsku left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

bentsku Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

bentsku Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

cloutierMat Jul 11, 2024

Choose a reason for hiding this comment

Uh oh!

bentsku Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bentsku Jul 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cloutierMat commented Jul 5, 2024 •

edited

Loading

github-actions bot commented Jul 5, 2024 •

edited

Loading

bentsku left a comment •

edited

Loading

bentsku Jul 10, 2024 •

edited

Loading