Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,15 @@ public class ReconControllerModule extends AbstractModule {
private static final Logger LOG =
LoggerFactory.getLogger(ReconControllerModule.class);

private final ReconServer reconServer;

public ReconControllerModule(ReconServer reconServer) {
this.reconServer = reconServer;
}

@Override
protected void configure() {
bind(ReconServer.class).toInstance(reconServer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need reconServer instance to be passes explicitly to ReconControllerModule ? can we not use Singleton ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This follows the same pattern as OM and SCM. In OM, the starter user and admin information are stored as instance fields in the OzoneManager object itself (https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L680-L682). Similarly, we store this information in the ReconServer instance and make it accessible to other components via Guice.

Using bind(ReconServer.class).toInstance(reconServer) allows other components to inject and access the starter user information, just like how OM exposes this through its instance methods.

bind(OzoneConfiguration.class).toProvider(ConfigurationProvider.class);
bind(ReconHttpServer.class).in(Singleton.class);
bind(ReconStorageConfig.class).in(Singleton.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,19 @@
import com.google.inject.Injector;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.util.Collection;
import java.util.concurrent.Callable;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.sql.DataSource;
import org.apache.hadoop.hdds.cli.GenericCli;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocolPB.SCMSecurityProtocolClientSideTranslatorPB;
import org.apache.hadoop.hdds.recon.ReconConfig;
import org.apache.hadoop.hdds.recon.ReconConfigKeys;
import org.apache.hadoop.hdds.scm.server.OzoneStorageContainerManager;
import org.apache.hadoop.hdds.security.SecurityConfig;
import org.apache.hadoop.hdds.security.x509.certificate.client.CertificateClient;
import org.apache.hadoop.hdds.server.OzoneAdmins;
import org.apache.hadoop.hdds.utils.HddsServerUtil;
import org.apache.hadoop.ozone.OzoneSecurityUtil;
import org.apache.hadoop.ozone.recon.api.types.FeatureProvider;
Expand Down Expand Up @@ -85,6 +88,7 @@ public class ReconServer extends GenericCli implements Callable<Void> {
private ReconStorageConfig reconStorage;
private CertificateClient certClient;
private ReconTaskStatusMetrics reconTaskStatusMetrics;
private OzoneAdmins reconAdmins;

private volatile boolean isStarted = false;

Expand All @@ -104,9 +108,23 @@ public Void call() throws Exception {
ReconServer.class, originalArgs, LOG, configuration);
ConfigurationProvider.setConfiguration(configuration);

String reconStarterUser = UserGroupInformation.getCurrentUser().getShortUserName();
Collection<String> adminUsers =
OzoneAdmins.getOzoneAdminsFromConfig(configuration, reconStarterUser);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of calling Admins and AdminsGroups method separately here, can we use this method directly and then add recon admins and recon admin groups. this will minimise the code lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion!
Since we need to add Recon-specific admins from OZONE_RECON_ADMINISTRATORS and OZONE_RECON_ADMINISTRATORS_GROUPS, using addAll() seemed like a straightforward approach.

adminUsers.addAll(
configuration.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS));

Collection<String> adminGroups =
OzoneAdmins.getOzoneAdminsGroupsFromConfig(configuration);
adminGroups.addAll(
configuration.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS_GROUPS));

reconAdmins = new OzoneAdmins(adminUsers, adminGroups);
LOG.info("Recon start with adminUsers: {}", reconAdmins.getAdminUsernames());

LOG.info("Initializing Recon server...");
try {
injector = Guice.createInjector(new ReconControllerModule(),
injector = Guice.createInjector(new ReconControllerModule(this),
new ReconRestServletModule(configuration),
new ReconSchemaGenerationModule());

Expand Down Expand Up @@ -427,4 +445,14 @@ public ReconTaskController getReconTaskController() {
ReconHttpServer getHttpServer() {
return httpServer;
}

/**
* Check if a user is a Recon administrator.
*
* @param user UserGroupInformation
* @return true if the user is an admin, false otherwise
*/
public boolean isAdmin(UserGroupInformation user) {
return reconAdmins.isAdmin(user);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.google.inject.Singleton;
import java.io.IOException;
import java.security.Principal;
import java.util.Collection;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
Expand All @@ -30,10 +29,7 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.recon.ReconConfigKeys;
import org.apache.hadoop.hdds.server.OzoneAdmins;
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.recon.ReconServer;
import org.apache.hadoop.security.UserGroupInformation;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -48,15 +44,17 @@ public class ReconAdminFilter implements Filter {
private static final Logger LOG =
LoggerFactory.getLogger(ReconAdminFilter.class);

private final OzoneConfiguration conf;
private final ReconServer reconServer;

@Inject
ReconAdminFilter(OzoneConfiguration conf) {
this.conf = conf;
ReconAdminFilter(ReconServer reconServer) {
this.reconServer = reconServer;
}

@Override
public void init(FilterConfig filterConfig) throws ServletException { }
public void init(FilterConfig filterConfig) throws ServletException {
LOG.info("ReconAdminFilter initialized");
}

@Override
public void doFilter(ServletRequest servletRequest,
Expand Down Expand Up @@ -100,15 +98,6 @@ public void doFilter(ServletRequest servletRequest,
public void destroy() { }

private boolean hasPermission(UserGroupInformation user) {
Collection<String> admins =
conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS);
admins.addAll(
conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS));
Collection<String> adminGroups =
conf.getStringCollection(OzoneConfigKeys.OZONE_ADMINISTRATORS_GROUPS);
adminGroups.addAll(
conf.getStringCollection(
ReconConfigKeys.OZONE_RECON_ADMINISTRATORS_GROUPS));
return new OzoneAdmins(admins, adminGroups).isAdmin(user);
return reconServer.isAdmin(user);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,15 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.google.common.collect.Sets;
import java.io.IOException;
import java.security.Principal;
import java.util.Collection;
import java.util.HashSet;
import java.util.Set;
import javax.servlet.FilterChain;
Expand All @@ -34,7 +37,9 @@
import javax.ws.rs.Path;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.recon.ReconConfigKeys;
import org.apache.hadoop.hdds.server.OzoneAdmins;
import org.apache.hadoop.ozone.OzoneConfigKeys;
import org.apache.hadoop.ozone.recon.ReconServer;
import org.apache.hadoop.ozone.recon.api.AdminOnly;
import org.apache.hadoop.ozone.recon.api.ClusterStateEndpoint;
import org.apache.hadoop.ozone.recon.api.MetricsProxyEndpoint;
Expand Down Expand Up @@ -170,8 +175,31 @@ public void testAdminFilterNoAdmins() throws Exception {
testAdminFilterWithPrincipal(new OzoneConfiguration(), "reject", false);
}

@Test
public void testAdminFilterStarterUserAutoAdmin() throws Exception {
OzoneConfiguration conf = new OzoneConfiguration();
String currentUser = UserGroupInformation.getCurrentUser().getShortUserName();

testAdminFilterWithPrincipal(conf, currentUser, true);
testAdminFilterWithPrincipal(conf, "otheruser", false);
}

@Test
public void testAdminFilterStarterUserPlusConfiguredAdmins() throws Exception {
OzoneConfiguration conf = new OzoneConfiguration();
conf.setStrings(OzoneConfigKeys.OZONE_ADMINISTRATORS, "configadmin");

String currentUser = UserGroupInformation.getCurrentUser().getShortUserName();

testAdminFilterWithPrincipal(conf, currentUser, true);
testAdminFilterWithPrincipal(conf, "configadmin", true);
testAdminFilterWithPrincipal(conf, "reject", false);
}

private void testAdminFilterWithPrincipal(OzoneConfiguration conf,
String principalToUse, boolean shouldPass) throws Exception {
ReconServer mockReconServer = createMockReconServer(conf);

Principal mockPrincipal = mock(Principal.class);
when(mockPrincipal.getName()).thenReturn(principalToUse);
HttpServletRequest mockRequest = mock(HttpServletRequest.class);
Expand All @@ -180,13 +208,41 @@ private void testAdminFilterWithPrincipal(OzoneConfiguration conf,
HttpServletResponse mockResponse = mock(HttpServletResponse.class);
FilterChain mockFilterChain = mock(FilterChain.class);

new ReconAdminFilter(conf).doFilter(mockRequest, mockResponse,
mockFilterChain);
ReconAdminFilter filter = new ReconAdminFilter(mockReconServer);
filter.init(null);
filter.doFilter(mockRequest, mockResponse, mockFilterChain);

if (shouldPass) {
verify(mockFilterChain).doFilter(mockRequest, mockResponse);
} else {
verify(mockResponse).setStatus(HttpServletResponse.SC_FORBIDDEN);
}
}

/**
* Creates a mock ReconServer that mimics the actual admin initialization logic.
*/
private ReconServer createMockReconServer(OzoneConfiguration conf) throws IOException {
String reconStarterUser = UserGroupInformation.getCurrentUser().getShortUserName();
Collection<String> adminUsers =
OzoneAdmins.getOzoneAdminsFromConfig(conf, reconStarterUser);
adminUsers.addAll(
conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS));

Collection<String> adminGroups =
OzoneAdmins.getOzoneAdminsGroupsFromConfig(conf);
adminGroups.addAll(
conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS_GROUPS));

OzoneAdmins reconAdmins = new OzoneAdmins(adminUsers, adminGroups);

ReconServer mockReconServer = mock(ReconServer.class);
when(mockReconServer.isAdmin(any(UserGroupInformation.class)))
.thenAnswer(invocation -> {
UserGroupInformation user = invocation.getArgument(0);
return reconAdmins.isAdmin(user);
});

return mockReconServer;
}
}