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