[GitHub] [ozone] errose28 commented on a change in pull request #3010: HDDS-6214. [Multi-Tenant] Fix KMS Encryption/Decryption

[GitBox <gi...@apache.org>]

errose28 commented on a change in pull request #3010:
URL: https://github.com/apache/ozone/pull/3010#discussion_r797898199



##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
##########
@@ -1124,17 +1124,15 @@ public TenantUserList listUsersInTenant(String tenantName, String prefix)
   }
 
   @Override
-  public OmVolumeArgs getS3Volume() throws IOException {
+  public GetS3VolumeResponse getS3VolumeInfo() throws IOException {
     final GetS3VolumeRequest request = GetS3VolumeRequest.newBuilder()
         .build();
     final OMRequest omRequest = createOMRequest(Type.GetS3Volume)
         .setGetS3VolumeRequest(request)
         .build();
     final OMResponse omResponse = submitRequest(omRequest);
-    final GetS3VolumeResponse resp = handleError(omResponse)
-        .getGetS3VolumeResponse();
 
-    return OmVolumeArgs.getFromProtobuf(resp.getVolumeInfo());
+    return handleError(omResponse).getGetS3VolumeResponse();

Review comment:
       This class is called the `Translator` because it translates proto java objects into the desired workable java object. We shouldn't return proto generated objects here, but translate them into a class in the `org.apache.hadoop.ozone.om.helpers` package like the rest of the methods here.

##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##########
@@ -437,12 +438,30 @@ public OzoneVolume getVolumeDetails(String volumeName)
   }
 
   @Override
-  public OzoneVolume getS3VolumeDetails() throws IOException {
-    OmVolumeArgs volume = ozoneManagerClient.getS3Volume();
+  public OzoneVolume getS3Volume() throws IOException {

Review comment:
       I think this method is better fit for `ObjectStore` than `RpcClient`. Putting it here seems to imply it corresponds to an RPC endpoint, when really it is a helper method that delegates to a differently named RPC call. Moving it to `ObjectStore` would make it function similarly to `ObjectStore#creates3Bucket`. There's no `createS3Bucket` RPC, but the method uses other RPCs under the hood. It is also not really part of the 'protocol', so moving it to `Objectstore` would remove it from the `ClientProtocol` interface and clear up some confusion.

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3077,12 +3078,14 @@ public TenantUserList listUsersInTenant(String tenantId, String prefix)
   }
 
   @Override
-  public OmVolumeArgs getS3Volume() throws IOException {
+  public GetS3VolumeResponse getS3VolumeInfo() throws IOException {
     // Unless the OM request contains S3 authentication info with an access
     // ID that corresponds to a tenant volume, the request will be directed
     // to the default S3 volume.
     String s3Volume = HddsClientUtils.getDefaultS3VolumeName(configuration);
     S3Authentication s3Auth = getS3Auth();
+    // TODO: Double check return value
+    String userPrincipal = Server.getRemoteUser().getShortUserName();

Review comment:
       What do we need to double check here? If we are merging this we should know if it is correct or not.

##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -1543,6 +1543,8 @@ message OmDBAccessInfo {
 
 message GetS3VolumeResponse {
   optional VolumeInfo volumeInfo = 1;
+  // Piggybacked user name (principal) response to be used for KMS operations
+  optional string userPrincipal = 2;

Review comment:
       Can we change the name of this proto to `GetS3VolumeInfoResponse`, and change the request name accordingly?  Then they match the name of the RPC call (getS3VolumeInfo) instead of the name of the client side wrapper method (getS3Volume). The current naming makes it seem like getS3Volume is the RPC call, which is a bit confusing if following the flow starting from the client.
   
   I added this proto on our branch, so changing the name now will not introduce any compatibility failures (and the build would fail if it did).




-- 
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