[
https://issues.apache.org/jira/browse/HDFS-6570?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14067685#comment-14067685
]
Chris Nauroth commented on HDFS-6570:
-------------------------------------
Hi, [~jnp]. The patch is looking good so far. Here are a few comments:
# acl.proto: I'm not sure it's backwards-compatible to take the existing
{{FsActionProto}} nested inside {{AclEntryProto}} and move it to top level. If
protobuf encodes the message name now as "AclEntryProto.FsActionProto", then it
might break interop. It would be interesting to test "hdfs dfs -getfacl" on
files with ACLs using a mix of old client + new server or new client + old
server. If there is a problem, then we might need to find a way to refer to
the nested definition, or if all else fails maintain duplicate definitions
(nested and top-level) just for comaptibility.
# {{FSNamesystem}}: There are a few things missing here. We need to hold the
read lock so that we don't get unexpected behavior while another thread mutates
the part of the tree that we're traversing. We also need to check that the
current HA context allows a read operation. {{getAclStatus}} is probably the
simplest method to look at for an example that does everything. Do you think
we need to write to the audit log for this method? I'm thinking that we
shouldn't, because the purpose of this method is to query whether or not the
user has access. A no answer isn't really denying the call from happening, so
I don't think it's an interesting event to audit. If you agree, then maybe we
should put a comment in here stating that we intentionally do not write to the
audit log.
# {{NamenodeWebHdfsMethods}}: There are some merge conflicts in the patch that
made it challenging to review, but it looks like the changes are on the right
track.
# {{WebHdfsFileSystem}}: Would this throw the expected
{{FileNotFoundException}} when trying to call {{access}} on a non-existent
path? Methods like {{getHdfsFileStatus}} and {{getAclStatus}} have coded an
explicit check on a null JSON response.
# {{FsActionParam}}: We could potentially improve input validation by
specifying a simple regex for the {{Domain}}, like \[rwx-\]\{3\}. See
{{AclPermissionParam}}, which embeds the same permission string format inside
ACL entries.
# {{GetOpParam}}: I don't think passing {{true}} for the {{requireAuth}}
argument is correct. That's just for the operations related to
getting/renewing/canceling delegation tokens, not the typical file system
operations.
# We'll need to add the new method to the WebHDFS REST API documentation.
# Just an optional thought: much of this patch file's size is due to reordering
import statements. You might consider dropping that part for now and filing a
separate pure refactoring patch later as cleanup to make the patches more
manageable. This way is fine too though if you prefer.
> add api that enables checking if a user has certain permissions on a file
> -------------------------------------------------------------------------
>
> Key: HDFS-6570
> URL: https://issues.apache.org/jira/browse/HDFS-6570
> Project: Hadoop HDFS
> Issue Type: Bug
> Reporter: Thejas M Nair
> Assignee: Jitendra Nath Pandey
> Attachments: HDFS-6570-prototype.1.patch, HDFS-6570.2.patch
>
>
> For some of the authorization modes in Hive, the servers in Hive check if a
> given user has permissions on a certain file or directory. For example, the
> storage based authorization mode allows hive table metadata to be modified
> only when the user has access to the corresponding table directory on hdfs.
> There are likely to be such use cases outside of Hive as well.
> HDFS does not provide an api for such checks. As a result, the logic to check
> if a user has permissions on a directory gets replicated in Hive. This
> results in duplicate logic and there introduces possibilities for
> inconsistencies in the interpretation of the permission model. This becomes a
> bigger problem with the complexity of ACL logic.
> HDFS should provide an api that provides functionality that is similar to
> access function in unistd.h - http://linux.die.net/man/2/access .
--
This message was sent by Atlassian JIRA
(v6.2#6252)
