[
https://issues.apache.org/jira/browse/YARN-9920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16963715#comment-16963715
]
Peter Bacsko edited comment on YARN-9920 at 10/31/19 7:08 AM:
--------------------------------------------------------------
Thanks for patch [~prabhujoseph], looks pretty good. I have only minor comments.
1. Very nit: Docs should say "*Returns* the caller's remote ip address"
{noformat}
/**
* The caller's remote ip address.
* @return the caller's remote ip address.
*/
String getRemoteAddress();
{noformat}
2. {{Queue.java}} is an interface. The javadoc is completely missing for
{{hasAccess()}}. If you're at it, could you please add Javadoc for this method?
3. Similar here, Javadoc is incomplete for {{checkAccess()}}, pls add some
description to the new as well as existing fields:
{noformat}
@@ -190,11 +190,14 @@ ApplicationResourceUsageReport getAppResourceUsageReport(
* @param callerUGI
* @param acl
* @param queueName
+ * @param remoteAddress
+ * @param forwardedAddresses
* @return <code>true</code> if the user has the permission,
* <code>false</code> otherwise
*/
boolean checkAccess(UserGroupInformation callerUGI,
- QueueACL acl, String queueName);
+ QueueACL acl, String queueName, String remoteAddress,
+ List<String> forwardedAddresses);
{noformat}
4. Ditto for {{CSQueue.java (acl, user)}}
{noformat}
* Check if the <code>user</code> has permission to perform the operation
* @param acl ACL
* @param user user
+ * @param remoteAddress caller's remote ip address.
+ * @param forwardedAddresses forwarded addresses in case of http request
* @return <code>true</code> if the user has the permission,
* <code>false</code> otherwise
*/
- public boolean hasAccess(QueueACL acl, UserGroupInformation user);
+ public boolean hasAccess(QueueACL acl, UserGroupInformation user,
+ String remoteAddress, List<String> forwardedAddresses);
{noformat}
5. I can see you added a couple of new tests. But, is this code path covered
properly?
{noformat}
List<String> forwardedAddresses = null;
String forwardedFor = hsr.getHeader(RMWSConsts.FORWARDED_FOR);
if (forwardedFor != null) {
forwardedAddresses = Arrays.asList(forwardedFor.split(","));
}
{noformat}
was (Author: pbacsko):
Thanks for patch [~prabhujoseph], looks pretty good. I have only minor comments.
1. Very nit: Docs should say "*Returns* the caller's remote ip address"
{noformat}
/**
* The caller's remote ip address.
* @return the caller's remote ip address.
*/
String getRemoteAddress();
{noformat}
2. {{Queue.java}} is an interface. The javadoc is completely missing for
{{hasAccess()}}. If you're at it, could you please add Javadoc for this method?
3. Similar here, Javadoc is incomplete for {{checkAccess()}, pls add some
description to the new as well as existing fields:
{noformat}
@@ -190,11 +190,14 @@ ApplicationResourceUsageReport getAppResourceUsageReport(
* @param callerUGI
* @param acl
* @param queueName
+ * @param remoteAddress
+ * @param forwardedAddresses
* @return <code>true</code> if the user has the permission,
* <code>false</code> otherwise
*/
boolean checkAccess(UserGroupInformation callerUGI,
- QueueACL acl, String queueName);
+ QueueACL acl, String queueName, String remoteAddress,
+ List<String> forwardedAddresses);
{noformat}
4. Ditto for {{CSQueue.java}}
{noformat}
* Check if the <code>user</code> has permission to perform the operation
* @param acl ACL
* @param user user
+ * @param remoteAddress caller's remote ip address.
+ * @param forwardedAddresses forwarded addresses in case of http request
* @return <code>true</code> if the user has the permission,
* <code>false</code> otherwise
*/
- public boolean hasAccess(QueueACL acl, UserGroupInformation user);
+ public boolean hasAccess(QueueACL acl, UserGroupInformation user,
+ String remoteAddress, List<String> forwardedAddresses);
{noformat}
5. I can see you added a couple of new tests. But, is this code path covered
properly?
{noformat}
List<String> forwardedAddresses = null;
String forwardedFor = hsr.getHeader(RMWSConsts.FORWARDED_FOR);
if (forwardedFor != null) {
forwardedAddresses = Arrays.asList(forwardedFor.split(","));
}
{noformat}
> YarnAuthorizationProvider AccessRequest gets Null RemoteAddress from
> FairScheduler
> ----------------------------------------------------------------------------------
>
> Key: YARN-9920
> URL: https://issues.apache.org/jira/browse/YARN-9920
> Project: Hadoop YARN
> Issue Type: Bug
> Components: fairscheduler, security
> Affects Versions: 3.3.0
> Reporter: Prabhu Joseph
> Assignee: Prabhu Joseph
> Priority: Major
> Attachments: YARN-9920-001.patch, YARN-9920-002.patch,
> YARN-9920-003.patch
>
>
> YarnAuthorizationProvider AccessRequest has null RemoteAddress in case of
> FairScheduler. FSQueue#hasAccess uses Server.getRemoteAddress() which will be
> null when the call is from RMWebServices and EventDispatcher. It works fine
> when called by IPC Server Handler.
> FSQueue#hasAccess is called at three places where (2) and (3) returns null.
> *1. IPC Server -> RMAppManager#createAndPopulateNewRMApp -> FSQueue#hasAccess
> -> Server.getRemoteAddress returns correct Remote IP.*
>
> *2. IPC Server -> RMAppManager#createAndPopulateNewRMApp ->
> AppAddedSchedulerEvent*
> *EventDispatcher -> FairScheduler#addApplication -> FSQueue.hasAccess ->
> Server.getRemoteAddress returns null*
>
> {code:java}
> org.apache.hadoop.yarn.security.ConfiguredYarnAuthorizer.checkPermission(ConfiguredYarnAuthorizer.java:101)
> at
> org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FSQueue.hasAccess(FSQueue.java:316)
> at
> org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.addApplication(FairScheduler.java:509)
> at
> org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.handle(FairScheduler.java:1268)
> at
> org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.handle(FairScheduler.java:133)
> at
> org.apache.hadoop.yarn.event.EventDispatcher$EventProcessor.run(EventDispatcher.java:66)
> {code}
>
> *3. RMWebServices -> QueueACLsManager#checkAccess -> FSQueue.hasAccess ->
> Server.getRemoteAddress returns null.*
> {code:java}
> org.apache.hadoop.yarn.security.ConfiguredYarnAuthorizer.checkPermission(ConfiguredYarnAuthorizer.java:101)
> at
> org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FSQueue.hasAccess(FSQueue.java:316)
> at
> org.apache.hadoop.yarn.server.resourcemanager.scheduler.fair.FairScheduler.checkAccess(FairScheduler.java:1610)
> at
> org.apache.hadoop.yarn.server.resourcemanager.security.QueueACLsManager.checkAccess(QueueACLsManager.java:84)
> at
> org.apache.hadoop.yarn.server.resourcemanager.webapp.RMWebServices.hasAccess(RMWebServices.java:270)
> at
> org.apache.hadoop.yarn.server.resourcemanager.webapp.RMWebServices.getApps(RMWebServices.java:553)
> {code}
>
> Have verified with CapacityScheduler and it works fine.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
