sqoop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jarek Cecho" <jar...@apache.org>
Subject Re: Review Request 40635: Sqoop2: Add kerberos support for SqoopMiniCluster
Date Wed, 25 Nov 2015 18:00:37 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message