> On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > It is taking shape, thanks for the updates, Michael. I've left some more > > comments when you have a minute.
Thanks a lot for your timely review feedback, Flavio. I've attached updated patch in both JIRA and review board. More comments below. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/c/tests/TestReconfigServer.cc, line 80 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514335#file1514335line80> > > > > 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. Good point, added new test for both Java and C client to test reconfig behavior when we don't start ensemble with the super user password. What user will get is reconfig failure, as expected, with error code 'NoAuth'. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/c/tests/TestReconfigServer.cc, line 342 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514335#file1514335line342> > > > > "... only admin / users that have permission can do reconfig." Updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/c/tests/TestReconfigServer.cc, line 381 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514335#file1514335line381> > > > > Why do we need this sleep here? Debug code forgot to remove - cleaned up in latest patch. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/c/tests/ZooKeeperQuorumServer.cc, line 29 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514337#file1514337line29> > > > > Minor: why is it configs (plural) rather than just config? it is just a > > single configuration, yes? I was thinking config contain multiple options, thus configs - however, I realized that is wrong level of abstraction (if that is the case, we should name it as 'configOptions' rather than 'configs'). So I've followed up your suggestion and update the naming to the singular form 'config'. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 944 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514338#file1514338line944> > > > > 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._ Updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 951 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514338#file1514338line951> > > > > 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? Great catch - I think this option should apply to cluster level rather than to individual server, right? So I've moved the documention to the 'cluster options' section. I hope this is enough so users will keep a consistent setting for reconfigureEnabled option across each server in the ensemble. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 321 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514339#file1514339line321> > > > > "... the ability to change..." instead? Updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 322 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514339#file1514339line322> > > > > "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? Updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 331 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514339#file1514339line331> > > > > "... interact with a ZooKeeper ensemble..." Updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 338 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514339#file1514339line338> > > > > "... 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? Updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 353 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514339#file1514339line353> > > > > "... only the super user has ..." Updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 1526 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514340#file1514340line1526> > > > > 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. This method needs to be public because ZooKeeperAdmin, which is in a different package (org.apache.zookeeper.admin) needs to access the method in reconfig API implementation. Previously this is not a problem as ZooKeeperAdmin was in the same package as ClientCnx and ZooKeeper (org.apache.zookeeper). I admit we should be cautious on making things public, though in this case seems we have to due to the access rules in java package. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ZooKeeperMain.java, line 88 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514343#file1514343line88> > > > > If `ZooKeeperAdmin` extends `ZooKeeper`, why do we need both a > > `ZooKeeper` object and a `ZooKeeperAdmin` object here? ZooKeeperAdmin object is for executing reconfig related commands (since ZooKeeper object does not have reconfig interfaces.). So we can't keep a single ZooKeeper object without a ZooKeeperAdmin object. We could keep a ZooKeeperAdmin object instead and remove ZooKeeper object, and have all commands executed by ZooKeeperAdmin object. I did not do that as I don't want to impact existing commands (separation of concern), but it seems not a big deal. Should we only keep a single ZooKeeperAdmin object here? I'd like to double check with you before I make the updates. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ZooKeeperMain.java, line 281 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514343#file1514343line281> > > > > 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(); > > } > > } Good point, updated. > On Oct. 2, 2016, 2:57 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java, line 177 > > <https://reviews.apache.org/r/51546/diff/6/?file=1514344#file1514344line177> > > > > Is access to ClientCnxn the main reason for extending `ZooKeeper`? The main reason is to utilize all the rich set of existing APIs that ZooKeeper interface has (such as watchers, addAuth, and so on) and to reuse implementation as much as possible. If we don't extend ZooKeeper then we'd have to duplicate lots of implementation. My feeling is ZooKeeperAdmin is a ZooKeeper client with more constraints (require auth to work), so extending seems naturual to me. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51546/#review151129 ----------------------------------------------------------- On Oct. 3, 2016, 11:52 p.m., Michael Han wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51546/ > ----------------------------------------------------------- > > (Updated Oct. 3, 2016, 11:52 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 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/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 > >
