hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "cheng xu" <cheng.a...@intel.com>
Subject Re: Review Request 28283: HIVE-8900:Create encryption testing framework
Date Tue, 02 Dec 2014 02:02:17 GMT


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> >

Thanks for your review. Please see my inline comments.


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 268
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line268>
> >
> >     Do we need to set this value? For what I know, AES/CTR/NoPadding is the only
cipher mode that HDFS supports.

Yes, you are right. We can remove it at this point. I add the setter here just in case one
or more cipher will be supported later.


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 365
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line365>
> >
> >     I think this method 'in itEncryptionRelatedConfIfNeeded()' can be called inside
the block line 370
> >     as it is only called when clusterType is encrypted. Also, we may rename the
method for a shorter name as IfNeeded won't be used.

I am afriad not since the initialization of dfs needs the security related properties. To
clean the code, I do a change in this snippet.


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 372
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line372>
> >
> >     What if we move this line inside initEncryptionConf()? It is part of encryption
initialization.

What the initEncryptionConf did is trying to set the security related properties. Another
bigger consideration is that the fs needs the security related configuration and we have to
complete the configuration setting work before the initilazing dfs or hes.


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 754
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line754>
> >
> >     - May we rename this method so that starts with the 'init' verb? This is just
a good pratice I've learned in order
> >       to read code much better. Also, IfNeeded() is the correct syntax.
> >     - We could also get rid of the IfNeeded() word (making the name shorter) if
if add the validation when this method
> >       is called instead of inside the method. It is just an opinion.

Thanks for your suggestion. FIXED it.


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 785
> > <https://reviews.apache.org/r/28283/diff/3/?file=778006#file778006line785>
> >
> >     Just to comment that AES-256 can be used only if JCE is installed in your environment.
Otherwise, any encryption
> >       with this key will fail. Keys can be created, but when you try to encrypt
something, fails. We should put a 
> >       comment here so that another developer knows this.

FIXED


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java, line 872
> > <https://reviews.apache.org/r/28283/diff/3/?file=778009#file778009line872>
> >
> >     I think we should leave the 'hadoop.encryption.is.not.supported' key name on
unsupported hadoop versions. This was left only as a comment for developers. Nobody will use
this configuration key anyways.

FIXED


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, line
497
> > <https://reviews.apache.org/r/28283/diff/3/?file=778010#file778010line497>
> >
> >     I think we should leave the 'hadoop.encryption.is.not.supported' key name on
unsupported hadoop versions. This was left only as a comment for developers. Nobody will use
this configuration key anyways.

FIXED


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java, lines
498-499
> > <https://reviews.apache.org/r/28283/diff/3/?file=778010#file778010line498>
> >
> >     Do we need these two configuration values in the configuration environment?
These are used only for test purposes on QTestUtil. The user won't use these fields on hive-site.xml
ever. Or not yet.

FIXED


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java, lines
960-970
> > <https://reviews.apache.org/r/28283/diff/3/?file=778011#file778011line960>
> >
> >     Why was this block removed? I see the keyProvider variable is initialized inside
getMiniDfs() method (testing). But what will happen with production code?

We should get the key provider via the name node who has already created a key provider. It
has no different for the KMS while not for the java key provider. For java key provider, it
stores the key into a file. And I digged into the code and found that two key providers are
trying to open the same key file and one thread will not be awared of the changed key by another
thread.


> On Dec. 1, 2014, 9:35 p.m., Sergio Pena wrote:
> > ql/src/test/templates/TestEncrytedHDFSCliDriver.vm, line 1
> > <https://reviews.apache.org/r/28283/diff/3/?file=778008#file778008line1>
> >
> >     Why do we need this new class instead of TestCliDriver.vm?

FIXED


- cheng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63437
-----------------------------------------------------------


On Nov. 28, 2014, 1:45 a.m., cheng xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
> 
> (Updated Nov. 28, 2014, 1:45 a.m.)
> 
> 
> Review request for hive.
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> The patch includes:
> 1. enable security properties for hive security cluster
> 
> 
> Diffs
> -----
> 
>   .gitignore c5decaf 
>   data/scripts/q_test_cleanup_for_encryption.sql PRE-CREATION 
>   data/scripts/q_test_init_for_encryption.sql PRE-CREATION 
>   itests/qtest/pom.xml 376f4a9 
>   itests/src/test/resources/testconfiguration.properties 3ae001d 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 31d5c29 
>   ql/src/test/queries/clientpositive/create_encrypted_table.q PRE-CREATION 
>   ql/src/test/templates/TestEncrytedHDFSCliDriver.vm PRE-CREATION 
>   shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java ff7a82c 
>   shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java 2e00d93

>   shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java 8161fc1 
>   shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java fa66a4a 
> 
> Diff: https://reviews.apache.org/r/28283/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> cheng xu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message