adoroszlai commented on code in PR #5449:
URL: https://github.com/apache/ozone/pull/5449#discussion_r1369799823
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/TestStorageContainerManager.java:
##########
@@ -190,85 +190,80 @@ public void cleanupDefaults() {
public void testRpcPermission() throws Exception {
// Test with default configuration
OzoneConfiguration defaultConf = new OzoneConfiguration();
- testRpcPermissionWithConf(defaultConf, "unknownUser", true);
+ testRpcPermissionWithConf(defaultConf, any -> false, "unknownUser");
// Test with ozone.administrators defined in configuration
+ String admins = "adminUser1, adminUser2";
OzoneConfiguration ozoneConf = new OzoneConfiguration();
- ozoneConf.setStrings(OzoneConfigKeys.OZONE_ADMINISTRATORS,
- "adminUser1, adminUser2");
+ ozoneConf.setStrings(OzoneConfigKeys.OZONE_ADMINISTRATORS, admins);
// Non-admin user will get permission denied.
- testRpcPermissionWithConf(ozoneConf, "unknownUser", true);
// Admin user will pass the permission check.
- testRpcPermissionWithConf(ozoneConf, "adminUser2", false);
+ testRpcPermissionWithConf(ozoneConf, admins::contains,
+ "unknownUser", "adminUser2");
}
private void testRpcPermissionWithConf(
- OzoneConfiguration ozoneConf, String fakeRemoteUsername,
- boolean expectPermissionDenied) throws Exception {
+ OzoneConfiguration ozoneConf,
+ Predicate<String> isAdmin,
+ String... usernames) throws Exception {
MiniOzoneCluster cluster = MiniOzoneCluster.newBuilder(ozoneConf).build();
- cluster.waitForClusterToBeReady();
try {
+ cluster.waitForClusterToBeReady();
+ for (String username : usernames) {
+ testRpcPermission(cluster, username,
+ !isAdmin.test(username));
+ }
+ } finally {
+ cluster.shutdown();
+ }
+ }
- SCMClientProtocolServer mockClientServer = Mockito.spy(
- cluster.getStorageContainerManager().getClientProtocolServer());
-
-
mockRemoteUser(UserGroupInformation.createRemoteUser(fakeRemoteUsername));
+ private void testRpcPermission(MiniOzoneCluster cluster,
+ String fakeRemoteUsername, boolean expectPermissionDenied) {
+ SCMClientProtocolServer mockClientServer = Mockito.spy(
+ cluster.getStorageContainerManager().getClientProtocolServer());
- try {
- mockClientServer.deleteContainer(
- ContainerTestHelper.getTestContainerID());
- fail("Operation should fail, expecting an IOException here.");
- } catch (Exception e) {
- if (expectPermissionDenied) {
- verifyPermissionDeniedException(e, fakeRemoteUsername);
- } else {
- // If passes permission check, it should fail with
- // container not exist exception.
- Assert.assertTrue(e instanceof ContainerNotFoundException);
- }
- }
+ mockRemoteUser(UserGroupInformation.createRemoteUser(fakeRemoteUsername));
- try {
- ContainerWithPipeline container2 = mockClientServer
- .allocateContainer(SCMTestUtils.getReplicationType(ozoneConf),
- HddsProtos.ReplicationFactor.ONE, OzoneConsts.OZONE);
- if (expectPermissionDenied) {
- fail("Operation should fail, expecting an IOException here.");
- } else {
- Assert.assertEquals(1, container2.getPipeline().getNodes().size());
- }
- } catch (Exception e) {
+ try {
+ mockClientServer.deleteContainer(
+ ContainerTestHelper.getTestContainerID());
+ fail("Operation should fail, expecting an IOException here.");
+ } catch (Exception e) {
+ if (expectPermissionDenied) {
verifyPermissionDeniedException(e, fakeRemoteUsername);
+ } else {
+ // If passes permission check, it should fail with
+ // container not exist exception.
+ Assert.assertTrue(e instanceof ContainerNotFoundException);
}
+ }
- try {
- ContainerWithPipeline container3 = mockClientServer
- .allocateContainer(SCMTestUtils.getReplicationType(ozoneConf),
- HddsProtos.ReplicationFactor.ONE, OzoneConsts.OZONE);
- if (expectPermissionDenied) {
- fail("Operation should fail, expecting an IOException here.");
- } else {
- Assert.assertEquals(1, container3.getPipeline().getNodes().size());
- }
- } catch (Exception e) {
- verifyPermissionDeniedException(e, fakeRemoteUsername);
+ try {
+ ContainerWithPipeline container2 = mockClientServer.allocateContainer(
Review Comment:
Thanks @Galsza for the review.
There was no `container` or `container1` earlier either.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
