Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-12345

Pad hostname correctly in CredentialsSys.java

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.6.0, 2.7.0
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: nfs
    • Labels:
      None
    • Target Version/s:

      Description

      Hi -

      There is a bug in the way hadoop-nfs sets the credential length in "Credentials" field of the NFS RPC packet when using AUTH_SYS

      In CredentialsSys.java, when we are writing the creds in to XDR object, we set the length as follows:

      // mStamp + mHostName.length + mHostName + mUID + mGID + mAuxGIDs.count
      96 mCredentialsLength = 20 + mHostName.getBytes().length;

      (20 corresponds to 4 bytes for mStamp, 4 bytes for mUID, 4 bytes for mGID, 4 bytes for length field of hostname, 4 bytes for number of aux 4 gids) and this is okay.

      However when we add the length of the hostname to this, we are not adding the extra padded bytes for the hostname (If the length is not a multiple of 4) and thus when the NFS server reads the packet, it returns GARBAGE_ARGS because it doesn't read the uid field when it is expected to read. I can reproduce this issue constantly on machines where the hostname length is not a multiple of 4.

      A possible fix is to do something this:
      int pad = mHostName.getBytes().length % 4;
      // mStamp + mHostName.length + mHostName + mUID + mGID + mAuxGIDs.count
      mCredentialsLength = 20 + mHostName.getBytes().length + pad;

      I would be happy to submit the patch but I need some help to commit into mainline. I haven't committed into Hadoop yet.

      Cheers!
      Pradeep

      1. HADOOP-12345.patch
        3 kB
        Pradeep Nayak Udupi Kadbet
      2. HADOOP-12345.001.patch
        4 kB
        Pradeep Nayak Udupi Kadbet
      3. HADOOP-12345.002.patch
        5 kB
        Pradeep Nayak Udupi Kadbet

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pradeep1288 commented on the issue:

          https://github.com/apache/hadoop/pull/104

          This has been checked in

          Show
          githubbot ASF GitHub Bot added a comment - Github user pradeep1288 commented on the issue: https://github.com/apache/hadoop/pull/104 This has been checked in
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pradeep1288 closed the pull request at:

          https://github.com/apache/hadoop/pull/104

          Show
          githubbot ASF GitHub Bot added a comment - Github user pradeep1288 closed the pull request at: https://github.com/apache/hadoop/pull/104
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #10040 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10040/)
          HADOOP-12345. Pad hostname correctly in CredentialsSys.java. Contributed (wang: rev 846ada2de33f9840d6e60fd7339ecc2d71f277c7)

          • hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/security/TestCredentialsSys.java
          • hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/security/CredentialsSys.java
          • hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/security/Credentials.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #10040 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10040/ ) HADOOP-12345 . Pad hostname correctly in CredentialsSys.java. Contributed (wang: rev 846ada2de33f9840d6e60fd7339ecc2d71f277c7) hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/security/TestCredentialsSys.java hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/security/CredentialsSys.java hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/security/Credentials.java
          Hide
          andrew.wang Andrew Wang added a comment -

          Thanks for the rev Pradeep, I've committed the patch to trunk, branch-2, branch-2.8 for inclusion in 2.8.0. Appreciate your work here!

          Show
          andrew.wang Andrew Wang added a comment - Thanks for the rev Pradeep, I've committed the patch to trunk, branch-2, branch-2.8 for inclusion in 2.8.0. Appreciate your work here!
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 48s 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 6m 40s trunk passed
          +1 compile 6m 33s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 17s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 22s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 12s the patch passed
          +1 compile 6m 26s the patch passed
          +1 javac 6m 26s the patch passed
          +1 checkstyle 0m 12s the patch passed
          +1 mvnsite 0m 16s the patch passed
          +1 mvneclipse 0m 11s the patch passed
          -1 whitespace 0m 0s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 findbugs 0m 29s the patch passed
          +1 javadoc 0m 13s the patch passed
          +1 unit 0m 22s hadoop-nfs in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          25m 50s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:85209cc
          JIRA Issue HADOOP-12345
          GITHUB PR https://github.com/apache/hadoop/pull/104
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 38b2beecb31d 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 / 5c07c57
          Default Java 1.8.0_91
          findbugs v3.0.0
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9905/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9905/testReport/
          modules C: hadoop-common-project/hadoop-nfs U: hadoop-common-project/hadoop-nfs
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9905/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 48s 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 6m 40s trunk passed +1 compile 6m 33s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 17s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 22s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 12s the patch passed +1 compile 6m 26s the patch passed +1 javac 6m 26s the patch passed +1 checkstyle 0m 12s the patch passed +1 mvnsite 0m 16s the patch passed +1 mvneclipse 0m 11s the patch passed -1 whitespace 0m 0s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 0m 29s the patch passed +1 javadoc 0m 13s the patch passed +1 unit 0m 22s hadoop-nfs in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 25m 50s Subsystem Report/Notes Docker Image:yetus/hadoop:85209cc JIRA Issue HADOOP-12345 GITHUB PR https://github.com/apache/hadoop/pull/104 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 38b2beecb31d 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 / 5c07c57 Default Java 1.8.0_91 findbugs v3.0.0 whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9905/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9905/testReport/ modules C: hadoop-common-project/hadoop-nfs U: hadoop-common-project/hadoop-nfs Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9905/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          Cool. I have addressed your comments.

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - Cool. I have addressed your comments.
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          Consolidated patch after addressing comments from Andrew.

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - Consolidated patch after addressing comments from Andrew.
          Hide
          andrew.wang Andrew Wang added a comment -

          Looks good overall, few more nits:

          • The "m" prefix on variables normally means non-public (e.g. http://source.android.com/source/code-style.html#follow-field-naming-conventions). I understand we need to expose these for testing though, so how about we leave these package-private (rather than public) and also put a @VisibleForTesting annotation on the new methods?
          • This comment needs to be updated since we're doing mod4 rather than if now: // we do not need to compute padding if the hostname is already a multiple of 4
          • Could you also post a consolidated patch on JIRA? The 001 patch doesn't apply by itself to trunk.
          Show
          andrew.wang Andrew Wang added a comment - Looks good overall, few more nits: The "m" prefix on variables normally means non-public (e.g. http://source.android.com/source/code-style.html#follow-field-naming-conventions ). I understand we need to expose these for testing though, so how about we leave these package-private (rather than public) and also put a @VisibleForTesting annotation on the new methods? This comment needs to be updated since we're doing mod4 rather than if now: // we do not need to compute padding if the hostname is already a multiple of 4 Could you also post a consolidated patch on JIRA? The 001 patch doesn't apply by itself to trunk.
          Hide
          hadoopqa Hadoop QA added a comment -
          -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 6m 58s trunk passed
          +1 compile 7m 26s trunk passed
          +1 checkstyle 0m 12s trunk passed
          +1 mvnsite 0m 17s trunk passed
          +1 mvneclipse 0m 11s trunk passed
          +1 findbugs 0m 22s trunk passed
          +1 javadoc 0m 14s trunk passed
          +1 mvninstall 0m 12s the patch passed
          +1 compile 7m 25s the patch passed
          +1 javac 7m 25s the patch passed
          -0 checkstyle 0m 12s hadoop-common-project/hadoop-nfs: The patch generated 1 new + 11 unchanged - 0 fixed = 12 total (was 11)
          +1 mvnsite 0m 20s the patch passed
          +1 mvneclipse 0m 13s the patch passed
          -1 whitespace 0m 0s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix.
          +1 findbugs 0m 37s the patch passed
          +1 javadoc 0m 16s the patch passed
          +1 unit 0m 26s hadoop-nfs in the patch passed.
          +1 asflicense 0m 22s The patch does not generate ASF License warnings.
          27m 39s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:85209cc
          JIRA Issue HADOOP-12345
          GITHUB PR https://github.com/apache/hadoop/pull/104
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 8bf9e45b11d3 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 / 77031a9
          Default Java 1.8.0_91
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-nfs.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/artifact/patchprocess/whitespace-eol.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/testReport/
          modules C: hadoop-common-project/hadoop-nfs U: hadoop-common-project/hadoop-nfs
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -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 6m 58s trunk passed +1 compile 7m 26s trunk passed +1 checkstyle 0m 12s trunk passed +1 mvnsite 0m 17s trunk passed +1 mvneclipse 0m 11s trunk passed +1 findbugs 0m 22s trunk passed +1 javadoc 0m 14s trunk passed +1 mvninstall 0m 12s the patch passed +1 compile 7m 25s the patch passed +1 javac 7m 25s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-nfs: The patch generated 1 new + 11 unchanged - 0 fixed = 12 total (was 11) +1 mvnsite 0m 20s the patch passed +1 mvneclipse 0m 13s the patch passed -1 whitespace 0m 0s The patch has 7 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 findbugs 0m 37s the patch passed +1 javadoc 0m 16s the patch passed +1 unit 0m 26s hadoop-nfs in the patch passed. +1 asflicense 0m 22s The patch does not generate ASF License warnings. 27m 39s Subsystem Report/Notes Docker Image:yetus/hadoop:85209cc JIRA Issue HADOOP-12345 GITHUB PR https://github.com/apache/hadoop/pull/104 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 8bf9e45b11d3 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 / 77031a9 Default Java 1.8.0_91 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-nfs.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/artifact/patchprocess/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/testReport/ modules C: hadoop-common-project/hadoop-nfs U: hadoop-common-project/hadoop-nfs Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9895/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          I have attached the new version of the patch here.

          Also added test cases for hostnames being multiple of 4 and not being multiple of 4. I need to add a couple of public methods to the class for this to happen.

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - I have attached the new version of the patch here. Also added test cases for hostnames being multiple of 4 and not being multiple of 4. I need to add a couple of public methods to the class for this to happen.
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          Updated patch file after addressing review comments from Andrew

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - Updated patch file after addressing review comments from Andrew
          Hide
          andrew.wang Andrew Wang added a comment -

          I normally just review the patch on JIRA, so you can just post it here.

          Show
          andrew.wang Andrew Wang added a comment - I normally just review the patch on JIRA, so you can just post it here.
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          Ok, I will put the patch file here after the code review. Does that sound okay ?

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - Ok, I will put the patch file here after the code review. Does that sound okay ?
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          Oops! I meant to say len % 4 != 0. I will update the pull request with the latest change. I will incorporate your suggestion.

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - Oops! I meant to say len % 4 != 0. I will update the pull request with the latest change. I will incorporate your suggestion.
          Hide
          andrew.wang Andrew Wang added a comment -

          Also, do you mind posting a patch here on JIRA? We use github optionally for code review, but don't have our precommit bot watching github PRs.

          Show
          andrew.wang Andrew Wang added a comment - Also, do you mind posting a patch here on JIRA? We use github optionally for code review, but don't have our precommit bot watching github PRs.
          Hide
          andrew.wang Andrew Wang added a comment -

          Looked at the PR. Can we add a test for the length % 4 == 0 case? I think the current code is not quite right:

          +    int padding = 0;
          +    // we do not need compute padding if the hostname is already a multiple of 4
          +    if (mHostName.getBytes(Charsets.UTF_8).length != 0) {
          +      padding = 4 - (mHostName.getBytes(Charsets.UTF_8).length % 4);
          +    }
          

          I think you meant to check that the len % 4 != 0. I think it'd be even better though if we just %4 the padding one more time, saves the if statement.

          Also we only need that comment about the padding once, can delete the second one.

          Show
          andrew.wang Andrew Wang added a comment - Looked at the PR. Can we add a test for the length % 4 == 0 case? I think the current code is not quite right: + int padding = 0; + // we do not need compute padding if the hostname is already a multiple of 4 + if (mHostName.getBytes(Charsets.UTF_8).length != 0) { + padding = 4 - (mHostName.getBytes(Charsets.UTF_8).length % 4); + } I think you meant to check that the len % 4 != 0. I think it'd be even better though if we just %4 the padding one more time, saves the if statement. Also we only need that comment about the padding once, can delete the second one.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user pradeep1288 opened a pull request:

          https://github.com/apache/hadoop/pull/104

          HADOOP-12345: Compute the correct credential length

          I had to discard my earlier pull request as that included fix for the jira HADOOP-11823, hence creating a separate one for each of them.

          The fix here addresses the correct credential length to be computed. We need to round of the machine name length to the next multiple of 4, else using such a credential in the NFS RPC request will result in GARBAGE_ARGS from the NFS server. See RFC-5531, page 24 and page 8

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/pradeep1288/hadoop trunk

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/hadoop/pull/104.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #104


          commit db234209007d32c877aae8ed9a1a083174631ce4
          Author: Pradeep Nayak <pradeepu@netapp.com>
          Date: 2016-06-23T18:10:46Z

          HADOOP-12345: Compute the correct credential length


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user pradeep1288 opened a pull request: https://github.com/apache/hadoop/pull/104 HADOOP-12345 : Compute the correct credential length I had to discard my earlier pull request as that included fix for the jira HADOOP-11823 , hence creating a separate one for each of them. The fix here addresses the correct credential length to be computed. We need to round of the machine name length to the next multiple of 4, else using such a credential in the NFS RPC request will result in GARBAGE_ARGS from the NFS server. See RFC-5531, page 24 and page 8 You can merge this pull request into a Git repository by running: $ git pull https://github.com/pradeep1288/hadoop trunk Alternatively you can review and apply these changes as the patch at: https://github.com/apache/hadoop/pull/104.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #104 commit db234209007d32c877aae8ed9a1a083174631ce4 Author: Pradeep Nayak <pradeepu@netapp.com> Date: 2016-06-23T18:10:46Z HADOOP-12345 : Compute the correct credential length
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pradeep1288 closed the pull request at:

          https://github.com/apache/hadoop/pull/103

          Show
          githubbot ASF GitHub Bot added a comment - Github user pradeep1288 closed the pull request at: https://github.com/apache/hadoop/pull/103
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user pradeep1288 commented on the issue:

          https://github.com/apache/hadoop/pull/103

          I will separate out the changes for HADOOP-12345 and HADOOP-11823 and submit two pull requests

          Show
          githubbot ASF GitHub Bot added a comment - Github user pradeep1288 commented on the issue: https://github.com/apache/hadoop/pull/103 I will separate out the changes for HADOOP-12345 and HADOOP-11823 and submit two pull requests
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Pradeep,

          Based on my recollection from reading the spec the last time, your patch looks correct, I just couldn't figure out where in the HDFS NFS gateway this was actually being used. What I'm ideally looking for is a unit test with the HDFS NFS gateway. If this is difficult, I'd appreciate if you could comment on manual reproduction and testing.

          Show
          andrew.wang Andrew Wang added a comment - Hi Pradeep, Based on my recollection from reading the spec the last time, your patch looks correct, I just couldn't figure out where in the HDFS NFS gateway this was actually being used. What I'm ideally looking for is a unit test with the HDFS NFS gateway. If this is difficult, I'd appreciate if you could comment on manual reproduction and testing.
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          Andrew -

          You would need a NFS-server to do the nfs server level test. I can point to the RFC where this is mentioned and comment on what is the error with which the NFS server would respond. Would that suffice ?

          I will separate out the changes for HADOOP-11823 and make that as a separate fix. I will try adding a unit test for that as well

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - Andrew - You would need a NFS-server to do the nfs server level test. I can point to the RFC where this is mentioned and comment on what is the error with which the NFS server would respond. Would that suffice ? I will separate out the changes for HADOOP-11823 and make that as a separate fix. I will try adding a unit test for that as well
          Hide
          andrew.wang Andrew Wang added a comment -

          Hi Pradeep, thanks for working on this,

          Is it possible to add an NFS-server level test? Or could you comment on manual repoduction and testing here?

          I'd also prefer to keep these two patches separate, since they seem to be different issues. I think you can reassign HADOOP-11823 to yourself, since Brandon doesn't seem to be actively working on it. This would also need a unit test. Thanks.

          Show
          andrew.wang Andrew Wang added a comment - Hi Pradeep, thanks for working on this, Is it possible to add an NFS-server level test? Or could you comment on manual repoduction and testing here? I'd also prefer to keep these two patches separate, since they seem to be different issues. I think you can reassign HADOOP-11823 to yourself, since Brandon doesn't seem to be actively working on it. This would also need a unit test. Thanks.
          Hide
          hadoopqa Hadoop QA added a comment -
          +1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 23s 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 6m 44s trunk passed
          +1 compile 6m 35s trunk passed
          +1 checkstyle 0m 14s trunk passed
          +1 mvnsite 0m 16s trunk passed
          +1 mvneclipse 0m 12s trunk passed
          +1 findbugs 0m 25s trunk passed
          +1 javadoc 0m 13s trunk passed
          +1 mvninstall 0m 12s the patch passed
          +1 compile 6m 51s the patch passed
          +1 javac 6m 51s the patch passed
          -0 checkstyle 0m 12s hadoop-common-project/hadoop-nfs: The patch generated 1 new + 9 unchanged - 0 fixed = 10 total (was 9)
          +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 29s the patch passed
          +1 javadoc 0m 13s the patch passed
          +1 unit 0m 22s hadoop-nfs in the patch passed.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          24m 51s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:e2f6409
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12812580/HADOOP-12345.patch
          JIRA Issue HADOOP-12345
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 785d889b7b0f 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 / 17eae9e
          Default Java 1.8.0_91
          findbugs v3.0.0
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9854/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-nfs.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9854/testReport/
          modules C: hadoop-common-project/hadoop-nfs U: hadoop-common-project/hadoop-nfs
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9854/console
          Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 23s 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 6m 44s trunk passed +1 compile 6m 35s trunk passed +1 checkstyle 0m 14s trunk passed +1 mvnsite 0m 16s trunk passed +1 mvneclipse 0m 12s trunk passed +1 findbugs 0m 25s trunk passed +1 javadoc 0m 13s trunk passed +1 mvninstall 0m 12s the patch passed +1 compile 6m 51s the patch passed +1 javac 6m 51s the patch passed -0 checkstyle 0m 12s hadoop-common-project/hadoop-nfs: The patch generated 1 new + 9 unchanged - 0 fixed = 10 total (was 9) +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 29s the patch passed +1 javadoc 0m 13s the patch passed +1 unit 0m 22s hadoop-nfs in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 24m 51s Subsystem Report/Notes Docker Image:yetus/hadoop:e2f6409 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12812580/HADOOP-12345.patch JIRA Issue HADOOP-12345 Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 785d889b7b0f 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 / 17eae9e Default Java 1.8.0_91 findbugs v3.0.0 checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/9854/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-nfs.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/9854/testReport/ modules C: hadoop-common-project/hadoop-nfs U: hadoop-common-project/hadoop-nfs Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/9854/console Powered by Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user pradeep1288 opened a pull request:

          https://github.com/apache/hadoop/pull/103

          HADOOP-12345,HADOOP-11823 fixes

          I have already filed the HADOOP-12345 and HADOOP-11823 jira.

          The fix for HADOOP-12345 is to correctly compute the credential length which is passed as part of the NFS request. We need to round the XDR bytes to a multiple of 4 if the hostname in the credential is not a multiple of 4.

          The fix for HADOOP-11823 is when RPC returns a denied reply, the code should not check for a verifier. It is a bug as it doesn't match the RPC protocol. (See Page 33 from NFS Illustrated book).

          This is my first time contributing to Hadoop.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/pradeep1288/hadoop trunk

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/hadoop/pull/103.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #103


          commit b65f2fb06263cb393cafc8aa5a04ddfe55851331
          Author: Pradeep Nayak <pradeepu@netapp.com>
          Date: 2016-06-22T18:47:20Z

          Fix for HADOOP-12345,HADOOP-11823 jira's


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user pradeep1288 opened a pull request: https://github.com/apache/hadoop/pull/103 HADOOP-12345 , HADOOP-11823 fixes I have already filed the HADOOP-12345 and HADOOP-11823 jira. The fix for HADOOP-12345 is to correctly compute the credential length which is passed as part of the NFS request. We need to round the XDR bytes to a multiple of 4 if the hostname in the credential is not a multiple of 4. The fix for HADOOP-11823 is when RPC returns a denied reply, the code should not check for a verifier. It is a bug as it doesn't match the RPC protocol. (See Page 33 from NFS Illustrated book). This is my first time contributing to Hadoop. You can merge this pull request into a Git repository by running: $ git pull https://github.com/pradeep1288/hadoop trunk Alternatively you can review and apply these changes as the patch at: https://github.com/apache/hadoop/pull/103.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #103 commit b65f2fb06263cb393cafc8aa5a04ddfe55851331 Author: Pradeep Nayak <pradeepu@netapp.com> Date: 2016-06-22T18:47:20Z Fix for HADOOP-12345 , HADOOP-11823 jira's
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          I have attached the patch for HADOOP-12345 and HADOOP-11823. Sorry for the confusion. Using Jira for the first time. The patch file is attached.

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - I have attached the patch for HADOOP-12345 and HADOOP-11823 . Sorry for the confusion. Using Jira for the first time. The patch file is attached.
          Hide
          pradeepu Pradeep Nayak Udupi Kadbet added a comment -

          The patch for both HADOOP-12345 and HADOOP-11823. Please let me know if I need to separate them out ?

          Show
          pradeepu Pradeep Nayak Udupi Kadbet added a comment - The patch for both HADOOP-12345 and HADOOP-11823 . Please let me know if I need to separate them out ?
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Not much going on here for a long time, dropping from 2.8.0.

          Not putting any target-version either anymore, let's target this depending on when there is patch activity.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Not much going on here for a long time, dropping from 2.8.0. Not putting any target-version either anymore, let's target this depending on when there is patch activity.
          Hide
          andrew.wang Andrew Wang added a comment -

          I'm downgrading this from a blocker since it's not a regression.

          I also spent a little time trying to make a repro. I'm not that familiar with the NFS gateway, and my IDE didn't see a place where mCredentialsLength was being used. I think this requires an actual NFS gateway unit test, which presumably uses this length somewhere in the RPC code.

          This is one is probably best handled by one of the NFS gateway experts, e.g. Brandon Li.

          Show
          andrew.wang Andrew Wang added a comment - I'm downgrading this from a blocker since it's not a regression. I also spent a little time trying to make a repro. I'm not that familiar with the NFS gateway, and my IDE didn't see a place where mCredentialsLength was being used. I think this requires an actual NFS gateway unit test, which presumably uses this length somewhere in the RPC code. This is one is probably best handled by one of the NFS gateway experts, e.g. Brandon Li .
          Hide
          rchiang Ray Chiang added a comment -

          Given that no patch has been submitted for 8 months, I'd recommend changing Target Version to 2.9.0 or getting something up for review ASAP.

          Show
          rchiang Ray Chiang added a comment - Given that no patch has been submitted for 8 months, I'd recommend changing Target Version to 2.9.0 or getting something up for review ASAP.
          Hide
          ajisakaa Akira Ajisaka added a comment -

          Is there anyone working on this issue?

          Show
          ajisakaa Akira Ajisaka added a comment - Is there anyone working on this issue?
          Hide
          Pradeep.Nayak@netapp.com UdupiKadbet, PradeepNayak added a comment -

          I am actually working on this

          Pradeep

          Show
          Pradeep.Nayak@netapp.com UdupiKadbet, PradeepNayak added a comment - I am actually working on this Pradeep
          Hide
          mamun S M Al Mamun added a comment -

          Is there anyone actively working on this? I was planning to commit a patch for this issue.

          Show
          mamun S M Al Mamun added a comment - Is there anyone actively working on this? I was planning to commit a patch for this issue.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Also 2.6.0/2.7.0/2.7.1 are released, targeting for 2.8.0.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Also 2.6.0/2.7.0/2.7.1 are released, targeting for 2.8.0.
          Hide
          vinodkv Vinod Kumar Vavilapalli added a comment -

          Pradeep Nayak Udupi Kadbet, please use "Target Version" only for your intention; fix-version is used by committers once a patch gets committed.

          Show
          vinodkv Vinod Kumar Vavilapalli added a comment - Pradeep Nayak Udupi Kadbet , please use "Target Version" only for your intention; fix-version is used by committers once a patch gets committed.
          Hide
          chris.douglas Chris Douglas added a comment -

          I would be happy to submit the patch but I need some help to commit into mainline. I haven't committed into Hadoop yet.

          The contrib docs are here. Thanks for looking into this!

          Show
          chris.douglas Chris Douglas added a comment - I would be happy to submit the patch but I need some help to commit into mainline. I haven't committed into Hadoop yet. The contrib docs are here . Thanks for looking into this!

            People

            • Assignee:
              pradeepu Pradeep Nayak Udupi Kadbet
              Reporter:
              pradeepu Pradeep Nayak Udupi Kadbet
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Due:
                Created:
                Updated:
                Resolved:

                Development