APIGW NG add HTTP and HTTP_PROXY integration #11143

bentsku · 2024-07-05T03:01:55Z

Motivation

This PR implements the HTTP and HTTP_PROXY integrations. The previous work we have done make those extremely simple to implement and understand, and it shows how much the current work is going to simplify APIGW.

The PR also contains a few changes across the board, because we can now test full end-to-end integration tests: the tests/aws/services/apigateway/test_apigateway_http.py test file is fully green with the new PR, with improved parity.

Changes

implemented the HTTP and HTTP_PROXY integrations
change some typing around some existing contexts, as those were not really Optional and it helps with making sure we're not lacking data once we're in the integration
remove the legacy handler as we can now enable full end-to-end tests
remove the full clear of headers when receiving a response from an integration that is not of _PROXY type, but instead have a mechanism to keep some
return the integration response/method response status code for the response
fix the path value, as we would replace the /<stage> multiple times through, and if you had a path looking /test/test where test was your stage and test just a regular resource, we're replace everything
add default values for Accept and Content-Type, behavior checked with AWS and add test for it
remove some snapshot skips when running the HTTP integrations integration tests with the new provider
update some unit tests to be a bit simpler and avoid mistakes

Testing

Set PROVIDER_OVERRIDE_APIGATEWAY=next_gen and run all tests in tests/aws/services/apigateway/test_apigateway_http.py, they now all run! 🥳

TODO

What's left to do:

PR description

github-actions · 2024-07-05T03:18:51Z

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 35m 36s ⏱️ + 1m 26s
3 177 tests +7 2 782 ✅ +7 395 💤 ±0 0 ❌ ±0
3 179 runs +7 2 782 ✅ +7 397 💤 ±0 0 ❌ ±0

Results for commit 6b0bf8f. ± Comparison against base commit d3aff2d.

♻️ This comment has been updated with latest results.

localstack-core/localstack/services/apigateway/next_gen/execute_api/integrations/http.py

cloutierMat

Fantastic work! So much to unpack in a small pr! 🚀

Great addition to the tests and super excited to see the tests starting to go green! and already some improvements on parity! 🕺

I have left a question about the way we delete the headers, but nothing blocking!

cloutierMat · 2024-07-05T17:43:22Z

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

@@ -12,7 +12,7 @@


 class InvocationRequest(TypedDict, total=False):


Awesome! That will make it so much easier to use now that we have a clear representation of what will always be there! 🚀

cloutierMat · 2024-07-05T17:49:33Z

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

+        # remove the stage from the path, only replace the first occurrence
+        raw_uri = raw_uri.replace(f"/{stage_name}", "", 1)


Nice ctach!

cloutierMat · 2024-07-05T17:50:51Z

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

+        for key in list(response.headers.keys(lower=True)):
+            if key not in ("connection", "content-type"):
+                del response.headers[key]


question: Wouldn't we save some work to build from scratch instead?

I'm not too sure about the whole "reconstruct" the whole Response object from the integration response handler, it almost defeat the purpose a little bit (we could just create a dict with the integration "response", and then only serialize it in the next "request handler", instead of having the integration response handler recreating the response completely. But still the big philosophical question I don't have a proper answer for 😄

But yes, sorry, we could basically override the whole Response.headers directly. One way would be initialize an empty Headers object and add the few values that should be kept, but this is a bit annoying because we need to be mindful of the fact that some headers might be "multi-values" and we need to call individually getlist on every key, and then update this with the mapping.

Something like:

response_headers = Headers() for header in ("connection", "content-type"): if values := response.headers.getlist(header): for value in values: response_headers.add(header, value)

or maybe a simplified version:

response_headers = Headers() for header in ("connection", "content-type"): if values := response.headers.getlist(header): response_headers.setlist(header, values)

I don't really have a preference, it also look a bit big, and same level of indentation!

@cloutierMat I think I'll merge the PR now, and we can update this in #11144 if you think any of the one above is better! I think we will need to rebase anyway. Is that ok?

Yes, totally. Sorry for the delay, I don't think it is a deal breaker at all. Let's have that conversation and adjust in a future PR.

No worries, no delay at all here 😄 Just didn't want to block future merge with conflicts 😅 sorry for that!

cloutierMat · 2024-07-05T18:09:52Z

localstack-core/localstack/services/apigateway/next_gen/execute_api/integrations/http.py

+        # TODO: configurable timeout (29 by default)
+        # request_parameters["timeout"] = 29


Question: Do you know if the timeout also affects proxy integration? Is it something we should manage at the Integration handler level?

I believe this should be the value timeoutInMillis from the Integration: https://docs.aws.amazon.com/apigateway/latest/api/API_Integration.html

So it should be managed at the integration level, I also had it for PROXY I think but removed it when refactoring. I'll update 👍

Thinking of which, there's also caching which is currently not supported at all, we might want to think of that later for the features not currently implemented!

cloutierMat · 2024-07-05T18:19:01Z

tests/aws/services/apigateway/test_apigateway_http.py

+        "$..headers.x-amzn-trace-id",
+        # TODO: only missing for HTTP
+        "$..headers.x-amzn-requestid",
+        "$..headers.x-amzn-trace-id",


nit: seems like "$..headers.x-amzn-trace-id" is missing for both.

good catch, thanks!

cloutierMat · 2024-07-05T18:36:59Z

tests/unit/services/apigateway/test_handler_integration_request.py

@@ -220,6 +224,20 @@ def test_request_override(self, integration_request_handler, default_context):
            "multivalue": ["1header", "2header"],
        }

+    def test_request_override_casing(self, integration_request_handler, default_context):


Good! Another strange behavior is mostly supported out of the box! 😄

add HTTP and HTTP_PROXY integration

ecaf822

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

bentsku self-assigned this Jul 5, 2024

bentsku mentioned this pull request Jul 5, 2024

make next-gen APIGW provider default #11035

Merged

2 tasks

bentsku added 2 commits July 5, 2024 15:16

update typing and refactor

68d01f5

update unit test to use the parser

1b6b71e

bentsku marked this pull request as ready for review July 5, 2024 13:40

bentsku requested a review from cloutierMat as a code owner July 5, 2024 13:40

bentsku commented Jul 5, 2024

View reviewed changes

localstack-core/localstack/services/apigateway/next_gen/execute_api/integrations/http.py Show resolved Hide resolved

fix method forwarding

263745a

cloutierMat approved these changes Jul 5, 2024

View reviewed changes

cloutierMat mentioned this pull request Jul 5, 2024

Apigw ng implement mock integration #11148

Merged

address PR comments

6b0bf8f

bentsku merged commit b2faf95 into master Jul 8, 2024
32 checks passed

bentsku deleted the apigw-ng-impl-http-integration branch July 8, 2024 15:44

		@@ -12,7 +12,7 @@


		class InvocationRequest(TypedDict, total=False):

		# remove the stage from the path, only replace the first occurrence
		raw_uri = raw_uri.replace(f"/{stage_name}", "", 1)

		# TODO: configurable timeout (29 by default)
		# request_parameters["timeout"] = 29

Uh oh!

APIGW NG add HTTP and HTTP_PROXY integration #11143

APIGW NG add HTTP and HTTP_PROXY integration #11143

Uh oh!

Conversation

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

Uh oh!

Motivation

Changes

Testing

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!

Uh oh!

cloutierMat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bentsku commented Jul 5, 2024 •

edited

Loading

github-actions bot commented Jul 5, 2024 •

edited

Loading