Correct casing for x-amzn-RequestId header #13230
Conversation
…rect request id casing, add requestid header to query protocol response
…l requests) to properly test headers
There was a problem hiding this comment.
LGTM, very nice catch, thanks for going the extra mile and validating the behavior against AWS. We might want to maybe check that against Pro just to be sure we don't have some tests validating the wrong behavior somewhere and then break.
(I'll maybe wait for the full run for fully LGTM-ing 😄)
This also looks like a safe change, as we're only increasing parity and most clients should already do the right thing 👍
| response_parser = create_parser(service_protocol) | ||
| # Properly use HeadersDict from botocore to properly parse headers | ||
| response_dict = serialized_response.to_readonly_response_dict() | ||
| response_dict["headers"] = HeadersDict(response_dict.get("headers", {})) |
There was a problem hiding this comment.
nice catch! I was going to mention that we should probably fix it for _botocore_error_serializer_integration_test too, but it was already there 😅
There was a problem hiding this comment.
I should have caught that, I had no idea as well 😅
Test Results - Alternative Providers180 tests 39 ✅ 2m 33s ⏱️ Results for commit 9d36015. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 38m 40s ⏱️ Results for commit 9d36015. |
LocalStack Community integration with Pro 2 files 2 suites 2h 0m 50s ⏱️ Results for commit 9d36015. |
alexrashed
left a comment
There was a problem hiding this comment.
Thanks a lot for jumping on this! Really surprising how many SDKs do not really implement the HTTP spec (like case-insensitivity of header fields), but rely on specifics in the implementation of AWS services. 😅
Good catch and thanks for verifying! 💯
|
Running pro test against this branch: https://github.com/localstack/localstack-pro/actions/runs/18311931930 (sorry, started another run but copied the wrong branch before 😅 ) |
|
Only one (seemingly unrelated) failure in the pro runs, will merge this one! |
Motivation
Even though [RFC 7230] states that header field names should be considered case insensitive, some clients to not conform to this specification.
Especially, the fluent bit cloudwatch client will assert the
x-amzn-RequestIdheader - case sensitive.To support this client, we need to return the correct casing for this response header.
This change not only changes the header (and has an AWS validated test to confirms it), but also adds the header to the
queryprotocol, something I confirmed for cloudwatch in the test, and manually for EC2.Changes
x-amzn-RequestIdheader forqueryprotocol servicesx-amzn-RequestIdheader forjsonandcbor(based) protocols