errose28 commented on a change in pull request #3177:
URL: https://github.com/apache/ozone/pull/3177#discussion_r827255565
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
##########
@@ -378,9 +382,60 @@ public void deactivateUser(String accessID)
}
- @Override
- public boolean isTenantAdmin(String user, String tenantId) {
- return true;
+ /**
+ * {@inheritDoc}
+ */
+ public boolean isTenantAdmin(UserGroupInformation callerUgi,
+ String tenantId, Boolean delegated) {
+ if (callerUgi == null) {
+ return false;
+ } else {
+ return isTenantAdmin(
+ callerUgi.getShortUserName(), tenantId, delegated)
+ || isTenantAdmin(
+ callerUgi.getUserName(), tenantId, delegated)
+ || ozoneManager.isAdmin(callerUgi.getShortUserName())
+ || ozoneManager.isAdmin(callerUgi.getUserName());
+ }
+ }
+
+ /**
+ * Internal isTenantAdmin method that takes a username String instead of UGI.
+ */
+ private boolean isTenantAdmin(String username, String tenantId,
+ Boolean delegated) {
+ if (StringUtils.isEmpty(username) || StringUtils.isEmpty(tenantId)) {
+ return false;
+ }
+
+ try {
+ final OmDBUserPrincipalInfo principalInfo =
+ omMetadataManager.getPrincipalToAccessIdsTable().get(username);
+
+ if (principalInfo == null) {
+ // The user is not assigned to any tenant
+ return false;
+ }
+
+ // Find accessId assigned to the specified tenant
+ for (final String accessId : principalInfo.getAccessIds()) {
+ final OmDBAccessIdInfo accessIdInfo =
+ omMetadataManager.getTenantAccessIdTable().get(accessId);
+ if (tenantId.equals(accessIdInfo.getTenantId())) {
Review comment:
accessIdInfo could be null since we may not have a lock on the tenant.
We can either lock on the tenant volume, or add a null check before this line
and return false if null.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
##########
@@ -3089,11 +3088,11 @@ public TenantUserList listUsersInTenant(String
tenantId, String prefix)
boolean lockAcquired =
metadataManager.getLock().acquireReadLock(VOLUME_LOCK, volumeName);
try {
- String userName = getRemoteUser().getUserName();
- if (!multiTenantManager.isTenantAdmin(userName, tenantId)
- && !omAdminUsernames.contains(userName)) {
- throw new IOException("Only tenant and ozone admins can access this " +
- "API. '" + userName + "' is not an admin.");
+ final UserGroupInformation ugi =
ProtobufRpcEngine.Server.getRemoteUser();
Review comment:
Wouldn't this be the s3 gateway's UGI?
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDBAccessIdInfo.java
##########
@@ -42,16 +47,23 @@
* Only effective if isAdmin is true.
*/
private final boolean isDelegatedAdmin;
+ /**
+ * Role names of the user (that this access ID is assigned to) in this
tenant.
+ * e.g. OzoneConsts.TENANT_ROLE_USER, OzoneConsts.TENANT_ROLE_ADMIN,
+ * or other custom role names.
+ */
+ private final Set<String> roleIds;
Review comment:
This name is kind of confusing, because in Ranger the role ID is a
number it generates to identify the role. According to Ranger, the role name is
really what we are storing here.
##########
File path: hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
##########
@@ -1414,14 +1414,41 @@ message TenantInfo {
optional string bucketPolicyGroupName = 5;
}
+message TenantUserPrincipalInfo {
+ repeated string accessIds = 1;
+}
+
+// For storing extended access ID information (related to a tenant) to OM DB
+// Does not include the access ID value itself
+message ExtendedAccessIdInfo {
+ optional string tenantId = 1;
+ optional string userPrincipal = 2;
+ optional bool isAdmin = 3;
+ optional bool isDelegatedAdmin = 4;
+ repeated string roleIds = 5;
+}
+
+// For `tenant user info` to store access ID along with its tenant-related info
+message TenantAccessIdInfo {
+ optional string accessId = 1;
+ optional string tenantId = 2;
+ optional bool isAdmin = 3;
+ optional bool isDelegatedAdmin = 4;
+}
+
+message TenantUserInfo {
Review comment:
I think we can pull this message's fields into the
TenantGetUserInfoResponse.
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -345,7 +345,11 @@ private OzoneConsts() {
public static final String DEFAULT_TENANT_USER_POLICY_SUFFIX = "-users";
public static final String DEFAULT_TENANT_BUCKET_POLICY_SUFFIX = "-buckets";
public static final String DEFAULT_TENANT_POLICY_ID_SUFFIX = "-default";
- public static final String DEFAULT_TENANT_USER_GROUP_SUFFIX = "-users";
+ // Predefined tenant roles
+ // Tenant user role. All tenant users are assigned this role by default
+ public static final String TENANT_ROLE_USER = "user";
+ // Tenant admin role. All tenant admins are assigned this role
+ public static final String TENANT_ROLE_ADMIN = "admin";
Review comment:
Also I think it would be better if these were internal to the
OMMultiTenantManager, and static helpers were used to construct policy and role
names.
##########
File path:
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
##########
@@ -345,7 +345,11 @@ private OzoneConsts() {
public static final String DEFAULT_TENANT_USER_POLICY_SUFFIX = "-users";
public static final String DEFAULT_TENANT_BUCKET_POLICY_SUFFIX = "-buckets";
public static final String DEFAULT_TENANT_POLICY_ID_SUFFIX = "-default";
- public static final String DEFAULT_TENANT_USER_GROUP_SUFFIX = "-users";
+ // Predefined tenant roles
+ // Tenant user role. All tenant users are assigned this role by default
+ public static final String TENANT_ROLE_USER = "user";
+ // Tenant admin role. All tenant admins are assigned this role
+ public static final String TENANT_ROLE_ADMIN = "admin";
Review comment:
Roles in Ranger don't have scope, so these should be suffixes to the
tenant name, like we do for the policies.
##########
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/OmDBTenantInfo.java
##########
@@ -152,6 +109,33 @@ public String getBucketPolicyGroupName() {
return bucketPolicyGroupName;
}
+ /**
+ * Convert OmDBTenantInfo to protobuf to be persisted to DB.
+ */
+ public OzoneManagerProtocolProtos.TenantInfo getProtobuf() {
+ return OzoneManagerProtocolProtos.TenantInfo.newBuilder()
+ .setTenantId(tenantId)
+ .setBucketNamespaceName(bucketNamespaceName)
+ .setAccountNamespaceName(accountNamespaceName)
Review comment:
I don't think this is used anymore, or even defined. Can we remove it
from the proto?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java
##########
@@ -199,15 +199,14 @@
OmDBAccessIdInfo.class, // tenantId, secret, principal
new OmDBAccessIdInfoCodec());
- public static final DBColumnFamilyDefinition<String,
- OmDBKerberosPrincipalInfo>
+ public static final DBColumnFamilyDefinition<String, OmDBUserPrincipalInfo>
PRINCIPAL_TO_ACCESS_IDS_TABLE =
new DBColumnFamilyDefinition<>(
OmMetadataManagerImpl.PRINCIPAL_TO_ACCESS_IDS_TABLE,
- String.class, // Kerberos principal
+ String.class, // User principal
new StringCodec(),
- OmDBKerberosPrincipalInfo.class, // List of accessIds
- new OmDBKerberosPrincipalInfoCodec());
+ OmDBUserPrincipalInfo.class, // List of accessIds
+ new OmDBUserPrincipalInfoCodec());
public static final DBColumnFamilyDefinition<String, OmDBTenantInfo>
TENANT_STATE_TABLE =
Review comment:
Also could we standardize the naming here, something like
TENANT_STATE_TABLE with OmDBTenantState or TENANT_INFO_TABLE with
OmDBTenantInfo?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRequestHelper.java
##########
@@ -59,16 +59,17 @@ static void checkAdmin(OzoneManager ozoneManager) throws
OMException {
}
/**
- * Passes check if caller is an Ozone cluster admin or tenant delegated
admin,
- * throws OMException otherwise.
+ * Check if caller is an Ozone cluster admin or tenant (delegated) admin.
+ * Throws PERMISSION_DENIED if the check failed.
+ *
* @throws OMException PERMISSION_DENIED
*/
static void checkTenantAdmin(OzoneManager ozoneManager, String tenantId)
throws OMException {
final UserGroupInformation ugi = ProtobufRpcEngine.Server.getRemoteUser();
Review comment:
Same as above, I'm not sure whether the UGI value here will be correct.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java
##########
@@ -218,24 +217,6 @@
OmDBTenantInfo.class,
new OmDBTenantInfoCodec());
- public static final DBColumnFamilyDefinition<String, String>
- TENANT_GROUP_TABLE =
- new DBColumnFamilyDefinition<>(
- OmMetadataManagerImpl.TENANT_GROUP_TABLE,
- String.class,
- new StringCodec(),
- String.class,
- new StringCodec());
-
- public static final DBColumnFamilyDefinition<String, String>
- TENANT_ROLE_TABLE =
- new DBColumnFamilyDefinition<>(
- OmMetadataManagerImpl.TENANT_ROLE_TABLE,
- String.class,
- new StringCodec(),
- String.class,
- new StringCodec());
-
public static final DBColumnFamilyDefinition<String, String>
TENANT_POLICY_TABLE =
Review comment:
Can this policy table be combined with the tenant state table?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
##########
@@ -378,9 +382,60 @@ public void deactivateUser(String accessID)
}
- @Override
- public boolean isTenantAdmin(String user, String tenantId) {
- return true;
+ /**
+ * {@inheritDoc}
+ */
+ public boolean isTenantAdmin(UserGroupInformation callerUgi,
+ String tenantId, Boolean delegated) {
+ if (callerUgi == null) {
+ return false;
+ } else {
+ return isTenantAdmin(
+ callerUgi.getShortUserName(), tenantId, delegated)
+ || isTenantAdmin(
+ callerUgi.getUserName(), tenantId, delegated)
+ || ozoneManager.isAdmin(callerUgi.getShortUserName())
+ || ozoneManager.isAdmin(callerUgi.getUserName());
+ }
+ }
+
+ /**
+ * Internal isTenantAdmin method that takes a username String instead of UGI.
+ */
+ private boolean isTenantAdmin(String username, String tenantId,
+ Boolean delegated) {
Review comment:
Why Boolean object instead of primitive?
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/codec/OMDBDefinition.java
##########
@@ -199,15 +199,14 @@
OmDBAccessIdInfo.class, // tenantId, secret, principal
new OmDBAccessIdInfoCodec());
- public static final DBColumnFamilyDefinition<String,
- OmDBKerberosPrincipalInfo>
+ public static final DBColumnFamilyDefinition<String, OmDBUserPrincipalInfo>
PRINCIPAL_TO_ACCESS_IDS_TABLE =
new DBColumnFamilyDefinition<>(
OmMetadataManagerImpl.PRINCIPAL_TO_ACCESS_IDS_TABLE,
- String.class, // Kerberos principal
+ String.class, // User principal
new StringCodec(),
- OmDBKerberosPrincipalInfo.class, // List of accessIds
- new OmDBKerberosPrincipalInfoCodec());
+ OmDBUserPrincipalInfo.class, // List of accessIds
+ new OmDBUserPrincipalInfoCodec());
public static final DBColumnFamilyDefinition<String, OmDBTenantInfo>
TENANT_STATE_TABLE =
Review comment:
Can we add a comment that the string key is a tenant ID?
--
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]
