Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-6451

NFS should not return NFS3ERR_IO for AccessControlException

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.0
    • Fix Version/s: 2.6.0
    • Component/s: nfs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      As Jing Zhao pointed out in HDFS-6411, we need to catch the AccessControlException from the HDFS calls, and return NFS3ERR_ACCESS instead of NFS3ERR_IO for it.

      Another possible improvement is to have a single class/method for the common exception handling process, instead of repeating the same exception handling process in different NFS methods.

      1. HDFS-6451.patch
        8 kB
        Abhiraj Butala
      2. HDFS-6451.003.patch
        65 kB
        Abhiraj Butala
      3. HDFS-6451.002.patch
        65 kB
        Abhiraj Butala

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1853 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1853/)
          HDFS-6451. NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1853 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1853/ ) HDFS-6451 . NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1828 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1828/)
          HDFS-6451. NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1828 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1828/ ) HDFS-6451 . NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #634 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/634/)
          HDFS-6451. NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #634 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/634/ ) HDFS-6451 . NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #6009 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6009/)
          HDFS-6451. NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702)

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #6009 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6009/ ) HDFS-6451 . NFS should not return NFS3ERR_IO for AccessControlException. Contributed by Abhiraj Butala (brandonli: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1615702 ) /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/test/java/org/apache/hadoop/hdfs/nfs/nfs3/TestRpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          Hide
          Brandon Li added a comment -

          I've committed the patch. Thank you, Abhiraj Butala, for the contribution!

          Show
          Brandon Li added a comment - I've committed the patch. Thank you, Abhiraj Butala , for the contribution!
          Hide
          Brandon Li added a comment -

          +1. The patch looks very nice.

          Show
          Brandon Li added a comment - +1. The patch looks very nice.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12659584/HDFS-6451.003.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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

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

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

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

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs-nfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7550//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7550//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/12659584/HDFS-6451.003.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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs-nfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7550//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7550//console This message is automatically generated.
          Hide
          Abhiraj Butala added a comment -

          Reattaching the patch with findbug warning addressed.

          Show
          Abhiraj Butala added a comment - Reattaching the patch with findbug warning addressed.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12659540/HDFS-6451.002.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 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. There were no new javadoc warning messages.

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

          -1 findbugs. The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs-nfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7547//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/7547//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs-nfs.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7547//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/12659540/HDFS-6451.002.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 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs-nfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7547//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/7547//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs-nfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7547//console This message is automatically generated.
          Hide
          Abhiraj Butala added a comment -

          Forgot to mention, I also cleaned up some white spaces in RpcProgramNf3.java. Please forgive me for that.

          Show
          Abhiraj Butala added a comment - Forgot to mention, I also cleaned up some white spaces in RpcProgramNf3.java. Please forgive me for that.
          Hide
          Abhiraj Butala added a comment -

          Attaching a patch which addresses the review comments by Brandon Li. Added tests for all the handlers in TestRpcProgramNfs3.java. Kept the tests generic, so they can be extended in future to include other tests (various corner cases, other NFS3ERR* messages, etc).

          While testing read() I hit HDFS-6582. I have made a note of this and commented that specific test for now.

          Let me know if there are any suggestions. Thanks!

          Show
          Abhiraj Butala added a comment - Attaching a patch which addresses the review comments by Brandon Li . Added tests for all the handlers in TestRpcProgramNfs3.java. Kept the tests generic, so they can be extended in future to include other tests (various corner cases, other NFS3ERR* messages, etc). While testing read() I hit HDFS-6582 . I have made a note of this and commented that specific test for now. Let me know if there are any suggestions. Thanks!
          Hide
          Abhiraj Butala added a comment -

          Thanks for your feedback Brandon Li. I am currently working on the unit tests and shall upload the patch soon.

          Show
          Abhiraj Butala added a comment - Thanks for your feedback Brandon Li . I am currently working on the unit tests and shall upload the patch soon.
          Hide
          Brandon Li added a comment -

          The patch looks pretty good to me. Some minor comments:
          1. checkIOException() might be better called mapError/mapErrorStatus/mapStatus or something similar
          2. it would be nice to add some unit tests

          Show
          Brandon Li added a comment - The patch looks pretty good to me. Some minor comments: 1. checkIOException() might be better called mapError/mapErrorStatus/mapStatus or something similar 2. it would be nice to add some unit tests
          Hide
          Hadoop QA added a comment -

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

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

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

          +1 javadoc. There were no new javadoc warning messages.

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

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

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

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs-nfs.

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7474//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7474//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/12658118/HDFS-6451.patch against trunk revision . +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs-nfs. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/7474//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/7474//console This message is automatically generated.
          Hide
          Abhiraj Butala added a comment -

          Attaching one version of the fix, which introduces a function to check the IOException type and return the NFS3Status code accordingly.

          Please note that, the function returns NFS3ERR_ACCES for AccessControlException (instead of NFS3ERR_PERM), as that gave 'Permission denied' responses on my setup as shown below (this is also in sync with some of the discussion in HDFS 6411):

          $ ls
          log4j.properties  loglog
          $ mv log4j.properties abc
          mv: cannot move `log4j.properties' to `abc': Permission denied
          $ ln -s log4j.properties abc
          ln: failed to create symbolic link `abc': Permission denied
          $ rm log4j.properties
          rm: remove write-protected regular file `log4j.properties'? y
          rm: cannot remove `log4j.properties': Permission denied
          

          Please let me know if there are any suggestions. Thank you!

          Show
          Abhiraj Butala added a comment - Attaching one version of the fix, which introduces a function to check the IOException type and return the NFS3Status code accordingly. Please note that, the function returns NFS3ERR_ACCES for AccessControlException (instead of NFS3ERR_PERM), as that gave 'Permission denied' responses on my setup as shown below (this is also in sync with some of the discussion in HDFS 6411): $ ls log4j.properties loglog $ mv log4j.properties abc mv: cannot move `log4j.properties' to `abc': Permission denied $ ln -s log4j.properties abc ln: failed to create symbolic link `abc': Permission denied $ rm log4j.properties rm: remove write- protected regular file `log4j.properties'? y rm: cannot remove `log4j.properties': Permission denied Please let me know if there are any suggestions. Thank you!
          Hide
          Brandon Li added a comment -

          From Zhongyi Xie:

          Hi Jing Zhao, it's definitely good to have a single exception handler instead of replicating the same code everywhere, but since each server procedure (ACCESS, GETATTR, FSSTAT, etc) might have their private data that needs to be written out, the child NFS3Response class still need to overload the writeHeaderAndResponse anyways
          for AccessControlException, do you mean we need to catch it together with AuthorizationException in RpcProgramNfs3.java?
          or do you mean we need to examine the whole codebase looking for every function that could potentially throw AccessControlException,
          and make sure the error code is set correctly in the catch clause?

          Show
          Brandon Li added a comment - From Zhongyi Xie : Hi Jing Zhao, it's definitely good to have a single exception handler instead of replicating the same code everywhere, but since each server procedure (ACCESS, GETATTR, FSSTAT, etc) might have their private data that needs to be written out, the child NFS3Response class still need to overload the writeHeaderAndResponse anyways for AccessControlException, do you mean we need to catch it together with AuthorizationException in RpcProgramNfs3.java? or do you mean we need to examine the whole codebase looking for every function that could potentially throw AccessControlException, and make sure the error code is set correctly in the catch clause?

            People

            • Assignee:
              Abhiraj Butala
              Reporter:
              Brandon Li
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development