Skip to content

HDDS-16189. Return application/xml for S3 error responses - #11031

Open
F64116045 wants to merge 3 commits into
apache:masterfrom
F64116045:HDDS-16189
Open

HDDS-16189. Return application/xml for S3 error responses#11031
F64116045 wants to merge 3 commits into
apache:masterfrom
F64116045:HDDS-16189

Conversation

@F64116045

@F64116045 F64116045 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

The makeBucket() test in MinIO's Mint test suite, included in the Ozone S3 compatibility report, exposed that S3 Gateway returns XML error bodies with Content-Type: text/plain.

OS3ExceptionMapper converts OS3Exception responses to XML but does not specify the response media type. Jersey therefore uses text/plain, causing clients such as the MinIO SDK to reject the response instead of parsing the S3 error.

This patch sets the response media type to application/xml and adds a unit test to verify it.

S3 REST error response reference: https://docs.aws.amazon.com/AmazonS3/latest/developerguide/ErrorResponses.html

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-16189

How was this patch tested?

mvn -pl :ozone-s3gateway test -Dtest=TestOS3Exceptions -DskipShade -DskipRecon -DskipDocs
mvn -pl :ozone-s3gateway test -DskipShade -DskipRecon -DskipDocs

CI: https://github.com/F64116045/ozone/actions/runs/31982282510

@F64116045
F64116045 marked this pull request as ready for review August 17, 2026 02:03
@Gargi-jais11 Gargi-jais11 added the s3 S3 Gateway label Aug 17, 2026

@chihsuan chihsuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! @F64116045 The change itself is correct. However, I tested the curl command from the Jira description, and it still returns text/plain. It looks like it takes a different error path. Please see inline comments.

}
exception.setRequestId(requestIdentifier.getRequestId());
return Response.status(exception.getHttpCode())
.type(MediaType.APPLICATION_XML_TYPE)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the change is correct, but I noticed that the exact reproduction of HDDS-16189 follows the authentication-error path, which bypasses OS3ExceptionMapper. Should we also set MediaType.APPLICATION_XML_TYPE in S3Utils.wrapOS3Exception() and add a test for that?

public static WebApplicationException wrapOS3Exception(OS3Exception ex) {
return new WebApplicationException(ex.getErrorMessage(), ex,
Response.status(ex.getHttpCode())
.entity(ex.toXml())
.build());
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed that the curl request stops at AuthorizationFilter and never reaches OS3ExceptionMapper.
Thanks a lot for catching this!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated S3Utils.wrapOS3Exception() and added a test.

import org.mockito.junit.jupiter.MockitoExtension;

/**
* This class tests OS3Exception class.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This class now also tests OS3ExceptionMapper. Could we update the class Javadoc accordingly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks.

@F64116045

Copy link
Copy Markdown
Contributor Author

@chihsuan Thanks a lot for the review and for catching the authentication-error path I missed. I've addressed the suggestions.

@Gargi-jais11 Gargi-jais11 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @F64116045 for the patch. Overall LGTM!
Let's add integration test as well in AbstractS3SDKV1Tests and AbstractS3SDKV2Tests for end to end testing.

@F64116045

Copy link
Copy Markdown
Contributor Author

Thanks @Gargi-jais11 for the review and suggestion, added the checks to both SDK v1 and v2 tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s3 S3 Gateway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants