Details
-
Type:
Improvement
-
Status: Resolved
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 2.7.2
-
Fix Version/s: 2.8.0, 3.0.0-alpha1
-
Component/s: fs/s3
-
Labels:None
-
Target Version/s:
Description
Right now S3Credentials only works with cleartext passwords in configs (as a secret access key or the URI). The non-URI version should use credential providers with a fallback to the clear text option.
Issue Links
- breaks
-
HADOOP-12801
Suppress obsolete S3FileSystem tests.
-
- Resolved
-
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
 -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | pre-patch | 17m 29s | Pre-patch trunk compilation is healthy. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | tests included | 0m 0s | The patch appears to include 2 new or modified test files. |
| +1 | javac | 7m 48s | There were no new javac warning messages. |
| +1 | javadoc | 9m 58s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 23s | The applied patch does not increase the total number of release audit warnings. |
| -1 | checkstyle | 1m 15s | The applied patch generated 1 new checkstyle issues (total was 0, now 1). |
| -1 | whitespace | 0m 1s | The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix. |
| +1 | install | 1m 37s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 33s | The patch built with eclipse:eclipse. |
| +1 | findbugs | 2m 37s | The patch does not introduce any new Findbugs (version 3.0.0) warnings. |
| +1 | common tests | 22m 34s | Tests passed in hadoop-common. |
| +1 | tools/hadoop tests | 0m 15s | Tests passed in hadoop-aws. |
| 64m 47s |
| Subsystem | Report/Notes |
|---|---|
| Patch URL | http://issues.apache.org/jira/secure/attachment/12737407/HADOOP-12059.1.patch |
| Optional Tests | javadoc javac unit findbugs checkstyle |
| git revision | trunk / bc85959 |
| checkstyle | https://builds.apache.org/job/PreCommit-HADOOP-Build/6911/artifact/patchprocess/diffcheckstylehadoop-common.txt |
| whitespace | https://builds.apache.org/job/PreCommit-HADOOP-Build/6911/artifact/patchprocess/whitespace.txt |
| hadoop-common test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/6911/artifact/patchprocess/testrun_hadoop-common.txt |
| hadoop-aws test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/6911/artifact/patchprocess/testrun_hadoop-aws.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/6911/testReport/ |
| Java | 1.7.0_55 |
| uname | Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/6911/console |
This message was automatically generated.
-02
- fixes checkstyle & whitespace errors
- updates LocalJavaKeyStoreProvider so that 0 length files are properly handled as uninitialized
- updates new test to stop working around improper handling of 0 length files in LJKSP
 +1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | pre-patch | 17m 1s | Pre-patch trunk compilation is healthy. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | tests included | 0m 0s | The patch appears to include 2 new or modified test files. |
| +1 | javac | 7m 28s | There were no new javac warning messages. |
| +1 | javadoc | 9m 36s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 22s | The applied patch does not increase the total number of release audit warnings. |
| +1 | checkstyle | 1m 24s | There were no new checkstyle issues. |
| +1 | whitespace | 0m 1s | The patch has no lines that end in whitespace. |
| +1 | install | 1m 36s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 32s | The patch built with eclipse:eclipse. |
| +1 | findbugs | 2m 36s | The patch does not introduce any new Findbugs (version 3.0.0) warnings. |
| +1 | common tests | 22m 25s | Tests passed in hadoop-common. |
| +1 | tools/hadoop tests | 0m 15s | Tests passed in hadoop-aws. |
| 63m 19s |
| Subsystem | Report/Notes |
|---|---|
| Patch URL | http://issues.apache.org/jira/secure/attachment/12737448/HADOOP-12059.2.patch |
| Optional Tests | javadoc javac unit findbugs checkstyle |
| git revision | trunk / b5f0d29 |
| hadoop-common test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/6913/artifact/patchprocess/testrun_hadoop-common.txt |
| hadoop-aws test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/6913/artifact/patchprocess/testrun_hadoop-aws.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/6913/testReport/ |
| Java | 1.7.0_55 |
| uname | Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/6913/console |
This message was automatically generated.
Hi Sean Busbey - Couple comments:
- it seems to me that wrapLocalFileUri should really be added to ProviderUtils rather than to the credential provider itself. There is a bit of a blurred line due to the fact that it creates a localjceks based URI but since it isn't part of the CredentialProvider interface and is basically needed for tests - I think it should go in either the ProviderUtils class or just be part of the test.
- I also just wanted to point out that the local version of the keystore provider is primarily useful when you CAN'T store the keystore in HDFS. For instance, the LDAPGroupsMapping can't use the Hadoop FileSystem abstraction because it causes a recursive infinite loop in order to look up groups to see if you can access the keystore. I just wanted to make sure that you were aware of the regular JavaKeyStoreProvider which allows for local file or hdfs.
it seems to me that wrapLocalFileUri should really be added to ProviderUtils rather than to the credential provider itself. There is a bit of a blurred line due to the fact that it creates a localjceks based URI but since it isn't part of the CredentialProvider interface and is basically needed for tests - I think it should go in either the ProviderUtils class or just be part of the test.
I'm happy to place the helper function where ever y'all would like. However, it's important to note that the wrapper is just implementing the munging described in the javadocs for LJKSP, it's not the same as the one described in ProviderUtils.unnest (though that one is a superset). I figure it ought not be a part of the specific test since it's generally useful for other tests that need to similarly test against a local jks.
Is something like ProviderUtils.nestLocalFileUriInLocalJavaKeyStoreProvider preferable?
I also just wanted to point out that the local version of the keystore provider is primarily useful when you CAN'T store the keystore in HDFS. For instance, the LDAPGroupsMapping can't use the Hadoop FileSystem abstraction because it causes a recursive infinite loop in order to look up groups to see if you can access the keystore. I just wanted to make sure that you were aware of the regular JavaKeyStoreProvider which allows for local file or hdfs.
Yep, I saw it. The test is meant to be lightweight without access to a minicluster, so it seemed best to ensure only local jks would be used.
it seems to me that wrapLocalFileUri should really be added to ProviderUtils rather than to the credential provider itself. There is a bit of a blurred line due to the fact that it creates a localjceks based URI but since it isn't part of the CredentialProvider interface and is basically needed for tests - I think it should go in either the ProviderUtils class or just be part of the test.
I'm happy to place the helper function where ever y'all would like. However, it's important to note that the wrapper is just implementing the munging described in the javadocs for LJKSP, it's not the same as the one described in ProviderUtils.unnest (though that one is a superset). I figure it ought not be a part of the specific test since it's generally useful for other tests that need to similarly test against a local jks.
Is something like ProviderUtils.nestLocalFileUriInLocalJavaKeyStoreProvider preferable?
alternatively, I could mark the method with @VisibleForTesting, though the class is already IA.Private.
Yes, I understand. That's why I said the line is a bit blurry.
If you put it into ProviderUtils something like:
ProviderUtils.nestUriForLocalJavaKeyStoreProvider()
would work for me.
We probably should add a convenience method for the other provider as well - if we add this.
I don't really feel that strongly about it.
The whole idea got me to thinking whether we needed to add a nestUri to the provider interface but then we would need an instance of the provider to get its implementation. Which got me to thinking that it is appropriate to be static - which, in my mind, put us into the ProviderUtils class.
-03
- rebased to current trunk
- moved helper method for nesting file:// under LJKSP to ProviderUtils
| -1 overall |
| Vote | Subsystem | Runtime | Comment |
|---|---|---|---|
| 0 | pre-patch | 16m 56s | Pre-patch trunk compilation is healthy. |
| +1 | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 | tests included | 0m 0s | The patch appears to include 2 new or modified test files. |
| +1 | javac | 7m 32s | There were no new javac warning messages. |
| +1 | javadoc | 9m 42s | There were no new javadoc warning messages. |
| +1 | release audit | 0m 22s | The applied patch does not increase the total number of release audit warnings. |
| +1 | checkstyle | 1m 21s | There were no new checkstyle issues. |
| +1 | whitespace | 0m 1s | The patch has no lines that end in whitespace. |
| +1 | install | 1m 34s | mvn install still works. |
| +1 | eclipse:eclipse | 0m 32s | The patch built with eclipse:eclipse. |
| +1 | findbugs | 2m 34s | The patch does not introduce any new Findbugs (version 3.0.0) warnings. |
| -1 | common tests | 22m 12s | Tests failed in hadoop-common. |
| +1 | tools/hadoop tests | 0m 14s | Tests passed in hadoop-aws. |
| 63m 5s |
| Reason | Tests |
|---|---|
| Failed unit tests | hadoop.security.token.delegation.web.TestWebDelegationToken |
| Subsystem | Report/Notes |
|---|---|
| Patch URL | http://issues.apache.org/jira/secure/attachment/12737982/HADOOP-12059.3.patch |
| Optional Tests | javadoc javac unit findbugs checkstyle |
| git revision | trunk / 6786daa |
| hadoop-common test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/6929/artifact/patchprocess/testrun_hadoop-common.txt |
| hadoop-aws test log | https://builds.apache.org/job/PreCommit-HADOOP-Build/6929/artifact/patchprocess/testrun_hadoop-aws.txt |
| Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/6929/testReport/ |
| Java | 1.7.0_55 |
| uname | Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
| Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/6929/console |
This message was automatically generated.
Tests in error:
TestWebDelegationToken.testIpaddressCheck:997 » Bind Address already in use
the failure in hadoop-common looks like an unrelated concurrency error in unmodified test code. it passes locally just fine and doesn't touch any of this patch.
Agreed - LGTM.
Thanks for including my suggestion!
+1
I'll commit this based on Larry's +1 if Jenkins comes back clean, thanks guys.
Sean pointed out that it already went through test-patch, my bad 
Committed to trunk and branch-2, thanks Sean for the patch and Larry for reviewing!
FAILURE: Integrated in Hadoop-trunk-Commit #7978 (See https://builds.apache.org/job/Hadoop-trunk-Commit/7978/)
HADOOP-12059. S3Credentials should support use of CredentialProvider. Contributed by Sean Busbey. (wang: rev 2dbc40e6086026ef02747223982aa68f2d328ade)
- hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3/TestS3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java
- hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3/S3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java
- .gitignore
- hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalJavaKeyStoreProvider.java
- hadoop-common-project/hadoop-common/CHANGES.txt
FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #220 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/220/)
HADOOP-12059. S3Credentials should support use of CredentialProvider. Contributed by Sean Busbey. (wang: rev 2dbc40e6086026ef02747223982aa68f2d328ade)
- hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java
- hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3/TestS3Credentials.java
- hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3/S3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalJavaKeyStoreProvider.java
- .gitignore
FAILURE: Integrated in Hadoop-Yarn-trunk #950 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/950/)
HADOOP-12059. S3Credentials should support use of CredentialProvider. Contributed by Sean Busbey. (wang: rev 2dbc40e6086026ef02747223982aa68f2d328ade)
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java
- hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java
- .gitignore
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java
- hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3/S3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalJavaKeyStoreProvider.java
- hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3/TestS3Credentials.java
- hadoop-common-project/hadoop-common/CHANGES.txt
FAILURE: Integrated in Hadoop-Hdfs-trunk #2148 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2148/)
HADOOP-12059. S3Credentials should support use of CredentialProvider. Contributed by Sean Busbey. (wang: rev 2dbc40e6086026ef02747223982aa68f2d328ade)
- hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java
- .gitignore
- hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3/S3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalJavaKeyStoreProvider.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3/TestS3Credentials.java
FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #209 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/209/)
HADOOP-12059. S3Credentials should support use of CredentialProvider. Contributed by Sean Busbey. (wang: rev 2dbc40e6086026ef02747223982aa68f2d328ade)
- .gitignore
- hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3/TestS3Credentials.java
- hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java
- hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3/S3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalJavaKeyStoreProvider.java
- hadoop-common-project/hadoop-common/CHANGES.txt
FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #218 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/218/)
HADOOP-12059. S3Credentials should support use of CredentialProvider. Contributed by Sean Busbey. (wang: rev 2dbc40e6086026ef02747223982aa68f2d328ade)
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java
- hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java
- hadoop-common-project/hadoop-common/CHANGES.txt
- .gitignore
- hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3/S3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalJavaKeyStoreProvider.java
- hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3/TestS3Credentials.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java
FAILURE: Integrated in Hadoop-Mapreduce-trunk #2166 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2166/)
HADOOP-12059. S3Credentials should support use of CredentialProvider. Contributed by Sean Busbey. (wang: rev 2dbc40e6086026ef02747223982aa68f2d328ade)
- hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/alias/TestCredentialProviderFactory.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/AbstractJavaKeyStoreProvider.java
- .gitignore
- hadoop-common-project/hadoop-common/CHANGES.txt
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/alias/LocalJavaKeyStoreProvider.java
- hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ProviderUtils.java
- hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3/S3Credentials.java
- hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3/TestS3Credentials.java
It seems like TestS3Credentials#noSecretShouldThrow and TestS3Credentials#noAccessIdShouldThrow have always failed. I went back to the commit that introduced these tests for the first time here in HADOOP-12059, and the tests still failed.
HADOOP-12801 tracks the failure. There is a proposal to delete these 2 tests, since the s3 file system is on a path towards deprecation anyway. If any of the original contributors here on HADOOP-12059 have a particular need to keep those tests around, then please comment on HADOOP-12801. Thank you.
-01
Those last three are not strictly a part of the functionality I set out to enable with this ticket, so if y'all want those broken into a different patch let me know.