----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51546/#review150301 -----------------------------------------------------------
Looks pretty good, Michael. I have left a few comments and questions when you have a minute. src/c/tests/TestReconfigServer.cc (line 78) <https://reviews.apache.org/r/51546/#comment218209> Is this saying that we don't need to test ACLs with reconfig for the C client? src/c/tests/TestReconfigServer.cc (line 80) <https://reviews.apache.org/r/51546/#comment218208> To confirm, setting skipACL to true isn't strictly necessary here, right? src/c/tests/ZooKeeperQuorumServer.cc (line 105) <https://reviews.apache.org/r/51546/#comment218210> The additional parameter adds optional parameters, so perhaps the name should reflect that it isn't a full config, but additional paramters instead. I'm wondering if instead of passing a string, which requires the caller to format it properly, we whould just pass the paramters explicitly. I don't feel strongly about it, but I wanted to run the idea by you. src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 942) <https://reviews.apache.org/r/51546/#comment218211> Should we actually explain what enabling/disabling reconfig entails? src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 152) <https://reviews.apache.org/r/51546/#comment218212> You may want to motivate why we would consider disabling the feature. Something along the lines of: "With reconfiguration enabled, we have a security concern that a malicious actor can make arbitrary changes to the configuration of a ZooKeeper ensemble, including adding a compromised server to the ensemble. We prefer to leave to the discretion of the user to decide whether to enable it or not and make sure that the appropriate security measure are in place." src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 314) <https://reviews.apache.org/r/51546/#comment218213> There is a bit of taste involved in this description. The need of security depends on lots of things and it is rarely black or white. I'd rather say like I suggested above that some users were concerned with the possibility of ensembles being arbitrarily reconfigured and we added the option of disabling it, so it is really up to the user to decide what he/she wants to do. src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 335) <https://reviews.apache.org/r/51546/#comment218214> This sentence is broken: "Clients that need to use reconfig commands or the reconfig API to change the state of a ZooKeeper ensemble should be configured as users that have write access to CONFIG_NODE." src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 337) <https://reviews.apache.org/r/51546/#comment218215> The contract is that by default only the super user can change the configuration, yes? src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java (line 19) <https://reviews.apache.org/r/51546/#comment218216> Should this class be in a separate zookeeper.admin package? src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java (line 129) <https://reviews.apache.org/r/51546/#comment218206> Please remove all red (spaces and tabs) from this file. This is mostly if not all from copying it from ZooKeeper.java. src/java/main/org/apache/zookeeper/server/DataTree.java (line 249) <https://reviews.apache.org/r/51546/#comment218217> You may additionally want to say in the comment that by default it is world readable, but not writable. I know it is documented elsewhere, but for emphasis. Actually, what happens if skipACL is set to true? Are we able to violate our contract in this case? We need to warn the user in the case reconfig and skipACL are both enabled, or possibly not even start the server. src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java (line 473) <https://reviews.apache.org/r/51546/#comment218207> This isn't really a case of bad argument, it is more like an unauthorized operation. does it make sense to create a new keeper exception for this? src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java (line 161) <https://reviews.apache.org/r/51546/#comment218219> This is doing more than just resetting zk admin, no? - fpj On Sept. 1, 2016, 4:24 p.m., Michael Han wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51546/ > ----------------------------------------------------------- > > (Updated Sept. 1, 2016, 4:24 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 4173550 > 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/ZooKeeper.java f2ec3d7 > src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeperMain.java 25d61a4 > 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/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 > >
