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/11 20:55:43 UTC
[GitHub] [ozone] errose28 commented on a diff in pull request #3397: HDDS-6612. [Multi-Tenant] Add a config key to enable or disable S3 Multi-Tenancy feature
errose28 commented on code in PR #3397:
URL: https://github.com/apache/ozone/pull/3397#discussion_r870736198
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -653,8 +707,12 @@ private void logVersionMismatch(OzoneConfiguration conf, ScmInfo scmInfo) {
private void instantiateServices(boolean withNewSnapshot) throws IOException {
metadataManager = new OmMetadataManagerImpl(configuration);
- multiTenantManager = new OMMultiTenantManagerImpl(this, configuration);
- OzoneAclUtils.setOMMultiTenantManager(multiTenantManager);
+ LOG.info("S3 Multi-Tenancy is {}",
+ isS3MultiTenancyEnabled ? "enabled" : "disabled");
Review Comment:
Left over debug statement? We should have covered necessary logging for enabling/disabling in the OM constructor.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -534,6 +542,52 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption)
blockTokenMgr = createBlockTokenSecretManager(configuration);
}
+ // Get S3 multi-tenancy enabled flag
+ this.isS3MultiTenancyEnabled = conf.getBoolean(
+ OZONE_OM_MULTITENANCY_ENABLED, OZONE_OM_MULTITENANCY_ENABLED_DEFAULT);
+
+ boolean devSkipMTCheck = conf.getBoolean(OZONE_OM_TENANT_DEV_SKIP_RANGER,
+ false);
+
+ if (isS3MultiTenancyEnabled && !devSkipMTCheck) {
+ // Validate required configs to enable S3 multi-tenancy
Review Comment:
I think making these fatal would be better user experience. If a user has enabled multi-tenancy, they are not expecting the OM start with it disabled, and may miss a log message.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -534,6 +542,52 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption)
blockTokenMgr = createBlockTokenSecretManager(configuration);
}
+ // Get S3 multi-tenancy enabled flag
Review Comment:
Can we do this in a helper method? OM constructor is already quite large.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -3125,6 +3203,8 @@ public S3VolumeContext getS3VolumeContext() throws IOException {
multiTenantManager.getTenantForAccessID(accessId);
if (optionalTenantId.isPresent()) {
+ // To block all S3 multi-tenancy requests if disabled
Review Comment:
The offline answer to this question was that access to multi-tenant data through non-S3 methods would still work as long as Ranger was still configured with acls to allow access.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java:
##########
@@ -756,6 +814,26 @@ public boolean isGrpcBlockTokenEnabled() {
return grpcBlockTokenEnabled;
}
+ /**
+ * Return config value of {@link OMConfigKeys#OZONE_OM_MULTITENANCY_ENABLED}.
+ */
+ public boolean isS3MultiTenancyEnabled() {
Review Comment:
I don't see any callers of this method. If we make config errors fatal it would be the same as checking the value of the config key.
--
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