[GitHub] [ozone] smengcl commented on a change in pull request #2924: HDDS-6004. Use changes from HDDS-5881 for volume chroot.

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

smengcl commented on a change in pull request #2924:
URL: https://github.com/apache/ozone/pull/2924#discussion_r773255570



##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
##########
@@ -883,7 +882,7 @@ OzoneKey headObject(String volumeName, String bucketName,
    * of the S3 API implementation within Ozone.
    * @param s3Auth authentication information for each S3 API call.
    */
-  void setTheadLocalS3Auth(S3Auth s3Auth);
+  void setThreadLocalS3Auth(S3Auth s3Auth);

Review comment:
       We could leave this typo fix alone in our feature branch. Preferably do it in the master branch? `clearTheadLocalS3Auth` below also has such typo.

##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -1532,6 +1532,13 @@ message TenantRevokeAdminResponse {
     optional bool success = 1;  // TODO: Remove this field
 }
 
+message OmDBAccessInfo {
+  optional string principal = 1;
+  optional bool isAdmin = 2;
+  optional bool isDelegatedAdmin = 3;
+  optional string tenantId = 4;

Review comment:
       Just curious. Is there anything in particular about the order of these fields?
   
   If not let's keep the original order. And `principal -> userPrincipal`.
   
   ```suggestion
     optional string tenantId = 1;
     optional string userPrincipal = 2;
     optional bool isAdmin = 3;
     optional bool isDelegatedAdmin = 4;
   ```

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMAssignUserToTenantRequest.java
##########
@@ -325,21 +324,10 @@ public OMClientResponse validateAndUpdateCache(
           new CacheKey<>(accessId),
           new CacheValue<>(Optional.of(roleName), transactionLogIndex));
 
-      // Add to S3SecretTable.
-      // Note: S3SecretTable will be deprecated in the future.
+      // Add S3SecretTable cache entry
       acquiredS3SecretLock = omMetadataManager.getLock()
           .acquireWriteLock(S3_SECRET_LOCK, accessId);
 
-      // Expect accessId absence from S3SecretTable
-      // TODO: This table might be merged with tenantAccessIdTable later.
-      if (omMetadataManager.getS3SecretTable().isExist(accessId)) {
-        LOG.error("accessId '{}' already exists in S3SecretTable", accessId);
-        throw new OMException("accessId '" + accessId +
-            "' already exists in S3SecretTable",
-            OMException.ResultCodes.INVALID_REQUEST);
-      }

Review comment:
       Keep this but instead of throwing, print a warning (to the OM log) if the accessId already exists in the `getS3SecretTable`?

##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -1503,7 +1503,7 @@ message TenantRevokeAdminRequest {
 }
 
 message GetS3VolumeRequest {
-  optional string accessID = 1;
+

Review comment:
       I believe I need to add this `accessID` field back if we piggyback the `accessID -> userPrincipal` request for KMS encryption. It's alright to remove it here in this patch.

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3013,38 +3013,42 @@ public TenantUserList listUsersInTenant(String tenantId, String prefix)
   }
 
   @Override
-  public OmVolumeArgs getS3Volume(String accessID) throws IOException {
-
-    final String tenantId;
-    try {
-      tenantId = multiTenantManagr.getTenantForAccessID(accessID);
-      // TODO: Get volume name from DB. Do not assume the same. e.g.
-      //metadataManager.getTenantStateTable().get(tenantId)
-      //    .getBucketNamespaceName();
-      final String volumeName = tenantId;
-      if (LOG.isDebugEnabled()) {
-        LOG.debug("Get S3 volume request for access ID {} belonging to tenant" +
-                " {} is directed to the volume {}.", accessID, tenantId,
-            volumeName);
-      }
-      // This call performs acl checks and checks volume existence.
-      return getVolumeInfo(volumeName);
-
-    } catch (OMException ex) {
-      if (ex.getResult().equals(INVALID_ACCESSID)) {
-        // If the user is not associated with a tenant, they will use the
-        // default s3 volume.
-        String defaultS3volume =
-            HddsClientUtils.getDefaultS3VolumeName(configuration);
-
+  public OmVolumeArgs getS3Volume() 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();
+
+    if (s3Auth != null) {
+      String accessID = s3Auth.getAccessId();
+      Optional<String> optionalTenantId =
+          multiTenantManager.getTenantForAccessID(accessID);
+
+      if (optionalTenantId.isPresent()) {
+        String tenantId = optionalTenantId.get();
+        s3Volume = metadataManager.getTenantStateTable().get(tenantId)

Review comment:
       Add a null check before this line just in case `metadataManager.getTenantStateTable().get(tenantId)` returns null (in the case of inconsistent OM DB)?




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