Details
-
Type:
Improvement
-
Status: Resolved
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 2.6.5
-
Fix Version/s: 2.9.0, 3.0.0-alpha4
-
Component/s: encryption
-
Labels:None
-
Target Version/s:
Description
As discovered on HADOOP-14333, Hive is using reflection to get a DFSClient for its encryption shim. We should provide proper public APIs for getting this information.
Issue Links
- is depended upon by
-
HIVE-16490
Hive should not use private HDFS APIs for encryption
-
- Open
-
- is related to
-
HIVE-16047
Shouldn't try to get KeyProvider unless encryption is enabled
-
- Resolved
-
- relates to
-
HDFS-11689
New exception thrown by DFSClient#isHDFSEncryptionEnabled broke hacky hive code
-
- Resolved
-
-
HADOOP-14104
Client should always ask namenode for kms provider path.
-
- Resolved
-
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
I'd like to get Hive off of DistributedFileSystem and DFSClient in Hadoop23Shims if possible. DFS is also a private API, and there's a lot going on here that makes me uncomfortable as an HDFS developer. This is additional work beyond just the immediate scope of this JIRA.
As a start, I think we should expose getKeyProvider in HDFSAdmin. isHdfsEncryptionEnabled is just a check for if getKeyProvider is null, so we don't need to expose that too.
As part of this, we should also see if the ERROR logging mentioned in HIVE-16047 and HDFS-7931 needs to be quashed some more.
Thanks Naveen Gangam. Besides isHDFSEncryptionEnabled, we also need APIs related to KeyProvider. Ferdinand Xu, Xuefu Zhang I think you know better about this.
The encryption related APIs are invoked in
HdfsEncryptionShim
in Hive side. APIs come as follows:
HdfsAdmin.getEncryptionZoneForPath(Path path) HdfsAdmin.createEncryptionZone(Path path, String keyName)
.
As Rui Li mentioned, we need a way to obtain KeyProvider instance like dfs.getClient().getKeyProvider()
Sergio Peña, do you have further comments for the API?
Add HdfsAdmin#getKeyProvider and change the visibility of DFSClient#isEncryptionEnabled to project-privated to avoid future misuse.
 -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 33s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
| 0 | mvndep | 0m 23s | Maven dependency ordering for branch |
| +1 | mvninstall | 14m 22s | trunk passed |
| +1 | compile | 1m 29s | trunk passed |
| +1 | checkstyle | 0m 42s | trunk passed |
| +1 | mvnsite | 1m 26s | trunk passed |
| +1 | mvneclipse | 0m 30s | trunk passed |
| -1 | findbugs | 1m 25s | hadoop-hdfs-project/hadoop-hdfs-client in trunk has 2 extant Findbugs warnings. |
| -1 | findbugs | 1m 46s | hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. |
| +1 | javadoc | 1m 7s | trunk passed |
| 0 | mvndep | 0m 7s | Maven dependency ordering for patch |
| +1 | mvninstall | 1m 34s | the patch passed |
| +1 | compile | 1m 49s | the patch passed |
| +1 | javac | 1m 49s | the patch passed |
| -0 | checkstyle | 0m 45s | hadoop-hdfs-project: The patch generated 1 new + 98 unchanged - 0 fixed = 99 total (was 98) |
| +1 | mvnsite | 1m 39s | the patch passed |
| +1 | mvneclipse | 0m 30s | the patch passed |
| +1 | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 | findbugs | 3m 53s | the patch passed |
| -1 | javadoc | 0m 21s | hadoop-hdfs-project_hadoop-hdfs-client generated 4 new + 1 unchanged - 0 fixed = 5 total (was 1) |
| +1 | unit | 1m 16s | hadoop-hdfs-client in the patch passed. |
| -1 | unit | 88m 43s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 20s | The patch does not generate ASF License warnings. |
| 127m 3s |
| Reason | Tests |
|---|---|
| Failed junit tests | hadoop.hdfs.server.balancer.TestBalancer |
| hadoop.hdfs.server.namenode.TestMetadataVersionOutput | |
| hadoop.hdfs.server.blockmanagement.TestComputeInvalidateWork | |
| hadoop.hdfs.server.namenode.TestStartup | |
| hadoop.hdfs.server.namenode.TestNameNodeMetadataConsistency |
This message was automatically generated.
Thanks Lei (Eddy) Xu for the patch.
The patch is v simple.
After this patch, we should also undo the changes done by HDFS-11689.
Could you also include that in your patch ?
Thanks for the suggestion, Rushabh S Shah. I reverted the changes in HDFS-11689. Also fixed the checkstyle warnings.
The failures are not relevant.
+1 ltgm. (non-binding)
Thanks Lei (Eddy) Xu !
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 22s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
| 0 | mvndep | 0m 31s | Maven dependency ordering for branch |
| +1 | mvninstall | 15m 52s | trunk passed |
| +1 | compile | 1m 52s | trunk passed |
| +1 | checkstyle | 0m 50s | trunk passed |
| +1 | mvnsite | 1m 50s | trunk passed |
| +1 | mvneclipse | 0m 39s | trunk passed |
| -1 | findbugs | 1m 25s | hadoop-hdfs-project/hadoop-hdfs-client in trunk has 2 extant Findbugs warnings. |
| -1 | findbugs | 1m 40s | hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. |
| +1 | javadoc | 1m 2s | trunk passed |
| 0 | mvndep | 0m 7s | Maven dependency ordering for patch |
| +1 | mvninstall | 1m 20s | the patch passed |
| +1 | compile | 1m 22s | the patch passed |
| +1 | javac | 1m 22s | hadoop-hdfs-project generated 0 new + 55 unchanged - 1 fixed = 55 total (was 56) |
| +1 | checkstyle | 0m 39s | the patch passed |
| +1 | mvnsite | 1m 21s | the patch passed |
| +1 | mvneclipse | 0m 22s | the patch passed |
| +1 | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 | findbugs | 3m 11s | the patch passed |
| +1 | javadoc | 0m 57s | the patch passed |
| +1 | unit | 1m 9s | hadoop-hdfs-client in the patch passed. |
| +1 | unit | 65m 49s | hadoop-hdfs in the patch passed. |
| +1 | asflicense | 0m 25s | The patch does not generate ASF License warnings. |
| 104m 13s |
| Subsystem | Report/Notes |
|---|---|
| Docker | Image:yetus/hadoop:14b5c93 |
| JIRA Issue | |
| JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12866258/HDFS-11687.01.patch |
| Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
| uname | Linux 9c0fa5f827f3 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
| git revision | trunk / 83dded5 |
| Default Java | 1.8.0_121 |
| findbugs | v3.1.0-RC1 |
| findbugs | https://builds.apache.org/job/PreCommit-HDFS-Build/19305/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-client-warnings.html |
| findbugs | https://builds.apache.org/job/PreCommit-HDFS-Build/19305/artifact/patchprocess/branch-findbugs-hadoop-hdfs-project_hadoop-hdfs-warnings.html |
| Test Results | https://builds.apache.org/job/PreCommit-HDFS-Build/19305/testReport/ |
| modules | C: hadoop-hdfs-project/hadoop-hdfs-client hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project |
| Console output | https://builds.apache.org/job/PreCommit-HDFS-Build/19305/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
The findbugs warnings are not relevant to this change. Could you give a review, Andrew Wang?
LGTM +1, I checked that calling HdfsAdmin#getKeyProvider without a KP set doesn't result in any logging too. Thanks for working on this Eddy!
One additional cooment, it'd be nice to also add some additional unit testing to TestHdfsAdmin.
Hi, Andrew Wang Thanks for the review, I added the test in TestHdfsAdmin as suggested.
Please take another review. Thanks!
It would be nice if someone from hive team can review it and if they need something more from hadoop then it would be nice to add now itself instead of creating another jira.
Cc Ferdinand Xu, Xuefu Zhang.
Remove DFSConfigKeys.DFS_NAMENODE_DELEGATION_TOKEN_ALWAYS_USE_KEY from test.
+1 LGTM, thanks Eddy!
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | reexec | 0m 17s | Docker mode activated. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | test4tests | 0m 0s | The patch appears to include 2 new or modified test files. |
| 0 | mvndep | 0m 25s | Maven dependency ordering for branch |
| +1 | mvninstall | 13m 0s | trunk passed |
| +1 | compile | 1m 20s | trunk passed |
| +1 | checkstyle | 0m 42s | trunk passed |
| +1 | mvnsite | 1m 22s | trunk passed |
| +1 | mvneclipse | 0m 30s | trunk passed |
| -1 | findbugs | 1m 15s | hadoop-hdfs-project/hadoop-hdfs-client in trunk has 2 extant Findbugs warnings. |
| -1 | findbugs | 1m 30s | hadoop-hdfs-project/hadoop-hdfs in trunk has 10 extant Findbugs warnings. |
| +1 | javadoc | 1m 3s | trunk passed |
| 0 | mvndep | 0m 8s | Maven dependency ordering for patch |
| +1 | mvninstall | 1m 14s | the patch passed |
| +1 | compile | 1m 15s | the patch passed |
| +1 | javac | 1m 15s | hadoop-hdfs-project generated 0 new + 55 unchanged - 1 fixed = 55 total (was 56) |
| -0 | checkstyle | 0m 37s | hadoop-hdfs-project: The patch generated 1 new + 117 unchanged - 0 fixed = 118 total (was 117) |
| +1 | mvnsite | 1m 16s | the patch passed |
| +1 | mvneclipse | 0m 23s | the patch passed |
| +1 | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 | findbugs | 2m 56s | the patch passed |
| +1 | javadoc | 0m 56s | the patch passed |
| +1 | unit | 1m 11s | hadoop-hdfs-client in the patch passed. |
| -1 | unit | 62m 29s | hadoop-hdfs in the patch failed. |
| +1 | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
| 95m 35s |
| Reason | Tests |
|---|---|
| Failed junit tests | hadoop.hdfs.web.TestWebHdfsTimeouts |
This message was automatically generated.
+1 ltgm. (non-binding)
Cc Sergio Peña
Committed to trunk and branch-2.
Thanks for the reviews, Andrew Wang, Rushabh S Shah, Ferdinand Xu.
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11684 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11684/)
HDFS-11687. Add new public encryption APIs required by Hive. (lei) (lei: rev 25f5d9ad5ee5ead349d259a99b49541a70b1604d)
- (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
- (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java
- (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java
- (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestHdfsAdmin.java
Lei (Eddy) Xu Seems like we have come full circle with this fix. Prior to HIVE-16047, hive code was already using the getKeyProvider API. However, each time it was called when encryption was disabled, there was a recurring message in HiveServer logs about hdfs.KeyProviderCache: Could not find uri with key [dfs.encryption.key.provider.uri] to create a keyProvider !! . The goal of HIVE-16047 was to address this log message.
So the fix included a call to DFSClient.isHDFSEncryptionEnabled(), which is now deleted in favor of HdfsAdmin.getKeyProvider() which appears to use the same underlying getKeyProvider(). Wouldnt we see the same log message again if we use HdfsAdmin.getKeyProvider() ? Thanks
Wouldnt we see the same log message again if we use HdfsAdmin.getKeyProvider()
Naveen Gangam: Why don't you do a null check before logging.
If encryption is disabled, HDFSAdmin#getKeyProvider will return null.
Lei (Eddy) Xu: We missed committing this patch to branch-2.8.
Can you please commit to branch-2.8 also.
Thanks Rushabh S Shah I believe this is being logged by the HDFS clientside code. I can check the hive codebase to confirm.
Rushabh S Shah Confirmed. No references in hive code. Appears this is the code, it initiates from
http://grepcode.com/file/repo1.maven.org/maven2/org.apache.hadoop/hadoop-hdfs/2.7.0/org/apache/hadoop/hdfs/KeyProviderCache.java#87
Could this have addressed it ? https://issues.apache.org/jira/browse/HDFS-7931 Thanks
Rushabh S Shah Backported to branch-2.8.
Hi Lei (Eddy) Xu, would you backport this to branch-2.8.1 as well?
Andrew Wang Is the proposal to add isHDFSEncryptionEnabled() to HdfsAdmin API? Will this be available on pre-2.8.1 releases? (guessing not given its a public API). Thanks