ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1373

Hardcoded SASL login context name clashes with Hadoop security configuration override

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.4.2
    • Fix Version/s: 3.4.3, 3.5.0
    • Component/s: java client
    • Labels:
      None

      Description

      I'm trying to configure a process with Hadoop security (Hive metastore server) to talk to ZooKeeper 3.4.2 with Kerberos authentication. In this scenario Hadoop controls the SASL configuration (org.apache.hadoop.security.UserGroupInformation.HadoopConfiguration), instead of setting up the ZooKeeper "Client" loginContext via jaas.conf and system property

      -Djava.security.auth.login.config

      Using the Hadoop configuration would work, except that ZooKeeper client code expects the loginContextName to be "Client" while Hadoop security will use "hadoop-keytab-kerberos". I verified that by changing the name in the debugger the SASL authentication succeeds while otherwise the login configuration cannot be resolved and the connection to ZooKeeper is unauthenticated.

      To integrate with Hadoop, the following in ZooKeeperSaslClient would need to change to make the name configurable:

      login = new Login("Client",new ClientCallbackHandler(null));

      1. ZOOKEEPER-1373-TW_3_4.patch
        3 kB
        Thomas Weise
      2. ZOOKEEPER-1373.patch
        5 kB
        Eugene Koontz
      3. ZOOKEEPER-1373.patch
        14 kB
        Eugene Koontz
      4. ZOOKEEPER-1373.patch
        23 kB
        Eugene Koontz
      5. ZOOKEEPER-1373.patch
        23 kB
        Eugene Koontz
      6. ZOOKEEPER-1373.patch
        28 kB
        Eugene Koontz
      7. ZOOKEEPER-1373.patch
        28 kB
        Eugene Koontz

        Issue Links

          Activity

          Hide
          Thomas Weise added a comment -

          Tried the 3.4.3-rc and it works as expected. Update on the originally reported Hadoop integration issue:

          With Hadoop 1.0, ZooKeeper client as of 3.4.2 with the default configuration will work, because Hadoop no longer hijacks the global javax.security configuration. See HADOOP-7853

          With earlier Hadoop versions, the changes made here will permit to reuse the configuration set by Hadoop.

          Show
          Thomas Weise added a comment - Tried the 3.4.3-rc and it works as expected. Update on the originally reported Hadoop integration issue: With Hadoop 1.0, ZooKeeper client as of 3.4.2 with the default configuration will work, because Hadoop no longer hijacks the global javax.security configuration. See HADOOP-7853 With earlier Hadoop versions, the changes made here will permit to reuse the configuration set by Hadoop.
          Hide
          Mahadev konar added a comment -

          @Thomas,
          Yes. The rc is up. Can you try it out: http://people.apache.org/~mahadev/zookeeper-3.4.3-candidate-0/

          Show
          Mahadev konar added a comment - @Thomas, Yes. The rc is up. Can you try it out: http://people.apache.org/~mahadev/zookeeper-3.4.3-candidate-0/
          Hide
          Thomas Weise added a comment -

          Thanks, is this going to go into 3.4.3 also?

          Show
          Thomas Weise added a comment - Thanks, is this going to go into 3.4.3 also?
          Hide
          Eugene Koontz added a comment -

          Thanks for the commit and the comments Mahadev. I'll keep thinking about how we can improve ClientCnxn as it relates to modularity and security.

          Show
          Eugene Koontz added a comment - Thanks for the commit and the comments Mahadev. I'll keep thinking about how we can improve ClientCnxn as it relates to modularity and security.
          Hide
          Hudson added a comment -

          Integrated in ZooKeeper-trunk #1451 (See https://builds.apache.org/job/ZooKeeper-trunk/1451/)
          ZOOKEEPER-1373. Hardcoded SASL login context name clashes with Hadoop security configuration override. (Eugene Koontz and Thomas Weise via mahadev)

          mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1240918
          Files :

          • /zookeeper/trunk/CHANGES.txt
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/Login.java
          • /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java
          • /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java
          Show
          Hudson added a comment - Integrated in ZooKeeper-trunk #1451 (See https://builds.apache.org/job/ZooKeeper-trunk/1451/ ) ZOOKEEPER-1373 . Hardcoded SASL login context name clashes with Hadoop security configuration override. (Eugene Koontz and Thomas Weise via mahadev) mahadev : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1240918 Files : /zookeeper/trunk/CHANGES.txt /zookeeper/trunk/src/java/main/org/apache/zookeeper/ClientCnxn.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/Login.java /zookeeper/trunk/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java /zookeeper/trunk/src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java
          Hide
          Mahadev konar added a comment -

          I just committed this. Thanks Eugene and Thomas!

          Show
          Mahadev konar added a comment - I just committed this. Thanks Eugene and Thomas!
          Hide
          Mahadev konar added a comment -

          @Eugene,
          The patch looks good, but we should work on cleaning up the security stuff a little. One thing would be to make ClientCnxn a little modular and not pass it arnd everywhere (like we do in ZKSaslClient). Neways thats for later. Ill go ahead and commit this for now.

          Show
          Mahadev konar added a comment - @Eugene, The patch looks good, but we should work on cleaning up the security stuff a little. One thing would be to make ClientCnxn a little modular and not pass it arnd everywhere (like we do in ZKSaslClient). Neways thats for later. Ill go ahead and commit this for now.
          Hide
          Mahadev konar added a comment -

          Javadoc warning is due to ZOOKEEPER-1386.

          Show
          Mahadev konar added a comment - Javadoc warning is due to ZOOKEEPER-1386 .
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12513087/ZOOKEEPER-1373.patch
          against trunk revision 1238176.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/937//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/937//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/937//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12513087/ZOOKEEPER-1373.patch against trunk revision 1238176. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/937//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/937//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/937//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Fix findbugs warning.

          I think the javadoc warning is due to a transitory (I hope) failure to contact http://java.sun.com :

          http://www.downforeveryoneorjustme.com/java.sun.com

          Show
          Eugene Koontz added a comment - Fix findbugs warning. I think the javadoc warning is due to a transitory (I hope) failure to contact http://java.sun.com : http://www.downforeveryoneorjustme.com/java.sun.com
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12513048/ZOOKEEPER-1373.patch
          against trunk revision 1238176.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/936//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/936//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/936//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12513048/ZOOKEEPER-1373.patch against trunk revision 1238176. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/936//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/936//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/936//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          New patch that moves most new code to ZooKeeperSaslClient rather than ClientCnxn.

          Show
          Eugene Koontz added a comment - New patch that moves most new code to ZooKeeperSaslClient rather than ClientCnxn.
          Hide
          Mahadev konar added a comment -

          Thanks Eugene!

          Show
          Mahadev konar added a comment - Thanks Eugene!
          Hide
          Eugene Koontz added a comment -

          Mahadev, thanks for your comments. Going to address your comment about ClientCnxn getting huge in a new patch.
          -Eugene

          Show
          Eugene Koontz added a comment - Mahadev, thanks for your comments. Going to address your comment about ClientCnxn getting huge in a new patch. -Eugene
          Hide
          Mahadev konar added a comment -

          Took a look at the patch. It looks good overall, like the new test cases. Some minor nits, I think the ClientCnxn code needs to move out a little (ClientCnxn is getting too huge). Can we do a helper class for Security? Something like ZooKeeperSecureUtil where all this code can reside (creating a zk sasl client?). Also its a little painful to see all the config property names spread around. This is probably another jira where we move all the properties in a single place so that we dont have to go hunting arnd for our config properties.

          Show
          Mahadev konar added a comment - Took a look at the patch. It looks good overall, like the new test cases. Some minor nits, I think the ClientCnxn code needs to move out a little (ClientCnxn is getting too huge). Can we do a helper class for Security? Something like ZooKeeperSecureUtil where all this code can reside (creating a zk sasl client?). Also its a little painful to see all the config property names spread around. This is probably another jira where we move all the properties in a single place so that we dont have to go hunting arnd for our config properties.
          Hide
          Mahadev konar added a comment -

          I just hate the way review board updates the comments. Looking at the patch now.

          Show
          Mahadev konar added a comment - I just hate the way review board updates the comments. Looking at the patch now.
          Hide
          Patrick Hunt added a comment -

          We'll likely cut a 3.4.3 soon, seems like it would be good to include this, if it's ready.

          Show
          Patrick Hunt added a comment - We'll likely cut a 3.4.3 soon, seems like it would be good to include this, if it's ready.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12512217/ZOOKEEPER-1373.patch
          against trunk revision 1234974.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/927//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/927//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/927//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12512217/ZOOKEEPER-1373.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/927//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/927//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/927//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          Patch against trunk. Includes 3 new tests.

          Show
          Eugene Koontz added a comment - Patch against trunk. Includes 3 new tests.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4656
          -----------------------------------------------------------

          Eugene, can you complete the patch for this issue? Documentation could be taken up separately.

          • Thomas

          On 2012-01-26 00:30:39, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Login.java 6d2a38c

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4656 ----------------------------------------------------------- Eugene, can you complete the patch for this issue? Documentation could be taken up separately. Thomas On 2012-01-26 00:30:39, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          Patrick Hunt added a comment -

          cancel pending updated patch

          Show
          Patrick Hunt added a comment - cancel pending updated patch
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-26 18:18:55, Thomas wrote:

          > conf/zoo_sample.cfg, line 30

          > <https://reviews.apache.org/r/3631/diff/3/?file=70817#file70817line30>

          >

          > What is the purpose of adding this to the server config?

          >

          Eugene Koontz wrote:

          Thomas, thanks for testing, and you are right - it doesn't belong here.

          It should probably be here:

          https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL

          I'll edit that today (feel free to do so also, and would be curious to hear your comments about this wiki entry, was it helpful for you?)

          Hi Eugene, I found your other page more useful as it covers the users perspective: https://github.com/ekoontz/zookeeper/wiki

          This should become part of the ZooKeeper documentation.

          • Thomas

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4626
          -----------------------------------------------------------

          On 2012-01-26 00:30:39, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Login.java 6d2a38c

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-26 18:18:55, Thomas wrote: > conf/zoo_sample.cfg, line 30 > < https://reviews.apache.org/r/3631/diff/3/?file=70817#file70817line30 > > > What is the purpose of adding this to the server config? > Eugene Koontz wrote: Thomas, thanks for testing, and you are right - it doesn't belong here. It should probably be here: https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL I'll edit that today (feel free to do so also, and would be curious to hear your comments about this wiki entry, was it helpful for you?) Hi Eugene, I found your other page more useful as it covers the users perspective: https://github.com/ekoontz/zookeeper/wiki This should become part of the ZooKeeper documentation. Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4626 ----------------------------------------------------------- On 2012-01-26 00:30:39, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-26 18:18:55, Thomas wrote:

          > conf/zoo_sample.cfg, line 30

          > <https://reviews.apache.org/r/3631/diff/3/?file=70817#file70817line30>

          >

          > What is the purpose of adding this to the server config?

          >

          Thomas, thanks for testing, and you are right - it doesn't belong here.

          It should probably be here:

          https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL

          I'll edit that today (feel free to do so also, and would be curious to hear your comments about this wiki entry, was it helpful for you?)

          • Eugene

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4626
          -----------------------------------------------------------

          On 2012-01-26 00:30:39, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Login.java 6d2a38c

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-26 18:18:55, Thomas wrote: > conf/zoo_sample.cfg, line 30 > < https://reviews.apache.org/r/3631/diff/3/?file=70817#file70817line30 > > > What is the purpose of adding this to the server config? > Thomas, thanks for testing, and you are right - it doesn't belong here. It should probably be here: https://cwiki.apache.org/confluence/display/ZOOKEEPER/Zookeeper+and+SASL I'll edit that today (feel free to do so also, and would be curious to hear your comments about this wiki entry, was it helpful for you?) Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4626 ----------------------------------------------------------- On 2012-01-26 00:30:39, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4626
          -----------------------------------------------------------

          Tested patch with Hadoop client and CLI and works as expected. Only remaining question I have is WRT added comments in zoo_sample.cfg

          conf/zoo_sample.cfg
          <https://reviews.apache.org/r/3631/#comment10288>

          What is the purpose of adding this to the server config?

          • Thomas

          On 2012-01-26 00:30:39, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Login.java 6d2a38c

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4626 ----------------------------------------------------------- Tested patch with Hadoop client and CLI and works as expected. Only remaining question I have is WRT added comments in zoo_sample.cfg conf/zoo_sample.cfg < https://reviews.apache.org/r/3631/#comment10288 > What is the purpose of adding this to the server config? Thomas On 2012-01-26 00:30:39, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-25 19:23:47, Ted Yu wrote:

          >

          Thanks Ted, fixed these.

          • Eugene

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4597
          -----------------------------------------------------------

          On 2012-01-26 00:30:39, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Login.java 6d2a38c

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-25 19:23:47, Ted Yu wrote: > Thanks Ted, fixed these. Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4597 ----------------------------------------------------------- On 2012-01-26 00:30:39, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-25 21:56:22, Thomas wrote:

          > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 950

          > <https://reviews.apache.org/r/3631/diff/2/?file=70795#file70795line950>

          >

          > SASL connection also needs to be attempted when ZooKeeperSaslClient.LOGIN_CONTEXT_NAME_KEY is set as system property and java.security.auth.login.config is not set (that's what happens with Hadoop security). That was part of my patch.

          >

          Hi Thomas,
          Sorry, I should have addressed your use case (no JAAS configuration file because using Hadoop security JAAS configuration). I made the properties-checking more elaborate - please see if my latest patch matches your intent. I tried to make the code guess the user's intentions regarding SASL configuration and give diagnostic log messages in case of failure. I also added another test for a missing Login Context section (please see SaslAuthMissingClientConfigTest.java).

          • Eugene

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4606
          -----------------------------------------------------------

          On 2012-01-26 00:30:39, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Login.java 6d2a38c

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-25 21:56:22, Thomas wrote: > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 950 > < https://reviews.apache.org/r/3631/diff/2/?file=70795#file70795line950 > > > SASL connection also needs to be attempted when ZooKeeperSaslClient.LOGIN_CONTEXT_NAME_KEY is set as system property and java.security.auth.login.config is not set (that's what happens with Hadoop security). That was part of my patch. > Hi Thomas, Sorry, I should have addressed your use case (no JAAS configuration file because using Hadoop security JAAS configuration). I made the properties-checking more elaborate - please see if my latest patch matches your intent. I tried to make the code guess the user's intentions regarding SASL configuration and give diagnostic log messages in case of failure. I also added another test for a missing Login Context section (please see SaslAuthMissingClientConfigTest.java). Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4606 ----------------------------------------------------------- On 2012-01-26 00:30:39, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-25 18:58:43, Eugene Koontz wrote:

          > conf/zoo_sample.cfg, line 30

          > <https://reviews.apache.org/r/3631/diff/2/?file=70794#file70794line30>

          >

          > note that 'org.apache.zookeeper' prefix breaks convention with other variables.

          Thomas wrote:

          zoo.cfg is read by the server. But here we need a setting on the client that goes into the system properties namespace. So certainly it needs to be prefixed in an appropriate way.

          If we follow existing convention as seen here:

          Boolean.getBoolean("zookeeper.disableAutoWatchReset");

          As per comment below, the originally suggested key seems to be fine:

          zookeeper.sasl.clientconfig

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373?focusedCommentId=13192817&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13192817

          using 'zookeeper.sasl.clientconfig' as property name as Thomas suggests.

          • Eugene

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4595
          -----------------------------------------------------------

          On 2012-01-26 00:30:39, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/main/org/apache/zookeeper/Login.java 6d2a38c

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-25 18:58:43, Eugene Koontz wrote: > conf/zoo_sample.cfg, line 30 > < https://reviews.apache.org/r/3631/diff/2/?file=70794#file70794line30 > > > note that 'org.apache.zookeeper' prefix breaks convention with other variables. Thomas wrote: zoo.cfg is read by the server. But here we need a setting on the client that goes into the system properties namespace. So certainly it needs to be prefixed in an appropriate way. If we follow existing convention as seen here: Boolean.getBoolean("zookeeper.disableAutoWatchReset"); As per comment below, the originally suggested key seems to be fine: zookeeper.sasl.clientconfig https://issues.apache.org/jira/browse/ZOOKEEPER-1373?focusedCommentId=13192817&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13192817 using 'zookeeper.sasl.clientconfig' as property name as Thomas suggests. Eugene ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4595 ----------------------------------------------------------- On 2012-01-26 00:30:39, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12511918/ZOOKEEPER-1373.patch
          against trunk revision 1234974.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/920//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/920//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/920//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12511918/ZOOKEEPER-1373.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/920//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/920//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/920//console This message is automatically generated.
          Hide
          Eugene Koontz added a comment -

          this patch addresses Ted and Thomas' reviews.

          Show
          Eugene Koontz added a comment - this patch addresses Ted and Thomas' reviews.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/
          -----------------------------------------------------------

          (Updated 2012-01-26 00:30:39.276464)

          Review request for zookeeper.

          Changes
          -------

          Address Ted and Thomas' reviews.

          Summary
          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.
          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs (updated)


          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION
          src/java/main/org/apache/zookeeper/Login.java 6d2a38c
          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e
          conf/zoo_sample.cfg aafb324
          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          Diff: https://reviews.apache.org/r/3631/diff

          Testing
          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-26 00:30:39.276464) Review request for zookeeper. Changes ------- Address Ted and Thomas' reviews. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs (updated) src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthMissingClientConfigTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/main/org/apache/zookeeper/Login.java 6d2a38c src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4606
          -----------------------------------------------------------

          src/java/main/org/apache/zookeeper/ClientCnxn.java
          <https://reviews.apache.org/r/3631/#comment10253>

          SASL connection also needs to be attempted when ZooKeeperSaslClient.LOGIN_CONTEXT_NAME_KEY is set as system property and java.security.auth.login.config is not set (that's what happens with Hadoop security). That was part of my patch.

          • Thomas

          On 2012-01-25 18:56:42, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-25 18:56:42)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4606 ----------------------------------------------------------- src/java/main/org/apache/zookeeper/ClientCnxn.java < https://reviews.apache.org/r/3631/#comment10253 > SASL connection also needs to be attempted when ZooKeeperSaslClient.LOGIN_CONTEXT_NAME_KEY is set as system property and java.security.auth.login.config is not set (that's what happens with Hadoop security). That was part of my patch. Thomas On 2012-01-25 18:56:42, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-25 18:56:42) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2012-01-25 18:58:43, Eugene Koontz wrote:

          > conf/zoo_sample.cfg, line 30

          > <https://reviews.apache.org/r/3631/diff/2/?file=70794#file70794line30>

          >

          > note that 'org.apache.zookeeper' prefix breaks convention with other variables.

          zoo.cfg is read by the server. But here we need a setting on the client that goes into the system properties namespace. So certainly it needs to be prefixed in an appropriate way.

          If we follow existing convention as seen here:

          Boolean.getBoolean("zookeeper.disableAutoWatchReset");

          As per comment below, the originally suggested key seems to be fine:

          zookeeper.sasl.clientconfig

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373?focusedCommentId=13192817&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13192817

          • Thomas

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4595
          -----------------------------------------------------------

          On 2012-01-25 18:56:42, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-25 18:56:42)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - On 2012-01-25 18:58:43, Eugene Koontz wrote: > conf/zoo_sample.cfg, line 30 > < https://reviews.apache.org/r/3631/diff/2/?file=70794#file70794line30 > > > note that 'org.apache.zookeeper' prefix breaks convention with other variables. zoo.cfg is read by the server. But here we need a setting on the client that goes into the system properties namespace. So certainly it needs to be prefixed in an appropriate way. If we follow existing convention as seen here: Boolean.getBoolean("zookeeper.disableAutoWatchReset"); As per comment below, the originally suggested key seems to be fine: zookeeper.sasl.clientconfig https://issues.apache.org/jira/browse/ZOOKEEPER-1373?focusedCommentId=13192817&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13192817 Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4595 ----------------------------------------------------------- On 2012-01-25 18:56:42, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-25 18:56:42) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12511868/ZOOKEEPER-1373.patch
          against trunk revision 1234974.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 4 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/919//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/919//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/919//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12511868/ZOOKEEPER-1373.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/919//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/919//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/919//console This message is automatically generated.
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4597
          -----------------------------------------------------------

          conf/zoo_sample.cfg
          <https://reviews.apache.org/r/3631/#comment10246>

          Redundant 'to ' in the beginning.

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java
          <https://reviews.apache.org/r/3631/#comment10247>

          Does this (classname) match the entry in zoo_sample.cfg ?

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java
          <https://reviews.apache.org/r/3631/#comment10248>

          Indentation is different from existing code.

          • Ted

          On 2012-01-25 18:56:42, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-25 18:56:42)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4597 ----------------------------------------------------------- conf/zoo_sample.cfg < https://reviews.apache.org/r/3631/#comment10246 > Redundant 'to ' in the beginning. src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java < https://reviews.apache.org/r/3631/#comment10247 > Does this (classname) match the entry in zoo_sample.cfg ? src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java < https://reviews.apache.org/r/3631/#comment10248 > Indentation is different from existing code. Ted On 2012-01-25 18:56:42, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-25 18:56:42) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Show
          Eugene Koontz added a comment - Same as https://reviews.apache.org/r/3631/diff/2/#index_header
          Hide
          Eugene Koontz added a comment -

          Created review at : https://reviews.apache.org/r/3631/

          Merges Thomas's changes.

          Also made the login context name more available for log messages and add some more log messages.

          Also now there are now 2 new tests for this issue: SaslAuthDesignatedClientTest and SaslAuthFailDesignatedClientTest.

          I have one concern: the name org.apache.zookeeper.client.ZooKeeperSaslClient.loginContextName - see in the patch the change to conf/zoo_sample.cfg. Why not just client.loginContext?

          Show
          Eugene Koontz added a comment - Created review at : https://reviews.apache.org/r/3631/ Merges Thomas's changes. Also made the login context name more available for log messages and add some more log messages. Also now there are now 2 new tests for this issue: SaslAuthDesignatedClientTest and SaslAuthFailDesignatedClientTest. I have one concern: the name org.apache.zookeeper.client.ZooKeeperSaslClient.loginContextName - see in the patch the change to conf/zoo_sample.cfg . Why not just client.loginContext ?
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/#review4595
          -----------------------------------------------------------

          conf/zoo_sample.cfg
          <https://reviews.apache.org/r/3631/#comment10242>

          note that 'org.apache.zookeeper' prefix breaks convention with other variables.

          • Eugene

          On 2012-01-25 18:56:42, Eugene Koontz wrote:

          -----------------------------------------------------------

          This is an automatically generated e-mail. To reply, visit:

          https://reviews.apache.org/r/3631/

          -----------------------------------------------------------

          (Updated 2012-01-25 18:56:42)

          Review request for zookeeper.

          Summary

          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.

          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs

          -----

          conf/zoo_sample.cfg aafb324

          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40

          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e

          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION

          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/3631/diff

          Testing

          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/#review4595 ----------------------------------------------------------- conf/zoo_sample.cfg < https://reviews.apache.org/r/3631/#comment10242 > note that 'org.apache.zookeeper' prefix breaks convention with other variables. Eugene On 2012-01-25 18:56:42, Eugene Koontz wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- (Updated 2012-01-25 18:56:42) Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs ----- conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          jiraposter@reviews.apache.org added a comment -

          -----------------------------------------------------------
          This is an automatically generated e-mail. To reply, visit:
          https://reviews.apache.org/r/3631/
          -----------------------------------------------------------

          Review request for zookeeper.

          Summary
          -------

          ZOOKEEPER-1373: Hardcoded SASL login context name clashes with Hadoop security configuration override

          Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use.

          This addresses bug ZOOKEEPER-1373.
          https://issues.apache.org/jira/browse/ZOOKEEPER-1373

          Diffs


          conf/zoo_sample.cfg aafb324
          src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40
          src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e
          src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION
          src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION

          Diff: https://reviews.apache.org/r/3631/diff

          Testing
          -------

          "ant test" java tests pass.

          Thanks,

          Eugene

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3631/ ----------------------------------------------------------- Review request for zookeeper. Summary ------- ZOOKEEPER-1373 : Hardcoded SASL login context name clashes with Hadoop security configuration override Fix is to allow system property to designate the JAAS configuration section that the zookeeper client will use. This addresses bug ZOOKEEPER-1373 . https://issues.apache.org/jira/browse/ZOOKEEPER-1373 Diffs conf/zoo_sample.cfg aafb324 src/java/main/org/apache/zookeeper/ClientCnxn.java 6c25e40 src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 722538e src/java/test/org/apache/zookeeper/test/SaslAuthDesignatedClientTest.java PRE-CREATION src/java/test/org/apache/zookeeper/test/SaslAuthFailDesignatedClientTest.java PRE-CREATION Diff: https://reviews.apache.org/r/3631/diff Testing ------- "ant test" java tests pass. Thanks, Eugene
          Hide
          Thomas Weise added a comment -

          Thanks Eugene. To me zookeeper.sasl.clientconfig looks like the better choice, even more so if it is inline with existing configuration key naming conventions compared to the more verbose key in my patch.

          Show
          Thomas Weise added a comment - Thanks Eugene. To me zookeeper.sasl.clientconfig looks like the better choice, even more so if it is inline with existing configuration key naming conventions compared to the more verbose key in my patch.
          Hide
          Eugene Koontz added a comment -

          Hi Thomas,
          I'm happy to merge our patches together - I'll use your system property name.
          -Eugene

          Show
          Eugene Koontz added a comment - Hi Thomas, I'm happy to merge our patches together - I'll use your system property name. -Eugene
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12511740/ZOOKEEPER-1373-TW_3_4.patch
          against trunk revision 1234974.

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/918//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/918//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/918//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12511740/ZOOKEEPER-1373-TW_3_4.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/918//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/918//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/918//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12511735/ZOOKEEPER-1373.patch
          against trunk revision 1234974.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 2 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/917//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/917//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/917//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12511735/ZOOKEEPER-1373.patch against trunk revision 1234974. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/917//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/917//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/917//console This message is automatically generated.
          Hide
          Thomas Weise added a comment -

          Eugene, could you please have a look and merge the two? Need fix for 3.4.3

          Show
          Thomas Weise added a comment - Eugene, could you please have a look and merge the two? Need fix for 3.4.3
          Hide
          Thomas Weise added a comment -

          Attaching the illustrative patch I had already created. It also takes care of handling the situation where system property java.security.auth.login.config is absent because that is handled by Hadoop:

          Verified with metastore server option:

          -Dorg.apache.zookeeper.client.ZooKeeperSaslClient.loginContextName=hadoop-keytab-kerberos

          Show
          Thomas Weise added a comment - Attaching the illustrative patch I had already created. It also takes care of handling the situation where system property java.security.auth.login.config is absent because that is handled by Hadoop: Verified with metastore server option: -Dorg.apache.zookeeper.client.ZooKeeperSaslClient.loginContextName=hadoop-keytab-kerberos
          Hide
          Eugene Koontz added a comment -

          (patch on 3.4 branch is identical)

          Show
          Eugene Koontz added a comment - (patch on 3.4 branch is identical)
          Hide
          Eugene Koontz added a comment -

          Fix bug and add new test file: SaslAuthDesignatedClientTest.java with one test.

          Show
          Eugene Koontz added a comment - Fix bug and add new test file: SaslAuthDesignatedClientTest.java with one test.
          Hide
          Mahadev konar added a comment -

          This is a bug. We should fix to have a login context name configurable.

          Show
          Mahadev konar added a comment - This is a bug. We should fix to have a login context name configurable.

            People

            • Assignee:
              Eugene Koontz
              Reporter:
              Thomas Weise
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development