You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/05/23 18:56:27 UTC

[GitHub] [ozone] smengcl opened a new pull request, #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger Background Sync Service and tenant requests

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

   ## What changes were proposed in this pull request?
   
   - Add proper locking between Ranger Background Sync Service and tenant requests
   - Address (most) small bugs filed in https://issues.apache.org/jira/issues/?jql=labels%20%3D%20ozone-multitenancy.
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-6701
   
   ## How was this patch tested?
   
   - [ ] Existing tests should pass.


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

[GitHub] [ozone] smengcl merged pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
smengcl merged PR #3450:
URL: https://github.com/apache/ozone/pull/3450


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

[GitHub] [ozone] smengcl commented on a diff in pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3450:
URL: https://github.com/apache/ozone/pull/3450#discussion_r883190314


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java:
##########
@@ -183,246 +197,456 @@ public OMMetadataManager getOmMetadataManager() {
     return omMetadataManager;
   }
 
-  // TODO: Cleanup up this Java doc.
+  @Override
+  public TenantOp getAuthorizerOp() {
+    return authorizerOp;
+  }
+
+  @Override
+  public TenantOp getCacheOp() {
+    return cacheOp;
+  }
+
   /**
-   *  Algorithm
-   *  OM State :
-   *    - Validation (Part of Ratis Request)
-   *    - create volume {Part of RATIS request}
-   *    - Persistence to OM DB {Part of RATIS request}
-   *  Authorizer-plugin(Ranger) State :
-   *    - For every tenant create two user groups
-   *        # GroupTenantAllUsers
-   *        # GroupTenantAllAdmins
-   *
-   *    - For every tenant create two default policies
-   *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
-   *      part of validation in Ratis-Request. if the groups/policies exist
-   *      with the same name (Due to an earlier failed/success request), in
-   *      plugin, we just update in-memory-map here and return success.
-   *    - The job of cleanup of any half-done authorizer-plugin state is done
-   *      by a background thread.
-   *  Finally :
-   *    - Update all Maps maintained by Multi-Tenant-Manager
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param tenantId
-   * @param userRoleName
-   * @param adminRoleName
-   * @return Tenant
-   * @throws IOException
+   * Implements tenant authorizer operations.
    */
-  @Override
-  public Tenant createTenantAccessInAuthorizer(String tenantId,
-      String userRoleName, String adminRoleName)
-      throws IOException {
+  public class AuthorizerOp implements TenantOp {
+
+    private final MultiTenantAccessAuthorizer authorizer;
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    AuthorizerOp(MultiTenantAccessAuthorizer authorizer,
+        Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.authorizer = authorizer;
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
 
-    Tenant tenant = new OzoneTenant(tenantId);
-    try {
-      tenantCacheLock.writeLock().lock();
+    private void checkAcquiredAuthorizerWriteLock() throws OMException {
 
-      // Create admin role first
-      String adminRoleId = authorizer.createRole(adminRoleName, null);
-      tenant.addTenantAccessRole(adminRoleId);
-
-      // Then create user role, and add admin role as its delegated admin
-      String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
-      tenant.addTenantAccessRole(userRoleId);
-
-      BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
-      // Bucket namespace is volume
-      for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
-        String volumeName = volume.getVolumeName();
-
-        final OzoneTenantRolePrincipal userRole =
-            new OzoneTenantRolePrincipal(userRoleName);
-        final OzoneTenantRolePrincipal adminRole =
-            new OzoneTenantRolePrincipal(adminRoleName);
-
-        // Allow Volume List access
-        AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
-            volumeName, userRole, adminRole);
-        tenantVolumeAccessPolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
-        tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
-
-        // Allow Bucket Create within Volume
-        AccessPolicy tenantBucketCreatePolicy =
-            newDefaultBucketAccessPolicy(volumeName, userRole);
-        tenantBucketCreatePolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantBucketCreatePolicy));
-        tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+      // Check if lock is acquired by the current thread
+      if (!authorizerLock.isHeldByCurrentThread()) {
+        throw new OMException("Authorizer write lock must have been held "
+            + "before calling this method", INTERNAL_ERROR);
       }
+    }
+
+    /**
+     *  Algorithm
+     *  OM State :
+     *    - Validation (Part of Ratis Request)
+     *    - create volume {Part of RATIS request}
+     *    - Persistence to OM DB {Part of RATIS request}
+     *  Authorizer-plugin(Ranger) State :
+     *    - For every tenant create two user groups
+     *        # GroupTenantAllUsers
+     *        # GroupTenantAllAdmins
+     *
+     *    - For every tenant create two default policies
+     *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
+     *      part of validation in Ratis-Request. if the groups/policies exist
+     *      with the same name (Due to an earlier failed/success request), in
+     *      plugin, we just update in-memory-map here and return success.
+     *    - The job of cleanup of any half-done authorizer-plugin state is done
+     *      by a background thread.
+     *  Finally :
+     *    - Update all Maps maintained by Multi-Tenant-Manager
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param tenantId tenant name
+     * @param userRoleName user role name
+     * @param adminRoleName admin role name
+     * @return Tenant
+     * @throws IOException
+     */
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      Tenant tenant = new OzoneTenant(tenantId);
 
-      if (tenantCache.containsKey(tenantId)) {
-        LOG.warn("Cache entry for tenant '{}' somehow already exists, "
-            + "will be overwritten", tenantId);  // TODO: throw exception?
+      try {
+        // Create admin role first
+        String adminRoleId = authorizer.createRole(adminRoleName, null);
+        tenant.addTenantAccessRole(adminRoleId);
+
+        // Then create user role, and add admin role as its delegated admin
+        String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
+        tenant.addTenantAccessRole(userRoleId);
+
+        BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
+        // Bucket namespace is volume
+        for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
+          String volumeName = volume.getVolumeName();
+
+          final OzoneTenantRolePrincipal userRole =
+              new OzoneTenantRolePrincipal(userRoleName);
+          final OzoneTenantRolePrincipal adminRole =
+              new OzoneTenantRolePrincipal(adminRoleName);
+
+          // Allow Volume List access
+          AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
+              volumeName, userRole, adminRole);
+          tenantVolumeAccessPolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
+          tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
+
+          // Allow Bucket Create within Volume
+          AccessPolicy tenantBucketCreatePolicy =
+              newDefaultBucketAccessPolicy(volumeName, userRole);
+          tenantBucketCreatePolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantBucketCreatePolicy));
+          tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+        }
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
+    }
 
-      // TODO: Move tenantCache update to a separate call createTenantAccessInDB
-      //  createTenantAccessInAuthorizer is called preExecute to update Ranger
-      //  createTenantAccessInDB will be called in validateAndUpdateCache
-      //  Do the same to all other InAuthorizer calls as well.
-      // New entry in tenant cache
-      tenantCache.put(tenantId, new CachedTenantState(
-          tenantId, userRoleName, adminRoleName));
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      LOG.info("Deleting tenant policies and roles from Ranger: {}", tenant);
 
-    } catch (IOException e) {
       try {
-        removeTenantAccessFromAuthorizer(tenant);
-      } catch (IOException ignored) {
-        // Best effort cleanup.
+        for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
+          authorizer.deletePolicyByName(policy.getPolicyName());
+        }
+
+        for (String roleName : tenant.getTenantRoles()) {
+          authorizer.deleteRoleByName(roleName);
+        }
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
-      throw e;
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
-    return tenant;
-  }
 
-  @Override
-  public void removeTenantAccessFromAuthorizer(Tenant tenant)
-      throws IOException {
-    try {
-      tenantCacheLock.writeLock().lock();
-      for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
-        authorizer.deletePolicyById(policy.getPolicyID());
+    /**
+     *  Algorithm
+     *  Authorizer-plugin(Ranger) State :
+     *    - create User in Ranger DB
+     *    - For every user created
+     *        Add them to # GroupTenantAllUsers
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param userPrincipal
+     * @param tenantId
+     * @param accessId
+     * @throws IOException
+     */
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        // Does NOT update tenant cache here
+
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.assignUserToRole(userPrincipal, roleJsonStr, false);
+
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("roleId that the user is assigned to: {}", roleId);
+        }
+
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      for (String roleId : tenant.getTenantRoles()) {
-        authorizer.deleteRole(roleId);
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final OmDBAccessIdInfo omDBAccessIdInfo =
+            omMetadataManager.getTenantAccessIdTable().get(accessId);
+        if (omDBAccessIdInfo == null) {
+          throw new OMException(INVALID_ACCESS_ID);
+        }
+        final String tenantIdGot = omDBAccessIdInfo.getTenantId();
+        Preconditions.checkArgument(tenantIdGot.equals(tenantId));
+
+        final String userPrincipal = omDBAccessIdInfo.getUserPrincipal();
+
+        // Delete user from role in Ranger
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.revokeUserFromRole(userPrincipal, roleJsonStr);
+        Preconditions.checkNotNull(roleId);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      if (tenantCache.containsKey(tenant.getTenantId())) {
-        LOG.info("Removing tenant {} from in memory cached state",
-            tenant.getTenantId());
-        tenantCache.remove(tenant.getTenantId());
+    }
+
+    @Override
+    public void assignTenantAdmin(String accessId, boolean delegated)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.assignUserToRole(
+            userPrincipal, roleJsonStr, delegated);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
+
+    @Override
+    public void revokeTenantAdmin(String accessId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.revokeUserFromRole(
+            userPrincipal, roleJsonStr);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
+      }
+    }
+
   }
 
   /**
-   *  Algorithm
-   *  Authorizer-plugin(Ranger) State :
-   *    - create User in Ranger DB
-   *    - For every user created
-   *        Add them to # GroupTenantAllUsers
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param principal
-   * @param tenantId
-   * @param accessId
-   * @return Tenant, or null on error
-   * @throws IOException
+   * Implements tenant cache operations.
    */
-  @Override
-  public String assignUserToTenant(BasicUserPrincipal principal,
-      String tenantId, String accessId) throws IOException {
+  public class CacheOp implements TenantOp {
 
-    final CachedAccessIdInfo cacheEntry =
-        new CachedAccessIdInfo(principal.getName(), false);
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    CacheOp(Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
+
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName) {
 
-    try {
       tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.warn("Cache entry for tenant '{}' already exists, "
+              + "will be overwritten", tenantId);
+        }
 
-      CachedTenantState cachedTenantState = tenantCache.get(tenantId);
-      Preconditions.checkNotNull(cachedTenantState,
-          "Cache entry for tenant '" + tenantId + "' does not exist");
-
-      LOG.info("Adding user '{}' access ID '{}' to tenant '{}' in-memory cache",
-          principal.getName(), accessId, tenantId);
-      cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
-
-      final String tenantUserRoleName =
-          tenantCache.get(tenantId).getTenantUserRoleName();
-      final OzoneTenantRolePrincipal tenantUserRolePrincipal =
-          new OzoneTenantRolePrincipal(tenantUserRoleName);
-      String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
-      final String roleId =
-          authorizer.assignUserToRole(principal, roleJsonStr, false);
-      return roleId;
-    } catch (IOException e) {
-      // Clean up
-      revokeUserAccessId(accessId);
-      tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+        // New entry in tenant cache
+        tenantCache.put(tenantId, new CachedTenantState(
+            tenantId, userRoleName, adminRoleName));
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
+    }
 
-      throw new OMException(e.getMessage(), TENANT_AUTHORIZER_ERROR);
-    } finally {
-      tenantCacheLock.writeLock().unlock();
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      final String tenantId = tenant.getTenantId();
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.info("Removing tenant from in-memory cache: {}", tenantId);
+          tenantCache.remove(tenantId);
+        } else {
+          throw new OMException("Tenant does not exist in cache: " + tenantId,
+              INTERNAL_ERROR);
+        }
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
     }
-  }
 
-  @Override
-  public void revokeUserAccessId(String accessId) throws IOException {
-    try {
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) {
+
+      final CachedAccessIdInfo cacheEntry =
+          new CachedAccessIdInfo(userPrincipal, false);
+
       tenantCacheLock.writeLock().lock();
-      final OmDBAccessIdInfo omDBAccessIdInfo =
-          omMetadataManager.getTenantAccessIdTable().get(accessId);
-      if (omDBAccessIdInfo == null) {
-        throw new OMException(INVALID_ACCESS_ID);
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        LOG.info("Adding to cache: user '{}' accessId '{}' in tenant '{}'",
+            userPrincipal, accessId, tenantId);
+        cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
+      } catch (Exception e) {
+        // Attempt to clean up tenant cache entry on exception
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+      } finally {
+        tenantCacheLock.writeLock().unlock();
       }
-      final String tenantId = omDBAccessIdInfo.getTenantId();
-      if (tenantId == null) {
-        LOG.error("Tenant doesn't exist");
-        return;
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        LOG.info("Removing from cache: accessId '{}' in tenant '{}'",
+            accessId, tenantId);
+        if (!tenantCache.get(tenantId).getAccessIdInfoMap()
+            .containsKey(accessId)) {
+          throw new OMException("accessId '" + accessId + "' doesn't exist "
+              + "in tenant cache!", INTERNAL_ERROR);
+        }
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+      } catch (NullPointerException e) {
+        // tenantCache is somehow empty. Ignore for now.
+        LOG.warn("NPE when removing accessId from cache", e);

Review Comment:
   Actually, NPE shouldn't be thrown here anymore.
   Tenant existence is checked in preExecute. Removed



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

[GitHub] [ozone] errose28 commented on a diff in pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
errose28 commented on code in PR #3450:
URL: https://github.com/apache/ozone/pull/3450#discussion_r883106891


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java:
##########
@@ -98,14 +98,23 @@ public class OMMultiTenantManagerImpl implements OMMultiTenantManager {
   public static final String OZONE_OM_TENANT_DEV_SKIP_RANGER =
       "ozone.om.tenant.dev.skip.ranger";
 
-  private MultiTenantAccessAuthorizer authorizer;
   private final OzoneManager ozoneManager;
   private final OMMetadataManager omMetadataManager;
   private final OzoneConfiguration conf;
   // tenantCache: tenantId -> CachedTenantState
   private final Map<String, CachedTenantState> tenantCache;
   private final ReentrantReadWriteLock tenantCacheLock;
   private final OMRangerBGSyncService omRangerBGSyncService;
+  private MultiTenantAccessAuthorizer authorizer;
+  private final AuthorizerLock authorizerLock;
+  /**
+   * Authorizer operations. Meant to be called in tenant preExecute.
+   */
+  private final TenantOp authorizerOp;
+  /**
+   * Authorizer operations. Meant to be called in tenant validateAndUpdateCache.

Review Comment:
   nit. Tenant operations.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java:
##########
@@ -183,246 +197,456 @@ public OMMetadataManager getOmMetadataManager() {
     return omMetadataManager;
   }
 
-  // TODO: Cleanup up this Java doc.
+  @Override
+  public TenantOp getAuthorizerOp() {
+    return authorizerOp;
+  }
+
+  @Override
+  public TenantOp getCacheOp() {
+    return cacheOp;
+  }
+
   /**
-   *  Algorithm
-   *  OM State :
-   *    - Validation (Part of Ratis Request)
-   *    - create volume {Part of RATIS request}
-   *    - Persistence to OM DB {Part of RATIS request}
-   *  Authorizer-plugin(Ranger) State :
-   *    - For every tenant create two user groups
-   *        # GroupTenantAllUsers
-   *        # GroupTenantAllAdmins
-   *
-   *    - For every tenant create two default policies
-   *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
-   *      part of validation in Ratis-Request. if the groups/policies exist
-   *      with the same name (Due to an earlier failed/success request), in
-   *      plugin, we just update in-memory-map here and return success.
-   *    - The job of cleanup of any half-done authorizer-plugin state is done
-   *      by a background thread.
-   *  Finally :
-   *    - Update all Maps maintained by Multi-Tenant-Manager
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param tenantId
-   * @param userRoleName
-   * @param adminRoleName
-   * @return Tenant
-   * @throws IOException
+   * Implements tenant authorizer operations.
    */
-  @Override
-  public Tenant createTenantAccessInAuthorizer(String tenantId,
-      String userRoleName, String adminRoleName)
-      throws IOException {
+  public class AuthorizerOp implements TenantOp {
+
+    private final MultiTenantAccessAuthorizer authorizer;
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    AuthorizerOp(MultiTenantAccessAuthorizer authorizer,
+        Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.authorizer = authorizer;
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
 
-    Tenant tenant = new OzoneTenant(tenantId);
-    try {
-      tenantCacheLock.writeLock().lock();
+    private void checkAcquiredAuthorizerWriteLock() throws OMException {
 
-      // Create admin role first
-      String adminRoleId = authorizer.createRole(adminRoleName, null);
-      tenant.addTenantAccessRole(adminRoleId);
-
-      // Then create user role, and add admin role as its delegated admin
-      String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
-      tenant.addTenantAccessRole(userRoleId);
-
-      BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
-      // Bucket namespace is volume
-      for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
-        String volumeName = volume.getVolumeName();
-
-        final OzoneTenantRolePrincipal userRole =
-            new OzoneTenantRolePrincipal(userRoleName);
-        final OzoneTenantRolePrincipal adminRole =
-            new OzoneTenantRolePrincipal(adminRoleName);
-
-        // Allow Volume List access
-        AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
-            volumeName, userRole, adminRole);
-        tenantVolumeAccessPolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
-        tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
-
-        // Allow Bucket Create within Volume
-        AccessPolicy tenantBucketCreatePolicy =
-            newDefaultBucketAccessPolicy(volumeName, userRole);
-        tenantBucketCreatePolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantBucketCreatePolicy));
-        tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+      // Check if lock is acquired by the current thread
+      if (!authorizerLock.isHeldByCurrentThread()) {
+        throw new OMException("Authorizer write lock must have been held "
+            + "before calling this method", INTERNAL_ERROR);
       }
+    }
+
+    /**
+     *  Algorithm
+     *  OM State :
+     *    - Validation (Part of Ratis Request)
+     *    - create volume {Part of RATIS request}
+     *    - Persistence to OM DB {Part of RATIS request}
+     *  Authorizer-plugin(Ranger) State :
+     *    - For every tenant create two user groups
+     *        # GroupTenantAllUsers
+     *        # GroupTenantAllAdmins
+     *
+     *    - For every tenant create two default policies
+     *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
+     *      part of validation in Ratis-Request. if the groups/policies exist
+     *      with the same name (Due to an earlier failed/success request), in
+     *      plugin, we just update in-memory-map here and return success.
+     *    - The job of cleanup of any half-done authorizer-plugin state is done
+     *      by a background thread.
+     *  Finally :
+     *    - Update all Maps maintained by Multi-Tenant-Manager
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param tenantId tenant name
+     * @param userRoleName user role name
+     * @param adminRoleName admin role name
+     * @return Tenant
+     * @throws IOException
+     */
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      Tenant tenant = new OzoneTenant(tenantId);
 
-      if (tenantCache.containsKey(tenantId)) {
-        LOG.warn("Cache entry for tenant '{}' somehow already exists, "
-            + "will be overwritten", tenantId);  // TODO: throw exception?
+      try {
+        // Create admin role first
+        String adminRoleId = authorizer.createRole(adminRoleName, null);
+        tenant.addTenantAccessRole(adminRoleId);
+
+        // Then create user role, and add admin role as its delegated admin
+        String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
+        tenant.addTenantAccessRole(userRoleId);
+
+        BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
+        // Bucket namespace is volume
+        for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
+          String volumeName = volume.getVolumeName();
+
+          final OzoneTenantRolePrincipal userRole =
+              new OzoneTenantRolePrincipal(userRoleName);
+          final OzoneTenantRolePrincipal adminRole =
+              new OzoneTenantRolePrincipal(adminRoleName);
+
+          // Allow Volume List access
+          AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
+              volumeName, userRole, adminRole);
+          tenantVolumeAccessPolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
+          tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
+
+          // Allow Bucket Create within Volume
+          AccessPolicy tenantBucketCreatePolicy =
+              newDefaultBucketAccessPolicy(volumeName, userRole);
+          tenantBucketCreatePolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantBucketCreatePolicy));
+          tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+        }
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
+    }
 
-      // TODO: Move tenantCache update to a separate call createTenantAccessInDB
-      //  createTenantAccessInAuthorizer is called preExecute to update Ranger
-      //  createTenantAccessInDB will be called in validateAndUpdateCache
-      //  Do the same to all other InAuthorizer calls as well.
-      // New entry in tenant cache
-      tenantCache.put(tenantId, new CachedTenantState(
-          tenantId, userRoleName, adminRoleName));
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      LOG.info("Deleting tenant policies and roles from Ranger: {}", tenant);
 
-    } catch (IOException e) {
       try {
-        removeTenantAccessFromAuthorizer(tenant);
-      } catch (IOException ignored) {
-        // Best effort cleanup.
+        for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
+          authorizer.deletePolicyByName(policy.getPolicyName());
+        }
+
+        for (String roleName : tenant.getTenantRoles()) {
+          authorizer.deleteRoleByName(roleName);
+        }
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
-      throw e;
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
-    return tenant;
-  }
 
-  @Override
-  public void removeTenantAccessFromAuthorizer(Tenant tenant)
-      throws IOException {
-    try {
-      tenantCacheLock.writeLock().lock();
-      for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
-        authorizer.deletePolicyById(policy.getPolicyID());
+    /**
+     *  Algorithm
+     *  Authorizer-plugin(Ranger) State :
+     *    - create User in Ranger DB
+     *    - For every user created
+     *        Add them to # GroupTenantAllUsers
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param userPrincipal
+     * @param tenantId
+     * @param accessId
+     * @throws IOException
+     */
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        // Does NOT update tenant cache here
+
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.assignUserToRole(userPrincipal, roleJsonStr, false);
+
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("roleId that the user is assigned to: {}", roleId);
+        }
+
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      for (String roleId : tenant.getTenantRoles()) {
-        authorizer.deleteRole(roleId);
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final OmDBAccessIdInfo omDBAccessIdInfo =
+            omMetadataManager.getTenantAccessIdTable().get(accessId);
+        if (omDBAccessIdInfo == null) {
+          throw new OMException(INVALID_ACCESS_ID);
+        }
+        final String tenantIdGot = omDBAccessIdInfo.getTenantId();
+        Preconditions.checkArgument(tenantIdGot.equals(tenantId));
+
+        final String userPrincipal = omDBAccessIdInfo.getUserPrincipal();
+
+        // Delete user from role in Ranger
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.revokeUserFromRole(userPrincipal, roleJsonStr);
+        Preconditions.checkNotNull(roleId);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      if (tenantCache.containsKey(tenant.getTenantId())) {
-        LOG.info("Removing tenant {} from in memory cached state",
-            tenant.getTenantId());
-        tenantCache.remove(tenant.getTenantId());
+    }
+
+    @Override
+    public void assignTenantAdmin(String accessId, boolean delegated)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.assignUserToRole(
+            userPrincipal, roleJsonStr, delegated);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
+
+    @Override
+    public void revokeTenantAdmin(String accessId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.revokeUserFromRole(
+            userPrincipal, roleJsonStr);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
+      }
+    }
+
   }
 
   /**
-   *  Algorithm
-   *  Authorizer-plugin(Ranger) State :
-   *    - create User in Ranger DB
-   *    - For every user created
-   *        Add them to # GroupTenantAllUsers
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param principal
-   * @param tenantId
-   * @param accessId
-   * @return Tenant, or null on error
-   * @throws IOException
+   * Implements tenant cache operations.
    */
-  @Override
-  public String assignUserToTenant(BasicUserPrincipal principal,
-      String tenantId, String accessId) throws IOException {
+  public class CacheOp implements TenantOp {
 
-    final CachedAccessIdInfo cacheEntry =
-        new CachedAccessIdInfo(principal.getName(), false);
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    CacheOp(Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
+
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName) {
 
-    try {
       tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.warn("Cache entry for tenant '{}' already exists, "
+              + "will be overwritten", tenantId);
+        }
 
-      CachedTenantState cachedTenantState = tenantCache.get(tenantId);
-      Preconditions.checkNotNull(cachedTenantState,
-          "Cache entry for tenant '" + tenantId + "' does not exist");
-
-      LOG.info("Adding user '{}' access ID '{}' to tenant '{}' in-memory cache",
-          principal.getName(), accessId, tenantId);
-      cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
-
-      final String tenantUserRoleName =
-          tenantCache.get(tenantId).getTenantUserRoleName();
-      final OzoneTenantRolePrincipal tenantUserRolePrincipal =
-          new OzoneTenantRolePrincipal(tenantUserRoleName);
-      String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
-      final String roleId =
-          authorizer.assignUserToRole(principal, roleJsonStr, false);
-      return roleId;
-    } catch (IOException e) {
-      // Clean up
-      revokeUserAccessId(accessId);
-      tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+        // New entry in tenant cache
+        tenantCache.put(tenantId, new CachedTenantState(
+            tenantId, userRoleName, adminRoleName));
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
+    }
 
-      throw new OMException(e.getMessage(), TENANT_AUTHORIZER_ERROR);
-    } finally {
-      tenantCacheLock.writeLock().unlock();
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      final String tenantId = tenant.getTenantId();
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.info("Removing tenant from in-memory cache: {}", tenantId);
+          tenantCache.remove(tenantId);
+        } else {
+          throw new OMException("Tenant does not exist in cache: " + tenantId,
+              INTERNAL_ERROR);
+        }
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
     }
-  }
 
-  @Override
-  public void revokeUserAccessId(String accessId) throws IOException {
-    try {
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) {
+
+      final CachedAccessIdInfo cacheEntry =
+          new CachedAccessIdInfo(userPrincipal, false);
+
       tenantCacheLock.writeLock().lock();
-      final OmDBAccessIdInfo omDBAccessIdInfo =
-          omMetadataManager.getTenantAccessIdTable().get(accessId);
-      if (omDBAccessIdInfo == null) {
-        throw new OMException(INVALID_ACCESS_ID);
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        LOG.info("Adding to cache: user '{}' accessId '{}' in tenant '{}'",
+            userPrincipal, accessId, tenantId);
+        cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
+      } catch (Exception e) {
+        // Attempt to clean up tenant cache entry on exception
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);

Review Comment:
   This is all in memory right? where would the exception come from?



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java:
##########
@@ -834,6 +1001,34 @@ public String getTenantAdminRoleName(String tenantId) throws IOException {
     }
   }
 
+  @Override
+  public Tenant getTenantFromDBById(String tenantId) throws IOException {
+
+    // Policy names (not cached at the moment) have to retrieved from OM DB.
+    // TODO: Store policy names in cache as well if needed.
+
+    final OmDBTenantState tenantState =
+        omMetadataManager.getTenantStateTable().get(tenantId);
+
+    if (tenantState == null) {
+      throw new OMException("Potential DB error or race condition. "
+          + "OmDBTenantState entry is missing for tenant '" + tenantId + "'.",
+          OMException.ResultCodes.TENANT_NOT_FOUND);

Review Comment:
   Anything OMException may be returned to the user by the caller of this method, so it should have a user facing message. Again, I think our input validation is lacking so we should not be so sure these things are server side errors.
   ```suggestion
   throw new OMException("Tenant " + tenantId + " does not exist.", OMException.ResultCodes.TENANT_NOT_FOUND);
   ```



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/multitenant/OMRangerBGSyncService.java:
##########
@@ -273,9 +280,18 @@ public BackgroundTaskResult call() {
   }
 
   private void triggerRangerSyncOnce() {
-    int attempt = 0;
+    long lockStamp = 0L;
     try {
-      // TODO: Acquire lock
+      // Acquire authorizer (Ranger) write lock

Review Comment:
   I don't think we need to acquire the lock to get the ranger service version. Other ranger ops by the user outside of ozone could also bump the ranger version, and bg sync is the only thing that can change the OM DB version.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java:
##########
@@ -183,246 +197,456 @@ public OMMetadataManager getOmMetadataManager() {
     return omMetadataManager;
   }
 
-  // TODO: Cleanup up this Java doc.
+  @Override
+  public TenantOp getAuthorizerOp() {
+    return authorizerOp;
+  }
+
+  @Override
+  public TenantOp getCacheOp() {
+    return cacheOp;
+  }
+
   /**
-   *  Algorithm
-   *  OM State :
-   *    - Validation (Part of Ratis Request)
-   *    - create volume {Part of RATIS request}
-   *    - Persistence to OM DB {Part of RATIS request}
-   *  Authorizer-plugin(Ranger) State :
-   *    - For every tenant create two user groups
-   *        # GroupTenantAllUsers
-   *        # GroupTenantAllAdmins
-   *
-   *    - For every tenant create two default policies
-   *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
-   *      part of validation in Ratis-Request. if the groups/policies exist
-   *      with the same name (Due to an earlier failed/success request), in
-   *      plugin, we just update in-memory-map here and return success.
-   *    - The job of cleanup of any half-done authorizer-plugin state is done
-   *      by a background thread.
-   *  Finally :
-   *    - Update all Maps maintained by Multi-Tenant-Manager
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param tenantId
-   * @param userRoleName
-   * @param adminRoleName
-   * @return Tenant
-   * @throws IOException
+   * Implements tenant authorizer operations.
    */
-  @Override
-  public Tenant createTenantAccessInAuthorizer(String tenantId,
-      String userRoleName, String adminRoleName)
-      throws IOException {
+  public class AuthorizerOp implements TenantOp {
+
+    private final MultiTenantAccessAuthorizer authorizer;
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    AuthorizerOp(MultiTenantAccessAuthorizer authorizer,
+        Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.authorizer = authorizer;
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
 
-    Tenant tenant = new OzoneTenant(tenantId);
-    try {
-      tenantCacheLock.writeLock().lock();
+    private void checkAcquiredAuthorizerWriteLock() throws OMException {
 
-      // Create admin role first
-      String adminRoleId = authorizer.createRole(adminRoleName, null);
-      tenant.addTenantAccessRole(adminRoleId);
-
-      // Then create user role, and add admin role as its delegated admin
-      String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
-      tenant.addTenantAccessRole(userRoleId);
-
-      BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
-      // Bucket namespace is volume
-      for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
-        String volumeName = volume.getVolumeName();
-
-        final OzoneTenantRolePrincipal userRole =
-            new OzoneTenantRolePrincipal(userRoleName);
-        final OzoneTenantRolePrincipal adminRole =
-            new OzoneTenantRolePrincipal(adminRoleName);
-
-        // Allow Volume List access
-        AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
-            volumeName, userRole, adminRole);
-        tenantVolumeAccessPolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
-        tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
-
-        // Allow Bucket Create within Volume
-        AccessPolicy tenantBucketCreatePolicy =
-            newDefaultBucketAccessPolicy(volumeName, userRole);
-        tenantBucketCreatePolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantBucketCreatePolicy));
-        tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+      // Check if lock is acquired by the current thread
+      if (!authorizerLock.isHeldByCurrentThread()) {
+        throw new OMException("Authorizer write lock must have been held "
+            + "before calling this method", INTERNAL_ERROR);
       }
+    }
+
+    /**
+     *  Algorithm
+     *  OM State :
+     *    - Validation (Part of Ratis Request)
+     *    - create volume {Part of RATIS request}
+     *    - Persistence to OM DB {Part of RATIS request}
+     *  Authorizer-plugin(Ranger) State :
+     *    - For every tenant create two user groups
+     *        # GroupTenantAllUsers
+     *        # GroupTenantAllAdmins
+     *
+     *    - For every tenant create two default policies
+     *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
+     *      part of validation in Ratis-Request. if the groups/policies exist
+     *      with the same name (Due to an earlier failed/success request), in
+     *      plugin, we just update in-memory-map here and return success.
+     *    - The job of cleanup of any half-done authorizer-plugin state is done
+     *      by a background thread.
+     *  Finally :
+     *    - Update all Maps maintained by Multi-Tenant-Manager
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param tenantId tenant name
+     * @param userRoleName user role name
+     * @param adminRoleName admin role name
+     * @return Tenant
+     * @throws IOException
+     */
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      Tenant tenant = new OzoneTenant(tenantId);
 
-      if (tenantCache.containsKey(tenantId)) {
-        LOG.warn("Cache entry for tenant '{}' somehow already exists, "
-            + "will be overwritten", tenantId);  // TODO: throw exception?
+      try {
+        // Create admin role first
+        String adminRoleId = authorizer.createRole(adminRoleName, null);
+        tenant.addTenantAccessRole(adminRoleId);
+
+        // Then create user role, and add admin role as its delegated admin
+        String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
+        tenant.addTenantAccessRole(userRoleId);
+
+        BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
+        // Bucket namespace is volume
+        for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
+          String volumeName = volume.getVolumeName();
+
+          final OzoneTenantRolePrincipal userRole =
+              new OzoneTenantRolePrincipal(userRoleName);
+          final OzoneTenantRolePrincipal adminRole =
+              new OzoneTenantRolePrincipal(adminRoleName);
+
+          // Allow Volume List access
+          AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
+              volumeName, userRole, adminRole);
+          tenantVolumeAccessPolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
+          tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
+
+          // Allow Bucket Create within Volume
+          AccessPolicy tenantBucketCreatePolicy =
+              newDefaultBucketAccessPolicy(volumeName, userRole);
+          tenantBucketCreatePolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantBucketCreatePolicy));
+          tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+        }
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
+    }
 
-      // TODO: Move tenantCache update to a separate call createTenantAccessInDB
-      //  createTenantAccessInAuthorizer is called preExecute to update Ranger
-      //  createTenantAccessInDB will be called in validateAndUpdateCache
-      //  Do the same to all other InAuthorizer calls as well.
-      // New entry in tenant cache
-      tenantCache.put(tenantId, new CachedTenantState(
-          tenantId, userRoleName, adminRoleName));
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      LOG.info("Deleting tenant policies and roles from Ranger: {}", tenant);
 
-    } catch (IOException e) {
       try {
-        removeTenantAccessFromAuthorizer(tenant);
-      } catch (IOException ignored) {
-        // Best effort cleanup.
+        for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
+          authorizer.deletePolicyByName(policy.getPolicyName());
+        }
+
+        for (String roleName : tenant.getTenantRoles()) {
+          authorizer.deleteRoleByName(roleName);
+        }
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
-      throw e;
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
-    return tenant;
-  }
 
-  @Override
-  public void removeTenantAccessFromAuthorizer(Tenant tenant)
-      throws IOException {
-    try {
-      tenantCacheLock.writeLock().lock();
-      for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
-        authorizer.deletePolicyById(policy.getPolicyID());
+    /**
+     *  Algorithm
+     *  Authorizer-plugin(Ranger) State :
+     *    - create User in Ranger DB
+     *    - For every user created
+     *        Add them to # GroupTenantAllUsers
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param userPrincipal
+     * @param tenantId
+     * @param accessId
+     * @throws IOException
+     */
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        // Does NOT update tenant cache here
+
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.assignUserToRole(userPrincipal, roleJsonStr, false);
+
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("roleId that the user is assigned to: {}", roleId);
+        }
+
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      for (String roleId : tenant.getTenantRoles()) {
-        authorizer.deleteRole(roleId);
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final OmDBAccessIdInfo omDBAccessIdInfo =
+            omMetadataManager.getTenantAccessIdTable().get(accessId);
+        if (omDBAccessIdInfo == null) {
+          throw new OMException(INVALID_ACCESS_ID);
+        }
+        final String tenantIdGot = omDBAccessIdInfo.getTenantId();
+        Preconditions.checkArgument(tenantIdGot.equals(tenantId));
+
+        final String userPrincipal = omDBAccessIdInfo.getUserPrincipal();
+
+        // Delete user from role in Ranger
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.revokeUserFromRole(userPrincipal, roleJsonStr);
+        Preconditions.checkNotNull(roleId);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      if (tenantCache.containsKey(tenant.getTenantId())) {
-        LOG.info("Removing tenant {} from in memory cached state",
-            tenant.getTenantId());
-        tenantCache.remove(tenant.getTenantId());
+    }
+
+    @Override
+    public void assignTenantAdmin(String accessId, boolean delegated)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.assignUserToRole(
+            userPrincipal, roleJsonStr, delegated);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
+
+    @Override
+    public void revokeTenantAdmin(String accessId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.revokeUserFromRole(
+            userPrincipal, roleJsonStr);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
+      }
+    }
+
   }
 
   /**
-   *  Algorithm
-   *  Authorizer-plugin(Ranger) State :
-   *    - create User in Ranger DB
-   *    - For every user created
-   *        Add them to # GroupTenantAllUsers
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param principal
-   * @param tenantId
-   * @param accessId
-   * @return Tenant, or null on error
-   * @throws IOException
+   * Implements tenant cache operations.
    */
-  @Override
-  public String assignUserToTenant(BasicUserPrincipal principal,
-      String tenantId, String accessId) throws IOException {
+  public class CacheOp implements TenantOp {
 
-    final CachedAccessIdInfo cacheEntry =
-        new CachedAccessIdInfo(principal.getName(), false);
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    CacheOp(Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
+
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName) {
 
-    try {
       tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.warn("Cache entry for tenant '{}' already exists, "
+              + "will be overwritten", tenantId);
+        }
 
-      CachedTenantState cachedTenantState = tenantCache.get(tenantId);
-      Preconditions.checkNotNull(cachedTenantState,
-          "Cache entry for tenant '" + tenantId + "' does not exist");
-
-      LOG.info("Adding user '{}' access ID '{}' to tenant '{}' in-memory cache",
-          principal.getName(), accessId, tenantId);
-      cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
-
-      final String tenantUserRoleName =
-          tenantCache.get(tenantId).getTenantUserRoleName();
-      final OzoneTenantRolePrincipal tenantUserRolePrincipal =
-          new OzoneTenantRolePrincipal(tenantUserRoleName);
-      String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
-      final String roleId =
-          authorizer.assignUserToRole(principal, roleJsonStr, false);
-      return roleId;
-    } catch (IOException e) {
-      // Clean up
-      revokeUserAccessId(accessId);
-      tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+        // New entry in tenant cache
+        tenantCache.put(tenantId, new CachedTenantState(
+            tenantId, userRoleName, adminRoleName));
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
+    }
 
-      throw new OMException(e.getMessage(), TENANT_AUTHORIZER_ERROR);
-    } finally {
-      tenantCacheLock.writeLock().unlock();
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      final String tenantId = tenant.getTenantId();
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.info("Removing tenant from in-memory cache: {}", tenantId);
+          tenantCache.remove(tenantId);
+        } else {
+          throw new OMException("Tenant does not exist in cache: " + tenantId,
+              INTERNAL_ERROR);
+        }
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
     }
-  }
 
-  @Override
-  public void revokeUserAccessId(String accessId) throws IOException {
-    try {
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) {
+
+      final CachedAccessIdInfo cacheEntry =
+          new CachedAccessIdInfo(userPrincipal, false);
+
       tenantCacheLock.writeLock().lock();
-      final OmDBAccessIdInfo omDBAccessIdInfo =
-          omMetadataManager.getTenantAccessIdTable().get(accessId);
-      if (omDBAccessIdInfo == null) {
-        throw new OMException(INVALID_ACCESS_ID);
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        LOG.info("Adding to cache: user '{}' accessId '{}' in tenant '{}'",
+            userPrincipal, accessId, tenantId);
+        cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
+      } catch (Exception e) {
+        // Attempt to clean up tenant cache entry on exception
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+      } finally {
+        tenantCacheLock.writeLock().unlock();
       }
-      final String tenantId = omDBAccessIdInfo.getTenantId();
-      if (tenantId == null) {
-        LOG.error("Tenant doesn't exist");
-        return;
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        LOG.info("Removing from cache: accessId '{}' in tenant '{}'",
+            accessId, tenantId);
+        if (!tenantCache.get(tenantId).getAccessIdInfoMap()
+            .containsKey(accessId)) {
+          throw new OMException("accessId '" + accessId + "' doesn't exist "
+              + "in tenant cache!", INTERNAL_ERROR);
+        }
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+      } catch (NullPointerException e) {
+        // tenantCache is somehow empty. Ignore for now.
+        LOG.warn("NPE when removing accessId from cache", e);

Review Comment:
   I'm not sure we have proper validation that tenant and access IDs given are valid in the callers of these methods. Might be better to to validate those against the cache under the lock in each operation and throw the corresponding OMException, instead of catching an unchecked exception and logging an error.



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/multitenant/OMRangerBGSyncService.java:
##########
@@ -394,11 +426,18 @@ long getOMDBRangerServiceVersion() throws IOException {
   private void executeOMDBToRangerSync(long baseVersion) throws IOException {
     clearPolicyAndRoleMaps();
 
-    // TODO: Acquire global lock
-    loadAllPoliciesAndRoleNamesFromRanger(baseVersion);
-    loadAllRolesFromRanger();
+
+    withReadLock(() -> {
+      try {
+        loadAllPoliciesAndRoleNamesFromRanger(baseVersion);
+        loadAllRolesFromRanger();

Review Comment:
   If we are locking while getting information from Ranger, we might as well add the OM DB load operations in this block as well, since those will be faster. We could even fire off single thread executors for each to do the loading in parallel and then wait for each to finish.



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

[GitHub] [ozone] smengcl commented on a diff in pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3450:
URL: https://github.com/apache/ozone/pull/3450#discussion_r883185549


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java:
##########
@@ -183,246 +197,456 @@ public OMMetadataManager getOmMetadataManager() {
     return omMetadataManager;
   }
 
-  // TODO: Cleanup up this Java doc.
+  @Override
+  public TenantOp getAuthorizerOp() {
+    return authorizerOp;
+  }
+
+  @Override
+  public TenantOp getCacheOp() {
+    return cacheOp;
+  }
+
   /**
-   *  Algorithm
-   *  OM State :
-   *    - Validation (Part of Ratis Request)
-   *    - create volume {Part of RATIS request}
-   *    - Persistence to OM DB {Part of RATIS request}
-   *  Authorizer-plugin(Ranger) State :
-   *    - For every tenant create two user groups
-   *        # GroupTenantAllUsers
-   *        # GroupTenantAllAdmins
-   *
-   *    - For every tenant create two default policies
-   *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
-   *      part of validation in Ratis-Request. if the groups/policies exist
-   *      with the same name (Due to an earlier failed/success request), in
-   *      plugin, we just update in-memory-map here and return success.
-   *    - The job of cleanup of any half-done authorizer-plugin state is done
-   *      by a background thread.
-   *  Finally :
-   *    - Update all Maps maintained by Multi-Tenant-Manager
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param tenantId
-   * @param userRoleName
-   * @param adminRoleName
-   * @return Tenant
-   * @throws IOException
+   * Implements tenant authorizer operations.
    */
-  @Override
-  public Tenant createTenantAccessInAuthorizer(String tenantId,
-      String userRoleName, String adminRoleName)
-      throws IOException {
+  public class AuthorizerOp implements TenantOp {
+
+    private final MultiTenantAccessAuthorizer authorizer;
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    AuthorizerOp(MultiTenantAccessAuthorizer authorizer,
+        Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.authorizer = authorizer;
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
 
-    Tenant tenant = new OzoneTenant(tenantId);
-    try {
-      tenantCacheLock.writeLock().lock();
+    private void checkAcquiredAuthorizerWriteLock() throws OMException {
 
-      // Create admin role first
-      String adminRoleId = authorizer.createRole(adminRoleName, null);
-      tenant.addTenantAccessRole(adminRoleId);
-
-      // Then create user role, and add admin role as its delegated admin
-      String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
-      tenant.addTenantAccessRole(userRoleId);
-
-      BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
-      // Bucket namespace is volume
-      for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
-        String volumeName = volume.getVolumeName();
-
-        final OzoneTenantRolePrincipal userRole =
-            new OzoneTenantRolePrincipal(userRoleName);
-        final OzoneTenantRolePrincipal adminRole =
-            new OzoneTenantRolePrincipal(adminRoleName);
-
-        // Allow Volume List access
-        AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
-            volumeName, userRole, adminRole);
-        tenantVolumeAccessPolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
-        tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
-
-        // Allow Bucket Create within Volume
-        AccessPolicy tenantBucketCreatePolicy =
-            newDefaultBucketAccessPolicy(volumeName, userRole);
-        tenantBucketCreatePolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantBucketCreatePolicy));
-        tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+      // Check if lock is acquired by the current thread
+      if (!authorizerLock.isHeldByCurrentThread()) {
+        throw new OMException("Authorizer write lock must have been held "
+            + "before calling this method", INTERNAL_ERROR);
       }
+    }
+
+    /**
+     *  Algorithm
+     *  OM State :
+     *    - Validation (Part of Ratis Request)
+     *    - create volume {Part of RATIS request}
+     *    - Persistence to OM DB {Part of RATIS request}
+     *  Authorizer-plugin(Ranger) State :
+     *    - For every tenant create two user groups
+     *        # GroupTenantAllUsers
+     *        # GroupTenantAllAdmins
+     *
+     *    - For every tenant create two default policies
+     *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
+     *      part of validation in Ratis-Request. if the groups/policies exist
+     *      with the same name (Due to an earlier failed/success request), in
+     *      plugin, we just update in-memory-map here and return success.
+     *    - The job of cleanup of any half-done authorizer-plugin state is done
+     *      by a background thread.
+     *  Finally :
+     *    - Update all Maps maintained by Multi-Tenant-Manager
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param tenantId tenant name
+     * @param userRoleName user role name
+     * @param adminRoleName admin role name
+     * @return Tenant
+     * @throws IOException
+     */
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      Tenant tenant = new OzoneTenant(tenantId);
 
-      if (tenantCache.containsKey(tenantId)) {
-        LOG.warn("Cache entry for tenant '{}' somehow already exists, "
-            + "will be overwritten", tenantId);  // TODO: throw exception?
+      try {
+        // Create admin role first
+        String adminRoleId = authorizer.createRole(adminRoleName, null);
+        tenant.addTenantAccessRole(adminRoleId);
+
+        // Then create user role, and add admin role as its delegated admin
+        String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
+        tenant.addTenantAccessRole(userRoleId);
+
+        BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
+        // Bucket namespace is volume
+        for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
+          String volumeName = volume.getVolumeName();
+
+          final OzoneTenantRolePrincipal userRole =
+              new OzoneTenantRolePrincipal(userRoleName);
+          final OzoneTenantRolePrincipal adminRole =
+              new OzoneTenantRolePrincipal(adminRoleName);
+
+          // Allow Volume List access
+          AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
+              volumeName, userRole, adminRole);
+          tenantVolumeAccessPolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
+          tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
+
+          // Allow Bucket Create within Volume
+          AccessPolicy tenantBucketCreatePolicy =
+              newDefaultBucketAccessPolicy(volumeName, userRole);
+          tenantBucketCreatePolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantBucketCreatePolicy));
+          tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+        }
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
+    }
 
-      // TODO: Move tenantCache update to a separate call createTenantAccessInDB
-      //  createTenantAccessInAuthorizer is called preExecute to update Ranger
-      //  createTenantAccessInDB will be called in validateAndUpdateCache
-      //  Do the same to all other InAuthorizer calls as well.
-      // New entry in tenant cache
-      tenantCache.put(tenantId, new CachedTenantState(
-          tenantId, userRoleName, adminRoleName));
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      LOG.info("Deleting tenant policies and roles from Ranger: {}", tenant);
 
-    } catch (IOException e) {
       try {
-        removeTenantAccessFromAuthorizer(tenant);
-      } catch (IOException ignored) {
-        // Best effort cleanup.
+        for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
+          authorizer.deletePolicyByName(policy.getPolicyName());
+        }
+
+        for (String roleName : tenant.getTenantRoles()) {
+          authorizer.deleteRoleByName(roleName);
+        }
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
-      throw e;
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
-    return tenant;
-  }
 
-  @Override
-  public void removeTenantAccessFromAuthorizer(Tenant tenant)
-      throws IOException {
-    try {
-      tenantCacheLock.writeLock().lock();
-      for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
-        authorizer.deletePolicyById(policy.getPolicyID());
+    /**
+     *  Algorithm
+     *  Authorizer-plugin(Ranger) State :
+     *    - create User in Ranger DB
+     *    - For every user created
+     *        Add them to # GroupTenantAllUsers
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param userPrincipal
+     * @param tenantId
+     * @param accessId
+     * @throws IOException
+     */
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        // Does NOT update tenant cache here
+
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.assignUserToRole(userPrincipal, roleJsonStr, false);
+
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("roleId that the user is assigned to: {}", roleId);
+        }
+
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      for (String roleId : tenant.getTenantRoles()) {
-        authorizer.deleteRole(roleId);
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final OmDBAccessIdInfo omDBAccessIdInfo =
+            omMetadataManager.getTenantAccessIdTable().get(accessId);
+        if (omDBAccessIdInfo == null) {
+          throw new OMException(INVALID_ACCESS_ID);
+        }
+        final String tenantIdGot = omDBAccessIdInfo.getTenantId();
+        Preconditions.checkArgument(tenantIdGot.equals(tenantId));
+
+        final String userPrincipal = omDBAccessIdInfo.getUserPrincipal();
+
+        // Delete user from role in Ranger
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.revokeUserFromRole(userPrincipal, roleJsonStr);
+        Preconditions.checkNotNull(roleId);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      if (tenantCache.containsKey(tenant.getTenantId())) {
-        LOG.info("Removing tenant {} from in memory cached state",
-            tenant.getTenantId());
-        tenantCache.remove(tenant.getTenantId());
+    }
+
+    @Override
+    public void assignTenantAdmin(String accessId, boolean delegated)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.assignUserToRole(
+            userPrincipal, roleJsonStr, delegated);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
+
+    @Override
+    public void revokeTenantAdmin(String accessId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.revokeUserFromRole(
+            userPrincipal, roleJsonStr);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
+      }
+    }
+
   }
 
   /**
-   *  Algorithm
-   *  Authorizer-plugin(Ranger) State :
-   *    - create User in Ranger DB
-   *    - For every user created
-   *        Add them to # GroupTenantAllUsers
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param principal
-   * @param tenantId
-   * @param accessId
-   * @return Tenant, or null on error
-   * @throws IOException
+   * Implements tenant cache operations.
    */
-  @Override
-  public String assignUserToTenant(BasicUserPrincipal principal,
-      String tenantId, String accessId) throws IOException {
+  public class CacheOp implements TenantOp {
 
-    final CachedAccessIdInfo cacheEntry =
-        new CachedAccessIdInfo(principal.getName(), false);
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    CacheOp(Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
+
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName) {
 
-    try {
       tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.warn("Cache entry for tenant '{}' already exists, "
+              + "will be overwritten", tenantId);
+        }
 
-      CachedTenantState cachedTenantState = tenantCache.get(tenantId);
-      Preconditions.checkNotNull(cachedTenantState,
-          "Cache entry for tenant '" + tenantId + "' does not exist");
-
-      LOG.info("Adding user '{}' access ID '{}' to tenant '{}' in-memory cache",
-          principal.getName(), accessId, tenantId);
-      cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
-
-      final String tenantUserRoleName =
-          tenantCache.get(tenantId).getTenantUserRoleName();
-      final OzoneTenantRolePrincipal tenantUserRolePrincipal =
-          new OzoneTenantRolePrincipal(tenantUserRoleName);
-      String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
-      final String roleId =
-          authorizer.assignUserToRole(principal, roleJsonStr, false);
-      return roleId;
-    } catch (IOException e) {
-      // Clean up
-      revokeUserAccessId(accessId);
-      tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+        // New entry in tenant cache
+        tenantCache.put(tenantId, new CachedTenantState(
+            tenantId, userRoleName, adminRoleName));
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
+    }
 
-      throw new OMException(e.getMessage(), TENANT_AUTHORIZER_ERROR);
-    } finally {
-      tenantCacheLock.writeLock().unlock();
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      final String tenantId = tenant.getTenantId();
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.info("Removing tenant from in-memory cache: {}", tenantId);
+          tenantCache.remove(tenantId);
+        } else {
+          throw new OMException("Tenant does not exist in cache: " + tenantId,
+              INTERNAL_ERROR);
+        }
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
     }
-  }
 
-  @Override
-  public void revokeUserAccessId(String accessId) throws IOException {
-    try {
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) {
+
+      final CachedAccessIdInfo cacheEntry =
+          new CachedAccessIdInfo(userPrincipal, false);
+
       tenantCacheLock.writeLock().lock();
-      final OmDBAccessIdInfo omDBAccessIdInfo =
-          omMetadataManager.getTenantAccessIdTable().get(accessId);
-      if (omDBAccessIdInfo == null) {
-        throw new OMException(INVALID_ACCESS_ID);
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        LOG.info("Adding to cache: user '{}' accessId '{}' in tenant '{}'",
+            userPrincipal, accessId, tenantId);
+        cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
+      } catch (Exception e) {
+        // Attempt to clean up tenant cache entry on exception
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);

Review Comment:
   Good point. Removed



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

[GitHub] [ozone] smengcl commented on a diff in pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3450:
URL: https://github.com/apache/ozone/pull/3450#discussion_r883188663


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/multitenant/OMRangerBGSyncService.java:
##########
@@ -273,9 +280,18 @@ public BackgroundTaskResult call() {
   }
 
   private void triggerRangerSyncOnce() {
-    int attempt = 0;
+    long lockStamp = 0L;
     try {
-      // TODO: Acquire lock
+      // Acquire authorizer (Ranger) write lock

Review Comment:
   I agree. Removed read lock



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

[GitHub] [ozone] smengcl commented on pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3450:
URL: https://github.com/apache/ozone/pull/3450#issuecomment-1139556349

   Thanks @prashantpogde @errose28 for the review.


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

[GitHub] [ozone] smengcl commented on a diff in pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
smengcl commented on code in PR #3450:
URL: https://github.com/apache/ozone/pull/3450#discussion_r883190314


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java:
##########
@@ -183,246 +197,456 @@ public OMMetadataManager getOmMetadataManager() {
     return omMetadataManager;
   }
 
-  // TODO: Cleanup up this Java doc.
+  @Override
+  public TenantOp getAuthorizerOp() {
+    return authorizerOp;
+  }
+
+  @Override
+  public TenantOp getCacheOp() {
+    return cacheOp;
+  }
+
   /**
-   *  Algorithm
-   *  OM State :
-   *    - Validation (Part of Ratis Request)
-   *    - create volume {Part of RATIS request}
-   *    - Persistence to OM DB {Part of RATIS request}
-   *  Authorizer-plugin(Ranger) State :
-   *    - For every tenant create two user groups
-   *        # GroupTenantAllUsers
-   *        # GroupTenantAllAdmins
-   *
-   *    - For every tenant create two default policies
-   *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
-   *      part of validation in Ratis-Request. if the groups/policies exist
-   *      with the same name (Due to an earlier failed/success request), in
-   *      plugin, we just update in-memory-map here and return success.
-   *    - The job of cleanup of any half-done authorizer-plugin state is done
-   *      by a background thread.
-   *  Finally :
-   *    - Update all Maps maintained by Multi-Tenant-Manager
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param tenantId
-   * @param userRoleName
-   * @param adminRoleName
-   * @return Tenant
-   * @throws IOException
+   * Implements tenant authorizer operations.
    */
-  @Override
-  public Tenant createTenantAccessInAuthorizer(String tenantId,
-      String userRoleName, String adminRoleName)
-      throws IOException {
+  public class AuthorizerOp implements TenantOp {
+
+    private final MultiTenantAccessAuthorizer authorizer;
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    AuthorizerOp(MultiTenantAccessAuthorizer authorizer,
+        Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.authorizer = authorizer;
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
 
-    Tenant tenant = new OzoneTenant(tenantId);
-    try {
-      tenantCacheLock.writeLock().lock();
+    private void checkAcquiredAuthorizerWriteLock() throws OMException {
 
-      // Create admin role first
-      String adminRoleId = authorizer.createRole(adminRoleName, null);
-      tenant.addTenantAccessRole(adminRoleId);
-
-      // Then create user role, and add admin role as its delegated admin
-      String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
-      tenant.addTenantAccessRole(userRoleId);
-
-      BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
-      // Bucket namespace is volume
-      for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
-        String volumeName = volume.getVolumeName();
-
-        final OzoneTenantRolePrincipal userRole =
-            new OzoneTenantRolePrincipal(userRoleName);
-        final OzoneTenantRolePrincipal adminRole =
-            new OzoneTenantRolePrincipal(adminRoleName);
-
-        // Allow Volume List access
-        AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
-            volumeName, userRole, adminRole);
-        tenantVolumeAccessPolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
-        tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
-
-        // Allow Bucket Create within Volume
-        AccessPolicy tenantBucketCreatePolicy =
-            newDefaultBucketAccessPolicy(volumeName, userRole);
-        tenantBucketCreatePolicy.setPolicyID(
-            authorizer.createAccessPolicy(tenantBucketCreatePolicy));
-        tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+      // Check if lock is acquired by the current thread
+      if (!authorizerLock.isHeldByCurrentThread()) {
+        throw new OMException("Authorizer write lock must have been held "
+            + "before calling this method", INTERNAL_ERROR);
       }
+    }
+
+    /**
+     *  Algorithm
+     *  OM State :
+     *    - Validation (Part of Ratis Request)
+     *    - create volume {Part of RATIS request}
+     *    - Persistence to OM DB {Part of RATIS request}
+     *  Authorizer-plugin(Ranger) State :
+     *    - For every tenant create two user groups
+     *        # GroupTenantAllUsers
+     *        # GroupTenantAllAdmins
+     *
+     *    - For every tenant create two default policies
+     *    - Note: plugin states are made idempotent. Onus of returning EEXIST is
+     *      part of validation in Ratis-Request. if the groups/policies exist
+     *      with the same name (Due to an earlier failed/success request), in
+     *      plugin, we just update in-memory-map here and return success.
+     *    - The job of cleanup of any half-done authorizer-plugin state is done
+     *      by a background thread.
+     *  Finally :
+     *    - Update all Maps maintained by Multi-Tenant-Manager
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param tenantId tenant name
+     * @param userRoleName user role name
+     * @param adminRoleName admin role name
+     * @return Tenant
+     * @throws IOException
+     */
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      Tenant tenant = new OzoneTenant(tenantId);
 
-      if (tenantCache.containsKey(tenantId)) {
-        LOG.warn("Cache entry for tenant '{}' somehow already exists, "
-            + "will be overwritten", tenantId);  // TODO: throw exception?
+      try {
+        // Create admin role first
+        String adminRoleId = authorizer.createRole(adminRoleName, null);
+        tenant.addTenantAccessRole(adminRoleId);
+
+        // Then create user role, and add admin role as its delegated admin
+        String userRoleId = authorizer.createRole(userRoleName, adminRoleName);
+        tenant.addTenantAccessRole(userRoleId);
+
+        BucketNameSpace bucketNameSpace = tenant.getTenantBucketNameSpace();
+        // Bucket namespace is volume
+        for (OzoneObj volume : bucketNameSpace.getBucketNameSpaceObjects()) {
+          String volumeName = volume.getVolumeName();
+
+          final OzoneTenantRolePrincipal userRole =
+              new OzoneTenantRolePrincipal(userRoleName);
+          final OzoneTenantRolePrincipal adminRole =
+              new OzoneTenantRolePrincipal(adminRoleName);
+
+          // Allow Volume List access
+          AccessPolicy tenantVolumeAccessPolicy = newDefaultVolumeAccessPolicy(
+              volumeName, userRole, adminRole);
+          tenantVolumeAccessPolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantVolumeAccessPolicy));
+          tenant.addTenantAccessPolicy(tenantVolumeAccessPolicy);
+
+          // Allow Bucket Create within Volume
+          AccessPolicy tenantBucketCreatePolicy =
+              newDefaultBucketAccessPolicy(volumeName, userRole);
+          tenantBucketCreatePolicy.setPolicyID(
+              authorizer.createAccessPolicy(tenantBucketCreatePolicy));
+          tenant.addTenantAccessPolicy(tenantBucketCreatePolicy);
+        }
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
+    }
 
-      // TODO: Move tenantCache update to a separate call createTenantAccessInDB
-      //  createTenantAccessInAuthorizer is called preExecute to update Ranger
-      //  createTenantAccessInDB will be called in validateAndUpdateCache
-      //  Do the same to all other InAuthorizer calls as well.
-      // New entry in tenant cache
-      tenantCache.put(tenantId, new CachedTenantState(
-          tenantId, userRoleName, adminRoleName));
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      LOG.info("Deleting tenant policies and roles from Ranger: {}", tenant);
 
-    } catch (IOException e) {
       try {
-        removeTenantAccessFromAuthorizer(tenant);
-      } catch (IOException ignored) {
-        // Best effort cleanup.
+        for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
+          authorizer.deletePolicyByName(policy.getPolicyName());
+        }
+
+        for (String roleName : tenant.getTenantRoles()) {
+          authorizer.deleteRoleByName(roleName);
+        }
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
       }
-      throw e;
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
-    return tenant;
-  }
 
-  @Override
-  public void removeTenantAccessFromAuthorizer(Tenant tenant)
-      throws IOException {
-    try {
-      tenantCacheLock.writeLock().lock();
-      for (AccessPolicy policy : tenant.getTenantAccessPolicies()) {
-        authorizer.deletePolicyById(policy.getPolicyID());
+    /**
+     *  Algorithm
+     *  Authorizer-plugin(Ranger) State :
+     *    - create User in Ranger DB
+     *    - For every user created
+     *        Add them to # GroupTenantAllUsers
+     *  In case of failure :
+     *    - Undo all Ranger State
+     *    - remove updates to the Map
+     *  Locking :
+     *    - Create/Manage Tenant/User operations are control path operations.
+     *      We can do all of this as part of holding a coarse lock and
+     *      synchronize these control path operations.
+     *
+     * @param userPrincipal
+     * @param tenantId
+     * @param accessId
+     * @throws IOException
+     */
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        // Does NOT update tenant cache here
+
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.assignUserToRole(userPrincipal, roleJsonStr, false);
+
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("roleId that the user is assigned to: {}", roleId);
+        }
+
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      for (String roleId : tenant.getTenantRoles()) {
-        authorizer.deleteRole(roleId);
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        final OmDBAccessIdInfo omDBAccessIdInfo =
+            omMetadataManager.getTenantAccessIdTable().get(accessId);
+        if (omDBAccessIdInfo == null) {
+          throw new OMException(INVALID_ACCESS_ID);
+        }
+        final String tenantIdGot = omDBAccessIdInfo.getTenantId();
+        Preconditions.checkArgument(tenantIdGot.equals(tenantId));
+
+        final String userPrincipal = omDBAccessIdInfo.getUserPrincipal();
+
+        // Delete user from role in Ranger
+        final String tenantUserRoleName =
+            tenantCache.get(tenantId).getTenantUserRoleName();
+        final OzoneTenantRolePrincipal tenantUserRolePrincipal =
+            new OzoneTenantRolePrincipal(tenantUserRoleName);
+        String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
+        final String roleId =
+            authorizer.revokeUserFromRole(userPrincipal, roleJsonStr);
+        Preconditions.checkNotNull(roleId);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the user role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-      if (tenantCache.containsKey(tenant.getTenantId())) {
-        LOG.info("Removing tenant {} from in memory cached state",
-            tenant.getTenantId());
-        tenantCache.remove(tenant.getTenantId());
+    }
+
+    @Override
+    public void assignTenantAdmin(String accessId, boolean delegated)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.assignUserToRole(
+            userPrincipal, roleJsonStr, delegated);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
       }
-    } finally {
-      tenantCacheLock.writeLock().unlock();
     }
+
+    @Override
+    public void revokeTenantAdmin(String accessId)
+        throws IOException {
+
+      checkAcquiredAuthorizerWriteLock();
+
+      tenantCacheLock.readLock().lock();
+      try {
+        // tenant name is needed to retrieve role name
+        final String tenantId = getTenantForAccessIDThrowIfNotFound(accessId);
+
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        final String tenantAdminRoleName =
+            cachedTenantState.getTenantAdminRoleName();
+        final OzoneTenantRolePrincipal existingAdminRole =
+            new OzoneTenantRolePrincipal(tenantAdminRoleName);
+
+        final String roleJsonStr = authorizer.getRole(existingAdminRole);
+        final String userPrincipal = getUserNameGivenAccessId(accessId);
+        // Update Ranger. Add user principal (not accessId!) to the role
+        final String roleId = authorizer.revokeUserFromRole(
+            userPrincipal, roleJsonStr);
+        assert (roleId != null);
+
+        // Does NOT update tenant cache here
+      } catch (IOException e) {
+        // Expect the sync thread to restore the admin role later if op succeeds
+        throw new OMException(e, TENANT_AUTHORIZER_ERROR);
+      } finally {
+        tenantCacheLock.readLock().unlock();
+      }
+    }
+
   }
 
   /**
-   *  Algorithm
-   *  Authorizer-plugin(Ranger) State :
-   *    - create User in Ranger DB
-   *    - For every user created
-   *        Add them to # GroupTenantAllUsers
-   *  In case of failure :
-   *    - Undo all Ranger State
-   *    - remove updates to the Map
-   *  Locking :
-   *    - Create/Manage Tenant/User operations are control path operations.
-   *      We can do all of this as part of holding a coarse lock and synchronize
-   *      these control path operations.
-   *
-   * @param principal
-   * @param tenantId
-   * @param accessId
-   * @return Tenant, or null on error
-   * @throws IOException
+   * Implements tenant cache operations.
    */
-  @Override
-  public String assignUserToTenant(BasicUserPrincipal principal,
-      String tenantId, String accessId) throws IOException {
+  public class CacheOp implements TenantOp {
 
-    final CachedAccessIdInfo cacheEntry =
-        new CachedAccessIdInfo(principal.getName(), false);
+    private final Map<String, CachedTenantState> tenantCache;
+    private final ReentrantReadWriteLock tenantCacheLock;
+
+    CacheOp(Map<String, CachedTenantState> tenantCache,
+        ReentrantReadWriteLock tenantCacheLock) {
+      this.tenantCache = tenantCache;
+      this.tenantCacheLock = tenantCacheLock;
+    }
+
+    @Override
+    public void createTenant(
+        String tenantId, String userRoleName, String adminRoleName) {
 
-    try {
       tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.warn("Cache entry for tenant '{}' already exists, "
+              + "will be overwritten", tenantId);
+        }
 
-      CachedTenantState cachedTenantState = tenantCache.get(tenantId);
-      Preconditions.checkNotNull(cachedTenantState,
-          "Cache entry for tenant '" + tenantId + "' does not exist");
-
-      LOG.info("Adding user '{}' access ID '{}' to tenant '{}' in-memory cache",
-          principal.getName(), accessId, tenantId);
-      cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
-
-      final String tenantUserRoleName =
-          tenantCache.get(tenantId).getTenantUserRoleName();
-      final OzoneTenantRolePrincipal tenantUserRolePrincipal =
-          new OzoneTenantRolePrincipal(tenantUserRoleName);
-      String roleJsonStr = authorizer.getRole(tenantUserRolePrincipal);
-      final String roleId =
-          authorizer.assignUserToRole(principal, roleJsonStr, false);
-      return roleId;
-    } catch (IOException e) {
-      // Clean up
-      revokeUserAccessId(accessId);
-      tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+        // New entry in tenant cache
+        tenantCache.put(tenantId, new CachedTenantState(
+            tenantId, userRoleName, adminRoleName));
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
+    }
 
-      throw new OMException(e.getMessage(), TENANT_AUTHORIZER_ERROR);
-    } finally {
-      tenantCacheLock.writeLock().unlock();
+    @Override
+    public void deleteTenant(Tenant tenant) throws IOException {
+
+      final String tenantId = tenant.getTenantId();
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        if (tenantCache.containsKey(tenantId)) {
+          LOG.info("Removing tenant from in-memory cache: {}", tenantId);
+          tenantCache.remove(tenantId);
+        } else {
+          throw new OMException("Tenant does not exist in cache: " + tenantId,
+              INTERNAL_ERROR);
+        }
+      } finally {
+        tenantCacheLock.writeLock().unlock();
+      }
     }
-  }
 
-  @Override
-  public void revokeUserAccessId(String accessId) throws IOException {
-    try {
+    @Override
+    public void assignUserToTenant(String userPrincipal,
+        String tenantId, String accessId) {
+
+      final CachedAccessIdInfo cacheEntry =
+          new CachedAccessIdInfo(userPrincipal, false);
+
       tenantCacheLock.writeLock().lock();
-      final OmDBAccessIdInfo omDBAccessIdInfo =
-          omMetadataManager.getTenantAccessIdTable().get(accessId);
-      if (omDBAccessIdInfo == null) {
-        throw new OMException(INVALID_ACCESS_ID);
+      try {
+        final CachedTenantState cachedTenantState = tenantCache.get(tenantId);
+        Preconditions.checkNotNull(cachedTenantState,
+            "Cache entry for tenant '" + tenantId + "' does not exist");
+
+        LOG.info("Adding to cache: user '{}' accessId '{}' in tenant '{}'",
+            userPrincipal, accessId, tenantId);
+        cachedTenantState.getAccessIdInfoMap().put(accessId, cacheEntry);
+      } catch (Exception e) {
+        // Attempt to clean up tenant cache entry on exception
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+      } finally {
+        tenantCacheLock.writeLock().unlock();
       }
-      final String tenantId = omDBAccessIdInfo.getTenantId();
-      if (tenantId == null) {
-        LOG.error("Tenant doesn't exist");
-        return;
+    }
+
+    @Override
+    public void revokeUserAccessId(String accessId, String tenantId)
+        throws IOException {
+
+      tenantCacheLock.writeLock().lock();
+      try {
+        LOG.info("Removing from cache: accessId '{}' in tenant '{}'",
+            accessId, tenantId);
+        if (!tenantCache.get(tenantId).getAccessIdInfoMap()
+            .containsKey(accessId)) {
+          throw new OMException("accessId '" + accessId + "' doesn't exist "
+              + "in tenant cache!", INTERNAL_ERROR);
+        }
+        tenantCache.get(tenantId).getAccessIdInfoMap().remove(accessId);
+      } catch (NullPointerException e) {
+        // tenantCache is somehow empty. Ignore for now.
+        LOG.warn("NPE when removing accessId from cache", e);

Review Comment:
   Actually, NPE shouldn't be explicitly caught and ignored here.
   
   `assign` really should have put it there in the cache. Otherwise we have a cache consistency issue.



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

[GitHub] [ozone] smengcl commented on pull request #3450: HDDS-6701. [Multi-Tenant] Add proper locking between Ranger background sync service and tenant requests; bug fixes

Posted by GitBox <gi...@apache.org>.
smengcl commented on PR #3450:
URL: https://github.com/apache/ozone/pull/3450#issuecomment-1139184432

   Thanks @errose28 for the review. I have addressed the comments. Please take another look, thx!


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