From 8a242b09879ea1eb79f7938876b9b47b28bf3c26 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Fri, 4 Oct 2024 12:48:58 +0530 Subject: [PATCH 01/35] HDDS-11041. Add admin request filter for S3 requests and passing UGI for GrpcOMTransport --- .../om/ha/GrpcOMFailoverProxyProvider.java | 21 ++++- .../ozone/om/protocolPB/GrpcOmTransport.java | 1 + .../ozone/s3secret/S3AdminEndpoint.java | 34 ++++++++ .../ozone/s3secret/S3SecretAdminFilter.java | 80 +++++++++++++++++++ .../s3secret/S3SecretManagementEndpoint.java | 4 +- 5 files changed, 137 insertions(+), 3 deletions(-) create mode 100644 hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java create mode 100644 hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index 65d9e559005..2100ed5e8cf 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -23,6 +23,8 @@ import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.HddsUtils; import org.apache.hadoop.hdds.utils.LegacyHadoopConfigurationSource; +import org.apache.hadoop.io.retry.RetryPolicies; +import org.apache.hadoop.io.retry.RetryPolicy; import org.apache.hadoop.ipc.RPC; import org.apache.hadoop.net.NetUtils; import org.apache.hadoop.ozone.OmUtils; @@ -41,6 +43,7 @@ import java.util.Optional; import java.util.OptionalInt; import io.grpc.StatusRuntimeException; +import org.apache.hadoop.security.UserGroupInformation; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -59,10 +62,14 @@ public class GrpcOMFailoverProxyProvider extends public static final Logger LOG = LoggerFactory.getLogger(GrpcOMFailoverProxyProvider.class); + private final UserGroupInformation ugi; + public GrpcOMFailoverProxyProvider(ConfigurationSource configuration, + UserGroupInformation ugi, String omServiceId, Class protocol) throws IOException { super(configuration, omServiceId, protocol); + this.ugi = ugi; } @Override @@ -118,9 +125,21 @@ private T createOMProxy() throws IOException { InetSocketAddress addr = new InetSocketAddress(0); Configuration hadoopConf = LegacyHadoopConfigurationSource.asHadoopConfiguration(getConf()); - return (T) RPC.getProxy(getInterface(), 0, addr, hadoopConf); + + // Ensure we do not attempt retry on the same OM in case of exceptions + RetryPolicy connectionRetryPolicy = RetryPolicies.failoverOnNetworkException(0); + + return (T) RPC.getProtocolProxy( + getInterface(), + 0, + addr, ugi, + hadoopConf, NetUtils.getDefaultSocketFactory(hadoopConf), + (int) OmUtils.getOMClientRpcTimeOut(getConf()), + connectionRetryPolicy).getProxy(); } + + /** * Get the proxy object which should be used until the next failover event * occurs. RPC proxy object is initialized lazily. diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java index ac2e85da84d..c9eb9cbb44f 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java @@ -121,6 +121,7 @@ public GrpcOmTransport(ConfigurationSource conf, omFailoverProxyProvider = new GrpcOMFailoverProxyProvider( conf, + ugi, omServiceId, OzoneManagerProtocolPB.class); diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java new file mode 100644 index 00000000000..285bbb873e8 --- /dev/null +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java @@ -0,0 +1,34 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + * + */ + +package org.apache.hadoop.ozone.s3secret; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import javax.ws.rs.NameBinding; + +/** + * Annotation to disable S3 Secure Endpoint. + */ +@NameBinding +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface S3AdminEndpoint { +} diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java new file mode 100644 index 00000000000..8da6119f1f6 --- /dev/null +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -0,0 +1,80 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + * + */ + +package org.apache.hadoop.ozone.s3secret; + + +import javax.inject.Inject; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.container.ContainerRequestFilter; +import javax.ws.rs.core.Response; +import javax.ws.rs.ext.Provider; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.server.OzoneAdmins; +import org.apache.hadoop.ozone.OzoneConfigKeys; +import org.apache.hadoop.security.UserGroupInformation; + +import java.io.IOException; +import java.security.Principal; +import java.util.Collection; + +/** + * Filter that disables all endpoints annotated with {@link S3AdminEndpoint}. + * Condition is based on the value of the configuration keys for + * - ozone.administrators + * - ozone.administrators.groups + */ + +@S3AdminEndpoint +@Provider +public class S3SecretAdminFilter implements ContainerRequestFilter { + @Inject + private OzoneConfiguration conf; + + @Override + public void filter(ContainerRequestContext requestContext) + throws IOException { + final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); + if (null != userPrincipal) { + UserGroupInformation user = + UserGroupInformation.createRemoteUser(userPrincipal.getName()); + if (!isAdmin(user)) { + requestContext.abortWith(Response.status(Response.Status.FORBIDDEN) + .entity("Non-Admin user accessing endpoint") + .build()); + } + } + } + + /** + * Checks if the user that is passed is an Admin + * @param user Stores the user to filter + * @return true if the user is admin else false + */ + private boolean isAdmin(UserGroupInformation user) { + Collection admins = + conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS); + Collection adminGroups = + conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS); + // If there are no admins or admin groups specified, return false + if (admins.isEmpty() || adminGroups.isEmpty()) { + return false; + } + return new OzoneAdmins(admins, adminGroups).isAdmin(user); + } +} diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java index 4ea17d2a2fd..e8ca32f0dfa 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java @@ -41,6 +41,7 @@ */ @Path("/secret") @S3SecretEnabled +@S3AdminEndpoint public class S3SecretManagementEndpoint extends S3SecretEndpointBase { private static final Logger LOG = LoggerFactory.getLogger(S3SecretManagementEndpoint.class); @@ -54,8 +55,7 @@ public Response generate() throws IOException { @Path("/{username}") public Response generate(@PathParam("username") String username) throws IOException { - // TODO: It is a temporary solution. To be removed after HDDS-11041 is done. - return Response.status(METHOD_NOT_ALLOWED).build(); + return generateInternal(username); } private Response generateInternal(@Nullable String username) throws IOException { From b9002fb259c80afa246246f7e27c8849ae3b0093 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Fri, 4 Oct 2024 13:11:29 +0530 Subject: [PATCH 02/35] Fixed checkstyle issues --- .../hadoop/ozone/s3secret/S3SecretAdminFilter.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index 8da6119f1f6..777ed11c0d0 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -52,25 +52,25 @@ public void filter(ContainerRequestContext requestContext) final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); if (null != userPrincipal) { UserGroupInformation user = - UserGroupInformation.createRemoteUser(userPrincipal.getName()); + UserGroupInformation.createRemoteUser(userPrincipal.getName()); if (!isAdmin(user)) { requestContext.abortWith(Response.status(Response.Status.FORBIDDEN) - .entity("Non-Admin user accessing endpoint") - .build()); + .entity("Non-Admin user accessing endpoint") + .build()); } } } /** - * Checks if the user that is passed is an Admin + * Checks if the user that is passed is an Admin. * @param user Stores the user to filter * @return true if the user is admin else false */ private boolean isAdmin(UserGroupInformation user) { Collection admins = - conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS); + conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS); Collection adminGroups = - conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS); + conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS); // If there are no admins or admin groups specified, return false if (admins.isEmpty() || adminGroups.isEmpty()) { return false; From d5272f54043a6fa156f3d2ac5fb1edb6a5a364e4 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Fri, 4 Oct 2024 14:31:56 +0530 Subject: [PATCH 03/35] Using OzoneConfigUtils to fetch s3 admins --- .../hadoop/ozone/om/OzoneConfigUtil.java | 4 ++-- hadoop-ozone/s3gateway/pom.xml | 6 ++++++ .../ozone/s3secret/S3SecretAdminFilter.java | 21 ++++++++++++------- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java index c09c5b91af5..e5a325cf527 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java @@ -49,7 +49,7 @@ private OzoneConfigUtil() { * If ozone.s3.administrators value is empty string or unset, * defaults to ozone.administrators value. */ - static Collection getS3AdminsFromConfig(OzoneConfiguration conf) + public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { Collection ozAdmins = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); @@ -63,7 +63,7 @@ static Collection getS3AdminsFromConfig(OzoneConfiguration conf) return ozAdmins; } - static Collection getS3AdminsGroupsFromConfig( + public static Collection getS3AdminsGroupsFromConfig( OzoneConfiguration conf) { Collection s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS_GROUPS); diff --git a/hadoop-ozone/s3gateway/pom.xml b/hadoop-ozone/s3gateway/pom.xml index c26171d98ac..4fe56dc4848 100644 --- a/hadoop-ozone/s3gateway/pom.xml +++ b/hadoop-ozone/s3gateway/pom.xml @@ -241,6 +241,12 @@ commons-lang3 + + + org.apache.ozone + ozone-manager + + org.apache.ozone diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index 777ed11c0d0..d31e69eff94 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -26,12 +26,13 @@ import javax.ws.rs.ext.Provider; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.server.OzoneAdmins; -import org.apache.hadoop.ozone.OzoneConfigKeys; +import org.apache.hadoop.ozone.om.OzoneConfigUtil; import org.apache.hadoop.security.UserGroupInformation; import java.io.IOException; import java.security.Principal; import java.util.Collection; +import java.util.Collections; /** * Filter that disables all endpoints annotated with {@link S3AdminEndpoint}. @@ -67,14 +68,20 @@ public void filter(ContainerRequestContext requestContext) * @return true if the user is admin else false */ private boolean isAdmin(UserGroupInformation user) { - Collection admins = - conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS); - Collection adminGroups = - conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS); + Collection s3Admins; + try { + s3Admins = OzoneConfigUtil.getS3AdminsFromConfig(conf); + } catch (IOException ie) { + // We caught an exception while trying to log in using the UGI user + // Refer to UserGroupInformation.getCurrentUser() in getS3AdminsFromConfig + s3Admins = Collections.emptyList(); + } + Collection s3AdminGroups = + OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); // If there are no admins or admin groups specified, return false - if (admins.isEmpty() || adminGroups.isEmpty()) { + if (s3Admins.isEmpty() || s3AdminGroups.isEmpty()) { return false; } - return new OzoneAdmins(admins, adminGroups).isAdmin(user); + return new OzoneAdmins(s3Admins, s3AdminGroups).isAdmin(user); } } From 932fffc138ac2681f1362b586b1781f4280bd7b9 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Fri, 4 Oct 2024 16:14:58 +0530 Subject: [PATCH 04/35] Addressed codestyle review comments --- .../om/ha/GrpcOMFailoverProxyProvider.java | 11 +++++----- hadoop-ozone/s3gateway/pom.xml | 10 ++++------ .../ozone/s3secret/S3AdminEndpoint.java | 2 +- .../ozone/s3secret/S3SecretAdminFilter.java | 20 +++++++++---------- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index 2100ed5e8cf..948322ae8c8 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -132,14 +132,15 @@ private T createOMProxy() throws IOException { return (T) RPC.getProtocolProxy( getInterface(), 0, - addr, ugi, - hadoopConf, NetUtils.getDefaultSocketFactory(hadoopConf), + addr, + ugi, + hadoopConf, + NetUtils.getDefaultSocketFactory(hadoopConf), (int) OmUtils.getOMClientRpcTimeOut(getConf()), - connectionRetryPolicy).getProxy(); + connectionRetryPolicy + ).getProxy(); } - - /** * Get the proxy object which should be used until the next failover event * occurs. RPC proxy object is initialized lazily. diff --git a/hadoop-ozone/s3gateway/pom.xml b/hadoop-ozone/s3gateway/pom.xml index 4fe56dc4848..1cc06f6153a 100644 --- a/hadoop-ozone/s3gateway/pom.xml +++ b/hadoop-ozone/s3gateway/pom.xml @@ -231,6 +231,10 @@ org.apache.ozone ozone-client + + org.apache.ozone + ozone-manager + org.apache.ozone hdds-docs @@ -241,12 +245,6 @@ commons-lang3 - - - org.apache.ozone - ozone-manager - - org.apache.ozone diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java index 285bbb873e8..b5c7b242cb5 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3AdminEndpoint.java @@ -25,7 +25,7 @@ import javax.ws.rs.NameBinding; /** - * Annotation to disable S3 Secure Endpoint. + * Annotation to only allow admin users to access the endpoint. */ @NameBinding @Retention(RetentionPolicy.RUNTIME) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index d31e69eff94..f9f7e78e62c 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -35,27 +35,27 @@ import java.util.Collections; /** - * Filter that disables all endpoints annotated with {@link S3AdminEndpoint}. - * Condition is based on the value of the configuration keys for - * - ozone.administrators - * - ozone.administrators.groups + * Filter that only allows admin to access endpoints annotated with {@link S3AdminEndpoint}. + * Condition is based on the value of the configuration keys for: + *

    + *
  • ozone.administrators
    • + *
  • ozone.administrators.groups
    • + *
*/ - @S3AdminEndpoint @Provider public class S3SecretAdminFilter implements ContainerRequestFilter { + @Inject private OzoneConfiguration conf; @Override - public void filter(ContainerRequestContext requestContext) - throws IOException { + public void filter(ContainerRequestContext requestContext) throws IOException { final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); if (null != userPrincipal) { - UserGroupInformation user = - UserGroupInformation.createRemoteUser(userPrincipal.getName()); + UserGroupInformation user = UserGroupInformation.createRemoteUser(userPrincipal.getName()); if (!isAdmin(user)) { - requestContext.abortWith(Response.status(Response.Status.FORBIDDEN) + requestContext.abortWith(Response.status(403) .entity("Non-Admin user accessing endpoint") .build()); } From a0ac299ee1c8e2085ecbfe1c3fc38b8cc4396c8f Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Sun, 6 Oct 2024 16:07:55 +0530 Subject: [PATCH 05/35] Add common utils to fetch S3 admin users --- .../hadoop/ozone/common/OmUserUtils.java | 83 +++++++++++++++++++ .../apache/hadoop/ozone/om/OzoneManager.java | 9 +- .../ozone/s3secret/S3SecretAdminFilter.java | 35 +------- 3 files changed, 90 insertions(+), 37 deletions(-) create mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java new file mode 100644 index 00000000000..c123a45e7f1 --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java @@ -0,0 +1,83 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS,WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.hadoop.ozone.common; + +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.server.OzoneAdmins; + +import org.apache.hadoop.ozone.om.OzoneConfigUtil; +import org.apache.hadoop.security.UserGroupInformation; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.util.Collection; + +/** + * Utility class to store User related utilities + */ +public final class OmUserUtils { + private static final Logger LOG = + LoggerFactory.getLogger(OmUserUtils.class); + + private OmUserUtils() { } + + /** + * Get the users and groups that are S3 admin. + * @param conf Stores the Ozone configuration being used + * @return an instance of {@link OzoneAdmins} containing the S3 admin users and groups + */ + public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { + Collection s3Admins; + try { + s3Admins = OzoneConfigUtil.getS3AdminsFromConfig(conf); + } catch (IOException ie) { + s3Admins = null; + } + Collection s3AdminGroups = + OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); + if (LOG.isDebugEnabled()) { + if (null == s3Admins) LOG.debug("S3 Admins are not set in configuration"); + if (null == s3AdminGroups) LOG.debug("S3 Admin Groups are not set in configuration"); + } + return new OzoneAdmins(s3Admins, s3AdminGroups); + } + + /** + * Check if the provided user is a part of the S3 admins + * @param user Stores the user to verify + * @param conf Stores the Ozone configuration being used + * @return true if the provided user is an S3 admin else false + */ + public static boolean isS3Admin(UserGroupInformation user, + OzoneConfiguration conf) { + OzoneAdmins s3Admins = getS3Admins(conf); + return null != user && s3Admins.isAdmin(user); + } + + /** + * Check if the provided user is a part of the S3 admins + * @param user Stores the user to verify + * @param s3Admins Stores the users and groups that are admins + * @return true if the provided user is an S3 admin else false + */ + public static boolean isS3Admin(UserGroupInformation user, + OzoneAdmins s3Admins) { + return null != user && s3Admins.isAdmin(user); + } +} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index 0038bca2e32..3b8e444ee58 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -151,6 +151,7 @@ import org.apache.hadoop.ozone.audit.AuditMessage; import org.apache.hadoop.ozone.audit.Auditor; import org.apache.hadoop.ozone.audit.OMAction; +import org.apache.hadoop.ozone.common.OmUserUtils; import org.apache.hadoop.ozone.common.Storage.StorageState; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes; @@ -704,11 +705,7 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption) // Get read only admin list readOnlyAdmins = OzoneAdmins.getReadonlyAdmins(conf); - Collection s3AdminUsernames = - OzoneConfigUtil.getS3AdminsFromConfig(configuration); - Collection s3AdminGroups = - OzoneConfigUtil.getS3AdminsGroupsFromConfig(configuration); - s3OzoneAdmins = new OzoneAdmins(s3AdminUsernames, s3AdminGroups); + s3OzoneAdmins = OmUserUtils.getS3Admins(conf); instantiateServices(false); // Create special volume s3v which is required for S3G. @@ -4338,7 +4335,7 @@ private void checkAdminUserPrivilege(String operation) throws IOException { } public boolean isS3Admin(UserGroupInformation callerUgi) { - return callerUgi != null && s3OzoneAdmins.isAdmin(callerUgi); + return OmUserUtils.isS3Admin(callerUgi, s3OzoneAdmins); } @VisibleForTesting diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index f9f7e78e62c..8b5c2020c24 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -23,16 +23,14 @@ import javax.ws.rs.container.ContainerRequestContext; import javax.ws.rs.container.ContainerRequestFilter; import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response.Status; import javax.ws.rs.ext.Provider; import org.apache.hadoop.hdds.conf.OzoneConfiguration; -import org.apache.hadoop.hdds.server.OzoneAdmins; -import org.apache.hadoop.ozone.om.OzoneConfigUtil; +import org.apache.hadoop.ozone.common.OmUserUtils; import org.apache.hadoop.security.UserGroupInformation; import java.io.IOException; import java.security.Principal; -import java.util.Collection; -import java.util.Collections; /** * Filter that only allows admin to access endpoints annotated with {@link S3AdminEndpoint}. @@ -54,34 +52,9 @@ public void filter(ContainerRequestContext requestContext) throws IOException { final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); if (null != userPrincipal) { UserGroupInformation user = UserGroupInformation.createRemoteUser(userPrincipal.getName()); - if (!isAdmin(user)) { - requestContext.abortWith(Response.status(403) - .entity("Non-Admin user accessing endpoint") - .build()); + if (!OmUserUtils.isS3Admin(user, conf)) { + requestContext.abortWith(Response.status(Status.FORBIDDEN).build()); } } } - - /** - * Checks if the user that is passed is an Admin. - * @param user Stores the user to filter - * @return true if the user is admin else false - */ - private boolean isAdmin(UserGroupInformation user) { - Collection s3Admins; - try { - s3Admins = OzoneConfigUtil.getS3AdminsFromConfig(conf); - } catch (IOException ie) { - // We caught an exception while trying to log in using the UGI user - // Refer to UserGroupInformation.getCurrentUser() in getS3AdminsFromConfig - s3Admins = Collections.emptyList(); - } - Collection s3AdminGroups = - OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); - // If there are no admins or admin groups specified, return false - if (s3Admins.isEmpty() || s3AdminGroups.isEmpty()) { - return false; - } - return new OzoneAdmins(s3Admins, s3AdminGroups).isAdmin(user); - } } From 065f9fdf447284dfb778781d8257e7a1c0af3bdc Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Sun, 6 Oct 2024 20:00:38 +0530 Subject: [PATCH 06/35] Added unauthorized test scenarios and fixed code style --- .../hadoop/ozone/common/OmUserUtils.java | 18 ++++--- .../ozone/s3secret/TestSecretGenerate.java | 47 +++++++++++++++---- .../ozone/s3secret/TestSecretRevoke.java | 45 +++++++++++++----- 3 files changed, 84 insertions(+), 26 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java index c123a45e7f1..1607303ce78 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java @@ -29,11 +29,11 @@ import java.util.Collection; /** - * Utility class to store User related utilities + * Utility class to store User related utilities. */ public final class OmUserUtils { private static final Logger LOG = - LoggerFactory.getLogger(OmUserUtils.class); + LoggerFactory.getLogger(OmUserUtils.class); private OmUserUtils() { } @@ -50,16 +50,20 @@ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { s3Admins = null; } Collection s3AdminGroups = - OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); + OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); if (LOG.isDebugEnabled()) { - if (null == s3Admins) LOG.debug("S3 Admins are not set in configuration"); - if (null == s3AdminGroups) LOG.debug("S3 Admin Groups are not set in configuration"); + if (null == s3Admins) { + LOG.debug("S3 Admins are not set in configuration"); + } + if (null == s3AdminGroups) { + LOG.debug("S3 Admin Groups are not set in configuration"); + } } return new OzoneAdmins(s3Admins, s3AdminGroups); } /** - * Check if the provided user is a part of the S3 admins + * Check if the provided user is a part of the S3 admins. * @param user Stores the user to verify * @param conf Stores the Ozone configuration being used * @return true if the provided user is an S3 admin else false @@ -71,7 +75,7 @@ public static boolean isS3Admin(UserGroupInformation user, } /** - * Check if the provided user is a part of the S3 admins + * Check if the provided user is a part of the S3 admins. * @param user Stores the user to verify * @param s3Admins Stores the users and groups that are admins * @return true if the provided user is an S3 admin else false diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index d1f81faddd2..1139f6d0c3c 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -41,6 +41,8 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.jupiter.MockitoExtension; +import static javax.ws.rs.core.Response.Status.FORBIDDEN; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.notNull; @@ -51,6 +53,7 @@ */ @ExtendWith(MockitoExtension.class) class TestSecretGenerate { + private static final String ADMIN_USER_NAME = "test_admin"; private static final String USER_NAME = "test"; private static final String OTHER_USER_NAME = "test2"; private static final String USER_SECRET = "test_secret"; @@ -76,6 +79,7 @@ private static S3SecretValue getS3SecretValue(InvocationOnMock invocation) { @BeforeEach void setUp() { OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(OZONE_ADMINISTRATORS, ADMIN_USER_NAME); OzoneClient client = new OzoneClientStub(new ObjectStoreStub(conf, proxy)); when(uriInfo.getPathParameters()).thenReturn(new MultivaluedHashMap<>()); @@ -88,20 +92,39 @@ void setUp() { } @Test - void testSecretGenerate() throws IOException { - setupSecurityContext(); + void testUnauthorizedSecretGeneration() throws IOException { + setupSecurityContext(false); + hasNoSecretYet(); + + Response response = endpoint.generate(); + assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); + } + + @Test + void testAuthorizedSecretGeneration() throws IOException { + setupSecurityContext(true); hasNoSecretYet(); S3SecretResponse response = - (S3SecretResponse) endpoint.generate().getEntity(); + (S3SecretResponse) endpoint.generate().getEntity(); assertEquals(USER_SECRET, response.getAwsSecret()); - assertEquals(USER_NAME, response.getAwsAccessKey()); + assertEquals(ADMIN_USER_NAME, response.getAwsAccessKey()); + } + + @Test + void testUnauthorizedUserSecretAlreadyExists() throws IOException { + setupSecurityContext(false); + hasSecretAlready(); + + Response response = endpoint.generate(); + + assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); } @Test - void testIfSecretAlreadyExists() throws IOException { - setupSecurityContext(); + void testAuthorizedUserSecretAlreadyExists() throws IOException { + setupSecurityContext(true); hasSecretAlready(); Response response = endpoint.generate(); @@ -122,8 +145,16 @@ void testSecretGenerateWithUsername() throws IOException { assertEquals(OTHER_USER_NAME, response.getAwsAccessKey()); } - private void setupSecurityContext() { - when(principal.getName()).thenReturn(USER_NAME); + /** + * Provides mocking for {@link ContainerRequestContext}. + * @param isAdmin Stores whether the user to mock is an admin or not + */ + private void setupSecurityContext(boolean isAdmin) { + if (isAdmin) { + when(principal.getName()).thenReturn(ADMIN_USER_NAME); + } else { + when(principal.getName()).thenReturn(USER_NAME); + } when(securityContext.getUserPrincipal()).thenReturn(principal); when(context.getSecurityContext()).thenReturn(securityContext); } diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java index 85e6bd4c10e..ac86829649c 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java @@ -26,9 +26,11 @@ import javax.ws.rs.core.SecurityContext; import javax.ws.rs.core.UriInfo; +import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ozone.client.ObjectStoreStub; import org.apache.hadoop.ozone.client.OzoneClient; import org.apache.hadoop.ozone.client.OzoneClientStub; +import org.apache.hadoop.ozone.client.protocol.ClientProtocol; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.ozone.test.tag.Unhealthy; import org.junit.jupiter.api.BeforeEach; @@ -37,9 +39,8 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; -import static javax.ws.rs.core.Response.Status.NOT_FOUND; -import static javax.ws.rs.core.Response.Status.OK; +import static javax.ws.rs.core.Response.Status.*; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.ACCESS_DENIED; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.S3_SECRET_NOT_FOUND; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -55,14 +56,15 @@ */ @ExtendWith(MockitoExtension.class) public class TestSecretRevoke { + private static final String ADMIN_USER_NAME = "test_admin"; private static final String USER_NAME = "test"; private static final String OTHER_USER_NAME = "test2"; private S3SecretManagementEndpoint endpoint; - - @Mock private ObjectStoreStub objectStore; + @Mock + private ClientProtocol proxy; private ContainerRequestContext context; @Mock private UriInfo uriInfo; @@ -73,6 +75,9 @@ public class TestSecretRevoke { @BeforeEach void setUp() { + OzoneConfiguration conf = new OzoneConfiguration(); + conf.set(OZONE_ADMINISTRATORS, ADMIN_USER_NAME); + objectStore = new ObjectStoreStub(conf, proxy); OzoneClient client = new OzoneClientStub(objectStore); when(uriInfo.getPathParameters()).thenReturn(new MultivaluedHashMap<>()); @@ -84,15 +89,23 @@ void setUp() { endpoint.setContext(context); } - private void mockSecurityContext() { + private void mockSecurityContext(boolean isAdmin) { when(principal.getName()).thenReturn(USER_NAME); when(securityContext.getUserPrincipal()).thenReturn(principal); when(context.getSecurityContext()).thenReturn(securityContext); } @Test - void testSecretRevoke() throws IOException { - mockSecurityContext(); + void testUnauthorizedUserSecretRevoke() throws IOException { + mockSecurityContext(false); + Response response = endpoint.revoke(); + + assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); + } + + @Test + void testAuthorizedUserSecretRevoke() throws IOException { + mockSecurityContext(true); endpoint.revoke(); verify(objectStore, times(1)).revokeS3Secret(eq(USER_NAME)); } @@ -106,8 +119,18 @@ void testSecretRevokeWithUsername() throws IOException { } @Test - void testSecretSequentialRevokes() throws IOException { - mockSecurityContext(); + void testUnauthorizedUserSecretSequentialRevokes() throws IOException { + mockSecurityContext(false); + Response firstResponse = endpoint.revoke(); + assertEquals(FORBIDDEN.getStatusCode(), firstResponse.getStatus()); + + Response secondResponse = endpoint.revoke(); + assertEquals(FORBIDDEN.getStatusCode(), secondResponse.getStatus()); + } + + @Test + void testAuthorizedUserSecretSequentialRevokes() throws IOException { + mockSecurityContext(true); Response firstResponse = endpoint.revoke(); assertEquals(OK.getStatusCode(), firstResponse.getStatus()); doThrow(new OMException(S3_SECRET_NOT_FOUND)) @@ -118,7 +141,7 @@ void testSecretSequentialRevokes() throws IOException { @Test void testSecretRevokesHandlesException() throws IOException { - mockSecurityContext(); + mockSecurityContext(true); doThrow(new OMException(ACCESS_DENIED)) .when(objectStore).revokeS3Secret(any()); Response response = endpoint.revoke(); From 4a3c3d552de366be51e8a8f6e319d3714dcf6413 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Sun, 6 Oct 2024 20:29:03 +0530 Subject: [PATCH 07/35] Fixed findbug issues --- .../hadoop/ozone/s3secret/TestSecretRevoke.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java index ac86829649c..667b0b73cda 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java @@ -39,7 +39,10 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import static javax.ws.rs.core.Response.Status.*; +import static javax.ws.rs.core.Response.Status.FORBIDDEN; +import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; +import static javax.ws.rs.core.Response.Status.NOT_FOUND; +import static javax.ws.rs.core.Response.Status.OK; import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.ACCESS_DENIED; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.S3_SECRET_NOT_FOUND; @@ -90,7 +93,11 @@ void setUp() { } private void mockSecurityContext(boolean isAdmin) { - when(principal.getName()).thenReturn(USER_NAME); + if (isAdmin) { + when(principal.getName()).thenReturn(ADMIN_USER_NAME); + } else { + when(principal.getName()).thenReturn(USER_NAME); + } when(securityContext.getUserPrincipal()).thenReturn(principal); when(context.getSecurityContext()).thenReturn(securityContext); } From cf3c4e34277be232d76b07f059d7dd83f6ddfa98 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Sun, 6 Oct 2024 22:01:28 +0530 Subject: [PATCH 08/35] Mock ContainerRequestContext --- .../org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java | 2 +- .../org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index 1139f6d0c3c..3618b927177 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -146,7 +146,7 @@ void testSecretGenerateWithUsername() throws IOException { } /** - * Provides mocking for {@link ContainerRequestContext}. + * Provides mocking for users and security context. * @param isAdmin Stores whether the user to mock is an admin or not */ private void setupSecurityContext(boolean isAdmin) { diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java index 667b0b73cda..b2077ca7649 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java @@ -68,6 +68,7 @@ public class TestSecretRevoke { @Mock private ClientProtocol proxy; + @Mock private ContainerRequestContext context; @Mock private UriInfo uriInfo; @@ -92,6 +93,10 @@ void setUp() { endpoint.setContext(context); } + /** + * Provides mocking for users and security context. + * @param isAdmin Stores whether the user is admin or not + */ private void mockSecurityContext(boolean isAdmin) { if (isAdmin) { when(principal.getName()).thenReturn(ADMIN_USER_NAME); From ff14d265465018ccd15ebc1fb727921d871ad8a9 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Thu, 10 Oct 2024 14:11:13 +0530 Subject: [PATCH 09/35] Addressed review comments --- .../hadoop/ozone/common/OmUserUtils.java | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java index 1607303ce78..e1fe4f67e5b 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java @@ -19,11 +19,12 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.server.OzoneAdmins; - import org.apache.hadoop.ozone.om.OzoneConfigUtil; import org.apache.hadoop.security.UserGroupInformation; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import jakarta.annotation.Nullable; import java.io.IOException; import java.util.Collection; @@ -32,14 +33,13 @@ * Utility class to store User related utilities. */ public final class OmUserUtils { - private static final Logger LOG = - LoggerFactory.getLogger(OmUserUtils.class); + private static final Logger LOG = LoggerFactory.getLogger(OmUserUtils.class); private OmUserUtils() { } /** - * Get the users and groups that are S3 admin. - * @param conf Stores the Ozone configuration being used + * Get the users and groups that are a part of S3 administrators. + * @param conf Stores an instance of {@link OzoneConfiguration} being used * @return an instance of {@link OzoneAdmins} containing the S3 admin users and groups */ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { @@ -49,8 +49,8 @@ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { } catch (IOException ie) { s3Admins = null; } - Collection s3AdminGroups = - OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); + Collection s3AdminGroups = OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); + if (LOG.isDebugEnabled()) { if (null == s3Admins) { LOG.debug("S3 Admins are not set in configuration"); @@ -63,25 +63,23 @@ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { } /** - * Check if the provided user is a part of the S3 admins. - * @param user Stores the user to verify - * @param conf Stores the Ozone configuration being used - * @return true if the provided user is an S3 admin else false + * Check if the provided user is an S3 administrator. + * @param user an instance of {@link UserGroupInformation} with information about the user to verify + * @param s3Admins an instance of {@link OzoneAdmins} containing information of the S3 administrator users and groups in the system + * @return {@code true} if the provided user is an S3 administrator else {@code false} */ - public static boolean isS3Admin(UserGroupInformation user, - OzoneConfiguration conf) { - OzoneAdmins s3Admins = getS3Admins(conf); + public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins s3Admins) { return null != user && s3Admins.isAdmin(user); } /** - * Check if the provided user is a part of the S3 admins. - * @param user Stores the user to verify - * @param s3Admins Stores the users and groups that are admins - * @return true if the provided user is an S3 admin else false + * Check if the provided user is an S3 administrator. + * @param user an instance of {@link UserGroupInformation} with information about the user to verify + * @param conf Stores an instance of {@link OzoneConfiguration} being used + * @return {@code true} if the provided user is an S3 administrator else {@code false} */ - public static boolean isS3Admin(UserGroupInformation user, - OzoneAdmins s3Admins) { - return null != user && s3Admins.isAdmin(user); + public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneConfiguration conf) { + OzoneAdmins s3Admins = getS3Admins(conf); + return isS3Admin(user, s3Admins); } } From 8e93f5caf1e4dd4b639b6115d99069341ce34d6a Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Fri, 11 Oct 2024 19:11:18 +0530 Subject: [PATCH 10/35] Addressed review comments --- hadoop-ozone/common/pom.xml | 4 + .../hadoop/ozone/conf/OzoneS3ConfigUtils.java | 112 ++++++++++++++++++ .../om/ha/GrpcOMFailoverProxyProvider.java | 28 +++-- .../ozone/conf/TestOzoneConfigUtils.java | 51 ++++++++ .../hadoop/ozone/common/OmUserUtils.java | 85 ------------- .../hadoop/ozone/om/OzoneConfigUtil.java | 42 ------- .../apache/hadoop/ozone/om/OzoneManager.java | 6 +- .../hadoop/ozone/om/TestOzoneConfigUtil.java | 39 ------ hadoop-ozone/s3gateway/pom.xml | 4 - .../ozone/s3secret/S3SecretAdminFilter.java | 4 +- 10 files changed, 192 insertions(+), 183 deletions(-) create mode 100644 hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java create mode 100644 hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneConfigUtils.java delete mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java diff --git a/hadoop-ozone/common/pom.xml b/hadoop-ozone/common/pom.xml index bd16a0a5dfe..fe81ea14864 100644 --- a/hadoop-ozone/common/pom.xml +++ b/hadoop-ozone/common/pom.xml @@ -65,6 +65,10 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd"> org.apache.ozone hdds-config + + org.apache.ozone + hdds-server-framework + org.apache.ozone hdds-interface-client diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java new file mode 100644 index 00000000000..68352cb6c33 --- /dev/null +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java @@ -0,0 +1,112 @@ +package org.apache.hadoop.ozone.conf; + +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.server.OzoneAdmins; +import org.apache.hadoop.security.UserGroupInformation; + +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import jakarta.annotation.Nullable; + +import java.io.IOException; +import java.util.Collection; + +public final class OzoneS3ConfigUtils { + static final Logger LOG = LoggerFactory.getLogger(OzoneS3ConfigUtils.class); + + private OzoneS3ConfigUtils() { } + + /** + * Get the list of S3 administrators from Ozone config. + * + * @param conf An instance of {@link OzoneConfiguration} being used + * @return A {@link Collection} of the S3 administrator users + * + * If ozone.s3.administrators value is empty string or unset, + * defaults to ozone.administrators value. + */ + public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { + Collection ozAdmins = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); + + if (ozAdmins == null || ozAdmins.isEmpty()) { + ozAdmins = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS); + } + String omSPN = UserGroupInformation.getCurrentUser().getShortUserName(); + if (!ozAdmins.contains(omSPN)) { + ozAdmins.add(omSPN); + } + + return ozAdmins; + } + + /** + * Get the list of the groups that are a part S3 administrators from Ozone config. + * + * @param conf An instance of {@link OzoneConfiguration} being used + * @return A {@link Collection} of the S3 administrator groups + * + * If ozone.s3.administrators.groups value is empty or unset, + * defaults to the ozone.administrators.groups value + */ + public static Collection getS3AdminsGroupsFromConfig(OzoneConfiguration conf) { + Collection s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS_GROUPS); + + if (s3AdminsGroup.isEmpty() && conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS).isEmpty()) { + s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS_GROUPS); + } + + return s3AdminsGroup; + } + + /** + * Get the users and groups that are a part of S3 administrators. + * @param conf Stores an instance of {@link OzoneConfiguration} being used + * @return an instance of {@link OzoneAdmins} containing the S3 admin users and groups + */ + public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { + Collection s3Admins; + try { + s3Admins = getS3AdminsFromConfig(conf); + } catch (IOException ie) { + s3Admins = null; + } + Collection s3AdminGroups = getS3AdminsGroupsFromConfig(conf); + + if (LOG.isDebugEnabled()) { + if (null == s3Admins) { + LOG.debug("S3 Admins are not set in configuration"); + } + if (null == s3AdminGroups) { + LOG.debug("S3 Admin Groups are not set in configuration"); + } + } + return new OzoneAdmins(s3Admins, s3AdminGroups); + } + + /** + * Check if the provided user is an S3 administrator. + * @param user An instance of {@link UserGroupInformation} with information about the user to verify + * @param s3Admins An instance of {@link OzoneAdmins} containing information + * of the S3 administrator users and groups in the system + * @return {@code true} if the provided user is an S3 administrator else {@code false} + */ + public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins s3Admins) { + return null != user && s3Admins.isAdmin(user); + } + + /** + * Check if the provided user is an S3 administrator. + * @param user An instance of {@link UserGroupInformation} with information about the user to verify + * @param conf An instance of {@link OzoneConfiguration} being used + * @return {@code true} if the provided user is an S3 administrator else {@code false} + */ + public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneConfiguration conf) { + OzoneAdmins s3Admins = getS3Admins(conf); + return isS3Admin(user, s3Admins); + } +} diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index 948322ae8c8..105e26e4c0c 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -63,6 +63,7 @@ public class GrpcOMFailoverProxyProvider extends LoggerFactory.getLogger(GrpcOMFailoverProxyProvider.class); private final UserGroupInformation ugi; + private final long protocolVer; public GrpcOMFailoverProxyProvider(ConfigurationSource configuration, UserGroupInformation ugi, @@ -70,6 +71,7 @@ public GrpcOMFailoverProxyProvider(ConfigurationSource configuration, Class protocol) throws IOException { super(configuration, omServiceId, protocol); this.ugi = ugi; + this.protocolVer = RPC.getProtocolVersion(protocol); } @Override @@ -123,6 +125,16 @@ protected void loadOMClientConfigs(ConfigurationSource config, String omSvcId) private T createOMProxy() throws IOException { InetSocketAddress addr = new InetSocketAddress(0); + return createOmProxy(addr); + } + + /** + * Get the protocol proxy for provided address + * @param address An instance of {@link InetSocketAddress} which contains the address to connect + * @return the proxy connection to the address and the set of methods supported by the server at the address + * @throws IOException if any error occurs while trying to get the proxy + */ + private T createOmProxy(InetSocketAddress address) throws IOException{ Configuration hadoopConf = LegacyHadoopConfigurationSource.asHadoopConfiguration(getConf()); @@ -130,14 +142,14 @@ private T createOMProxy() throws IOException { RetryPolicy connectionRetryPolicy = RetryPolicies.failoverOnNetworkException(0); return (T) RPC.getProtocolProxy( - getInterface(), - 0, - addr, - ugi, - hadoopConf, - NetUtils.getDefaultSocketFactory(hadoopConf), - (int) OmUtils.getOMClientRpcTimeOut(getConf()), - connectionRetryPolicy + getInterface(), + protocolVer, + address, + ugi, + hadoopConf, + NetUtils.getDefaultSocketFactory(hadoopConf), + (int) OmUtils.getOMClientRpcTimeOut(getConf()), + connectionRetryPolicy ).getProxy(); } diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneConfigUtils.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneConfigUtils.java new file mode 100644 index 00000000000..8b289c0a382 --- /dev/null +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneConfigUtils.java @@ -0,0 +1,51 @@ +package org.apache.hadoop.ozone.conf; + +import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.ozone.OzoneConfigKeys; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.Arrays; + +import static org.assertj.core.api.Assertions.assertThat; + +public class TestOzoneConfigUtils { + + @Test + public void testS3AdminExtraction() throws IOException { + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice,bob"); + + assertThat(OzoneS3ConfigUtils.getS3AdminsFromConfig(configuration)) + .containsAll(Arrays.asList("alice", "bob")); + } + + @Test + public void testS3AdminExtractionWithFallback() throws IOException { + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice,bob"); + + assertThat(OzoneS3ConfigUtils.getS3AdminsFromConfig(configuration)) + .containsAll(Arrays.asList("alice", "bob")); + } + + @Test + public void testS3AdminGroupExtraction() { + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, + "test1, test2"); + + assertThat(OzoneS3ConfigUtils.getS3AdminsGroupsFromConfig(configuration)) + .containsAll(Arrays.asList("test1", "test2")); + } + + @Test + public void testS3AdminGroupExtractionWithFallback() { + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, + "test1, test2"); + + assertThat(OzoneS3ConfigUtils.getS3AdminsGroupsFromConfig(configuration)) + .containsAll(Arrays.asList("test1", "test2")); + } +} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java deleted file mode 100644 index e1fe4f67e5b..00000000000 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/common/OmUserUtils.java +++ /dev/null @@ -1,85 +0,0 @@ -/** - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with this - * work for additional information regarding copyright ownership. The ASF - * licenses this file to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS,WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations under - * the License. - */ - -package org.apache.hadoop.ozone.common; - -import org.apache.hadoop.hdds.conf.OzoneConfiguration; -import org.apache.hadoop.hdds.server.OzoneAdmins; -import org.apache.hadoop.ozone.om.OzoneConfigUtil; -import org.apache.hadoop.security.UserGroupInformation; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import jakarta.annotation.Nullable; - -import java.io.IOException; -import java.util.Collection; - -/** - * Utility class to store User related utilities. - */ -public final class OmUserUtils { - private static final Logger LOG = LoggerFactory.getLogger(OmUserUtils.class); - - private OmUserUtils() { } - - /** - * Get the users and groups that are a part of S3 administrators. - * @param conf Stores an instance of {@link OzoneConfiguration} being used - * @return an instance of {@link OzoneAdmins} containing the S3 admin users and groups - */ - public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { - Collection s3Admins; - try { - s3Admins = OzoneConfigUtil.getS3AdminsFromConfig(conf); - } catch (IOException ie) { - s3Admins = null; - } - Collection s3AdminGroups = OzoneConfigUtil.getS3AdminsGroupsFromConfig(conf); - - if (LOG.isDebugEnabled()) { - if (null == s3Admins) { - LOG.debug("S3 Admins are not set in configuration"); - } - if (null == s3AdminGroups) { - LOG.debug("S3 Admin Groups are not set in configuration"); - } - } - return new OzoneAdmins(s3Admins, s3AdminGroups); - } - - /** - * Check if the provided user is an S3 administrator. - * @param user an instance of {@link UserGroupInformation} with information about the user to verify - * @param s3Admins an instance of {@link OzoneAdmins} containing information of the S3 administrator users and groups in the system - * @return {@code true} if the provided user is an S3 administrator else {@code false} - */ - public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins s3Admins) { - return null != user && s3Admins.isAdmin(user); - } - - /** - * Check if the provided user is an S3 administrator. - * @param user an instance of {@link UserGroupInformation} with information about the user to verify - * @param conf Stores an instance of {@link OzoneConfiguration} being used - * @return {@code true} if the provided user is an S3 administrator else {@code false} - */ - public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneConfiguration conf) { - OzoneAdmins s3Admins = getS3Admins(conf); - return isS3Admin(user, s3Admins); - } -} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java index e5a325cf527..cad987bb7da 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneConfigUtil.java @@ -19,21 +19,11 @@ import org.apache.hadoop.hdds.client.DefaultReplicationConfig; import org.apache.hadoop.hdds.client.ReplicationConfig; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.ozone.om.exceptions.OMException; -import org.apache.hadoop.security.UserGroupInformation; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.io.IOException; -import java.util.Collection; - -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS; - /** * Utility class for ozone configurations. */ @@ -43,38 +33,6 @@ public final class OzoneConfigUtil { private OzoneConfigUtil() { } - /** - * Return list of s3 administrators prop from config. - * - * If ozone.s3.administrators value is empty string or unset, - * defaults to ozone.administrators value. - */ - public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) - throws IOException { - Collection ozAdmins = - conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); - if (ozAdmins == null || ozAdmins.isEmpty()) { - ozAdmins = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS); - } - String omSPN = UserGroupInformation.getCurrentUser().getShortUserName(); - if (!ozAdmins.contains(omSPN)) { - ozAdmins.add(omSPN); - } - return ozAdmins; - } - - public static Collection getS3AdminsGroupsFromConfig( - OzoneConfiguration conf) { - Collection s3AdminsGroup = - conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS_GROUPS); - if (s3AdminsGroup.isEmpty() && conf - .getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS).isEmpty()) { - s3AdminsGroup = conf - .getTrimmedStringCollection(OZONE_ADMINISTRATORS_GROUPS); - } - return s3AdminsGroup; - } - public static ReplicationConfig resolveReplicationConfigPreference( HddsProtos.ReplicationType clientType, HddsProtos.ReplicationFactor clientFactor, diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index 3b8e444ee58..d385f400766 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -94,6 +94,7 @@ import org.apache.hadoop.ozone.OzoneFsServerDefaults; import org.apache.hadoop.ozone.OzoneManagerVersion; import org.apache.hadoop.ozone.audit.OMSystemAction; +import org.apache.hadoop.ozone.conf.OzoneS3ConfigUtils; import org.apache.hadoop.ozone.om.helpers.LeaseKeyInfo; import org.apache.hadoop.ozone.om.helpers.ListOpenFilesResult; import org.apache.hadoop.ozone.om.helpers.SnapshotDiffJob; @@ -151,7 +152,6 @@ import org.apache.hadoop.ozone.audit.AuditMessage; import org.apache.hadoop.ozone.audit.Auditor; import org.apache.hadoop.ozone.audit.OMAction; -import org.apache.hadoop.ozone.common.OmUserUtils; import org.apache.hadoop.ozone.common.Storage.StorageState; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes; @@ -705,7 +705,7 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption) // Get read only admin list readOnlyAdmins = OzoneAdmins.getReadonlyAdmins(conf); - s3OzoneAdmins = OmUserUtils.getS3Admins(conf); + s3OzoneAdmins = OzoneS3ConfigUtils.getS3Admins(conf); instantiateServices(false); // Create special volume s3v which is required for S3G. @@ -4335,7 +4335,7 @@ private void checkAdminUserPrivilege(String operation) throws IOException { } public boolean isS3Admin(UserGroupInformation callerUgi) { - return OmUserUtils.isS3Admin(callerUgi, s3OzoneAdmins); + return OzoneS3ConfigUtils.isS3Admin(callerUgi, s3OzoneAdmins); } @VisibleForTesting diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java index 0bd99d49499..a65042a904d 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java @@ -29,7 +29,6 @@ import java.io.IOException; import java.util.Arrays; -import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -119,42 +118,4 @@ public void testResolveClientSideRepConfigWhenBucketHasEC3() // should return ratis. assertEquals(ratisReplicationConfig, replicationConfig); } - - @Test - public void testS3AdminExtraction() throws IOException { - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice,bob"); - - assertThat(OzoneConfigUtil.getS3AdminsFromConfig(configuration)) - .containsAll(Arrays.asList("alice", "bob")); - } - - @Test - public void testS3AdminExtractionWithFallback() throws IOException { - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice,bob"); - - assertThat(OzoneConfigUtil.getS3AdminsFromConfig(configuration)) - .containsAll(Arrays.asList("alice", "bob")); - } - - @Test - public void testS3AdminGroupExtraction() { - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, - "test1, test2"); - - assertThat(OzoneConfigUtil.getS3AdminsGroupsFromConfig(configuration)) - .containsAll(Arrays.asList("test1", "test2")); - } - - @Test - public void testS3AdminGroupExtractionWithFallback() { - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, - "test1, test2"); - - assertThat(OzoneConfigUtil.getS3AdminsGroupsFromConfig(configuration)) - .containsAll(Arrays.asList("test1", "test2")); - } } diff --git a/hadoop-ozone/s3gateway/pom.xml b/hadoop-ozone/s3gateway/pom.xml index 1cc06f6153a..c26171d98ac 100644 --- a/hadoop-ozone/s3gateway/pom.xml +++ b/hadoop-ozone/s3gateway/pom.xml @@ -231,10 +231,6 @@ org.apache.ozone ozone-client - - org.apache.ozone - ozone-manager - org.apache.ozone hdds-docs diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index 8b5c2020c24..47ee99044a7 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -26,7 +26,7 @@ import javax.ws.rs.core.Response.Status; import javax.ws.rs.ext.Provider; import org.apache.hadoop.hdds.conf.OzoneConfiguration; -import org.apache.hadoop.ozone.common.OmUserUtils; +import org.apache.hadoop.ozone.conf.OzoneS3ConfigUtils; import org.apache.hadoop.security.UserGroupInformation; import java.io.IOException; @@ -52,7 +52,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); if (null != userPrincipal) { UserGroupInformation user = UserGroupInformation.createRemoteUser(userPrincipal.getName()); - if (!OmUserUtils.isS3Admin(user, conf)) { + if (!OzoneS3ConfigUtils.isS3Admin(user, conf)) { requestContext.abortWith(Response.status(Status.FORBIDDEN).build()); } } From 44bad3ba0e96ffcac93a7dff1516670f20cfe5b9 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Fri, 11 Oct 2024 20:29:59 +0530 Subject: [PATCH 11/35] Addressed checkstyle and RAT issues --- .../hadoop/ozone/conf/OzoneS3ConfigUtils.java | 21 +++++++++++++++++ .../om/ha/GrpcOMFailoverProxyProvider.java | 4 ++-- ...Utils.java => TestOzoneS3ConfigUtils.java} | 23 ++++++++++++++++++- .../hadoop/ozone/om/TestOzoneConfigUtil.java | 5 ---- 4 files changed, 45 insertions(+), 8 deletions(-) rename hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/{TestOzoneConfigUtils.java => TestOzoneS3ConfigUtils.java} (65%) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java index 68352cb6c33..952f4d0813b 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + * + */ + package org.apache.hadoop.ozone.conf; import org.apache.hadoop.hdds.conf.OzoneConfiguration; @@ -16,6 +34,9 @@ import java.io.IOException; import java.util.Collection; +/** + * Config based utilities for Ozone S3 + */ public final class OzoneS3ConfigUtils { static final Logger LOG = LoggerFactory.getLogger(OzoneS3ConfigUtils.class); diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index 105e26e4c0c..f771f3b90cf 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -129,12 +129,12 @@ private T createOMProxy() throws IOException { } /** - * Get the protocol proxy for provided address + * Get the protocol proxy for provided address. * @param address An instance of {@link InetSocketAddress} which contains the address to connect * @return the proxy connection to the address and the set of methods supported by the server at the address * @throws IOException if any error occurs while trying to get the proxy */ - private T createOmProxy(InetSocketAddress address) throws IOException{ + private T createOmProxy(InetSocketAddress address) throws IOException { Configuration hadoopConf = LegacyHadoopConfigurationSource.asHadoopConfiguration(getConf()); diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneConfigUtils.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java similarity index 65% rename from hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneConfigUtils.java rename to hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java index 8b289c0a382..5d5541f01f1 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneConfigUtils.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java @@ -1,3 +1,21 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * http://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + * + */ + package org.apache.hadoop.ozone.conf; import org.apache.hadoop.hdds.conf.OzoneConfiguration; @@ -9,7 +27,10 @@ import static org.assertj.core.api.Assertions.assertThat; -public class TestOzoneConfigUtils { +/** + * This class is to test S3 configuration based utils + */ +public class TestOzoneS3ConfigUtils { @Test public void testS3AdminExtraction() throws IOException { diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java index a65042a904d..41d6c28e2b9 100644 --- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOzoneConfigUtil.java @@ -20,15 +20,10 @@ import org.apache.hadoop.hdds.client.ECReplicationConfig; import org.apache.hadoop.hdds.client.RatisReplicationConfig; import org.apache.hadoop.hdds.client.ReplicationConfig; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; -import org.apache.hadoop.ozone.OzoneConfigKeys; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import java.io.IOException; -import java.util.Arrays; - import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; From e0e8748dbbbf289bac96eec4037b37238b2a60b9 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Fri, 11 Oct 2024 20:49:43 +0530 Subject: [PATCH 12/35] Addressed checkstyle issues --- .../java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java | 2 +- .../org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java index 952f4d0813b..716ef85f63f 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java @@ -35,7 +35,7 @@ import java.util.Collection; /** - * Config based utilities for Ozone S3 + * Config based utilities for Ozone S3. */ public final class OzoneS3ConfigUtils { static final Logger LOG = LoggerFactory.getLogger(OzoneS3ConfigUtils.class); diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java index 5d5541f01f1..a367af732e8 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java @@ -28,7 +28,7 @@ import static org.assertj.core.api.Assertions.assertThat; /** - * This class is to test S3 configuration based utils + * This class is to test S3 configuration based utils. */ public class TestOzoneS3ConfigUtils { From 664489a5322d3bb40122b14bd3e5b831fd95b573 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 14 Oct 2024 17:38:31 +0530 Subject: [PATCH 13/35] Fixed test verification and RPC failover config --- .../om/ha/GrpcOMFailoverProxyProvider.java | 4 +++ .../ozone/s3secret/TestSecretRevoke.java | 27 ++++++++++++------- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index f771f3b90cf..feb3a35b504 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -25,6 +25,7 @@ import org.apache.hadoop.hdds.utils.LegacyHadoopConfigurationSource; import org.apache.hadoop.io.retry.RetryPolicies; import org.apache.hadoop.io.retry.RetryPolicy; +import org.apache.hadoop.ipc.ProtobufRpcEngine; import org.apache.hadoop.ipc.RPC; import org.apache.hadoop.net.NetUtils; import org.apache.hadoop.ozone.OmUtils; @@ -138,6 +139,9 @@ private T createOmProxy(InetSocketAddress address) throws IOException { Configuration hadoopConf = LegacyHadoopConfigurationSource.asHadoopConfiguration(getConf()); + // TODO: Post upgrade to Protobuf 3.x we need to use ProtobufRpcEngine2 + RPC.setProtocolEngine(hadoopConf, getInterface(), ProtobufRpcEngine.class); + // Ensure we do not attempt retry on the same OM in case of exceptions RetryPolicy connectionRetryPolicy = RetryPolicies.failoverOnNetworkException(0); diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java index b2077ca7649..97194180ec0 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java @@ -81,8 +81,7 @@ public class TestSecretRevoke { void setUp() { OzoneConfiguration conf = new OzoneConfiguration(); conf.set(OZONE_ADMINISTRATORS, ADMIN_USER_NAME); - objectStore = new ObjectStoreStub(conf, proxy); - OzoneClient client = new OzoneClientStub(objectStore); + OzoneClient client = new OzoneClientStub(new ObjectStoreStub(conf, proxy)); when(uriInfo.getPathParameters()).thenReturn(new MultivaluedHashMap<>()); when(uriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>()); @@ -118,18 +117,28 @@ void testUnauthorizedUserSecretRevoke() throws IOException { @Test void testAuthorizedUserSecretRevoke() throws IOException { mockSecurityContext(true); - endpoint.revoke(); - verify(objectStore, times(1)).revokeS3Secret(eq(USER_NAME)); + Response response = endpoint.revoke(); + + assertEquals(OK.getStatusCode(), response.getStatus()); } @Test - @Unhealthy("HDDS-11041") - void testSecretRevokeWithUsername() throws IOException { - endpoint.revoke(OTHER_USER_NAME); - verify(objectStore, times(1)) - .revokeS3Secret(eq(OTHER_USER_NAME)); + void testUnauthorizedSecretRevokeWithUsername() throws IOException { + mockSecurityContext(false); + Response response = endpoint.revoke(OTHER_USER_NAME); + + assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); } + @Test + void testAuthorizedSecretRevokeWithUsername() throws IOException { + mockSecurityContext(true); + Response response = endpoint.revoke(OTHER_USER_NAME); + + assertEquals(OK.getStatusCode(), response.getStatus()); + } + + @Test void testUnauthorizedUserSecretSequentialRevokes() throws IOException { mockSecurityContext(false); From f8a4e2b6b9efba1db79e61011beda8fcef92c4ae Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 14 Oct 2024 18:16:26 +0530 Subject: [PATCH 14/35] Fixed tests --- .../ozone/s3secret/S3SecretManagementEndpoint.java | 3 +-- .../hadoop/ozone/s3secret/TestSecretGenerate.java | 1 - .../hadoop/ozone/s3secret/TestSecretRevoke.java | 11 +++-------- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java index e8ca32f0dfa..0a0428b456a 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java @@ -95,8 +95,7 @@ public Response revoke() throws IOException { @Path("/{username}") public Response revoke(@PathParam("username") String username) throws IOException { - // TODO: It is a temporary solution. To be removed after HDDS-11041 is done. - return Response.status(METHOD_NOT_ALLOWED).build(); + return revokeInternal(username); } private Response revokeInternal(@Nullable String username) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index 3618b927177..3fd6d6a4eb7 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -135,7 +135,6 @@ void testAuthorizedUserSecretAlreadyExists() throws IOException { } @Test - @Unhealthy("HDDS-11041") void testSecretGenerateWithUsername() throws IOException { hasNoSecretYet(); diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java index 97194180ec0..12953deac37 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java @@ -32,7 +32,6 @@ import org.apache.hadoop.ozone.client.OzoneClientStub; import org.apache.hadoop.ozone.client.protocol.ClientProtocol; import org.apache.hadoop.ozone.om.exceptions.OMException; -import org.apache.ozone.test.tag.Unhealthy; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -47,11 +46,8 @@ import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.ACCESS_DENIED; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.S3_SECRET_NOT_FOUND; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.Mockito.any; +import static org.mockito.Mockito.anyString; import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.eq; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -64,7 +60,6 @@ public class TestSecretRevoke { private static final String OTHER_USER_NAME = "test2"; private S3SecretManagementEndpoint endpoint; - private ObjectStoreStub objectStore; @Mock private ClientProtocol proxy; @@ -155,7 +150,7 @@ void testAuthorizedUserSecretSequentialRevokes() throws IOException { Response firstResponse = endpoint.revoke(); assertEquals(OK.getStatusCode(), firstResponse.getStatus()); doThrow(new OMException(S3_SECRET_NOT_FOUND)) - .when(objectStore).revokeS3Secret(any()); + .when(proxy).revokeS3Secret(anyString()); Response secondResponse = endpoint.revoke(); assertEquals(NOT_FOUND.getStatusCode(), secondResponse.getStatus()); } @@ -164,7 +159,7 @@ void testAuthorizedUserSecretSequentialRevokes() throws IOException { void testSecretRevokesHandlesException() throws IOException { mockSecurityContext(true); doThrow(new OMException(ACCESS_DENIED)) - .when(objectStore).revokeS3Secret(any()); + .when(proxy).revokeS3Secret(anyString()); Response response = endpoint.revoke(); assertEquals(INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); } From 0a3266780fb9954e43362beed9b9980d9802eb17 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 14 Oct 2024 19:02:19 +0530 Subject: [PATCH 15/35] Fixed checkstyles --- .../org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index 3fd6d6a4eb7..d896f65630a 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -33,7 +33,6 @@ import org.apache.hadoop.ozone.client.protocol.ClientProtocol; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.helpers.S3SecretValue; -import org.apache.ozone.test.tag.Unhealthy; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; From db908fb1033eff54e4b11f55c54c905e896982bd Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 14 Oct 2024 19:34:17 +0530 Subject: [PATCH 16/35] Fixed checkstyles --- .../apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java index 0a0428b456a..739dadfb28e 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretManagementEndpoint.java @@ -33,7 +33,6 @@ import java.io.IOException; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; -import static javax.ws.rs.core.Response.Status.METHOD_NOT_ALLOWED; import static javax.ws.rs.core.Response.Status.NOT_FOUND; /** From 2daecc9cf71112d0c0813479e3d49181cf230e9b Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 01:16:32 +0530 Subject: [PATCH 17/35] Added test coverage for S3 admin utils --- .../hadoop/ozone/conf/OzoneS3ConfigUtils.java | 27 ++--- .../ozone/conf/TestOzoneS3ConfigUtils.java | 103 +++++++++++++++++- .../ozone/s3secret/S3SecretAdminFilter.java | 1 + .../ozone/s3secret/TestSecretGenerate.java | 4 +- 4 files changed, 109 insertions(+), 26 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java index 716ef85f63f..107be87d9d7 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java @@ -38,7 +38,7 @@ * Config based utilities for Ozone S3. */ public final class OzoneS3ConfigUtils { - static final Logger LOG = LoggerFactory.getLogger(OzoneS3ConfigUtils.class); + private static final Logger LOG = LoggerFactory.getLogger(OzoneS3ConfigUtils.class); private OzoneS3ConfigUtils() { } @@ -52,17 +52,17 @@ private OzoneS3ConfigUtils() { } * defaults to ozone.administrators value. */ public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { - Collection ozAdmins = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); + Collection ozoneAdmins = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); - if (ozAdmins == null || ozAdmins.isEmpty()) { - ozAdmins = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS); + if (ozoneAdmins == null || ozoneAdmins.isEmpty()) { + ozoneAdmins = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS); } String omSPN = UserGroupInformation.getCurrentUser().getShortUserName(); - if (!ozAdmins.contains(omSPN)) { - ozAdmins.add(omSPN); + if (!ozoneAdmins.contains(omSPN)) { + ozoneAdmins.add(omSPN); } - return ozAdmins; + return ozoneAdmins; } /** @@ -109,17 +109,6 @@ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { return new OzoneAdmins(s3Admins, s3AdminGroups); } - /** - * Check if the provided user is an S3 administrator. - * @param user An instance of {@link UserGroupInformation} with information about the user to verify - * @param s3Admins An instance of {@link OzoneAdmins} containing information - * of the S3 administrator users and groups in the system - * @return {@code true} if the provided user is an S3 administrator else {@code false} - */ - public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins s3Admins) { - return null != user && s3Admins.isAdmin(user); - } - /** * Check if the provided user is an S3 administrator. * @param user An instance of {@link UserGroupInformation} with information about the user to verify @@ -128,6 +117,6 @@ public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins */ public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneConfiguration conf) { OzoneAdmins s3Admins = getS3Admins(conf); - return isS3Admin(user, s3Admins); + return (null != user && s3Admins.isAdmin(user)); } } diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java index a367af732e8..a7785686632 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java @@ -19,21 +19,29 @@ package org.apache.hadoop.ozone.conf; import org.apache.hadoop.hdds.conf.OzoneConfiguration; +import org.apache.hadoop.hdds.server.OzoneAdmins; import org.apache.hadoop.ozone.OzoneConfigKeys; +import org.apache.hadoop.security.UserGroupInformation; import org.junit.jupiter.api.Test; +import org.mockito.Mock; import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; /** * This class is to test S3 configuration based utils. */ -public class TestOzoneS3ConfigUtils { +class TestOzoneS3ConfigUtils { + + @Mock + UserGroupInformation ugi; @Test - public void testS3AdminExtraction() throws IOException { + void testS3AdminExtraction() throws IOException { OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice,bob"); @@ -42,7 +50,7 @@ public void testS3AdminExtraction() throws IOException { } @Test - public void testS3AdminExtractionWithFallback() throws IOException { + void testS3AdminExtractionWithFallback() throws IOException { OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice,bob"); @@ -51,7 +59,7 @@ public void testS3AdminExtractionWithFallback() throws IOException { } @Test - public void testS3AdminGroupExtraction() { + void testS3AdminGroupExtraction() { OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test1, test2"); @@ -61,7 +69,7 @@ public void testS3AdminGroupExtraction() { } @Test - public void testS3AdminGroupExtractionWithFallback() { + void testS3AdminGroupExtractionWithFallback() { OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test1, test2"); @@ -69,4 +77,89 @@ public void testS3AdminGroupExtractionWithFallback() { assertThat(OzoneS3ConfigUtils.getS3AdminsGroupsFromConfig(configuration)) .containsAll(Arrays.asList("test1", "test2")); } + + @Test + void testGetS3AdminsWhenS3AdminPresent() { + // When there is S3 admin present + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice"); + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test_group"); + + OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(configuration); + UserGroupInformation ugi = UserGroupInformation.createUserForTesting( + "alice", new String[] {"test_group"}); + + assertThat(admins.isAdmin(ugi)).isEqualTo(true); + + // Test that when a user is present in an admin group but not an Ozone Admin + UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( + "bob", new String[] {"test_group"}); + assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(true); + } + @Test + void testGetS3AdminsWhenNoS3AdminPresent() { + // When there is no S3 admin, but Ozone admins present + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); + + OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(configuration); + UserGroupInformation ugi = UserGroupInformation.createUserForTesting( + "alice", new String[] {"test_group"}); + + assertThat(admins.isAdmin(ugi)).isEqualTo(true); + + // Test that when a user is present in an admin group but not an Ozone Admin + UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( + "bob", new String[] {"test_group"}); + assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(true); + } + + @Test + void testGetS3AdminsWithNoAdmins() { + OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(new OzoneConfiguration()); + UserGroupInformation ugi = UserGroupInformation.createUserForTesting( + "alice", new String[] {"test_group"}); + assertThat(admins.isAdmin(ugi)).isEqualTo(false); + } + + @Test + void testIsS3AdminForS3AdminUser() { + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice"); + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test_group"); + + UserGroupInformation ugi = UserGroupInformation.createUserForTesting( + "alice", new String[] {"test_group"}); + // Scenario when user is present in an admin group but not an Ozone Admin + UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( + "bob", new String[] {"test_group"}); + + assertThat(OzoneS3ConfigUtils.isS3Admin(ugi, configuration)).isEqualTo(true); + assertThat(OzoneS3ConfigUtils.isS3Admin(ugiGroupOnly, configuration)).isEqualTo(true); + } + + @Test + void testIsS3AdminForAdminUser() { + // When there is no S3 admin, but Ozone admins present + OzoneConfiguration configuration = new OzoneConfiguration(); + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); + + OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(configuration); + UserGroupInformation ugi = UserGroupInformation.createUserForTesting( + "alice", new String[] {"test_group"}); + // Test that when a user is present in an admin group but not an Ozone Admin + UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( + "bob", new String[] {"test_group"}); + + assertThat(admins.isAdmin(ugi)).isEqualTo(true); + assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(true); + } + + @Test + void testIsS3AdminForNoUser() { + OzoneConfiguration configuration = new OzoneConfiguration(); + assertThat(OzoneS3ConfigUtils.isS3Admin(null, configuration)).isEqualTo(false); + } } diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index 47ee99044a7..2b3d93129e8 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -50,6 +50,7 @@ public class S3SecretAdminFilter implements ContainerRequestFilter { @Override public void filter(ContainerRequestContext requestContext) throws IOException { final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); + System.out.println("Requested User Principal is: " + userPrincipal.getName()); if (null != userPrincipal) { UserGroupInformation user = UserGroupInformation.createRemoteUser(userPrincipal.getName()); if (!OzoneS3ConfigUtils.isS3Admin(user, conf)) { diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index d896f65630a..41eed38d584 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -118,7 +118,7 @@ void testUnauthorizedUserSecretAlreadyExists() throws IOException { Response response = endpoint.generate(); - assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); + assertEquals(BAD_REQUEST.getStatusCode(), response.getStatus()); } @Test @@ -126,7 +126,7 @@ void testAuthorizedUserSecretAlreadyExists() throws IOException { setupSecurityContext(true); hasSecretAlready(); - Response response = endpoint.generate(); + Response response = endpoint.generate(ADMIN_USER_NAME); assertEquals(BAD_REQUEST.getStatusCode(), response.getStatus()); assertEquals(OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS.toString(), From e67269efe5ee232e7c7f94c0bc5d7e2d125f338e Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 01:19:37 +0530 Subject: [PATCH 18/35] Fixed checkstyle issue --- .../org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java index a7785686632..d6ce6a595d6 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java @@ -27,19 +27,14 @@ import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.when; /** * This class is to test S3 configuration based utils. */ class TestOzoneS3ConfigUtils { - @Mock - UserGroupInformation ugi; - @Test void testS3AdminExtraction() throws IOException { OzoneConfiguration configuration = new OzoneConfiguration(); From d2653a5ba6058e7a3d95f6d14db26b9811799b7c Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 02:04:30 +0530 Subject: [PATCH 19/35] Fixed build issues --- .../hadoop/ozone/conf/OzoneS3ConfigUtils.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java index 107be87d9d7..207224a71c6 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java @@ -109,6 +109,17 @@ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { return new OzoneAdmins(s3Admins, s3AdminGroups); } + /** + * Check if the provided user is an S3 administrator. + * @param user An instance of {@link UserGroupInformation} with information about the user to verify + * @param s3Admins An instance of {@link OzoneAdmins} containing information + * of the S3 administrator users and groups in the system + * @return {@code true} if the provided user is an S3 administrator else {@code false} + */ + public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins s3Admins) { + return null != user && s3Admins.isAdmin(user); + } + /** * Check if the provided user is an S3 administrator. * @param user An instance of {@link UserGroupInformation} with information about the user to verify @@ -117,6 +128,6 @@ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { */ public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneConfiguration conf) { OzoneAdmins s3Admins = getS3Admins(conf); - return (null != user && s3Admins.isAdmin(user)); + return isS3Admin(user, s3Admins); } } From 9c9a9daeb06801482840cc3665477759ec4c3c13 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 12:27:01 +0530 Subject: [PATCH 20/35] Fixed checkstyle issues --- .../org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java | 1 - .../org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java index d6ce6a595d6..5b025c9b446 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java +++ b/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java @@ -23,7 +23,6 @@ import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.security.UserGroupInformation; import org.junit.jupiter.api.Test; -import org.mockito.Mock; import java.io.IOException; import java.util.Arrays; diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index 2b3d93129e8..7c2f23fb04c 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -50,8 +50,8 @@ public class S3SecretAdminFilter implements ContainerRequestFilter { @Override public void filter(ContainerRequestContext requestContext) throws IOException { final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); - System.out.println("Requested User Principal is: " + userPrincipal.getName()); if (null != userPrincipal) { + System.out.println("Requested User Principal is: " + userPrincipal.getName()); UserGroupInformation user = UserGroupInformation.createRemoteUser(userPrincipal.getName()); if (!OzoneS3ConfigUtils.isS3Admin(user, conf)) { requestContext.abortWith(Response.status(Status.FORBIDDEN).build()); From fb6dcb972e4c797001669495e24dd6849f4aa01e Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 13:19:25 +0530 Subject: [PATCH 21/35] Fix tests --- .../ozone/s3secret/TestSecretGenerate.java | 49 ++--------- .../ozone/s3secret/TestSecretRevoke.java | 85 +++++-------------- 2 files changed, 31 insertions(+), 103 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index 41eed38d584..3f05db8581e 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -40,8 +40,6 @@ import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.jupiter.MockitoExtension; -import static javax.ws.rs.core.Response.Status.FORBIDDEN; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.Mockito.notNull; @@ -52,7 +50,6 @@ */ @ExtendWith(MockitoExtension.class) class TestSecretGenerate { - private static final String ADMIN_USER_NAME = "test_admin"; private static final String USER_NAME = "test"; private static final String OTHER_USER_NAME = "test2"; private static final String USER_SECRET = "test_secret"; @@ -78,7 +75,6 @@ private static S3SecretValue getS3SecretValue(InvocationOnMock invocation) { @BeforeEach void setUp() { OzoneConfiguration conf = new OzoneConfiguration(); - conf.set(OZONE_ADMINISTRATORS, ADMIN_USER_NAME); OzoneClient client = new OzoneClientStub(new ObjectStoreStub(conf, proxy)); when(uriInfo.getPathParameters()).thenReturn(new MultivaluedHashMap<>()); @@ -91,43 +87,24 @@ void setUp() { } @Test - void testUnauthorizedSecretGeneration() throws IOException { - setupSecurityContext(false); - hasNoSecretYet(); - - Response response = endpoint.generate(); - assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); - } - - @Test - void testAuthorizedSecretGeneration() throws IOException { - setupSecurityContext(true); + void testSecretGenerate() throws IOException { + setupSecurityContext(); hasNoSecretYet(); S3SecretResponse response = (S3SecretResponse) endpoint.generate().getEntity(); assertEquals(USER_SECRET, response.getAwsSecret()); - assertEquals(ADMIN_USER_NAME, response.getAwsAccessKey()); + assertEquals(USER_NAME, response.getAwsAccessKey()); } @Test - void testUnauthorizedUserSecretAlreadyExists() throws IOException { - setupSecurityContext(false); + void testIfSecretAlreadyExists() throws IOException { + setupSecurityContext(); hasSecretAlready(); Response response = endpoint.generate(); - assertEquals(BAD_REQUEST.getStatusCode(), response.getStatus()); - } - - @Test - void testAuthorizedUserSecretAlreadyExists() throws IOException { - setupSecurityContext(true); - hasSecretAlready(); - - Response response = endpoint.generate(ADMIN_USER_NAME); - assertEquals(BAD_REQUEST.getStatusCode(), response.getStatus()); assertEquals(OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS.toString(), response.getStatusInfo().getReasonPhrase()); @@ -138,21 +115,13 @@ void testSecretGenerateWithUsername() throws IOException { hasNoSecretYet(); S3SecretResponse response = - (S3SecretResponse) endpoint.generate(OTHER_USER_NAME).getEntity(); + (S3SecretResponse) endpoint.generate(OTHER_USER_NAME).getEntity(); assertEquals(USER_SECRET, response.getAwsSecret()); assertEquals(OTHER_USER_NAME, response.getAwsAccessKey()); } - /** - * Provides mocking for users and security context. - * @param isAdmin Stores whether the user to mock is an admin or not - */ - private void setupSecurityContext(boolean isAdmin) { - if (isAdmin) { - when(principal.getName()).thenReturn(ADMIN_USER_NAME); - } else { - when(principal.getName()).thenReturn(USER_NAME); - } + private void setupSecurityContext() { + when(principal.getName()).thenReturn(USER_NAME); when(securityContext.getUserPrincipal()).thenReturn(principal); when(context.getSecurityContext()).thenReturn(securityContext); } @@ -166,4 +135,4 @@ private void hasSecretAlready() throws IOException { when(proxy.getS3Secret(notNull())) .thenThrow(new OMException("Secret already exists", OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS)); } -} +} \ No newline at end of file diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java index 12953deac37..f4025dc2994 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java @@ -26,11 +26,9 @@ import javax.ws.rs.core.SecurityContext; import javax.ws.rs.core.UriInfo; -import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ozone.client.ObjectStoreStub; import org.apache.hadoop.ozone.client.OzoneClient; import org.apache.hadoop.ozone.client.OzoneClientStub; -import org.apache.hadoop.ozone.client.protocol.ClientProtocol; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -38,16 +36,17 @@ import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import static javax.ws.rs.core.Response.Status.FORBIDDEN; import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR; import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static javax.ws.rs.core.Response.Status.OK; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.ACCESS_DENIED; import static org.apache.hadoop.ozone.om.exceptions.OMException.ResultCodes.S3_SECRET_NOT_FOUND; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.any; import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; /** @@ -55,14 +54,13 @@ */ @ExtendWith(MockitoExtension.class) public class TestSecretRevoke { - private static final String ADMIN_USER_NAME = "test_admin"; private static final String USER_NAME = "test"; private static final String OTHER_USER_NAME = "test2"; private S3SecretManagementEndpoint endpoint; @Mock - private ClientProtocol proxy; + private ObjectStoreStub objectStore; @Mock private ContainerRequestContext context; @Mock @@ -74,9 +72,7 @@ public class TestSecretRevoke { @BeforeEach void setUp() { - OzoneConfiguration conf = new OzoneConfiguration(); - conf.set(OZONE_ADMINISTRATORS, ADMIN_USER_NAME); - OzoneClient client = new OzoneClientStub(new ObjectStoreStub(conf, proxy)); + OzoneClient client = new OzoneClientStub(objectStore); when(uriInfo.getPathParameters()).thenReturn(new MultivaluedHashMap<>()); when(uriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>()); @@ -87,80 +83,43 @@ void setUp() { endpoint.setContext(context); } - /** - * Provides mocking for users and security context. - * @param isAdmin Stores whether the user is admin or not - */ - private void mockSecurityContext(boolean isAdmin) { - if (isAdmin) { - when(principal.getName()).thenReturn(ADMIN_USER_NAME); - } else { - when(principal.getName()).thenReturn(USER_NAME); - } + private void mockSecurityContext() { + when(principal.getName()).thenReturn(USER_NAME); when(securityContext.getUserPrincipal()).thenReturn(principal); when(context.getSecurityContext()).thenReturn(securityContext); } @Test - void testUnauthorizedUserSecretRevoke() throws IOException { - mockSecurityContext(false); - Response response = endpoint.revoke(); - - assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); - } - - @Test - void testAuthorizedUserSecretRevoke() throws IOException { - mockSecurityContext(true); - Response response = endpoint.revoke(); - - assertEquals(OK.getStatusCode(), response.getStatus()); + void testSecretRevoke() throws IOException { + mockSecurityContext(); + endpoint.revoke(); + verify(objectStore, times(1)).revokeS3Secret(eq(USER_NAME)); } @Test - void testUnauthorizedSecretRevokeWithUsername() throws IOException { - mockSecurityContext(false); - Response response = endpoint.revoke(OTHER_USER_NAME); - - assertEquals(FORBIDDEN.getStatusCode(), response.getStatus()); - } - - @Test - void testAuthorizedSecretRevokeWithUsername() throws IOException { - mockSecurityContext(true); - Response response = endpoint.revoke(OTHER_USER_NAME); - - assertEquals(OK.getStatusCode(), response.getStatus()); - } - - - @Test - void testUnauthorizedUserSecretSequentialRevokes() throws IOException { - mockSecurityContext(false); - Response firstResponse = endpoint.revoke(); - assertEquals(FORBIDDEN.getStatusCode(), firstResponse.getStatus()); - - Response secondResponse = endpoint.revoke(); - assertEquals(FORBIDDEN.getStatusCode(), secondResponse.getStatus()); + void testSecretRevokeWithUsername() throws IOException { + endpoint.revoke(OTHER_USER_NAME); + verify(objectStore, times(1)) + .revokeS3Secret(eq(OTHER_USER_NAME)); } @Test - void testAuthorizedUserSecretSequentialRevokes() throws IOException { - mockSecurityContext(true); + void testSecretSequentialRevokes() throws IOException { + mockSecurityContext(); Response firstResponse = endpoint.revoke(); assertEquals(OK.getStatusCode(), firstResponse.getStatus()); doThrow(new OMException(S3_SECRET_NOT_FOUND)) - .when(proxy).revokeS3Secret(anyString()); + .when(objectStore).revokeS3Secret(any()); Response secondResponse = endpoint.revoke(); assertEquals(NOT_FOUND.getStatusCode(), secondResponse.getStatus()); } @Test void testSecretRevokesHandlesException() throws IOException { - mockSecurityContext(true); + mockSecurityContext(); doThrow(new OMException(ACCESS_DENIED)) - .when(proxy).revokeS3Secret(anyString()); + .when(objectStore).revokeS3Secret(any()); Response response = endpoint.revoke(); assertEquals(INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); } -} +} \ No newline at end of file From 30e55e27be4991d1fc25df6af1ea153625eb0e06 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 14:04:28 +0530 Subject: [PATCH 22/35] Add newline at file end for checkstyles --- .../org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java | 2 +- .../java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java index 3f05db8581e..b548d17d9ff 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java @@ -135,4 +135,4 @@ private void hasSecretAlready() throws IOException { when(proxy.getS3Secret(notNull())) .thenThrow(new OMException("Secret already exists", OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS)); } -} \ No newline at end of file +} diff --git a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java index f4025dc2994..b26df0e8996 100644 --- a/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java +++ b/hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretRevoke.java @@ -122,4 +122,4 @@ void testSecretRevokesHandlesException() throws IOException { Response response = endpoint.revoke(); assertEquals(INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); } -} \ No newline at end of file +} From f4cfed4330fbb8faecf29bc1b07b72235522493a Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 16:41:19 +0530 Subject: [PATCH 23/35] Remove print line --- .../org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index 7c2f23fb04c..47ee99044a7 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -51,7 +51,6 @@ public class S3SecretAdminFilter implements ContainerRequestFilter { public void filter(ContainerRequestContext requestContext) throws IOException { final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); if (null != userPrincipal) { - System.out.println("Requested User Principal is: " + userPrincipal.getName()); UserGroupInformation user = UserGroupInformation.createRemoteUser(userPrincipal.getName()); if (!OzoneS3ConfigUtils.isS3Admin(user, conf)) { requestContext.abortWith(Response.status(Status.FORBIDDEN).build()); From ad12dfe1c9ec4c7bc430e4fc73ddbbc72ef69e7d Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 20:51:44 +0530 Subject: [PATCH 24/35] Re-enable robot tests and add robot tests for non-admin user --- .../dist/src/main/smoketest/s3/secretgenerate.robot | 8 ++++++-- .../dist/src/main/smoketest/s3/secretrevoke.robot | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot b/hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot index e9b5dd5df72..e0c2fc7f818 100644 --- a/hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot +++ b/hadoop-ozone/dist/src/main/smoketest/s3/secretgenerate.robot @@ -45,15 +45,19 @@ S3 Gateway Secret Already Exists Should contain ${result} HTTP/1.1 400 S3_SECRET_ALREADY_EXISTS ignore_case=True S3 Gateway Generate Secret By Username - [Tags] robot:skip # TODO: Enable after HDDS-11041 is done. Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled ${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser Should contain ${result} HTTP/1.1 200 OK ignore_case=True Should Match Regexp ${result} .*.* S3 Gateway Generate Secret By Username For Other User - [Tags] robot:skip # TODO: Enable after HDDS-11041 is done. Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled ${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser2 Should contain ${result} HTTP/1.1 200 OK ignore_case=True Should Match Regexp ${result} .*.* + +S3 Gateway Reject Secret Generation By Non-admin User + Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled + Run Keyword Kinit test user testuser2 testuser2.keytab + ${result} = Execute curl -X PUT --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser + Should contain ${result} HTTP/1.1 403 FORBIDDEN ignore_case=True \ No newline at end of file diff --git a/hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot b/hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot index 59725c0416c..ffb03a85a8a 100644 --- a/hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot +++ b/hadoop-ozone/dist/src/main/smoketest/s3/secretrevoke.robot @@ -38,15 +38,19 @@ S3 Gateway Revoke Secret Should contain ${result} HTTP/1.1 200 OK ignore_case=True S3 Gateway Revoke Secret By Username - [Tags] robot:skip # TODO: Enable after HDDS-11041 is done. Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled Execute ozone s3 getsecret -u testuser ${OM_HA_PARAM} ${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser Should contain ${result} HTTP/1.1 200 OK ignore_case=True S3 Gateway Revoke Secret By Username For Other User - [Tags] robot:skip # TODO: Enable after HDDS-11041 is done. Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled Execute ozone s3 getsecret -u testuser2 ${OM_HA_PARAM} ${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser2 Should contain ${result} HTTP/1.1 200 OK ignore_case=True + +S3 Gateway Reject Secret Revoke By Non-admin User + Pass Execution If '${SECURITY_ENABLED}' == 'false' Skipping this check as security is not enabled + Run Keyword Kinit test user testuser2 testuser2.keytab + ${result} = Execute curl -X DELETE --negotiate -u : -v ${ENDPOINT_URL}/secret/testuser + Should contain ${result} HTTP/1.1 403 FORBIDDEN ignore_case=True \ No newline at end of file From bf6fb0239af226cebc857632e31b968b635d64b3 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 21:08:51 +0530 Subject: [PATCH 25/35] Refactor S3 config utils to hdds-framework --- .../hadoop/hdds/server/OzoneAdmins.java | 89 +++++++++++- .../hdds/server/TestOzoneAdminUtils.java | 31 ++-- hadoop-ozone/common/pom.xml | 4 - .../hadoop/ozone/conf/OzoneS3ConfigUtils.java | 133 ------------------ .../apache/hadoop/ozone/om/OzoneManager.java | 5 +- .../ozone/s3secret/S3SecretAdminFilter.java | 4 +- 6 files changed, 103 insertions(+), 163 deletions(-) rename hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java => hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java (84%) delete mode 100644 hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java index 12b6b64f49a..1990a961943 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hdds.server; +import java.io.IOException; import java.util.Collection; import java.util.Collections; import java.util.LinkedHashSet; @@ -24,15 +25,12 @@ import com.google.common.collect.Sets; +import jakarta.annotation.Nullable; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.security.AccessControlException; import org.apache.hadoop.security.UserGroupInformation; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_WILDCARD; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_READONLY_ADMINISTRATORS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_READONLY_ADMINISTRATORS_GROUPS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.*; /** * This class contains ozone admin user information, username and group, @@ -186,4 +184,85 @@ public static Collection getOzoneReadOnlyAdminsGroupsFromConfig( return conf.getTrimmedStringCollection( OZONE_READONLY_ADMINISTRATORS_GROUPS); } + + /** + * Get the list of S3 administrators from Ozone config. + * + * @param conf An instance of {@link OzoneConfiguration} being used + * @return A {@link Collection} of the S3 administrator users + * + * If ozone.s3.administrators value is empty string or unset, + * defaults to ozone.administrators value. + */ + public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { + Collection ozoneAdmins = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); + + if (ozoneAdmins == null || ozoneAdmins.isEmpty()) { + ozoneAdmins = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS); + } + String omSPN = UserGroupInformation.getCurrentUser().getShortUserName(); + if (!ozoneAdmins.contains(omSPN)) { + ozoneAdmins.add(omSPN); + } + + return ozoneAdmins; + } + + /** + * Get the list of the groups that are a part S3 administrators from Ozone config. + * + * @param conf An instance of {@link OzoneConfiguration} being used + * @return A {@link Collection} of the S3 administrator groups + * + * If ozone.s3.administrators.groups value is empty or unset, + * defaults to the ozone.administrators.groups value + */ + public static Collection getS3AdminsGroupsFromConfig(OzoneConfiguration conf) { + Collection s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS_GROUPS); + + if (s3AdminsGroup.isEmpty() && conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS).isEmpty()) { + s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS_GROUPS); + } + + return s3AdminsGroup; + } + + /** + * Get the users and groups that are a part of S3 administrators. + * @param conf Stores an instance of {@link OzoneConfiguration} being used + * @return an instance of {@link OzoneAdmins} containing the S3 admin users and groups + */ + public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { + Collection s3Admins; + try { + s3Admins = getS3AdminsFromConfig(conf); + } catch (IOException ie) { + s3Admins = null; + } + Collection s3AdminGroups = getS3AdminsGroupsFromConfig(conf); + + return new OzoneAdmins(s3Admins, s3AdminGroups); + } + + /** + * Check if the provided user is an S3 administrator. + * @param user An instance of {@link UserGroupInformation} with information about the user to verify + * @param s3Admins An instance of {@link OzoneAdmins} containing information + * of the S3 administrator users and groups in the system + * @return {@code true} if the provided user is an S3 administrator else {@code false} + */ + public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins s3Admins) { + return null != user && s3Admins.isAdmin(user); + } + + /** + * Check if the provided user is an S3 administrator. + * @param user An instance of {@link UserGroupInformation} with information about the user to verify + * @param conf An instance of {@link OzoneConfiguration} being used + * @return {@code true} if the provided user is an S3 administrator else {@code false} + */ + public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneConfiguration conf) { + OzoneAdmins s3Admins = getS3Admins(conf); + return isS3Admin(user, s3Admins); + } } diff --git a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java similarity index 84% rename from hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java rename to hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java index 5b025c9b446..a6f7a78fa6c 100644 --- a/hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/conf/TestOzoneS3ConfigUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java @@ -16,10 +16,9 @@ * */ -package org.apache.hadoop.ozone.conf; +package org.apache.hadoop.hdds.server; import org.apache.hadoop.hdds.conf.OzoneConfiguration; -import org.apache.hadoop.hdds.server.OzoneAdmins; import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.security.UserGroupInformation; import org.junit.jupiter.api.Test; @@ -30,16 +29,16 @@ import static org.assertj.core.api.Assertions.assertThat; /** - * This class is to test S3 configuration based utils. + * This class is to test the utilities present in the OzoneAdmins class. */ -class TestOzoneS3ConfigUtils { - +class TestOzoneAdminUtils { + // The following set of tests are to validate the S3 based utilities present in OzoneAdmins @Test void testS3AdminExtraction() throws IOException { OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice,bob"); - assertThat(OzoneS3ConfigUtils.getS3AdminsFromConfig(configuration)) + assertThat(OzoneAdmins.getS3AdminsFromConfig(configuration)) .containsAll(Arrays.asList("alice", "bob")); } @@ -48,7 +47,7 @@ void testS3AdminExtractionWithFallback() throws IOException { OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice,bob"); - assertThat(OzoneS3ConfigUtils.getS3AdminsFromConfig(configuration)) + assertThat(OzoneAdmins.getS3AdminsFromConfig(configuration)) .containsAll(Arrays.asList("alice", "bob")); } @@ -58,7 +57,7 @@ void testS3AdminGroupExtraction() { configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test1, test2"); - assertThat(OzoneS3ConfigUtils.getS3AdminsGroupsFromConfig(configuration)) + assertThat(OzoneAdmins.getS3AdminsGroupsFromConfig(configuration)) .containsAll(Arrays.asList("test1", "test2")); } @@ -68,7 +67,7 @@ void testS3AdminGroupExtractionWithFallback() { configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test1, test2"); - assertThat(OzoneS3ConfigUtils.getS3AdminsGroupsFromConfig(configuration)) + assertThat(OzoneAdmins.getS3AdminsGroupsFromConfig(configuration)) .containsAll(Arrays.asList("test1", "test2")); } @@ -79,7 +78,7 @@ void testGetS3AdminsWhenS3AdminPresent() { configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice"); configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test_group"); - OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(configuration); + OzoneAdmins admins = OzoneAdmins.getS3Admins(configuration); UserGroupInformation ugi = UserGroupInformation.createUserForTesting( "alice", new String[] {"test_group"}); @@ -97,7 +96,7 @@ void testGetS3AdminsWhenNoS3AdminPresent() { configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); - OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(configuration); + OzoneAdmins admins = OzoneAdmins.getS3Admins(configuration); UserGroupInformation ugi = UserGroupInformation.createUserForTesting( "alice", new String[] {"test_group"}); @@ -111,7 +110,7 @@ void testGetS3AdminsWhenNoS3AdminPresent() { @Test void testGetS3AdminsWithNoAdmins() { - OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(new OzoneConfiguration()); + OzoneAdmins admins = OzoneAdmins.getS3Admins(new OzoneConfiguration()); UserGroupInformation ugi = UserGroupInformation.createUserForTesting( "alice", new String[] {"test_group"}); assertThat(admins.isAdmin(ugi)).isEqualTo(false); @@ -129,8 +128,8 @@ void testIsS3AdminForS3AdminUser() { UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( "bob", new String[] {"test_group"}); - assertThat(OzoneS3ConfigUtils.isS3Admin(ugi, configuration)).isEqualTo(true); - assertThat(OzoneS3ConfigUtils.isS3Admin(ugiGroupOnly, configuration)).isEqualTo(true); + assertThat(OzoneAdmins.isS3Admin(ugi, configuration)).isEqualTo(true); + assertThat(OzoneAdmins.isS3Admin(ugiGroupOnly, configuration)).isEqualTo(true); } @Test @@ -140,7 +139,7 @@ void testIsS3AdminForAdminUser() { configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); - OzoneAdmins admins = OzoneS3ConfigUtils.getS3Admins(configuration); + OzoneAdmins admins = OzoneAdmins.getS3Admins(configuration); UserGroupInformation ugi = UserGroupInformation.createUserForTesting( "alice", new String[] {"test_group"}); // Test that when a user is present in an admin group but not an Ozone Admin @@ -154,6 +153,6 @@ void testIsS3AdminForAdminUser() { @Test void testIsS3AdminForNoUser() { OzoneConfiguration configuration = new OzoneConfiguration(); - assertThat(OzoneS3ConfigUtils.isS3Admin(null, configuration)).isEqualTo(false); + assertThat(OzoneAdmins.isS3Admin(null, configuration)).isEqualTo(false); } } diff --git a/hadoop-ozone/common/pom.xml b/hadoop-ozone/common/pom.xml index fe81ea14864..bd16a0a5dfe 100644 --- a/hadoop-ozone/common/pom.xml +++ b/hadoop-ozone/common/pom.xml @@ -65,10 +65,6 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd"> org.apache.ozone hdds-config - - org.apache.ozone - hdds-server-framework - org.apache.ozone hdds-interface-client diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java deleted file mode 100644 index 207224a71c6..00000000000 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/conf/OzoneS3ConfigUtils.java +++ /dev/null @@ -1,133 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with this - * work for additional information regarding copyright ownership. The ASF - * licenses this file to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - *

- * http://www.apache.org/licenses/LICENSE-2.0 - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations under - * the License. - * - */ - -package org.apache.hadoop.ozone.conf; - -import org.apache.hadoop.hdds.conf.OzoneConfiguration; -import org.apache.hadoop.hdds.server.OzoneAdmins; -import org.apache.hadoop.security.UserGroupInformation; - -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS; -import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import jakarta.annotation.Nullable; - -import java.io.IOException; -import java.util.Collection; - -/** - * Config based utilities for Ozone S3. - */ -public final class OzoneS3ConfigUtils { - private static final Logger LOG = LoggerFactory.getLogger(OzoneS3ConfigUtils.class); - - private OzoneS3ConfigUtils() { } - - /** - * Get the list of S3 administrators from Ozone config. - * - * @param conf An instance of {@link OzoneConfiguration} being used - * @return A {@link Collection} of the S3 administrator users - * - * If ozone.s3.administrators value is empty string or unset, - * defaults to ozone.administrators value. - */ - public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { - Collection ozoneAdmins = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); - - if (ozoneAdmins == null || ozoneAdmins.isEmpty()) { - ozoneAdmins = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS); - } - String omSPN = UserGroupInformation.getCurrentUser().getShortUserName(); - if (!ozoneAdmins.contains(omSPN)) { - ozoneAdmins.add(omSPN); - } - - return ozoneAdmins; - } - - /** - * Get the list of the groups that are a part S3 administrators from Ozone config. - * - * @param conf An instance of {@link OzoneConfiguration} being used - * @return A {@link Collection} of the S3 administrator groups - * - * If ozone.s3.administrators.groups value is empty or unset, - * defaults to the ozone.administrators.groups value - */ - public static Collection getS3AdminsGroupsFromConfig(OzoneConfiguration conf) { - Collection s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS_GROUPS); - - if (s3AdminsGroup.isEmpty() && conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS).isEmpty()) { - s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS_GROUPS); - } - - return s3AdminsGroup; - } - - /** - * Get the users and groups that are a part of S3 administrators. - * @param conf Stores an instance of {@link OzoneConfiguration} being used - * @return an instance of {@link OzoneAdmins} containing the S3 admin users and groups - */ - public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { - Collection s3Admins; - try { - s3Admins = getS3AdminsFromConfig(conf); - } catch (IOException ie) { - s3Admins = null; - } - Collection s3AdminGroups = getS3AdminsGroupsFromConfig(conf); - - if (LOG.isDebugEnabled()) { - if (null == s3Admins) { - LOG.debug("S3 Admins are not set in configuration"); - } - if (null == s3AdminGroups) { - LOG.debug("S3 Admin Groups are not set in configuration"); - } - } - return new OzoneAdmins(s3Admins, s3AdminGroups); - } - - /** - * Check if the provided user is an S3 administrator. - * @param user An instance of {@link UserGroupInformation} with information about the user to verify - * @param s3Admins An instance of {@link OzoneAdmins} containing information - * of the S3 administrator users and groups in the system - * @return {@code true} if the provided user is an S3 administrator else {@code false} - */ - public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneAdmins s3Admins) { - return null != user && s3Admins.isAdmin(user); - } - - /** - * Check if the provided user is an S3 administrator. - * @param user An instance of {@link UserGroupInformation} with information about the user to verify - * @param conf An instance of {@link OzoneConfiguration} being used - * @return {@code true} if the provided user is an S3 administrator else {@code false} - */ - public static boolean isS3Admin(@Nullable UserGroupInformation user, OzoneConfiguration conf) { - OzoneAdmins s3Admins = getS3Admins(conf); - return isS3Admin(user, s3Admins); - } -} diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java index d385f400766..a90764714da 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java @@ -94,7 +94,6 @@ import org.apache.hadoop.ozone.OzoneFsServerDefaults; import org.apache.hadoop.ozone.OzoneManagerVersion; import org.apache.hadoop.ozone.audit.OMSystemAction; -import org.apache.hadoop.ozone.conf.OzoneS3ConfigUtils; import org.apache.hadoop.ozone.om.helpers.LeaseKeyInfo; import org.apache.hadoop.ozone.om.helpers.ListOpenFilesResult; import org.apache.hadoop.ozone.om.helpers.SnapshotDiffJob; @@ -705,7 +704,7 @@ private OzoneManager(OzoneConfiguration conf, StartupOption startupOption) // Get read only admin list readOnlyAdmins = OzoneAdmins.getReadonlyAdmins(conf); - s3OzoneAdmins = OzoneS3ConfigUtils.getS3Admins(conf); + s3OzoneAdmins = OzoneAdmins.getS3Admins(conf); instantiateServices(false); // Create special volume s3v which is required for S3G. @@ -4335,7 +4334,7 @@ private void checkAdminUserPrivilege(String operation) throws IOException { } public boolean isS3Admin(UserGroupInformation callerUgi) { - return OzoneS3ConfigUtils.isS3Admin(callerUgi, s3OzoneAdmins); + return OzoneAdmins.isS3Admin(callerUgi, s3OzoneAdmins); } @VisibleForTesting diff --git a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java index 47ee99044a7..5ecdfa7c121 100644 --- a/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java +++ b/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3secret/S3SecretAdminFilter.java @@ -26,7 +26,7 @@ import javax.ws.rs.core.Response.Status; import javax.ws.rs.ext.Provider; import org.apache.hadoop.hdds.conf.OzoneConfiguration; -import org.apache.hadoop.ozone.conf.OzoneS3ConfigUtils; +import org.apache.hadoop.hdds.server.OzoneAdmins; import org.apache.hadoop.security.UserGroupInformation; import java.io.IOException; @@ -52,7 +52,7 @@ public void filter(ContainerRequestContext requestContext) throws IOException { final Principal userPrincipal = requestContext.getSecurityContext().getUserPrincipal(); if (null != userPrincipal) { UserGroupInformation user = UserGroupInformation.createRemoteUser(userPrincipal.getName()); - if (!OzoneS3ConfigUtils.isS3Admin(user, conf)) { + if (!OzoneAdmins.isS3Admin(user, conf)) { requestContext.abortWith(Response.status(Status.FORBIDDEN).build()); } } From e2f9248660b181188b2a3ce5689e30c66d7448b5 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Mon, 21 Oct 2024 21:20:58 +0530 Subject: [PATCH 26/35] Fixed checkstyle issues --- .../java/org/apache/hadoop/hdds/server/OzoneAdmins.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java index 1990a961943..a25e808d4af 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java @@ -30,7 +30,14 @@ import org.apache.hadoop.security.AccessControlException; import org.apache.hadoop.security.UserGroupInformation; -import static org.apache.hadoop.ozone.OzoneConfigKeys.*; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_WILDCARD; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_READONLY_ADMINISTRATORS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_READONLY_ADMINISTRATORS_GROUPS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS; +import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS; + /** * This class contains ozone admin user information, username and group, From 2acffdaa039b0d8ef0b54f81d146664e574e8a96 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Wed, 23 Oct 2024 12:37:44 +0530 Subject: [PATCH 27/35] Refactored createOMProxy to parent provider and enabled GrpcOmTransport for acceptance env --- ...neAdminUtils.java => TestOzoneAdmins.java} | 2 +- .../om/ha/GrpcOMFailoverProxyProvider.java | 34 +-------------- .../ha/HadoopRpcOMFailoverProxyProvider.java | 22 +--------- .../om/ha/OMFailoverProxyProviderBase.java | 41 +++++++++++++++++++ .../compose/ozonesecure/docker-compose.yaml | 2 +- 5 files changed, 46 insertions(+), 55 deletions(-) rename hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/{TestOzoneAdminUtils.java => TestOzoneAdmins.java} (99%) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java similarity index 99% rename from hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java rename to hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java index a6f7a78fa6c..65042d495d0 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdminUtils.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java @@ -31,7 +31,7 @@ /** * This class is to test the utilities present in the OzoneAdmins class. */ -class TestOzoneAdminUtils { +class TestOzoneAdmins { // The following set of tests are to validate the S3 based utilities present in OzoneAdmins @Test void testS3AdminExtraction() throws IOException { diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index feb3a35b504..71f5fb9560c 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -63,15 +63,13 @@ public class GrpcOMFailoverProxyProvider extends public static final Logger LOG = LoggerFactory.getLogger(GrpcOMFailoverProxyProvider.class); - private final UserGroupInformation ugi; private final long protocolVer; public GrpcOMFailoverProxyProvider(ConfigurationSource configuration, UserGroupInformation ugi, String omServiceId, Class protocol) throws IOException { - super(configuration, omServiceId, protocol); - this.ugi = ugi; + super(configuration, ugi, omServiceId, protocol); this.protocolVer = RPC.getProtocolVersion(protocol); } @@ -126,35 +124,7 @@ protected void loadOMClientConfigs(ConfigurationSource config, String omSvcId) private T createOMProxy() throws IOException { InetSocketAddress addr = new InetSocketAddress(0); - return createOmProxy(addr); - } - - /** - * Get the protocol proxy for provided address. - * @param address An instance of {@link InetSocketAddress} which contains the address to connect - * @return the proxy connection to the address and the set of methods supported by the server at the address - * @throws IOException if any error occurs while trying to get the proxy - */ - private T createOmProxy(InetSocketAddress address) throws IOException { - Configuration hadoopConf = - LegacyHadoopConfigurationSource.asHadoopConfiguration(getConf()); - - // TODO: Post upgrade to Protobuf 3.x we need to use ProtobufRpcEngine2 - RPC.setProtocolEngine(hadoopConf, getInterface(), ProtobufRpcEngine.class); - - // Ensure we do not attempt retry on the same OM in case of exceptions - RetryPolicy connectionRetryPolicy = RetryPolicies.failoverOnNetworkException(0); - - return (T) RPC.getProtocolProxy( - getInterface(), - protocolVer, - address, - ugi, - hadoopConf, - NetUtils.getDefaultSocketFactory(hadoopConf), - (int) OmUtils.getOMClientRpcTimeOut(getConf()), - connectionRetryPolicy - ).getProxy(); + return createOMProxy(addr); } /** diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java index 543d2e4aed3..a9d900848d9 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java @@ -61,7 +61,6 @@ public class HadoopRpcOMFailoverProxyProvider extends private final long omVersion; private final Text delegationTokenService; - private final UserGroupInformation ugi; private Map omProxyInfos; private List retryExceptions = new ArrayList<>(); @@ -75,8 +74,7 @@ public HadoopRpcOMFailoverProxyProvider(ConfigurationSource configuration, UserGroupInformation ugi, String omServiceId, Class protocol) throws IOException { - super(configuration, omServiceId, protocol); - this.ugi = ugi; + super(configuration, ugi, omServiceId, protocol); this.omVersion = RPC.getProtocolVersion(protocol); this.delegationTokenService = computeDelegationTokenService(); } @@ -130,24 +128,6 @@ protected void loadOMClientConfigs(ConfigurationSource config, String omSvcId) setOmNodeAddressMap(omNodeAddressMap); } - private T createOMProxy(InetSocketAddress omAddress) throws IOException { - Configuration hadoopConf = - LegacyHadoopConfigurationSource.asHadoopConfiguration(getConf()); - RPC.setProtocolEngine(hadoopConf, getInterface(), ProtobufRpcEngine.class); - - // FailoverOnNetworkException ensures that the IPC layer does not attempt - // retries on the same OM in case of connection exception. This retry - // policy essentially results in TRY_ONCE_THEN_FAIL. - RetryPolicy connectionRetryPolicy = RetryPolicies - .failoverOnNetworkException(0); - - return (T) RPC.getProtocolProxy(getInterface(), omVersion, - omAddress, ugi, hadoopConf, NetUtils.getDefaultSocketFactory( - hadoopConf), (int) OmUtils.getOMClientRpcTimeOut(getConf()), - connectionRetryPolicy).getProxy(); - - } - /** * Get the proxy object which should be used until the next failover event * occurs. RPC proxy object is intialized lazily. diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProviderBase.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProviderBase.java index 1a738b2ac84..5045a32bdcd 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProviderBase.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProviderBase.java @@ -21,17 +21,25 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.protobuf.ServiceException; +import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hdds.HddsUtils; import org.apache.hadoop.hdds.conf.ConfigurationSource; +import org.apache.hadoop.hdds.utils.LegacyHadoopConfigurationSource; import org.apache.hadoop.io.retry.FailoverProxyProvider; +import org.apache.hadoop.io.retry.RetryPolicies; import org.apache.hadoop.io.retry.RetryPolicy; import org.apache.hadoop.io.retry.RetryPolicy.RetryAction.RetryDecision; +import org.apache.hadoop.ipc.ProtobufRpcEngine; +import org.apache.hadoop.ipc.RPC; import org.apache.hadoop.ipc.RemoteException; +import org.apache.hadoop.net.NetUtils; +import org.apache.hadoop.ozone.OmUtils; import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.ozone.om.exceptions.OMException; import org.apache.hadoop.ozone.om.exceptions.OMLeaderNotReadyException; import org.apache.hadoop.ozone.om.exceptions.OMNotLeaderException; import org.apache.hadoop.security.AccessControlException; +import org.apache.hadoop.security.UserGroupInformation; import org.apache.hadoop.security.token.SecretManager; import org.apache.ratis.protocol.exceptions.StateMachineException; import org.slf4j.Logger; @@ -85,13 +93,17 @@ public abstract class OMFailoverProxyProviderBase implements private Set accessControlExceptionOMs = new HashSet<>(); private boolean performFailoverDone; + private final UserGroupInformation ugi; + public OMFailoverProxyProviderBase(ConfigurationSource configuration, + UserGroupInformation ugi, String omServiceId, Class protocol) throws IOException { this.conf = configuration; this.protocolClass = protocol; this.performFailoverDone = true; this.omServiceId = omServiceId; + this.ugi = ugi; waitBetweenRetries = conf.getLong( OzoneConfigKeys.OZONE_CLIENT_WAIT_BETWEEN_RETRIES_MILLIS_KEY, @@ -112,6 +124,35 @@ protected abstract void loadOMClientConfigs(ConfigurationSource config, String omSvcId) throws IOException; + /** + * Get the protocol proxy for provided address. + * @param omAddress An instance of {@link InetSocketAddress} which contains the address to connect + * @return the proxy connection to the address and the set of methods supported by the server at the address + * @throws IOException if any error occurs while trying to get the proxy + */ + protected T createOMProxy(InetSocketAddress omAddress) throws IOException { + Configuration hadoopConf = + LegacyHadoopConfigurationSource.asHadoopConfiguration(getConf()); + + // TODO: Post upgrade to Protobuf 3.x we need to use ProtobufRpcEngine2 + RPC.setProtocolEngine(hadoopConf, getInterface(), ProtobufRpcEngine.class); + + // Ensure we do not attempt retry on the same OM in case of exceptions + RetryPolicy connectionRetryPolicy = RetryPolicies.failoverOnNetworkException(0); + + return (T) RPC.getProtocolProxy( + getInterface(), + RPC.getProtocolVersion(protocolClass), + omAddress, + ugi, + hadoopConf, + NetUtils.getDefaultSocketFactory(hadoopConf), + (int) OmUtils.getOMClientRpcTimeOut(getConf()), + connectionRetryPolicy + ).getProxy(); + } + + protected synchronized boolean shouldFailover(Exception ex) { Throwable unwrappedException = HddsUtils.getUnwrappedException(ex); if (unwrappedException instanceof AccessControlException || diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml index 39d26c362f6..4dc88b92d5e 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml @@ -96,7 +96,7 @@ services: - 9878:9878 env_file: - ./docker-config - command: ["/opt/hadoop/bin/ozone","s3g"] + command: ["/opt/hadoop/bin/ozone","s3g", "-Dozone.om.transport.class=org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransportFactory"] environment: OZONE_OPTS: recon: From c52deaec7d51ec9adea755d75d83e82bcdd74e3d Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Wed, 23 Oct 2024 13:02:04 +0530 Subject: [PATCH 28/35] Removed unused imports and variables --- .../hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java | 8 -------- .../ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java | 8 -------- 2 files changed, 16 deletions(-) diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index 71f5fb9560c..df79be821cc 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -18,15 +18,9 @@ package org.apache.hadoop.ozone.om.ha; import io.grpc.Status; -import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hdds.conf.ConfigurationException; import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.HddsUtils; -import org.apache.hadoop.hdds.utils.LegacyHadoopConfigurationSource; -import org.apache.hadoop.io.retry.RetryPolicies; -import org.apache.hadoop.io.retry.RetryPolicy; -import org.apache.hadoop.ipc.ProtobufRpcEngine; -import org.apache.hadoop.ipc.RPC; import org.apache.hadoop.net.NetUtils; import org.apache.hadoop.ozone.OmUtils; import org.apache.hadoop.ozone.OzoneConsts; @@ -63,14 +57,12 @@ public class GrpcOMFailoverProxyProvider extends public static final Logger LOG = LoggerFactory.getLogger(GrpcOMFailoverProxyProvider.class); - private final long protocolVer; public GrpcOMFailoverProxyProvider(ConfigurationSource configuration, UserGroupInformation ugi, String omServiceId, Class protocol) throws IOException { super(configuration, ugi, omServiceId, protocol); - this.protocolVer = RPC.getProtocolVersion(protocol); } @Override diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java index a9d900848d9..4447a72ab13 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/HadoopRpcOMFailoverProxyProvider.java @@ -29,15 +29,9 @@ import java.util.List; import java.util.Map; -import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hdds.conf.ConfigurationSource; -import org.apache.hadoop.hdds.utils.LegacyHadoopConfigurationSource; import org.apache.hadoop.io.Text; -import org.apache.hadoop.io.retry.RetryPolicies; -import org.apache.hadoop.io.retry.RetryPolicy; -import org.apache.hadoop.ipc.ProtobufRpcEngine; import org.apache.hadoop.ipc.RPC; -import org.apache.hadoop.net.NetUtils; import org.apache.hadoop.ozone.OmUtils; import org.apache.hadoop.ozone.OzoneConsts; import org.apache.hadoop.ozone.ha.ConfUtils; @@ -59,7 +53,6 @@ public class HadoopRpcOMFailoverProxyProvider extends public static final Logger LOG = LoggerFactory.getLogger(HadoopRpcOMFailoverProxyProvider.class); - private final long omVersion; private final Text delegationTokenService; private Map omProxyInfos; private List retryExceptions = new ArrayList<>(); @@ -75,7 +68,6 @@ public HadoopRpcOMFailoverProxyProvider(ConfigurationSource configuration, String omServiceId, Class protocol) throws IOException { super(configuration, ugi, omServiceId, protocol); - this.omVersion = RPC.getProtocolVersion(protocol); this.delegationTokenService = computeDelegationTokenService(); } From 3a606378028792f2b4b0f01de7a7d8bf166a7d45 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Wed, 23 Oct 2024 15:07:57 +0530 Subject: [PATCH 29/35] Add fallback value to OM transport and set it to Hadoop3OmTransport in fcq --- .../dist/src/main/compose/ozonesecure/docker-compose.yaml | 2 +- hadoop-ozone/dist/src/main/compose/ozonesecure/test-fcq.sh | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml index 4dc88b92d5e..026dfa1edc3 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure/docker-compose.yaml @@ -96,7 +96,7 @@ services: - 9878:9878 env_file: - ./docker-config - command: ["/opt/hadoop/bin/ozone","s3g", "-Dozone.om.transport.class=org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransportFactory"] + command: ["/opt/hadoop/bin/ozone","s3g", "-Dozone.om.transport.class=${OZONE_S3_OM_TRANSPORT:-org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransportFactory}"] environment: OZONE_OPTS: recon: diff --git a/hadoop-ozone/dist/src/main/compose/ozonesecure/test-fcq.sh b/hadoop-ozone/dist/src/main/compose/ozonesecure/test-fcq.sh index 644e45c4d5a..a9e87a60cdd 100644 --- a/hadoop-ozone/dist/src/main/compose/ozonesecure/test-fcq.sh +++ b/hadoop-ozone/dist/src/main/compose/ozonesecure/test-fcq.sh @@ -25,6 +25,7 @@ source "$COMPOSE_DIR/../testlib.sh" export SECURITY_ENABLED=true export COMPOSE_FILE=docker-compose.yaml:fcq.yaml +export OZONE_S3_OM_TRANSPORT="org.apache.hadoop.ozone.om.protocolPB.Hadoop3OmTransportFactory" start_docker_env From 9e8c64e08e7b902e8a7331d5654c877ad43b97e4 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Thu, 24 Oct 2024 14:41:45 +0530 Subject: [PATCH 30/35] Addressed review comments --- .../hadoop/hdds/server/OzoneAdmins.java | 41 +++--- .../hadoop/hdds/server/TestOzoneAdmins.java | 138 ++++++++---------- .../om/ha/GrpcOMFailoverProxyProvider.java | 1 - 3 files changed, 81 insertions(+), 99 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java index a25e808d4af..5ee38e12eb7 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.Set; @@ -194,19 +195,25 @@ public static Collection getOzoneReadOnlyAdminsGroupsFromConfig( /** * Get the list of S3 administrators from Ozone config. - * + *

+ * Notes: + *

    + *
  • If ozone.s3.administrators value is empty string or unset, + * defaults to ozone.administrators value.
    • + *
  • If current user is not part of the administrators group, + * {@link UserGroupInformation#getCurrentUser()} will be added to the resulting list
    • + *
* @param conf An instance of {@link OzoneConfiguration} being used * @return A {@link Collection} of the S3 administrator users * - * If ozone.s3.administrators value is empty string or unset, - * defaults to ozone.administrators value. */ - public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { - Collection ozoneAdmins = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS); + public static Set getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { + Set ozoneAdmins = new HashSet<>(conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS)); - if (ozoneAdmins == null || ozoneAdmins.isEmpty()) { - ozoneAdmins = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS); + if (ozoneAdmins.isEmpty()) { + ozoneAdmins = new HashSet<>(conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS)); } + String omSPN = UserGroupInformation.getCurrentUser().getShortUserName(); if (!ozoneAdmins.contains(omSPN)) { ozoneAdmins.add(omSPN); @@ -216,19 +223,19 @@ public static Collection getS3AdminsFromConfig(OzoneConfiguration conf) } /** - * Get the list of the groups that are a part S3 administrators from Ozone config. + * Get the list of the groups that are a part of S3 administrators from Ozone config. + *

+ * Note: If ozone.s3.administrators.groups value is empty or unset, + * defaults to the ozone.administrators.groups value * * @param conf An instance of {@link OzoneConfiguration} being used * @return A {@link Collection} of the S3 administrator groups - * - * If ozone.s3.administrators.groups value is empty or unset, - * defaults to the ozone.administrators.groups value */ - public static Collection getS3AdminsGroupsFromConfig(OzoneConfiguration conf) { - Collection s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS_GROUPS); + public static Set getS3AdminsGroupsFromConfig(OzoneConfiguration conf) { + Set s3AdminsGroup = new HashSet<>(conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS_GROUPS)); if (s3AdminsGroup.isEmpty() && conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS).isEmpty()) { - s3AdminsGroup = conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS_GROUPS); + s3AdminsGroup = new HashSet<>(conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS_GROUPS)); } return s3AdminsGroup; @@ -240,13 +247,13 @@ public static Collection getS3AdminsGroupsFromConfig(OzoneConfiguration * @return an instance of {@link OzoneAdmins} containing the S3 admin users and groups */ public static OzoneAdmins getS3Admins(OzoneConfiguration conf) { - Collection s3Admins; + Set s3Admins; try { s3Admins = getS3AdminsFromConfig(conf); } catch (IOException ie) { - s3Admins = null; + s3Admins = Collections.emptySet(); } - Collection s3AdminGroups = getS3AdminsGroupsFromConfig(conf); + Set s3AdminGroups = getS3AdminsGroupsFromConfig(conf); return new OzoneAdmins(s3Admins, s3AdminGroups); } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java index 65042d495d0..7ec9bc6f15b 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java @@ -21,7 +21,9 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.security.UserGroupInformation; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; import java.io.IOException; import java.util.Arrays; @@ -33,68 +35,49 @@ */ class TestOzoneAdmins { // The following set of tests are to validate the S3 based utilities present in OzoneAdmins - @Test - void testS3AdminExtraction() throws IOException { - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice,bob"); - assertThat(OzoneAdmins.getS3AdminsFromConfig(configuration)) - .containsAll(Arrays.asList("alice", "bob")); + /** + * Value provider method for ParameterizedTest + * @return A 2D array of {@link String} containing the pair of + * Administrator configuration key, Administrator Group configuration key + * to set in the config + */ + static String[][] getAdminsAndGroupsSet() { + return new String[][]{ + { OzoneConfigKeys.OZONE_ADMINISTRATORS, OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS }, + { OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS } + }; } - @Test - void testS3AdminExtractionWithFallback() throws IOException { + @ParameterizedTest + @ValueSource(strings = {OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, + OzoneConfigKeys.OZONE_ADMINISTRATORS}) + void testS3AdminExtraction(String configKey) throws IOException { OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice,bob"); + configuration.set(configKey, "alice,bob"); assertThat(OzoneAdmins.getS3AdminsFromConfig(configuration)) .containsAll(Arrays.asList("alice", "bob")); } - @Test - void testS3AdminGroupExtraction() { - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, - "test1, test2"); - - assertThat(OzoneAdmins.getS3AdminsGroupsFromConfig(configuration)) - .containsAll(Arrays.asList("test1", "test2")); - } - - @Test - void testS3AdminGroupExtractionWithFallback() { + @ParameterizedTest + @ValueSource(strings = {OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, + OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS}) + void testS3AdminGroupExtraction(String configKey) { OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, - "test1, test2"); + configuration.set(configKey, "test1, test2"); assertThat(OzoneAdmins.getS3AdminsGroupsFromConfig(configuration)) .containsAll(Arrays.asList("test1", "test2")); } - @Test - void testGetS3AdminsWhenS3AdminPresent() { - // When there is S3 admin present - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice"); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test_group"); - - OzoneAdmins admins = OzoneAdmins.getS3Admins(configuration); - UserGroupInformation ugi = UserGroupInformation.createUserForTesting( - "alice", new String[] {"test_group"}); - - assertThat(admins.isAdmin(ugi)).isEqualTo(true); - - // Test that when a user is present in an admin group but not an Ozone Admin - UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( - "bob", new String[] {"test_group"}); - assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(true); - } - @Test - void testGetS3AdminsWhenNoS3AdminPresent() { + @ParameterizedTest + @MethodSource(value = "getAdminsAndGroupsSet") + void testIsAdmin(String[] adminsAndGroupsSet) { // When there is no S3 admin, but Ozone admins present OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); + configuration.set(adminsAndGroupsSet[0], "alice"); + configuration.set(adminsAndGroupsSet[1], "test_group"); OzoneAdmins admins = OzoneAdmins.getS3Admins(configuration); UserGroupInformation ugi = UserGroupInformation.createUserForTesting( @@ -108,37 +91,14 @@ void testGetS3AdminsWhenNoS3AdminPresent() { assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(true); } - @Test - void testGetS3AdminsWithNoAdmins() { - OzoneAdmins admins = OzoneAdmins.getS3Admins(new OzoneConfiguration()); - UserGroupInformation ugi = UserGroupInformation.createUserForTesting( - "alice", new String[] {"test_group"}); - assertThat(admins.isAdmin(ugi)).isEqualTo(false); - } - - @Test - void testIsS3AdminForS3AdminUser() { - OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice"); - configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test_group"); - - UserGroupInformation ugi = UserGroupInformation.createUserForTesting( - "alice", new String[] {"test_group"}); - // Scenario when user is present in an admin group but not an Ozone Admin - UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( - "bob", new String[] {"test_group"}); - - assertThat(OzoneAdmins.isS3Admin(ugi, configuration)).isEqualTo(true); - assertThat(OzoneAdmins.isS3Admin(ugiGroupOnly, configuration)).isEqualTo(true); - } - - @Test - void testIsS3AdminForAdminUser() { - // When there is no S3 admin, but Ozone admins present + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void testIsAdminWithUgi(boolean isAdminSet) { OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); - configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); - + if (isAdminSet) { + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); + configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); + } OzoneAdmins admins = OzoneAdmins.getS3Admins(configuration); UserGroupInformation ugi = UserGroupInformation.createUserForTesting( "alice", new String[] {"test_group"}); @@ -146,13 +106,29 @@ void testIsS3AdminForAdminUser() { UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( "bob", new String[] {"test_group"}); - assertThat(admins.isAdmin(ugi)).isEqualTo(true); - assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(true); + assertThat(admins.isAdmin(ugi)).isEqualTo(isAdminSet); + assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(isAdminSet); } - @Test - void testIsS3AdminForNoUser() { + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void testIsS3AdminWithUgiAndConfiguration(boolean isAdminSet) { OzoneConfiguration configuration = new OzoneConfiguration(); - assertThat(OzoneAdmins.isS3Admin(null, configuration)).isEqualTo(false); + if (isAdminSet) { + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice"); + configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test_group"); + UserGroupInformation ugi = UserGroupInformation.createUserForTesting( + "alice", new String[] {"test_group"}); + // Scenario when user is present in an admin group but not an Ozone Admin + UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( + "bob", new String[] {"test_group"}); + + assertThat(OzoneAdmins.isS3Admin(ugi, configuration)).isEqualTo(true); + assertThat(OzoneAdmins.isS3Admin(ugiGroupOnly, configuration)).isEqualTo(true); + } + else { + assertThat(OzoneAdmins.isS3Admin(null, configuration)).isEqualTo(false); + } + } } diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java index df79be821cc..744ada797e7 100644 --- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java +++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/GrpcOMFailoverProxyProvider.java @@ -57,7 +57,6 @@ public class GrpcOMFailoverProxyProvider extends public static final Logger LOG = LoggerFactory.getLogger(GrpcOMFailoverProxyProvider.class); - public GrpcOMFailoverProxyProvider(ConfigurationSource configuration, UserGroupInformation ugi, String omServiceId, From c674834d024dcb2f64ee2933ed577a383aca074b Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Thu, 24 Oct 2024 14:50:30 +0530 Subject: [PATCH 31/35] Fixed checkstyles --- .../apache/hadoop/hdds/server/TestOzoneAdmins.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java index 7ec9bc6f15b..ad8cd7932fd 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java @@ -37,15 +37,15 @@ class TestOzoneAdmins { // The following set of tests are to validate the S3 based utilities present in OzoneAdmins /** - * Value provider method for ParameterizedTest + * Value provider method for ParameterizedTest. * @return A 2D array of {@link String} containing the pair of * Administrator configuration key, Administrator Group configuration key * to set in the config */ static String[][] getAdminsAndGroupsSet() { return new String[][]{ - { OzoneConfigKeys.OZONE_ADMINISTRATORS, OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS }, - { OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS } + {OzoneConfigKeys.OZONE_ADMINISTRATORS, OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS}, + {OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS} }; } @@ -106,8 +106,8 @@ void testIsAdminWithUgi(boolean isAdminSet) { UserGroupInformation ugiGroupOnly = UserGroupInformation.createUserForTesting( "bob", new String[] {"test_group"}); - assertThat(admins.isAdmin(ugi)).isEqualTo(isAdminSet); - assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(isAdminSet); + assertThat(admins.isAdmin(ugi)).isEqualTo(isAdminSet); + assertThat(admins.isAdmin(ugiGroupOnly)).isEqualTo(isAdminSet); } @ParameterizedTest @@ -125,8 +125,7 @@ void testIsS3AdminWithUgiAndConfiguration(boolean isAdminSet) { assertThat(OzoneAdmins.isS3Admin(ugi, configuration)).isEqualTo(true); assertThat(OzoneAdmins.isS3Admin(ugiGroupOnly, configuration)).isEqualTo(true); - } - else { + } else { assertThat(OzoneAdmins.isS3Admin(null, configuration)).isEqualTo(false); } From bd8e47dbb822814c3e134579b334ec2b777565f3 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Thu, 24 Oct 2024 15:23:58 +0530 Subject: [PATCH 32/35] Fixed test issues --- .../hadoop/hdds/server/TestOzoneAdmins.java | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java index ad8cd7932fd..f7079441fbe 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java @@ -22,6 +22,7 @@ import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.security.UserGroupInformation; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; @@ -35,20 +36,6 @@ */ class TestOzoneAdmins { // The following set of tests are to validate the S3 based utilities present in OzoneAdmins - - /** - * Value provider method for ParameterizedTest. - * @return A 2D array of {@link String} containing the pair of - * Administrator configuration key, Administrator Group configuration key - * to set in the config - */ - static String[][] getAdminsAndGroupsSet() { - return new String[][]{ - {OzoneConfigKeys.OZONE_ADMINISTRATORS, OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS}, - {OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS} - }; - } - @ParameterizedTest @ValueSource(strings = {OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, OzoneConfigKeys.OZONE_ADMINISTRATORS}) @@ -72,12 +59,15 @@ void testS3AdminGroupExtraction(String configKey) { } @ParameterizedTest - @MethodSource(value = "getAdminsAndGroupsSet") - void testIsAdmin(String[] adminsAndGroupsSet) { + @CsvSource({ + OzoneConfigKeys.OZONE_ADMINISTRATORS + ", " + OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, + OzoneConfigKeys.OZONE_S3_ADMINISTRATORS + ", " + OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS + }) + void testIsAdmin(String adminKey, String adminGroupKey) { // When there is no S3 admin, but Ozone admins present OzoneConfiguration configuration = new OzoneConfiguration(); - configuration.set(adminsAndGroupsSet[0], "alice"); - configuration.set(adminsAndGroupsSet[1], "test_group"); + configuration.set(adminKey, "alice"); + configuration.set(adminGroupKey, "test_group"); OzoneAdmins admins = OzoneAdmins.getS3Admins(configuration); UserGroupInformation ugi = UserGroupInformation.createUserForTesting( From c331c99f392d34219adf03d2ea87fd8e118111a3 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Thu, 24 Oct 2024 16:17:13 +0530 Subject: [PATCH 33/35] Fixed checkstyle issues --- .../test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java index f7079441fbe..bbfd507d767 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java @@ -23,7 +23,6 @@ import org.apache.hadoop.security.UserGroupInformation; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; -import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import java.io.IOException; From e569e5f31b92e6426b3797d0a436457fe59b3da0 Mon Sep 17 00:00:00 2001 From: Abhishek Pal Date: Sat, 26 Oct 2024 00:09:02 +0530 Subject: [PATCH 34/35] Addressed review comments --- .../org/apache/hadoop/hdds/server/OzoneAdmins.java | 6 +----- .../apache/hadoop/hdds/server/TestOzoneAdmins.java | 13 ++++++++----- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java index 5ee38e12eb7..1f8568866d8 100644 --- a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java +++ b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/server/OzoneAdmins.java @@ -39,7 +39,6 @@ import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS; import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS; - /** * This class contains ozone admin user information, username and group, * and is able to check whether the provided {@link UserGroupInformation} @@ -205,7 +204,6 @@ public static Collection getOzoneReadOnlyAdminsGroupsFromConfig( * * @param conf An instance of {@link OzoneConfiguration} being used * @return A {@link Collection} of the S3 administrator users - * */ public static Set getS3AdminsFromConfig(OzoneConfiguration conf) throws IOException { Set ozoneAdmins = new HashSet<>(conf.getTrimmedStringCollection(OZONE_S3_ADMINISTRATORS)); @@ -215,9 +213,7 @@ public static Set getS3AdminsFromConfig(OzoneConfiguration conf) throws } String omSPN = UserGroupInformation.getCurrentUser().getShortUserName(); - if (!ozoneAdmins.contains(omSPN)) { - ozoneAdmins.add(omSPN); - } + ozoneAdmins.add(omSPN); return ozoneAdmins; } diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java index bbfd507d767..41231eeb26f 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java @@ -21,6 +21,7 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.ozone.OzoneConfigKeys; import org.apache.hadoop.security.UserGroupInformation; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; @@ -35,11 +36,17 @@ */ class TestOzoneAdmins { // The following set of tests are to validate the S3 based utilities present in OzoneAdmins + private static OzoneConfiguration configuration; + + @BeforeEach + void setUp() { + configuration = new OzoneConfiguration(); + } + @ParameterizedTest @ValueSource(strings = {OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, OzoneConfigKeys.OZONE_ADMINISTRATORS}) void testS3AdminExtraction(String configKey) throws IOException { - OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(configKey, "alice,bob"); assertThat(OzoneAdmins.getS3AdminsFromConfig(configuration)) @@ -50,7 +57,6 @@ void testS3AdminExtraction(String configKey) throws IOException { @ValueSource(strings = {OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS}) void testS3AdminGroupExtraction(String configKey) { - OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(configKey, "test1, test2"); assertThat(OzoneAdmins.getS3AdminsGroupsFromConfig(configuration)) @@ -64,7 +70,6 @@ void testS3AdminGroupExtraction(String configKey) { }) void testIsAdmin(String adminKey, String adminGroupKey) { // When there is no S3 admin, but Ozone admins present - OzoneConfiguration configuration = new OzoneConfiguration(); configuration.set(adminKey, "alice"); configuration.set(adminGroupKey, "test_group"); @@ -83,7 +88,6 @@ void testIsAdmin(String adminKey, String adminGroupKey) { @ParameterizedTest @ValueSource(booleans = {true, false}) void testIsAdminWithUgi(boolean isAdminSet) { - OzoneConfiguration configuration = new OzoneConfiguration(); if (isAdminSet) { configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS, "alice"); configuration.set(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS, "test_group"); @@ -102,7 +106,6 @@ void testIsAdminWithUgi(boolean isAdminSet) { @ParameterizedTest @ValueSource(booleans = {true, false}) void testIsS3AdminWithUgiAndConfiguration(boolean isAdminSet) { - OzoneConfiguration configuration = new OzoneConfiguration(); if (isAdminSet) { configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS, "alice"); configuration.set(OzoneConfigKeys.OZONE_S3_ADMINISTRATORS_GROUPS, "test_group"); From bf5b181826215a0d7535811b133ed28e2b00d949 Mon Sep 17 00:00:00 2001 From: "Doroszlai, Attila" <6454655+adoroszlai@users.noreply.github.com> Date: Sat, 26 Oct 2024 07:21:15 +0200 Subject: [PATCH 35/35] Fix findbugs Co-authored-by: Ivan Zlenko <241953+ivanzlenko@users.noreply.github.com> --- .../java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java index 41231eeb26f..47a90d05df7 100644 --- a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java +++ b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/server/TestOzoneAdmins.java @@ -36,7 +36,7 @@ */ class TestOzoneAdmins { // The following set of tests are to validate the S3 based utilities present in OzoneAdmins - private static OzoneConfiguration configuration; + private OzoneConfiguration configuration; @BeforeEach void setUp() {