Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1296

using delegation token over hftp for long running clients (spawn from hdfs-1007).

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Tags:
      security
    1. HDFS-1296-14.patch
      52 kB
      Boris Shkolnik
    2. HDFS-1296-13.patch
      52 kB
      Boris Shkolnik
    3. hdfs-1007-securityutil-fix.patch
      0.9 kB
      Boris Shkolnik
    4. hdfs-1007-long-running-hftp-client.patch
      57 kB
      Boris Shkolnik

      Issue Links

        Activity

        Hide
        Boris Shkolnik added a comment -

        previous version patches. Not for commit. For forward porting.

        Show
        Boris Shkolnik added a comment - previous version patches. Not for commit. For forward porting.
        Hide
        Boris Shkolnik added a comment -

        +1 overall.
        [exec]
        [exec] +1 @author. The patch does not contain any @author tags.
        [exec]
        [exec] +1 tests included. The patch appears to include 9 new or modified tests.
        [exec]
        [exec] +1 javadoc. The javadoc tool did not generate any warning messages.
        [exec]
        [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
        [exec]
        [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
        [exec]
        [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
        [exec]

        Show
        Boris Shkolnik added a comment - +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 9 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. [exec]
        Hide
        Jitendra Nath Pandey added a comment -

        1. Token and DelegationTokenIdentifier both have a toString method. Can we use those instead of new stringifyToken? toString in the Token will need a fix not to write the password.
        2. In HftpFileSystem.java
        + delegationToken =
        + (Token<DelegationTokenIdentifier>) getDelegationToken(null);
        + renewer.addTokenToRenew(this);

        If renewer is passed as null in getDelegationToken, the renewer thread won't be able to renew it.

        3. In GetDelegationTokenServlet, the new delegation token may be returned as null. We should check for null.

        4. minor: I noticed similar code at a few places; is it possible to abstract it into a method, which takes scheme, hostname and port and constructs the uri?
        + sb = new StringBuilder("hdfs://");
        + sb.append(nnAddr.getHostName());
        + sb.append(":");
        + sb.append(nnPort);

        Show
        Jitendra Nath Pandey added a comment - 1. Token and DelegationTokenIdentifier both have a toString method. Can we use those instead of new stringifyToken? toString in the Token will need a fix not to write the password. 2. In HftpFileSystem.java + delegationToken = + (Token<DelegationTokenIdentifier>) getDelegationToken(null); + renewer.addTokenToRenew(this); If renewer is passed as null in getDelegationToken, the renewer thread won't be able to renew it. 3. In GetDelegationTokenServlet, the new delegation token may be returned as null. We should check for null. 4. minor: I noticed similar code at a few places; is it possible to abstract it into a method, which takes scheme, hostname and port and constructs the uri? + sb = new StringBuilder("hdfs://"); + sb.append(nnAddr.getHostName()); + sb.append(":"); + sb.append(nnPort);
        Hide
        Boris Shkolnik added a comment -

        1. Token and DelegationTokenIdentifier both have a toString method. Can we use those instead of new stringifyToken? toString in the Token will need a fix not to write the password.

        this is client side. On client side DTIdentifier is just a binary blob, so we cannot use toString().

        2. In HftpFileSystem.java

        + delegationToken =
        + (Token<DelegationTokenIdentifier>) getDelegationToken(null);
        + renewer.addTokenToRenew(this);
        If renewer is passed as null in getDelegationToken, the renewer thread won't be able to renew it.

        well, looks like it is added here in GetDelegationTokenServlet:
        final String renewerFinal = (renewer == null) ?
        req.getUserPrincipal().getName() : renewer;

        3. In GetDelegationTokenServlet, the new delegation token may be returned as null. We should check for null.

        DONE

        4. minor: I noticed similar code at a few places; is it possible to abstract it into a method, which takes scheme, hostname and port and constructs the uri?
        + sb = new StringBuilder("hdfs://");
        + sb.append(nnAddr.getHostName());
        + sb.append(":");
        + sb.append(nnPort);

        DONE

        Show
        Boris Shkolnik added a comment - 1. Token and DelegationTokenIdentifier both have a toString method. Can we use those instead of new stringifyToken? toString in the Token will need a fix not to write the password. this is client side. On client side DTIdentifier is just a binary blob, so we cannot use toString(). 2. In HftpFileSystem.java + delegationToken = + (Token<DelegationTokenIdentifier>) getDelegationToken(null); + renewer.addTokenToRenew(this); If renewer is passed as null in getDelegationToken, the renewer thread won't be able to renew it. well, looks like it is added here in GetDelegationTokenServlet: final String renewerFinal = (renewer == null) ? req.getUserPrincipal().getName() : renewer; 3. In GetDelegationTokenServlet, the new delegation token may be returned as null. We should check for null. DONE 4. minor: I noticed similar code at a few places; is it possible to abstract it into a method, which takes scheme, hostname and port and constructs the uri? + sb = new StringBuilder("hdfs://"); + sb.append(nnAddr.getHostName()); + sb.append(":"); + sb.append(nnPort); DONE
        Hide
        Jitendra Nath Pandey added a comment -

        +1

        Show
        Jitendra Nath Pandey added a comment - +1
        Hide
        Boris Shkolnik added a comment -

        committed to Trunk

        Show
        Boris Shkolnik added a comment - committed to Trunk
        Hide
        Jakob Homan added a comment -

        I've reverted this commit; it was breaking the build:

        compile-hdfs-classes:
            [javac] Compiling 206 source files to /Users/jhoman/work/git/hadoop-hdfs/build/classes
            [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:693: 
        method does not override or implement a method from a supertype
            [javac]   @Override
            [javac]   ^
            [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:704: 
        cannot find symbol
            [javac] symbol  : method getCanonicalServiceName()
            [javac] location: class org.apache.hadoop.hdfs.DistributedFileSystem
            [javac]     result.setService(new Text(getCanonicalServiceName()));
            [javac]                                ^
            [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:698: 
        method does not override or implement a method from a supertype
            [javac]   @Override
            [javac]   ^
            [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/HftpFileSystem.java:117: method 
        does not override or implement a method from a supertype
            [javac]   @Override
            [javac]   ^
            [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/HftpFileSystem.java:122: method 
        does not override or implement a method from a supertype
            [javac]   @Override
            [javac]   ^
            [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/HftpFileSystem.java:187: method 
        does not override or implement a method from a supertype
            [javac]   @Override
            [javac]   ^
            [javac] Note: Some input files use or override a deprecated API.
            [javac] Note: Recompile with -Xlint:deprecation for details.
            [javac] 6 errors
        

        Boris is out this week, so I'll take a look at updating the patch on Monday.

        Show
        Jakob Homan added a comment - I've reverted this commit; it was breaking the build: compile-hdfs-classes: [javac] Compiling 206 source files to /Users/jhoman/work/git/hadoop-hdfs/build/classes [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:693: method does not override or implement a method from a supertype [javac] @Override [javac] ^ [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:704: cannot find symbol [javac] symbol : method getCanonicalServiceName() [javac] location: class org.apache.hadoop.hdfs.DistributedFileSystem [javac] result.setService(new Text(getCanonicalServiceName())); [javac] ^ [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java:698: method does not override or implement a method from a supertype [javac] @Override [javac] ^ [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/HftpFileSystem.java:117: method does not override or implement a method from a supertype [javac] @Override [javac] ^ [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/HftpFileSystem.java:122: method does not override or implement a method from a supertype [javac] @Override [javac] ^ [javac] /Users/jhoman/work/git/hadoop-hdfs/src/java/org/apache/hadoop/hdfs/HftpFileSystem.java:187: method does not override or implement a method from a supertype [javac] @Override [javac] ^ [javac] Note: Some input files use or override a deprecated API. [javac] Note: Recompile with -Xlint:deprecation for details. [javac] 6 errors Boris is out this week, so I'll take a look at updating the patch on Monday.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #358 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/358/)
        Reverting HDFS-1296. Broke the build.

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #358 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/358/ ) Reverting HDFS-1296 . Broke the build.
        Hide
        Jakob Homan added a comment -

        The build-breaking seems to be related to the common part (HADOOP-6873) not being integrated into the common jar yet. If I build the common jar, install it locally and build from that, everything is fine. I'll ping Giri and ask him to take a look.

        Show
        Jakob Homan added a comment - The build-breaking seems to be related to the common part ( HADOOP-6873 ) not being integrated into the common jar yet. If I build the common jar, install it locally and build from that, everything is fine. I'll ping Giri and ask him to take a look.
        Hide
        Jakob Homan added a comment -

        The problem was that Hudson was down and had not promoted the dependent common change to maven yet. Giri fixed this, I've verified the fix and have re-committed the same patch. Resolving as fixed.

        Show
        Jakob Homan added a comment - The problem was that Hudson was down and had not promoted the dependent common change to maven yet. Giri fixed this, I've verified the fix and have re-committed the same patch. Resolving as fixed.
        Hide
        Hudson added a comment -

        Integrated in Hadoop-Hdfs-trunk-Commit #370 (See https://hudson.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/370/)

        Show
        Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #370 (See https://hudson.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/370/ )
        Hide
        Tsz Wo Nicholas Sze added a comment -
        //HftpFileSystem
        +  private static RenewerThread renewer = new RenewerThread();
        +  static {
        +    renewer.start();
        +  }
        

        It is not a good idea to create and start a thread in the HftpFileSystem static initializer. Starting a thread is expensive. In many cases, the renewer thread is not required. One typical example is calling HftpFileSystem.makeQualified(Path path).

        Show
        Tsz Wo Nicholas Sze added a comment - //HftpFileSystem + private static RenewerThread renewer = new RenewerThread(); + static { + renewer.start(); + } It is not a good idea to create and start a thread in the HftpFileSystem static initializer. Starting a thread is expensive. In many cases, the renewer thread is not required. One typical example is calling HftpFileSystem.makeQualified(Path path).
        Hide
        Tsz Wo Nicholas Sze added a comment -

        I filed HDFS-1389 for the RenewerThread.

        Show
        Tsz Wo Nicholas Sze added a comment - I filed HDFS-1389 for the RenewerThread.

          People

          • Assignee:
            Boris Shkolnik
            Reporter:
            Boris Shkolnik
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development