Hadoop Common
  1. Hadoop Common
  2. HADOOP-8310

FileContext#checkPath should handle URIs with no port

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.0-alpha, 0.23.4
    • Component/s: fs
    • Labels:
      None

      Description

      AbstractFileSystem#checkPath is used to verify that a given path is for the same file system as represented by the AbstractFileSystem instance.

      The original intent of the code was to allow for no port to be provided in the checked path, if the default port was being used by the AbstractFileSystem instance. However, before performing port handling, AFS#checkPath compares the full URI authorities for equality. Since the URI authority includes the port, the port handling code is never reached, and thus valid paths may be erroneously considered invalid.

      1. HADOOP-8310.patch
        7 kB
        Aaron T. Myers

        Activity

        Hide
        Aaron T. Myers added a comment -

        Here's a patch which fixes the issue, by strictly comparing initially only the host components of the URIs, instead of the full authorities.

        Show
        Aaron T. Myers added a comment - Here's a patch which fixes the issue, by strictly comparing initially only the host components of the URIs, instead of the full authorities.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12523944/HADOOP-8310.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.fs.viewfs.TestViewFsTrash

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/884//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/884//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12523944/HADOOP-8310.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 1 new or modified test files. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.fs.viewfs.TestViewFsTrash +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/884//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/884//console This message is automatically generated.
        Hide
        John George added a comment -

        Good catch Aaron. +1 as long as the test failure is unrelated.

        Show
        John George added a comment - Good catch Aaron. +1 as long as the test failure is unrelated.
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the review, John. I'm confident that the test failure of TestViewFsTrash is unrelated.

        I should also mention that I discovered this issue because without this fix one cannot view the conf page of a running MR2 job in a cluster whose fs.default.name is configured without a port. I manually verified that with this fix, one can get to the page just fine.

        I also ran the full HDFS test suite with this patch applied and everything passed.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the review, John. I'm confident that the test failure of TestViewFsTrash is unrelated. I should also mention that I discovered this issue because without this fix one cannot view the conf page of a running MR2 job in a cluster whose fs.default.name is configured without a port. I manually verified that with this fix, one can get to the page just fine. I also ran the full HDFS test suite with this patch applied and everything passed.
        Hide
        Daryn Sharp added a comment -

        AbstractFileSystem should use the same implementation as FileSystem. This patch doesn't appear to support canonicalization correctly.

        Show
        Daryn Sharp added a comment - AbstractFileSystem should use the same implementation as FileSystem . This patch doesn't appear to support canonicalization correctly.
        Hide
        Aaron T. Myers added a comment -

        AbstractFileSystem should use the same implementation as FileSystem.

        The same implementation of checkPath?

        This patch doesn't appear to support canonicalization correctly.

        I don't follow. This patch doesn't change canonicalization at all.

        Show
        Aaron T. Myers added a comment - AbstractFileSystem should use the same implementation as FileSystem. The same implementation of checkPath? This patch doesn't appear to support canonicalization correctly. I don't follow. This patch doesn't change canonicalization at all.
        Hide
        Daryn Sharp added a comment -

        The same implementation of checkPath?

        Yes, FileSystem#checkPath has code to handle host canonicalization that works in conjunction with host-based tokens. It's an oversight on my part that it didn't make it into FileContext...

        Show
        Daryn Sharp added a comment - The same implementation of checkPath? Yes, FileSystem#checkPath has code to handle host canonicalization that works in conjunction with host-based tokens. It's an oversight on my part that it didn't make it into FileContext ...
        Hide
        John George added a comment -

        @Daryn - do you think that should be handled as part of another JIRA since this specifically addresses a different issue?

        Show
        John George added a comment - @Daryn - do you think that should be handled as part of another JIRA since this specifically addresses a different issue?
        Hide
        Aaron T. Myers added a comment -

        @Daryn - do you think that should be handled as part of another JIRA since this specifically addresses a different issue?

        That would be my preference as well. Making FC use FS's checkPath implementation is a bigger, potentially more destabilizing change. It's also not obvious to me that such a change would be entirely compatible, consdering FS#checkPath currently throws IllegalArgEx, whereas FC throws InvalidPathEx.

        Perhaps we can commit this patch as-is, and open another JIRA to make the two methods share code.

        Is that OK by you, Daryn?

        Show
        Aaron T. Myers added a comment - @Daryn - do you think that should be handled as part of another JIRA since this specifically addresses a different issue? That would be my preference as well. Making FC use FS's checkPath implementation is a bigger, potentially more destabilizing change. It's also not obvious to me that such a change would be entirely compatible, consdering FS#checkPath currently throws IllegalArgEx, whereas FC throws InvalidPathEx. Perhaps we can commit this patch as-is, and open another JIRA to make the two methods share code. Is that OK by you, Daryn?
        Hide
        Daryn Sharp added a comment -

        They actually are related. When I changed the FileSystem#checkPath handling, I also removed DistributedFileSystem#checkPath due to redundancy with the improved FileSystem#checkPath. Part of the change specifically involved having FileSystem#checkPath handle default ports. That change is probably what broke FileContext.

        FileContext really should have identical URI handling to FileSystem, sans the exception types. InvalidPathException derives from IllegalArgumentException so the difference won't matter esp. since FileContext can continue to throw the path exception.

        +0 All that said, I'm very reluctantly willing to let this patch go as-is if there is a followup jira.

        Show
        Daryn Sharp added a comment - They actually are related. When I changed the FileSystem#checkPath handling, I also removed DistributedFileSystem#checkPath due to redundancy with the improved FileSystem#checkPath . Part of the change specifically involved having FileSystem#checkPath handle default ports. That change is probably what broke FileContext . FileContext really should have identical URI handling to FileSystem , sans the exception types. InvalidPathException derives from IllegalArgumentException so the difference won't matter esp. since FileContext can continue to throw the path exception. +0 All that said, I'm very reluctantly willing to let this patch go as-is if there is a followup jira.
        Hide
        Todd Lipcon added a comment -

        +1 for this small fix in lieu of reworking the whole thing.

        Show
        Todd Lipcon added a comment - +1 for this small fix in lieu of reworking the whole thing.
        Hide
        Aaron T. Myers added a comment -

        Thanks a lot for the review, Todd. I've just committed this to trunk and branch-2.

        Daryn: I've filed HADOOP-8320 as a follow-up JIRA.

        Show
        Aaron T. Myers added a comment - Thanks a lot for the review, Todd. I've just committed this to trunk and branch-2. Daryn: I've filed HADOOP-8320 as a follow-up JIRA.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk #1027 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1027/)
        HADOOP-8310. FileContext#checkPath should handle URIs with no port. Contributed by Aaron T. Myers. (Revision 1331007)

        Result = FAILURE
        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331007
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestAfsCheckPath.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1027 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1027/ ) HADOOP-8310 . FileContext#checkPath should handle URIs with no port. Contributed by Aaron T. Myers. (Revision 1331007) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331007 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestAfsCheckPath.java
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Mapreduce-trunk #1062 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1062/)
        HADOOP-8310. FileContext#checkPath should handle URIs with no port. Contributed by Aaron T. Myers. (Revision 1331007)

        Result = FAILURE
        atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331007
        Files :

        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java
        • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestAfsCheckPath.java
        Show
        Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1062 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1062/ ) HADOOP-8310 . FileContext#checkPath should handle URIs with no port. Contributed by Aaron T. Myers. (Revision 1331007) Result = FAILURE atm : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1331007 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestAfsCheckPath.java
        Hide
        Jason Lowe added a comment -

        Thanks Aaron, I pulled this into branch-0.23.

        Show
        Jason Lowe added a comment - Thanks Aaron, I pulled this into branch-0.23.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-0.23-Build #392 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/392/)
        svn merge -c 1331007 FIXES: HADOOP-8310. FileContext#checkPath should handle URIs with no port. Contributed by Aaron T. Myers. (Revision 1392622)

        Result = UNSTABLE
        jlowe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1392622
        Files :

        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java
        • /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestAfsCheckPath.java
        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-0.23-Build #392 (See https://builds.apache.org/job/Hadoop-Hdfs-0.23-Build/392/ ) svn merge -c 1331007 FIXES: HADOOP-8310 . FileContext#checkPath should handle URIs with no port. Contributed by Aaron T. Myers. (Revision 1392622) Result = UNSTABLE jlowe : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1392622 Files : /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/AbstractFileSystem.java /hadoop/common/branches/branch-0.23/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestAfsCheckPath.java

          People

          • Assignee:
            Aaron T. Myers
            Reporter:
            Aaron T. Myers
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development