Hadoop Common
  1. Hadoop Common
  2. HADOOP-10862

Miscellaneous trivial corrections to KMS classes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0.0
    • Fix Version/s: 2.6.0
    • Component/s: security
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      KMSRESTConstants.java, KEY_OP should be KEYS and value should be keys.

      KMS.java should be annotated with Jersey @Singleton to avoid creating an instance on every request, it is thread safe already.

      Make sure all KMS related classes are annotated with private audience.

      1. HADOOP-10862.1.patch
        25 kB
        Arun Suresh
      2. HADOOP-10862.2.patch
        23 kB
        Arun Suresh

        Activity

        Hide
        Arun Suresh added a comment -
        • Changed all log messages to use String.format()
        • Moved all KMS Operations from a string to an enum
        • Changed KEY_OP to KEY
        • Added @InterfaceAudience.Private to all KMS classes
        Show
        Arun Suresh added a comment - Changed all log messages to use String.format() Moved all KMS Operations from a string to an enum Changed KEY_OP to KEY Added @InterfaceAudience.Private to all KMS classes
        Hide
        Arun Suresh added a comment -

        Alejandro Abdelnur, apparently, making the KMS class a Singleton required some extra code changes. as per this link, we would need to Create an instance of an Application and register the Singleton with it.

        Also, considering the fact that the KMS class does'nt really hold state (the provider and kmsaudit references are passed to it by KMSWebApp) I feel it is lightweight enough to be instantiated for every call.

        Show
        Arun Suresh added a comment - Alejandro Abdelnur , apparently, making the KMS class a Singleton required some extra code changes. as per this link , we would need to Create an instance of an Application and register the Singleton with it. Also, considering the fact that the KMS class does'nt really hold state (the provider and kmsaudit references are passed to it by KMSWebApp ) I feel it is lightweight enough to be instantiated for every call.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12660318/HADOOP-10862.1.patch
        against trunk revision .

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

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

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

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

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

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

        -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-common-project/hadoop-kms:

        org.apache.hadoop.ha.TestZKFailoverControllerStress

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4437//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4437//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/12660318/HADOOP-10862.1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-common-project/hadoop-kms: org.apache.hadoop.ha.TestZKFailoverControllerStress +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4437//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4437//console This message is automatically generated.
        Hide
        Alejandro Abdelnur added a comment -

        KMS.java:

        • On the message template constants, we were using 2 single quotes because the single quote has a special meaning (take {} as literal) and you have to escape it using 2 single quotes. Now that we are using String.format(), I believe we should be just 1 single quote.
        • The KMSOps enum seems identical to the KMSACLs.Type enum, would make sense to bubble up KMSOps enum to top level class and use that in both places?

        KMSAudit.java:

        • I was not suggesting changing the slf4j message templates using {} to String.format(), we should not use String.format() here, so the message is resolved only if the log level is enabled (using String.format(), the message is always resolved).
        Show
        Alejandro Abdelnur added a comment - KMS.java : On the message template constants, we were using 2 single quotes because the single quote has a special meaning (take {} as literal) and you have to escape it using 2 single quotes. Now that we are using String.format() , I believe we should be just 1 single quote. The KMSOps enum seems identical to the KMSACLs.Type enum, would make sense to bubble up KMSOps enum to top level class and use that in both places? KMSAudit.java : I was not suggesting changing the slf4j message templates using {} to String.format(), we should not use String.format() here, so the message is resolved only if the log level is enabled (using String.format() , the message is always resolved).
        Hide
        Arun Suresh added a comment -

        @tucu, thanks for the feedback

        The KMSOps enum seems identical to the KMSACLs.Type enum..

        I was considering that initially, but realized that there are ACLs such as SET_KEY_VERSION, which is not a KMS operation. Also, multiple KMS ops might map to a single ACL (for eg. GET) which is the reason I felt we could keep them separate.

        Show
        Arun Suresh added a comment - @tucu, thanks for the feedback The KMSOps enum seems identical to the KMSACLs.Type enum.. I was considering that initially, but realized that there are ACLs such as SET_KEY_VERSION, which is not a KMS operation. Also, multiple KMS ops might map to a single ACL (for eg. GET) which is the reason I felt we could keep them separate.
        Hide
        Arun Suresh added a comment -

        Updating patch with Alejandro Abdelnur's feedback

        Show
        Arun Suresh added a comment - Updating patch with Alejandro Abdelnur 's feedback
        Hide
        Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12660579/HADOOP-10862.2.patch
        against trunk revision .

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

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

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

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

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

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

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-common-project/hadoop-kms.

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

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4441//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4441//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/12660579/HADOOP-10862.2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-common-project/hadoop-kms. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4441//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4441//console This message is automatically generated.
        Hide
        Alejandro Abdelnur added a comment -

        +1

        Show
        Alejandro Abdelnur added a comment - +1
        Hide
        Alejandro Abdelnur added a comment -

        Thanks Arun. Committed to trunk.

        Show
        Alejandro Abdelnur added a comment - Thanks Arun. Committed to trunk.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #6040 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6040/)
        HADOOP-10862. Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
        Show
        Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6040 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6040/ ) HADOOP-10862 . Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Yarn-trunk #639 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/639/)
        HADOOP-10862. Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #639 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/639/ ) HADOOP-10862 . Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Hdfs-trunk #1832 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1832/)
        HADOOP-10862. Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1832 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1832/ ) HADOOP-10862 . Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1858 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1858/)
        HADOOP-10862. Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903)

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java
        Show
        Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1858 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1858/ ) HADOOP-10862 . Miscellaneous trivial corrections to KMS classes. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616903 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSRESTConstants.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMS.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSAudit.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSConfiguration.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KMSJMXServlet.java /hadoop/common/trunk/hadoop-common-project/hadoop-kms/src/test/java/org/apache/hadoop/crypto/key/kms/server/TestKMSAudit.java

          People

          • Assignee:
            Arun Suresh
            Reporter:
            Alejandro Abdelnur
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development