[
https://issues.apache.org/jira/browse/MAPREDUCE-1543?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12845766#action_12845766
]
Amar Kamat commented on MAPREDUCE-1543:
---------------------------------------
bq. I think AuditLogManager can be pulled into a separate class file.
+1.
bq. I would instead suggest that the Keys be defined as an Enum
+1
bq. I am not sure 'agent' is conveying the right meaning.
HDFS uses ugi. As we discussed, we would be logging the usernames in MapReduce.
I think agent can act as a common word for ugi or username. Agent is someone
who is external to the system and tries to cause a state change. I am ok with
changing it to something better. I named the IP key as 'ip' because hdfs kept
it that way.
bq. Reason seems confusing.
I have removed it in my next patch. It doesnt add much value to MapReduce
because we are dealing with authorization events here. The reason why I made
them enums was for ease of parsing. Having strings in key=value formats can
lead to issues to do with spaces etc.
bq. Don't tabs make the lines appear too long,
This is inline with hdfs.
bq. Some values like 'permissions' (if this is a free format string) can have
whitespaces
I would rather prefer having one word representation for permissions for audit
logs. Example
{noformat}
user{u1,u2,u3}:group{g1,g2,g3}
{noformat}
Key=Value pairs with free format strings can lead to issues. We can quote them
but the parser logic would become complex.
bq. Suggest the parameter 'operation' could be of type String as well.
I would rather prefer Enums and have JobTracker.Operations and
JobInProgress.Operations. Note we already have QueueManager.QueueOperations.
bq. If there's no cost to logging remote-ip,
+1
bq. The comment '//for testcases' is misleading.
+1
bq. Why are we logging success again in JobTracker.addJob, wouldn't the same
line come up in QueueManager.checkAccess ?
Because they are 2 authorization checks involved :
1) Queue access (success or failure)
2) Job submission (success or failure based on username in config)
bq. 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 ?
+1 for factoring it out.
bq. I don't think we need to give 'Queue:' prefix,
+1
bq.Failure logging should be at WARN level - as done in the other audit log.
If audit logs are important then what is the point of making some of them WARN,
ERROR or FATAL? Shouldn't all the audit logs be INFO logs?
bq. Can we move the refreshAcls logging into QueueManager.refreshQueueAcls - so
that failures can be logged too.
+1
bq. Can we add a few simple unit tests that test the formatting of the log
message alone
+1
> 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.
