You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Sergio Pena <se...@cloudera.com> on 2014/12/01 22:35:21 UTC
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63437
-----------------------------------------------------------
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105686>
Do we need to set this value? For what I know, AES/CTR/NoPadding is the only cipher mode that HDFS supports.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105687>
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.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105688>
What if we move this line inside initEncryptionConf()? It is part of encryption initialization.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105689>
- 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.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java
<https://reviews.apache.org/r/28283/#comment105690>
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.
ql/src/test/templates/TestEncrytedHDFSCliDriver.vm
<https://reviews.apache.org/r/28283/#comment105692>
Why do we need this new class instead of TestCliDriver.vm?
shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java
<https://reviews.apache.org/r/28283/#comment105696>
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.
shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java
<https://reviews.apache.org/r/28283/#comment105695>
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.
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java
<https://reviews.apache.org/r/28283/#comment105693>
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.
shims/0.20S/src/main/java/org/apache/hadoop/hive/shims/Hadoop20SShims.java
<https://reviews.apache.org/r/28283/#comment105694>
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.
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/28283/#comment105697>
Let's import the necessary modules only. I think the IDE did this replacement.
shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java
<https://reviews.apache.org/r/28283/#comment105698>
Why was this block removed? I see the keyProvider variable is initialized inside getMiniDfs() method (testing). But what will happen with production code?
- Sergio Pena
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
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by Sergio Pena <se...@cloudera.com>.
> On Dic. 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.
>
> cheng xu wrote:
> 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.
Thanks. I don't think hdfs will add new ciphers in the near future. CTR is the only cipher mode that fits with how files are stored on hdfs, and it is very secure, and AES is the best approved algorithm by PCI & HIPAA these days.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28283/#review63437
-----------------------------------------------------------
On Dic. 2, 2014, 2:58 a.m., cheng xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28283/
> -----------------------------------------------------------
>
> (Updated Dic. 2, 2014, 2:58 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
> 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
>
>
Re: Review Request 28283: HIVE-8900:Create encryption testing
framework
Posted by cheng xu <ch...@intel.com>.
> 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
>
>