APIGW: add Binary Media support for response in `AWS_PROXY` #12199

bentsku · 2025-01-29T00:05:10Z

Motivation

As reported in #12115, we did not support returning binary responses from AWS_PROXY integration. In the previous implementation of API Gateway, we were returning binary responses but in the wrong way: if the isBase64Encoded was set to True in the response, we decode it and send binary data.

However, it has a deep link to the binaryMediaTypes configuration of the REST API. Only if the client sends an Accept header that matches the binaryMediaTypes then the AWS_PROXY will respond with binary data.

This PR was quite frustrating, as it took me hours to implement the test and all the different cases, and I hit some caching issues, things failing in one run and not another, I had to deploy to different stages to be sure I wasn't hitting some weird issues.

And the implementation itself took less than 15min 🤦‍♂️ At least we have confidence in the different cases, and that the Content-Type header is actually not taken into account.

The implementation for now only covers AWS_PROXY, because the logic for other integrations is more complex, as there also is logic to decode the incoming payload, and the ContentHandling parameter, both for input and output. This will be tackled in a follow up.
I can set the Accept header and binaryMediaTypes in the request context then, if needed.

More resources from AWS:

Changes

implement a pretty thorough test (without snapshot!) to test all the possible use cases I could think of
implement the logic in the AWS_PROXY integration to return or not binary data

fixes #12115

github-actions · 2025-01-29T00:37:28Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 27m 49s ⏱️ - 1h 26m 12s
1 053 tests - 3 019 989 ✅ - 2 768 64 💤 - 251 0 ❌ ±0
1 055 runs - 3 019 989 ✅ - 2 768 66 💤 - 251 0 ❌ ±0

Results for commit 294e374. ± Comparison against base commit 952eb3e.

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

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]
…

tests.aws.services.apigateway.test_apigateway_lambda ‑ test_aws_proxy_binary_response

♻️ This comment has been updated with latest results.

cloutierMat

Awesome investigation with a really clean fix!

The amount of work behind those test is impressive. I left a question, but feel free to ignore as it seems to be getting around the caching issue. I think maybe on first deployment we could do without the extra wait, but it also makes for improved consistency and easy to see what is being tested!

Great work 🚀

cloutierMat · 2025-02-03T12:36:12Z

tests/aws/services/apigateway/test_apigateway_lambda.py

+    # we poll that the API is returning the right data after deployment
+    poll_condition(
+        lambda: _assert_invoke(accept="image/png", expect_binary=False), timeout=timeout, interval=1
+    )
+    if is_aws_cloud():
+        time.sleep(5)
+
+    # we did not configure binaryMedias so the API is not returning binary data even if all conditions are met
+    assert _assert_invoke(accept="image/png", expect_binary=False)


question: Any reason for not doing assert poll_condition(...). And since to move forward we already needed _assert_invoke() to be truthy, isn't the time.sleep(5) redundant?

Yes, I agree. I had super weird issues with AWS returning "all is good" and then failing and returning 403 or 500 again, so I had the sleep to have it more consistent. I also used poll_condition because debugging with retry is a true nightmare, as pytest only catches the first exception raised which has nothing to do with the actual failure...

I could try re-running the test against AWS to see if it's really needed.

I'll merge to rebase #12215, but we should probably find a way to properly validate deployments are.. well, deployed 😄

I remember facing that issue before, where there is a transition period where the request can be handled either by the new or the old deployment.

Yes, which is why I also created multiple stages now 😅 I did everything on one stage before, and the stability of the test was horrible 😭

I think that even on multiple stage, you can hit that issue, right? I ended up created a parametrize test back then to have a completely new api each time. your solution does make it a bit nicer to read, at the cost of extra wait time in aws.

Makes sense to create the new API to be honest... would need to have class scoped lambda, would be a bit easier. Wait before the next PR, where with the same API I test 12 combinations with 3 inputs... 😭 what a pain

I will give it a minute and catch up on a few other things first! 😉

Welcome back 😆 no worries, take your time, there's no hurry for this one 😄

cloutierMat · 2025-02-03T12:40:15Z

tests/aws/services/apigateway/test_apigateway_lambda.py

+    # we poll that the API is returning the right data after deployment
+    poll_condition(
+        lambda: _assert_invoke(accept="image/png", expect_binary=True), timeout=timeout, interval=1
+    )
+    if is_aws_cloud():
+        time.sleep(10)
+
+    # all conditions are met
+    assert _assert_invoke(accept="image/png", expect_binary=True)


Same here, though this one feels like it could work around caching issues with the multiple deployment 🤔

Yeah, same issue, weird caching or AWS returning "ok" then failing again... no idea why 😭

cloutierMat · 2025-02-03T12:41:22Z

tests/aws/services/apigateway/test_apigateway_lambda.py

+    # Accept has to exactly match what is configured
+    assert _assert_invoke(accept="*/*", expect_binary=False)
+
+    # client is sending a multiple accept, but AWS only checks the first one
+    assert _assert_invoke(accept="image/webp,image/png,*/*;q=0.8", expect_binary=False)
+
+    # client is sending a multiple accept, but AWS only checks the first one, which is right
+    assert _assert_invoke(accept="image/png,image/*,*/*;q=0.8", expect_binary=True)


That does simplify the implementation! 🙏

bentsku added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Jan 29, 2025

bentsku added this to the 4.2 milestone Jan 29, 2025

bentsku self-assigned this Jan 29, 2025

bentsku requested a review from cloutierMat as a code owner January 29, 2025 00:05

bentsku changed the title ~~Apigw binary support aws proxy~~ APIGW: add Binary Media support for response in AWS_PROXY Jan 29, 2025

bentsku mentioned this pull request Jan 29, 2025

feature request: Enabling binary support using the API Gateway REST API #5642

Closed

1 task

bentsku mentioned this pull request Feb 1, 2025

APIGW: binary support for non-proxy integration #12215

Merged

bentsku added 4 commits February 3, 2025 10:31

add test for binary support of AWS_PROXY

e46e413

add support for binary response in AWS_PROXY

f099a9a

update test with better case and comments

79384b4

port back mime_type helper changes

294e374

bentsku force-pushed the apigw-binary-support-aws-proxy branch from 349b657 to 294e374 Compare February 3, 2025 09:31

cloutierMat approved these changes Feb 3, 2025

View reviewed changes

bentsku merged commit 84b2f5f into master Feb 3, 2025
31 checks passed

bentsku deleted the apigw-binary-support-aws-proxy branch February 3, 2025 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

APIGW: add Binary Media support for response in `AWS_PROXY` #12199

APIGW: add Binary Media support for response in `AWS_PROXY` #12199

Uh oh!

bentsku commented Jan 29, 2025

Uh oh!

github-actions bot commented Jan 29, 2025 •

edited

Loading

Uh oh!

cloutierMat left a comment

Uh oh!

cloutierMat Feb 3, 2025

Uh oh!

bentsku Feb 3, 2025

Uh oh!

bentsku Feb 3, 2025

Uh oh!

cloutierMat Feb 3, 2025

Uh oh!

bentsku Feb 3, 2025

Uh oh!

cloutierMat Feb 3, 2025

Uh oh!

bentsku Feb 3, 2025 •

edited

Loading

Uh oh!

cloutierMat Feb 3, 2025

Uh oh!

bentsku Feb 3, 2025

Uh oh!

cloutierMat Feb 3, 2025

Uh oh!

bentsku Feb 3, 2025

Uh oh!

cloutierMat Feb 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

APIGW: add Binary Media support for response in AWS_PROXY #12199

APIGW: add Binary Media support for response in AWS_PROXY #12199

Uh oh!

Conversation

bentsku commented Jan 29, 2025

Motivation

Changes

Uh oh!

github-actions bot commented Jan 29, 2025 • 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!

bentsku Feb 3, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

APIGW: add Binary Media support for response in `AWS_PROXY` #12199

APIGW: add Binary Media support for response in `AWS_PROXY` #12199

github-actions bot commented Jan 29, 2025 •

edited

Loading

bentsku Feb 3, 2025 •

edited

Loading