Details
-
Sub-task
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
None
-
None
-
None
-
Reviewed
Description
HDFS Router should support issuing HDFS delegation tokens through WebHDFS.
Attachments
Attachments
- HDFS-13972-HDFS-13891.001.patch
- 41 kB
- CR Hota
- HDFS-13972-HDFS-13891.002.patch
- 10 kB
- CR Hota
- HDFS-13972-HDFS-13891.003.patch
- 8 kB
- CR Hota
- HDFS-13972-HDFS-13891.004.patch
- 13 kB
- CR Hota
- TestRouterWebHDFSContractTokens.java
- 2 kB
- CR Hota
- HDFS-13972-HDFS-13891.005.patch
- 21 kB
- CR Hota
- HDFS-13972-HDFS-13891.006.patch
- 27 kB
- Íñigo Goiri
- HDFS-13972-HDFS-13891.007.patch
- 20 kB
- CR Hota
- HDFS-13972-HDFS-13891.008.patch
- 20 kB
- CR Hota
- HDFS-13972-HDFS-13891.009.patch
- 22 kB
- CR Hota
- HDFS-13972-HDFS-13891.010.patch
- 27 kB
- CR Hota
- HDFS-13972-HDFS-13891.011.patch
- 28 kB
- CR Hota
- HDFS-13972-HDFS-13891.012.patch
- 30 kB
- CR Hota
- HDFS-13972-HDFS-13891.013.patch
- 30 kB
- CR Hota
Issue Links
- is blocked by
-
HDFS-14052 RBF: Use Router keytab for WebHDFS
- Resolved
-
HDFS-14006 Refactor name node to allow different token verification implementations
- Resolved
-
HDFS-14070 Refactor NameNodeWebHdfsMethods to allow better extensibility
- Resolved
-
HDFS-14302 Refactor NameNodeWebHdfsMethods#generateDelegationToken() to allow better extensibility
- Resolved
- is related to
-
HDFS-12512 RBF: Add WebHDFS
- Resolved
-
HDFS-14006 Refactor name node to allow different token verification implementations
- Resolved
-
HDFS-12945 Switch to ClientProtocol instead of NamenodeProtocols in NamenodeWebHdfsMethods
- Resolved
- relates to
-
HDFS-14052 RBF: Use Router keytab for WebHDFS
- Resolved
Activity
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.
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.
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-13891branch 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.
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.
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?
-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. |
|
|||
+1 | mvninstall | 19m 53s | |
+1 | compile | 0m 30s | |
+1 | checkstyle | 0m 23s | |
+1 | mvnsite | 0m 33s | |
+1 | shadedclient | 12m 12s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 52s | |
+1 | javadoc | 0m 34s | |
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 | |
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 | |
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
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
elgoiri brahmareddy Please help rebase rbf branch HDFS-13891 with trunk. Need the namenode refactoring changes to re-work on couple of things here.
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.
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 ..
Can we rebase after HDFS-13358?
This should get pretty contained right now.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
-1 | patch | 0m 7s | |
Subsystem | Report/Notes |
---|---|
JIRA Issue | |
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.
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.
-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. |
|
|||
0 | mvndep | 0m 18s | Maven dependency ordering for branch |
+1 | mvninstall | 19m 1s | |
+1 | compile | 2m 57s | |
+1 | checkstyle | 0m 59s | |
+1 | mvnsite | 1m 26s | |
+1 | shadedclient | 12m 51s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 2m 42s | |
+1 | javadoc | 1m 11s | |
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 | |
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 | |
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.
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 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.
Thanks for reminder on HDFS-14006, that makes sense.
Rebased HDFS-13891.
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?
-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. |
|
|||
+1 | mvninstall | 23m 37s | |
+1 | compile | 0m 28s | |
+1 | checkstyle | 0m 21s | |
+1 | mvnsite | 0m 33s | |
+1 | shadedclient | 12m 0s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 57s | |
+1 | javadoc | 0m 35s | |
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 | |
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 | |
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.
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.
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: -/-]
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"} ]}}
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.
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.
+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. |
|
|||
+1 | mvninstall | 18m 49s | |
+1 | compile | 0m 31s | |
+1 | checkstyle | 0m 23s | |
+1 | mvnsite | 0m 37s | |
+1 | shadedclient | 12m 55s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 57s | |
+1 | javadoc | 0m 35s | |
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 | |
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 | |
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.
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.
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 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.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
-1 | patch | 0m 8s | |
Subsystem | Report/Notes |
---|---|
JIRA Issue | |
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.
+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. |
|
|||
+1 | mvninstall | 15m 58s | |
+1 | compile | 0m 32s | |
+1 | checkstyle | 0m 24s | |
+1 | mvnsite | 0m 33s | |
+1 | shadedclient | 11m 58s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 51s | |
+1 | javadoc | 0m 36s | |
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 | |
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 | |
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.
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?
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.
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.
-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. |
|
|||
+1 | mvninstall | 18m 10s | |
+1 | compile | 0m 32s | |
+1 | checkstyle | 0m 22s | |
+1 | mvnsite | 0m 34s | |
+1 | shadedclient | 12m 20s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 55s | |
+1 | javadoc | 0m 38s | |
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 | |
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 | |
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.
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.
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?
+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. |
|
|||
+1 | mvninstall | 16m 47s | |
+1 | compile | 0m 26s | |
+1 | checkstyle | 0m 21s | |
+1 | mvnsite | 0m 32s | |
+1 | shadedclient | 11m 42s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 54s | |
+1 | javadoc | 0m 30s | |
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 | |
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 | |
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.
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?
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?
+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. |
|
|||
+1 | mvninstall | 17m 17s | |
+1 | compile | 0m 30s | |
+1 | checkstyle | 0m 21s | |
+1 | mvnsite | 0m 32s | |
+1 | shadedclient | 12m 12s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 50s | |
+1 | javadoc | 0m 35s | |
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 | |
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 | |
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.
+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. |
|
|||
+1 | mvninstall | 21m 57s | |
+1 | compile | 0m 32s | |
+1 | checkstyle | 0m 22s | |
+1 | mvnsite | 0m 35s | |
+1 | shadedclient | 13m 0s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 1m 0s | |
+1 | javadoc | 0m 37s | |
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 | |
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 | |
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.
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.
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 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?
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.
Thanks surendrasingh for bringing this up.
I think we should do it here; this is the basis of DT in WebHDFS.
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.
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.
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.
+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. |
|
|||
+1 | mvninstall | 17m 46s | |
+1 | compile | 0m 29s | |
+1 | checkstyle | 0m 22s | |
+1 | mvnsite | 0m 32s | |
+1 | shadedclient | 12m 33s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 58s | |
+1 | javadoc | 0m 35s | |
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 | |
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 | |
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.
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).
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 ?
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 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.
+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. |
|
|||
+1 | mvninstall | 17m 5s | |
+1 | compile | 0m 28s | |
+1 | checkstyle | 0m 18s | |
+1 | mvnsite | 0m 31s | |
+1 | shadedclient | 11m 6s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 52s | |
+1 | javadoc | 0m 35s | |
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 | |
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 | |
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.
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 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?
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 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.
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.
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(); + }
+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. |
|
|||
+1 | mvninstall | 20m 6s | |
+1 | compile | 0m 33s | |
+1 | checkstyle | 0m 25s | |
+1 | mvnsite | 0m 34s | |
+1 | shadedclient | 13m 10s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 0m 56s | |
+1 | javadoc | 0m 36s | |
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 | |
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 | |
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.?
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 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.
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 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(); }
+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. |
|
|||
+1 | mvninstall | 24m 39s | |
+1 | compile | 0m 29s | |
+1 | checkstyle | 0m 22s | |
+1 | mvnsite | 0m 33s | |
+1 | shadedclient | 12m 13s | branch has no errors when building and testing our client artifacts. |
+1 | findbugs | 1m 5s | |
+1 | javadoc | 0m 37s | |
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 | |
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 | |
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.
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 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.
I am fine with HDFS-13972-HDFS-13891.013.patch.
I'd like to get +1s from others.
Thanks elgoiri
brahmareddy surendrasingh daryn
Please help take a look at HDFS-13972-HDFS-13891.013.patch
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.
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
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