----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40635/#review108018 -----------------------------------------------------------
Thanks for picking this up Dian! I really like how we're making the Kerberos integration for our test suite in a way that is compatible with making them run on a real cluster, huge +1 from my side! Couple of general notes: 1) I don't like how we had to add "throws Exception" to a lot of methods that are not suppose to throw Exception (as a class). I can see that it's mostly because we have added KerberosTestUtils.authenticateWithSqoopServer() to the getClient() method. I have note below mentioning that I don't feel that it's right approach, but in case that there is no other way, can we perhaps move that logic to @Before method rather then adding "throws Exception" everywhere? 2) I know that the RealKdcRunner currently does nothing and hence it's also serving the purpose of "NoKerberosRunner", but can we add such class explicitly? I'm pretty sure that once we start testing the RealKdcRunner, we will have to add some code inside and will no longer me be "NoKerberosRunner", so we should have such runner present. client/src/main/java/org/apache/sqoop/client/request/SqoopResourceRequests.java (lines 49 - 51) <https://reviews.apache.org/r/40635/#comment167393> I'm a bit concerned with exposing this method as public just for one single test case. Can we generate the token in the RestTest separately without getting it from the client? Or is that too much copy&pasting around? pom.xml (lines 774 - 778) <https://reviews.apache.org/r/40635/#comment167392> I'm not sure what the felix dependency provide us. Would you mind describing that? test/src/main/java/org/apache/sqoop/test/infrastructure/providers/KdcInfrastructureProvider.java (lines 49 - 50) <https://reviews.apache.org/r/40635/#comment167400> Shouldn't this be part of MiniKdcRunner rather then inside the provider itself? I don't think that it's relevant for RealKdcRunner for example. test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java (lines 62 - 70) <https://reviews.apache.org/r/40635/#comment167401> Would you mind describing why do we need to use "localhost" here? Shouldn't we configure the kerberos environment to work with FQDN rather then localhost (e.g. nobody will use localhost in production). test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java (line 202) <https://reviews.apache.org/r/40635/#comment167402> Shouldn't we be checking the provider rather then a UtilClass here? test/src/main/java/org/apache/sqoop/test/utils/KerberosTestUtils.java (line 42) <https://reviews.apache.org/r/40635/#comment167411> Looking through this class, I feel that it's not really a KerberosTestUtils, as it's relevant only to MiniKdcRunner. Can we perhaps refactore it in a way to move most of the relevant code to the MiniKDC runner itself? E.g. stuff like principal, keytab creation, ... test/src/main/java/org/apache/sqoop/test/utils/KerberosTestUtils.java (lines 155 - 166) <https://reviews.apache.org/r/40635/#comment167410> This seems very fragile approach. Is there a reason why in case of MiniKdc we're not loading generated credentials to the JVM itsemf? This way we would not have to do any "hacks" to propagate the security context and at the same time we would emulate what's gonna happen in real production environments. Jarcec - Jarek Cecho On Nov. 25, 2015, 4:50 a.m., Dian Fu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40635/ > ----------------------------------------------------------- > > (Updated Nov. 25, 2015, 4:50 a.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-2705 > https://issues.apache.org/jira/browse/SQOOP-2705 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > At the first step of providing kerberos support for integration tests, I'd > like to add kerberos support for SqoopMiniCluster make sure the communication > between sqoop client and sqoop server are kerberos enabled. > > > Diffs > ----- > > client/src/main/java/org/apache/sqoop/client/SqoopClient.java 3d3425d > > client/src/main/java/org/apache/sqoop/client/request/SqoopResourceRequests.java > 0987703 > pom.xml 91721ce > test/pom.xml 4e1e197 > test/src/main/java/org/apache/sqoop/test/infrastructure/SqoopTestCase.java > 4c5d3a8 > > test/src/main/java/org/apache/sqoop/test/infrastructure/providers/KdcInfrastructureProvider.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/KdcRunner.java PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/KdcRunnerFactory.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/MiniKdcRunner.java > PRE-CREATION > test/src/main/java/org/apache/sqoop/test/kdc/RealKdcRunner.java > PRE-CREATION > > test/src/main/java/org/apache/sqoop/test/minicluster/JettySqoopMiniCluster.java > 325a790 > test/src/main/java/org/apache/sqoop/test/minicluster/SqoopMiniCluster.java > 9ae941f > test/src/main/java/org/apache/sqoop/test/testcases/JettyTestCase.java > bd4ba6a > test/src/main/java/org/apache/sqoop/test/utils/KerberosTestUtils.java > PRE-CREATION > > test/src/test/java/org/apache/sqoop/integration/server/InformalObjectNameTest.java > 920679f > > test/src/test/java/org/apache/sqoop/integration/server/ShowJobInOrderTest.java > cbf1e90 > > test/src/test/java/org/apache/sqoop/integration/server/SubmissionWithDisabledModelObjectsTest.java > 9e682bc > test/src/test/java/org/apache/sqoop/integration/server/VersionTest.java > e86a6cc > > test/src/test/java/org/apache/sqoop/integration/server/rest/LinkRestTest.java > 99959ac > test/src/test/java/org/apache/sqoop/integration/server/rest/RestTest.java > 4ac564c > > Diff: https://reviews.apache.org/r/40635/diff/ > > > Testing > ------- > > > Thanks, > > Dian Fu > >
