Hadoop Common
  1. Hadoop Common
  2. HADOOP-7104

Remove unnecessary DNS reverse lookups from RPC layer

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.22.0
    • Component/s: ipc, security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      RPC connection authorization needs to verify client's Kerberos principal name matches what specified for the protocol. For service clients like DN's, their Kerberos principal names can be specified in the form of "datanode/_HOST@DOMAIN.COM". To get the expected
      client principal name, the server needs to substitute "_HOST" with the client's fully qualified domain name, which requires a reverse DNS lookup from client IP address. However, for connections from clients whose principal name are either unspecified or specified not using the "_HOST" convention, the substitution is not required and the reverse DNS lookup should be avoided. Currently the reverse DNS lookup is done for all clients, which could slow services like NN down, when local named cache is not available.

      1. 7104-few-edits.patch
        14 kB
        Todd Lipcon
      2. c7104-01.patch
        12 kB
        Kan Zhang
      3. c7104-03.patch
        13 kB
        Kan Zhang

        Issue Links

          Activity

          Hide
          Kan Zhang added a comment -

          Attaching a patch that

          1 . Passes InetAddress of the client to the authorization layer instead of hostname. The reverse lookup from InetAddress to hostname is done only when necessary.

          2. Added a supporting utility method for substituting "_HOST" that takes InetAddress instead of String.

          3. Reverting the principal name checking from using shortname back to using full kerberos principal name. Using the shortname, one can't check the hostname part, i.e., whether the connection is coming from a host that the Kerberos key is supposed to be used on.

          Show
          Kan Zhang added a comment - Attaching a patch that 1 . Passes InetAddress of the client to the authorization layer instead of hostname. The reverse lookup from InetAddress to hostname is done only when necessary. 2. Added a supporting utility method for substituting "_HOST" that takes InetAddress instead of String. 3. Reverting the principal name checking from using shortname back to using full kerberos principal name. Using the shortname, one can't check the hostname part, i.e., whether the connection is coming from a host that the Kerberos key is supposed to be used on.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12468304/c7104-01.patch
          against trunk revision 1058343.

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

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

          +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 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 passed core unit tests.

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/180//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/180//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/180//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/12468304/c7104-01.patch against trunk revision 1058343. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/180//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/180//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/180//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Uploaded review here:
          https://reviews.apache.org/r/316/diff/

          Comments to follow later today.

          Show
          Todd Lipcon added a comment - Uploaded review here: https://reviews.apache.org/r/316/diff/ Comments to follow later today.
          Hide
          Todd Lipcon added a comment -

          Commented on the reviewboard diff with some suggested test case improvements, but mostly looks good.

          Show
          Todd Lipcon added a comment - Commented on the reviewboard diff with some suggested test case improvements, but mostly looks good.
          Hide
          Kan Zhang added a comment -

          Todd, thanks for the review. Attaching a new patch incorporating your suggestions on testing. Note that by removing the dependency on DNS resolver, we're no longer testing the default behavior of using local hostname for substitution when hostname is null.

          Show
          Kan Zhang added a comment - Todd, thanks for the review. Attaching a new patch incorporating your suggestions on testing. Note that by removing the dependency on DNS resolver, we're no longer testing the default behavior of using local hostname for substitution when hostname is null.
          Hide
          Kan Zhang added a comment -

          Also, manually verified the patch on my one-node cluster.

          Show
          Kan Zhang added a comment - Also, manually verified the patch on my one-node cluster.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12468419/c7104-03.patch
          against trunk revision 1058881.

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

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

          +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 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 passed core unit tests.

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/183//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/183//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/183//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/12468419/c7104-03.patch against trunk revision 1058881. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/183//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/183//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/183//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Hi Kan,

          I take your point that we're no longer testing the 0.0.0.0 case. I just modifed your patch a little bit to include a new case that does this again. I think it's safe to rely on getLocalHostname being self-consistent, I was just worried that before we were doing the hostname -> ip -> hostname cycle which might not work on a build host. So the new test case just makes sure we return the local hostname for 0.0.0.0 and null.

          Also I flipped some assertTrue(...equals(...)) to assertEquals for better output on failure.

          If you're OK with these small changes give a +1 and I'll commit.

          Show
          Todd Lipcon added a comment - Hi Kan, I take your point that we're no longer testing the 0.0.0.0 case. I just modifed your patch a little bit to include a new case that does this again. I think it's safe to rely on getLocalHostname being self-consistent, I was just worried that before we were doing the hostname -> ip -> hostname cycle which might not work on a build host. So the new test case just makes sure we return the local hostname for 0.0.0.0 and null. Also I flipped some assertTrue(...equals(...)) to assertEquals for better output on failure. If you're OK with these small changes give a +1 and I'll commit.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12468429/7104-few-edits.patch
          against trunk revision 1058881.

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

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

          +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 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 passed core unit tests.

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

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/184//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/184//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/184//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/12468429/7104-few-edits.patch against trunk revision 1058881. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +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 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 passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/184//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/184//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/184//console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          Todd, the changes you made look fine. Thanks again!

          +1.

          Show
          Kan Zhang added a comment - Todd, the changes you made look fine. Thanks again! +1.
          Hide
          Todd Lipcon added a comment -

          I committed this to trunk only. Please let me know if you think it belongs in 0.22

          Show
          Todd Lipcon added a comment - I committed this to trunk only. Please let me know if you think it belongs in 0.22
          Hide
          Kan Zhang added a comment -

          Todd, please commit it to 0.22 as well. If it can't be cleanly applied, let me know.

          Show
          Kan Zhang added a comment - Todd, please commit it to 0.22 as well. If it can't be cleanly applied, let me know.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #577 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/577/)
          HADOOP-7104. Remove unnecessary DNS reverse lookups from RPC layer. Contributed by Kan Zhang

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #577 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/577/ ) HADOOP-7104 . Remove unnecessary DNS reverse lookups from RPC layer. Contributed by Kan Zhang
          Hide
          Jakob Homan added a comment -

          As Nigel detailed in his recent email, only critical or blocker bugs should be applied to the 22 branch, and only with strong justification. This issue is correctly categorized as an improvement. Therefore, it's up to Nigel as RM to pull in the patch to 22 if he wishes.

          Show
          Jakob Homan added a comment - As Nigel detailed in his recent email, only critical or blocker bugs should be applied to the 22 branch, and only with strong justification. This issue is correctly categorized as an improvement. Therefore, it's up to Nigel as RM to pull in the patch to 22 if he wishes.
          Hide
          Kan Zhang added a comment -

          Thanks, Jakob. Let's leave it to Nigel then (it can do real damage when the condition is right).

          Show
          Kan Zhang added a comment - Thanks, Jakob. Let's leave it to Nigel then (it can do real damage when the condition is right).
          Hide
          Koji Noguchi added a comment -

          Kan, Deveraj, isn't this a regression bug instead of an improvement?
          A single accept thread falling behind due to reverse DNS lookup leading to unresponsive Namenode. (still to be confirmed.)

          Nigel and dev, please consider this for 0.22.

          Show
          Koji Noguchi added a comment - Kan, Deveraj, isn't this a regression bug instead of an improvement? A single accept thread falling behind due to reverse DNS lookup leading to unresponsive Namenode. (still to be confirmed.) Nigel and dev, please consider this for 0.22.
          Hide
          Jakob Homan added a comment -

          Trusting Koji (always a good idea), this sounds like this is a bug, not an improvement. To 22 it should go.

          Show
          Jakob Homan added a comment - Trusting Koji (always a good idea), this sounds like this is a bug, not an improvement. To 22 it should go.
          Hide
          Todd Lipcon added a comment -

          Seems everyone's in agreement. Committed to branch-0.22 and moved the CHANGES.txt entry in trunk. Thanks again Kan.

          Show
          Todd Lipcon added a comment - Seems everyone's in agreement. Committed to branch-0.22 and moved the CHANGES.txt entry in trunk. Thanks again Kan.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #479 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/479/)
          Move HADOOP-7104 in CHANGES.txt to reflect inclusion in 0.22 branch

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #479 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/479/ ) Move HADOOP-7104 in CHANGES.txt to reflect inclusion in 0.22 branch
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #580 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/580/)
          Move HADOOP-7104 in CHANGES.txt to reflect inclusion in 0.22 branch

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #580 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/580/ ) Move HADOOP-7104 in CHANGES.txt to reflect inclusion in 0.22 branch
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-22-branch #16 (See https://hudson.apache.org/hudson/job/Hadoop-Common-22-branch/16/)
          HADOOP-7104. Remove unnecessary DNS reverse lookups from RPC layer. Contributed by Kan Zhang

          Show
          Hudson added a comment - Integrated in Hadoop-Common-22-branch #16 (See https://hudson.apache.org/hudson/job/Hadoop-Common-22-branch/16/ ) HADOOP-7104 . Remove unnecessary DNS reverse lookups from RPC layer. Contributed by Kan Zhang

            People

            • Assignee:
              Kan Zhang
              Reporter:
              Kan Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development