Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-13437

KMS should reload whitelist and default key ACLs when hot-reloading

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.0
    • Fix Version/s: 2.9.0, 3.0.0-alpha1
    • Component/s: kms
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      When hot-reloading, KMSACLs#setKeyACLs ignores whitelist and default key entries if they're present in memory.

      We should reload them, hot-reload and cold-start should not have any difference in behavior.
      Credit to Dilaver for finding this.

      1. HADOOP-13437.01.patch
        11 kB
        Xiao Chen
      2. HADOOP-13437.02.patch
        11 kB
        Xiao Chen
      3. HADOOP-13437.03.patch
        13 kB
        Xiao Chen
      4. HADOOP-13437.04.patch
        13 kB
        Xiao Chen
      5. HADOOP-13437.05.patch
        14 kB
        Xiao Chen

        Activity

        Hide
        xiaochen Xiao Chen added a comment -

        Patch 1 to fix this, with a unit test that fails without.

        Show
        xiaochen Xiao Chen added a comment - Patch 1 to fix this, with a unit test that fails without.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 17s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 31s trunk passed
        +1 compile 6m 45s trunk passed
        +1 checkstyle 0m 12s trunk passed
        +1 mvnsite 0m 18s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 0m 21s trunk passed
        +1 javadoc 0m 12s trunk passed
        +1 mvninstall 0m 15s the patch passed
        +1 compile 6m 44s the patch passed
        +1 javac 6m 44s the patch passed
        -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 4 new + 11 unchanged - 5 fixed = 15 total (was 16)
        +1 mvnsite 0m 18s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 0m 29s the patch passed
        +1 javadoc 0m 12s the patch passed
        +1 unit 2m 4s hadoop-kms in the patch passed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        26m 59s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820771/HADOOP-13437.01.patch
        JIRA Issue HADOOP-13437
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 0606573edf9a 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 26de4f0
        Default Java 1.8.0_101
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10107/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10107/testReport/
        modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10107/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 31s trunk passed +1 compile 6m 45s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 21s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 15s the patch passed +1 compile 6m 44s the patch passed +1 javac 6m 44s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 4 new + 11 unchanged - 5 fixed = 15 total (was 16) +1 mvnsite 0m 18s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 4s hadoop-kms in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 26m 59s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820771/HADOOP-13437.01.patch JIRA Issue HADOOP-13437 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 0606573edf9a 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 26de4f0 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10107/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10107/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10107/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        xiaochen Xiao Chen added a comment -

        Fixed checkstyle

        Show
        xiaochen Xiao Chen added a comment - Fixed checkstyle
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 15s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 7m 20s trunk passed
        +1 compile 7m 47s trunk passed
        +1 checkstyle 0m 13s trunk passed
        +1 mvnsite 0m 21s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 0m 25s trunk passed
        +1 javadoc 0m 13s trunk passed
        +1 mvninstall 0m 17s the patch passed
        +1 compile 8m 18s the patch passed
        +1 javac 8m 18s the patch passed
        +1 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 0 new + 8 unchanged - 8 fixed = 8 total (was 16)
        +1 mvnsite 0m 19s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 0m 33s the patch passed
        +1 javadoc 0m 11s the patch passed
        +1 unit 2m 7s hadoop-kms in the patch passed.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        30m 39s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820779/HADOOP-13437.02.patch
        JIRA Issue HADOOP-13437
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 4ec7420c49bb 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 26de4f0
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10109/testReport/
        modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10109/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 20s trunk passed +1 compile 7m 47s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 21s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 25s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 17s the patch passed +1 compile 8m 18s the patch passed +1 javac 8m 18s the patch passed +1 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 0 new + 8 unchanged - 8 fixed = 8 total (was 16) +1 mvnsite 0m 19s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 33s the patch passed +1 javadoc 0m 11s the patch passed +1 unit 2m 7s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 30m 39s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820779/HADOOP-13437.02.patch JIRA Issue HADOOP-13437 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4ec7420c49bb 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 26de4f0 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10109/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10109/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        andrew.wang Andrew Wang added a comment -

        Sounds like a good bug! Few review comments though:

        • Is the new "continue" case important? Not sure if we should ride over a possibly malformed ACL file.
        • By removing the if checks, it means if an ACL is specified multiple times, we'll use the last one rather than the first one. This seems like a big behavior change. I think what would be better is doing a "swap" like we do for the key ACLs, and breaking this function up into a few functions for clarity.
        Show
        andrew.wang Andrew Wang added a comment - Sounds like a good bug! Few review comments though: Is the new "continue" case important? Not sure if we should ride over a possibly malformed ACL file. By removing the if checks, it means if an ACL is specified multiple times, we'll use the last one rather than the first one. This seems like a big behavior change. I think what would be better is doing a "swap" like we do for the key ACLs, and breaking this function up into a few functions for clarity.
        Hide
        xiaochen Xiao Chen added a comment - - edited

        Thanks Andrew Wang for the great comments! I confess I was too obsessed with the fact that reloading doesn't load the properties, and overlooked the 'like a big behavior change'.

        However, with more digging, I still think this is right.

        • In Configuration parsing, later entries with the same name win. So in a Configuration object, a key will only have 1 value - the latest specified value.
        • Good call on swapping. Updated the code to do it this way - previously, the old entries for default/whitelist are preserved after reloading. I think this is purely a bug, because 1. looking at the latest config file you'll never figure out the actual acls in memory. 2. no way to update/remove a previously configured entry without restart.
          Added a test case for this.
        • Above said, I really don't see why the containsKey check should be kept. I looked at HADOOP-10433, HADOOP-10758 and HADOOP-11341, but don't see it mentioned. That seems to be a redundant check for me.

        Is the new "continue" case important? Not sure if we should ride over a possibly malformed ACL file.

        Not at all. It's ignoring the exception, and then depending on the fact aclType != null to continue - functionally the same. I can leave that untouched to reduce our change scope.

        Patch 3 is attached, with more thorough testing. I will do another round of test-case thinking tomorrow, but feel free to comment on what's here.

        Show
        xiaochen Xiao Chen added a comment - - edited Thanks Andrew Wang for the great comments! I confess I was too obsessed with the fact that reloading doesn't load the properties, and overlooked the 'like a big behavior change'. However, with more digging, I still think this is right. In Configuration parsing, later entries with the same name win. So in a Configuration object, a key will only have 1 value - the latest specified value. Good call on swapping. Updated the code to do it this way - previously, the old entries for default/whitelist are preserved after reloading. I think this is purely a bug, because 1. looking at the latest config file you'll never figure out the actual acls in memory. 2. no way to update/remove a previously configured entry without restart. Added a test case for this. Above said, I really don't see why the containsKey check should be kept. I looked at HADOOP-10433 , HADOOP-10758 and HADOOP-11341 , but don't see it mentioned. That seems to be a redundant check for me. Is the new "continue" case important? Not sure if we should ride over a possibly malformed ACL file. Not at all. It's ignoring the exception, and then depending on the fact aclType != null to continue - functionally the same. I can leave that untouched to reduce our change scope. Patch 3 is attached, with more thorough testing. I will do another round of test-case thinking tomorrow, but feel free to comment on what's here.
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 10s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 45s trunk passed
        +1 compile 7m 12s trunk passed
        +1 checkstyle 0m 13s trunk passed
        +1 mvnsite 0m 18s trunk passed
        +1 mvneclipse 0m 11s trunk passed
        +1 findbugs 0m 23s trunk passed
        +1 javadoc 0m 11s trunk passed
        +1 mvninstall 0m 17s the patch passed
        +1 compile 7m 14s the patch passed
        +1 javac 7m 14s the patch passed
        -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 2 new + 8 unchanged - 8 fixed = 10 total (was 16)
        +1 mvnsite 0m 18s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 0m 30s the patch passed
        +1 javadoc 0m 12s the patch passed
        +1 unit 2m 12s hadoop-kms in the patch passed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        28m 14s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820891/HADOOP-13437.03.patch
        JIRA Issue HADOOP-13437
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 88f6c78ebe27 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 8ebf2e9
        Default Java 1.8.0_101
        findbugs v3.0.0
        checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10117/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10117/testReport/
        modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10117/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 10s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 45s trunk passed +1 compile 7m 12s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 11s trunk passed +1 findbugs 0m 23s trunk passed +1 javadoc 0m 11s trunk passed +1 mvninstall 0m 17s the patch passed +1 compile 7m 14s the patch passed +1 javac 7m 14s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 2 new + 8 unchanged - 8 fixed = 10 total (was 16) +1 mvnsite 0m 18s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 30s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 12s hadoop-kms in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 28m 14s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12820891/HADOOP-13437.03.patch JIRA Issue HADOOP-13437 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 88f6c78ebe27 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8ebf2e9 Default Java 1.8.0_101 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/10117/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-kms.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10117/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10117/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        xiaochen Xiao Chen added a comment -

        Uploading patch 4 as a refined version.

        I'm still confused of the containsKey check though: after an admin replaced a kms-acls.xml, why do we only want to update the named keys, but keep the old whitelist/defaults? After the replacement (suppose there was no backup), how could the admin figure out what exactly the whitelist/defaults are? The config file is not loaded, and nowhere else to look at....

        Arun Suresh, could you please chime in? Is this an expected behavior (so we need to keep compatible behavior), or is this a bug (so we can fix it here)? Thanks in advance...

        Show
        xiaochen Xiao Chen added a comment - Uploading patch 4 as a refined version. I'm still confused of the containsKey check though: after an admin replaced a kms-acls.xml, why do we only want to update the named keys, but keep the old whitelist/defaults? After the replacement (suppose there was no backup), how could the admin figure out what exactly the whitelist/defaults are? The config file is not loaded, and nowhere else to look at.... Arun Suresh , could you please chime in? Is this an expected behavior (so we need to keep compatible behavior), or is this a bug (so we can fix it here)? Thanks in advance...
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 13s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 1s The patch appears to include 1 new or modified test files.
        +1 mvninstall 6m 56s trunk passed
        +1 compile 6m 44s trunk passed
        +1 checkstyle 0m 12s trunk passed
        +1 mvnsite 0m 18s trunk passed
        +1 mvneclipse 0m 13s trunk passed
        +1 findbugs 0m 21s trunk passed
        +1 javadoc 0m 12s trunk passed
        +1 mvninstall 0m 16s the patch passed
        +1 compile 6m 43s the patch passed
        +1 javac 6m 43s the patch passed
        +1 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 0 new + 7 unchanged - 9 fixed = 7 total (was 16)
        +1 mvnsite 0m 18s the patch passed
        +1 mvneclipse 0m 12s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 0m 28s the patch passed
        +1 javadoc 0m 12s the patch passed
        +1 unit 2m 6s hadoop-kms in the patch passed.
        +1 asflicense 0m 20s The patch does not generate ASF License warnings.
        27m 21s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823171/HADOOP-13437.04.patch
        JIRA Issue HADOOP-13437
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux 794302a8e58b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / e7e8aed
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10225/testReport/
        modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10225/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 1s The patch appears to include 1 new or modified test files. +1 mvninstall 6m 56s trunk passed +1 compile 6m 44s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 21s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 16s the patch passed +1 compile 6m 43s the patch passed +1 javac 6m 43s the patch passed +1 checkstyle 0m 12s hadoop-common-project/hadoop-kms: The patch generated 0 new + 7 unchanged - 9 fixed = 7 total (was 16) +1 mvnsite 0m 18s the patch passed +1 mvneclipse 0m 12s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 28s the patch passed +1 javadoc 0m 12s the patch passed +1 unit 2m 6s hadoop-kms in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 27m 21s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823171/HADOOP-13437.04.patch JIRA Issue HADOOP-13437 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 794302a8e58b 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / e7e8aed Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10225/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10225/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        asuresh Arun Suresh added a comment -

        ..Is this an expected behavior (so we need to keep compatible behavior), or is this a bug (so we can fix it here)? Thanks in advance...

        IIRC, this is actually expected behavior. This way, the default and whitelists are specified only once at startup, based on some deployment policy. New KeyACLs for individual users/groups and keys can be added / removed as users / keys are created.

        After the replacement (suppose there was no backup), how could the admin figure out what exactly the whitelist/defaults are?

        I feel this outside the scope of what KMS should worry about (Or we should build config management features that supports stuff like rollback etc. into KMS). The deployment environment / admin should ensure backups of the files are maintained.

        Show
        asuresh Arun Suresh added a comment - ..Is this an expected behavior (so we need to keep compatible behavior), or is this a bug (so we can fix it here)? Thanks in advance... IIRC, this is actually expected behavior. This way, the default and whitelists are specified only once at startup, based on some deployment policy. New KeyACLs for individual users/groups and keys can be added / removed as users / keys are created. After the replacement (suppose there was no backup), how could the admin figure out what exactly the whitelist/defaults are? I feel this outside the scope of what KMS should worry about (Or we should build config management features that supports stuff like rollback etc. into KMS). The deployment environment / admin should ensure backups of the files are maintained.
        Hide
        andrew.wang Andrew Wang added a comment -

        Hey Arun Suresh, could you go into a little more detail as to why this behavior is desirable? I agree with Xiao on it making things more complicated regarding the whitelist/defaults and ACL deployment generally.

        FWIW the KMS docs just say "This file is hot-reloaded when it changes" without mention of it applying to some types of ACLs and not others.

        One patch review comment, could we change the "Should not configure..." log warn to say something like "Invalid {} ACL for KEY_OP {}, ignoring"? I think that's more clear. Otherwise the intent looks good to me.

        Show
        andrew.wang Andrew Wang added a comment - Hey Arun Suresh , could you go into a little more detail as to why this behavior is desirable? I agree with Xiao on it making things more complicated regarding the whitelist/defaults and ACL deployment generally. FWIW the KMS docs just say "This file is hot-reloaded when it changes" without mention of it applying to some types of ACLs and not others. One patch review comment, could we change the "Should not configure..." log warn to say something like "Invalid {} ACL for KEY_OP {}, ignoring"? I think that's more clear. Otherwise the intent looks good to me.
        Hide
        xiaochen Xiao Chen added a comment -

        Attaching patch 5 to update the log message, and added lines in the test to verify ALL is not parsed for whitelist/default. (Log message says invalid key_op for xxx_acl, since I think that's more accurate.)

        Show
        xiaochen Xiao Chen added a comment - Attaching patch 5 to update the log message, and added lines in the test to verify ALL is not parsed for whitelist/default. (Log message says invalid key_op for xxx_acl, since I think that's more accurate.)
        Hide
        hadoopqa Hadoop QA added a comment -
        +1 overall



        Vote Subsystem Runtime Comment
        0 reexec 0m 14s Docker mode activated.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 test4tests 0m 0s The patch appears to include 1 new or modified test files.
        +1 mvninstall 7m 15s trunk passed
        +1 compile 8m 1s trunk passed
        +1 checkstyle 0m 12s trunk passed
        +1 mvnsite 0m 19s trunk passed
        +1 mvneclipse 0m 12s trunk passed
        +1 findbugs 0m 23s trunk passed
        +1 javadoc 0m 12s trunk passed
        +1 mvninstall 0m 18s the patch passed
        +1 compile 8m 14s the patch passed
        +1 javac 8m 14s the patch passed
        +1 checkstyle 0m 14s hadoop-common-project/hadoop-kms: The patch generated 0 new + 7 unchanged - 9 fixed = 7 total (was 16)
        +1 mvnsite 0m 24s the patch passed
        +1 mvneclipse 0m 15s the patch passed
        +1 whitespace 0m 0s The patch has no whitespace issues.
        +1 findbugs 0m 32s the patch passed
        +1 javadoc 0m 13s the patch passed
        +1 unit 2m 9s hadoop-kms in the patch passed.
        +1 asflicense 0m 21s The patch does not generate ASF License warnings.
        30m 56s



        Subsystem Report/Notes
        Docker Image:yetus/hadoop:9560f25
        JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823768/HADOOP-13437.05.patch
        JIRA Issue HADOOP-13437
        Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
        uname Linux d843892d6bdf 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Build tool maven
        Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
        git revision trunk / 03dea65
        Default Java 1.8.0_101
        findbugs v3.0.0
        Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10252/testReport/
        modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms
        Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10252/console
        Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 14s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 1 new or modified test files. +1 mvninstall 7m 15s trunk passed +1 compile 8m 1s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 19s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 23s trunk passed +1 javadoc 0m 12s trunk passed +1 mvninstall 0m 18s the patch passed +1 compile 8m 14s the patch passed +1 javac 8m 14s the patch passed +1 checkstyle 0m 14s hadoop-common-project/hadoop-kms: The patch generated 0 new + 7 unchanged - 9 fixed = 7 total (was 16) +1 mvnsite 0m 24s the patch passed +1 mvneclipse 0m 15s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 32s the patch passed +1 javadoc 0m 13s the patch passed +1 unit 2m 9s hadoop-kms in the patch passed. +1 asflicense 0m 21s The patch does not generate ASF License warnings. 30m 56s Subsystem Report/Notes Docker Image:yetus/hadoop:9560f25 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12823768/HADOOP-13437.05.patch JIRA Issue HADOOP-13437 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux d843892d6bdf 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 03dea65 Default Java 1.8.0_101 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/10252/testReport/ modules C: hadoop-common-project/hadoop-kms U: hadoop-common-project/hadoop-kms Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/10252/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
        Hide
        asuresh Arun Suresh added a comment -

        So, the original intent was based on the assumption that defaults and whitelists should not be changed once KMS is started.. I agree it should have been doc-ed. (On startup, all KeyOpType defaults and whitelists would be specified.. and no new defaults and whitelists would be added... which is reason for the containsKey)
        The kms-acls.xml hot reloading was meant to be used in the event keys and associated users/groups were added/removed, not for modifying the defaults and whitelists.
        I agree hot reloading of these would make things more flexible though. I guess we should either:

        1. move defaults and whitelists to kms-site.xml and thereby ensure these are unambiguously NOT hot reloadable.
        2. as per this JIRA remove the restriction and allow everything to be hot reloadable.

        Thoughts?

        Show
        asuresh Arun Suresh added a comment - So, the original intent was based on the assumption that defaults and whitelists should not be changed once KMS is started.. I agree it should have been doc-ed. (On startup, all KeyOpType defaults and whitelists would be specified.. and no new defaults and whitelists would be added... which is reason for the containsKey ) The kms-acls.xml hot reloading was meant to be used in the event keys and associated users/groups were added/removed, not for modifying the defaults and whitelists. I agree hot reloading of these would make things more flexible though. I guess we should either: move defaults and whitelists to kms-site.xml and thereby ensure these are unambiguously NOT hot reloadable. as per this JIRA remove the restriction and allow everything to be hot reloadable. Thoughts?
        Hide
        xiaochen Xiao Chen added a comment -

        Thanks for the review and discussions, Arun and Andrew.

        Personally I would prefer #2, since I think this would minimize user surprise, and make things easier to maintain, both for developers and admins. This way, we can treat this as a bug fix, and put it into 2.x. #1 will be incompatible so we can't do anything until 3.0. m2c.

        Show
        xiaochen Xiao Chen added a comment - Thanks for the review and discussions, Arun and Andrew. Personally I would prefer #2, since I think this would minimize user surprise, and make things easier to maintain, both for developers and admins. This way, we can treat this as a bug fix, and put it into 2.x. #1 will be incompatible so we can't do anything until 3.0. m2c.
        Hide
        andrew.wang Andrew Wang added a comment -

        Thanks for commenting Arun. I like #2 as well, +1 for the most recent patch also. Overall I don't see a downside to making everything configurable, we can advise users about the recommended way to use the ACLs via documentation.

        Show
        andrew.wang Andrew Wang added a comment - Thanks for commenting Arun. I like #2 as well, +1 for the most recent patch also. Overall I don't see a downside to making everything configurable, we can advise users about the recommended way to use the ACLs via documentation.
        Hide
        asuresh Arun Suresh added a comment -

        Agreed. We can always re-look at this later if there is a strong case to keep this behavior.
        +1 from me as well.

        Show
        asuresh Arun Suresh added a comment - Agreed. We can always re-look at this later if there is a strong case to keep this behavior. +1 from me as well.
        Hide
        xiaochen Xiao Chen added a comment -

        Committed to trunk and branch-2. Thanks much, Andrew and Arun!

        Show
        xiaochen Xiao Chen added a comment - Committed to trunk and branch-2. Thanks much, Andrew and Arun!
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10277 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10277/)
        HADOOP-13437. KMS should reload whitelist and default key ACLs when (xiao: rev 9daa9979a1f92fb3230361c10ddfcc1633795c0e)

        • (edit) hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSACLs.java
        • (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSACLs.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10277 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10277/ ) HADOOP-13437 . KMS should reload whitelist and default key ACLs when (xiao: rev 9daa9979a1f92fb3230361c10ddfcc1633795c0e) (edit) hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSACLs.java (edit) hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSACLs.java

          People

          • Assignee:
            xiaochen Xiao Chen
            Reporter:
            xiaochen Xiao Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development