Apigw ng implement header remapping #11237

cloutierMat · 2024-07-19T03:09:21Z

Motivation

This PR aims at bringing a higher level of parity with apigateway headers. Some headers when not passed through appropriately will fail the invocation.

As noted on the Important Notes of the Api Gateway documention, some headers are either, dropped, passthrough or remapped from the users requests and following data transformations.

Changes

A series of header manipulation in the handlers was added.

Invocation

(parsing handler)

Some headers are not making it into the context, for these we prevent populating them to the InvocationRequest when first parsing the incoming request (This change was introduces by Apigw ng refactor headers to werkzeug headers #11227 )

Request

( integration request handler)

For HTTP_PPROXY integration, while most headers available to the context are passed through to the integration request, some seem to be removed BEFORE applying the mapping templates.
A set of headers will raise an InternalServerError if modified by request parameters of mapping templates. Even for AWS_PROXY where they are not applied, go figure 😄
Some request headers are dropped after being applied.
Some request headers are default and we only apply them if not already set by the templates

Response

( method response handler)

Some headers are remapped with x-amzn-Remapped-
Some headers are dropped
Some headers are added as default if not set by the integration response. For example, Connection, seems to be taking from the original requests Connection

github-actions · 2024-07-19T03:29:11Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 36m 9s ⏱️ -3s
3 299 tests +88 2 908 ✅ +85 391 💤 +3 0 ❌ ±0
3 301 runs +88 2 908 ✅ +85 393 💤 +3 0 ❌ ±0

Results for commit 29f4f64. ± Comparison against base commit 4928fe8.

This pull request removes 54 and adds 142 tests. Note that renamed tests count towards both.

tests.aws.services.events.test_events_rules ‑ test_put_event_with_content_base_rule_in_pattern
tests.aws.services.events.test_events_rules ‑ test_put_events_with_rule_anything_but_to_sqs
tests.aws.services.events.test_events_rules ‑ test_put_events_with_rule_exists_false_to_sqs
tests.aws.services.events.test_events_rules ‑ test_put_events_with_rule_exists_true_to_sqs
tests.aws.services.events.test_events_rules ‑ test_put_rule
tests.aws.services.events.test_events_rules ‑ test_rule_disable
tests.aws.services.events.test_events_rules ‑ test_verify_rule_event_content
tests.aws.services.lambda_.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_deletion_event_source_mapping_with_dynamodb
tests.aws.services.lambda_.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_disabled_dynamodb_event_source_mapping
tests.aws.services.lambda_.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_duplicate_event_source_mappings
…

tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_aws[AWS]
tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_aws[AWS_PROXY]
tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_http[HTTP]
tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_http[HTTP_PROXY]
tests.aws.services.cloudformation.resources.test_ec2 ‑ test_ec2_security_group_id_with_vpc
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_deletion_event_source_mapping_with_dynamodb
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_disabled_dynamodb_event_source_mapping
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_duplicate_event_source_mappings
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_dynamodb_event_filter[content_filter_type]
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_dynamodbstreams.TestDynamoDBEventSourceMapping ‑ test_dynamodb_event_filter[content_multiple_filters]
…

This pull request removes 2 skipped tests and adds 5 skipped tests. Note that renamed tests count towards both.

tests.aws.services.events.test_events_rules ‑ test_verify_rule_event_content
tests.aws.services.lambda_.test_lambda_integration_kinesis.TestKinesisSource ‑ test_kinesis_event_source_mapping_with_async_invocation

tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_aws[AWS]
tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_aws[AWS_PROXY]
tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_http[HTTP]
tests.aws.services.apigateway.test_apigateway_integrations.TestApiGatewayHeaderRemapping ‑ test_apigateway_header_remapping_http[HTTP_PROXY]
tests.aws.services.lambda_.event_source_mapping.test_lambda_integration_kinesis.TestKinesisSource ‑ test_kinesis_event_source_mapping_with_async_invocation

♻️ This comment has been updated with latest results.

bentsku

Wow, that is a lot of work! Impressive tests, they really cover a lot of ground here.

I'm a bit torn about the headers helper file, as some of those methods are very tied to their handler and integration, and I'm wondering if it would maybe make more sense to have them in the handler directly, as they most probably won't be able to be re-used as is. We could keep the base header manipulation utils there, and move the "handler" specific methods directly there? What do you think? It feels like having helpers at first made sense, but now that there is a lot of integration specific (that we didn't expect), maybe it could be simpler and more contained.

Also, the fact that one is used for both exceptions and invocations makes me want to create a response handler for those, maybe it could simplify it a bit. It's not a big deal if the header doesn't do a lot, I think it might even be a good thing! 😄

We could also attach the integration type to the context in the request routing, I think that could be worth it as we access it quite often now.

All in all, this is really great work, you really went above and beyond here 🚀

localstack-core/localstack/testing/snapshots/transformer_utility.py

tests/aws/services/apigateway/test_apigateway_dynamodb.py

...ack-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_request.py

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

tests/aws/services/apigateway/test_apigateway_http.py

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

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

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

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

cloutierMat · 2024-07-22T16:12:33Z

Thanks for the review @bentsku. I agree that the headers helper is doing a bit more than it should really. You are right that in the and, most of the methods are tightly coupled with their integrations. I will move those methods in their respective handlers and keep only the shared methods in there

bentsku

Wow, nice changes! Thanks a lot for addressing my concerns, this looks really good! The PR felt very easy to parse and understand, that was a lot of work! Congrats!
I just have minor comments, but nothing too major. After the questions are answered, this can be merged 😄 LGTM!

bentsku · 2024-07-23T07:42:09Z

...stack-core/localstack/services/apigateway/next_gen/execute_api/handlers/gateway_exception.py

        error = self.create_exception_response(exception, context)
        if error:
            response.update_from(error)

+    @staticmethod
+    def set_error_context(exception: BaseGatewayException, context: RestApiInvocationContext):


so neat 👌

bentsku · 2024-07-23T08:19:31Z

...stack-core/localstack/services/apigateway/next_gen/execute_api/handlers/gateway_exception.py

        # TODO apply responseParameters to the headers and get content-type from the gateway_response
-        return {"content-type": APPLICATION_JSON, "x-amzn-ErrorType": exception.code}


nit: any particular reason we're deleting the Content-Type here? We could avoid the workaround to remove the default content-type of the response if we'd let this for now, as right above we call json.dumps, maybe setting the content type explicitly makes sense?

Yeah, that makes more sense. Then when we implement the response mappings, we will have the content-type available from the template! 👍

bentsku · 2024-07-23T08:20:58Z

...ack-core/localstack/services/apigateway/next_gen/execute_api/handlers/integration_request.py

+        # Some headers can't be modified by parameter mappings or mapping templates.
+        # Aws will raise in those were present. Even for AWS_PROXY, where it is not applying them.
+        if header_mappings := request_data_mapping["header"]:
+            self._validate_headers_mapping(header_mappings, integration_type)


super neat!

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

bentsku · 2024-07-23T08:44:23Z

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

+        if not (connection := request.headers.get("Connection")) or connection != "close":
+            # We only set the connection if it isn't close.
+            # There appears to be in issue in Localstack, where setting "close" will result in "close, close"


ah, good catch!

bentsku · 2024-07-23T08:52:54Z

tests/unit/services/apigateway/test_handler_integration_request.py

+        assert default_context.integration_request["body"] == b""
+        assert default_context.integration_request["headers"]["Accept"] == "application/json"
+        assert default_context.integration_request["http_method"] == "POST"
+        assert default_context.integration_request["query_string_parameters"] == {}
+        assert default_context.integration_request["uri"] == "https://example.com"


good change! might be easier to parse what's wrong in the test 👍

bentsku · 2024-07-23T08:55:46Z

tests/unit/services/apigateway/test_response_context_handler.py

+    return ApiGatewayEndpoint.create_response(Request())
+
+
+class TestResponseContextHandler:


q: should we add a test for the keep-alive behavior? e.g. not setting the close if it's there, and setting keep-alive if anything else?

I am not sure this is the right place for this test, but I can create a new test file to test this behavior

bentsku · 2024-07-23T09:00:48Z

...lstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/response_context.py

+from localstack.utils.strings import short_uid
+
+
+class ResponseContextHandler(RestApiGatewayHandler):


nit: what do you think about the name InvocationResponseEnricher? I'm not sure the name convey what it really does here.

cloutierMat self-assigned this Jul 19, 2024

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

cloutierMat added this to the 3.6 milestone Jul 19, 2024

cloutierMat marked this pull request as ready for review July 19, 2024 21:35

cloutierMat requested review from bentsku, dominikschubert and steffyP as code owners July 19, 2024 21:35

cloutierMat mentioned this pull request Jul 19, 2024

make next-gen APIGW provider default #11035

Merged

2 tasks

cloutierMat added 9 commits July 19, 2024 15:44

Implement header drop and remap

483871a

fix unit test

107bd63

removed integration unit test for remapping

5308be7

Fixed tests

f521c7f

update snapshots

4af0e9f

fix exception handler unit test

6409318

Added tests for headers mapping

0b1a810

transformer utility

98b3349

fix dynamo tests

254c093

cloutierMat force-pushed the apigw-ng-implement-header-remapping branch from 90ca21d to 254c093 Compare July 19, 2024 21:44

bentsku reviewed Jul 22, 2024

View reviewed changes

cloutierMat added 8 commits July 22, 2024 10:23

pr nits

130c169

Added x-forwarded-proto and Fixed apigw transformers

2c5aa11

maved default connection and content-type to Response creation

82fc708

add integration to context

7bc0dea

fixed tests

ffd9de0

move headers utils to integration request

044a55b

added response handler

4962f0e

added unit tests

7d2140f

removed unused argument

adc4ca5

cloutierMat requested a review from bentsku July 23, 2024 01:43

bentsku approved these changes Jul 23, 2024

View reviewed changes

pr comment

29f4f64

cloutierMat merged commit e67710d into master Jul 23, 2024
31 of 32 checks passed

cloutierMat deleted the apigw-ng-implement-header-remapping branch July 23, 2024 21:39

bentsku mentioned this pull request Jul 24, 2024

fix APIGW NG response enricher handler #11256

Merged

		# TODO apply responseParameters to the headers and get content-type from the gateway_response
		return {"content-type": APPLICATION_JSON, "x-amzn-ErrorType": exception.code}

		return ApiGatewayEndpoint.create_response(Request())


		class TestResponseContextHandler:

		from localstack.utils.strings import short_uid


		class ResponseContextHandler(RestApiGatewayHandler):

Uh oh!

Apigw ng implement header remapping #11237

Apigw ng implement header remapping #11237

Uh oh!

Conversation

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

Uh oh!

Motivation

Changes

Invocation

Request

Response

Uh oh!

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

Uh oh!

LocalStack Community integration with Pro

Uh oh!

bentsku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloutierMat commented Jul 22, 2024

Uh oh!

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

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!

cloutierMat commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading