[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
