----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51546/#review151129 -----------------------------------------------------------
It is taking shape, thanks for the updates, Michael. I've left some more comments when you have a minute. src/c/tests/TestReconfigServer.cc (line 80) <https://reviews.apache.org/r/51546/#comment219278> We are setting up a cluster with a super password. Is it worth testing the behavior for a cluster without the super digest set? I'm actually assuming that we will warn the user and not let reconfig happen in the case the user forgets to set the super password. src/c/tests/TestReconfigServer.cc (line 342) <https://reviews.apache.org/r/51546/#comment219275> "... only admin / users that have permission can do reconfig." src/c/tests/TestReconfigServer.cc (line 381) <https://reviews.apache.org/r/51546/#comment219276> Why do we need this sleep here? src/c/tests/TestReconfigServer.cc (line 385) <https://reviews.apache.org/r/51546/#comment219277> and here? src/c/tests/ZooKeeperQuorumServer.cc (line 29) <https://reviews.apache.org/r/51546/#comment219279> Minor: why is it configs (plural) rather than just config? it is just a single configuration, yes? src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 944) <https://reviews.apache.org/r/51546/#comment219280> This is great, here is a slightly modified version: _When the feature is enabled, users can perform reconfigure operations through the ZooKeeper client API or through the ZooKeeper command line tools assuming users are authorized to perform such operations. When the feature is disabled, no user, including the super user, can perform a reconfiguration. Any attempt to reconfigure will return an error._ src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 951) <https://reviews.apache.org/r/51546/#comment219281> What happens if this is set inconsistently? If one server sets it and another doesn't, then what is the expected behavior? Shall we document it? src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 321) <https://reviews.apache.org/r/51546/#comment219282> "... the ability to change..." instead? src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 322) <https://reviews.apache.org/r/51546/#comment219283> "It is thus possible for a malicious client to make arbitrary changes to an ensemble, e.g., add a compromised server, remove legitimate servers." instead? src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 331) <https://reviews.apache.org/r/51546/#comment219284> "... interact with a ZooKeeper ensemble..." src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 338) <https://reviews.apache.org/r/51546/#comment219285> "... allow a user to configure different levels..." Also, it is not really clear what are the different levels that this is referring to. Could you clarify, please? src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 353) <https://reviews.apache.org/r/51546/#comment219286> "... only the super user has ..." src/java/main/org/apache/zookeeper/ClientCnxn.java (line 1526) <https://reviews.apache.org/r/51546/#comment219287> It is a bit odd that we are making this public. I suppose this is for tests, so I'm wondering if we can extend the class in the tests to avoid making the method public. src/java/main/org/apache/zookeeper/ZooKeeperMain.java (line 88) <https://reviews.apache.org/r/51546/#comment219290> If `ZooKeeperAdmin` extends `ZooKeeper`, why do we need both a `ZooKeeper` object and a `ZooKeeperAdmin` object here? src/java/main/org/apache/zookeeper/ZooKeeperMain.java (line 281) <https://reviews.apache.org/r/51546/#comment219288> if we close the zk handle three lines above, then it is possible that `zk.getState().isAlive()` is false, but zkAdmin is true, no? I think this code should be something like: if (zk.getState().isAlive()) { if (zk != null) { zk.close(); } if (zkAdmin != null) { zkAdmin.close(); } } src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 127) <https://reviews.apache.org/r/51546/#comment219291> Could you break the line, please? src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 169) <https://reviews.apache.org/r/51546/#comment219289> White spaces. src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 177) <https://reviews.apache.org/r/51546/#comment219292> Is access to ClientCnxn the main reason for extending `ZooKeeper`? - fpj On Sept. 29, 2016, 1:41 a.m., Michael Han wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51546/ > ----------------------------------------------------------- > > (Updated Sept. 29, 2016, 1:41 a.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 96c9ef8 > 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 f729095 > 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 f2ec3d7 > 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 06c80d3 > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java 33f638d > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > e772fa8 > 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/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 > >
