ArafatKhan2198 commented on code in PR #10814:
URL: https://github.com/apache/ozone/pull/10814#discussion_r3620127695
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java:
##########
@@ -243,6 +247,34 @@ protected OzoneVolume getVolume() throws IOException {
return client.getObjectStore().getS3Volume();
}
+ /**
+ * Maps a duplicate bucket create to the S3 error expected by AWS when the
+ * requester already owns the bucket name.
+ */
+ protected OS3Exception newDuplicateBucketError(String bucketName,
OMException cause) {
+ try {
+ OzoneBucket existingBucket = getVolume().getBucket(bucketName);
+ if (isSameBucketOwner(existingBucket.getOwner())) {
+ return newError(BUCKET_ALREADY_OWNED_BY_YOU, bucketName, cause);
+ }
+ } catch (IOException ex) {
+ LOG.debug("Could not resolve duplicate bucket owner for {}", bucketName,
ex);
+ }
+ return newError(BUCKET_ALREADY_EXISTS, bucketName, cause);
+ }
+
+ private boolean isSameBucketOwner(String bucketOwner) {
Review Comment:
Correctness issue: `null == null` becomes “same owner”
```java
private boolean isSameBucketOwner(String bucketOwner) {
return Objects.equals(getRequestOwner(), bucketOwner);
}
```
`getRequestOwner()` explicitly returns `null` when `s3Auth` or its principal
is unavailable. If the stored bucket owner is also `null`,
`Objects.equals(null, null)` returns `true`, producing
`BucketAlreadyOwnedByYou` even though ownership was not established.
Unknown identity should use the conservative fallback, `BucketAlreadyExists`.
Suggested change:
```java
private boolean isSameBucketOwner(String bucketOwner) {
String requestOwner = getRequestOwner();
return requestOwner != null && requestOwner.equals(bucketOwner);
}
```
##########
hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestBucketPut.java:
##########
@@ -35,25 +38,37 @@
*/
public class TestBucketPut {
+ private static final String OTHER_BUCKET_OWNER = "other-s3-owner";
+
private String bucketName = OzoneConsts.BUCKET;
private BucketEndpoint bucketEndpoint;
+ private OzoneClient clientStub;
@BeforeEach
public void setup() throws Exception {
- OzoneClient clientStub = new OzoneClientStub();
+ clientStub = new OzoneClientStub();
bucketEndpoint = EndpointBuilder.newBucketEndpointBuilder()
.setClient(clientStub)
.build();
}
@Test
- public void testCreateBucketAndFailOnDuplicate() throws Exception {
+ public void testCreateBucketAndFailOnDuplicateWithSameOWNER() throws
Exception {
Response response = bucketEndpoint.put(bucketName, null);
assertEquals(HTTP_OK, response.getStatus());
assertNotNull(response.getLocation());
- // Create-bucket on an existing bucket fails
+ assertErrorResponse(BUCKET_ALREADY_OWNED_BY_YOU,
+ () -> bucketEndpoint.put(bucketName, null));
+ }
+
+ @Test
+ public void testCreateBucketAndFailOnDuplicateWithDifferentOwner() throws
Exception {
Review Comment:
Unit test does not exercise a real owner match
`testCreateBucketAndFailOnDuplicateWithSameOWNER()` currently succeeds
because:
- The mocked `SignatureInfo` has no access ID/principal.
- The stub-created bucket has no owner.
- Therefore, the test verifies `null == null`, not an actual owner match.
This is also the currently unresolved PR review comment.
The test should set a non-null requester principal and pre-create a bucket
with the matching owner:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
