Hadoop Common
  1. Hadoop Common
  2. HADOOP-10150 Hadoop cryptographic file system
  3. HADOOP-10886

CryptoCodec#getCodecclasses throws NPE when configurations not loaded.

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: fs-encryption (HADOOP-10150 and HDFS-6134)
    • Fix Version/s: 2.6.0
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      There are some test cases which will not load the xml defaults. In this case, CryptoCodec#getCodecclasses will fail with NPE.

      java.lang.NullPointerException: null
              at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:187)
              at com.google.common.base.Splitter.split(Splitter.java:371)
              at org.apache.hadoop.crypto.CryptoCodec.getCodecClasses(CryptoCodec.java:100)
              at org.apache.hadoop.crypto.CryptoCodec.getInstance(CryptoCodec.java:54)
              at org.apache.hadoop.crypto.CryptoCodec.getInstance(CryptoCodec.java:91)
              at org.apache.hadoop.crypto.TestCryptoStreamsForLocalFS.init(TestCryptoStreamsForLocalFS.java:53)
      

      https://builds.apache.org/job/Hadoop-fs-encryption-nightly/71/

      1. HADOOP-10886.patch
        6 kB
        Uma Maheswara Rao G

        Activity

        Hide
        Yi Liu added a comment -

        Thanks Uma. The issue is caused by test case doesn't load the default configuration. Charles Lamb also found org.apache.hadoop.hdfs.TestDistributedFileSystem.testAllWithNoXmlDefaults failed and was caused by same issue, please fix them together.

        Show
        Yi Liu added a comment - Thanks Uma. The issue is caused by test case doesn't load the default configuration. Charles Lamb also found org.apache.hadoop.hdfs.TestDistributedFileSystem.testAllWithNoXmlDefaults failed and was caused by same issue, please fix them together.
        Hide
        Uma Maheswara Rao G added a comment -

        Attached a patch with what I am thinking to fix this issue.
        Andrew Wang, Could you please take a look at this?

        Now CrptoCodec#getInstance may return null in the case of no configuration. We can not throw exception as DFSClient may fail to initialize as we do initialization of codec in DFSClient's Ctor. TestDistributedFileSystem#testAllWithNoXmlDefaults is trying to verify the server basoc with no default xmls loaded. But if there an exception due to no configurations in xml, then dfsClient initialization itself will fail. So, it will mandate to configure that codec related configurations. So,To avoid this, now we simply return null if no configuration available with a log and has javadoc updated about the behaviour.

        Show
        Uma Maheswara Rao G added a comment - Attached a patch with what I am thinking to fix this issue. Andrew Wang , Could you please take a look at this? Now CrptoCodec#getInstance may return null in the case of no configuration. We can not throw exception as DFSClient may fail to initialize as we do initialization of codec in DFSClient's Ctor. TestDistributedFileSystem#testAllWithNoXmlDefaults is trying to verify the server basoc with no default xmls loaded. But if there an exception due to no configurations in xml, then dfsClient initialization itself will fail. So, it will mandate to configure that codec related configurations. So,To avoid this, now we simply return null if no configuration available with a log and has javadoc updated about the behaviour.
        Hide
        Charles Lamb added a comment -

        +1. Thanks Uma.

        Show
        Charles Lamb added a comment - +1. Thanks Uma.
        Hide
        Uma Maheswara Rao G added a comment -

        Thanks a lot for the review. I have just committed this to branch.

        Show
        Uma Maheswara Rao G added a comment - Thanks a lot for the review. I have just committed this to branch.
        Hide
        Andrew Wang added a comment -

        This looks generally okay, just had two comments:

        • There was an unrelated import change in TestDistributedFileSystem
        • Precondition checks are normally for either validating user arguments or the equivalent of runtime assertions. I'd rather have just thrown an IOException in DFSClient. It would have been good for the text to provide some guidance regarding CRYPTO_CIPHER_SUITE and CRYPTO_CODEC_CLASSES_KEY so the user knows what to fix.

        Do you want to do this in a follow-on?

        Show
        Andrew Wang added a comment - This looks generally okay, just had two comments: There was an unrelated import change in TestDistributedFileSystem Precondition checks are normally for either validating user arguments or the equivalent of runtime assertions. I'd rather have just thrown an IOException in DFSClient. It would have been good for the text to provide some guidance regarding CRYPTO_CIPHER_SUITE and CRYPTO_CODEC_CLASSES_KEY so the user knows what to fix. Do you want to do this in a follow-on?
        Hide
        Uma Maheswara Rao G added a comment -

        Thanks a lot, Andrew for the review!
        Sure, I will do that guidance part improvement for Exception message and will change to IOE.
        And for the first comment, I think as part of verification of a test failures in TestDistributedFileSystem, I would have removed that unused import on my usual practice whenever I see unused import. I will leave that comment for now as I already pushed it? ( Not a big change and will not lead to any confusion with that changes)

        Show
        Uma Maheswara Rao G added a comment - Thanks a lot, Andrew for the review! Sure, I will do that guidance part improvement for Exception message and will change to IOE. And for the first comment, I think as part of verification of a test failures in TestDistributedFileSystem, I would have removed that unused import on my usual practice whenever I see unused import. I will leave that comment for now as I already pushed it? ( Not a big change and will not lead to any confusion with that changes)
        Hide
        Andrew Wang added a comment -

        Yep, works for me. I plan to look at the entire diff before we merge to try and shrink the the # of files we touch, so I'll fix TestDistributedFileSystem there too if it makes sense.

        Show
        Andrew Wang added a comment - Yep, works for me. I plan to look at the entire diff before we merge to try and shrink the the # of files we touch, so I'll fix TestDistributedFileSystem there too if it makes sense.

          People

          • Assignee:
            Uma Maheswara Rao G
            Reporter:
            Uma Maheswara Rao G
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development