New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
YARN-6539. Create SecureLogin inside Router. #4354
Conversation
🎊 +1 overall
This message was automatically generated. |
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Outdated
Show resolved
Hide resolved
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
@goiri I have already fix it. Can you help me review it again and check whether could merge to truck or not? Thanks again. |
...ain/java/org/apache/hadoop/yarn/server/router/clientrm/AbstractClientRequestInterceptor.java
Show resolved
Hide resolved
testMiniKDC.start(); | ||
testMiniKDC.createPrincipal(routerKeytabFile, "yarn/localhost"); | ||
} catch (Exception e) { | ||
assertTrue("Couldn't setup MiniKDC", false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this trying to fail if there is a failure?
Just do:
fail("Couldn't setup MiniKDC")
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Show resolved
Hide resolved
UserGroupInformation.setConfiguration(conf); | ||
router = new Router(); | ||
router.init(conf); | ||
router.start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stop part?
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri |
@zhengchenyu Can this pr guarantee that the router will run for a long time? Is ticket renewa considered? |
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Show resolved
Hide resolved
router.start(); | ||
assertEquals(UserGroupInformation.getLoginUser() | ||
.getAuthenticationMethod(), AuthenticationMethod.KERBEROS); | ||
assertEquals(UserGroupInformation.getLoginUser().getUserName(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually expected goes first.
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Show resolved
Hide resolved
@Test | ||
public void testRouterSecureLogin() { | ||
try { | ||
conf = new YarnConfiguration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define conf in the method, no need to make it a field.
@slfan1989 The purpose of SecureLogin is that we can't fake user to start router in startup stage. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
LGTM +1 |
@goiri @steveloughran How about this PR merge to trunk? |
i don't go near YARN. better that way for all. asking me to review things there is counterproductive, because at best all that will happen is that I will request changes of the test code, so adding extra homework. once you've done that you will still need a YARN expert to review the production code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't review hdfs or yarn code. looked at the tests
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Outdated
Show resolved
Hide resolved
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Outdated
Show resolved
Hide resolved
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Outdated
Show resolved
Hide resolved
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Outdated
Show resolved
Hide resolved
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Show resolved
Hide resolved
Okay, I know. Thank you all the same for review! |
...rver-router/src/test/java/org/apache/hadoop/yarn/server/router/security/TestSecureLogin.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Description of PR
https://issues.apache.org/jira/browse/YARN-6539
This PR is contributed by Xie YiFan, I transform from patch to PR, and fix some ut then add yarn-default.xml and license.
It is precondition so that we could use yarn federation in security mode.
How was this patch tested?
uni-test and test in our cluster manually.
For code changes:
support security for router