Skip to content

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Oct 29, 2024

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_APIGATEWAY flag 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 the AWS_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 OPTIONS request, for example.

Changes

  • add a custom CORS handler for APIGW NextGen
  • we currently already have tests for the CORS handling when config.DISABLE_CUSTOM_CORS_APIGATEWAY is set, and the pipeline is green
  • we also will have custom tests in the upstream repo

@bentsku bentsku added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Oct 29, 2024
@bentsku bentsku self-assigned this Oct 29, 2024
@github-actions
Copy link

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   25m 55s ⏱️ - 1h 16m 55s
818 tests  - 2 701  759 ✅  - 2 346  59 💤  - 355  0 ❌ ±0 
820 runs   - 2 701  759 ✅  - 2 346  61 💤  - 355  0 ❌ ±0 

Results for commit 5bdc7fb. ± Comparison against base commit 0662122.

This pull request removes 2701 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]
…

@bentsku bentsku marked this pull request as ready for review October 29, 2024 21:34
@bentsku bentsku requested a review from cloutierMat as a code owner October 29, 2024 21:34
@bentsku bentsku added this to the 4.0 milestone Oct 29, 2024
]
)
self.response_handlers.extend(
self.exception_handlers.extend(
Copy link
Contributor Author

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 😄

Copy link
Member

@cloutierMat cloutierMat left a 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! 🙏

Comment on lines +30 to +31
if not context.invocation_request:
return
Copy link
Member

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?

Copy link
Contributor Author

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 😬

@bentsku bentsku merged commit fb7cde9 into master Oct 30, 2024
24 checks passed
@bentsku bentsku deleted the add-apigw-ng-cors-handler branch October 30, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants