Details

    • Sub-task
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • None
    • 3.3.0, HDFS-13891
    • None
    • None
    • Reviewed

    Description

      HDFS Router should support issuing HDFS delegation tokens through WebHDFS.

      Attachments

        1. HDFS-13972-HDFS-13891.001.patch
          41 kB
          CR Hota
        2. HDFS-13972-HDFS-13891.002.patch
          10 kB
          CR Hota
        3. HDFS-13972-HDFS-13891.003.patch
          8 kB
          CR Hota
        4. HDFS-13972-HDFS-13891.004.patch
          13 kB
          CR Hota
        5. TestRouterWebHDFSContractTokens.java
          2 kB
          CR Hota
        6. HDFS-13972-HDFS-13891.005.patch
          21 kB
          CR Hota
        7. HDFS-13972-HDFS-13891.006.patch
          27 kB
          Íñigo Goiri
        8. HDFS-13972-HDFS-13891.007.patch
          20 kB
          CR Hota
        9. HDFS-13972-HDFS-13891.008.patch
          20 kB
          CR Hota
        10. HDFS-13972-HDFS-13891.009.patch
          22 kB
          CR Hota
        11. HDFS-13972-HDFS-13891.010.patch
          27 kB
          CR Hota
        12. HDFS-13972-HDFS-13891.011.patch
          28 kB
          CR Hota
        13. HDFS-13972-HDFS-13891.012.patch
          30 kB
          CR Hota
        14. HDFS-13972-HDFS-13891.013.patch
          30 kB
          CR Hota

        Issue Links

          Activity

            crh CR Hota added a comment -

            elgoiri For webhdfs route, current dependency on hadoop-hdfs project doesn't allow injecting router instance easily. How do you propose we tackle this issue?

            In the file JspHelper.java the below code has to be refactored. Is changing JspHelper itself an option? We can introduce a flag to allow understanding context better, instead of always looking for name.node

                if (context != null) {
                  final NameNode nn = NameNodeHttpServer.getNameNodeFromContext(context);
                  if (nn != null) {
                    // Verify the token.
                    nn.getNamesystem().verifyToken(id, token.getPassword());
                  }
                }
            
            
            crh CR Hota added a comment - elgoiri For webhdfs route, current dependency on hadoop-hdfs project doesn't allow injecting router instance easily. How do you propose we tackle this issue? In the file JspHelper.java the below code has to be refactored. Is changing JspHelper itself an option? We can introduce a flag to allow understanding context better, instead of always looking for name.node if (context != null ) { final NameNode nn = NameNodeHttpServer.getNameNodeFromContext(context); if (nn != null ) { // Verify the token. nn.getNamesystem().verifyToken(id, token.getPassword()); } }
            elgoiri Íñigo Goiri added a comment -

            For HDFS-12512, we had to do HDFS-12945.
            In HDFS-12945, we switched the extraction of the ClientProtocol from the Namenode and then HDFS-12512 was able to reimplement getRpcClientProtocol() to inspect the Router instead of the Namenode.
            I think the approach should be similar: we refactor the JspHelper to have a method that checks the Namenode but then we subclass it (or similar) so we can have a version that gets a Router.
            I'm not very familiar with the JspHelper but I'm guessing we can make a RouterJspHelper or similar which extends the JspHelper.
            Anyway, in general, we should refactor either the NamenodeWebHdfsMethods or the JspHelper so we can tune it for the Router.
            We can do the refactor in trunk.

            elgoiri Íñigo Goiri added a comment - For HDFS-12512 , we had to do HDFS-12945 . In HDFS-12945 , we switched the extraction of the ClientProtocol from the Namenode and then HDFS-12512 was able to reimplement getRpcClientProtocol() to inspect the Router instead of the Namenode. I think the approach should be similar: we refactor the JspHelper to have a method that checks the Namenode but then we subclass it (or similar) so we can have a version that gets a Router. I'm not very familiar with the JspHelper but I'm guessing we can make a RouterJspHelper or similar which extends the JspHelper . Anyway, in general, we should refactor either the NamenodeWebHdfsMethods or the JspHelper so we can tune it for the Router. We can do the refactor in trunk.
            crh CR Hota added a comment -

            Thanks elgoiri for your comments.

            This issue is little more involved as JspHelper is not injected from outside. So creating something like RouterJspHelper isn't quite simple, as UserProvider.java itself also have to change etc. I think our best bet would be to refactor JspHelper and proceed, this keeps things simple. As part of that refactor the code can try to understand what instance it is (whether namenode or router). The main point to note is hadoop-common project now will have an understanding of router. I can first make that change through another Jira.

            crh CR Hota added a comment - Thanks elgoiri for your comments. This issue is little more involved as JspHelper is not injected from outside. So creating something like RouterJspHelper isn't quite simple, as UserProvider.java itself also have to change etc. I think our best bet would be to refactor JspHelper and proceed, this keeps things simple. As part of that refactor the code can try to understand what instance it is (whether namenode or router). The main point to note is hadoop-common project now will have an understanding of router. I can first make that change through another Jira.
            crh CR Hota added a comment -

            elgoiri brahmareddy Could you help rebase HDFS-13891 branch with trunk. Need the refactor changes done to name node to proceed on this.

            Thanks !

            crh CR Hota added a comment - elgoiri   brahmareddy Could you help rebase  HDFS-13891  branch with trunk. Need the refactor changes done to name node to proceed on this. Thanks !

            Could you help rebase HDFS-13891 branch with trunk. 

            Done.Please pull the branch before you work.You might get conflicts , AS HDFS-13834 was missed(Sorry for this),I pushed this commit.

            brahmareddy Brahma Reddy Battula added a comment - Could you help rebase  HDFS-13891  branch with trunk.  Done.Please pull the branch before you work.You might get conflicts , AS HDFS-13834 was missed(Sorry for this),I pushed this commit.
            crh CR Hota added a comment -

            elgoiri  brahmareddy Thanks for helping with the merge.

            I could make webhdfs work in my environment, uploading the patch here for early review. This patch is not final and needs change, specially after rpc patch is committed. Please take a look and see if things look fine to you folks.

            Also as discussed in previous threads, we can do optimizations to re-use namenode code, but have kept it simple for now.

             

            crh CR Hota added a comment - elgoiri   brahmareddy Thanks for helping with the merge. I could make webhdfs work in my environment, uploading the patch here for early review. This patch is not final and needs change, specially after rpc patch is committed. Please take a look and see if things look fine to you folks. Also as discussed in previous threads, we can do optimizations to re-use namenode code, but have kept it simple for now.  
            elgoiri Íñigo Goiri added a comment -

            I would do this one after HDFS-13358 (setting it as a dependency) to make it easier to handle.
            Basically, the changes are:

            • RouterJspHelper. Do we want to keep referring to NN there and make it easier to merge at the end or should we start using Router naming?
            • RouterUserProvider. This looks pretty straightforward. Where do we use this class?
            elgoiri Íñigo Goiri added a comment - I would do this one after HDFS-13358 (setting it as a dependency) to make it easier to handle. Basically, the changes are: RouterJspHelper. Do we want to keep referring to NN there and make it easier to merge at the end or should we start using Router naming? RouterUserProvider. This looks pretty straightforward. Where do we use this class?
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 18s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            -1 test4tests 0m 0s 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.
                  HDFS-13891 Compile Tests
            +1 mvninstall 19m 53s HDFS-13891 passed
            +1 compile 0m 30s HDFS-13891 passed
            +1 checkstyle 0m 23s HDFS-13891 passed
            +1 mvnsite 0m 33s HDFS-13891 passed
            +1 shadedclient 12m 12s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 52s HDFS-13891 passed
            +1 javadoc 0m 34s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 29s the patch passed
            +1 compile 0m 25s the patch passed
            +1 javac 0m 25s the patch passed
            -0 checkstyle 0m 16s hadoop-hdfs-project/hadoop-hdfs-rbf: The patch generated 18 new + 21 unchanged - 0 fixed = 39 total (was 21)
            +1 mvnsite 0m 28s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 xml 0m 1s The patch has no ill-formed XML file.
            +1 shadedclient 12m 32s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 58s the patch passed
            +1 javadoc 0m 33s the patch passed
                  Other Tests
            +1 unit 20m 26s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 27s The patch does not generate ASF License warnings.
            72m 49s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12948755/HDFS-13972-HDFS-13891.001.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
            uname Linux f7fceb9803fa 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / 4d8cc85
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_181
            findbugs v3.1.0-RC1
            checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/25563/artifact/out/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs-rbf.txt
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/25563/testReport/
            Max. process+thread count 1340 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/25563/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 18s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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.       HDFS-13891 Compile Tests +1 mvninstall 19m 53s HDFS-13891 passed +1 compile 0m 30s HDFS-13891 passed +1 checkstyle 0m 23s HDFS-13891 passed +1 mvnsite 0m 33s HDFS-13891 passed +1 shadedclient 12m 12s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 52s HDFS-13891 passed +1 javadoc 0m 34s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 29s the patch passed +1 compile 0m 25s the patch passed +1 javac 0m 25s the patch passed -0 checkstyle 0m 16s hadoop-hdfs-project/hadoop-hdfs-rbf: The patch generated 18 new + 21 unchanged - 0 fixed = 39 total (was 21) +1 mvnsite 0m 28s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 1s The patch has no ill-formed XML file. +1 shadedclient 12m 32s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 58s the patch passed +1 javadoc 0m 33s the patch passed       Other Tests +1 unit 20m 26s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 72m 49s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12948755/HDFS-13972-HDFS-13891.001.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml uname Linux f7fceb9803fa 4.4.0-138-generic #164-Ubuntu SMP Tue Oct 2 17:16:02 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / 4d8cc85 maven version: Apache Maven 3.3.9 Default Java 1.8.0_181 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/25563/artifact/out/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs-rbf.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/25563/testReport/ Max. process+thread count 1340 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/25563/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.

            crh thanks for working on this. The approach looks good to me apart from the following.

            Also as discussed in previous threads, we can do optimizations to re-use namenode code, but have kept it simple for now.

            Yes, we can optimize. JspHelper.java and UserProvider.java also will be loaded classpath which is similar(except following) to RouterJSPHelper(RJH) and RouterUserProvider(RUP) . You might have get success RJH and RUP loaded first in Classpath in your test.

            • We can have one interface for verifytoken(...) which can be implmented by both namenode and Router(like below) So that we no need to have RJH and RUP.
            +package org.apache.hadoop.hdfs.server.common;
            +
            +import java.io.IOException;
            +
            +import org.apache.hadoop.security.token.delegation.AbstractDelegationTokenIdentifier;
            +
            +public interface TokenVerifier<T extends AbstractDelegationTokenIdentifier> {
            + void verifyToken(T t, byte[] password) throws IOException;
            +}
            
            --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/JspHelper.java
            +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/JspHelper.java
            @@ -176,10 +176,10 @@ private static UserGroupInformation getTokenUGI(ServletContext context,
            DelegationTokenIdentifier id = new DelegationTokenIdentifier();
            id.readFields(in);
            if (context != null) {
            - final NameNode nn = NameNodeHttpServer.getNameNodeFromContext(context);
            + final TokenVerifier nn = NameNodeHttpServer.getTokenVerifierFromContext(context);
            if (nn != null) {
            // Verify the token.
            - nn.getNamesystem().verifyToken(id, token.getPassword());
            + nn.verifyToken(id, token.getPassword());
            }
            
            --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
            +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
            @@ -47,6 +47,7 @@
            import org.apache.hadoop.hdfs.protocol.ClientProtocol;
            import org.apache.hadoop.hdfs.protocol.HdfsConstants;
            import org.apache.hadoop.hdfs.protocol.HdfsConstants.StoragePolicySatisfierMode;
            +import org.apache.hadoop.hdfs.security.token.delegation.DelegationTokenIdentifier;
            import org.apache.hadoop.hdfs.server.aliasmap.InMemoryAliasMap;
            import org.apache.hadoop.hdfs.server.aliasmap.InMemoryLevelDBAliasMapServer;
            import org.apache.hadoop.hdfs.server.blockmanagement.DatanodeManager;
            @@ -55,6 +56,7 @@
            import org.apache.hadoop.hdfs.server.common.HdfsServerConstants.StartupOption;
            import org.apache.hadoop.hdfs.server.common.MetricsLoggerTask;
            import org.apache.hadoop.hdfs.server.common.Storage.StorageDirectory;
            +import org.apache.hadoop.hdfs.server.common.TokenVerifier;
            import org.apache.hadoop.hdfs.server.namenode.ha.ActiveState;
            import org.apache.hadoop.hdfs.server.namenode.ha.BootstrapStandby;
            import org.apache.hadoop.hdfs.server.namenode.ha.HAContext;
            @@ -208,7 +210,7 @@
            **********************************************************/
            @InterfaceAudience.Private
            public class NameNode extends ReconfigurableBase implements
            - NameNodeStatusMXBean {
            + NameNodeStatusMXBean, TokenVerifier<DelegationTokenIdentifier> {
            static{
            HdfsConfiguration.init();
            }
            @@ -2202,4 +2204,10 @@ String reconfigureSPSModeEvent(String newVal, String property)
            protected Configuration getNewConf() {
            return new HdfsConfiguration();
            }
            +
            + @Override
            + public void verifyToken(DelegationTokenIdentifier tokenId, byte[] password)
            + throws IOException {
            + namesystem.verifyToken(tokenId, password);
            + }
            }
            diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java
            index 1bc43b896ae..e199a10bd5b 100644
            --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java
            +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java
            @@ -36,7 +36,9 @@
            import org.apache.hadoop.hdfs.DFSConfigKeys;
            import org.apache.hadoop.hdfs.DFSUtil;
            import org.apache.hadoop.hdfs.client.HdfsClientConfigKeys;
            +import org.apache.hadoop.hdfs.security.token.delegation.DelegationTokenIdentifier;
            import org.apache.hadoop.hdfs.server.common.JspHelper;
            +import org.apache.hadoop.hdfs.server.common.TokenVerifier;
            import org.apache.hadoop.hdfs.server.namenode.startupprogress.StartupProgress;
            import org.apache.hadoop.hdfs.server.namenode.web.resources.NamenodeWebHdfsMethods;
            import org.apache.hadoop.hdfs.web.AuthFilter;
            @@ -308,6 +310,10 @@ public static NameNode getNameNodeFromContext(ServletContext context) {
            return (NameNode)context.getAttribute(NAMENODE_ATTRIBUTE_KEY);
            }
            
            + public static TokenVerifier getTokenVerifierFromContext(ServletContext context) {
            + return (TokenVerifier) context.getAttribute(NAMENODE_ATTRIBUTE_KEY);
            + }
            +
            
            --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/Router.java
            +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/Router.java
            @@ -33,6 +33,8 @@
            import org.apache.hadoop.conf.Configuration;
            import org.apache.hadoop.hdfs.DFSUtil;
            import org.apache.hadoop.hdfs.HAUtil;
            +import org.apache.hadoop.hdfs.security.token.delegation.DelegationTokenIdentifier;
            +import org.apache.hadoop.hdfs.server.common.TokenVerifier;
            import org.apache.hadoop.hdfs.server.federation.metrics.FederationMetrics;
            import org.apache.hadoop.hdfs.server.federation.metrics.NamenodeBeanMetrics;
            import org.apache.hadoop.hdfs.server.federation.resolver.ActiveNamenodeResolver;
            @@ -70,7 +72,8 @@
            */
            @InterfaceAudience.Private
            @InterfaceStability.Evolving
            -public class Router extends CompositeService {
            +public class Router extends CompositeService
            + implements TokenVerifier<DelegationTokenIdentifier> {
            
            private static final Logger LOG = LoggerFactory.getLogger(Router.class);
            
            @@ -673,4 +676,10 @@ RouterQuotaUpdateService getQuotaCacheUpdateService() {
            RouterSafemodeService getSafemodeService() {
            return this.safemodeService;
            }
            +
            + @Override
            + public void verifyToken(DelegationTokenIdentifier tokenId, byte[] password)
            + throws IOException {
            + getRpcServer().getRouterSecurityManager().verifyToken(id, token.getPassword());
            
            + }
            • I think, while storing keys we can put in currentTokens so that we no need to connect ZK while geting the token.May be check this change in your cluster and handle in seperate Jira so that it can be committed to trunk also.
            diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java
             index 88bd29bd730..741163bc116 100644
             — a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java
             +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java
             @@ -807,6 +807,7 @@ protected void storeToken(TokenIdent ident,
             DelegationTokenInformation tokenInfo) throws IOException {
             try
            {
             addOrUpdateToken(ident, tokenInfo, false); 
            + currentTokens.put(ident, tokenInfo);
             }catch (Exception e){ 
            throw new RuntimeException(e); 
            }
            
            •  Need to see load on ZK when there multiple DT are created( which creates so many ZK connections).Do you kept eye on that?
            • Better to have some UT

             

            brahmareddy Brahma Reddy Battula added a comment - crh thanks for working on this. The approach looks good to me apart from the following. Also as discussed in previous threads, we can do optimizations to re-use namenode code, but have kept it simple for now. Yes, we can optimize. JspHelper.java and UserProvider.java also will be loaded classpath which is similar(except following) to RouterJSPHelper(RJH) and RouterUserProvider(RUP) . You might have get success RJH and RUP loaded first in Classpath in your test. We can have one interface for verifytoken(...) which can be implmented by both namenode and Router(like below) So that we no need to have RJH and RUP. + package org.apache.hadoop.hdfs.server.common; + + import java.io.IOException; + + import org.apache.hadoop.security.token.delegation.AbstractDelegationTokenIdentifier; + + public interface TokenVerifier<T extends AbstractDelegationTokenIdentifier> { + void verifyToken(T t, byte [] password) throws IOException; +} --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/JspHelper.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/JspHelper.java @@ -176,10 +176,10 @@ private static UserGroupInformation getTokenUGI(ServletContext context, DelegationTokenIdentifier id = new DelegationTokenIdentifier(); id.readFields(in); if (context != null ) { - final NameNode nn = NameNodeHttpServer.getNameNodeFromContext(context); + final TokenVerifier nn = NameNodeHttpServer.getTokenVerifierFromContext(context); if (nn != null ) { // Verify the token. - nn.getNamesystem().verifyToken(id, token.getPassword()); + nn.verifyToken(id, token.getPassword()); } --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java @@ -47,6 +47,7 @@ import org.apache.hadoop.hdfs.protocol.ClientProtocol; import org.apache.hadoop.hdfs.protocol.HdfsConstants; import org.apache.hadoop.hdfs.protocol.HdfsConstants.StoragePolicySatisfierMode; + import org.apache.hadoop.hdfs.security.token.delegation.DelegationTokenIdentifier; import org.apache.hadoop.hdfs.server.aliasmap.InMemoryAliasMap; import org.apache.hadoop.hdfs.server.aliasmap.InMemoryLevelDBAliasMapServer; import org.apache.hadoop.hdfs.server.blockmanagement.DatanodeManager; @@ -55,6 +56,7 @@ import org.apache.hadoop.hdfs.server.common.HdfsServerConstants.StartupOption; import org.apache.hadoop.hdfs.server.common.MetricsLoggerTask; import org.apache.hadoop.hdfs.server.common.Storage.StorageDirectory; + import org.apache.hadoop.hdfs.server.common.TokenVerifier; import org.apache.hadoop.hdfs.server.namenode.ha.ActiveState; import org.apache.hadoop.hdfs.server.namenode.ha.BootstrapStandby; import org.apache.hadoop.hdfs.server.namenode.ha.HAContext; @@ -208,7 +210,7 @@ **********************************************************/ @InterfaceAudience.Private public class NameNode extends ReconfigurableBase implements - NameNodeStatusMXBean { + NameNodeStatusMXBean, TokenVerifier<DelegationTokenIdentifier> { static { HdfsConfiguration.init(); } @@ -2202,4 +2204,10 @@ String reconfigureSPSModeEvent( String newVal, String property) protected Configuration getNewConf() { return new HdfsConfiguration(); } + + @Override + public void verifyToken(DelegationTokenIdentifier tokenId, byte [] password) + throws IOException { + namesystem.verifyToken(tokenId, password); + } } diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java index 1bc43b896ae..e199a10bd5b 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeHttpServer.java @@ -36,7 +36,9 @@ import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.DFSUtil; import org.apache.hadoop.hdfs.client.HdfsClientConfigKeys; + import org.apache.hadoop.hdfs.security.token.delegation.DelegationTokenIdentifier; import org.apache.hadoop.hdfs.server.common.JspHelper; + import org.apache.hadoop.hdfs.server.common.TokenVerifier; import org.apache.hadoop.hdfs.server.namenode.startupprogress.StartupProgress; import org.apache.hadoop.hdfs.server.namenode.web.resources.NamenodeWebHdfsMethods; import org.apache.hadoop.hdfs.web.AuthFilter; @@ -308,6 +310,10 @@ public static NameNode getNameNodeFromContext(ServletContext context) { return (NameNode)context.getAttribute(NAMENODE_ATTRIBUTE_KEY); } + public static TokenVerifier getTokenVerifierFromContext(ServletContext context) { + return (TokenVerifier) context.getAttribute(NAMENODE_ATTRIBUTE_KEY); + } + --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/Router.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/Router.java @@ -33,6 +33,8 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.hdfs.DFSUtil; import org.apache.hadoop.hdfs.HAUtil; + import org.apache.hadoop.hdfs.security.token.delegation.DelegationTokenIdentifier; + import org.apache.hadoop.hdfs.server.common.TokenVerifier; import org.apache.hadoop.hdfs.server.federation.metrics.FederationMetrics; import org.apache.hadoop.hdfs.server.federation.metrics.NamenodeBeanMetrics; import org.apache.hadoop.hdfs.server.federation.resolver.ActiveNamenodeResolver; @@ -70,7 +72,8 @@ */ @InterfaceAudience.Private @InterfaceStability.Evolving - public class Router extends CompositeService { + public class Router extends CompositeService + implements TokenVerifier<DelegationTokenIdentifier> { private static final Logger LOG = LoggerFactory.getLogger(Router.class); @@ -673,4 +676,10 @@ RouterQuotaUpdateService getQuotaCacheUpdateService() { RouterSafemodeService getSafemodeService() { return this .safemodeService; } + + @Override + public void verifyToken(DelegationTokenIdentifier tokenId, byte [] password) + throws IOException { + getRpcServer().getRouterSecurityManager().verifyToken(id, token.getPassword()); + } I think, while storing keys we can put in currentTokens so that we no need to connect ZK while geting the token.May be check this change in your cluster and handle in seperate Jira so that it can be committed to trunk also. diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java index 88bd29bd730..741163bc116 100644 — a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java @@ -807,6 +807,7 @@ protected void storeToken(TokenIdent ident, DelegationTokenInformation tokenInfo) throws IOException { try { addOrUpdateToken(ident, tokenInfo, false ); + currentTokens.put(ident, tokenInfo); } catch (Exception e){ throw new RuntimeException(e); }  Need to see load on ZK when there multiple DT are created( which creates so many ZK connections).Do you kept eye on that? Better to have some UT  
            crh CR Hota added a comment -

            elgoiri brahmareddy Thanks for your initial review.

            Based on what Brahma has mentioned, I am then going to first refactor name node and then come back and look at this again.

            Had anyways created the refactor ticket earlier knowing that this is inevitable. HDFS-14006

            crh CR Hota added a comment - elgoiri brahmareddy Thanks for your initial review. Based on what Brahma has mentioned, I am then going to first refactor name node and then come back and look at this again. Had anyways created the refactor ticket earlier knowing that this is inevitable.  HDFS-14006
            crh CR Hota added a comment -

            elgoiri brahmareddy Please help rebase rbf branch HDFS-13891 with trunk. Need the namenode refactoring changes to re-work on couple of things here.

             

            crh CR Hota added a comment - elgoiri brahmareddy Please help rebase rbf branch  HDFS-13891  with trunk. Need the namenode refactoring changes to re-work on couple of things here.  
            elgoiri Íñigo Goiri added a comment -

            It looks like the rebase worked this time for me.
            It's a little weird as it shows in the UI as all committed by me:
            https://github.com/apache/hadoop/commits/HDFS-13891
            However, git log looks OK.
            Let me know if there's anything to fix.

            elgoiri Íñigo Goiri added a comment - It looks like the rebase worked this time for me. It's a little weird as it shows in the UI as all committed by me: https://github.com/apache/hadoop/commits/HDFS-13891 However, git log looks OK. Let me know if there's anything to fix.
            crh CR Hota added a comment -

            elgoiri Thanks, the rebase looks good to me.

            crh CR Hota added a comment - elgoiri Thanks, the rebase looks good to me.

            Hi crh

            any update on this jira..? 

            brahmareddy Brahma Reddy Battula added a comment - Hi  crh any update on this jira..? 
            crh CR Hota added a comment -

            brahmareddy Thanks for the follow-up. I am getting some of our 2019 planning done along with deployment of secure rbf in a datacenter for testing.
            Will surely focus back on all these tickets in a week.

            elgoiri FYI ..

            crh CR Hota added a comment - brahmareddy Thanks for the follow-up. I am getting some of our 2019 planning done along with deployment of secure rbf in a datacenter for testing. Will surely focus back on all these tickets in a week. elgoiri FYI ..
            elgoiri Íñigo Goiri added a comment -

            Can we rebase after HDFS-13358?
            This should get pretty contained right now.

            elgoiri Íñigo Goiri added a comment - Can we rebase after HDFS-13358 ? This should get pretty contained right now.
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 0s Docker mode activated.
            -1 patch 0m 7s HDFS-13972 does not apply to HDFS-13891. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 7s HDFS-13972 does not apply to HDFS-13891 . Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12948755/HDFS-13972-HDFS-13891.001.patch Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26229/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.

            crh can you update the patch..?

            brahmareddy Brahma Reddy Battula added a comment - crh can you update the patch..?
            crh CR Hota added a comment -

            brahmareddy elgoiri Uploaded a new patch after rebasing with latest changes for early feedback. Working on wiring the tests.

            Realized that we did not add any WEB based tests as part kerberos patch in HDFS-12284.

            crh CR Hota added a comment - brahmareddy elgoiri Uploaded a new patch after rebasing with latest changes for early feedback. Working on wiring the tests. Realized that we did not add any WEB based tests as part kerberos patch in HDFS-12284 .
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 15s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            -1 test4tests 0m 0s 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.
                  HDFS-13891 Compile Tests
            0 mvndep 0m 18s Maven dependency ordering for branch
            +1 mvninstall 19m 1s HDFS-13891 passed
            +1 compile 2m 57s HDFS-13891 passed
            +1 checkstyle 0m 59s HDFS-13891 passed
            +1 mvnsite 1m 26s HDFS-13891 passed
            +1 shadedclient 12m 51s branch has no errors when building and testing our client artifacts.
            +1 findbugs 2m 42s HDFS-13891 passed
            +1 javadoc 1m 11s HDFS-13891 passed
                  Patch Compile Tests
            0 mvndep 0m 8s Maven dependency ordering for patch
            +1 mvninstall 1m 23s the patch passed
            +1 compile 2m 57s the patch passed
            +1 javac 2m 57s the patch passed
            -0 checkstyle 0m 56s hadoop-hdfs-project: The patch generated 2 new + 132 unchanged - 0 fixed = 134 total (was 132)
            +1 mvnsite 1m 22s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 11m 26s patch has no errors when building and testing our client artifacts.
            +1 findbugs 3m 8s the patch passed
            +1 javadoc 1m 19s the patch passed
                  Other Tests
            -1 unit 74m 1s hadoop-hdfs in the patch failed.
            +1 unit 21m 32s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 26s The patch does not generate ASF License warnings.
            159m 46s



            Reason Tests
            Failed junit tests hadoop.hdfs.web.TestWebHdfsTimeouts
              hadoop.hdfs.qjournal.server.TestJournalNodeSync



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12959406/HDFS-13972-HDFS-13891.002.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux 737018ce8229 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / f94b6e3
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/26267/artifact/out/diff-checkstyle-hadoop-hdfs-project.txt
            unit https://builds.apache.org/job/PreCommit-HDFS-Build/26267/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26267/testReport/
            Max. process+thread count 4451 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26267/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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.       HDFS-13891 Compile Tests 0 mvndep 0m 18s Maven dependency ordering for branch +1 mvninstall 19m 1s HDFS-13891 passed +1 compile 2m 57s HDFS-13891 passed +1 checkstyle 0m 59s HDFS-13891 passed +1 mvnsite 1m 26s HDFS-13891 passed +1 shadedclient 12m 51s branch has no errors when building and testing our client artifacts. +1 findbugs 2m 42s HDFS-13891 passed +1 javadoc 1m 11s HDFS-13891 passed       Patch Compile Tests 0 mvndep 0m 8s Maven dependency ordering for patch +1 mvninstall 1m 23s the patch passed +1 compile 2m 57s the patch passed +1 javac 2m 57s the patch passed -0 checkstyle 0m 56s hadoop-hdfs-project: The patch generated 2 new + 132 unchanged - 0 fixed = 134 total (was 132) +1 mvnsite 1m 22s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 11m 26s patch has no errors when building and testing our client artifacts. +1 findbugs 3m 8s the patch passed +1 javadoc 1m 19s the patch passed       Other Tests -1 unit 74m 1s hadoop-hdfs in the patch failed. +1 unit 21m 32s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 26s The patch does not generate ASF License warnings. 159m 46s Reason Tests Failed junit tests hadoop.hdfs.web.TestWebHdfsTimeouts   hadoop.hdfs.qjournal.server.TestJournalNodeSync Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12959406/HDFS-13972-HDFS-13891.002.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux 737018ce8229 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / f94b6e3 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/26267/artifact/out/diff-checkstyle-hadoop-hdfs-project.txt unit https://builds.apache.org/job/PreCommit-HDFS-Build/26267/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26267/testReport/ Max. process+thread count 4451 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26267/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            elgoiri Íñigo Goiri added a comment -

            Thanks crh for the update; this looks pretty self-contained.

            • I see this includes HDFS-14052.
            • Extend javadoc for the new methods in RouterSecurityManager.
            • Why do we need the Router to implement the TokenVerifier? Where is this used?
            • The changes in NamenodeWebHdfsMethods can be done in a separate JIRA to regular HDFS.
            • The log in RouterSecurityManager#verifyToken() should use logger style {}.
            • As you mentioned, it needs some tests.
            elgoiri Íñigo Goiri added a comment - Thanks crh for the update; this looks pretty self-contained. I see this includes HDFS-14052 . Extend javadoc for the new methods in RouterSecurityManager. Why do we need the Router to implement the TokenVerifier? Where is this used? The changes in NamenodeWebHdfsMethods can be done in a separate JIRA to regular HDFS. The log in RouterSecurityManager#verifyToken() should use logger style {}. As you mentioned, it needs some tests.
            crh CR Hota added a comment -

            elgoiri Thanks for the early feedback.  Please help rebasing this branch with trunk.

            TokenVerifier was introduced to allow routers to utilize existing JspHelper class methods. TokenVerifier is needed to verify tokens that are intercepted as part of webhdfs. This change was done in HDFS-14006.

            crh CR Hota added a comment - elgoiri Thanks for the early feedback.  Please help rebasing this branch with trunk. TokenVerifier was introduced to allow routers to utilize existing JspHelper class methods. TokenVerifier is needed to verify tokens that are intercepted as part of webhdfs. This change was done in HDFS-14006 .
            elgoiri Íñigo Goiri added a comment -

            Thanks for reminder on HDFS-14006, that makes sense.

            Rebased HDFS-13891.

            elgoiri Íñigo Goiri added a comment - Thanks for reminder on HDFS-14006 , that makes sense. Rebased HDFS-13891 .
            crh CR Hota added a comment -

            elgoiri brahmareddy 

            Thanks for the previous reviews.

            Can you help me apply this patch (HDFS-13972-HDFS-13891.003.patch) and test the web functionalities? I have verified the changes in our env.

            I am still figuring out how and what to wire as part of unit tests. Seems we did not add any unit tests wrt kerberos negotiate web part in the kerberos patch. Any pointers on what are the good areas to test in this? I found this class TestWebHdfsTokens in the namenode side. Should we do something similar?

            crh CR Hota added a comment - elgoiri brahmareddy   Thanks for the previous reviews. Can you help me apply this patch ( HDFS-13972 - HDFS-13891 .003.patch) and test the web functionalities? I have verified the changes in our env. I am still figuring out how and what to wire as part of unit tests. Seems we did not add any unit tests wrt kerberos negotiate web part in the kerberos patch. Any pointers on what are the good areas to test in this? I found this class TestWebHdfsTokens in the namenode side. Should we do something similar?
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 30s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            -1 test4tests 0m 0s 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.
                  HDFS-13891 Compile Tests
            +1 mvninstall 23m 37s HDFS-13891 passed
            +1 compile 0m 28s HDFS-13891 passed
            +1 checkstyle 0m 21s HDFS-13891 passed
            +1 mvnsite 0m 33s HDFS-13891 passed
            +1 shadedclient 12m 0s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 57s HDFS-13891 passed
            +1 javadoc 0m 35s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 27s the patch passed
            +1 compile 0m 26s the patch passed
            +1 javac 0m 26s the patch passed
            +1 checkstyle 0m 19s the patch passed
            +1 mvnsite 0m 29s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 12m 40s patch has no errors when building and testing our client artifacts.
            +1 findbugs 1m 2s the patch passed
            +1 javadoc 0m 30s the patch passed
                  Other Tests
            +1 unit 22m 44s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 29s The patch does not generate ASF License warnings.
            79m 4s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12960819/HDFS-13972-HDFS-13891.003.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux 7438ee7bd214 4.4.0-139-generic #165~14.04.1-Ubuntu SMP Wed Oct 31 10:55:11 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / da99828
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26375/testReport/
            Max. process+thread count 996 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26375/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 30s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s 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.       HDFS-13891 Compile Tests +1 mvninstall 23m 37s HDFS-13891 passed +1 compile 0m 28s HDFS-13891 passed +1 checkstyle 0m 21s HDFS-13891 passed +1 mvnsite 0m 33s HDFS-13891 passed +1 shadedclient 12m 0s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 57s HDFS-13891 passed +1 javadoc 0m 35s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 27s the patch passed +1 compile 0m 26s the patch passed +1 javac 0m 26s the patch passed +1 checkstyle 0m 19s the patch passed +1 mvnsite 0m 29s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 12m 40s patch has no errors when building and testing our client artifacts. +1 findbugs 1m 2s the patch passed +1 javadoc 0m 30s the patch passed       Other Tests +1 unit 22m 44s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 29s The patch does not generate ASF License warnings. 79m 4s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12960819/HDFS-13972-HDFS-13891.003.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux 7438ee7bd214 4.4.0-139-generic #165~14.04.1-Ubuntu SMP Wed Oct 31 10:55:11 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / da99828 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26375/testReport/ Max. process+thread count 996 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26375/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            elgoiri Íñigo Goiri added a comment -

            Thanks crh for HDFS-13972-HDFS-13891.003.patch.
            I think TestWebHdfsTokens is a good base.
            Maybe it is a little too exhaustive even; a simpler version of that which focuses on the Router wiring should be enough.

            elgoiri Íñigo Goiri added a comment - Thanks crh for HDFS-13972-HDFS-13891.003.patch . I think TestWebHdfsTokens is a good base. Maybe it is a little too exhaustive even; a simpler version of that which focuses on the Router wiring should be enough.
            aajisaka Akira Ajisaka added a comment -

            Thanks crh for the 003 patch. Mostly works fine, but WebHDFS liststatus couldn't list the mount point correctly in my environment.

            Using HDFS API, two directory are listed.

            $ hdfs dfs -ls hdfs://<router>:8888/tmp/
            Found 2 items
            drwxrwxrwx   - hdfs supergroup          0 2018-11-09 17:50 hdfs://<router>:8888/tmp/tmp1
            drwxrwxrwx   - hdfs supergroup          0 2018-11-09 17:50 hdfs://<router>:8888/tmp/tmp2
            

            Using WebHDFS API, only one directory is listed.

            $ curl -u : --negotiate -i "http://<router>:50071/webhdfs/v1/tmp/?op=LISTSTATUS"
            (snip)
            {"FileStatuses":{"FileStatus":[
            {"accessTime":0,"blockSize":0,"childrenNum":0,"fileId":16387,"group":"supergroup","length":0,"modificationTime":1552016766769,"owner":"hdfs","pathSuffix":"tmp1","permission":"755","replication":0,"storagePolicy":0,"type":"DIRECTORY"}
            ]}}
            

            The mount table is as follows:

            $ hdfs dfsrouteradmin -ls /tmp
            Mount Table Entries:
            Source                    Destinations              Owner                     Group                     Mode                      Quota/Usage              
            /tmp                      ns1->/tmp                 aajisaka                  users                     rwxr-xr-x                 [NsQuota: -/-, SsQuota: -/-]
            /tmp/tmp1                 ns1->/tmp/tmp1            aajisaka                  users                     rwxr-xr-x                 [NsQuota: -/-, SsQuota: -/-]
            /tmp/tmp2                 ns2->/tmp/tmp2            aajisaka                  users                     rwxr-xr-x                 [NsQuota: -/-, SsQuota: -/-]
            
            aajisaka Akira Ajisaka added a comment - Thanks crh for the 003 patch. Mostly works fine, but WebHDFS liststatus couldn't list the mount point correctly in my environment. Using HDFS API, two directory are listed. $ hdfs dfs -ls hdfs://<router>:8888/tmp/ Found 2 items drwxrwxrwx - hdfs supergroup 0 2018-11-09 17:50 hdfs://<router>:8888/tmp/tmp1 drwxrwxrwx - hdfs supergroup 0 2018-11-09 17:50 hdfs://<router>:8888/tmp/tmp2 Using WebHDFS API, only one directory is listed. $ curl -u : --negotiate -i "http://<router>:50071/webhdfs/v1/tmp/?op=LISTSTATUS" (snip) {"FileStatuses":{"FileStatus":[ {"accessTime":0,"blockSize":0,"childrenNum":0,"fileId":16387,"group":"supergroup","length":0,"modificationTime":1552016766769,"owner":"hdfs","pathSuffix":"tmp1","permission":"755","replication":0,"storagePolicy":0,"type":"DIRECTORY"} ]}} The mount table is as follows: $ hdfs dfsrouteradmin -ls /tmp Mount Table Entries: Source Destinations Owner Group Mode Quota/Usage /tmp ns1->/tmp aajisaka users rwxr-xr-x [NsQuota: -/-, SsQuota: -/-] /tmp/tmp1 ns1->/tmp/tmp1 aajisaka users rwxr-xr-x [NsQuota: -/-, SsQuota: -/-] /tmp/tmp2 ns2->/tmp/tmp2 aajisaka users rwxr-xr-x [NsQuota: -/-, SsQuota: -/-]
            aajisaka Akira Ajisaka added a comment -

            Without trailing thrash, two directories are listed.

            $ curl -u : --negotiate -i "http://<router>:50071/webhdfs/v1/tmp?op=LISTSTATUS"
            (snip)
            {"FileStatuses":{"FileStatus":[
            {"accessTime":1541753421917,"blockSize":0,"childrenNum":0,"fileId":0,"group":"supergroup","length":0,"modificationTime":1541753421917,"owner":"hdfs","pathSuffix":"tmp1","permission":"777","replication":0,"storagePolicy":0,"symlink":"","type":"DIRECTORY"},
            {"accessTime":1541753429812,"blockSize":0,"childrenNum":0,"fileId":0,"group":"supergroup","length":0,"modificationTime":1541753429812,"owner":"hdfs","pathSuffix":"tmp2","permission":"777","replication":0,"storagePolicy":0,"symlink":"","type":"DIRECTORY"}
            ]}}
            
            aajisaka Akira Ajisaka added a comment - Without trailing thrash, two directories are listed. $ curl -u : --negotiate -i "http://<router>:50071/webhdfs/v1/tmp?op=LISTSTATUS" (snip) {"FileStatuses":{"FileStatus":[ {"accessTime":1541753421917,"blockSize":0,"childrenNum":0,"fileId":0,"group":"supergroup","length":0,"modificationTime":1541753421917,"owner":"hdfs","pathSuffix":"tmp1","permission":"777","replication":0,"storagePolicy":0,"symlink":"","type":"DIRECTORY"}, {"accessTime":1541753429812,"blockSize":0,"childrenNum":0,"fileId":0,"group":"supergroup","length":0,"modificationTime":1541753429812,"owner":"hdfs","pathSuffix":"tmp2","permission":"777","replication":0,"storagePolicy":0,"symlink":"","type":"DIRECTORY"} ]}}
            crh CR Hota added a comment -

            ajisakaa

            Thank you for trying the patch. Wondering if you also tried the RPC patch in HDFS-13358.

            The issue you reported with liststatus is agnostic to security. This is how current router mount table parsing works. Parsing currently looks for the entire path with trailing slash to find out mappings. We can modify the logic a bit in a separate jira.

            crh CR Hota added a comment - ajisakaa Thank you for trying the patch. Wondering if you also tried the RPC patch in  HDFS-13358 . The issue you reported with liststatus is agnostic to security. This is how current router mount table parsing works. Parsing currently looks for the entire path with trailing slash to find out mappings. We can modify the logic a bit in a separate jira.
            aajisaka Akira Ajisaka added a comment -

            crh, thank you for your detailed information!

            aajisaka Akira Ajisaka added a comment - crh , thank you for your detailed information!
            elgoiri Íñigo Goiri added a comment -

            We may need to fix the trailing / issue.
            With the HDFS client this is not an issue as it gets trimmed but it looks like WebHDFS uses it directly.
            We should open a JIRA for this.

            elgoiri Íñigo Goiri added a comment - We may need to fix the trailing / issue. With the HDFS client this is not an issue as it gets trimmed but it looks like WebHDFS uses it directly. We should open a JIRA for this.
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 20s Docker mode activated.
                  Prechecks
            +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.
                  HDFS-13891 Compile Tests
            +1 mvninstall 18m 49s HDFS-13891 passed
            +1 compile 0m 31s HDFS-13891 passed
            +1 checkstyle 0m 23s HDFS-13891 passed
            +1 mvnsite 0m 37s HDFS-13891 passed
            +1 shadedclient 12m 55s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 57s HDFS-13891 passed
            +1 javadoc 0m 35s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 27s the patch passed
            +1 compile 0m 25s the patch passed
            +1 javac 0m 25s the patch passed
            +1 checkstyle 0m 17s the patch passed
            +1 mvnsite 0m 29s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 12m 45s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 56s the patch passed
            +1 javadoc 0m 32s the patch passed
                  Other Tests
            +1 unit 23m 21s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 27s The patch does not generate ASF License warnings.
            75m 34s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962023/HDFS-13972-HDFS-13891.004.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux 27e7743e57bc 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / c359a52
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26450/testReport/
            Max. process+thread count 995 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26450/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated.       Prechecks +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.       HDFS-13891 Compile Tests +1 mvninstall 18m 49s HDFS-13891 passed +1 compile 0m 31s HDFS-13891 passed +1 checkstyle 0m 23s HDFS-13891 passed +1 mvnsite 0m 37s HDFS-13891 passed +1 shadedclient 12m 55s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 57s HDFS-13891 passed +1 javadoc 0m 35s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 27s the patch passed +1 compile 0m 25s the patch passed +1 javac 0m 25s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 29s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 12m 45s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 56s the patch passed +1 javadoc 0m 32s the patch passed       Other Tests +1 unit 23m 21s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 75m 34s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962023/HDFS-13972-HDFS-13891.004.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux 27e7743e57bc 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / c359a52 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26450/testReport/ Max. process+thread count 995 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26450/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            crh CR Hota added a comment -

            elgoiri brahmareddy ajisakaa

            Thanks for the previous reviews.

            I haven't been able to wire a kerberos negotiation enabled webhdfs end to end. For this change however unit tests are added which tests the new code that router security manager exposes. Please take a look and share thoughts.

            crh CR Hota added a comment - elgoiri brahmareddy ajisakaa Thanks for the previous reviews. I haven't been able to wire a kerberos negotiation enabled webhdfs end to end. For this change however unit tests are added which tests the new code that router security manager exposes. Please take a look and share thoughts.
            elgoiri Íñigo Goiri added a comment - - edited

            Thanks crh for HDFS-13972-HDFS-13891.004.patch.

            • Can we extract the ternary in RouterWebHdfsMethods#662?
            • Can we check something else than assertNotNull() in testCreateCredentials()?
            • In TestRouterSecurityManager#133, we should add a space after the for.
            • I think we should avoid the log and throw and just throw in RouterSecurityManager#verifyToken().
            • Why do we have RouterSecurityManager#verifyToken(); this is only used by the test?
            • Make TestRouterSecurityManager#getUserGroupForTesting private and static.

            What issues are you facing for using WebHDFS to get a token?
            The Router you are getting should already provide a fully initialized HTTP server right?
            I think we should at least go through the HTTP server.

            elgoiri Íñigo Goiri added a comment - - edited Thanks crh for HDFS-13972-HDFS-13891.004.patch . Can we extract the ternary in RouterWebHdfsMethods#662 ? Can we check something else than assertNotNull() in testCreateCredentials() ? In TestRouterSecurityManager#133 , we should add a space after the for . I think we should avoid the log and throw and just throw in RouterSecurityManager#verifyToken() . Why do we have RouterSecurityManager#verifyToken() ; this is only used by the test? Make TestRouterSecurityManager#getUserGroupForTesting private and static. What issues are you facing for using WebHDFS to get a token? The Router you are getting should already provide a fully initialized HTTP server right? I think we should at least go through the HTTP server.
            crh CR Hota added a comment -

            elgoiri Thanks for the review.

            Verify token is used by JspHelper for validating urlencoded tokens send via web calls.

            I still haven't got the root cause of http server issue. But essentially, in AbstractFSContractTestBase.java#setup (java.net.ConnectException: 0.0.0.0:xxxxx: Connection refused) which is a test "@before" method fails to connect to secured cluster via webhdfs to create test dir. I will have to dig further on why http server initialization with security enabled isn't correct. There is no existing test hence we didn't know. Just attached the sample code am trying for reference.

            One more approach am thinking is to initialize a non secured http server and allow delegation token operations for testing purposes even if the server is non secured.

            crh CR Hota added a comment - elgoiri Thanks for the review. Verify token is used by JspHelper for validating urlencoded tokens send via web calls. I still haven't got the root cause of http server issue. But essentially, in AbstractFSContractTestBase.java#setup (java.net.ConnectException: 0.0.0.0:xxxxx: Connection refused) which is a test "@before" method fails to connect to secured cluster via webhdfs to create test dir. I will have to dig further on why http server initialization with security enabled isn't correct. There is no existing test hence we didn't know. Just attached the sample code am trying for reference. One more approach am thinking is to initialize a non secured http server and allow delegation token operations for testing purposes even if the server is non secured.
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 0s Docker mode activated.
            -1 patch 0m 8s HDFS-13972 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



            Subsystem Report/Notes
            JIRA Issue HDFS-13972
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26462/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 8s HDFS-13972 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HDFS-13972 Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26462/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 15s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 15m 58s HDFS-13891 passed
            +1 compile 0m 32s HDFS-13891 passed
            +1 checkstyle 0m 24s HDFS-13891 passed
            +1 mvnsite 0m 33s HDFS-13891 passed
            +1 shadedclient 11m 58s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 51s HDFS-13891 passed
            +1 javadoc 0m 36s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 27s the patch passed
            +1 compile 0m 26s the patch passed
            +1 javac 0m 26s the patch passed
            +1 checkstyle 0m 16s the patch passed
            +1 mvnsite 0m 30s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 xml 0m 2s The patch has no ill-formed XML file.
            +1 shadedclient 12m 7s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 59s the patch passed
            +1 javadoc 0m 34s the patch passed
                  Other Tests
            +1 unit 23m 52s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 20s The patch does not generate ASF License warnings.
            71m 31s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962375/HDFS-13972-HDFS-13891.005.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
            uname Linux e0b8ef448f7a 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / c359a52
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26470/testReport/
            Max. process+thread count 1372 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26470/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 15m 58s HDFS-13891 passed +1 compile 0m 32s HDFS-13891 passed +1 checkstyle 0m 24s HDFS-13891 passed +1 mvnsite 0m 33s HDFS-13891 passed +1 shadedclient 11m 58s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 51s HDFS-13891 passed +1 javadoc 0m 36s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 27s the patch passed +1 compile 0m 26s the patch passed +1 javac 0m 26s the patch passed +1 checkstyle 0m 16s the patch passed +1 mvnsite 0m 30s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 xml 0m 2s The patch has no ill-formed XML file. +1 shadedclient 12m 7s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 59s the patch passed +1 javadoc 0m 34s the patch passed       Other Tests +1 unit 23m 52s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 71m 31s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962375/HDFS-13972-HDFS-13891.005.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml uname Linux e0b8ef448f7a 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / c359a52 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26470/testReport/ Max. process+thread count 1372 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26470/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            crh CR Hota added a comment -

            elgoiri
            I introduced a similar flag as hdfs (DFS_ROUTER_DELEGATION_TOKEN_ALWAYS_USE_KEY) to initialize secret manager for testing. Using this new flag, all web based token related calls could be wired. HDFS-13972-HDFS-13891.005.patch also incorporates your previous review comments.

            The only thing I din't change is the below comment. This ternary check also exists in current hdfs. Extracting this makes code very verbose as all input params to the function are final modifiers.

            • Can we extract the ternary in RouterWebHdfsMethods#662?

             

            crh CR Hota added a comment - elgoiri I introduced a similar flag as hdfs (DFS_ROUTER_DELEGATION_TOKEN_ALWAYS_USE_KEY) to initialize secret manager for testing. Using this new flag, all web based token related calls could be wired. HDFS-13972 - HDFS-13891 .005.patch also incorporates your previous review comments. The only thing I din't change is the below comment. This ternary check also exists in current hdfs. Extracting this makes code very verbose as all input params to the function are final modifiers. Can we extract the ternary in  RouterWebHdfsMethods#662 ?  
            elgoiri Íñigo Goiri added a comment -

            I'm not a big fan of adding a new config key for testing.
            A couple options I can think are:

            • Initialize the cluster and the access the Router to replace the security manager.
            • Create a new auth method for testing which initializes the RouterSecurityManager.

            In general I would do a test without the full MiniDFSRouterCluster where we just start a Router and mock the interactions with the Namenode.
            I don't think we need to interact with a Namenode anyway.

            elgoiri Íñigo Goiri added a comment - I'm not a big fan of adding a new config key for testing. A couple options I can think are: Initialize the cluster and the access the Router to replace the security manager. Create a new auth method for testing which initializes the RouterSecurityManager. In general I would do a test without the full MiniDFSRouterCluster where we just start a Router and mock the interactions with the Namenode. I don't think we need to interact with a Namenode anyway.
            elgoiri Íñigo Goiri added a comment -

            crh, I added HDFS-13972-HDFS-13891.006.patch.
            This adds a new test TestRouterHttpDelegationToken which only starts a Router and uses the mocking etc.
            I think this covers what we wanted right?
            This is also pretty fast as it only starts a Router.

            elgoiri Íñigo Goiri added a comment - crh , I added HDFS-13972-HDFS-13891.006.patch . This adds a new test TestRouterHttpDelegationToken which only starts a Router and uses the mocking etc. I think this covers what we wanted right? This is also pretty fast as it only starts a Router.
            hadoopqa Hadoop QA added a comment -
            -1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 27s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 18m 10s HDFS-13891 passed
            +1 compile 0m 32s HDFS-13891 passed
            +1 checkstyle 0m 22s HDFS-13891 passed
            +1 mvnsite 0m 34s HDFS-13891 passed
            +1 shadedclient 12m 20s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 55s HDFS-13891 passed
            +1 javadoc 0m 38s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 30s the patch passed
            +1 compile 0m 26s the patch passed
            +1 javac 0m 26s the patch passed
            +1 checkstyle 0m 16s the patch passed
            +1 mvnsite 0m 30s the patch passed
            -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
            +1 xml 0m 1s The patch has no ill-formed XML file.
            +1 shadedclient 12m 49s patch has no errors when building and testing our client artifacts.
            +1 findbugs 1m 9s the patch passed
            +1 javadoc 0m 35s the patch passed
                  Other Tests
            +1 unit 24m 57s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 27s The patch does not generate ASF License warnings.
            76m 28s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962409/HDFS-13972-HDFS-13891.006.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml
            uname Linux db796ef023cf 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / c359a52
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/26471/artifact/out/whitespace-eol.txt
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26471/testReport/
            Max. process+thread count 1010 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26471/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 27s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 18m 10s HDFS-13891 passed +1 compile 0m 32s HDFS-13891 passed +1 checkstyle 0m 22s HDFS-13891 passed +1 mvnsite 0m 34s HDFS-13891 passed +1 shadedclient 12m 20s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 55s HDFS-13891 passed +1 javadoc 0m 38s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 30s the patch passed +1 compile 0m 26s the patch passed +1 javac 0m 26s the patch passed +1 checkstyle 0m 16s the patch passed +1 mvnsite 0m 30s the patch passed -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply +1 xml 0m 1s The patch has no ill-formed XML file. +1 shadedclient 12m 49s patch has no errors when building and testing our client artifacts. +1 findbugs 1m 9s the patch passed +1 javadoc 0m 35s the patch passed       Other Tests +1 unit 24m 57s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 76m 28s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962409/HDFS-13972-HDFS-13891.006.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle xml uname Linux db796ef023cf 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / c359a52 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 whitespace https://builds.apache.org/job/PreCommit-HDFS-Build/26471/artifact/out/whitespace-eol.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26471/testReport/ Max. process+thread count 1010 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26471/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            crh CR Hota added a comment -

            elgoiri

            Thanks a ton for taking a stab at this ! Highly appreciate it.

            I tried to introduce this DFS_ROUTER_DELEGATION_TOKEN_ALWAYS_USE_KEY seeing how hdfs solves similar problem of token verifications in test mode, looking at what you did it should be fine for our testing. I was trying to make the current entire minirouterdfs set-up work.

            Cleaned up the code and uploaded 007 patch.

            crh CR Hota added a comment - elgoiri Thanks a ton for taking a stab at this ! Highly appreciate it. I tried to introduce this DFS_ROUTER_DELEGATION_TOKEN_ALWAYS_USE_KEY seeing how hdfs solves similar problem of token verifications in test mode, looking at what you did it should be fine for our testing. I was trying to make the current entire minirouterdfs set-up work. Cleaned up the code and uploaded 007 patch.
            elgoiri Íñigo Goiri added a comment - - edited

            One of the things I couldn't figure is why the renew token didn't extend the max data for the token (left the TODO).
            My theory is that the MockSecretManager values are preventing this.
            However, I couldn't figure out exactly what was it.
            We have the same issue in the RPC version.

            BTW, any chance we can prevent having:

            RouterHDFSContract.createCluster(initSecurity());
            

            In TestRouterSecurityManager#testCreateCredentials() and just use a Router?

            elgoiri Íñigo Goiri added a comment - - edited One of the things I couldn't figure is why the renew token didn't extend the max data for the token (left the TODO). My theory is that the MockSecretManager values are preventing this. However, I couldn't figure out exactly what was it. We have the same issue in the RPC version. BTW, any chance we can prevent having: RouterHDFSContract.createCluster(initSecurity()); In TestRouterSecurityManager#testCreateCredentials() and just use a Router?
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 20s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 16m 47s HDFS-13891 passed
            +1 compile 0m 26s HDFS-13891 passed
            +1 checkstyle 0m 21s HDFS-13891 passed
            +1 mvnsite 0m 32s HDFS-13891 passed
            +1 shadedclient 11m 42s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 54s HDFS-13891 passed
            +1 javadoc 0m 30s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 28s the patch passed
            +1 compile 0m 27s the patch passed
            +1 javac 0m 27s the patch passed
            +1 checkstyle 0m 15s the patch passed
            +1 mvnsite 0m 29s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 11m 20s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 57s the patch passed
            +1 javadoc 0m 35s the patch passed
                  Other Tests
            +1 unit 22m 29s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 24s The patch does not generate ASF License warnings.
            69m 44s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962435/HDFS-13972-HDFS-13891.007.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux b4a4097a32f3 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / c359a52
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26475/testReport/
            Max. process+thread count 1485 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26475/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 20s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 16m 47s HDFS-13891 passed +1 compile 0m 26s HDFS-13891 passed +1 checkstyle 0m 21s HDFS-13891 passed +1 mvnsite 0m 32s HDFS-13891 passed +1 shadedclient 11m 42s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 54s HDFS-13891 passed +1 javadoc 0m 30s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 28s the patch passed +1 compile 0m 27s the patch passed +1 javac 0m 27s the patch passed +1 checkstyle 0m 15s the patch passed +1 mvnsite 0m 29s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 11m 20s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 57s the patch passed +1 javadoc 0m 35s the patch passed       Other Tests +1 unit 22m 29s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 24s The patch does not generate ASF License warnings. 69m 44s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962435/HDFS-13972-HDFS-13891.007.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux b4a4097a32f3 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / c359a52 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26475/testReport/ Max. process+thread count 1485 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26475/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            crh CR Hota added a comment - - edited

            elgoiri

            Took a look at this now.

            Mock values are fine. The assert has to be changed, its incorrectly checking >= instead of a <= . Max date (typically 7 days) of a token will never change irrespective of number of renewals. What changes is expiry date (which is different than max date) if not renewed within a certain interval. Expiry date should always be <= maxDate. The equals is needed because towards the end of almost 7 days if client tries to renew a token expiry date will be send back as the maxDate as after maxDate token becomes invalid and brand new token is needed to continue as renewals wont work.

            Will change it in here first for web and then in RPC through a separate ticket to make sure we pass the context to users correctly of why the change is needed.

            For TestRouterSecurityManager#testCreateCredentials I am leaving initialization of routers as is to keep it consistent with the similar tests and for brevity. I think at some point we may have to refactor createCluster completely to take configs and create appropriate clusters such as ALL, ROUTER, ROUTER_RPC, ROUTER_HTTP etc. Thoughts?

             

            crh CR Hota added a comment - - edited elgoiri Took a look at this now. Mock values are fine. The assert has to be changed, its incorrectly checking >= instead of a <= . Max date (typically 7 days) of a token will never change irrespective of number of renewals. What changes is expiry date (which is different than max date) if not renewed within a certain interval. Expiry date should always be <= maxDate. The equals is needed because towards the end of almost 7 days if client tries to renew a token expiry date will be send back as the maxDate as after maxDate token becomes invalid and brand new token is needed to continue as renewals wont work. Will change it in here first for web and then in RPC through a separate ticket to make sure we pass the context to users correctly of why the change is needed. For TestRouterSecurityManager#testCreateCredentials I am leaving initialization of routers as is to keep it consistent with the similar tests and for brevity. I think at some point we may have to refactor createCluster completely to take configs and create appropriate clusters such as ALL, ROUTER, ROUTER_RPC, ROUTER_HTTP etc. Thoughts?  
            elgoiri Íñigo Goiri added a comment -

            For TestRouterSecurityManager#testCreateCredentials I am leaving initialization of routers as is to keep it consistent with the similar tests and for brevity. I think at some point we may have to refactor createCluster completely to take configs and create appropriate clusters such as ALL, ROUTER, ROUTER_RPC, ROUTER_HTTP etc. Thoughts?

            You can already set the Router config to just be some of the services using RouterConfigBuilder, right?

            elgoiri Íñigo Goiri added a comment - For TestRouterSecurityManager#testCreateCredentials I am leaving initialization of routers as is to keep it consistent with the similar tests and for brevity. I think at some point we may have to refactor createCluster completely to take configs and create appropriate clusters such as ALL, ROUTER, ROUTER_RPC, ROUTER_HTTP etc. Thoughts? You can already set the Router config to just be some of the services using RouterConfigBuilder, right?
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 21s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 17m 17s HDFS-13891 passed
            +1 compile 0m 30s HDFS-13891 passed
            +1 checkstyle 0m 21s HDFS-13891 passed
            +1 mvnsite 0m 32s HDFS-13891 passed
            +1 shadedclient 12m 12s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 50s HDFS-13891 passed
            +1 javadoc 0m 35s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 28s the patch passed
            +1 compile 0m 25s the patch passed
            +1 javac 0m 25s the patch passed
            +1 checkstyle 0m 16s the patch passed
            +1 mvnsite 0m 28s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 12m 37s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 57s the patch passed
            +1 javadoc 0m 30s the patch passed
                  Other Tests
            +1 unit 23m 9s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 25s The patch does not generate ASF License warnings.
            72m 40s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962512/HDFS-13972-HDFS-13891.008.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux c459c27ba210 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / c359a52
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26480/testReport/
            Max. process+thread count 1006 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26480/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 21s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 17m 17s HDFS-13891 passed +1 compile 0m 30s HDFS-13891 passed +1 checkstyle 0m 21s HDFS-13891 passed +1 mvnsite 0m 32s HDFS-13891 passed +1 shadedclient 12m 12s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 50s HDFS-13891 passed +1 javadoc 0m 35s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 28s the patch passed +1 compile 0m 25s the patch passed +1 javac 0m 25s the patch passed +1 checkstyle 0m 16s the patch passed +1 mvnsite 0m 28s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 12m 37s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 57s the patch passed +1 javadoc 0m 30s the patch passed       Other Tests +1 unit 23m 9s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 25s The patch does not generate ASF License warnings. 72m 40s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962512/HDFS-13972-HDFS-13891.008.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux c459c27ba210 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / c359a52 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26480/testReport/ Max. process+thread count 1006 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26480/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 41s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 21m 57s HDFS-13891 passed
            +1 compile 0m 32s HDFS-13891 passed
            +1 checkstyle 0m 22s HDFS-13891 passed
            +1 mvnsite 0m 35s HDFS-13891 passed
            +1 shadedclient 13m 0s branch has no errors when building and testing our client artifacts.
            +1 findbugs 1m 0s HDFS-13891 passed
            +1 javadoc 0m 37s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 28s the patch passed
            +1 compile 0m 25s the patch passed
            +1 javac 0m 25s the patch passed
            +1 checkstyle 0m 17s the patch passed
            +1 mvnsite 0m 28s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 12m 41s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 57s the patch passed
            +1 javadoc 0m 32s the patch passed
                  Other Tests
            +1 unit 22m 53s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 25s The patch does not generate ASF License warnings.
            78m 39s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962532/HDFS-13972-HDFS-13891.009.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux efc54a0bddba 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / c359a52
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26481/testReport/
            Max. process+thread count 1055 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26481/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 41s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 21m 57s HDFS-13891 passed +1 compile 0m 32s HDFS-13891 passed +1 checkstyle 0m 22s HDFS-13891 passed +1 mvnsite 0m 35s HDFS-13891 passed +1 shadedclient 13m 0s branch has no errors when building and testing our client artifacts. +1 findbugs 1m 0s HDFS-13891 passed +1 javadoc 0m 37s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 28s the patch passed +1 compile 0m 25s the patch passed +1 javac 0m 25s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 28s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 12m 41s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 57s the patch passed +1 javadoc 0m 32s the patch passed       Other Tests +1 unit 22m 53s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 25s The patch does not generate ASF License warnings. 78m 39s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12962532/HDFS-13972-HDFS-13891.009.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux efc54a0bddba 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / c359a52 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26481/testReport/ Max. process+thread count 1055 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26481/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            crh CR Hota added a comment -

            elgoiri

            Thanks for your earlier comment.

            In HDFS-13972-HDFS-13891.009.patch added a stripped down router to test createCredentials, but feel we should re-look at createCluster for better code structure and re-usability overall. Will look at this once we wrap up security related feature.

            BTW, I also added RPC token asserts to reflect renewal expiry time vs max date difference instead of a separate ticket.

            crh CR Hota added a comment - elgoiri Thanks for your earlier comment. In HDFS-13972 - HDFS-13891 .009.patch added a stripped down router to test createCredentials, but feel we should re-look at createCluster for better code structure and re-usability overall. Will look at this once we wrap up security related feature. BTW, I also added RPC token asserts to reflect renewal expiry time vs max date difference instead of a separate ticket.
            elgoiri Íñigo Goiri added a comment -

            The tests look good now.

            For the record, in HDFS-14351 I did some work to make the tests lighter too.
            In most cases we don't really need full Namenodes and they are pretty heavy.
            I think we can do a lighter version of the mini DFS cluster for the Routers.

            elgoiri Íñigo Goiri added a comment - The tests look good now. For the record, in HDFS-14351 I did some work to make the tests lighter too. In most cases we don't really need full Namenodes and they are pretty heavy. I think we can do a lighter version of the mini DFS cluster for the Routers.
            crh CR Hota added a comment -

            elgoiri Thanks for the info. This is helpful.

             

            brahmareddy ajisakaa surendrasingh

            Could you help take a look at HDFS-13972-HDFS-13891.009.patch as well before we commit?

            crh CR Hota added a comment - elgoiri Thanks for the info. This is helpful.   brahmareddy ajisakaa surendrasingh Could you help take a look at  HDFS-13972 - HDFS-13891 .009.patch as well before we commit?
            aajisaka Akira Ajisaka added a comment -

            Thanks elgoiri and crh for the work.

            In TestRouterSecurityManager#testCreateCredentials, I'm thinking it's better to assert that there is at least one token in creds.

                Credentials creds = RouterSecurityManager.createCredentials(
                    router, ugi, "some_renewer");
                for (Token token : creds.getAllTokens()) {
            

            The rest looks good to me.

            aajisaka Akira Ajisaka added a comment - Thanks elgoiri and crh for the work. In TestRouterSecurityManager#testCreateCredentials, I'm thinking it's better to assert that there is at least one token in creds . Credentials creds = RouterSecurityManager.createCredentials( router, ugi, "some_renewer" ); for (Token token : creds.getAllTokens()) { The rest looks good to me.

            Thanks crh for the patch.

            One issue I observed here in webhdfs, not sure we should handle here or not.

            RouterWebHdfsMethods#redirectURI (Line #399) is replacing only the routerId in redirected URL but is should replace the router token also with namenode token. It should be same as RouterWebHdfsMethods#redirectURI ((Line #504).

            // We modify the namenode location and the path
            redirectLocation = redirectLocation
            .replaceAll("(?<=[?&;])namenoderpcaddress=.*?(?=[&;])",
            "namenoderpcaddress=" + router.getRouterId())
            .replaceAll("(?<=[/])webhdfs/v1/.*?(?=[?])",
            "webhdfs/v1" + path);

            Currently CREATE will fail without this change.

            surendralilhore Surendra Singh Lilhore added a comment - Thanks crh for the patch. One issue I observed here in webhdfs, not sure we should handle here or not. RouterWebHdfsMethods#redirectURI (Line #399) is replacing only the routerId in redirected URL but is should replace the router token also with namenode token. It should be same as RouterWebHdfsMethods#redirectURI ((Line #504). // We modify the namenode location and the path redirectLocation = redirectLocation .replaceAll( "(?<=[?&;])namenoderpcaddress=.*?(?=[&;])" , "namenoderpcaddress=" + router.getRouterId()) .replaceAll( "(?<=[/])webhdfs/v1/.*?(?=[?])" , "webhdfs/v1" + path); Currently CREATE will fail without this change.
            elgoiri Íñigo Goiri added a comment -

            Thanks surendrasingh for bringing this up.
            I think we should do it here; this is the basis of DT in WebHDFS.

            elgoiri Íñigo Goiri added a comment - Thanks surendrasingh for bringing this up. I think we should do it here; this is the basis of DT in WebHDFS.
            crh CR Hota added a comment -

            surendrasingh ajisakaa elgoiri 

            Thanks for the review. We should definitely support DT for create and do the change here. Will work on the change and submit a patch soon.

            crh CR Hota added a comment - surendrasingh ajisakaa elgoiri   Thanks for the review. We should definitely support DT for create and do the change here. Will work on the change and submit a patch soon.
            crh CR Hota added a comment -

            surendrasingh

            In the case of OPEN/APPEND etc, clients are redirected to datanode and datanode then connects to router back for operation. Routers are the issuer of the delegation token.

            But for CREATE, since the first redirect is to the appropriate namenode, router has to try and create delegation token for the namenode. This has 2 issues, one clients will be able to get details of downstream namenode along with delegation token for that namenode exposing details of how federation is set-up, second this flow creates 2 redirects (instead of one) which is inefficient.

            I think router should send back a redirect to datanode (instead of namenode) for CREATE calls with router's address and DT itself. This needs fundamental change in how "create" works in router web.

             

            crh CR Hota added a comment - surendrasingh In the case of OPEN/APPEND etc, clients are redirected to datanode and datanode then connects to router back for operation. Routers are the issuer of the delegation token. But for CREATE, since the first redirect is to the appropriate namenode, router has to try and create delegation token for the namenode. This has 2 issues, one clients will be able to get details of downstream namenode along with delegation token for that namenode exposing details of how federation is set-up, second this flow creates 2 redirects (instead of one) which is inefficient. I think router should send back a redirect to datanode (instead of namenode) for CREATE calls with router's address and DT itself. This needs fundamental change in how "create" works in router web.  

            I think router should send back a redirect to datanode (instead of namenode) for CREATE calls with router's address and DT itself. This needs fundamental change in how "create" works in router web.

            Yes, this is fine.

            surendralilhore Surendra Singh Lilhore added a comment - I think router should send back a redirect to datanode (instead of namenode) for CREATE calls with router's address and DT itself. This needs fundamental change in how "create" works in router web. Yes, this is fine.
            crh CR Hota added a comment -

            surendrasingh elgoiri

            Thanks for the previous comment. Changed CREATE to redirect directly to datanode instead of namenode. One important change added is to make "getDatanodereport" use superuser admin credentials to connect to namenode instead of the upstream user/client.

            There are couple of optimizations which we can target through another Jira as these are not directly about webhdfs security. Have added a todo comment to introduce a better way of handling methods that will use super user credentials. The other is to locate a datanode which is closer to client for create calls. 

             

            crh CR Hota added a comment - surendrasingh elgoiri Thanks for the previous comment. Changed CREATE to redirect directly to datanode instead of namenode. One important change added is to make "getDatanodereport" use superuser admin credentials to connect to namenode instead of the upstream user/client. There are couple of optimizations which we can target through another Jira as these are not directly about webhdfs security. Have added a todo comment to introduce a better way of handling methods that will use super user credentials. The other is to locate a datanode which is closer to client for create calls.   
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 33s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 17m 46s HDFS-13891 passed
            +1 compile 0m 29s HDFS-13891 passed
            +1 checkstyle 0m 22s HDFS-13891 passed
            +1 mvnsite 0m 32s HDFS-13891 passed
            +1 shadedclient 12m 33s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 58s HDFS-13891 passed
            +1 javadoc 0m 35s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 28s the patch passed
            +1 compile 0m 26s the patch passed
            +1 javac 0m 26s the patch passed
            -0 checkstyle 0m 16s hadoop-hdfs-project/hadoop-hdfs-rbf: The patch generated 9 new + 15 unchanged - 0 fixed = 24 total (was 15)
            +1 mvnsite 0m 29s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 12m 47s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 57s the patch passed
            +1 javadoc 0m 33s the patch passed
                  Other Tests
            +1 unit 22m 54s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 25s The patch does not generate ASF License warnings.
            73m 48s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12963082/HDFS-13972-HDFS-13891.010.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux 3ac96d16f2af 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / 0c686b7
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/26498/artifact/out/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs-rbf.txt
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26498/testReport/
            Max. process+thread count 1002 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26498/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 33s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 17m 46s HDFS-13891 passed +1 compile 0m 29s HDFS-13891 passed +1 checkstyle 0m 22s HDFS-13891 passed +1 mvnsite 0m 32s HDFS-13891 passed +1 shadedclient 12m 33s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 58s HDFS-13891 passed +1 javadoc 0m 35s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 28s the patch passed +1 compile 0m 26s the patch passed +1 javac 0m 26s the patch passed -0 checkstyle 0m 16s hadoop-hdfs-project/hadoop-hdfs-rbf: The patch generated 9 new + 15 unchanged - 0 fixed = 24 total (was 15) +1 mvnsite 0m 29s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 12m 47s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 57s the patch passed +1 javadoc 0m 33s the patch passed       Other Tests +1 unit 22m 54s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 25s The patch does not generate ASF License warnings. 73m 48s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12963082/HDFS-13972-HDFS-13891.010.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux 3ac96d16f2af 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / 0c686b7 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/26498/artifact/out/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs-rbf.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26498/testReport/ Max. process+thread count 1002 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26498/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            elgoiri Íñigo Goiri added a comment -

            Thanks crh for HDFS-13972-HDFS-13891.010.patch.
            Can we leave getDatanodeReport() as it was and target the proper change in a follow up JIRA? Or is it needed for this to work?
            Some simple comments:

            • Fix the check style warnings.
            • TestRouterHttpDelegationToken#87 is repeated (I know... I added this ).
            • In TestRouterHttpDelegationToken#setup we should use the RouterConfigBuilder and just set the needed services instead of disabling all (same... that was me).
            elgoiri Íñigo Goiri added a comment - Thanks crh for HDFS-13972-HDFS-13891.010.patch . Can we leave getDatanodeReport() as it was and target the proper change in a follow up JIRA? Or is it needed for this to work? Some simple comments: Fix the check style warnings. TestRouterHttpDelegationToken#87 is repeated (I know... I added this ). In TestRouterHttpDelegationToken#setup we should use the RouterConfigBuilder and just set the needed services instead of disabling all (same... that was me).
            crh CR Hota added a comment -

            elgoiri

            Thanks for the review. "getDatanodeReport" is invoked in for create webhdfs calls. super user credentials are needed for this to work. To optimize, I am planning to open a new Jira but to support create we need this change.

            For other points, will work on them finally.

             

            surendrasingh ajisakaa brahmareddy

            Could you please help take a look at HDFS-13972-HDFS-13891.010.patch ?

             

            crh CR Hota added a comment - elgoiri Thanks for the review. "getDatanodeReport" is invoked in for create webhdfs calls. super user credentials are needed for this to work. To optimize, I am planning to open a new Jira but to support create we need this change. For other points, will work on them finally.   surendrasingh ajisakaa brahmareddy Could you please help take a look at  HDFS-13972 - HDFS-13891 .010.patch ?  
            crh CR Hota added a comment -

            surendrasingh brahmareddy ajisakaa Gentle reminder !

            Could you help take a look at the latest patch 010 and share final thoughts?

             

            crh CR Hota added a comment - surendrasingh brahmareddy ajisakaa Gentle reminder ! Could you help take a look at the latest patch 010 and share final thoughts?  

            crh thanks for working on this.

              

             

            I think router should send back a redirect to datanode (instead of namenode) for CREATE calls with router's address and DT itself. This needs fundamental change in how "create" works in router web.

            Yes, we shouldn't expose namenode details to client. Even you can remove namenode attribute ("name.node),may followup jira.

             

             

            Couple of issues/queries on HDFS-13972-HDFS-13891.010.patch

             

            437 final Token<? extends TokenIdentifier> t = generateDelegationToken(
            438 ugi, request.getUserPrincipal().getName());
            

             i) request.getUserPrincipal().getName()  needs to replace with ugi.getUserName(), as this can throw exception as servlet context can be different.

            ,ii) generateDelegationToken(..) should be implemented in router, Orelse this request will fail while creating Credentials as it's expect namenode but it will return the router.See, the following for same.

            public Credentials createCredentials(final UserGroupInformation ugi,
             final String renewer) throws IOException {
             final NameNode namenode = (NameNode)context.getAttribute("name.node");
             final Credentials c = DelegationTokenSecretManager.createCredentials(
             namenode, ugi, renewer != null? renewer: ugi.getShortUserName());
             return c;
            }

             

            So, finally it should like following.

            // generate a token
            final Token<? extends TokenIdentifier> t = generateDelegationToken(router, ugi, ugi.getUserName());

             

            Please  check all the webhdfs commands.

            brahmareddy Brahma Reddy Battula added a comment - crh thanks for working on this.      I think router should send back a redirect to datanode (instead of namenode) for CREATE calls with router's address and DT itself. This needs fundamental change in how "create" works in router web. Yes, we shouldn't expose namenode details to client. Even you can remove namenode attribute ("name.node),may followup jira.     Couple of issues/queries on  HDFS-13972 - HDFS-13891 .010.patch   437 final Token<? extends TokenIdentifier> t = generateDelegationToken( 438 ugi, request.getUserPrincipal().getName());  i) request.getUserPrincipal().getName()  needs to replace with ugi.getUserName(), as this can throw exception as servlet context can be different. ,ii) generateDelegationToken(..) should be implemented in router, Orelse this request will fail while creating Credentials as it's expect namenode but it will return the router.See, the following for same. public Credentials createCredentials( final UserGroupInformation ugi, final String renewer) throws IOException { final NameNode namenode = (NameNode)context.getAttribute( "name.node" ); final Credentials c = DelegationTokenSecretManager.createCredentials( namenode, ugi, renewer != null ? renewer: ugi.getShortUserName()); return c; }   So, finally it should like following. // generate a token final Token<? extends TokenIdentifier> t = generateDelegationToken(router, ugi, ugi.getUserName());   Please  check all the webhdfs commands.
            crh CR Hota added a comment -

            brahmareddy Thanks for reviewing the change.

            Could you help better understand the generateDelegationToken function issue you described. This flow is tested and works fine. createCredentials is overridden in RouterWebHdfsMethods.java. To accomplish this we had worked on HDFS-14302

            crh CR Hota added a comment - brahmareddy Thanks for reviewing the change. Could you help better understand the generateDelegationToken function issue you described. This flow is tested and works fine. createCredentials is overridden in RouterWebHdfsMethods.java. To accomplish this we had worked on  HDFS-14302 . 

            Oh,Yes.. I missed HDFS-14302 in my local. 

            brahmareddy Brahma Reddy Battula added a comment - Oh,Yes.. I missed  HDFS-14302 in my local. 
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 34s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 17m 5s HDFS-13891 passed
            +1 compile 0m 28s HDFS-13891 passed
            +1 checkstyle 0m 18s HDFS-13891 passed
            +1 mvnsite 0m 31s HDFS-13891 passed
            +1 shadedclient 11m 6s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 52s HDFS-13891 passed
            +1 javadoc 0m 35s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 27s the patch passed
            +1 compile 0m 26s the patch passed
            +1 javac 0m 26s the patch passed
            +1 checkstyle 0m 16s the patch passed
            +1 mvnsite 0m 28s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 11m 39s patch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 57s the patch passed
            +1 javadoc 0m 29s the patch passed
                  Other Tests
            +1 unit 22m 25s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 30s The patch does not generate ASF License warnings.
            69m 56s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12964506/HDFS-13972-HDFS-13891.011.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux aa3a1a9c86ba 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / dea3798
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26565/testReport/
            Max. process+thread count 1414 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26565/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 34s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 17m 5s HDFS-13891 passed +1 compile 0m 28s HDFS-13891 passed +1 checkstyle 0m 18s HDFS-13891 passed +1 mvnsite 0m 31s HDFS-13891 passed +1 shadedclient 11m 6s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 52s HDFS-13891 passed +1 javadoc 0m 35s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 27s the patch passed +1 compile 0m 26s the patch passed +1 javac 0m 26s the patch passed +1 checkstyle 0m 16s the patch passed +1 mvnsite 0m 28s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 11m 39s patch has no errors when building and testing our client artifacts. +1 findbugs 0m 57s the patch passed +1 javadoc 0m 29s the patch passed       Other Tests +1 unit 22m 25s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 30s The patch does not generate ASF License warnings. 69m 56s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12964506/HDFS-13972-HDFS-13891.011.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux aa3a1a9c86ba 4.4.0-139-generic #165-Ubuntu SMP Wed Oct 24 10:58:50 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / dea3798 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26565/testReport/ Max. process+thread count 1414 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26565/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            elgoiri Íñigo Goiri added a comment -

            Not very convinced that getDatanodeReport() should be done the way HDFS-13972-HDFS-13891.011.patch does.
            Can we address this in a follow up?

            elgoiri Íñigo Goiri added a comment - Not very convinced that getDatanodeReport() should be done the way HDFS-13972-HDFS-13891.011.patch does. Can we address this in a follow up?
            crh CR Hota added a comment -

            elgoiri Thanks for the review.

            Could you help me understand your concerns better?

            Sadly, without this change, we can't support secure create calls. Create calls would need datanodes to be able to redirect requests. Challenge is datanode report needs super user creds in secured env.

            Other ways possible are

            1. To relax datanode report functionality in hdfs to NOT expect superuser creds. Am not sure if this is the right approach wrt security i.e exposing all datanodes to users without superuser creds.
            2. To forward requests to namenode first. This would mean 2 redirects and also dependency of router to generate yet another token for downstream namenode. This flow would be significantly more inefficient compared to current approach.
            3. Keep a copy of getDatanode report in a async manner in router, this would again get us into tricky situations of consistency and efficiency. For ex, create may not be used by a customer at all, but router keep invoking this call in regular cadence adding inefficiency and extra load on routers. Also will be hard to keep up to date and consistent view of datanodes for the cluster when a create call is actually invoked.

            If we address this in a follow-up Jira it essentially means not to support secured CREATE calls for the time being. 

            surendrasingh brahmareddy Could you also help share your thoughts here?

            crh CR Hota added a comment - elgoiri Thanks for the review. Could you help me understand your concerns better? Sadly, without this change, we can't support secure create calls. Create calls would need datanodes to be able to redirect requests. Challenge is datanode report needs super user creds in secured env. Other ways possible are To relax datanode report functionality in hdfs to NOT expect superuser creds. Am not sure if this is the right approach wrt security i.e exposing all datanodes to users without superuser creds. To forward requests to namenode first. This would mean 2 redirects and also dependency of router to generate yet another token for downstream namenode. This flow would be significantly more inefficient compared to current approach. Keep a copy of getDatanode report in a async manner in router, this would again get us into tricky situations of consistency and efficiency. For ex, create may not be used by a customer at all, but router keep invoking this call in regular cadence adding inefficiency and extra load on routers. Also will be hard to keep up to date and consistent view of datanodes for the cluster when a create call is actually invoked. If we address this in a follow-up Jira it essentially means not to support secured CREATE calls for the time being.  surendrasingh   brahmareddy Could you also help share your thoughts here?
            elgoiri Íñigo Goiri added a comment -

            Thanks crh for laying down the issues with the datanode report.
            My main concern is with making it superuser by default.
            If we need to know the nodes within the CREATE method, we should just use the superuser for that call internally.
            Actually, the current code in chooseDatanode() is already doing loginUser.doAs().
            Why isn't this enough? What is the code path to get to getDatanodeReport()?

            elgoiri Íñigo Goiri added a comment - Thanks crh for laying down the issues with the datanode report. My main concern is with making it superuser by default. If we need to know the nodes within the CREATE method, we should just use the superuser for that call internally. Actually, the current code in chooseDatanode() is already doing loginUser.doAs() . Why isn't this enough? What is the code path to get to getDatanodeReport() ?
            crh CR Hota added a comment -

            elgoiri Thanks for taking a look.

            getDatanodeReport in secured cluster will need superuser creds irrespective of code path. In chooseDatanode there is loginUser.doAs which is fundamentally using user creds and NOT superuser creds. Even if that is removed, RouterRpcClient will still use ugi that belongs to end user and not router superuser in the invokeConcurrent method. This happens because ugi is constructed in RouterRpcClient by doing

            final UserGroupInformation ugi = RouterRpcServer.getRemoteUser();
            

            Hence this change is necessary for router to talk to namenode with super user privilege. As per the change this is ONLY for getDatanodeReport.

            crh CR Hota added a comment - elgoiri Thanks for taking a look. getDatanodeReport in secured cluster will need superuser creds irrespective of code path. In chooseDatanode there is loginUser.doAs which is fundamentally using user creds and NOT superuser creds. Even if that is removed, RouterRpcClient will still use ugi that belongs to end user and not router superuser in the invokeConcurrent method. This happens because ugi is constructed in RouterRpcClient by doing final UserGroupInformation ugi = RouterRpcServer.getRemoteUser(); Hence this change is necessary for router to talk to namenode with super user privilege. As per the change this is ONLY for getDatanodeReport.

             As per the change this is ONLY for getDatanodeReport.

            IMO, RouterRpcServer.getRemoteUser() can be replaced with UserGroupInformation.getCurrentUser().Anyway handler will get remote user and execute. we can change for invokeSingle(..), invokeConcurrent(..) and invokeSequential(..) .

             

            The fix HDFS-13972-HDFS-13891.011.patch is not proper,as this is not specific to client protocol(generic).Later if we found some other API again we need to filter.

            brahmareddy Brahma Reddy Battula added a comment -  As per the change this is ONLY for getDatanodeReport. IMO, RouterRpcServer.getRemoteUser() can be replaced with UserGroupInformation.getCurrentUser() .Anyway handler will get remote user and execute. we can change for invokeSingle(..), invokeConcurrent(..) and invokeSequential(..) .   The fix  HDFS-13972-HDFS-13891.011.patch  is not proper,as this is not specific to client protocol(generic).Later if we found some other API again we need to filter.
            daryn Daryn Sharp added a comment -

            I've only skimmed the Jira based on activity.  I haven't checked what all is in the datanode report but I see no reason to expose getDatanodeReport to non-superusers.  First, it's insanely expensive.  Second, why allow a nefarious user to trivially discover the topology?

            What caught my eye though was the references to ugi.

            1. UserGroupInformation.getCurrentUser() is not a cheap call. If a cached ugi is available that is guaranteed to always be the current ugi, I'd recommend using it.
            2. RPC calls should not be invoked on behalf of a user as the login user. Always use the caller's context or it's a slippery slope to privilege escalation.
            daryn Daryn Sharp added a comment - I've only skimmed the Jira based on activity.  I haven't checked what all is in the datanode report but I see no reason to expose  getDatanodeReport  to non-superusers.  First, it's insanely expensive.  Second, why allow a nefarious user to trivially discover the topology? What caught my eye though was the references to ugi. UserGroupInformation.getCurrentUser() is not a cheap call. If a cached ugi is available that is guaranteed to always be the current ugi, I'd recommend using it. RPC calls should not be invoked on behalf of a user as the login user. Always use the caller's context or it's a slippery slope to privilege escalation.
            crh CR Hota added a comment -

            brahmareddy daryn Thanks for the review.

            Yes, we will restrict using getDatanodeReport to only "CREATE" webhdfs call. Am working on changing the patch to include a privileged ugi for getDatanode method thus not changing client protocol and restricting nefarious users from knowing the cluster topo. Essentially in "chooseDatanode" of RouterWebHdfsMethods, ugi will be replaced by router ugi before getDataNodereport is invoked. This will not change ClientProtocol so a bad user cannot invoke getDatanodeReport with super creds.

            I would like to leave "RouterRpcServer.getRemoteUser()" as is as part of this Jira and handle optimizations as Daryn suggested in a follow-up.

             

            crh CR Hota added a comment - brahmareddy daryn Thanks for the review. Yes, we will restrict using getDatanodeReport to only "CREATE" webhdfs call. Am working on changing the patch to include a privileged ugi for getDatanode method thus not changing client protocol and restricting nefarious users from knowing the cluster topo. Essentially in "chooseDatanode" of RouterWebHdfsMethods, ugi will be replaced by router ugi before getDataNodereport is invoked. This will not change ClientProtocol so a bad user cannot invoke getDatanodeReport with super creds. I would like to leave "RouterRpcServer.getRemoteUser()" as is as part of this Jira and handle optimizations as Daryn suggested in a follow-up.  

             RPC calls should not be invoked on behalf of a user as the login user. Always use the caller's context or it's a slippery slope to privilege escalation.

            Yes, As crh mentioned, getDatanodeReport () will be called by router ( while choosing the datanodes in rotuerwebhdfs).So it's not exposed to user.

            UserGroupInformation.getCurrentUser() is not a cheap call. If a cached ugi is available that is guaranteed to always be the current ugi, I'd recommend using it

            Agree.

             

            We can have one thread local ugi which we can set and reset while choosing the datanode like below and we might not require doAs(..) as it will not used

             

            --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java
            +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java
            @@ -193,6 +193,9 @@
               /** ClientProtocol calls. */
               private final RouterClientProtocol clientProto;
            
            +  private static final ThreadLocal<UserGroupInformation> curUser
            +      = new ThreadLocal<>();
            +
               /**
                * Construct a router RPC server.
                *
            @@ -1423,10 +1426,19 @@ private boolean isPathReadOnly(final String path) {
                * @throws IOException If we cannot get the user information.
                */
               static UserGroupInformation getRemoteUser() throws IOException {
            -    UserGroupInformation ugi = Server.getRemoteUser();
            +    UserGroupInformation ugi = curUser.get();
            +    ugi = (ugi != null) ? ugi : Server.getRemoteUser();
                 return (ugi != null) ? ugi : UserGroupInformation.getCurrentUser();
               }
            
            +  static void setCurrentUser(UserGroupInformation ugi) {
            +    curUser.set(ugi);
            +  }
            +
            +  static void resetCurrentUser() {
            +    curUser.set(null);
            +  }
            +
               /**
                * Merge the outputs from multiple namespaces.
                *
            diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
            index a10764a8fe7..985ace1f273 100644
            --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
            +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
            @@ -552,19 +552,13 @@ private DatanodeInfo chooseDatanode(final Router router,
                 // We need to get the DNs as a privileged user
                 final RouterRpcServer rpcServer = getRPCServer(router);
                 UserGroupInformation loginUser = UserGroupInformation.getLoginUser();
            -
            -    DatanodeInfo[] dns = loginUser.doAs(
            -        new PrivilegedAction<DatanodeInfo[]>() {
            -          @Override
            -          public DatanodeInfo[] run() {
            -            try {
            -              return rpcServer.getDatanodeReport(DatanodeReportType.LIVE);
            -            } catch (IOException e) {
            -              LOG.error("Cannot get the datanodes from the RPC server", e);
            -              return null;
            -            }
            -          }
            -        });
            +    RouterRpcServer.setCurrentUser(loginUser);
            +    DatanodeInfo[] dns;
            +    try {
            +      dns = rpcServer.getDatanodeReport(DatanodeReportType.LIVE);
            +    } finally {
            +      RouterRpcServer.resetCurrentUser();
            +    }
            brahmareddy Brahma Reddy Battula added a comment -  RPC calls should  not  be invoked on behalf of a user as the login user. Always use the caller's context or it's a slippery slope to privilege escalation. Yes, As crh  mentioned, getDatanodeReport () will be called by router ( while choosing the datanodes in rotuerwebhdfs).So it's not exposed to user. UserGroupInformation.getCurrentUser()  is not a cheap call. If a cached ugi is available that is guaranteed to always be the current ugi, I'd recommend using it Agree.   We can have one thread local ugi which we can set and reset while choosing the datanode like below and we might not require doAs(..) as it will not used   --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java @@ -193,6 +193,9 @@     /** ClientProtocol calls. */     private   final  RouterClientProtocol clientProto; +   private   static   final  ThreadLocal<UserGroupInformation> curUser +      =  new  ThreadLocal<>(); +    /**     * Construct a router RPC server.     * @@ -1423,10 +1426,19 @@  private   boolean  isPathReadOnly( final   String  path) {     * @ throws  IOException If we cannot get the user information.     */     static  UserGroupInformation getRemoteUser()  throws  IOException { -    UserGroupInformation ugi = Server.getRemoteUser(); +    UserGroupInformation ugi = curUser.get(); +    ugi = (ugi !=  null ) ? ugi : Server.getRemoteUser();       return  (ugi !=  null ) ? ugi : UserGroupInformation.getCurrentUser();    } +   static  void setCurrentUser(UserGroupInformation ugi) { +    curUser.set(ugi); +  } + +   static  void resetCurrentUser() { +    curUser.set( null ); +  } +    /**     * Merge the outputs from multiple namespaces.     * diff --git a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java index a10764a8fe7..985ace1f273 100644 --- a/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java +++ b/hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java @@ -552,19 +552,13 @@  private  DatanodeInfo chooseDatanode( final  Router router,       // We need to get the DNs as a privileged user       final  RouterRpcServer rpcServer = getRPCServer(router);      UserGroupInformation loginUser = UserGroupInformation.getLoginUser(); - -    DatanodeInfo[] dns = loginUser.doAs( -         new  PrivilegedAction<DatanodeInfo[]>() { -          @Override -           public  DatanodeInfo[] run() { -             try  { -               return  rpcServer.getDatanodeReport(DatanodeReportType.LIVE); -            }  catch  (IOException e) { -              LOG.error( "Cannot get the datanodes from the RPC server" , e); -               return   null ; -            } -          } -        }); +    RouterRpcServer.setCurrentUser(loginUser); +    DatanodeInfo[] dns; +     try  { +      dns = rpcServer.getDatanodeReport(DatanodeReportType.LIVE); +    }  finally  { +      RouterRpcServer.resetCurrentUser(); +    }
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 36s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 20m 6s HDFS-13891 passed
            +1 compile 0m 33s HDFS-13891 passed
            +1 checkstyle 0m 25s HDFS-13891 passed
            +1 mvnsite 0m 34s HDFS-13891 passed
            +1 shadedclient 13m 10s branch has no errors when building and testing our client artifacts.
            +1 findbugs 0m 56s HDFS-13891 passed
            +1 javadoc 0m 36s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 28s the patch passed
            +1 compile 0m 25s the patch passed
            +1 javac 0m 25s the patch passed
            -0 checkstyle 0m 17s hadoop-hdfs-project/hadoop-hdfs-rbf: The patch generated 2 new + 16 unchanged - 0 fixed = 18 total (was 16)
            +1 mvnsite 0m 29s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 13m 6s patch has no errors when building and testing our client artifacts.
            +1 findbugs 1m 0s the patch passed
            +1 javadoc 0m 31s the patch passed
                  Other Tests
            +1 unit 24m 54s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 40s The patch does not generate ASF License warnings.
            79m 33s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12965532/HDFS-13972-HDFS-13891.012.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux 91a7193e852b 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / e508ab9
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/26611/artifact/out/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs-rbf.txt
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26611/testReport/
            Max. process+thread count 997 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26611/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 36s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 20m 6s HDFS-13891 passed +1 compile 0m 33s HDFS-13891 passed +1 checkstyle 0m 25s HDFS-13891 passed +1 mvnsite 0m 34s HDFS-13891 passed +1 shadedclient 13m 10s branch has no errors when building and testing our client artifacts. +1 findbugs 0m 56s HDFS-13891 passed +1 javadoc 0m 36s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 28s the patch passed +1 compile 0m 25s the patch passed +1 javac 0m 25s the patch passed -0 checkstyle 0m 17s hadoop-hdfs-project/hadoop-hdfs-rbf: The patch generated 2 new + 16 unchanged - 0 fixed = 18 total (was 16) +1 mvnsite 0m 29s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 13m 6s patch has no errors when building and testing our client artifacts. +1 findbugs 1m 0s the patch passed +1 javadoc 0m 31s the patch passed       Other Tests +1 unit 24m 54s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 40s The patch does not generate ASF License warnings. 79m 33s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12965532/HDFS-13972-HDFS-13891.012.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux 91a7193e852b 3.13.0-153-generic #203-Ubuntu SMP Thu Jun 14 08:52:28 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / e508ab9 maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 checkstyle https://builds.apache.org/job/PreCommit-HDFS-Build/26611/artifact/out/diff-checkstyle-hadoop-hdfs-project_hadoop-hdfs-rbf.txt Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26611/testReport/ Max. process+thread count 997 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26611/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.

            latest patch lgtm apart from checkstyle issues.crh could you address the checkstyle issues.?

            daryn can you check the latest patch if you get time.?

            brahmareddy Brahma Reddy Battula added a comment - latest patch lgtm apart from checkstyle issues. crh could you address the checkstyle issues.? daryn can you check the latest patch if you get time.?
            elgoiri Íñigo Goiri added a comment -

            I'm not sure if it's only me but the current approach looks a little hacky (I'm a little prejudiced against anything involving statics and ThreadLocals tbh).
            The approach here would be that by default we get the user in the Router using Server.getRemoteUser().
            Here we are adding a way to set for the current thread the user we want to use.
            So now, when we get the HTTP requests to create a file, we go through the internal chooseDatanode() in RouterWebHdfsMethods and then change the current user in the RouterRPCServer and then do the invokation.
            This explains why the old loginUser.doAs() didn't work but it looks to me a little hard to follow.

            What about creating an internal method in RouterRpcServer#getDatanodeReport which takes the UGI as a parameter and makes it go through all they way until RouterRpcClient?
            Setting this kind of things through the API is easier to follow than through static and ThreadLocals.
            I'm willing to be convinced otherwise but I want to bring this up.

            In an unrelated note, can we add some test coverage for this and check which users is running what?

            I also agree that we should have a follow up JIRA to do some caching of the DN reports and probably refactor this part.

            elgoiri Íñigo Goiri added a comment - I'm not sure if it's only me but the current approach looks a little hacky (I'm a little prejudiced against anything involving statics and ThreadLocals tbh). The approach here would be that by default we get the user in the Router using Server.getRemoteUser() . Here we are adding a way to set for the current thread the user we want to use. So now, when we get the HTTP requests to create a file, we go through the internal chooseDatanode() in RouterWebHdfsMethods and then change the current user in the RouterRPCServer and then do the invokation. This explains why the old loginUser.doAs() didn't work but it looks to me a little hard to follow. What about creating an internal method in RouterRpcServer#getDatanodeReport which takes the UGI as a parameter and makes it go through all they way until RouterRpcClient ? Setting this kind of things through the API is easier to follow than through static and ThreadLocals. I'm willing to be convinced otherwise but I want to bring this up. In an unrelated note, can we add some test coverage for this and check which users is running what? I also agree that we should have a follow up JIRA to do some caching of the DN reports and probably refactor this part.
            crh CR Hota added a comment -

            elgoiri Thanks for the feedback.

            Initially I was inclined with the approach you mentioned , but upon looking more threadlocal model seems to me be a better way to handle it than refactoring functions to allow ugi as an input param. It's more error prone to pass ugi through whole function stack. With threadlocal and getRemoteUser method, this change is restricted to a specific place and not allowing developers to think about what ugi to pass around. This is an edge case where ugi needs to be flipped, for remaining all cases until now, ugi remains that of end user.

            Yes, DN reports surely needs caching, specially also because its a fan out call to all namenodes and needs multiple optimizations. Will create a Jira for that.

            crh CR Hota added a comment - elgoiri  Thanks for the feedback. Initially I was inclined with the approach you mentioned , but upon looking more threadlocal model seems to me be a better way to handle it than refactoring functions to allow ugi as an input param. It's more error prone to pass ugi through whole function stack. With threadlocal and getRemoteUser method, this change is restricted to a specific place and not allowing developers to think about what ugi to pass around. This is an edge case where ugi needs to be flipped, for remaining all cases until now, ugi remains that of end user. Yes, DN reports surely needs caching, specially also because its a fan out call to all namenodes and needs multiple optimizations. Will create a Jira for that.
            elgoiri Íñigo Goiri added a comment -

            If nobody else has concerns about the static approach, I'm fine with it.
            Can we add the check to verify we are using the super user only for this case?
            The unit test should have some way to check the UGI of the calls.

            elgoiri Íñigo Goiri added a comment - If nobody else has concerns about the static approach, I'm fine with it. Can we add the check to verify we are using the super user only for this case? The unit test should have some way to check the UGI of the calls.
            crh CR Hota added a comment -

            elgoiri Thanks for the comment.

            In terms of testing just this feature, since its not an input/output param, the only way I tried locally is by refactoring and adding a simple check for method name. But to prevent erroneous usage of this by developers for other methods may be tricky. 

            Is there anything specific you have in mind?

             

              private void setSuperUser(String method) throws IOException {
                if ("getDatanodeReport".equals(method)) {
                  UserGroupInformation loginUser = UserGroupInformation.getLoginUser();
                  RouterRpcServer.setCurrentUser(loginUser);
                }
              }
            
              private DatanodeInfo chooseDatanode(final Router router,
                  final String path, final HttpOpParam.Op op, final long openOffset,
                  final String excludeDatanodes) throws IOException {
                // We need to get the DNs as a privileged user
                final RouterRpcServer rpcServer = getRPCServer(router);
                setSuperUser("getDatanodeReport");
                DatanodeInfo[] dns = null;
                try {
                  dns = rpcServer.getDatanodeReport(DatanodeReportType.LIVE);
                } catch (IOException e) {
                  LOG.error("Cannot get the datanodes from the RPC server", e);
                } finally {
                  // Reset ugi to remote user for remaining operations.
                  RouterRpcServer.resetCurrentUser();
                }
            
            

             

            crh CR Hota added a comment - elgoiri Thanks for the comment. In terms of testing just this feature, since its not an input/output param, the only way I tried locally is by refactoring and adding a simple check for method name. But to prevent erroneous usage of this by developers for other methods may be tricky.  Is there anything specific you have in mind?   private void setSuperUser( String method) throws IOException { if ( "getDatanodeReport" .equals(method)) { UserGroupInformation loginUser = UserGroupInformation.getLoginUser(); RouterRpcServer.setCurrentUser(loginUser); } } private DatanodeInfo chooseDatanode( final Router router, final String path, final HttpOpParam.Op op, final long openOffset, final String excludeDatanodes) throws IOException { // We need to get the DNs as a privileged user final RouterRpcServer rpcServer = getRPCServer(router); setSuperUser( "getDatanodeReport" ); DatanodeInfo[] dns = null ; try { dns = rpcServer.getDatanodeReport(DatanodeReportType.LIVE); } catch (IOException e) { LOG.error( "Cannot get the datanodes from the RPC server" , e); } finally { // Reset ugi to remote user for remaining operations. RouterRpcServer.resetCurrentUser(); }  
            hadoopqa Hadoop QA added a comment -
            +1 overall



            Vote Subsystem Runtime Comment
            0 reexec 0m 15s Docker mode activated.
                  Prechecks
            +1 @author 0m 0s The patch does not contain any @author tags.
            +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.
                  HDFS-13891 Compile Tests
            +1 mvninstall 24m 39s HDFS-13891 passed
            +1 compile 0m 29s HDFS-13891 passed
            +1 checkstyle 0m 22s HDFS-13891 passed
            +1 mvnsite 0m 33s HDFS-13891 passed
            +1 shadedclient 12m 13s branch has no errors when building and testing our client artifacts.
            +1 findbugs 1m 5s HDFS-13891 passed
            +1 javadoc 0m 37s HDFS-13891 passed
                  Patch Compile Tests
            +1 mvninstall 0m 30s the patch passed
            +1 compile 0m 28s the patch passed
            +1 javac 0m 28s the patch passed
            +1 checkstyle 0m 17s the patch passed
            +1 mvnsite 0m 33s the patch passed
            +1 whitespace 0m 0s The patch has no whitespace issues.
            +1 shadedclient 12m 52s patch has no errors when building and testing our client artifacts.
            +1 findbugs 1m 8s the patch passed
            +1 javadoc 0m 38s the patch passed
                  Other Tests
            +1 unit 23m 45s hadoop-hdfs-rbf in the patch passed.
            +1 asflicense 0m 27s The patch does not generate ASF License warnings.
            81m 44s



            Subsystem Report/Notes
            Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f
            JIRA Issue HDFS-13972
            JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12966155/HDFS-13972-HDFS-13891.013.patch
            Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle
            uname Linux a1ac432098f1 4.4.0-143-generic #169~14.04.2-Ubuntu SMP Wed Feb 13 15:00:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
            Build tool maven
            Personality /testptch/patchprocess/precommit/personality/provided.sh
            git revision HDFS-13891 / bd3161e
            maven version: Apache Maven 3.3.9
            Default Java 1.8.0_191
            findbugs v3.1.0-RC1
            Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26652/testReport/
            Max. process+thread count 998 (vs. ulimit of 10000)
            modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf
            Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26652/console
            Powered by Apache Yetus 0.8.0 http://yetus.apache.org

            This message was automatically generated.

            hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 reexec 0m 15s Docker mode activated.       Prechecks +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 3 new or modified test files.       HDFS-13891 Compile Tests +1 mvninstall 24m 39s HDFS-13891 passed +1 compile 0m 29s HDFS-13891 passed +1 checkstyle 0m 22s HDFS-13891 passed +1 mvnsite 0m 33s HDFS-13891 passed +1 shadedclient 12m 13s branch has no errors when building and testing our client artifacts. +1 findbugs 1m 5s HDFS-13891 passed +1 javadoc 0m 37s HDFS-13891 passed       Patch Compile Tests +1 mvninstall 0m 30s the patch passed +1 compile 0m 28s the patch passed +1 javac 0m 28s the patch passed +1 checkstyle 0m 17s the patch passed +1 mvnsite 0m 33s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 shadedclient 12m 52s patch has no errors when building and testing our client artifacts. +1 findbugs 1m 8s the patch passed +1 javadoc 0m 38s the patch passed       Other Tests +1 unit 23m 45s hadoop-hdfs-rbf in the patch passed. +1 asflicense 0m 27s The patch does not generate ASF License warnings. 81m 44s Subsystem Report/Notes Docker Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:8f97d6f JIRA Issue HDFS-13972 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12966155/HDFS-13972-HDFS-13891.013.patch Optional Tests dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient findbugs checkstyle uname Linux a1ac432098f1 4.4.0-143-generic #169~14.04.2-Ubuntu SMP Wed Feb 13 15:00:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/patchprocess/precommit/personality/provided.sh git revision HDFS-13891 / bd3161e maven version: Apache Maven 3.3.9 Default Java 1.8.0_191 findbugs v3.1.0-RC1 Test Results https://builds.apache.org/job/PreCommit-HDFS-Build/26652/testReport/ Max. process+thread count 998 (vs. ulimit of 10000) modules C: hadoop-hdfs-project/hadoop-hdfs-rbf U: hadoop-hdfs-project/hadoop-hdfs-rbf Console output https://builds.apache.org/job/PreCommit-HDFS-Build/26652/console Powered by Apache Yetus 0.8.0 http://yetus.apache.org This message was automatically generated.
            elgoiri Íñigo Goiri added a comment -

            I was thinking if there was some counter we could check to see what user executed commands.
            However, the RPC side doesn't have it available.
            Not sure how to check for this easily...

            How easy to make the stub for the getDatanodeReport() as a cache?
            No need to implement the full caching but just checking for it.
            In the follow-up we actually track that.
            Actually, isn't the RouterRpcServer already caching it?
            Can we rely on it?

            elgoiri Íñigo Goiri added a comment - I was thinking if there was some counter we could check to see what user executed commands. However, the RPC side doesn't have it available. Not sure how to check for this easily... How easy to make the stub for the getDatanodeReport() as a cache? No need to implement the full caching but just checking for it. In the follow-up we actually track that. Actually, isn't the RouterRpcServer already caching it? Can we rely on it?
            crh CR Hota added a comment -

            elgoiri Thanks for the suggestions.

            Haven't been successful in getting anything meaningful wired for this case. Caching/Setting a value to be retrieved post the function call may not help. To keep intact goal of this test is to make sure that only this method uses super user, unless an explicit check is added that "getDatanodeReport" is the method and through webhdfs only there won't be much benefit from unit test perspective.

            crh CR Hota added a comment - elgoiri Thanks for the suggestions. Haven't been successful in getting anything meaningful wired for this case. Caching/Setting a value to be retrieved post the function call may not help. To keep intact goal of this test is to make sure that only this method uses super user, unless an explicit check is added that "getDatanodeReport" is the method and through webhdfs only there won't be much benefit from unit test perspective.
            elgoiri Íñigo Goiri added a comment -

            I am fine with HDFS-13972-HDFS-13891.013.patch.
            I'd like to get +1s from others.

            elgoiri Íñigo Goiri added a comment - I am fine with HDFS-13972-HDFS-13891.013.patch . I'd like to get +1s from others.
            crh CR Hota added a comment -

            Thanks elgoiri

             

            brahmareddy  surendrasingh  daryn 

            Please help take a look at HDFS-13972-HDFS-13891.013.patch 

             

            crh CR Hota added a comment - Thanks elgoiri   brahmareddy    surendrasingh   daryn   Please help take a look at  HDFS-13972-HDFS-13891.013.patch    

            +1 on HDFS-13972-HDFS-13891.013.patch  .

            Will hold for commit for couple of days.

             

            brahmareddy Brahma Reddy Battula added a comment - +1 on  HDFS-13972-HDFS-13891.013.patch   . Will hold for commit for couple of days.  
            brahmareddy Brahma Reddy Battula added a comment - - edited

            As there are no objections and no further comments, going to commit now.

            brahmareddy Brahma Reddy Battula added a comment - - edited As there are no objections and no further comments, going to commit now.

            Committed to HDFS-13891. crh thanks for your contribution and thanks all for useful discussions and review.

            brahmareddy Brahma Reddy Battula added a comment - Committed to HDFS-13891 . crh thanks for your contribution and thanks all for useful discussions and review.
            hudson Hudson added a comment -

            FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #16813 (See https://builds.apache.org/job/Hadoop-trunk-Commit/16813/)
            HDFS-13972. RBF: Support for Delegation Token (WebHDFS). Contributed by (brahma: rev 021a43b1a4bbc8a68c31461e206214a5eadc38dd)

            • (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/security/RouterSecurityManager.java
            • (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java
            • (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/Router.java
            • (add) hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterHttpDelegationToken.java
            • (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java
            • (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/fs/contract/router/TestRouterHDFSContractDelegationToken.java
            • (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java
            hudson Hudson added a comment - FAILURE: Integrated in Jenkins build Hadoop-trunk-Commit #16813 (See https://builds.apache.org/job/Hadoop-trunk-Commit/16813/ ) HDFS-13972 . RBF: Support for Delegation Token (WebHDFS). Contributed by (brahma: rev 021a43b1a4bbc8a68c31461e206214a5eadc38dd) (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/security/RouterSecurityManager.java (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterSecurityManager.java (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/Router.java (add) hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/hdfs/server/federation/security/TestRouterHttpDelegationToken.java (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/test/java/org/apache/hadoop/fs/contract/router/TestRouterHDFSContractDelegationToken.java (edit) hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterWebHdfsMethods.java

            People

              crh CR Hota
              elgoiri Íñigo Goiri
              Votes:
              0 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: