[
https://issues.apache.org/jira/browse/HDFS-9395?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15136617#comment-15136617
]
Vinayakumar B commented on HDFS-9395:
-------------------------------------
Below are the places need fix
*Audit Logging In Finally block*
------------------
{code}
addCacheDirective
addCachePool
getEZForPath
listCacheDirectives
listCachePools
modifyCacheDirective
modifyCachePool
removeCacheDirective
removeCachePool
setErasureCodingPolicy
{code}
*Only Success audit logs ( Successfull Authorization )*
--------------------
{code}
allowSnapshot
concat
createSnapshot
deleteSnapshot
disallowSnapshot
finalizeRollingUpgrade
computeSnapshotDiff
listSnapshottableDirectory
renameSnapshot
startRollingUpgrade
{code}
There might be some cases, where ACE will be thrown for Superuser check (admin
commands).
Logging failure audit log for this can be discussed in other Jira (May be
HDFS-5040, since it is intended for that).
Other places needs to be fixed.
In Addition,
{{FSN#renameTo(..)}}, is mixing rename result with permission check to log at
last. This will conflict with above scheme. So, instead only consider
permission check.
Based on above changes, Some audit logs ( for non-ACE failures ) will go
missing.
So this change needs to be marked as Incompatible, for heads-up.
Patch should cover all above methods. I see some are not covered in current
patch.
Current changes in the patch looks good.
Following are some nits.
1.
{code}
+ } catch (AccessControlException ace) {
+ success = false;
+ logAuditEvent(success, "deleteSnapshot", rootPath, null, null);
+ throw ace;
{code}
{code}
+ } catch (AccessControlException ace) {
+ success = false;
+ logAuditEvent(success, "modifyCacheDirective", idStr,
+ directive.toString(), null);
+ throw ace;
} finally {
{code}
{{success=false}} is not required. Its already false.
2.
{code} effectiveDirective = FSNDNCacheOp.addCacheDirective(this,
cacheManager,
directive, flags, logRetryCache);
+ } catch (AccessControlException ace) {
+ logAuditEvent(success, "addCacheDirective", null,
+ null, null);
+ throw ace;{code}
In {{FSNamesystem#addCacheDirective(..)}}, above code will not return null, it
will either return non-null value, or throw exception.
So, similar to other methods, {{success = true;}} can be inside try-block
itself. And other null checks may not be required for {{effectiveDirective}}
3. How about making the one cluster for all tests in @BeforeClass to speedup
test?
> HDFS operations vary widely in which failures they put in the audit log and
> which they leave out
> ------------------------------------------------------------------------------------------------
>
> Key: HDFS-9395
> URL: https://issues.apache.org/jira/browse/HDFS-9395
> Project: Hadoop HDFS
> Issue Type: Bug
> Reporter: Kihwal Lee
> Assignee: Kuhu Shukla
> Attachments: HDFS-9395.001.patch, HDFS-9395.002.patch,
> HDFS-9395.003.patch, HDFS-9395.004.patch
>
>
> So, the big question here is what should go in the audit log? All failures,
> or just "permission denied" failures? Or, to put it a different way, if
> someone attempts to do something and it fails because a file doesn't exist,
> is that worth an audit log entry?
> We are currently inconsistent on this point. For example, concat,
> getContentSummary, addCacheDirective, and setErasureEncodingPolicy create an
> audit log entry for all failures, but setOwner, delete, and setAclEntries
> attempt to only create an entry for AccessControlException-based failures.
> There are a few operations, like allowSnapshot, disallowSnapshot, and
> startRollingUpgrade that never create audit log failure entries at all. They
> simply log nothing for any failure, and log success for a successful
> operation.
> So to summarize, different HDFS operations currently fall into 3 categories:
> 1. audit-log all failures
> 2. audit-log only AccessControlException failures
> 3. never audit-log failures
> Which category is right? And how can we fix the inconsistency
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
