[
https://issues.apache.org/jira/browse/MAPREDUCE-1543?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12845757#action_12845757
]
Hemanth Yamijala commented on MAPREDUCE-1543:
---------------------------------------------
Some review comments on the patch uploaded (mapreduce-1543-y20s.patch):
Most of the comments are minor adjustments to the log API to make it a little
of cleaner.. Overall the approach in the patch seems clean to me.
- I think AuditLogManager can be pulled into a separate class file.
- Keeping the list of keys as javadoc comments seems like a maintenance
overhead. I would instead suggest that the Keys be defined as an Enum. Then it
will be easy to query the Enum names as an API. Doing this will also prevent
the duplication of the key names in the logSuccess and logFailure APIs.
- I am not sure 'agent' is conveying the right meaning. Seems like 'user' or
'userid' will be more clear. What is the equivalent in DFS ? Also, ip should be
'remote-ip'.
- Reason seems confusing. Can we keep it generic - maybe as a string ? For e.g.
in JobInProgress, where we check the conf user is the same as the submitting
user, and fails, 'permission denied' is not giving a proper meaning. We can
expand and say what exactly failed. In fact, maybe we can change the name of
the tag 'Reason' to 'AdditionalInfo' or something like that so it is more
general. Then, in QueueManager, I think it makes sense to log the jobid, when
it is available (note there are cases when it is not available), maybe as
additional Info. This way we can do things like grep by job-id and get a full
trail.
- Don't tabs make the lines appear too long, Blanks would suffice, no ?
- Some values like 'permissions' and 'reason' (if this is a free format string)
can have whitespaces. Would it be good to surround these with quotes for easy
parsing ?
- Suggest the parameter 'operation' could be of type String as well - if we are
going to do a toString anyway.
- If there's no cost to logging remote-ip, I'd recommend adding it everywhere
as a mandatory parameter.
- The comment '//for testcases' is misleading. I think what you mean is that
the check for IP being null is required as test cases may not have this
defined. I'd suggest it be documented as such.
- Why are we logging success again in JobTracker.addJob, wouldn't the same line
come up in QueueManager.checkAccess ?
- Seems like Operations "REFRESH_NODES", "QUEUE_REFRESH" and target
"Jobtracker" can be static final variables. Should we pull all such
AuditLogManager related constants into some constants file ? Like
AuditLogConstants or something ?
- "REFRESH_NODES" and "QUEUE_REFRESH" seem similar operations, but they have
different names, it should be "REFRESH_NODES" and "REFRESH_QUEUES"
- JobACLsManager.checkAccess has some lines incorrectly indented.
- I don't think we need to give 'Queue:' prefix, it is understood that if it is
a queue operation, then the target will be a queue.
- Failure logging should be at WARN level - as done in the other audit log.
- Can we move the refreshAcls logging into QueueManager.refreshQueueAcls - so
that failures can be logged too.
- Can we add a few simple unit tests that test the formatting of the log
message alone. i.e. we can define a package private API in AuditLogManager -
something like String getLogLine(...). We can call these in the log*** APIs in
AuditLogManager. We can also call it with various arguments in tests and verify
that the return string is proper. We can test null values, values with
whitespace etc.
> Log messages of JobACLsManager should use security logging of HADOOP-6586
> -------------------------------------------------------------------------
>
> Key: MAPREDUCE-1543
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-1543
> Project: Hadoop Map/Reduce
> Issue Type: Bug
> Components: security
> Reporter: Vinod K V
> Assignee: Amar Kamat
> Fix For: 0.22.0
>
> Attachments: mapreduce-1543-y20s.patch
>
>
> {{JobACLsManager}} added in MAPREDUCE-1307 logs the successes and failures
> w.r.t job-level authorization in the corresponding Daemons' logs. The log
> messages should instead use security logging of HADOOP-6586.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
