cloud_storage_clients: check Content-Type header before parsing REST error responses in s3_client
#25738
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WillemKauf
changed the title
cloud_storage_clients: check Content-Type header before parsing REST error responsescloud_storage_clients: check Content-Type header before parsing REST error responses in s3_client
CI test resultstest results on build#64237
test results on build#64258
test results on build#64526
|
WillemKauf
force-pushed
the
s3_content_type_parse
branch
from
4ef35e1 to
e060ecb
Compare
WillemKauf
force-pushed
the
s3_content_type_parse
branch
from
e060ecb to
78455b6
Compare
`ss::httpd::function_handler` calls `rep->done(_type)` on the reply before returning it. Because certain code paths in imposter services may want to return a different `Content-Type` in their response, this impl's `handle()` function allows for modifying the `type` set in the function handler's body before the final call to `done(type)` is performed. This class also handles an oversight in `mime_types.cc`, in which `xml` is not handled as a possible `mime` type.
All S3 error codes should be returned using `application/xml` as the `Content-Type` header.
There are code paths here that possibly return a `xml` body for their response. They need to ensure the response will be properly marked with `application/xml` in the `Content-Type` header.
…ly_with()` Sometimes we need to be able to set the headers and the body of a response. Add a new overload that allows us to do so.
…tor_test` The `not_found_response` is XML encoded.
…t_test` More XML encoded responses that need their `Content-Type` headers properly set.
Future commits will add use of this function to the `s3_client`. For that reason, move the `enum class response_content_type` to `types.h`, and `get_response_content_type()` helper function to `util.cc/.h`.
According to the S3 docs (`https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html`), REST error responses _should_ have the Content-Type header set to `application/xml`. However, response like `503 Service Unavailable` may not be of this assumed `Content-Type`. As seen in CI (abridged output): ``` WARN s3 - s3_client.cc:841 - S3 PUT request failed for key "...": Service Unavailable [Content-Type: text/plain; charset=utf-8]; ... ERROR s3 - util.cc:209 - !!parsing error boost::wrapexcept<boost::property_tree::xml_parser::xml_parser_error> (<unspecified file>(1): expected <) ERROR s3 - s3_client.cc:441 - !!error parse error boost::wrapexcept<boost::property_tree::xml_parser::xml_parser_error> (<unspecified file>(1): expected <) ``` To fix this issue, read the `Content-Type` header in `s3_client` instead of assuming `xml` as the `Content-Type` and attempting to parse it. If the `Content-Type` is not `xml`, we will return a partially filled `rest_error_response`.
WillemKauf
force-pushed
the
s3_content_type_parse
branch
from
78455b6 to
1cc76c7
Compare
Lazin
approved these changes
Apr 8, 2025
nvartolomei
approved these changes
Apr 8, 2025
| } | ||
| } | ||
|
|
||
| auto result_prefix = buf.empty() ? "Empty error response, " : ""; |
There was a problem hiding this comment.
shall we dump few bytes of the response at trace log level to aid debugging in the future if we fall into this unexpected branch with non-xml/non-empty response?
There was a problem hiding this comment.
Yeah that would be a good idea, I can add it in a follow up
|
/cdt |
|
Running a cdt test for a sanity check, will merge after. |
|
/ci-repeat |
|
/backport v25.1.x |
|
/backport v24.3.x |
|
Failed to create a backport PR to v24.3.x branch. I tried: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to the S3 docs, REST error responses should have the Content-Type header set to
application/xml. However, response like503 Service Unavailablemay not be of this assumedContent-Type.As seen in CI (abridged output):
To fix this issue, read the
Content-Typeheader ins3_clientinstead of assumingxmlas theContent-Typeand attempting to parse it. If theContent-Typeis notxml, we will return a partially filledrest_error_response.There are a number of JIRA tickets associated with this
xml_parser_error- however, there may be an actual root cause for these errors that are not fixed by simply fixing thexmlparsing.Commits TL;DR
The first 6 commits deal with our unit and fixture testing expectations, since previously we would always parse S3 REST responses as
XMLwithout checking theContent-Typeheader.Now that we don't, many changes had to be made through
http,cloud_storage_clientsandcloud_storagetests to ensure responses from imposter services which have content encoded withXMLalso now have a properly setContent-Typeheader.The last 2 commits are the real implementation changes for this PR.
Backports Required
Release Notes
Improvements
Content-TypeREST error responses in thes3_client.