Skip to content

HDDS-13737. S3 ETag JSON should be quoted#9248

Merged
ivandika3 merged 5 commits intoapache:masterfrom
echonesis:HDDS-13737
Nov 6, 2025
Merged

HDDS-13737. S3 ETag JSON should be quoted#9248
ivandika3 merged 5 commits intoapache:masterfrom
echonesis:HDDS-13737

Conversation

@echonesis
Copy link
Contributor

@echonesis echonesis commented Nov 4, 2025

What changes were proposed in this pull request?

This PR updates the output response during using aws s3api complete-multipart-upload function for Apache Ozone.

When we want to execute multi-part uploading, three steps/functions are included:

  • create-multipart-upload
  • upload-part
  • complete-multipart-upload

We could refer to the doc and figure out the defined response of upload-part should be

{
    "ETag": "\"e868e0f4719e394144ef36531ee6824c\""
}

What is the link to the Apache JIRA

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

How was this patch tested?

aws s3api --endpoint $ENDPOINT create-bucket --bucket=$BUCKET

UPLOAD_JSON=$(aws s3api create-multipart-upload \
  --bucket "$BUCKET" \
  --key "$KEY" \
  --endpoint-url "$ENDPOINT")

UPLOAD_ID=$(echo "$UPLOAD_JSON" | jq -r '.UploadId')

UPLOAD_PART_JSON=$(aws s3api upload-part \
  --bucket "$BUCKET" \
  --key "$KEY" \
  --part-number 1 \
  --body "$PART_FILE" \
  --upload-id "$UPLOAD_ID" \
  --endpoint-url "$ENDPOINT")

echo "$UPLOAD_PART_JSON"

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses ETag handling inconsistencies in the S3 gateway by ensuring ETags are properly quoted in responses and unquoted when stored or compared internally. The changes standardize the format of ETags across multipart upload operations.

  • Adds stripQuotes and wrapInQuotes utility methods for consistent ETag formatting
  • Updates multipart upload endpoints to wrap ETags in quotes for HTTP responses
  • Modifies tests to strip quotes from ETags before comparisons and XML serialization

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ObjectEndpointStreaming.java Wraps ETag in quotes for multipart upload response headers
ObjectEndpoint.java Adds stripQuotes/wrapInQuotes utilities and applies proper ETag formatting in multipart operations
AbstractS3SDKV2Tests.java Updates tests to strip quotes from ETags before comparison and XML construction
AbstractS3SDKV1Tests.java Updates tests to strip quotes from ETags before XML construction
S3SDKTestUtils.java Adds stripQuotes utility method for test code reuse
mpu_lib.robot Strips quotes from ETag before MD5 comparison in Robot Framework tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1070 to 1073
String raw = ozoneOutputStream.getMetadata().get(ETAG);
if (raw != null) {
ozoneOutputStream.getMetadata().put(ETAG, stripQuotes(raw));
}
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

Inconsistent ETag handling: The stripQuotes logic is only applied in the 'else' branch (lines 1070-1073) but not in the 'if (range != null)' branch (lines 1049-1059). Both branches copy source key metadata using putAll(), which means both should handle potentially quoted ETags consistently.

Copilot uses AI. Check for mistakes.
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.conf.StorageUnit;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

Duplicate import: PerformanceStringBuilder is imported both as a static import (line 30) and as a regular import (line 115). The regular import should be removed since the static import is sufficient and already used throughout the code.

Suggested change
import org.apache.hadoop.ozone.audit.AuditLogger.PerformanceStringBuilder;

Copilot uses AI. Check for mistakes.
import org.apache.hadoop.ozone.s3.util.RangeHeader;
import org.apache.hadoop.ozone.s3.util.RangeHeaderParserUtil;
import org.apache.hadoop.ozone.s3.util.S3Consts;
import org.apache.hadoop.ozone.s3.util.S3Consts.CopyDirective;
Copy link

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

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

Duplicate import: CopyDirective is imported both as a static import (line 50) and as a regular import (line 142). The regular import should be removed since the static import is sufficient.

Suggested change
import org.apache.hadoop.ozone.s3.util.S3Consts.CopyDirective;

Copilot uses AI. Check for mistakes.
* @param input The input string.
* @return The string without leading and trailing quotes.
*/
public static String stripQuotes(String input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this helper method. There's one in ObjectEndpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jojochuang !
I remove it as expected.

@jojochuang
Copy link
Contributor

jojochuang commented Nov 4, 2025

After applying this PR, the same Mint test no longer fails for ETag error. Though it failed later for an unsupported feature.

(3/15) Running awscli tests ... FAILED in 25 seconds
{
"name": "awscli",
"duration": 2108,
"function": "aws --endpoint-url http://host.containers.internal:9878 s3api head-object --bucket awscli-mint-test-bucket-28157 --key datafile-1-kB\n",
"status": "FAIL",
"error": "StorageClass was applied incorrectly"
}

So yes, looks like this PR is functionally correct. As long as the duplicate method and code style issue is fixed, this PR is ready.

@ivandika3 ivandika3 added the s3 S3 Gateway label Nov 5, 2025
Copy link
Contributor

@ivandika3 ivandika3 left a comment

Choose a reason for hiding this comment

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

@echonesis Thanks for the fix, left some comments.

Copy link
Contributor

@ivandika3 ivandika3 left a comment

Choose a reason for hiding this comment

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

@echonesis Thanks for the update. LGTM +1.

@ivandika3 ivandika3 changed the title HDDS-13737. S3 Etag JSON should be quoted HDDS-13737. S3 ETag JSON should be quoted Nov 5, 2025
@echonesis echonesis requested a review from jojochuang November 5, 2025 15:29
@ivandika3 ivandika3 merged commit 5ab59c9 into apache:master Nov 6, 2025
45 checks passed
@ivandika3
Copy link
Contributor

Thanks @echonesis for the patch and @jojochuang for the review.

@echonesis echonesis deleted the HDDS-13737 branch November 18, 2025 13:31
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.

4 participants