You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "hevinhsu (via GitHub)" <gi...@apache.org> on 2025/04/22 01:15:52 UTC

[PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

hevinhsu opened a new pull request, #8315:
URL: https://github.com/apache/ozone/pull/8315

   ## What changes were proposed in this pull request?
   Add owner field in the S3 listBuckets request.
   
   ## What is the link to the Apache JIRA
   https://issues.apache.org/jira/browse/HDDS-11298
   
   ## How was this patch tested?
   https://github.com/hevinhsu/ozone/actions/runs/14570769206
   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "hevinhsu (via GitHub)" <gi...@apache.org>.
hevinhsu commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2079166899


##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/S3ClientFactory.java:
##########
@@ -158,4 +160,54 @@ public S3Client createS3ClientV2(boolean enablePathStyle) throws Exception {
         .forcePathStyle(enablePathStyle)
         .build();
   }
+
+  /**
+   * Creates an S3AsyncClient (AWS SDK V2) with path style access enabled.
+   *
+   * @return S3AsyncClient
+   * @throws URISyntaxException if there is an error creating the client
+   */
+  public S3AsyncClient createS3AsyncClientV2() throws Exception {

Review Comment:
   Thanks for the reviewe.
   I'll remove ```S3AsyncClient``` in this PR.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2055203551


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -61,6 +62,9 @@ public Response get()
         throw e;
       }
 
+      OzoneVolume volume = getVolume();
+      response.setOwner(new S3Owner(volume.getName(), volume.getName()));

Review Comment:
   I think we can follow the the current convention of setting both arguments to the volume owner name similar to the other S3Owner usage.
   
   From what I see, the unique account ID is coupled to the specific IAM implementation that will generate a unique ID for each user account. However, since Hadoop authentication (UserGroupInformation) do not have this concept of unique account ID, we cannot be fully compatible with this format. I'm not sure about Ranger though.
   
   Additionally, we need to write tests for this since we don't know whether S3 client will actually enforce this format. If it is, then we have to adhere to it. In that case, maybe we can write a simple hash function to generate this 64-character encoded unique user name, but this won't be unique for user with the same user name (which should be fine).
   
   However, you can see at my overall comment that our ListBuckets implementation is not really compatible with the S3 API specification in the first place.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2069670123


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -61,6 +62,9 @@ public Response get()
         throw e;
       }
 
+      OzoneVolume volume = getVolume();

Review Comment:
   getVolume() is an RPC to Ozone Manager, and listS3Buckets(null) (above) internally invokes getVolume().
   So this is redundant and put extra latency/overhead at OM. Can we refactor the code so getVolume() is called only once.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2055076240


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -61,6 +62,9 @@ public Response get()
         throw e;
       }
 
+      OzoneVolume volume = getVolume();
+      response.setOwner(new S3Owner(volume.getName(), volume.getName()));

Review Comment:
   ```suggestion
         response.setOwner(new S3Owner(0, volume.getOwner()));
   ```
   
   The first argument of S3Owner is ID, which is a 64-character encoded unique user name. We dont' support it in Ozone. https://docs.aws.amazon.com/accounts/latest/reference/manage-acct-identifiers.html
   
   @ivandika3 any ideas about the owner id?



##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ListBucketResponse.java:
##########
@@ -39,6 +39,9 @@ public class ListBucketResponse {
   @XmlElement(name = "Bucket")
   private List<BucketMetadata> buckets = new ArrayList<>();
 
+  @XmlElement(name = "Owner")
+  private S3Owner owner;

Review Comment:
   Unrelated, but S3Owner javadoc is not useful. We should fix it in another task.
   
       /**
        * TODO: javadoc.
        */
       @XmlAccessorType(XmlAccessType.FIELD)
       @XmlRootElement(name = "Owner")
       public class S3Owner {



##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -61,6 +62,9 @@ public Response get()
         throw e;
       }
 
+      OzoneVolume volume = getVolume();
+      response.setOwner(new S3Owner(volume.getName(), volume.getName()));

Review Comment:
   this line doesn't look right. The owner is the user owner of the volume, not the name of the volume



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2069674677


##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/S3ClientFactory.java:
##########
@@ -158,4 +160,54 @@ public S3Client createS3ClientV2(boolean enablePathStyle) throws Exception {
         .forcePathStyle(enablePathStyle)
         .build();
   }
+
+  /**
+   * Creates an S3AsyncClient (AWS SDK V2) with path style access enabled.
+   *
+   * @return S3AsyncClient
+   * @throws URISyntaxException if there is an error creating the client
+   */
+  public S3AsyncClient createS3AsyncClientV2() throws Exception {

Review Comment:
   Is S3AsyncClient required here? 



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "hevinhsu (via GitHub)" <gi...@apache.org>.
hevinhsu commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2079169673


##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -152,116 +133,6 @@ public void testPutObject() {
     assertEquals("\"37b51d194a7513e45b56f6524f2d51f2\"", getObjectResponse.eTag());
   }
 
-  @Test
-  public void testListObjectsMany() throws Exception {
-    testListObjectsMany(false);
-  }
-
-  @Test
-  public void testListObjectsManyV2() throws Exception {
-    testListObjectsMany(true);
-  }

Review Comment:
   Oops, sorry about that. I'll fix it soon.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #8315:
URL: https://github.com/apache/ozone/pull/8315#issuecomment-2822758459

   @hevinhsu thanks for opening a PR. Do you have an Apache jira id? Id' like to add you as the assignee. Thanks.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2055197163


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -61,6 +62,9 @@ public Response get()
         throw e;
       }
 
+      OzoneVolume volume = getVolume();
+      response.setOwner(new S3Owner(volume.getName(), volume.getName()));

Review Comment:
   I think we can make it simple. See how MinIO does it: https://github.com/minio/minio/issues/4553
   Ceph S3 does not compute a user name hash either.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2055203551


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -61,6 +62,9 @@ public Response get()
         throw e;
       }
 
+      OzoneVolume volume = getVolume();
+      response.setOwner(new S3Owner(volume.getName(), volume.getName()));

Review Comment:
   I think we can follow the the current convention of setting both arguments to the volume owner name similar to the other S3Owner usage.
   
   From what I see, the unique account ID is coupled to the specific IAM implementation that will generate a unique ID for each user account. However, since Hadoop authentication (UserGroupInformation) do not have this concept of unique account ID, we cannot be fully compatible with this format. I'm not sure about Ranger though.
   
   Additionally, we need to write tests for this since we don't know whether S3 client will actually enforce this format. If it is, then we have to adhere to it. In that case, maybe we can write a simple hash function to generate this 64-character encoded unique user name, but this won't be unique for user with the same user name (which should be fine).
   
   However, you can see at my overall comment that our ListBuckets implementation is not really compatible with the S3 API specification in the first place.
   
   Edit: We can follow @jojochuang suggestion to follower other system's logic.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #8315:
URL: https://github.com/apache/ozone/pull/8315#issuecomment-2824827523

   > The current situation is that currently of listBuckets will list all the buckets under the /s3v volume, whereas the ListBuckets API specification will only list all the buckets owned by the user. If we want to follow this, then the of listBuckets flow needs to be changed to filter our all the buckets that the user does not own, this might cause regressions on some users since the buckets that were returned before are now excluded (most users will not be able to list the buckets at all).
   
   I think this needs some clarification.
   
   If ACL is enabled, it should forbid users from listing all buckets: https://github.com/apache/ozone/blob/42133076373dd3c1b72f0f8028771166a0594b50/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L2908-L2912


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "hevinhsu (via GitHub)" <gi...@apache.org>.
hevinhsu commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2072752436


##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/S3ClientFactory.java:
##########
@@ -158,4 +160,54 @@ public S3Client createS3ClientV2(boolean enablePathStyle) throws Exception {
         .forcePathStyle(enablePathStyle)
         .build();
   }
+
+  /**
+   * Creates an S3AsyncClient (AWS SDK V2) with path style access enabled.
+   *
+   * @return S3AsyncClient
+   * @throws URISyntaxException if there is an error creating the client
+   */
+  public S3AsyncClient createS3AsyncClientV2() throws Exception {

Review Comment:
   Hi,
   I’ve implemented the S3AsyncClient based on the TODO in this file: [link](https://github.com/apache/ozone/blob/master/hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java#L93-L97)
   
   I haven’t worked on the crt-based-s3-client test yet, since it would require introducing a new dependency.
   
   Just wanted to confirm—are both of these tests expected to be included as part of this issue?
   
   Thanks!



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2079152466


##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java:
##########
@@ -152,116 +133,6 @@ public void testPutObject() {
     assertEquals("\"37b51d194a7513e45b56f6524f2d51f2\"", getObjectResponse.eTag());
   }
 
-  @Test
-  public void testListObjectsMany() throws Exception {
-    testListObjectsMany(false);
-  }
-
-  @Test
-  public void testListObjectsManyV2() throws Exception {
-    testListObjectsMany(true);
-  }

Review Comment:
   Seems there is a mistake when merging to master and all of the `AbstractS3SDKV2Tests` changes in https://github.com/apache/ozone/pull/8356 were removed. Please kindly revert all the deletions in this file.



##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v1/AbstractS3SDKV1Tests.java:
##########
@@ -249,7 +250,7 @@ public void testBucketACLOperations() {
   }
 
   @Test
-  public void testListBuckets() {
+  public void testListBuckets() throws IOException {

Review Comment:
   We can add this `testListBuckets` to `AbstractS3SDKV2Tests`.



##########
hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/S3ClientFactory.java:
##########
@@ -158,4 +160,54 @@ public S3Client createS3ClientV2(boolean enablePathStyle) throws Exception {
         .forcePathStyle(enablePathStyle)
         .build();
   }
+
+  /**
+   * Creates an S3AsyncClient (AWS SDK V2) with path style access enabled.
+   *
+   * @return S3AsyncClient
+   * @throws URISyntaxException if there is an error creating the client
+   */
+  public S3AsyncClient createS3AsyncClientV2() throws Exception {

Review Comment:
   @hevinhsu Thanks for considering `S3AsyncClient`. I think we can introduce `S3AsyncClient` in another task which will add the S3 Async client in all existing tests in  `AbstractS3SDKV2Tests`. We can exclude `S3AsyncClient` and  focus on the synchronous `S3Client` for this patch.



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "hevinhsu (via GitHub)" <gi...@apache.org>.
hevinhsu commented on code in PR #8315:
URL: https://github.com/apache/ozone/pull/8315#discussion_r2079146038


##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/RootEndpoint.java:
##########
@@ -61,6 +62,9 @@ public Response get()
         throw e;
       }
 
+      OzoneVolume volume = getVolume();

Review Comment:
   @jojochuang  Thanks for the feedback!
   
   I'm currently using a functional interface to inject the owner into the response. This allows me to reuse the ```getVolume()``` error handling logic inside BaseEndpoint and avoid passing the response object into it.
   
   The goal is to keep error handling consistent and isolate response construction from the base layer.
   
   Could you help confirm if this approach aligns with our design expectations?



-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-11298. Support owner field in the S3 listBuckets request [ozone]

Posted by "hevinhsu (via GitHub)" <gi...@apache.org>.
hevinhsu commented on PR #8315:
URL: https://github.com/apache/ozone/pull/8315#issuecomment-2875556538

   Thanks everyone for the thoughtful feedback!
    
   I've:
   
   - Added acceptance test
   - Added integration tests for both S3 SDK v1 and v2
   - Fixed incorrect merge code
    
   @jojochuang @ivandika3 
   
   Feel free to take another look when you get a chance — let me know if there’s anything else I should address.
   
    


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org