Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Michael Han]

---
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

[Michael Han]

---
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

[Michael Han]

---
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

[Michael Han]

> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/KeeperException.java, line 317
> > 
> >
> > 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 
>   

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Rakesh R]

> On Oct. 13, 2016, 9:57 p.m., Abraham Fine wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 1134
> > 
> >
> > 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 

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Rakesh R]

---
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 
>   

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Rakesh R]

---
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)


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 
>   

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Rakesh R]

> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/KeeperException.java, line 317
> > 
> >
> > 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 
>   
> 

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Rakesh R]

---
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)


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)


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)


Please use {} instead of string concatenation.



src/java/test/org/apache/zookeeper/test/ReconfigExceptionTest.java (line 110)


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 
>   

RE: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Kanniappan, Mohanarangan (Nokia - IN/Bangalore)]

Anyone can guide me to unsubscribe ?

-Original Message-
From: Michael Han [mailto:nore...@reviews.apache.org] On Behalf Of Michael Han
Sent: Tuesday, October 18, 2016 10:46 AM
To: Raul Gutierrez Segales ; Patrick Hunt 
; f...@apache.org; Alexander Shraer 
Cc: Abraham Fine ; zookeeper ; 
Edward Ribeiro ; Rakesh R 
; Michael Han 
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 
  

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Michael Han]

---
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 

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Michael Han]

> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 
>   

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Abraham Fine]

---
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)


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 


whitespace change?



src/java/main/org/apache/zookeeper/cli/CliCommand.java (line 23)


unused import?



src/java/main/org/apache/zookeeper/cli/CliCommand.java (line 65)


can this file be reverted instead of being part of this patch?



src/java/main/org/apache/zookeeper/cli/ReconfigCommand.java (line 148)


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)


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)


not sure how this change relates to the purpose of the patch



src/java/test/org/apache/zookeeper/server/quorum/ReconfigBackupTest.java (line 
66)


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)


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 
>   

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Michael Han]

---
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

[Michael Han]

---
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)


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

[Michael Han]

---
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

[Michael Han]

---
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

[Michael Han]

> 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
> > 
> >
> > 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
> > 
> >
> > "... 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > "... 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
> > 
> >
> > "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
> > 
> >
> > "... 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
> > 
> >
> > "... 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
> > 
> >
> > "... only the super user has ..."

Updated.


> On Oct. 2, 

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Michael Han]

---
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

[fpj]

---
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)


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)


"... only admin / users that have permission can do reconfig."



src/c/tests/TestReconfigServer.cc (line 381)


Why do we need this sleep here?



src/c/tests/TestReconfigServer.cc (line 385)


and here?



src/c/tests/ZooKeeperQuorumServer.cc (line 29)


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)


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)


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)


"... the ability to change..." instead?



src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 322)


"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)


"... interact with a ZooKeeper ensemble..."



src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 338)


"... 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)


"... only the super user has ..."



src/java/main/org/apache/zookeeper/ClientCnxn.java (line 1526)


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)


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)


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)


Could you break the line, please?



src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 169)


White spaces.



src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 177)


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:

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Michael Han]

---
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

[Michael Han]

---
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

[Michael Han]

---
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

[Michael Han]

> On Sept. 27, 2016, 6:37 p.m., Rakesh R wrote:
> > src/java/main/org/apache/zookeeper/KeeperException.java, line 317
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > Its good practise to add timeout in tests. may be smaller value can be 
> > given.
> > 
> > @Test(timeout = 1)

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 
>   

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Rakesh R]

---
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)


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)


Please add class level javadoc. Thanks!



src/java/main/org/apache/zookeeper/admin/ZooKeeperAdmin.java (line 46)


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)


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)


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)


Its good practise to add timeout in tests. may be smaller value can be 
given.

@Test(timeout = 1)


- 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 

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Michael Han]

---
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

[Michael Han]

> On Sept. 24, 2016, 3:02 p.m., fpj wrote:
> > src/c/tests/TestReconfigServer.cc, line 78
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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:
> > 

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[fpj]

---
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)


Is this saying that we don't need to test ACLs with reconfig for the C 
client?



src/c/tests/TestReconfigServer.cc (line 80)


To confirm, setting skipACL to true isn't strictly necessary here, right?



src/c/tests/ZooKeeperQuorumServer.cc (line 105)


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)


Should we actually explain what enabling/disabling reconfig entails?



src/docs/src/documentation/content/xdocs/zookeeperReconfig.xml (line 152)


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)


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)


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)


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)


Should this class be in a separate zookeeper.admin package?



src/java/main/org/apache/zookeeper/ZooKeeperAdmin.java (line 129)


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)


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)


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)


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
> 
> 
> 

Re: Review Request 51546: ZOOKEEPER-2014: Only admin should be allowed to reconfig a cluster

[Edward Ribeiro]

---
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)


It would be nice to remove these Tab(?) spaces in this file.



src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java (line 
746)


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

[Michael Han]

---
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