-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
APIGW NG: add LocalStack-only CORS handling for REST API #11764
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
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 25m 55s ⏱️ - 1h 16m 55s Results for commit 5bdc7fb. ± Comparison against base commit 0662122. This pull request removes 2701 tests. |
| ] | ||
| ) | ||
| self.response_handlers.extend( | ||
| self.exception_handlers.extend( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was just a switch around, because exception handlers are executed before response handlers, as we've learned the hard way in the beginning 😄
cloutierMat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another great example of the power of the handler chain! Everyday I am learning a bit more about cors! 🙏
| if not context.invocation_request: | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What is the scenario in which this handler gets called without an invocation request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good question 😅 I've taken it from the HTTP CORS handler, probably if we encounter an unexpected exception in the InvocationRequestParser, we could maybe raise some kind of exception, and the response handlers are always executed, so we would avoid a weird attribute error, or trying to access ["headers"] of None maybe? I think it's pretty unlikely, but better safe than sorry with the response handlers 😬
Motivation
We currently have special logic in
localstack/aws/handlers/cors.py, special for S3 and API Gateway who can self manage their CORS behavior.In order to guess that we are going to target an edge route, we currently have custom logic in
should_enforce_self_managed_service, which uses some heuristic to guess it and let the CORS handler do its thing.However, this does not catch every API Gateway routes, especially the ones related to custom domains. It would be good to move most of the CORS related logic for those "self-managed" services inside the service itself, like we have done for S3: meaning that if the config flag to disable it is enabled, we should let the service apply the global LocalStack CORS logic in its own way (as they most probably already have logic in place for it).
We have the
DISABLE_CUSTOM_CORS_APIGATEWAYflag to disable the CORS handling of API Gateway, and try to apply the default LocalStack CORS logic to API Gateway routes, allowing users to disregard and not modify their lambda for example for theAWS_PROXY(managing CORS for REST API can be quite a pain, see https://docs.aws.amazon.com/apigateway/latest/developerguide/how-to-cors.html). This specific handler will be executed only if the flag is on, and is meant to properly apply the CORS headers in all situations for the API Gateway edge routes.It also also allows us to have custom logic for the
OPTIONSrequest, for example.Changes
config.DISABLE_CUSTOM_CORS_APIGATEWAYis set, and the pipeline is green