----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6290/#review9703 -----------------------------------------------------------
Overall looks good. Some comments: /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/6290/#comment20673> Can we update the code to make "Server" a constant as well? and reuse where "Server" is referenced in the sasl code. /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/6290/#comment20674> make this a constant as well. /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/6290/#comment20675> our coding guidelines are to make this a {} block and not single line cond/stmt. /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/6290/#comment20676> substitute the actual config key string rather than the internal constant field name. /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java <https://reviews.apache.org/r/6290/#comment20678> please ref ZooKeeper as ZooKeeper and not Zookeeper. /src/java/test/org/apache/zookeeper/JaasConfiguration.java <https://reviews.apache.org/r/6290/#comment20679> some javadoc for the class and public methods would be good to add. /src/java/test/org/apache/zookeeper/JaasConfiguration.java <https://reviews.apache.org/r/6290/#comment20680> a small comment on this would be useful (the implicit pairs?) /src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedServerTest.java <https://reviews.apache.org/r/6290/#comment20681> javadoc describing the test would be useful here. /src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedServerTest.java <https://reviews.apache.org/r/6290/#comment20683> why is this sleep here? /src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedServerTest.java <https://reviews.apache.org/r/6290/#comment20682> why is this sleep here? - Patrick Hunt On Aug. 1, 2012, 6:20 p.m., Matteo Bertozzi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6290/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2012, 6:20 p.m.) > > > Review request for zookeeper, Patrick Hunt and Eugene Koontz. > > > Description > ------- > > Currently the CnxnFactory checks for "java.security.auth.login.config" to > decide whether or not enable SASL. > - zookeeper/server/NIOServerCnxnFactory.java > - zookeeper/server/NettyServerCnxnFactory.java > - configure() checks for "java.security.auth.login.config" > - If present start the new Login("Server", > SaslServerCallbackHandler(conf)) > > But since the SaslServerCallbackHandler does the right thing just checking if > getAppConfigurationEntry() is empty, we can allow SASL with JAAS > configuration to be programmatically just checking weather or not a > configuration entry is present instead of "java.security.auth.login.config". > (Something quite similar was done for the SaslClient in ZOOKEEPER-1373) > > > This addresses bug ZOOKEEPER-1497. > https://issues.apache.org/jira/browse/ZOOKEEPER-1497 > > > Diffs > ----- > > /src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java > 1360369 > /src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 1360369 > /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1360369 > /src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java 1360369 > > /src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java > 1360369 > /src/java/test/org/apache/zookeeper/JaasConfiguration.java PRE-CREATION > /src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedServerTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/6290/diff/ > > > Testing > ------- > > New testcase added SaslAuthDesignatedServerTest to check if > ZooKeeperSaslServer.LOGIN_CONTEXT_NAME_KEY is used. > (A new JaasConfiguration class was added to wrap the jaas.conf) > > +Manual testing for HBASE-4791 > > > Thanks, > > Matteo Bertozzi > >
