[
https://issues.apache.org/jira/browse/MAPREDUCE-1542?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12844608#action_12844608
]
Hemanth Yamijala commented on MAPREDUCE-1542:
---------------------------------------------
I looked at this patch, and have a few comments:
- HttpServer: In the error message, is 'clusterOwner' a familiar term ? Maybe
'User who started the cluster' while more verbose conveys the meaning better.
- JobTracker: JobConf.MR_SUPERGROUP reads mapred.permissions.supergroup. Error
message here says mapreduce.cluster.permissions.supergroup.
- JobTracker.checkAccess is saying that mrOwner and admins can do any operation
on the job; however the code is checking queue ACLs first. Hence if the UGI
does not have access to do the appropriate operation for the queue, this
documentation will be incorrect.
- JobACLsManager.checkAccess has a typo in javadoc: "The mrOwner and admins
always" -> 'are' is missing.
- One reference to mapred.permissions.supergroup in description of
mapreduce.job.acl-modify-job in mapred-default.xml
- isMROwnerOrAdmin seems out of place in JobTracker. Note that it takes the
owner and adminsACL as parameters, and hence can live in any class. Had this
not been the case, then the API is fine. I see JobACLsManager as the class
responsible for all ACLs checks. So, why can't the mrowner and acls be created
inside this class when it is instantiated ? Then, isMROwnerOrAdmin can be
implemented directly in JobACLsManager itself. This will also avoid the static
call dependencies between TaskTracker classes and JobTracker which also seem a
little odd.
- I think it makes sense to add checks for queue ACLs refresh, service refresh
and user-to-group mapping refresh also similar to node refresh. I don't see a
semantic difference between these operations and node refresh and they should
be implemented in the same way, I think. But I am OK if these are taken up in a
following JIRA, as their addition may not be in the scope of this JIRA.
- Regarding tests, I was expecting to see tests that set up users and groups in
the hadoop.cluster.administrators ACL and then checks that various operations
of view and modify succeed with combinations of both allowed and disallowed
users. Exhaustive tests need not be end-to-end I think - i.e. they need not run
real jobs. Since most of the code goes through JobInProgress.checkAccess, can
we just create fake objects and test the checkAccess method ? And maybe have
some end-to-end tests for very important functions like killJob, killTask and
view job details in a JSP.
> Deprecate mapred.permissions.supergroup in favor of
> hadoop.cluster.administrators
> ---------------------------------------------------------------------------------
>
> Key: MAPREDUCE-1542
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-1542
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Components: security
> Reporter: Vinod K V
> Assignee: Ravi Gummadi
> Fix For: 0.22.0
>
> Attachments: 1542.20S.patch, 1542.patch, 1542.v1.patch,
> mapreduce-1542-y20s.patch
>
>
> HADOOP-6568 added the configuration {{hadoop.cluster.administrators}} through
> which admins can configure who the superusers/supergroups for the cluster
> are. MAPREDUCE itself already has {{mapred.permissions.supergroup}} (which is
> just a single group). As agreed upon at HADOOP-6568, this should be
> deprecated in favor of {{hadoop.cluster.administrators}}.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
