-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47354/#review149566
-----------------------------------------------------------




src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java (line 
112)
<https://reviews.apache.org/r/47354/#comment217205>

    This inprogressConnections is used to make sure we don't have duplicated 
auth session for outgoing connections, so it make sense to me.
    
    However, this set only applies to outgoing connections. For incoming 
connections, there is no corresponding structure to deduplicate. Do we need to 
also do similar book keeping on the receiver side (i.e. in handleConnections)?



src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java (line 
161)
<https://reviews.apache.org/r/47354/#comment217204>

    I have a mixed feeling about this change - it makes QuorumCnxManager's 
constructor more complicated comparing to the previous constructor 
QuorumCnxManager(QuorumPeer), where we just pass a QuorumPeer object. That said 
though, with the new QuorumPeer.createCnxManager method, this is not a big 
problem, as a QuorumCnxManager can be conveniently created given a QuorumPeer 
object. 
    
    Alternatively, we can keep QuorumCnxManager's constructor as it is, so it 
still accepts a QuorumPeer object and inside QuorumCnxManager's constructor, we 
initialize various fields required here (such as authServer/Learner, and so 
on.). This requires QuorumCnxManager be able to access QuorumPeer's protected / 
private fields, so it requires adding some getter methods on QuorumPeer to 
expose those properties. 
    
    Overall I don't have a strong opinion one way or the other, wondering what 
other reviewers think about this.



src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java (line 
212)
<https://reviews.apache.org/r/47354/#comment217206>

    Should we put the check of in progress sid here, or up on the caller site 
(connectOne) who calls initateConnection? I am asking because I spotted there 
could be a potential socket leak if we put check here. Consider this case:
    
    * In connectOne(long sid), we first open a new socket and then call 
initiateConnection(sock, sid).
    * In side initiateConnection(sock, sid), we are doing this sid check and 
will return if we find out there is an ongoing outgoing connections to the same 
sid.
    * Now, the opened socket in connectOne is a strayed socket, and will not 
get used, or closed.
    
    If we put same check in connectOne, this problem can be fixed. Does this 
make sense for the case I am describing above?



src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java (line 
319)
<https://reviews.apache.org/r/47354/#comment217213>

    Here we create new threads to handle both incoming connection request 
handling and out going connection initiation and set up. It makes sense to me 
as SASL auth evaluation might take a while - I am curious, Rakesh, do you have 
any rough idea / data on how long the auth would take? Would it take  at the 
magnitude of million seconds, or seconds, or even minutes? My impression is 
that SASL evaluation between client and server should be done at the scale of 
seconds at most, as it purely involves two servers which sending small data 
between each other, and does not involve KDC (as Kerberos login should have 
already been finished when we reach here.).
    
    I am trying to justify the usage of threads here because we did not create 
threads to handle such connection requests, so this change is pretty big in 
terms of impact to existing codebase. Because we don't create threads, the old 
code base was executed purely sequentially. With threads, the semantic is 
slightly changed as the code after receiveConnection might be executing without 
receiveConnection actually finished. So far I have not spotted anywhere in code 
base with such sequential dependency, but want to raise the awareness of this 
and see what others think about this.
    
    Also, note that this thread code path is by default (and the only) code 
path for QuorumCnxManager connection handling, regardless if users enable 
server to server auth or not. I am wondering should we have another code path 
so users who don't enable server to server auth will get exactly the old 
behavior. In other words, when server to server auth is not enabled, we will be 
very confident that our change will not introduce any regressions.


- Michael Han


On Sept. 15, 2016, 4:48 p.m., Rakesh R wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47354/
> -----------------------------------------------------------
> 
> (Updated Sept. 15, 2016, 4:48 p.m.)
> 
> 
> Review request for zookeeper, fpj, Ivan Kelly, Patrick Hunt, and Raul 
> Gutierrez Segales.
> 
> 
> Bugs: ZOOKEEPER-1045
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1045
> 
> 
> Repository: zookeeper-git
> 
> 
> Description
> -------
> 
> Quorum mutual authentication using SASL mechanism - Digest/Kerberos
> 
> 
> Diffs
> -----
> 
>   build.xml ee6834c 
>   ivy.xml 95b0e5a 
>   src/java/main/org/apache/zookeeper/Login.java aaa220c 
>   src/java/main/org/apache/zookeeper/SaslClientCallbackHandler.java 
> PRE-CREATION 
>   src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 21ef0fa 
>   src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java 71870ce 
>   
> src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java 
> 2fbd6ed 
>   src/java/main/org/apache/zookeeper/server/quorum/Leader.java c83d352 
>   src/java/main/org/apache/zookeeper/server/quorum/Learner.java 647b8a2 
>   src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java 
> 8a748c7 
>   src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java 
> 20e5f16 
>   src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 2f0f21b 
>   src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 
> 0924ef6 
>   src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java 
> e9c8007 
>   
> src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthLearner.java
>  PRE-CREATION 
>   
> src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthServer.java
>  PRE-CREATION 
>   src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java 
> PRE-CREATION 
>   
> src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthLearner.java 
> PRE-CREATION 
>   src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthServer.java 
> PRE-CREATION 
>   src/java/main/org/apache/zookeeper/server/quorum/auth/README.md 
> PRE-CREATION 
>   
> src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java
>  PRE-CREATION 
>   
> src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java
>  PRE-CREATION 
>   
> src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumServerCallbackHandler.java
>  PRE-CREATION 
>   src/java/main/org/apache/zookeeper/util/SecurityUtils.java PRE-CREATION 
>   src/java/test/data/kerberos/minikdc-krb5.conf PRE-CREATION 
>   src/java/test/data/kerberos/minikdc.ldiff PRE-CREATION 
>   src/java/test/org/apache/zookeeper/server/quorum/CnxManagerTest.java 
> 8db7fa8 
>   
> src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java
>  c1259d1 
>   src/java/test/org/apache/zookeeper/server/quorum/FLECompatibilityTest.java 
> 72e4fc9 
>   src/java/test/org/apache/zookeeper/server/quorum/FLEDontCareTest.java 
> a4c0cb0 
>   src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java 
> 39a53ca 
>   src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 2ae57ce 
>   src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java 
> PRE-CREATION 
>   src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java 
> 85817b2 
>   src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java ab8ce42 
>   
> src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosSecurityTestcase.java
>  PRE-CREATION 
>   
> src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosTestUtils.java 
> PRE-CREATION 
>   src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdc.java 
> PRE-CREATION 
>   src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdcTest.java 
> PRE-CREATION 
>   
> src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java 
> PRE-CREATION 
>   
> src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthUpgradeTest.java
>  PRE-CREATION 
>   
> src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumDigestAuthTest.java
>  PRE-CREATION 
>   
> src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumKerberosAuthTest.java
>  PRE-CREATION 
>   
> src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumKerberosHostBasedAuthTest.java
>  PRE-CREATION 
>   src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 8088505 
>   src/zookeeper.jute 6521e54 
> 
> Diff: https://reviews.apache.org/r/47354/diff/
> 
> 
> Testing
> -------
> 
> Added unit test cases to verify the changes.
> 
> 
> Thanks,
> 
> Rakesh R
> 
>

Reply via email to