> On Oct. 13, 2016, 9:57 p.m., Abraham Fine wrote: > >
Thanks for the review Abe. I'll post updated patch after consolidating feedback from Flavio regarding my last patch update. > On Oct. 13, 2016, 9:57 p.m., Abraham Fine wrote: > > src/java/main/org/apache/zookeeper/server/DataTree.java, line 259 > > <https://reviews.apache.org/r/51546/diff/10/?file=1531339#file1531339line259> > > > > if setting the acl throws an exception would the server continue > > starting with incorrect permissioning on the config znode? Chances are rare this will fail but good point on bringing it up - as it would be security issue if we allow server starts with incorrect acls on config node. I updated code to let the exception bubble up so ZooKeeperMain will catch it and declare a failure when start. > On Oct. 13, 2016, 9:57 p.m., Abraham Fine wrote: > > src/java/test/org/apache/zookeeper/server/DataTreeTest.java, line 203 > > <https://reviews.apache.org/r/51546/diff/10/?file=1531343#file1531343line203> > > > > not sure how this change relates to the purpose of the patch Will add a comment in test code. > On Oct. 13, 2016, 9:57 p.m., Abraham Fine wrote: > > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java, > > line 66 > > <https://reviews.apache.org/r/51546/diff/10/?file=1531344#file1531344line66> > > > > is there some sort of method we could call to generate the digest > > instead of inserting these as magic strings? it may make it easier to tell > > what is going on? DigestAuthenticationProvider can be used for this - though the password bits will never change given the same seed so it might be ok by just baking the password, to save some time on calling the digest calculator in the spirit of making test faster. Also note that existing auth test use baked digest too, so it's better to clean this up in a separate jira across entire code base if we decide to do so. > On Oct. 13, 2016, 9:57 p.m., Abraham Fine wrote: > > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 1134 > > <https://reviews.apache.org/r/51546/diff/10/?file=1531330#file1531330line1134> > > > > is there a way for us to sanity check this (make sure that their > > individual reconfigEnabled matches that of the leader) when servers join > > the quorum? I am not aware of any approach of doing the check - probably document it like this is enough. Looking forward hearing what others think about this. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51546/#review152577 ----------------------------------------------------------- On Oct. 11, 2016, 10:33 p.m., Michael Han wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51546/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2016, 10:33 p.m.) > > > Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and > Alexander Shraer. > > > Bugs: ZOOKEEPER-2014 > https://issues.apache.org/jira/browse/ZOOKEEPER-2014 > > > Repository: zookeeper-git > > > Description > ------- > > Address various security concerns around reconfig feature (ZOOKEEPER-2014) to > unblock 3.5.3 release. > > > Diffs > ----- > > build.xml 5328035 > src/c/include/zookeeper.h 18a203d > src/c/tests/TestReconfigServer.cc 6a429ac > src/c/tests/ZooKeeperQuorumServer.h aa8b7cc > src/c/tests/ZooKeeperQuorumServer.cc 23392cd > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 5a30da8 > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml 7168a01 > src/java/main/org/apache/zookeeper/ClientCnxn.java 12dd51c > src/java/main/org/apache/zookeeper/KeeperException.java a05f1ab > src/java/main/org/apache/zookeeper/ZooKeeper.java 1c9ed4e > src/java/main/org/apache/zookeeper/ZooKeeperMain.java 25d61a4 > src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java PRE-CREATION > src/java/main/org/apache/zookeeper/cli/CliCommand.java 3d0a90b > src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java deb7914 > src/java/main/org/apache/zookeeper/server/DataTree.java 4666578 > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > e772fa8 > src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf > src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643 > src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java > 241af52 > > src/java/test/org/apache/zookeeper/server/quorum/ReconfigDuringLeaderSyncTest.java > 301837d > > src/java/test/org/apache/zookeeper/server/quorum/ReconfigFailureCasesTest.java > e7147b3 > src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java > ee9f2e2 > > src/java/test/org/apache/zookeeper/server/quorum/StandaloneDisabledTest.java > 1f6ce1f > src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4 > src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/ReconfigMisconfigTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/ReconfigTest.java 248a754 > src/java/test/org/apache/zookeeper/test/StandaloneTest.java 5c95280 > > Diff: https://reviews.apache.org/r/51546/diff/ > > > Testing > ------- > > Added new Java unit tests that cover various exception cases of using > reconfig API without proper set up. > All existing tests (Java and C) passed under stress tests (minors those known > flaky tests.). > Manuelly tested reconfig commands using ZooKeeper command line tool. > > > Thanks, > > Michael Han > >
