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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
