tomicooler commented on a change in pull request #3938:
URL: https://github.com/apache/hadoop/pull/3938#discussion_r808892061
##########
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:
```
public class PrivilegedEntity {
public enum EntityType {
QUEUE
}
```
A normal constructor with a default QUEUE type would do.
##########
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:
Yeah, good point.
##########
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:
Changed a little bit, I didn't introduce local variables however.
##########
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:
I can rename it and make static, but I would rather not put it in the
AccessRequest class, I don't want to make AccessRequest dependant on the
Server.getRemoteAddress(). (Most of the cases that's the default parameter
but..)
```
new AccessRequest(privilegedEntity, userUgi,
SchedulerUtils.toAccessType(submitApplications),
applicationId.toString(), appName, Server.getRemoteAddress(),
null);
```
##########
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:
Unfortunately determineMissingParents was not really designed for
reusability in mind, and I didn't want to refactor it, but, I tried to
introduce different types of exceptions for different errors, e.g:
SchedulerDynamicMissingParentException,
SchedulerDynamicAmbiguousParentException,
SchedulerDynamicAboveMaxDepthLimitException etc.
This whole ambiguous thing is a bit misleading, two test cases:
1. TestCapacitySchedulerAmbiguousLeafs.testAmbiguousSubmissionWithACL: the
queue is ambiguous not the parent, so the determineMissingParents would throw
SchedulerDynamicMissingParentException, for ambiguity the queue must be checked
beforehand. The app submission will fail later if we don't check queue acls
here.
2. TestApplicationACLs.verifyInvalidQueueWithAcl: the queue is invalid, and
no acl checks should be done on it. The app submission will fail later.
The conclusion is that we must ONLY check dynamic queue ACLs when we can
construct the dynamic missing parents. The error logging is not relevant, the
app submission will fail anyway (hopefully in every case, I didn't touch that
logic with this PR..., to be honest it looks fragile that in some "error" like
cases we don't check queue ACLs).
Removed the ambiguous variable, and now the code is much cleaner:
```
YarnAuthorizationProvider dynamicAuthorizer = null;
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 ignored) {
}
}
if (csqueue != null || dynamicAuthorizer != null) {
// here we have to check the queue ACLs
```
##########
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:
```
public List<String> determineMissingParents(
QueuePath queue) throws SchedulerDynamicEditException {
if (!queue.hasParent()) {
```
There is no null check, so it is not redundant.
--
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]
