> On Sept. 20, 2016, 10:24 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java, > > line 77 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498971#file1498971line77> > > > > How about test if 'quorumRequireSasl' flag is set or not, and if it's > > not set, immediately return here? By doing that we can remove a couple of > > 'if (quorumRequireSasl) {' conditions in the subsequent code.
Please refer QuorumAuthUpgradeTest.java, there is a step while upgrade. To support this, we need to keep the 'if (quorumRequireSasl)' inside another 'if (!QuorumAuth.nextPacketIsAuth(din))'. We have couple of test cases verifying this behavior. Please refer, QuorumAuthUpgradeTest#testRollingUpgrade, QuorumCnxManagerTest.java test cases. Please let me know if I miss any specific, I'm happy to include. QuorumAuthUpgradeTest.java * step-2) Stop the server one by one and change the flags and restart the server. * quorum.auth.enableSasl=true, quorum.auth.learnerRequireSasl=true and quorum.auth.serverRequireSasl=false * Ensure that all the servers should complete this step. Now, move to next step. > On Sept. 20, 2016, 10:24 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java, > > line 174 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498970#file1498970line174> > > > > The implementation of this is an almost identical copy of > > ZooKeeperSaslClient.createSaslToken (minors one logging changes). I suggest > > we add a TODO here about consolidating the createSaslToken implementation > > between ZooKeeperSaslClient and here , so we can refactor the code after > > the patch landed. Agreed, I will add a TODO. > On Sept. 20, 2016, 10:24 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java, > > line 131 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498970#file1498970line131> > > > > This 'if' condition can be removed, because when we can't reach here > > unless the auth process finished successfully, which means we don't hit any > > exceptions plus the sc.isComplete would evaluate to true (otherwise we > > would not get out of the while loop.). Good catch. Please refer https://docs.oracle.com/javase/7/docs/api/javax/security/sasl/SaslClient.html javadoc. In example, they have an additional if check after the looping. I think I need to include additional check for ERROR after the loop, right? like below, if (sc.isComplete()) { if(qpStatus == QuorumAuth.Status.SUCCESS) LOG.info("Successfully completed the authentication using SASL. server addr: {}, status: {}", sock.getRemoteSocketAddress(), qpStatus); else { throw new SaslException( "Authentication failed against server addr: " + sock.getRemoteSocketAddress() + ", qpStatus: " + qpStatus); } } > On Sept. 20, 2016, 10:24 p.m., Michael Han wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 106 > > <https://reviews.apache.org/r/47354/diff/9/?file=1498961#file1498961line106> > > > > Should we move both ThreadPoolExecutor and inprogressConnections into > > QuorumCnxManager? Both are implementation details around managing > > connections, and QuorumPeer does not need to know about these details. > > Abstract such details seems exactly what QuorumCnxManager was designed for. Agreed, I will change it. - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47354/#review149705 ----------------------------------------------------------- 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 > >
