-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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. #4712
Conversation
Because the implementation of YarnRouterApi (DelegationToken) requires a router in safe mode, I will continue to follow up on this pr, thanks to those who originally contributed to this jira. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -195,4 +197,12 @@ public static void main(String[] argv) { | |||
System.exit(-1); | |||
} | |||
} | |||
|
|||
public RouterClientRMService getClientRMProxyService() { |
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.
VisibleForTesting?
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.
Thank you very much for helping to review the code, I will modify the code.
...outer/src/main/java/org/apache/hadoop/yarn/server/router/clientrm/RouterClientRMService.java
Show resolved
Hide resolved
|
||
private static final Logger LOG = LoggerFactory.getLogger(AbstractSecureRouterTest.class); | ||
|
||
public static final Configuration CONF; |
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.
Why do we make this static?
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.
This part is not necessary, the reason for defining it as static is to initialize it in beforeSecureRouterTestClass.
@BeforeClass
public static void beforeSecureRouterTestClass() throws Exception {
// Sets up the KDC and Principals.
setupKDCAndPrincipals();
// Init YarnConfiguration
conf = new YarnConfiguration();
// Enable Kerberos authentication configuration
conf.setBoolean(CommonConfigurationKeysPublic.HADOOP_SECURITY_AUTHORIZATION, true);
conf.set(CommonConfigurationKeysPublic.HADOOP_SECURITY_AUTHENTICATION, KERBEROS);
// Router configuration
conf.set(YarnConfiguration.ROUTER_BIND_HOST, "0.0.0.0");
conf.set(YarnConfiguration.ROUTER_CLIENTRM_INTERCEPTOR_CLASS_PIPELINE,
CLIENT_RM_FEDERATION_CLIENT_INTERCEPTOR);
// Router Kerberos KeyTab configuration
conf.set(YarnConfiguration.ROUTER_PRINCIPAL, ROUTER_LOCALHOST_REALM);
conf.set(YarnConfiguration.ROUTER_KEYTAB, routerKeytab.getAbsolutePath());
}
public static final String SUN_SECURITY_KRB5_DEBUG = "sun.security.krb5.debug"; | ||
|
||
public static final String CLIENT_RM_FEDERATION_CLIENT_INTERCEPTOR = | ||
"org.apache.hadoop.yarn.server.router.clientrm.FederationClientInterceptor"; |
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.
Can we do something like FederationClientInterceptor.class.toString()?
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.
your suggestion is correct and I will fix it.
*/ | ||
public static void setupSecureMockRM() throws Exception { | ||
for (int i = 0; i < NUM_SUBCLUSTER; i++) { | ||
SubClusterId sc = SubClusterId.newInstance(Integer.toString(i)); |
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.
We keep doing this. Can we create a newInstance taking integer?
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 define a method in SubCluster with integer parameter in new newInstance.
* @throws Exception an error occurred. | ||
*/ | ||
public static File createKeytab(String principal, String filename) throws Exception { | ||
Assert.assertTrue("empty principal", StringUtils.isNotBlank(principal)); |
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.
Let's statically import the asserts
|
||
MockRM firstRM = mockRMs.entrySet().stream().findFirst().get().getValue(); | ||
RouterRMAdminService routerRMAdminService = router.getRmAdminProxyService(); | ||
RouterRMAdminService.RequestInterceptorChainWrapper rmAdminChainWrapper = |
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.
We had to expose a lot of these methods for the tests.
Is there a better way? Some mock or test class extension?
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.
Some mock or test class extension?
This is a good idea, I will refactor this part of the code.
...uter/src/test/java/org/apache/hadoop/yarn/server/router/secure/AbstractSecureRouterTest.java
Show resolved
Hide resolved
import java.util.Properties; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
|
||
public class AbstractSecureRouterTest { |
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.
Are we going to use this abstract test in more places?
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.
This class should be abstract class, I will add abstract identifier on the class.
@@ -4107,6 +4107,12 @@ public static boolean isAclEnabled(Configuration conf) { | |||
public static final long DEFAULT_ROUTER_WEBAPP_READ_TIMEOUT = | |||
TimeUnit.SECONDS.toMillis(30); | |||
|
|||
/** The Kerberos keytab for the yarn router.*/ | |||
public static final String ROUTER_KEYTAB = ROUTER_PREFIX + "keytab"; |
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.
Should we add some documentation on how to set this up?
Is there an md file?
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Please help to review the code again, Thank you very much! I want to follow up on YARN-11158, need this pr. |
@goiri Thank you very much for your help reviewing the code! |
JIRA. YARN-6539. Create SecureLogin inside Router.
The purpose of this pr is to serve the subsequent development and testing of DelegationToken-related APIs, complete the following:
1.Router supports SecureLogin.
2.UnmanagedApplicationManager supports ProxyUser.
3.Amrmproxy Interceptor supports ProxyUser.
4.Write code to support RouterSecure tests, covering RouterSecureLogin、RouterClientRMProxyService、RouterRMAdminService Basic Test.
5.TestSecurityMockRM is used in the test to support the DelegationToken test.