Uploaded image for project: 'HBase'
  1. HBase
  2. HBASE-14901

There is duplicated code to create/manage encryption keys

Details

    • Bug
    • Status: Resolved
    • Minor
    • Resolution: Fixed
    • 2.0.0
    • 2.0.0
    • encryption
    • None
    • Reviewed

    Description

      There is duplicated code from MobUtils.createEncryptionContext in HStore, and there is a subset of that code in HFileReaderImpl.

      Refactored key selection
      Moved both to EncryptionUtil.java
      Can't figure out how to write a unit test for this, but there's no new code just refactoring.

      Attachments

        1. HBASE-14901.1.patch
          19 kB
          Nate Edel
        2. HBASE-14901.2.patch
          19 kB
          Nate Edel
        3. HBASE-14901.3.patch
          23 kB
          Nate Edel
        4. HBASE-14901.5.patch
          23 kB
          Nate Edel
        5. HBASE-14901.6.patch
          23 kB
          Nate Edel

        Issue Links

          Activity

            nkedel Nate Edel added a comment -

            ghelmling or eclark can one of you take a quick look at this?

            nkedel Nate Edel added a comment - ghelmling or eclark can one of you take a quick look at this?
            nkedel Nate Edel added a comment -

            Bad imports when applied to an unmodified tree; trying again.

            nkedel Nate Edel added a comment - Bad imports when applied to an unmodified tree; trying again.
            ghelmling Gary Helmling added a comment -

            How about renaming EncryptionUtils.getgetKeyFromBytesOrMasterKey(...) to just EncryptionUtils.unwrapKey(Configuration, byte[]), since it's just delegating to unwrap key with known config values. Since it's a public method it should have some javadoc as well. The rest of the refactor looks good to me.

            Seems okay to me to relocate these to the hbase-client module since the other unwrapKey() methods are already there.

            apurtell any thoughts?

            ghelmling Gary Helmling added a comment - How about renaming EncryptionUtils.getgetKeyFromBytesOrMasterKey(...) to just EncryptionUtils.unwrapKey(Configuration, byte[]), since it's just delegating to unwrap key with known config values. Since it's a public method it should have some javadoc as well. The rest of the refactor looks good to me. Seems okay to me to relocate these to the hbase-client module since the other unwrapKey() methods are already there. apurtell any thoughts?

            I concur ghelmling, rename that method as you suggest, put in hbase-client, and lgtm

            apurtell Andrew Kyle Purtell added a comment - I concur ghelmling , rename that method as you suggest, put in hbase-client, and lgtm
            nkedel Nate Edel added a comment -

            Changes made as requested to name and (yes, that's much better naming) and JavaDoc added. Submitting to get QABot's opinion.

            apurtell - EncryptionUtil.java is already in hbase-client; is there anything else that should be moved?

            nkedel Nate Edel added a comment - Changes made as requested to name and (yes, that's much better naming) and JavaDoc added. Submitting to get QABot's opinion. apurtell - EncryptionUtil.java is already in hbase-client; is there anything else that should be moved?
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12775191/HBASE-14901.2.patch
            against master branch at commit cbbad6e5fbcf95f585c850a688d9cce51da06e24.
            ATTACHMENT ID: 12775191

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

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

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

            -1 checkstyle. The applied patch generated new checkstyle errors. Check build console for list of new errors.

            +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 lineLengths. The patch does not introduce lines longer than 100

            +1 site. The mvn post-site goal succeeds with this patch.

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

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//testReport/
            Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//artifact/patchprocess/newFindbugsWarnings.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//artifact/patchprocess/checkstyle-aggregate.html

            Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12775191/HBASE-14901.2.patch against master branch at commit cbbad6e5fbcf95f585c850a688d9cce51da06e24. ATTACHMENT ID: 12775191 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated new checkstyle errors. Check build console for list of new errors. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16726//console This message is automatically generated.
            nkedel Nate Edel added a comment -

            Fixed checkstyle errors (bad imports.)
            Fixed some existing checkstyle errors (bad import order, no private constructor on EncryptionUtil and MobUtils).
            Fixed "protected" LOG on EncryptionUtil to private

            nkedel Nate Edel added a comment - Fixed checkstyle errors (bad imports.) Fixed some existing checkstyle errors (bad import order, no private constructor on EncryptionUtil and MobUtils). Fixed "protected" LOG on EncryptionUtil to private
            ghelmling Gary Helmling added a comment -

            +1 on patch v3. Will wait for HadoopQA to come back on that patch before committing.

            ghelmling Gary Helmling added a comment - +1 on patch v3. Will wait for HadoopQA to come back on that patch before committing.
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12775384/HBASE-14901.3.patch
            against master branch at commit da0cc598feab995eed12527d90805dd627674035.
            ATTACHMENT ID: 12775384

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

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

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

            -1 checkstyle. The applied patch generated new checkstyle errors. Check build console for list of new errors.

            +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 lineLengths. The patch does not introduce lines longer than 100

            +1 site. The mvn post-site goal succeeds with this patch.

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

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//testReport/
            Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//artifact/patchprocess/newFindbugsWarnings.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//artifact/patchprocess/checkstyle-aggregate.html

            Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12775384/HBASE-14901.3.patch against master branch at commit da0cc598feab995eed12527d90805dd627674035. ATTACHMENT ID: 12775384 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. -1 checkstyle . The applied patch generated new checkstyle errors. Check build console for list of new errors. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16733//console This message is automatically generated.
            nkedel Nate Edel added a comment -

            Cascading checkstyles errors... added final to the two utility classes. QABot, what say you?

            nkedel Nate Edel added a comment - Cascading checkstyles errors... added final to the two utility classes. QABot, what say you?
            stack Michael Stack added a comment -

            Sorry about that nkedel Looks like your test run got thrown out of the queue because of my messing w/ hadoopqa scripts. Thanks for requeuing.

            stack Michael Stack added a comment - Sorry about that nkedel Looks like your test run got thrown out of the queue because of my messing w/ hadoopqa scripts. Thanks for requeuing.
            nkedel Nate Edel added a comment -

            Didn't re-queue after cancel/subit; re-uploading the same patch (no change from .4) and resubmitting.

            nkedel Nate Edel added a comment - Didn't re-queue after cancel/subit; re-uploading the same patch (no change from .4) and resubmitting.
            hadoopqa Hadoop QA added a comment -

            -1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12775827/HBASE-14901.5.patch
            against master branch at commit 03e4712f0ca08d57586b3fc4d93cf02c999515d8.
            ATTACHMENT ID: 12775827

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

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

            -1 patch. The patch command could not apply the patch.

            Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16767//console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12775827/HBASE-14901.5.patch against master branch at commit 03e4712f0ca08d57586b3fc4d93cf02c999515d8. ATTACHMENT ID: 12775827 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16767//console This message is automatically generated.
            nkedel Nate Edel added a comment -

            Merge in HStore from change yesterday...

            nkedel Nate Edel added a comment - Merge in HStore from change yesterday...
            hadoopqa Hadoop QA added a comment -

            +1 overall. Here are the results of testing the latest attachment
            http://issues.apache.org/jira/secure/attachment/12775846/HBASE-14901.6.patch
            against master branch at commit d18c1af3bb12d6bf1800f22a5289bcb71020bb65.
            ATTACHMENT ID: 12775846

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

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

            +1 hadoop versions. The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1)

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

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

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

            +1 checkstyle. The applied patch does not generate new checkstyle errors.

            +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 lineLengths. The patch does not introduce lines longer than 100

            +1 site. The mvn post-site goal succeeds with this patch.

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

            +1 zombies. No zombie tests found running at the end of the build.

            Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//testReport/
            Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//artifact/patchprocess/newFindbugsWarnings.html
            Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//artifact/patchprocess/checkstyle-aggregate.html

            Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//console

            This message is automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12775846/HBASE-14901.6.patch against master branch at commit d18c1af3bb12d6bf1800f22a5289bcb71020bb65. ATTACHMENT ID: 12775846 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 4 new or modified tests. +1 hadoop versions . The patch compiles with all supported hadoop versions (2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.0 2.6.1 2.7.0 2.7.1) +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 protoc . The applied patch does not increase the total number of protoc compiler warnings. +1 javadoc . The javadoc tool did not generate any warning messages. +1 checkstyle . The applied patch does not generate new checkstyle errors. +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 lineLengths . The patch does not introduce lines longer than 100 +1 site . The mvn post-site goal succeeds with this patch. +1 core tests . The patch passed unit tests in . +1 zombies . No zombie tests found running at the end of the build. Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//testReport/ Release Findbugs (version 2.0.3) warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//artifact/patchprocess/newFindbugsWarnings.html Checkstyle Errors: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//artifact/patchprocess/checkstyle-aggregate.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/16771//console This message is automatically generated.
            ghelmling Gary Helmling added a comment -

            Committed to master. Thanks for the patch, nkedel!

            ghelmling Gary Helmling added a comment - Committed to master. Thanks for the patch, nkedel !
            hudson Hudson added a comment -

            FAILURE: Integrated in HBase-Trunk_matrix #545 (See https://builds.apache.org/job/HBase-Trunk_matrix/545/)
            HBASE-14901 Remove duplicate code to create/manage encryption keys (garyh: rev 9511150bd60e5149856c23c90422e2da7114892e)

            • hbase-server/src/main/java/org/apache/hadoop/hbase/mob/mapreduce/MemStoreWrapper.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
            • hbase-client/src/test/java/org/apache/hadoop/hbase/security/TestEncryptionUtil.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/mob/MobUtils.java
            • hbase-client/src/main/java/org/apache/hadoop/hbase/security/EncryptionUtil.java
            • hbase-server/src/main/java/org/apache/hadoop/hbase/mob/compactions/PartitionedMobCompactor.java
            hudson Hudson added a comment - FAILURE: Integrated in HBase-Trunk_matrix #545 (See https://builds.apache.org/job/HBase-Trunk_matrix/545/ ) HBASE-14901 Remove duplicate code to create/manage encryption keys (garyh: rev 9511150bd60e5149856c23c90422e2da7114892e) hbase-server/src/main/java/org/apache/hadoop/hbase/mob/mapreduce/MemStoreWrapper.java hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java hbase-client/src/test/java/org/apache/hadoop/hbase/security/TestEncryptionUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileReaderImpl.java hbase-server/src/main/java/org/apache/hadoop/hbase/mob/MobUtils.java hbase-client/src/main/java/org/apache/hadoop/hbase/security/EncryptionUtil.java hbase-server/src/main/java/org/apache/hadoop/hbase/mob/compactions/PartitionedMobCompactor.java

            People

              nkedel Nate Edel
              nkedel Nate Edel
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - 2h
                  2h
                  Remaining:
                  Remaining Estimate - 2h
                  2h
                  Logged:
                  Time Spent - Not Specified
                  Not Specified