You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Michael Han <ha...@cloudera.com> on 2016/09/01 16:24:20 UTC
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
-----------------------------------------------------------
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.
Changes
-------
Fix find bug warnings.
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
> On Sept. 20, 2016, 7:04 p.m., Edward Ribeiro wrote:
> > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 746
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490156#file1490156line746>
> >
> > Okay, I am gonna put the old grumpy java coder hat here, sorry. :)
> >
> > When we write a Java method that returns a boolean is a widely used practice to name it boolean isXXX(). So it would be nice to rename this method to:
> >
> > ```
> > isReconfigEnabled()
> > ```
Thanks will rename in next batch update.
> On Sept. 20, 2016, 7:04 p.m., Edward Ribeiro wrote:
> > src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java, line 152
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490150#file1490150line152>
> >
> > It would be nice to remove these Tab(?) spaces in this file.
Thanks, will do (tabs were unfortunately inherited from ZooKeeper.java) in next batch update.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review149706
-----------------------------------------------------------
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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Edward Ribeiro <ed...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review149706
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java (line 152)
<https://reviews.apache.org/r/51546/#comment217397>
It would be nice to remove these Tab(?) spaces in this file.
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java (line 746)
<https://reviews.apache.org/r/51546/#comment217392>
Okay, I am gonna put the old grumpy java coder hat here, sorry. :)
When we write a Java method that returns a boolean is a widely used practice to name it boolean isXXX(). So it would be nice to rename this method to:
```
isReconfigEnabled()
```
- Edward Ribeiro
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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
> 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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by fp...@apache.org.
-----------------------------------------------------------
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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review152214
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ZooKeeper.java (line 224)
<https://reviews.apache.org/r/51546/#comment221074>
I have to make these public so they can be accessed in ZooKeeperAdmin (which is in a different package than ZooKeeper.).
If not doing this, then we can't reuse the existing test infrastructure (the TestableZooKeeper and createClient and so on), and the change would be a lot more intrusive and tedious.
- Michael Han
On Oct. 11, 2016, 9:32 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, 9:32 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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
> 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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Rakesh R <ra...@intel.com>.
> 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?
>
> Michael Han wrote:
> 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.
Thanks Abraham for bringing out this point. I'm OK with the clear documentation and the present explanation about the "inconsistent behabior" looks good to me.
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review152577
-----------------------------------------------------------
On Oct. 31, 2016, 4:31 a.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 4:31 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 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 c39395a
> 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/NIOServerCnxnFactory.java 7a72757
> src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
> src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
> src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
> src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
> src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
> src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
> src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
> src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
> src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
> src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
> src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
> src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
> src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
> src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
> src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
> src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
> src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
> src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
> 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/server/quorum/Zab1_0Test.java 6a01447
> src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
> src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
> src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
> src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
> src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
> src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
> src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
> 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
> -------
>
>
> Thanks,
>
> Michael Han
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Abraham Fine <ab...@abrahamfine.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review152577
-----------------------------------------------------------
src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 1134)
<https://reviews.apache.org/r/51546/#comment221657>
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?
src/java/main/org/apache/zookeeper/KeeperException.java
<https://reviews.apache.org/r/51546/#comment221640>
whitespace change?
src/java/main/org/apache/zookeeper/cli/CliCommand.java (line 23)
<https://reviews.apache.org/r/51546/#comment221636>
unused import?
src/java/main/org/apache/zookeeper/cli/CliCommand.java (line 65)
<https://reviews.apache.org/r/51546/#comment221637>
can this file be reverted instead of being part of this patch?
src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java (line 148)
<https://reviews.apache.org/r/51546/#comment221651>
This would need to be replicated on future commands that live in ZooKeeperAdmin.
Perhaps it would be useful to subclass CliCommand with CliAdminCommand for commands that need ZooKeeperAdmin.
src/java/main/org/apache/zookeeper/server/DataTree.java (line 259)
<https://reviews.apache.org/r/51546/#comment221639>
if setting the acl throws an exception would the server continue starting with incorrect permissioning on the config znode?
src/java/test/org/apache/zookeeper/server/DataTreeTest.java (line 201)
<https://reviews.apache.org/r/51546/#comment221643>
not sure how this change relates to the purpose of the patch
src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java (line 66)
<https://reviews.apache.org/r/51546/#comment221650>
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?
src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java (line 201)
<https://reviews.apache.org/r/51546/#comment221652>
this method is shared with ReconfigMisconfigTest so perhaps it could be moved to ZkTestCase?
- Abraham Fine
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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Rakesh R <ra...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review154230
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/ZooKeeper.java (line 224)
<https://reviews.apache.org/r/51546/#comment223725>
I think, it is not required to increase the visibility to 'public', instead this could be 'protected', right?
Similarly there are few more visibility changes to functions, variables etc. Could you please revisit all these and do approriate changes to the visibility.
src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 47)
<https://reviews.apache.org/r/51546/#comment223730>
Please add "@since 3.5.3" at the class level javadoc, that would convey the timeline clearly.
src/java/main/org/apache/zookeeper/server/DataTree.java (line 261)
<https://reviews.apache.org/r/51546/#comment223722>
Please use {} instead of string concatenation.
src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java (line 110)
<https://reviews.apache.org/r/51546/#comment223727>
Many places its using deprecated APIs. Please visit all such occurances and replace with new API.
Assert.assertTrue(e.getCode() == KeeperException.Code.NoAuth);
replace with
Assert.assertTrue(e.code() == KeeperException.Code.NOAUTH);
- Rakesh R
On Oct. 18, 2016, 5:15 a.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2016, 5:15 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 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/NIOServerCnxnFactory.java 7a72757
> src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
> src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
> src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
> src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
> src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
> src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
> src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
> src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
> src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
> src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
> src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
> src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
> src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
> src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
> src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
> src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
> src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
> src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
> 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/server/quorum/Zab1_0Test.java 6a01447
> src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
> src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
> src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
> src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
> src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
> src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
> src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
> 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
> -------
>
>
> Thanks,
>
> Michael Han
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Rakesh R <ra...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review154254
-----------------------------------------------------------
src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml (line 1134)
<https://reviews.apache.org/r/51546/#comment223757>
Thanks Abraham for bringing out this point. I'm OK with the clear documentation and the present explanation about the "inconsistent behabior" looks good to me.
- Rakesh R
On Oct. 31, 2016, 4:31 a.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 4:31 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 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 c39395a
> 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/NIOServerCnxnFactory.java 7a72757
> src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
> src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
> src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
> src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
> src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
> src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
> src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
> src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
> src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
> src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
> src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
> src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
> src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
> src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
> src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
> src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
> src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
> src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
> 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/server/quorum/Zab1_0Test.java 6a01447
> src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
> src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
> src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
> src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
> src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
> src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
> src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
> 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
> -------
>
>
> Thanks,
>
> Michael Han
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Nov. 6, 2016, 7:02 a.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Update patch to address review comments from Ben and Raul.
Note - patch on JIRA and GIT PR is updated too, for the record.
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 (updated)
-----
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 c39395a
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/ZooKeeperServer.java d4f150b
src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 24a4ec1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
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/LearnerTest.java 4debe74
src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
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 37bd8e4
src/java/test/org/apache/zookeeper/server/quorum/StandaloneDisabledTest.java 1f6ce1f
src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 6a01447
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
-------
Thanks,
Michael Han
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Nov. 4, 2016, 4:48 p.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Two junk files were included in previous diff. Updated the patch to remove two junk files (JIRA patch and GIT PR is updated as well.).
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 (updated)
-----
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 c39395a
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/ZooKeeperServer.java d4f150b
src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 24a4ec1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
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/LearnerTest.java 4debe74
src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
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 37bd8e4
src/java/test/org/apache/zookeeper/server/quorum/StandaloneDisabledTest.java 1f6ce1f
src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 6a01447
src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
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
-------
Thanks,
Michael Han
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Nov. 4, 2016, 4:32 p.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Address Ben's comment by replacing KeeperException.NoNodeException with RuntimeException in addConfigNode to avoid changing function signatures across code base.
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 (updated)
-----
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 c39395a
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/ZooKeeperServer.java d4f150b
src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java 24a4ec1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java.orig PRE-CREATION
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
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/LearnerTest.java 4debe74
src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
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 37bd8e4
src/java/test/org/apache/zookeeper/server/quorum/ReconfigLegacyTest.java.orig PRE-CREATION
src/java/test/org/apache/zookeeper/server/quorum/StandaloneDisabledTest.java 1f6ce1f
src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 6a01447
src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
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
-------
Thanks,
Michael Han
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Rakesh R <ra...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review154255
-----------------------------------------------------------
Ship it!
Ship It!
- Rakesh R
On Oct. 31, 2016, 4:31 a.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 4:31 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 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 c39395a
> 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/NIOServerCnxnFactory.java 7a72757
> src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
> src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
> src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
> src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
> src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
> src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
> src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
> src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
> src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
> src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
> src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
> src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
> src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
> src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
> src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
> src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
> src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
> src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
> 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/server/quorum/Zab1_0Test.java 6a01447
> src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
> src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
> src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
> src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
> src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
> src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
> src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
> 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
> -------
>
>
> Thanks,
>
> Michael Han
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Oct. 31, 2016, 4:31 a.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Update patch to address Rakesh's code review comments.
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 (updated)
-----
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 c39395a
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/NIOServerCnxnFactory.java 7a72757
src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
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/server/quorum/Zab1_0Test.java 6a01447
src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
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
-------
Thanks,
Michael Han
RE: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by "Kanniappan, Mohanarangan (Nokia - IN/Bangalore)" <mo...@nokia.com>.
Anyone can guide me to unsubscribe ?
-----Original Message-----
From: Michael Han [mailto:noreply@reviews.apache.org] On Behalf Of Michael Han
Sent: Tuesday, October 18, 2016 10:46 AM
To: Raul Gutierrez Segales <rg...@itevenworks.net>; Patrick Hunt <ph...@apache.org>; fpj@apache.org; Alexander Shraer <sh...@yahoo-inc.com>
Cc: Abraham Fine <ab...@abrahamfine.com>; zookeeper <de...@zookeeper.apache.org>; Edward Ribeiro <ed...@gmail.com>; Rakesh R <ra...@intel.com>; Michael Han <ha...@cloudera.com>
Subject: Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Oct. 18, 2016, 5:15 a.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Upload new patch to address Abe's review comments.
Don't swallow the KeeperException when setting ACL on the config node during DataTree creation, instead let the exception bubble up to stop ZK server startup. This is to make sure we always have access controlled config node after ZK server is started.
The rest of changes are really mechanical changes due to the exception specification changes in signature.
Passed internal stress test of all unit tests.
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 (updated)
-----
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/NIOServerCnxnFactory.java 7a72757
src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
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/server/quorum/Zab1_0Test.java 6a01447
src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
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 (updated)
-------
Thanks,
Michael Han
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Oct. 18, 2016, 5:15 a.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Upload new patch to address Abe's review comments.
Don't swallow the KeeperException when setting ACL on the config node during DataTree creation, instead let the exception bubble up to stop ZK server startup. This is to make sure we always have access controlled config node after ZK server is started.
The rest of changes are really mechanical changes due to the exception specification changes in signature.
Passed internal stress test of all unit tests.
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 (updated)
-----
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/NIOServerCnxnFactory.java 7a72757
src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
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/server/quorum/Zab1_0Test.java 6a01447
src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
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 (updated)
-------
Thanks,
Michael Han
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Fix findbug warnings.
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Oct. 11, 2016, 9:32 p.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Patch updated:
1. Consolidate ZooKeeper and ZooKeeperAdmin object usage in CliCommand stack.
2. Fix confusing KeeperException error message when path is set as empty.
3. More documentation on having a consistent setting for the reconfigEnabled option across ensemble.
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Oct. 10, 2016, 9:41 p.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Update patch with a one line change that fixes reconfig command NoAuth failure when doing reconfig through command line.
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 (updated)
-----
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/AddAuthCommand.java e2a333a
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
Update patch that fixes a couple of places pointed by Flavio:
- Add test case to test reconfig should fail when server does not have the super user's password configured.
- A couple of documentation updates.
- Misc small fixes and clean ups (naming, remove debug code, etc.).
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
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.
Changes
-------
A small fix to previous diff - the new C reconfig test testReconfigFailureWithoutAuth was added but not enabled.
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Sept. 28, 2016, 11:39 p.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Update patch that includes C client side test update.
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Sept. 27, 2016, 10:39 p.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Update patch based on review feedback from Rakesh. C client update is still WIP.
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/KeeperException.java, line 317
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510450#file1510450line317>
> >
> > Do we need @Deprecated constant value? Since we are in 3.5 branch, I feel its not required.
> >
> > Secondly, we can't we use -122?
>
> Michael Han wrote:
> Thanks for all the reviews, Rakesh!
>
> We need deprecated here but I see what you mean - instead of adding a deprecated value I can just skip adding it.
>
> -122 is used in zookeeper.h as ZRWSERVERFOUND... this maybe a bug as there is no corresponding code in Java client. I'd like to deal with this later so skip -122 and choose next value -123.
>
> Rakesh R wrote:
> How about raising a tracking jira to revisit this part. Thanks!
Just for the record, rased ZOOKEEPER-2627 to track this part.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review150538
-----------------------------------------------------------
On Oct. 31, 2016, 4:31 a.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Oct. 31, 2016, 4:31 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 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 c39395a
> 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/NIOServerCnxnFactory.java 7a72757
> src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
> src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
> src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
> src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
> src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
> src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
> src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
> src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
> src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
> src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
> src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
> src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
> src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
> src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
> src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
> src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
> src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
> src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
> 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/server/quorum/Zab1_0Test.java 6a01447
> src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
> src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
> src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
> src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
> src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
> src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
> src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
> 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
> -------
>
>
> Thanks,
>
> Michael Han
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/KeeperException.java, line 317
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510450#file1510450line317>
> >
> > Do we need @Deprecated constant value? Since we are in 3.5 branch, I feel its not required.
> >
> > Secondly, we can't we use -122?
Thanks for all the reviews, Rakesh!
We need deprecated here but I see what you mean - instead of adding a deprecated value I can just skip adding it.
-122 is used in zookeeper.h as ZRWSERVERFOUND... this maybe a bug as there is no corresponding code in Java client. I'd like to deal with this later so skip -122 and choose next value -123.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java, line 42
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510453#file1510453line42>
> >
> > Please add class level javadoc. Thanks!
Good catch, will update.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java, line 46
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510453#file1510453line46>
> >
> > I could see ZooKeeper.java has the static block printing "Client environment" details. I think it will simply duplicate the information, can we remove this static block. Also, move the LOG instantiation directly to the LOG variable.
Will fix.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 472
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510457#file1510457line472>
> >
> > Its throwing exception and failing the operation. Should we need an error log priority instead of warn?
Good one, I agree LOG.error is better and is consistent with existing behavior.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, line 291
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510458#file1510458line291>
> >
> > I agree its not followed previously in zookeeper code base. But I feel to print the configured value in case of invalid to improve debuggability, like below as we are adding new items and we have the chance now:-)
> >
> > throw new ConfigException("Invalid option:"+ value +" for reconfigEnabled flag. Choose 'true' or 'false.'");
Sounds good.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java, line 82
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510466#file1510466line82>
> >
> > Its good practise to add timeout in tests. may be smaller value can be given.
> >
> > @Test(timeout = 10000)
Will fix.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review150538
-----------------------------------------------------------
On Sept. 26, 2016, 10:48 p.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2016, 10:48 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/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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Rakesh R <ra...@intel.com>.
> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/KeeperException.java, line 317
> > <https://reviews.apache.org/r/51546/diff/3/?file=1510450#file1510450line317>
> >
> > Do we need @Deprecated constant value? Since we are in 3.5 branch, I feel its not required.
> >
> > Secondly, we can't we use -122?
>
> Michael Han wrote:
> Thanks for all the reviews, Rakesh!
>
> We need deprecated here but I see what you mean - instead of adding a deprecated value I can just skip adding it.
>
> -122 is used in zookeeper.h as ZRWSERVERFOUND... this maybe a bug as there is no corresponding code in Java client. I'd like to deal with this later so skip -122 and choose next value -123.
How about raising a tracking jira to revisit this part. Thanks!
- Rakesh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review150538
-----------------------------------------------------------
On Oct. 18, 2016, 5:15 a.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2016, 5:15 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 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/NIOServerCnxnFactory.java 7a72757
> src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java 25b682b
> src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java e275f9c
> src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java fc6766c
> src/java/main/org/apache/zookeeper/server/SnapshotFormatter.java f94c54d
> src/java/main/org/apache/zookeeper/server/ZKDatabase.java 05bbb91
> src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java d4f150b
> src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 885a5e1
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 68bef42
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java e772fa8
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java bfe8588
> src/java/systest/org/apache/zookeeper/test/system/BaseSysTest.java 109c1b5
> src/java/test/org/apache/zookeeper/TestableZooKeeper.java 4d46fdf
> src/java/test/org/apache/zookeeper/server/CRCTest.java da4ebaf
> src/java/test/org/apache/zookeeper/server/DataTreeTest.java d726643
> src/java/test/org/apache/zookeeper/server/SessionTrackerTest.java 87aa73e
> src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1030209
> src/java/test/org/apache/zookeeper/server/quorum/CommitProcessorTest.java c6ee230
> src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java cc44243
> src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java 95e5e53
> src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 4debe74
> src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTest.java fae7e5b
> src/java/test/org/apache/zookeeper/server/quorum/RaceConditionTest.java ef7f3df
> 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/server/quorum/Zab1_0Test.java 6a01447
> src/java/test/org/apache/zookeeper/test/ACLTest.java 9920fc4
> src/java/test/org/apache/zookeeper/test/ClientBase.java 309b2b2
> src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java a4244d8
> src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 2bbf7b5
> src/java/test/org/apache/zookeeper/test/QuorumBase.java f687f45
> src/java/test/org/apache/zookeeper/test/QuorumUtil.java 314171d
> src/java/test/org/apache/zookeeper/test/QuorumUtilTest.java 76e6df0
> 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
> -------
>
>
> Thanks,
>
> Michael Han
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Rakesh R <ra...@intel.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review150538
-----------------------------------------------------------
src/java/main/org/apache/zookeeper/KeeperException.java (line 317)
<https://reviews.apache.org/r/51546/#comment218584>
Do we need @Deprecated constant value? Since we are in 3.5 branch, I feel its not required.
Secondly, we can't we use -122?
src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 42)
<https://reviews.apache.org/r/51546/#comment218548>
Please add class level javadoc. Thanks!
src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 46)
<https://reviews.apache.org/r/51546/#comment218585>
I could see ZooKeeper.java has the static block printing "Client environment" details. I think it will simply duplicate the information, can we remove this static block. Also, move the LOG instantiation directly to the LOG variable.
src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java (line 472)
<https://reviews.apache.org/r/51546/#comment218587>
Its throwing exception and failing the operation. Should we need an error log priority instead of warn?
src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java (line 291)
<https://reviews.apache.org/r/51546/#comment218589>
I agree its not followed previously in zookeeper code base. But I feel to print the configured value in case of invalid to improve debuggability, like below as we are adding new items and we have the chance now:-)
throw new ConfigException("Invalid option:"+ value +" for reconfigEnabled flag. Choose 'true' or 'false.'");
src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java (line 82)
<https://reviews.apache.org/r/51546/#comment218623>
Its good practise to add timeout in tests. may be smaller value can be given.
@Test(timeout = 10000)
- Rakesh R
On Sept. 26, 2016, 10:48 p.m., Michael Han wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51546/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2016, 10:48 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/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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/
-----------------------------------------------------------
(Updated Sept. 26, 2016, 10:48 p.m.)
Review request for zookeeper, fpj, Patrick Hunt, Raul Gutierrez Segales, and Alexander Shraer.
Changes
-------
Update patch based on Flavio's and Edward's feedback. Note this is not the final patch as there are still some C client side work pending. Upload for the purpose of preview / feedback.
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 (updated)
-----
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
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by Michael Han <ha...@cloudera.com>.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/c/tests/TestReconfigServer.cc, line 78
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490144#file1490144line78>
> >
> > Is this saying that we don't need to test ACLs with reconfig for the C client?
Yes - These changes are pretty selfish ones just to make C test pass :)
On a second thought I think it is neccessary to add at least a minimum set of tests to check the sanity of reconfig with acl for C client. I'll add more C tests.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/c/tests/TestReconfigServer.cc, line 80
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490144#file1490144line80>
> >
> > To confirm, setting skipACL to true isn't strictly necessary here, right?
This option is required here because we did not set up auth for C client tests - so skipACL took the short cut and just let the test pass. As previously commented, I'll add more tests for C client.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/c/tests/ZooKeeperQuorumServer.cc, line 105
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490146#file1490146line105>
> >
> > 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.
Yes agree, the naming of the parameter is confusing. Better to name it as 'optionalConfig'.
Regarding the option of passing parameters explicitly vs the option of passing a string that encodes optional additional parameter, I think passing a string is more flexible but passing explicitly will be safer and more convenient to use; probably we can take an approach in the middle, for example we can pass a list of variadic key-value pairs and then genereate the config strings from the list, so we get better control over the final formatted config string while maintaining the flexibility of the interface.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 942
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490147#file1490147line942>
> >
> > Should we actually explain what enabling/disabling reconfig entails?
Sounds good idea, will add more details here.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 152
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490148#file1490148line152>
> >
> > 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."
Agreed.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 314
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490148#file1490148line314>
> >
> > 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.
Good point.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml, line 337
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490148#file1490148line337>
> >
> > The contract is that by default only the super user can change the configuration, yes?
Yes, superuser by default can do anything - if superuser is compromised, all bets are off I think.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java, line 19
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490150#file1490150line19>
> >
> > Should this class be in a separate zookeeper.admin package?
Agreed.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java, line 129
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490150#file1490150line129>
> >
> > Please remove all red (spaces and tabs) from this file. This is mostly if not all from copying it from ZooKeeper.java.
Yes will do - same problem was pointed out by Edward as well.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/server/DataTree.java, line 249
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490154#file1490154line249>
> >
> > 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.
Very good point - when skipACL is set we will allow reconfig proceed from any user. This should at least be documented.
Regarding the behavior of both reconfig enabled and skipACL is set, I feel gave a warning to user might be better than not starting the server, because that provides an escape hatch for users who fully control their environment and want reconfig, but don't want to go through the process of configure the credentials and ACLs. It's up to user to decide what to do by giving them all options and risks at different level, which is more flexible.
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java, line 473
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490155#file1490155line473>
> >
> > 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?
Yes, definitely makes sense. I actually thought about this initially and ends up reusing this code as I've seen some existing cases where we use this KeeperException (those are separate problems to fix.).
> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java, line 161
> > <https://reviews.apache.org/r/51546/diff/2/?file=1490163#file1490163line161>
> >
> > This is doing more than just resetting zk admin, no?
Yeah it needs a better name.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51546/#review150301
-----------------------------------------------------------
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
>
>
Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be
allowed to reconfig a cluster
Posted by fp...@apache.org.
-----------------------------------------------------------
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
>
>