[ 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: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org