> On Aug. 1, 2012, 6:38 p.m., Patrick Hunt wrote: > > /src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedServerTest.java, > > line 77 > > <https://reviews.apache.org/r/6290/diff/1/?file=132325#file132325line77> > > > > why is this sleep here? > > Eugene Koontz wrote: > These sleep()s are present in other SASL-related tests (for example > SaslAuthTest) as a work-around for ZOOKEEPER-1437: without the sleep()s, the > client may be denied permission because the SASL authentication process has > not completed yet. In the patch for ZOOKEEPER-1437, these sleeps are removed, > and thus could be removed also here, too. > > Patrick Hunt wrote: > Ah. Thanks Eugene. However given we know this is a SASL connection can't > we just look for the KeeperState.SaslAuthenticated to come to the watcher? > > Eugene Koontz wrote: > Sure, that should work. Perhaps I should file a new JIRA for removing the > various sleep()s in the existing tests (and mentioning this JIRA too), and > replace with watches? > > Patrick Hunt wrote: > Sounds good. Although we should have a specific test that exercises this > case for ZOOKEEPER-1437, such a test is included in 1437? > > Eugene Koontz wrote: > The latest patch for ZOOKEEPER-1437 simply remove the sleep()s from the > tests but do not do not add any watches for the SaslAuthenticated event. So I > will add watches in a new patch for ZK-1437. > >
Eugene my previous comment about 1437, what I meant was that 1437 should exercise the code path for the case where we don't wait (your comment about having to have the sleeps in the current code). Seems like 1437 should have both test cases, 1) run the command w/o waiting, 2) wait for the watch and verify that code path works as well. - Patrick ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6290/#review9703 ----------------------------------------------------------- On Aug. 1, 2012, 8:33 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, 8:33 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/Environment.java 1368201 > /src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 1368201 > /src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java > 1368201 > /src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java > 1368201 > /src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 1368201 > /src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java 1368201 > > /src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java > 1368201 > /src/java/test/org/apache/zookeeper/JaasConfiguration.java PRE-CREATION > /src/java/test/org/apache/zookeeper/test/ClientBase.java 1368201 > /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 > >
