add parsing and routing to APIGW next gen #11051

bentsku · 2024-06-18T17:18:08Z

Motivation

This PR introduces the parsing of the request via a handler to populate a context with what I think is necessary right now.

It also introduces the routing handler, which will take the incoming request and match it against the deployment, to get the right Resource and Method to attach it to the context. Those will be used by all the next handlers, it will allow us to know what to do against the incoming request (method request parameters, integration request, integration itself).

Currently, we re-use all the current code to do the routing, as the routing logic is quite complex. This can be replaced in the future by a router like the ServiceRequestRouter, and especially op_router.py, as it would help us to extract the parameters, and it seems the logic and even the syntax is pretty similar.

I've tackled the migration from the legacy routing to the Werkzeug routing like op_router.py in a follow up PR, see #11057.
If needed, I can already merge the 2 PRs in one if that makes it easier 😄

Changes

update the parsing handler, with what I think is necessary, but we can add as we go
add the routing handler, copy-pasting the current routing code (it is quite complex) and adapt it to our use
add unit tests for the 2 handlers

TODO

What's left to do:

add tests

github-actions · 2024-06-18T18:10:45Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 36m 24s ⏱️ + 1m 27s
3 114 tests +24 2 744 ✅ +12 370 💤 +12 0 ❌ ±0
3 116 runs +24 2 744 ✅ +12 372 💤 +12 0 ❌ ±0

Results for commit 82845b6. ± Comparison against base commit c35ce3b.

This pull request removes 1 and adds 25 tests. Note that renamed tests count towards both.

tests.aws.services.lambda_.test_lambda.TestLambdaMultiAccounts ‑ test_cross_account_access

tests.aws.services.events.test_events.TestEventBus ‑ test_put_permission[custom]
tests.aws.services.events.test_events.TestEventBus ‑ test_put_permission[default]
tests.aws.services.events.test_events.TestEventBus ‑ test_put_permission_non_existing_event_bus
tests.aws.services.events.test_events.TestEventBus ‑ test_remove_permission[custom]
tests.aws.services.events.test_events.TestEventBus ‑ test_remove_permission[default]
tests.aws.services.events.test_events.TestEventBus ‑ test_remove_permission_non_existing_sid[False-custom]
tests.aws.services.events.test_events.TestEventBus ‑ test_remove_permission_non_existing_sid[False-default]
tests.aws.services.events.test_events.TestEventBus ‑ test_remove_permission_non_existing_sid[True-custom]
tests.aws.services.events.test_events.TestEventBus ‑ test_remove_permission_non_existing_sid[True-default]
tests.aws.services.events.test_events.TestEvents ‑ test_put_events_exceed_limit_ten_entries[custom]
…

This pull request skips 1 test.

tests.aws.services.events.test_events.TestEventBus ‑ test_list_event_buses_with_prefix

♻️ This comment has been updated with latest results.

cloutierMat

🚀
The amount of work you put into this really shows!

I have left one question regarding the body in the InvocationRequest

cloutierMat · 2024-06-19T18:53:16Z

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

+def get_resources_from_deployment(deployment: RestApiDeployment) -> ListOfResource:
+    """
+    This returns the `Resources` from a deployment
+    This allows to decouple the underlying split of resources between Moto and LocalStack, and always return the right
+    format.
+    """


cloutierMat · 2024-06-19T18:56:53Z

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

+        for resource_path, resource in path_map.items():
+            api_path_regex = re.sub(r"{[^+]+\+}", r"[^\?#]+", resource_path)
+            api_path_regex = re.sub(r"{[^}]+}", r"[^/]+", api_path_regex)
+            if re.match(r"^%s$" % api_path_regex, path):
+                matches.append((resource_path, resource))


Glad to see the follow up that aims at getting rid of all this custom routing! 🤘

cloutierMat · 2024-06-19T19:02:00Z

tests/unit/services/apigateway/test_handler_request.py

+        """
+        This can be represented by the following routes:
+        - /
+        - GET /test
+        - PUT /test/{param}
+        - DELETE /{proxy+}
+        """


Nice and clean! The added comment helps a lot

cloutierMat · 2024-06-19T19:03:24Z

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

+    """Multi value query string parameters of the request"""
+    multi_value_headers: Optional[dict[str, list[str]]]
+    """Multi value headers of the request"""
+    body: Optional[bytes]


Would it be useful to have raw_body and decoded_body, or similar?

Not sure I understand, how do you mean?

Currently, we use restore_payload for the body, because the request parsing of Werkzeug might consume the data to create a parsed form, and then we would not be able to forward/read the raw body.

It splits in 3 parts: data, form and files. My belief is that APIGW does not manipulate different parts of the body directly? But I'm not sure.

It's also possible I'm not familiar with what is possible to do with APIGW 😅

🤔 I think the only thing we can do on the body is through the request templates. I haven't looked fully into the integration, so maybe this is the right way to go. I was just concerned about decoding it in with LS parser to restore it, to decode it again to apply the templates.

Ah, I see.
I think the issue here is what restore_payload does. We don't really "decode", we "re-encode" it to be like what we actually received from the client.

For example, the usual way is to call request.data to get the bytes content of the body.

But let's imagine your request is a Content-Type: application/x-www-form-urlencoded.
You get an HTTP body looking like this: field1=value1&field2=value2

When accessing request.data, you'll get... b"", an empty bytes object because Werkzeug will have parsed the request and consumed the stream, and set request.form to be equal to an ImmutableMultiDict looking like {"field1": "value1", "field2": "value2"}.

One of the way here is to directly consume the request.stream once to get the raw data out. But anything earlier in the handler chain called request.data, the stream will have be consumed and you have no way to get it back (yet! with the new twisted web server, we actually have a way now 🥳 which means we can have a short circuit restore_payload), which is why we re-encode the body 😕

So in itself I'd say we have the proper raw_body now, the one the request template should act upon?

I guess it makes sense then since only one handler should need to perform operations on the body, that step can ensure the body returns to bytes once it has completed and and shouldn't need to keep a dict representation of it anywhere. 👍

alexrashed

This is really a great push forward for the Next Gen API GW, providing a solid foundation re-using our internal frameworks 😍
Especially in combination with #11057 (where basically all the parsing / routing is then replaced to directly done by Werkzeug! 🥳

If needed, I can already merge the 2 PRs in one if that makes it easier 😄

Having two separate PRs helped to really review the different iterations (where each are quite big). Now, after the individual reviews, merging one into the other should work perfectly fine. :)

add parsing and routing

24dc690

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

bentsku self-assigned this Jun 18, 2024

add tests

0e5e23e

bentsku requested a review from alexrashed June 19, 2024 16:54

bentsku marked this pull request as ready for review June 19, 2024 16:54

bentsku requested a review from cloutierMat as a code owner June 19, 2024 16:54

This was referenced Jun 19, 2024

make next-gen APIGW provider default #11035

Merged

migrating NG APIGW routing to Werkzeug routing #11057

Merged

cloutierMat reviewed Jun 19, 2024

View reviewed changes

cloutierMat approved these changes Jun 19, 2024

View reviewed changes

alexrashed approved these changes Jun 20, 2024

View reviewed changes

migrating NG APIGW routing to Werkzeug routing (#11057)

82845b6

bentsku merged commit 946b273 into master Jun 20, 2024
30 checks passed

bentsku deleted the add-apigw-ng-routing branch June 20, 2024 13:02

bentsku mentioned this pull request Jun 21, 2024

fix NG APIGW unit tests usage of RestApiDeployment #11070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

add parsing and routing to APIGW next gen #11051

add parsing and routing to APIGW next gen #11051

Uh oh!

bentsku commented Jun 18, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jun 18, 2024 •

edited

Loading

Uh oh!

cloutierMat left a comment

Uh oh!

cloutierMat Jun 19, 2024

Uh oh!

cloutierMat Jun 19, 2024

Uh oh!

cloutierMat Jun 19, 2024

Uh oh!

cloutierMat Jun 19, 2024

Uh oh!

bentsku Jun 19, 2024

Uh oh!

cloutierMat Jun 19, 2024

Uh oh!

bentsku Jun 19, 2024

Uh oh!

cloutierMat Jun 19, 2024

Uh oh!

alexrashed left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

add parsing and routing to APIGW next gen #11051

add parsing and routing to APIGW next gen #11051

Uh oh!

Conversation

bentsku commented Jun 18, 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 Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

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!

alexrashed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bentsku commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading