[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
ayushtkn commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1217470177 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java: ## @@ -712,13 +715,22 @@ public long getCurrentTokensCount() { @Override public String getTopTokenRealOwners() { Review Comment: @NishthaShah Where is this test added, can you add a test in the PR as well? -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
ayushtkn commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1217529624 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java: ## @@ -712,13 +715,22 @@ public long getCurrentTokensCount() { @Override public String getTopTokenRealOwners() { Review Comment: TestRouterHDFSContractDelegationToken contract test aren't a good place, can explore ``TestRouterSecurityManager`` -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
ayushtkn commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1219823209 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java: ## @@ -259,6 +262,15 @@ private static String[] getUserGroupForTesting() { return groupsForTesting; } + @Test + public void testGetTopTokenRealOwners() throws Exception { Review Comment: this test passes only when run with the entire class, but fails if run alone ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/metrics/RBFMetrics.java: ## @@ -712,13 +715,21 @@ public long getCurrentTokensCount() { @Override public String getTopTokenRealOwners() { -RouterSecurityManager mgr = -this.router.getRpcServer().getRouterSecurityManager(); -if (mgr != null && mgr.getSecretManager() != null) { - return JSON.toString(mgr.getSecretManager() - .getTopTokenRealOwners(this.topTokenRealOwners)); +String topTokenRealOwnersString = ""; +RouterSecurityManager routerSecurityManager = +this.router.getRpcServer().getRouterSecurityManager(); +if (routerSecurityManager == null || routerSecurityManager.getSecretManager() == null) + return topTokenRealOwnersString; +try { + List topOwners = routerSecurityManager.getSecretManager() + .getTopTokenRealOwners(this.topTokenRealOwners); + if (!topOwners.isEmpty()) { +topTokenRealOwnersString = new ObjectMapper().writeValueAsString(topOwners); + } +} catch (JsonProcessingException jpe) { + LOG.error("Unable to parse the top token real owners to string {}", jpe.getMessage()); } -return ""; +return topTokenRealOwnersString; Review Comment: Can we use a method from ``JsonUtil.java`` ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java: ## @@ -259,6 +262,15 @@ private static String[] getUserGroupForTesting() { return groupsForTesting; } + @Test + public void testGetTopTokenRealOwners() throws Exception { +RouterMBean bean = FederationTestUtils.getBean( +ROUTER_BEAN, RouterMBean.class); +String topTokenRealOwners = bean.getTopTokenRealOwners(); +assertNotNull("The value of output should not be null " + topTokenRealOwners, topTokenRealOwners); +assertTrue("The value contains incorrect message " + topTokenRealOwners, !topTokenRealOwners.contains("NameValuePair")); Review Comment: can we assert some output which should be there post this fix rather than what shouldn't be there? btw. rather than ``!``, you could have used ``assertFalse`` -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
ayushtkn commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1222765730 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java: ## @@ -81,6 +87,17 @@ public static void createMockSecretManager() throws IOException { @Rule public ExpectedException exceptionRule = ExpectedException.none(); + private Router initializeAndStartRouter(Configuration configuration) { +Router router = new Router(); +try { + router.init(configuration); + router.start(); +} catch (MetricsException e) { + //do nothing Review Comment: I don't think we need this try-catch logic itself, put ``` DefaultMetricsSystem.setMiniClusterMode(true); ``` in the ``BeforeClass`` And can you run the test locally as well, for me if ``testNotRunningSecretManager`` runs after your test it fails ``` java.lang.AssertionError: Expecting org.apache.hadoop.service.ServiceStateException with text Failed to create SecretManager but got : Expected to find 'Failed to create SecretManager' but got unexpected exception: org.apache.hadoop.service.ServiceStateException: org.apache.hadoop.security.KerberosAuthException: failure to login: for principal: router/localh...@example.com from keytab /Users/ayushsaxena/code/hadoop-code/hadoop/hadoop-hdfs-project/hadoop-hdfs-rbf/target/test/data/SecurityConfUtil/test.keytab javax.security.auth.login.LoginException: Integrity check on decrypted field failed (31) - PREAUTH_FAILED at org.apache.hadoop.service.ServiceStateException.convert(ServiceStateException.java:105) at org.apache.hadoop.service.AbstractService.init(AbstractService.java:174) at org.apache.hadoop.hdfs.server.federation.security.TestRouterSecurityManager.lambda$testNotRunningSecretManager$1(TestRouterSecurityManager.java:327) at org.apache.hadoop.test.LambdaTestUtils.lambda$intercept$0(LambdaTestUtils.java:534) at org.apache.hadoop.test.LambdaTestUtils.intercept(LambdaTestUtils.java:498) at org.apache.hadoop.test.LambdaTestUtils.intercept(LambdaTestUtils.java:529) at org.apache.hadoop.hdfs.server.federation.security.TestRouterSecurityManager.testNotRunningSecretManager(TestRouterSecurityManager.java:326) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:258) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63) at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329) at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306) at org.junit.runners.ParentRunner.run(ParentRunner.java:413) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69) at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38) at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11) at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35) at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235) at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54) Caused by: org.apache.hadoop.security.KerberosAuthException: failure to login: for principal: router/localh...@example.com from keytab /Users/ayushsaxena/
[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
ayushtkn commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1223781601 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java: ## @@ -259,6 +272,39 @@ private static String[] getUserGroupForTesting() { return groupsForTesting; } + @Test + public void testGetTopTokenRealOwners() throws Exception { +// Create conf and start routers with only an RPC service +Configuration conf = initSecurity(); + +Configuration routerConf = new RouterConfigBuilder() +.metrics() +.rpc() +.build(); +conf.addResource(routerConf); + +Router router = initializeAndStartRouter(conf); + +// Create credentials +UserGroupInformation ugi = UserGroupInformation.createUserForTesting("router", getUserGroupForTesting()); +Credentials creds = RouterSecurityManager.createCredentials(router, ugi, "some_renewer"); Review Comment: assigning to ``creds`` is not required maybe -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] ayushtkn commented on a diff in pull request #5696: HDFS-16946. Fix getTopTokenRealOwners to return String
ayushtkn commented on code in PR #5696: URL: https://github.com/apache/hadoop/pull/5696#discussion_r1224038596 ## hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java: ## @@ -259,6 +272,39 @@ private static String[] getUserGroupForTesting() { return groupsForTesting; } + @Test + public void testGetTopTokenRealOwners() throws Exception { +// Create conf and start routers with only an RPC service +Configuration conf = initSecurity(); + +Configuration routerConf = new RouterConfigBuilder() +.metrics() +.rpc() +.build(); +conf.addResource(routerConf); + +Router router = initializeAndStartRouter(conf); + +// Create credentials +UserGroupInformation ugi = UserGroupInformation.createUserForTesting("router", getUserGroupForTesting()); +Credentials creds = RouterSecurityManager.createCredentials(router, ugi, "some_renewer"); Review Comment: only doing this ``` RouterSecurityManager.createCredentials(router, ugi, "some_renewer"); ``` instead of ``` Credentials creds = RouterSecurityManager.createCredentials(router, ugi, "some_renewer"); ``` creates issues, it is just not assigning to creds, because we don't use ``creds`` variable anywhere -- 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org