From 3ebd57ad9a7cf7d833fc8f91bb06a8603b06d869 Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Wed, 18 Jun 2025 15:47:22 +0800 Subject: [PATCH 1/8] Refactor: extract verify logic --- .../hadoop/ozone/s3/endpoint/S3Owner.java | 37 +++++++------------ 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java index f9baf0d5b7ff..6fe6d25cd5e2 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java @@ -96,19 +96,7 @@ public String toString() { */ public static void verifyBucketOwnerCondition(HttpHeaders headers, String bucketName, String bucketOwner) throws OS3Exception { - if (headers == null || bucketOwner == null) { - return; - } - - final String expectedBucketOwner = headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER); - - if (StringUtils.isEmpty(expectedBucketOwner)) { - return; - } - if (expectedBucketOwner.equals(bucketOwner)) { - return; - } - throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, bucketName); + verify(headers, S3Consts.EXPECTED_BUCKET_OWNER_HEADER, bucketOwner, bucketName); } /** @@ -125,19 +113,22 @@ public static void verifyBucketOwnerConditionOnCopyOperation(HttpHeaders headers String sourceOwner, String destBucketName, String destOwner) throws OS3Exception { - if (headers == null) { + verify(headers, S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER, sourceOwner, sourceBucketName); + verify(headers, S3Consts.EXPECTED_BUCKET_OWNER_HEADER, destOwner, destBucketName); + } + + private static void verify(HttpHeaders headers, String headerKey, String actualOwner, String bucketName) + throws OS3Exception { + if (headers == null || actualOwner == null) { return; } - - final String expectedSourceOwner = headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER); - final String expectedDestOwner = headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER); - - if (expectedSourceOwner != null && sourceOwner != null && !sourceOwner.equals(expectedSourceOwner)) { - throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, sourceBucketName); + final String expectedBucketOwner = headers.getHeaderString(headerKey); + if (StringUtils.isEmpty(expectedBucketOwner)) { + return; } - - if (expectedDestOwner != null && destOwner != null && !destOwner.equals(expectedDestOwner)) { - throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, destBucketName); + if (expectedBucketOwner.equals(actualOwner)) { + return; } + throw S3ErrorTable.newError(S3ErrorTable.BUCKET_OWNER_MISMATCH, bucketName); } } From 195d047385e30cd8604820fa85a668559d2717e2 Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Wed, 18 Jun 2025 15:52:38 +0800 Subject: [PATCH 2/8] Add hasBucketOwnershipVerificationCondition --- .../hadoop/ozone/s3/endpoint/S3Owner.java | 20 ++++++++++ .../hadoop/ozone/s3/endpoint/TestS3Owner.java | 37 +++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java index 6fe6d25cd5e2..13997af615e1 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java @@ -86,6 +86,26 @@ public String toString() { '}'; } + /** + * Checks whether the HTTP headers contain a bucket owner condition, + * specifically if either the `expected-bucket-owner` or + * `expected-source-bucket-owner` header is present and not empty. + * + * @param headers the HTTP headers to check + * @return true if either bucket owner condition header is present and not empty, false otherwise + */ + public static boolean hasBucketOwnerCondition(HttpHeaders headers) { + if (headers == null) { + return false; + } + final String expectedBucketOwner = headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER); + if (!StringUtils.isEmpty(expectedBucketOwner)) { + return true; + } + final String expectedSourceBucketOwner = headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER); + return !StringUtils.isEmpty(expectedSourceBucketOwner); + } + /** * Verify the bucket owner condition. * diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java index c47759e6369f..66783c6b315c 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java @@ -46,6 +46,43 @@ public void setup() { headers = mock(HttpHeaders.class); } + @Test + public void testHasBucketOwnershipVerificationConditionsWithNullHeaders() { + assertThat(S3Owner.hasBucketOwnershipVerificationConditions(null)).isFalse(); + } + + @Test + public void testHasBucketOwnershipVerificationConditionsWithEmptyHeaders() { + when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(null); + when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(null); + assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isFalse(); + + when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(""); + when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(""); + assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isFalse(); + } + + @Test + public void testHasBucketOwnerConditionWithExpectedBucketOwnerVerificationPresent() { + when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn("owner1"); + when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(null); + assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isTrue(); + } + + @Test + public void testHasBucketOwnerConditionWithExpectedSourceBucketOwnerVerificationPresent() { + when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(null); + when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn("owner2"); + assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isTrue(); + } + + @Test + public void testHasBucketOwnershipVerificationConditionsWithBothPresent() { + when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn("owner1"); + when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn("owner2"); + assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isTrue(); + } + @Test public void testHeaderIsNull() { assertDoesNotThrow(() -> S3Owner.verifyBucketOwnerCondition(null, SOURCE_BUCKET_NAME, "test")); From c1da26d279c7a8e1784f41d98de49e05ef8f5300 Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Wed, 18 Jun 2025 15:54:05 +0800 Subject: [PATCH 3/8] Feat: reduce getBucket api call --- .../ozone/s3/endpoint/BucketEndpoint.java | 23 +++--- .../ozone/s3/endpoint/ObjectEndpoint.java | 71 ++++++++++--------- .../hadoop/ozone/s3/endpoint/S3Owner.java | 4 +- .../ozone/s3/endpoint/TestBucketAcl.java | 24 ++++--- .../s3/endpoint/TestPermissionCheck.java | 2 +- 5 files changed, 65 insertions(+), 59 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java index cd3739c4023d..b7e0b15672b8 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java @@ -138,7 +138,7 @@ public Response get( S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); if (aclMarker != null) { s3GAction = S3GAction.GET_ACL; - S3BucketAcl result = getAcl(bucketName); + S3BucketAcl result = getAcl(bucket); getMetrics().updateGetAclSuccessStats(startNanos); AUDIT.logReadSuccess( buildAuditMessageForSuccess(s3GAction, getAuditParameters())); @@ -147,7 +147,7 @@ public Response get( if (uploads != null) { s3GAction = S3GAction.LIST_MULTIPART_UPLOAD; - return listMultipartUploads(bucketName, prefix, keyMarker, uploadIdMarker, maxUploads); + return listMultipartUploads(bucket, prefix, keyMarker, uploadIdMarker, maxUploads); } maxKeys = validateMaxKeys(maxKeys); @@ -345,7 +345,7 @@ public Response put(@PathParam("bucket") String bucketName, } public Response listMultipartUploads( - String bucketName, + OzoneBucket bucket, String prefix, String keyMarker, String uploadIdMarker, @@ -360,14 +360,12 @@ public Response listMultipartUploads( long startNanos = Time.monotonicNowNanos(); S3GAction s3GAction = S3GAction.LIST_MULTIPART_UPLOAD; - OzoneBucket bucket = getBucket(bucketName); - try { OzoneMultipartUploadList ozoneMultipartUploadList = bucket.listMultipartUploads(prefix, keyMarker, uploadIdMarker, maxUploads); ListMultipartUploadsResult result = new ListMultipartUploadsResult(); - result.setBucket(bucketName); + result.setBucket(bucket.getName()); result.setKeyMarker(keyMarker); result.setUploadIdMarker(uploadIdMarker); result.setNextKeyMarker(ozoneMultipartUploadList.getNextKeyMarker()); @@ -441,8 +439,10 @@ public Response delete(@PathParam("bucket") String bucketName) S3GAction s3GAction = S3GAction.DELETE_BUCKET; try { - OzoneBucket bucket = getBucket(bucketName); - S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); + if (S3Owner.hasBucketOwnershipVerificationConditions(headers)) { + OzoneBucket bucket = getBucket(bucketName); + S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); + } deleteS3Bucket(bucketName); } catch (OMException ex) { AUDIT.logWriteFailure( @@ -540,12 +540,12 @@ public MultiDeleteResponse multiDelete(@PathParam("bucket") String bucketName, *

* see: https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketAcl.html */ - public S3BucketAcl getAcl(String bucketName) + public S3BucketAcl getAcl(OzoneBucket bucket) throws OS3Exception, IOException { long startNanos = Time.monotonicNowNanos(); S3BucketAcl result = new S3BucketAcl(); + String bucketName = bucket.getName(); try { - OzoneBucket bucket = getBucket(bucketName); S3Owner owner = S3Owner.of(bucket.getOwner()); result.setOwner(owner); @@ -573,9 +573,6 @@ public S3BucketAcl getAcl(String bucketName) } else { throw newError(S3ErrorTable.INTERNAL_ERROR, bucketName, ex); } - } catch (OS3Exception ex) { - getMetrics().updateGetAclFailureStats(startNanos); - throw ex; } } diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java index 02482023e5df..a76d6456eb66 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java @@ -244,11 +244,10 @@ public Response put( } OzoneVolume volume = getVolume(); OzoneBucket bucket = volume.getBucket(bucketName); - String bucketOwner = bucket.getOwner(); - S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucketOwner); + S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); if (taggingMarker != null) { s3GAction = S3GAction.PUT_OBJECT_TAGGING; - return putObjectTagging(volume, bucketName, keyPath, body); + return putObjectTagging(bucket, keyPath, body); } if (uploadID != null && !uploadID.equals("")) { @@ -258,7 +257,7 @@ public Response put( s3GAction = S3GAction.CREATE_MULTIPART_KEY_BY_COPY; } // If uploadID is specified, it is a request for upload part - return createMultipartKey(volume, bucketName, keyPath, length, + return createMultipartKey(volume, bucket, keyPath, length, partNumber, uploadID, body, perf); } @@ -434,14 +433,14 @@ public Response get( S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); if (taggingMarker != null) { s3GAction = S3GAction.GET_OBJECT_TAGGING; - return getObjectTagging(bucketName, keyPath); + return getObjectTagging(bucket, keyPath); } if (uploadId != null) { // When we have uploadId, this is the request for list Parts. s3GAction = S3GAction.LIST_PARTS; int partMarker = parsePartNumberMarker(partNumberMarker); - Response response = listParts(bucketName, keyPath, uploadId, + Response response = listParts(bucket, keyPath, uploadId, partMarker, maxParts, perf); AUDIT.logReadSuccess(buildAuditMessageForSuccess(s3GAction, getAuditParameters(), perf)); @@ -626,8 +625,10 @@ public Response head( OzoneKey key; try { - OzoneBucket bucket = getBucket(bucketName); - S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); + if (S3Owner.hasBucketOwnershipVerificationConditions(headers)) { + OzoneBucket bucket = getBucket(bucketName); + S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); + } key = getClientProtocol().headS3Object(bucketName, keyPath); isFile(keyPath, key); @@ -749,8 +750,10 @@ public Response delete( try { OzoneVolume volume = getVolume(); - OzoneBucket bucket = volume.getBucket(bucketName); - S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); + if (S3Owner.hasBucketOwnershipVerificationConditions(headers)) { + OzoneBucket bucket = volume.getBucket(bucketName); + S3Owner.verifyBucketOwnerCondition(headers, bucketName, bucket.getOwner()); + } if (taggingMarker != null) { s3GAction = S3GAction.DELETE_OBJECT_TAGGING; return deleteObjectTagging(volume, bucketName, keyPath); @@ -958,13 +961,14 @@ public Response completeMultipartUpload(@PathParam("bucket") String bucket, } @SuppressWarnings({"checkstyle:MethodLength", "checkstyle:ParameterNumber"}) - private Response createMultipartKey(OzoneVolume volume, String bucket, + private Response createMultipartKey(OzoneVolume volume, OzoneBucket ozoneBucket, String key, long length, int partNumber, String uploadID, final InputStream body, PerformanceStringBuilder perf) throws IOException, OS3Exception { long startNanos = Time.monotonicNowNanos(); String copyHeader = null; DigestInputStream digestInputStream = null; + final String bucketName = ozoneBucket.getName(); try { String amzDecodedLength = headers.getHeaderString(DECODED_CONTENT_LENGTH_HEADER); S3ChunkInputStreamInfo chunkInputStreamInfo = getS3ChunkInputStreamInfo( @@ -975,7 +979,6 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, copyHeader = headers.getHeaderString(COPY_SOURCE_HEADER); String storageType = headers.getHeaderString(STORAGE_CLASS_HEADER); String storageConfig = headers.getHeaderString(CUSTOM_METADATA_HEADER_PREFIX + STORAGE_CONFIG_HEADER); - final OzoneBucket ozoneBucket = volume.getBucket(bucket); ReplicationConfig replicationConfig = getReplicationConfig(ozoneBucket, storageType, storageConfig); @@ -1000,9 +1003,11 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, Pair result = parseSourceHeader(copyHeader); String sourceBucket = result.getLeft(); String sourceKey = result.getRight(); - String sourceBucketOwner = volume.getBucket(sourceBucket).getOwner(); - S3Owner.verifyBucketOwnerConditionOnCopyOperation(headers, sourceBucket, sourceBucketOwner, bucket, - ozoneBucket.getOwner()); + if (S3Owner.hasBucketOwnershipVerificationConditions(headers)) { + String sourceBucketOwner = volume.getBucket(sourceBucket).getOwner(); + S3Owner.verifyBucketOwnerConditionOnCopyOperation(headers, sourceBucket, sourceBucketOwner, bucketName, + ozoneBucket.getOwner()); + } OzoneKeyDetails sourceKeyDetails = getClientProtocol().getKeyDetails( volume.getName(), sourceBucket, sourceKey); @@ -1040,7 +1045,7 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, + rangeHeader.getStartOffset() + " actual: " + skipped); } try (OzoneOutputStream ozoneOutputStream = getClientProtocol() - .createMultipartKey(volume.getName(), bucket, key, length, + .createMultipartKey(volume.getName(), bucketName, key, length, partNumber, uploadID)) { metadataLatencyNs = getMetrics().updateCopyKeyMetadataStats(startNanos); @@ -1052,7 +1057,7 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, } } else { try (OzoneOutputStream ozoneOutputStream = getClientProtocol() - .createMultipartKey(volume.getName(), bucket, key, length, + .createMultipartKey(volume.getName(), bucketName, key, length, partNumber, uploadID)) { metadataLatencyNs = getMetrics().updateCopyKeyMetadataStats(startNanos); @@ -1069,7 +1074,7 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, } else { long putLength; try (OzoneOutputStream ozoneOutputStream = getClientProtocol() - .createMultipartKey(volume.getName(), bucket, key, length, + .createMultipartKey(volume.getName(), bucketName, key, length, partNumber, uploadID)) { metadataLatencyNs = getMetrics().updatePutKeyMetadataStats(startNanos); @@ -1112,7 +1117,7 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, if (ex.getResult() == ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR) { throw newError(NO_SUCH_UPLOAD, uploadID, ex); } else if (isAccessDenied(ex)) { - throw newError(S3ErrorTable.ACCESS_DENIED, bucket + "/" + key, ex); + throw newError(S3ErrorTable.ACCESS_DENIED, bucketName + "/" + key, ex); } else if (ex.getResult() == ResultCodes.INVALID_PART) { OS3Exception os3Exception = newError( S3ErrorTable.INVALID_ARGUMENT, String.valueOf(partNumber), ex); @@ -1132,7 +1137,7 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, /** * Returns response for the listParts request. * See: https://docs.aws.amazon.com/AmazonS3/latest/API/mpUploadListParts.html - * @param bucket + * @param ozoneBucket * @param key * @param uploadID * @param partNumberMarker @@ -1141,16 +1146,16 @@ private Response createMultipartKey(OzoneVolume volume, String bucket, * @throws IOException * @throws OS3Exception */ - private Response listParts(String bucket, String key, String uploadID, + private Response listParts(OzoneBucket ozoneBucket, String key, String uploadID, int partNumberMarker, int maxParts, PerformanceStringBuilder perf) throws IOException, OS3Exception { long startNanos = Time.monotonicNowNanos(); ListPartsResponse listPartsResponse = new ListPartsResponse(); + String bucketName = ozoneBucket.getName(); try { - OzoneBucket ozoneBucket = getBucket(bucket); OzoneMultipartUploadPartListParts ozoneMultipartUploadPartListParts = ozoneBucket.listParts(key, uploadID, partNumberMarker, maxParts); - listPartsResponse.setBucket(bucket); + listPartsResponse.setBucket(bucketName); listPartsResponse.setKey(key); listPartsResponse.setUploadID(uploadID); listPartsResponse.setMaxParts(maxParts); @@ -1185,7 +1190,7 @@ private Response listParts(String bucket, String key, String uploadID, throw newError(NO_SUCH_UPLOAD, uploadID, ex); } else if (isAccessDenied(ex)) { throw newError(S3ErrorTable.ACCESS_DENIED, - bucket + "/" + key + "/" + uploadID, ex); + bucketName + "/" + key + "/" + uploadID, ex); } throw ex; } @@ -1250,9 +1255,11 @@ private CopyObjectResponse copyObject(OzoneVolume volume, String sourceKey = result.getRight(); DigestInputStream sourceDigestInputStream = null; - String sourceBucketOwner = volume.getBucket(sourceBucket).getOwner(); - // The destBucket owner has already been checked in the caller method - S3Owner.verifyBucketOwnerConditionOnCopyOperation(headers, sourceBucket, sourceBucketOwner, null, null); + if (S3Owner.hasBucketOwnershipVerificationConditions(headers)) { + String sourceBucketOwner = volume.getBucket(sourceBucket).getOwner(); + // The destBucket owner has already been checked in the caller method + S3Owner.verifyBucketOwnerConditionOnCopyOperation(headers, sourceBucket, sourceBucketOwner, null, null); + } try { OzoneKeyDetails sourceKeyDetails = getClientProtocol().getKeyDetails( volume.getName(), sourceBucket, sourceKey); @@ -1435,7 +1442,7 @@ public static boolean checkCopySourceModificationTime( (lastModificationTime <= copySourceIfUnmodifiedSince); } - private Response putObjectTagging(OzoneVolume volume, String bucketName, String keyName, InputStream body) + private Response putObjectTagging(OzoneBucket bucket, String keyName, InputStream body) throws IOException, OS3Exception { long startNanos = Time.monotonicNowNanos(); S3Tagging tagging = null; @@ -1455,7 +1462,7 @@ private Response putObjectTagging(OzoneVolume volume, String bucketName, String ); try { - volume.getBucket(bucketName).putObjectTagging(keyName, tags); + bucket.putObjectTagging(keyName, tags); } catch (OMException ex) { if (ex.getResult() == ResultCodes.INVALID_REQUEST) { throw S3ErrorTable.newError(S3ErrorTable.INVALID_REQUEST, keyName); @@ -1473,12 +1480,10 @@ private Response putObjectTagging(OzoneVolume volume, String bucketName, String return Response.ok().build(); } - private Response getObjectTagging(String bucketName, String keyName) throws IOException { + private Response getObjectTagging(OzoneBucket bucket, String keyName) throws IOException { long startNanos = Time.monotonicNowNanos(); - OzoneVolume volume = getVolume(); - - Map tagMap = volume.getBucket(bucketName).getObjectTagging(keyName); + Map tagMap = bucket.getObjectTagging(keyName); getMetrics().updateGetObjectTaggingSuccessStats(startNanos); return Response.ok(S3Tagging.fromMap(tagMap), MediaType.APPLICATION_XML_TYPE).build(); diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java index 13997af615e1..84c9676c297a 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java @@ -87,14 +87,14 @@ public String toString() { } /** - * Checks whether the HTTP headers contain a bucket owner condition, + * Checks whether the HTTP headers contain a bucket ownership verification conditions, * specifically if either the `expected-bucket-owner` or * `expected-source-bucket-owner` header is present and not empty. * * @param headers the HTTP headers to check * @return true if either bucket owner condition header is present and not empty, false otherwise */ - public static boolean hasBucketOwnerCondition(HttpHeaders headers) { + public static boolean hasBucketOwnershipVerificationConditions(HttpHeaders headers) { if (headers == null) { return false; } diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java index 84bc414e37ee..c7deb3334fa5 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java @@ -32,6 +32,7 @@ import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.Response; import org.apache.hadoop.ozone.OzoneConsts; +import org.apache.hadoop.ozone.client.OzoneBucket; import org.apache.hadoop.ozone.client.OzoneClient; import org.apache.hadoop.ozone.client.OzoneClientStub; import org.apache.hadoop.ozone.client.OzoneVolume; @@ -53,12 +54,14 @@ public class TestBucketAcl { private Map parameterMap; private HttpHeaders headers; private BucketEndpoint bucketEndpoint; + private OzoneBucket bucket; private static final String ACL_MARKER = "acl"; @BeforeEach public void setup() throws IOException { client = new OzoneClientStub(); client.getObjectStore().createS3Bucket(BUCKET_NAME); + bucket = client.getObjectStore().getS3Bucket(BUCKET_NAME); servletRequest = mock(HttpServletRequest.class); parameterMap = mock(Map.class); @@ -106,7 +109,7 @@ public void testRead() throws Exception { Response response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); - S3BucketAcl getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + S3BucketAcl getResponse = bucketEndpoint.getAcl(bucket); assertEquals(1, getResponse.getAclList().getGrantList().size()); assertEquals(S3Acl.ACLType.READ.getValue(), getResponse.getAclList().getGrantList().get(0).getPermission()); @@ -120,7 +123,7 @@ public void testWrite() throws Exception { Response response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); - S3BucketAcl getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + S3BucketAcl getResponse = bucketEndpoint.getAcl(bucket); assertEquals(1, getResponse.getAclList().getGrantList().size()); assertEquals(S3Acl.ACLType.WRITE.getValue(), getResponse.getAclList().getGrantList().get(0).getPermission()); @@ -135,7 +138,7 @@ public void testReadACP() throws Exception { bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); S3BucketAcl getResponse = - bucketEndpoint.getAcl(BUCKET_NAME); + bucketEndpoint.getAcl(bucket); assertEquals(1, getResponse.getAclList().getGrantList().size()); assertEquals(S3Acl.ACLType.READ_ACP.getValue(), getResponse.getAclList().getGrantList().get(0).getPermission()); @@ -149,7 +152,7 @@ public void testWriteACP() throws Exception { Response response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); - S3BucketAcl getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + S3BucketAcl getResponse = bucketEndpoint.getAcl(bucket); assertEquals(1, getResponse.getAclList().getGrantList().size()); assertEquals(S3Acl.ACLType.WRITE_ACP.getValue(), getResponse.getAclList().getGrantList().get(0).getPermission()); @@ -163,7 +166,7 @@ public void testFullControl() throws Exception { Response response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); - S3BucketAcl getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + S3BucketAcl getResponse = bucketEndpoint.getAcl(bucket); assertEquals(1, getResponse.getAclList().getGrantList().size()); assertEquals(S3Acl.ACLType.FULL_CONTROL.getValue(), getResponse.getAclList().getGrantList().get(0).getPermission()); @@ -185,7 +188,7 @@ public void testCombination() throws Exception { Response response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); - S3BucketAcl getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + S3BucketAcl getResponse = bucketEndpoint.getAcl(bucket); assertEquals(5, getResponse.getAclList().getGrantList().size()); } @@ -198,7 +201,7 @@ public void testPutClearOldAcls() throws Exception { Response response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); - S3BucketAcl getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + S3BucketAcl getResponse = bucketEndpoint.getAcl(bucket); assertEquals(1, getResponse.getAclList().getGrantList().size()); assertEquals(S3Acl.ACLType.READ.getValue(), getResponse.getAclList().getGrantList().get(0).getPermission()); @@ -215,7 +218,7 @@ public void testPutClearOldAcls() throws Exception { response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, null); assertEquals(HTTP_OK, response.getStatus()); - getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + getResponse = bucketEndpoint.getAcl(bucket); assertEquals(1, getResponse.getAclList().getGrantList().size()); assertEquals(S3Acl.ACLType.WRITE.getValue(), getResponse.getAclList().getGrantList().get(0).getPermission()); @@ -242,7 +245,7 @@ public void testAclInBody() throws Exception { Response response = bucketEndpoint.put(BUCKET_NAME, ACL_MARKER, inputBody); assertEquals(HTTP_OK, response.getStatus()); - S3BucketAcl getResponse = bucketEndpoint.getAcl(BUCKET_NAME); + S3BucketAcl getResponse = bucketEndpoint.getAcl(bucket); assertEquals(2, getResponse.getAclList().getGrantList().size()); } @@ -252,7 +255,8 @@ public void testBucketNotExist() throws Exception { when(headers.getHeaderString(S3Acl.GRANT_READ)) .thenReturn(S3Acl.ACLIdentityType.USER.getHeaderType() + "=root"); OS3Exception e = assertThrows(OS3Exception.class, () -> - bucketEndpoint.getAcl("bucket-not-exist")); + bucketEndpoint.get("bucket-not-exist", null, null, null, 0, null, + null, null, null, ACL_MARKER, null, null, 0)); assertEquals(e.getHttpCode(), HTTP_NOT_FOUND); } } diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java index 81f6853bf73f..d279497f5631 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java @@ -154,7 +154,7 @@ public void testListMultiUpload() throws IOException { .setClient(client) .build(); OS3Exception e = assertThrows(OS3Exception.class, () -> - bucketEndpoint.listMultipartUploads("bucketName", "prefix", "", "", 10)); + bucketEndpoint.listMultipartUploads(bucket, "prefix", "", "", 10)); assertEquals(HTTP_FORBIDDEN, e.getHttpCode()); } From f0ad83ccc3ef86062b33ad9dc7349f0c82fe273c Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Wed, 18 Jun 2025 16:02:50 +0800 Subject: [PATCH 4/8] Update javadoc --- .../main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java index 84c9676c297a..13018d956d3b 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java @@ -92,7 +92,7 @@ public String toString() { * `expected-source-bucket-owner` header is present and not empty. * * @param headers the HTTP headers to check - * @return true if either bucket owner condition header is present and not empty, false otherwise + * @return true if either bucket ownership verificatoin condition header is present and not empty, false otherwise */ public static boolean hasBucketOwnershipVerificationConditions(HttpHeaders headers) { if (headers == null) { From f28bb159df1424bcbbb0e7599b1394856b819508 Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Wed, 18 Jun 2025 16:04:07 +0800 Subject: [PATCH 5/8] Fix typo --- .../main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java index 13018d956d3b..4f84bb0b4664 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java @@ -92,7 +92,7 @@ public String toString() { * `expected-source-bucket-owner` header is present and not empty. * * @param headers the HTTP headers to check - * @return true if either bucket ownership verificatoin condition header is present and not empty, false otherwise + * @return true if either bucket ownership verification condition header is present and not empty, false otherwise */ public static boolean hasBucketOwnershipVerificationConditions(HttpHeaders headers) { if (headers == null) { From 9faebcbee6afcc58ea53c09d92664a999de84017 Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Fri, 20 Jun 2025 09:56:20 +0800 Subject: [PATCH 6/8] Refactor: simplify test cases using @ParameterizedTest --- .../hadoop/ozone/s3/endpoint/TestS3Owner.java | 53 +++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java index 66783c6b315c..fe5b606a480c 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestS3Owner.java @@ -23,6 +23,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import java.util.stream.Stream; import javax.ws.rs.core.HttpHeaders; import org.apache.hadoop.ozone.s3.exception.OS3Exception; import org.apache.hadoop.ozone.s3.exception.S3ErrorTable; @@ -30,6 +31,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.NullAndEmptySource; /** @@ -46,40 +49,36 @@ public void setup() { headers = mock(HttpHeaders.class); } - @Test - public void testHasBucketOwnershipVerificationConditionsWithNullHeaders() { - assertThat(S3Owner.hasBucketOwnershipVerificationConditions(null)).isFalse(); + private static Stream noBucketOwnerSourceProvider() { + return Stream.of( + Arguments.of(null, null), + Arguments.of(null, ""), + Arguments.of("", null), + Arguments.of("", "") + ); } - @Test - public void testHasBucketOwnershipVerificationConditionsWithEmptyHeaders() { - when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(null); - when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(null); - assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isFalse(); - - when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(""); - when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(""); + @ParameterizedTest + @MethodSource("noBucketOwnerSourceProvider") + public void testHasBucketOwnershipVerificationConditionsFailed(String bucketOwner, String sourceBucketOwner) { + when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(bucketOwner); + when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(sourceBucketOwner); assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isFalse(); } - @Test - public void testHasBucketOwnerConditionWithExpectedBucketOwnerVerificationPresent() { - when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn("owner1"); - when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(null); - assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isTrue(); + private static Stream hasBucketOwnerSourceProvider() { + return Stream.of( + Arguments.of("owner1", null), + Arguments.of(null, "owner2"), + Arguments.of("owner1", "owner2") + ); } - @Test - public void testHasBucketOwnerConditionWithExpectedSourceBucketOwnerVerificationPresent() { - when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(null); - when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn("owner2"); - assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isTrue(); - } - - @Test - public void testHasBucketOwnershipVerificationConditionsWithBothPresent() { - when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn("owner1"); - when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn("owner2"); + @ParameterizedTest + @MethodSource("hasBucketOwnerSourceProvider") + public void testHasBucketOwnershipVerificationConditionsPass(String bucketOwner, String sourceBucketOwner) { + when(headers.getHeaderString(S3Consts.EXPECTED_BUCKET_OWNER_HEADER)).thenReturn(bucketOwner); + when(headers.getHeaderString(S3Consts.EXPECTED_SOURCE_BUCKET_OWNER_HEADER)).thenReturn(sourceBucketOwner); assertThat(S3Owner.hasBucketOwnershipVerificationConditions(headers)).isTrue(); } From 81993fbd76f787140097384b4ad99b9697f71678 Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Fri, 20 Jun 2025 14:44:39 +0800 Subject: [PATCH 7/8] Chore: cleanup unnecessary field due to main branch merge --- .../org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java index 605060828b66..84bc414e37ee 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketAcl.java @@ -32,7 +32,6 @@ import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.Response; import org.apache.hadoop.ozone.OzoneConsts; -import org.apache.hadoop.ozone.client.OzoneBucket; import org.apache.hadoop.ozone.client.OzoneClient; import org.apache.hadoop.ozone.client.OzoneClientStub; import org.apache.hadoop.ozone.client.OzoneVolume; @@ -54,14 +53,12 @@ public class TestBucketAcl { private Map parameterMap; private HttpHeaders headers; private BucketEndpoint bucketEndpoint; - private OzoneBucket bucket; private static final String ACL_MARKER = "acl"; @BeforeEach public void setup() throws IOException { client = new OzoneClientStub(); client.getObjectStore().createS3Bucket(BUCKET_NAME); - bucket = client.getObjectStore().getS3Bucket(BUCKET_NAME); servletRequest = mock(HttpServletRequest.class); parameterMap = mock(Map.class); From 6d4a04a1df4880e0d7b66dbf55aad7b7f31e1383 Mon Sep 17 00:00:00 2001 From: hevinhsu Date: Sat, 21 Jun 2025 20:47:28 +0800 Subject: [PATCH 8/8] Fix: fix unit test failed after merge master branch --- .../org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java index 77d0c5b630b0..ed75c68cb263 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java @@ -578,6 +578,9 @@ public S3BucketAcl getAcl(String bucketName) } else { throw newError(S3ErrorTable.INTERNAL_ERROR, bucketName, ex); } + } catch (OS3Exception ex) { + getMetrics().updateGetAclFailureStats(startNanos); + throw ex; } }