ZooKeeper
  1. ZooKeeper
  2. ZOOKEEPER-1525

Plumb ZooKeeperServer object into auth plugins

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.5.0
    • Component/s: None
    • Labels:
      None

      Description

      I want to plumb the ZooKeeperServer object into the auth plugins so that I can store authentication data in zookeeper itself. With access to the ZooKeeperServer object, I also have access to the ZKDatabase and can look up entries in the local copy of the zookeeper data.

      In order to implement this, I make sure that a ZooKeeperServer instance is passed in to the ProviderRegistry.initialize() method. Then initialize() will try to find a constructor for the AuthenticationProvider that takes a ZooKeeperServer instance. If the constructor is found, it will be used. Otherwise, initialize() will look for a constructor that takes no arguments and use that instead.

        Activity

        Hide
        Warren Turkal added a comment -

        Convert tabs to spaces.

        Show
        Warren Turkal added a comment - Convert tabs to spaces.
        Hide
        Warren Turkal added a comment -

        Fixed some more whitespace issues. Improved type safety of reflection code.

        Show
        Warren Turkal added a comment - Fixed some more whitespace issues. Improved type safety of reflection code.
        Hide
        Eugene Koontz added a comment -

        Hi Warren,

        I'm not sure why the Jenkins build did not get triggered here on your patch.

        Although, even if Jenkins was triggered, I'm afraid this patch might not work with Jenkins because it has "a/" and "b/" paths in the patch. I think you need to add

        [diff]
             noprefix = true
        

        to your ~/.gitconfig.

        Show
        Eugene Koontz added a comment - Hi Warren, I'm not sure why the Jenkins build did not get triggered here on your patch. Although, even if Jenkins was triggered, I'm afraid this patch might not work with Jenkins because it has "a/" and "b/" paths in the patch. I think you need to add [diff] noprefix = true to your ~/.gitconfig.
        Hide
        Warren Turkal added a comment -

        I am working on making the change to the patch now.

        However, would it be possible to make the jenkins job accept git patches as is so that folks using the apache git won't have to remember to adopt a special setting to submit patches? For example, could the job try -p1 if -p0 fails when patching in the diff?

        Show
        Warren Turkal added a comment - I am working on making the change to the patch now. However, would it be possible to make the jenkins job accept git patches as is so that folks using the apache git won't have to remember to adopt a special setting to submit patches? For example, could the job try -p1 if -p0 fails when patching in the diff?
        Hide
        Warren Turkal added a comment -

        updated patch

        Show
        Warren Turkal added a comment - updated patch
        Hide
        Warren Turkal added a comment -

        This still doesn't appear to have triggered a jenkins build. What's wrong?

        Show
        Warren Turkal added a comment - This still doesn't appear to have triggered a jenkins build. What's wrong?
        Hide
        Warren Turkal added a comment -

        Patrick Hunt, Is assigning this to you the right way to move this issue forward?

        Show
        Warren Turkal added a comment - Patrick Hunt , Is assigning this to you the right way to move this issue forward?
        Hide
        Hadoop QA added a comment -

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

        +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/1159//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//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/12539665/ZOOKEEPER-1525.patch against trunk revision 1373156. +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/1159//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1159//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/12539665/ZOOKEEPER-1525.patch
        against trunk revision 1530166.

        +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/1658//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//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/12539665/ZOOKEEPER-1525.patch against trunk revision 1530166. +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/1658//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1658//console This message is automatically generated.
        Hide
        Michi Mutsuzaki added a comment -

        Hi Warren,

        Sorry for super-slow response. I have 2 comments:

        1. Could you fix these warnings?

        compile:
            [javac] Compiling 3 source files to /home/michi/svndev/zookeeper/trunk/build/classes
            [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.5
            [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:57: warning: [cast] redundant cast to AuthenticationProvider
            [javac]                             ap = (AuthenticationProvider)constructor.newInstance(zks);
            [javac]                                  ^
            [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:60: warning: [cast] redundant cast to AuthenticationProvider
            [javac]                             ap = (AuthenticationProvider)constructor.newInstance();
            [javac]                                  ^
            [javac] 3 warnings
        

        2. When does this throw NullPointerException? Isn't it supposed to throw IllegalArgumentException if the arguments don't match?

        +                        Constructor<AuthenticationProvider> constructor = c.getConstructor(ZooKeeperServer.class);
        +                        try {
        +                            ap = (AuthenticationProvider)constructor.newInstance(zks);
        +                        } catch (NullPointerException e) {
        +                            constructor = c.getConstructor();
        +                            ap = (AuthenticationProvider)constructor.newInstance();
        +                        }
        
        Show
        Michi Mutsuzaki added a comment - Hi Warren, Sorry for super-slow response. I have 2 comments: 1. Could you fix these warnings? compile: [javac] Compiling 3 source files to /home/michi/svndev/zookeeper/trunk/build/classes [javac] warning: [options] bootstrap class path not set in conjunction with -source 1.5 [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:57: warning: [cast] redundant cast to AuthenticationProvider [javac] ap = (AuthenticationProvider)constructor.newInstance(zks); [javac] ^ [javac] /home/michi/svndev/zookeeper/trunk/src/java/main/org/apache/zookeeper/server/auth/ProviderRegistry.java:60: warning: [cast] redundant cast to AuthenticationProvider [javac] ap = (AuthenticationProvider)constructor.newInstance(); [javac] ^ [javac] 3 warnings 2. When does this throw NullPointerException? Isn't it supposed to throw IllegalArgumentException if the arguments don't match? + Constructor<AuthenticationProvider> constructor = c.getConstructor(ZooKeeperServer.class); + try { + ap = (AuthenticationProvider)constructor.newInstance(zks); + } catch (NullPointerException e) { + constructor = c.getConstructor(); + ap = (AuthenticationProvider)constructor.newInstance(); + }
        Hide
        Hadoop QA added a comment -

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

        +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/1995//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//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/12539665/ZOOKEEPER-1525.patch against trunk revision 1582572. +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/1995//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-ZOOKEEPER-Build/1995//console This message is automatically generated.
        Hide
        Michi Mutsuzaki added a comment -

        It looks like this patch breaks SASL tests.

        Show
        Michi Mutsuzaki added a comment - It looks like this patch breaks SASL tests.

          People

          • Assignee:
            Warren Turkal
            Reporter:
            Warren Turkal
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development