Ravi Prakash added a comment - 21/Jun/16 19:13 Sorry! I was trying to run the tests, but got distracted before I could figure out how. By the way my handle is raviprak

Hudson added a comment - 21/Jun/16 19:11 SUCCESS: Integrated in Hadoop-trunk-Commit #9996 (See https://builds.apache.org/job/Hadoop-trunk-Commit/9996/ ) HADOOP-13287 . TestS3ACredentials#testInstantiateFromURL fails if AWS (cnauroth: rev b2c596cdda7c129951074bc53b4b9ecfedbf080a) hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3ACredentialsInURL.java

Chris Nauroth added a comment - 21/Jun/16 18:37 Steve, thank you for the review. I committed this to trunk, branch-2 and branch-2.8. ravi , if you do notice any problems when you get around to trying the tests, please let me know.

Steve Loughran added a comment - 20/Jun/16 12:50 (all tests passing; S3 ireland, this time unintentionally VPNed in to the US for extra latency)

Steve Loughran added a comment - 20/Jun/16 12:49 +1 from me; tests passing and verified with a hadoop dist build, hadoop fs -ls URL with the URL containing a / secret. ravi might want to test it too

Hadoop QA added a comment - 19/Jun/16 22:00 +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 27s 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. +1 mvninstall 7m 11s trunk passed +1 compile 0m 15s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 13s trunk passed +1 findbugs 0m 27s trunk passed +1 javadoc 0m 22s trunk passed +1 mvninstall 0m 14s the patch passed +1 compile 0m 12s the patch passed +1 javac 0m 12s the patch passed +1 checkstyle 0m 10s the patch passed +1 mvnsite 0m 16s the patch passed +1 mvneclipse 0m 11s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 34s the patch passed +1 javadoc 0m 11s the patch passed +1 unit 0m 13s hadoop-aws in the patch passed. +1 asflicense 0m 16s The patch does not generate ASF License warnings. 12m 18s Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811731/HADOOP-13287.002.patch JIRA Issue HADOOP-13287 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux c50fe11da9b9 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 Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 0319d73 Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9830/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9830/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Chris Nauroth added a comment - 19/Jun/16 21:48 - edited Thanks for testing, Steve. Could you please try again with patch 002? That assertion needed to be updated to reflect the fact that we're now double-encoding the input, for symmetry with the double-decoding done in the main code. I also added a similar assertion for '+'. I retested against US-west-2 using my secret key with a '+'.

Steve Loughran added a comment - 19/Jun/16 14:29 -1. I'm afrad I'm getting failures in a test, when I use a secret with a / in it testInstantiateFromURL(org.apache.hadoop.fs.s3a.TestS3ACredentialsInURL) Time elapsed: 0.146 sec <<< FAILURE! java.lang.AssertionError: test URI encodes the / symbol at org.junit.Assert.fail(Assert.java:88) at org.junit.Assert.assertTrue(Assert.java:41) at org.apache.hadoop.fs.s3a.TestS3ACredentialsInURL.testInstantiateFromURL(TestS3ACredentialsInURL.java:74) Revert the patch and the test runs The encoded string now contains "%252F"

Hadoop QA added a comment - 17/Jun/16 22:10 +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s 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. +1 mvninstall 7m 52s trunk passed +1 compile 0m 15s trunk passed +1 checkstyle 0m 13s trunk passed +1 mvnsite 0m 18s trunk passed +1 mvneclipse 0m 14s trunk passed +1 findbugs 0m 24s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 12s the patch passed +1 compile 0m 12s the patch passed +1 javac 0m 12s the patch passed +1 checkstyle 0m 10s the patch passed +1 mvnsite 0m 14s the patch passed +1 mvneclipse 0m 9s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 0m 30s the patch passed +1 javadoc 0m 10s the patch passed +1 unit 0m 12s hadoop-aws in the patch passed. +1 asflicense 0m 15s The patch does not generate ASF License warnings. 12m 31s Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12811471/HADOOP-13287.001.patch JIRA Issue HADOOP-13287 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux ec989efeb769 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 Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 2800695 Default Java 1.8.0_91 findbugs v3.0.0 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9820/testReport/ modules C: hadoop-tools/hadoop-aws U: hadoop-tools/hadoop-aws Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9820/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.

Chris Nauroth added a comment - 17/Jun/16 21:52 Reattaching correct patch.

Chris Nauroth added a comment - 17/Jun/16 21:48 - edited My test run was on branch-2.8 against an S3 bucket in US-west-2. What I saw happening was a double decoding in S3xLoginHelper#extractLoginDetails : public static Login extractLoginDetails(URI name) { try { String authority = name.getAuthority(); ... String password = URLDecoder.decode(login.substring(loginSplit + 1), "UTF-8" ); According to the JavaDocs for URI#getAuthority , it performs decoding already on the output. Then we do a second explicit decoding by calling URLDecoder#decode . First, getAuthority translates "%2B" to "+". Then, URLDecoder#decode translates "+" to " ", which isn't correct for the credentials. However, this appear to be only a problem in the JUnit test runs. I also built a distro and tested manually with URIs that contain '+' encoded as "%2B", and that worked just fine. The reason it works fine there is because of different encoding rules applied by round-tripping through a Path before the FileSystem#get call gets triggered. With Path , the '+' gets double-encoded to "%252B", so double-decoding at the S3A layer is correct logic. To make this work, the test should follow the same encoding as would be used on the CLI. The attached path switches from constructing a URI to constructing a Path . I switched the exception stifling logic to catch IllegalArgumentException , becauses that's what Path throws. With this, the test passes with a secret containing a '+'. Steve Loughran or Ravi Prakash , I understand one of you might have a secret with a '/' from your work on HADOOP-3733 . Would you mind testing this patch to make sure the test still passes with '/'?