9uapaw commented on a change in pull request #3938:
URL: https://github.com/apache/hadoop/pull/3938#discussion_r807915774
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java
##########
@@ -635,6 +638,42 @@ public AbstractLeafQueue createQueue(QueuePath queue)
return parentsToCreate;
}
+ public List<Permission> getPermissionsForDynamicQueue(
+ QueuePath queuePath,
+ CapacitySchedulerConfiguration csConf) throws
SchedulerDynamicEditException {
+
+ List<Permission> permissions = new ArrayList<>();
+ PrivilegedEntity privilegedEntity =
getPrivilegedEntity(queuePath.getFullPath());
+
+ CSQueue parentQueue = getQueueByFullName(queuePath.getParent());
+ if (parentQueue == null) {
+ for (String missingParent : determineMissingParents(queuePath)) {
+ String parentOfMissingParent = new
QueuePath(missingParent).getParent();
+ permissions.add(new Permission(getPrivilegedEntity(missingParent),
+ getACLsForFlexibleAutoCreatedParentQueue(
+ new AutoCreatedQueueTemplate(csConf,
+ new QueuePath(parentOfMissingParent)))));
+ }
+ }
+
+ if (parentQueue instanceof AbstractManagedParentQueue) {
+ // An AbstractManagedParentQueue must have been found for Legacy AQC
+ permissions.add(new Permission(privilegedEntity,
+ csConf.getACLsForLegacyAutoCreatedLeafQueue(queuePath.getParent())));
+ } else {
+ // Every other case must be a Flexible Leaf Queue
+ permissions.add(new Permission(privilegedEntity,
+ getACLsForFlexibleAutoCreatedLeafQueue(
+ new AutoCreatedQueueTemplate(csConf, new
QueuePath(queuePath.getParent())))));
+ }
+
+ return permissions;
+ }
+
+ public static PrivilegedEntity getPrivilegedEntity(String queuePath) {
Review comment:
This should be a static factory method on PrivilegedEntity itself.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
##########
@@ -566,6 +578,29 @@ private RMAppImpl createAndPopulateNewRMApp(
return application;
}
+ private boolean checkSubmitACLPermission(AccessRequest accessRequest,
+ YarnAuthorizationProvider
dynamicAuthorizer) {
+ return authorizer.checkPermission(accessRequest) ||
+ (dynamicAuthorizer != null &&
dynamicAuthorizer.checkPermission(accessRequest));
+ }
+
+ private boolean checkAdminACLPermission(AccessRequest accessRequest,
+ YarnAuthorizationProvider
dynamicAuthorizer) {
+ return authorizer.checkPermission(accessRequest) ||
+ (dynamicAuthorizer != null &&
dynamicAuthorizer.checkPermission(accessRequest));
+ }
+
+ private AccessRequest getAccessRequest(PrivilegedEntity privilegedEntity,
Review comment:
This is misleading a bit. Its actually a factory method, therefore
either make it static and name it createAccessRequest or create a constructor
on the class itself.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
##########
@@ -467,28 +474,33 @@ private RMAppImpl createAndPopulateNewRMApp(
if (scheduler instanceof CapacityScheduler) {
String queueName = placementContext == null ?
submissionContext.getQueue() : placementContext.getFullQueuePath();
-
- String appName = submissionContext.getApplicationName();
- CSQueue csqueue = ((CapacityScheduler) scheduler).getQueue(queueName);
-
- if (csqueue == null && placementContext != null) {
- //could be an auto created queue through queue mapping. Validate
- // parent queue exists and has valid acls
- String parentQueueName = placementContext.getParentQueue();
- csqueue = ((CapacityScheduler) scheduler).getQueue(parentQueueName);
+ CapacityScheduler cs = (CapacityScheduler) scheduler;
+ CSQueue csqueue = cs.getQueue(queueName);
+ PrivilegedEntity privilegedEntity = getPrivilegedEntity(
+ csqueue == null ? queueName : csqueue.getQueuePath());
+
+ YarnAuthorizationProvider dynamicAuthorizer = null;
+ boolean ambiguous = false;
+ if (csqueue == null) {
+ try {
+ List<Permission> permissions =
+
cs.getCapacitySchedulerQueueManager().getPermissionsForDynamicQueue(
+ new QueuePath(queueName), cs.getConfiguration());
+ if (!permissions.isEmpty()) {
+ dynamicAuthorizer = new ConfiguredYarnAuthorizer();
+ dynamicAuthorizer.setPermission(permissions, userUgi);
+ }
+ } catch (SchedulerDynamicEditException e) {
+ ambiguous = true;
+ }
}
- if (csqueue != null
- && !authorizer.checkPermission(
- new AccessRequest(csqueue.getPrivilegedEntity(), userUgi,
- SchedulerUtils.toAccessType(QueueACL.SUBMIT_APPLICATIONS),
- applicationId.toString(), appName, Server.getRemoteAddress(),
- null))
- && !authorizer.checkPermission(
- new AccessRequest(csqueue.getPrivilegedEntity(), userUgi,
- SchedulerUtils.toAccessType(QueueACL.ADMINISTER_QUEUE),
- applicationId.toString(), appName, Server.getRemoteAddress(),
- null))) {
+ String appName = submissionContext.getApplicationName();
+ if (!ambiguous && !checkSubmitACLPermission(
+ getAccessRequest(privilegedEntity, userUgi, applicationId,
+ appName, QueueACL.SUBMIT_APPLICATIONS), dynamicAuthorizer)
+ && !checkAdminACLPermission(getAccessRequest(privilegedEntity,
userUgi, applicationId,
+ appName, QueueACL.ADMINISTER_QUEUE), dynamicAuthorizer)) {
Review comment:
This expression is a bit hard to read due to multiple long method calls.
My suggestion is to introduce some local variable.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java
##########
@@ -635,6 +638,42 @@ public AbstractLeafQueue createQueue(QueuePath queue)
return parentsToCreate;
}
+ public List<Permission> getPermissionsForDynamicQueue(
+ QueuePath queuePath,
+ CapacitySchedulerConfiguration csConf) throws
SchedulerDynamicEditException {
+
+ List<Permission> permissions = new ArrayList<>();
+ PrivilegedEntity privilegedEntity =
getPrivilegedEntity(queuePath.getFullPath());
+
+ CSQueue parentQueue = getQueueByFullName(queuePath.getParent());
+ if (parentQueue == null) {
Review comment:
I am not sure about this, but probably the null check is redundant here.
determineMissingParents should return with an empty list, if the parentQueue is
not null, therefore the iteration will not be executed.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
##########
@@ -467,28 +474,33 @@ private RMAppImpl createAndPopulateNewRMApp(
if (scheduler instanceof CapacityScheduler) {
String queueName = placementContext == null ?
submissionContext.getQueue() : placementContext.getFullQueuePath();
-
- String appName = submissionContext.getApplicationName();
- CSQueue csqueue = ((CapacityScheduler) scheduler).getQueue(queueName);
-
- if (csqueue == null && placementContext != null) {
- //could be an auto created queue through queue mapping. Validate
- // parent queue exists and has valid acls
- String parentQueueName = placementContext.getParentQueue();
- csqueue = ((CapacityScheduler) scheduler).getQueue(parentQueueName);
+ CapacityScheduler cs = (CapacityScheduler) scheduler;
+ CSQueue csqueue = cs.getQueue(queueName);
+ PrivilegedEntity privilegedEntity = getPrivilegedEntity(
+ csqueue == null ? queueName : csqueue.getQueuePath());
+
+ YarnAuthorizationProvider dynamicAuthorizer = null;
+ boolean ambiguous = false;
+ if (csqueue == null) {
+ try {
+ List<Permission> permissions =
+
cs.getCapacitySchedulerQueueManager().getPermissionsForDynamicQueue(
+ new QueuePath(queueName), cs.getConfiguration());
+ if (!permissions.isEmpty()) {
+ dynamicAuthorizer = new ConfiguredYarnAuthorizer();
+ dynamicAuthorizer.setPermission(permissions, userUgi);
+ }
+ } catch (SchedulerDynamicEditException e) {
+ ambiguous = true;
Review comment:
getPermissionForDynamicQueue could produce a few exceptions other than
ambiguous queue. It is also silently swallowing the exception. I suggest a
renaming and a logging mechanism here.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/RMAppManager.java
##########
@@ -566,6 +578,29 @@ private RMAppImpl createAndPopulateNewRMApp(
return application;
}
+ private boolean checkSubmitACLPermission(AccessRequest accessRequest,
+ YarnAuthorizationProvider
dynamicAuthorizer) {
+ return authorizer.checkPermission(accessRequest) ||
+ (dynamicAuthorizer != null &&
dynamicAuthorizer.checkPermission(accessRequest));
+ }
+
+ private boolean checkAdminACLPermission(AccessRequest accessRequest,
+ YarnAuthorizationProvider
dynamicAuthorizer) {
+ return authorizer.checkPermission(accessRequest) ||
+ (dynamicAuthorizer != null &&
dynamicAuthorizer.checkPermission(accessRequest));
+ }
Review comment:
These are identical methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
