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/02/23 21:14:42 UTC
[GitHub] [ozone] errose28 commented on a change in pull request #3051: HDDS-6063. [Multi-Tenant] Use VOLUME_LOCK in read and write requests, and some minor refactoring
errose28 commented on a change in pull request #3051:
URL: https://github.com/apache/ozone/pull/3051#discussion_r812469110
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3000,6 +3010,10 @@ public TenantUserInfoValue tenantGetUserInfo(String userPrincipal)
final List<TenantAccessIdInfo> accessIdInfoList = new ArrayList<>();
+ // Won't iterate cache here for a similar reason as in OM#listTenant
+ // tenantGetUserInfo lists all accessIds assigned to a user across
+ // multiple tenants.
+
Review comment:
Since we aren't taking a lock here, the info we got on line 3018 may be stale by the time we iterate it and re-check the database with it on line 3031. We need more graceful handling of this legit case instead of the error log and NPE. Possibly skip processing for that access ID, with a comment in the code acknowledging that we aren't taking a lock so it is ok that the expected access ID is not present in the DB anymore.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
##########
@@ -374,10 +374,15 @@ public void deactivateUser(String accessID)
}
@Override
- public boolean isTenantAdmin(String user, String tenantName) {
+ public boolean isTenantAdmin(String user, String tenantId) {
Review comment:
There's a lot of problematic method stubs and unused methods in this class, and this is probably one of the worst examples. We must be sure to purge anything like this stub before the merge.
For admin checks specifically, it's a bit of a mess. At the top level, we have this method in `OMMultiTenantManager`, and `OMTenantRequestHelper#checkTenantAdmin`. There's one in the `OzoneManager` as well that `OMTenantRequestHelper#checkTenantAdmin` delegates to, but I don't think that belongs there because it does not use any OM instance fields and is only called from outside the OM.
As it is I'm not sure the difference between `OMMultiTenantManager` and `OMTenantRequestHelper`. `OMTenantRequestHelper` is not useful as a static utility class because it is called from requests which can already access `OMMultiTenantManager` through their OM instance. I think the two should be consolidated to reduce code duplication, bugs, and confusion. With these admin checks, for example, we have all three of those.
I think a follow up Jira is necessary to consolidate these two classes and clean them out so they only contain methods that are both needed and correctly implemented.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
##########
@@ -380,19 +372,19 @@ public OMClientResponse validateAndUpdateCache(
// Audit
auditMap.put(OzoneConsts.TENANT, tenantId);
- auditMap.put("user", principal);
+ auditMap.put("user", userPrincipal);
auditMap.put("accessId", accessId);
auditLog(ozoneManager.getAuditLogger(), buildAuditMessage(
OMAction.TENANT_ASSIGN_USER_ACCESSID, auditMap, exception,
getOmRequest().getUserInfo()));
if (exception == null) {
LOG.info("Assigned user '{}' to tenant '{}' with accessId '{}'",
- principal, tenantId, accessId);
+ userPrincipal, tenantId, accessId);
// TODO: omMetrics.incNumTenantAssignUser()
} else {
LOG.error("Failed to assign '{}' to tenant '{}' with accessId '{}': {}",
- principal, tenantId, accessId, exception.getMessage());
+ userPrincipal, tenantId, accessId, exception.getMessage());
Review comment:
The message here should be sufficient, since we are returning the error response. We can remove the todo on the line below this.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
##########
@@ -380,19 +372,19 @@ public OMClientResponse validateAndUpdateCache(
// Audit
auditMap.put(OzoneConsts.TENANT, tenantId);
- auditMap.put("user", principal);
+ auditMap.put("user", userPrincipal);
auditMap.put("accessId", accessId);
auditLog(ozoneManager.getAuditLogger(), buildAuditMessage(
OMAction.TENANT_ASSIGN_USER_ACCESSID, auditMap, exception,
getOmRequest().getUserInfo()));
if (exception == null) {
LOG.info("Assigned user '{}' to tenant '{}' with accessId '{}'",
- principal, tenantId, accessId);
+ userPrincipal, tenantId, accessId);
// TODO: omMetrics.incNumTenantAssignUser()
Review comment:
Do we have jiras for these metrics related TODOs? We should file some and link them here if not already.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
##########
@@ -308,20 +302,20 @@ public OMClientResponse validateAndUpdateCache(
principalInfo.addAccessId(accessId);
}
omMetadataManager.getPrincipalToAccessIdsTable().addCacheEntry(
- new CacheKey<>(principal),
+ new CacheKey<>(userPrincipal),
new CacheValue<>(Optional.of(principalInfo),
transactionLogIndex));
// Add to tenantGroupTable
- // TODO: DOUBLE CHECK GROUP NAME USAGE
+ // TODO: TenantGroupTable is unused for now.
final String defaultGroupName =
tenantId + OzoneConsts.DEFAULT_TENANT_USER_GROUP_SUFFIX;
omMetadataManager.getTenantGroupTable().addCacheEntry(
new CacheKey<>(accessId),
new CacheValue<>(Optional.of(defaultGroupName), transactionLogIndex));
// Add to tenantRoleTable
- // TODO: DOUBLE CHECK ROLENAME
+ // TODO: TenantRoleTable is unused for now.
Review comment:
Do we have plans to use this later? maybe as part of the Ranger background sync?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3054,17 +3068,30 @@ public TenantUserList listUsersInTenant(String tenantId, String prefix)
return null;
}
+ if (!multiTenantManager.tenantExists(tenantId)) {
+ // Throw exception to the client, which will handle this gracefully
+ throw new IOException("Tenant '" + tenantId + "' not found!");
Review comment:
Would an OMException with result code be better here?
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
##########
@@ -308,20 +302,20 @@ public OMClientResponse validateAndUpdateCache(
principalInfo.addAccessId(accessId);
}
omMetadataManager.getPrincipalToAccessIdsTable().addCacheEntry(
- new CacheKey<>(principal),
+ new CacheKey<>(userPrincipal),
new CacheValue<>(Optional.of(principalInfo),
transactionLogIndex));
// Add to tenantGroupTable
- // TODO: DOUBLE CHECK GROUP NAME USAGE
+ // TODO: TenantGroupTable is unused for now.
Review comment:
If it is unused we should remove it. Once it is released we will need to keep it forever. Also, we are using roles, not groups now, so I'm not sure what purpose this table would serve.
--
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