[GitHub] [ozone] avijayanhwx commented on a change in pull request #2649: HDDS-5750. [Multi-Tenant] GetS3Secret should retrieve secret from new tables as well

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

avijayanhwx commented on a change in pull request #2649:
URL: https://github.com/apache/ozone/pull/2649#discussion_r713362714



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
##########
@@ -126,38 +129,48 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
     String kerberosID = updateGetS3SecretRequest.getKerberosID();
     try {
       String awsSecret = updateGetS3SecretRequest.getAwsSecret();
-      acquiredLock =
-         omMetadataManager.getLock().acquireWriteLock(S3_SECRET_LOCK,
-             kerberosID);
-
-      S3SecretValue s3SecretValue =
-          omMetadataManager.getS3SecretTable().get(kerberosID);
-
-      // If s3Secret for user is not in S3Secret table, add the Secret to cache.
-      if (s3SecretValue == null) {
-        omMetadataManager.getS3SecretTable().addCacheEntry(
-            new CacheKey<>(kerberosID),
-            new CacheValue<>(Optional.of(new S3SecretValue(kerberosID,
-                awsSecret)), transactionLogIndex));
+      // Note: We use the same S3_SECRET_LOCK for TenantAccessIdTable.
+      acquiredLock = omMetadataManager.getLock()
+          .acquireWriteLock(S3_SECRET_LOCK, kerberosID);
+
+      // Check multi-tenant table first: tenantAccessIdTable
+      final S3SecretValue assignS3SecretValue;
+      final OmDBAccessIdInfo omDBAccessIdInfo =
+          omMetadataManager.getTenantAccessIdTable().get(kerberosID);
+      if (omDBAccessIdInfo == null) {
+        // Not found in TenantAccessIdTable. Fallback to S3SecretTable.
+        final S3SecretValue s3SecretValue =
+            omMetadataManager.getS3SecretTable().get(kerberosID);
+
+        if (s3SecretValue == null) {
+          // Still not found in S3SecretTable. Will add new entry in this case.
+          assignS3SecretValue = new S3SecretValue(kerberosID, awsSecret);
+          // Add cache entry first.
+          omMetadataManager.getS3SecretTable().addCacheEntry(
+              new CacheKey<>(kerberosID),
+              new CacheValue<>(
+                  Optional.of(assignS3SecretValue), transactionLogIndex));
+        } else {
+          // Found in S3SecretTable.
+          awsSecret = s3SecretValue.getAwsSecret();
+          assignS3SecretValue = null;

Review comment:
       Can we initialize '**assignS3SecretValue**' to null, instead of setting to null in the 2 places?

##########
File path: hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/GetS3SecretHandler.java
##########
@@ -35,8 +35,8 @@
 public class GetS3SecretHandler extends S3Handler {
 
   @Option(names = "-u",
-      description = "Specify the user name to perform the operation on "
-          + "(admins only)'")
+      description = "Specify the user to perform the operation on "
+          + "(Admins only)'")

Review comment:
       Is there a plan to allow tenant admins to get secret for a tenant user? If the user has lost the secret, only an ozone admin can retrieve it then. Or, is there a plan to add a new 'tenant' based command for getting accessId / secret key for a user?




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