introduce handler chain (next gen) in apigateway v1 invocation #10982

bentsku · 2024-06-07T14:55:41Z

Motivation

This is the first implementation of the handler chain inside an edge router handler for API Gateway REST APIs invocations.

Currently, APIGW registers an edge router handler which then invokes a function which passes a context around, where the separation of concerns is not very clear (it does a few things globally, like checking for UsagePlans API Key, or body/parameters validation), but then defers most of it to the BackendIntegration subclasses, which do things quite differently between themselves.
However, when looking at API Gateway and how they trigger/dispatch requests, a lot should be done outside the integrations themselves.

Using the new LocalStack constructs, we will be able to have clear separations of concerns between different steps of the invocation. This PR already introduces a few handlers, but more will come (API Key validation as a separate step). We are also making use of CompositeHandler in order for the code to be extendable from outside.

As a next step, we will also make use of Plugin and PluginManager to register the different AWS integrations transparently, and the IntegrationHandler will contain the logic necessary to load them and invoke them.

Additionally, a parity issue is present in LocalStack: once we create a REST API, it is available to be invoked under the execute-api route. However, this is not the case in AWS. You will need to create a Deployment, which is a snapshot (also referred as an "executable" of your API by AWS) of your current REST API, which then can be served by linking that deployment to a Stage. Currently, in LocalStack, invoking your API will always hit your "current" REST API, meaning if you do some updates, they will automatically be used while invoking the API. While this might be useful, this is very far from parity (we might re-introduce this mode with a feature flag, something akin to "hot reloading" of your API, but this would be opt-in) and it prevents things like testing canary releases or having different stages pointing to different versions of your API.
This new implementation, freezing the REST API and populating the context with it, will simplify greatly how to fetch the data in order to do the work during the invocation steps.

Currently, in order to work in an iterative way, we have introduced a few handlers outlining the future work to split the current code into a few different handlers, but one "temporary" or "legacy" handler holds all the invocation. This PR simply introduces the concept of a handler chain for the invocation.

Changes

create a new opt-in provider called next_gen in order to separate the new code and registrations of routes from the current implementation
create a new edge router handler, registered on the global APIGW routes. This handler instantiates a subclass or the rolo.Gateway in order to pass the incoming invocation request through a HandlerChain. This handler can be subclassed.
the RestApiGateway contains a first iteration of the handler list to be executed while invoking a REST API
introduce a new subclass of the RequestContext which will be extended by future PRs, to contain all the necessary data for a successful REST API invocation.
some new fields in the APIGW store in order to store the deployments and active deployments for the edge router handler to properly understand what API is being invoked and populate the context
a few fixes to existing only_localstack or needs_fixing tests that did not deploy the REST API before invoking it.

TODO

What's left to do:

remove the forced usage of this new prototype in config.py before merging

github-actions · 2024-06-07T15:24:28Z

S3 Image Test Results (AMD64 / ARM64)

2 files 2 suites 3m 11s ⏱️
404 tests 352 ✅ 52 💤 0 ❌
808 runs 704 ✅ 104 💤 0 ❌

Results for commit e03db85.

♻️ This comment has been updated with latest results.

github-actions · 2024-06-07T15:49:28Z

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 36m 36s ⏱️ + 1m 13s
3 074 tests +2 2 729 ✅ +2 345 💤 ±0 0 ❌ ±0
3 076 runs +2 2 729 ✅ +2 347 💤 ±0 0 ❌ ±0

Results for commit e03db85. ± Comparison against base commit 8e5b9ff.

♻️ This comment has been updated with latest results.

bentsku · 2024-06-10T18:44:46Z

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

+LOG = logging.getLogger(__name__)
+
+
+class InvocationRequestParser(RestApiGatewayHandler):


naming might be a bit misleading here: we probably think this handler will populate the context from the linked deployment, by "routing" the request depending on its path to the right APIGW Resource & Method. Naming is not set in stone and will be updated in follow up PRs.

Interesting, this is exactly what I thought this class would do 😄
But what is it intended to do then?

I think this class might still be needed under this name, to parse for example the RAW_URI from the request and such things.
And we can also have a "router" handler that will properly match the incoming request to the right APIGW resource. Once we start splitting things (or not for this handler, we might handle parsing/routing as one operation), I think naming will become clearer. I still think we might need a parser somewhere 😄

bentsku · 2024-06-11T14:27:37Z

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

+
+        else:
+            # TODO: return right response
+            print("here?")


oopsie, already removed in a not-yet-pushed commit

alexrashed

Awesome! This is a great starting point for the upcoming work on API Gateway and you can already see that this will bring way more structure, way less patching, way more extensibility, and way less boilerplate code (f.e. for the request parsing)! 🧹 💯
The (currently dummy) handlers in the chain already show how well structured this is going to look like afterwards 🤩
I only found some nitpicks, nothing blocking a merge. The changes contained (as soon as the provider override switch is removed again) should be completely safe to merge! 🥳

alexrashed · 2024-06-11T15:02:20Z

localstack-core/localstack/config.py

+# Whether to enable the Next Gen APIGW invocation logic (handler chain)
+# TODO: remove this before merge!
+if not os.environ.get("PROVIDER_OVERRIDE_APIGATEWAY"):
+    os.environ["PROVIDER_OVERRIDE_APIGATEWAY"] = "next_gen"


TODO remove before merge (as you already mentioned in the PR description, just another reminder 😛)

alexrashed · 2024-06-11T15:04:00Z

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

+                handlers.integration_request_handler,
+                handlers.integration_handler,
+                # temporary handler which executes everything for now
+                handlers.global_temporary_handler,


nit: Previously we used to use the term "legacy" for this kind of backwards compatibility / migration logic.

alexrashed · 2024-06-11T15:05:37Z

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

+from .context import RestApiInvocationContext
+
+
+class RestApiGateway(Gateway):


nit: Would be great to add a bit of documentation here: This is the main part controlling the API GW invocation handling now. Maybe we can add a few sentences (can also be in upcoming iterations) on what this is and how to extend / plug into this.

alexrashed · 2024-06-11T15:07:25Z

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

+    This class might not too much, we still need to investigate but from a first look, does not have much impact
+    on the HTTP response


nit: This comment is a bit confusing 😄

Yes, sorry, this was mostly internal 😅 we realized with @cloutierMat that the MethodResponse resource in APIGW might just be for "decorative purposes" and used to create SDKs from the REST API, but we're not 100% sure this handler does nothing. I'll update the comment!

alexrashed · 2024-06-11T15:08:39Z

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

+LOG = logging.getLogger(__name__)
+
+
+class InvocationRequestParser(RestApiGatewayHandler):


Interesting, this is exactly what I thought this class would do 😄
But what is it intended to do then?

alexrashed · 2024-06-11T15:12:53Z

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

Beautiful 😍

alexrashed · 2024-06-11T15:15:07Z

tests/integration/test_security.py

@@ -178,7 +178,7 @@ def test_cors_apigw_not_applied(self, aws_client):
                "Origin": "https://app.localstack.cloud",
            },
        )
-        assert response.status_code == 404
+        assert response.status_code in (403, 404)


nit: Maybe we could make this parameterized to already take the proper return code in case of the new provider (same below)?

Thing is, I'm not too sure yet what is the appropriate status code, I believe it is 403.

How would we parametrize this?
Shouldn't we instead use the config variable to do the assert? if config.APIGW_NEXT_GEN_PROVIDER: assert 403 else assert 404?

Well, if we don't know it currently then no.
I had the impression that the old code (404) is not in parity with AWS and the new provider will (403).
If this is the case, then we should make sure that the assert for the old provider is properly removed.
So if we don't know this yet, then it's fine for now, if we know it, then yes, a conditional like you mentioned would be perfectly fine (it would remind us to remove the old part from the test when we remove the config at the end of the project). ;)

alexrashed · 2024-06-11T15:19:35Z

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

+
+
+# TODO: subclass from -ext, and this handler will do the dispatching between v1 and v2 Gateways
+class ApiGatewayHandler:


nit: As discussed, we could maybe rename this to Endpoint or EndpointHandler to differentiate between HandlerChain handlers here (since this is what's invoked by the router on a URL match).

alexrashed · 2024-06-11T15:20:44Z

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

+
+    def __call__(self, request: Request, **kwargs) -> Response:
+        # api_id can be cased because of custom-tag id
+        api_id, stage = kwargs.get("api_id", "").lower(), kwargs.get("stage")


Awesome! This means we are finally using werkzeug's request parsing directly from the route matching! 🚀 🤩

alexrashed · 2024-06-11T15:22:18Z

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

+        # api_id can be cased because of custom-tag id
+        api_id, stage = kwargs.get("api_id", "").lower(), kwargs.get("stage")
+        if self.is_rest_api(api_id, stage):
+            LOG.info("Next-gen handler for APIGW v1 called")


nit: Maybe we shouldn't have the term "next gen" leak into the logs...

True, yes, APIGW v1 Endpoint or something similar will be better

bentsku

Thanks a lot for the review 🙏 will address the comments shortly!
When this PR is green, I will remove the flag and merge 🚀

bentsku · 2024-06-11T16:25:32Z

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

+    This class might not too much, we still need to investigate but from a first look, does not have much impact
+    on the HTTP response


Yes, sorry, this was mostly internal 😅 we realized with @cloutierMat that the MethodResponse resource in APIGW might just be for "decorative purposes" and used to create SDKs from the REST API, but we're not 100% sure this handler does nothing. I'll update the comment!

bentsku · 2024-06-11T16:28:13Z

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

+LOG = logging.getLogger(__name__)
+
+
+class InvocationRequestParser(RestApiGatewayHandler):


I think this class might still be needed under this name, to parse for example the RAW_URI from the request and such things.
And we can also have a "router" handler that will properly match the incoming request to the right APIGW resource. Once we start splitting things (or not for this handler, we might handle parsing/routing as one operation), I think naming will become clearer. I still think we might need a parser somewhere 😄

bentsku · 2024-06-11T16:29:05Z

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

+        # api_id can be cased because of custom-tag id
+        api_id, stage = kwargs.get("api_id", "").lower(), kwargs.get("stage")
+        if self.is_rest_api(api_id, stage):
+            LOG.info("Next-gen handler for APIGW v1 called")


True, yes, APIGW v1 Endpoint or something similar will be better

bentsku · 2024-06-11T16:31:05Z

tests/integration/test_security.py

@@ -178,7 +178,7 @@ def test_cors_apigw_not_applied(self, aws_client):
                "Origin": "https://app.localstack.cloud",
            },
        )
-        assert response.status_code == 404
+        assert response.status_code in (403, 404)


Thing is, I'm not too sure yet what is the appropriate status code, I believe it is 403.

How would we parametrize this?
Shouldn't we instead use the config variable to do the assert? if config.APIGW_NEXT_GEN_PROVIDER: assert 403 else assert 404?

localstack-core/localstack/config.py

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

bentsku self-assigned this Jun 7, 2024

bentsku added this to the Playground milestone Jun 7, 2024

bentsku force-pushed the next-gen-apigw-v1 branch 2 times, most recently from bbed56c to 1eb9126 Compare June 9, 2024 18:17

bentsku changed the title ~~WIP: next gen apigw v1~~ introduce handler chain (next gen) in apigateway v1 invocation Jun 10, 2024

bentsku commented Jun 10, 2024

View reviewed changes

alexrashed self-requested a review June 11, 2024 07:00

alexrashed modified the milestones: Playground, 3.6 Jun 11, 2024

bentsku marked this pull request as ready for review June 11, 2024 13:45

bentsku requested review from cloutierMat and thrau as code owners June 11, 2024 13:45

bentsku commented Jun 11, 2024

View reviewed changes

alexrashed approved these changes Jun 11, 2024

View reviewed changes

bentsku commented Jun 11, 2024

View reviewed changes

bentsku added 11 commits June 11, 2024 18:53

add config flag

d57a82b

wip, first prototype

032ae31

register route on deploy

e4b12b6

wip

f1dc6f9

add handlers, fix removing rules

a3d9e79

rewrite to use a global route

63d9a49

small fixes

4496e92

fix routing?

f971a2c

fix tests

bec98ea

remove print

0b72d7e

address PR comments

5687faa

bentsku force-pushed the next-gen-apigw-v1 branch from ff18519 to 5687faa Compare June 11, 2024 16:54

remove override flag

e03db85

bentsku modified the milestones: 3.6, 3.5 Jun 12, 2024

bentsku merged commit 7133027 into master Jun 12, 2024
37 checks passed

bentsku deleted the next-gen-apigw-v1 branch June 12, 2024 09:41

bentsku mentioned this pull request Jun 17, 2024

make next-gen APIGW provider default #11035

Merged

2 tasks

		LOG = logging.getLogger(__name__)


		class InvocationRequestParser(RestApiGatewayHandler):

		from .context import RestApiInvocationContext


		class RestApiGateway(Gateway):

		This class might not too much, we still need to investigate but from a first look, does not have much impact
		on the HTTP response



		# TODO: subclass from -ext, and this handler will do the dispatching between v1 and v2 Gateways
		class ApiGatewayHandler:

Uh oh!

introduce handler chain (next gen) in apigateway v1 invocation #10982

introduce handler chain (next gen) in apigateway v1 invocation #10982

Uh oh!

Conversation

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

Uh oh!

S3 Image Test Results (AMD64 / ARM64)

Uh oh!

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

Uh oh!

LocalStack Community integration with Pro

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bentsku Jun 11, 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!

Choose a reason for hiding this comment

Uh oh!

alexrashed 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!

alexrashed Jun 12, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

bentsku Jun 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bentsku commented Jun 7, 2024 •

edited

Loading

github-actions bot commented Jun 7, 2024 •

edited

Loading

github-actions bot commented Jun 7, 2024 •

edited

Loading

bentsku Jun 11, 2024 •

edited

Loading

alexrashed Jun 12, 2024 •

edited

Loading

bentsku left a comment •

edited

Loading

bentsku Jun 11, 2024 •

edited

Loading