[
https://issues.apache.org/jira/browse/HDFS-5673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13863358#comment-13863358
]
Chris Nauroth commented on HDFS-5673:
-------------------------------------
Thanks, Haohui. In that case, let's work towards getting some variant of the
v4 patch committed. I'll get to work on a v5. Some of the simplifications you
proposed might not be feasible due to a few complexities in the requirements.
See details below. Bottom line: if I can get the unit tests passing with these
simplifications, then we can keep them.
bq. promoteDefaultEntries...
There are a couple of additional complexities to the logic: 1) This promotion
must only be triggered if the ACL spec has specified some kind of change to the
default ACL. A change to only the access ACL must not trigger promotion. 2)
Promotion only occurs for the base entries that are missing from the default
ACL. It must not overwrite a default entry that the user provided in the ACL
spec. This is why the logic is tracking both access entries and default
entries. Example: if user specifies default owner entry and default group
entry, but no default other entry, then the logic must use the provided default
owner and default group entries and then copy the access ACL owner entry to the
default ACL. 3) The access mask is an access entry with no name, but the logic
must never promote it to a default mask.
bq. calculateMask...
A few of the challenges with this logic: 1) Automatic mask calculation must not
trigger for the default ACL if the ACL spec is changing only the access ACL
(and vice versa). 2) The logic must know whether a mask entry is coming from
the existing ACL or being added by the ACL spec. The version in the ACL spec
must override the automatic calculation. 3) In the case of
{{filterAclEntriesByAclSpec}}, the logic must have knowledge that some kind of
change was made to either the access ACL or default ACL. The list of ACL
entries alone isn't sufficient to know that, because absence of something in
that list does not necessarily imply deletion. All of these requirements drove
the state tracking related to scopeDirty/maskDirty/providedMask.
bq. I also propose to move the compareTo() function AclEntry out to a new
comparator inside AclTransformationLogic.
Yes, we can do that. Just so I understand the motivation, is this to reduce
the footprint of code shipped client side, particularly code that is sensitive
to server side implementation details? (If so, then I agree with you.)
bq. Is it possible to have multiple entries whose type equals to ACCESS, and
empty names? If the answer is yes, copyDefaultsIfNeeded will always pick up the
last entry. Is it expected?
I didn't quite follow this question, because ACCESS is a scope, and the types
are USER, GROUP, MASK and OTHER. Within any ACL, the combination of scope +
type + name is unique for each entry. Attempting to create entries with
duplicate scope + type + name in the same ACL would get rejected during the
validation checks for duplicate entries. Therefore, for any invocation of
{{copyDefaultsIfNeeded}}, I'd expect each of those code paths to execute at
most once.
bq. How are you going to use these functions?
Each {{FSNamesystem}} method that modifies an ACL will get the current ACL from
the inode feature, pass the existing ACL + the ACL spec to the corresponding
{{AclTransformation}} method, and then set the resulting new ACL back on the
inode feature. {{FSNamesystem#removeAcl}} is unique in that it can simply
remove the ACL feature. There will be some integration with the existing
{{FSNamesystem#setPermission}} too. If calling {{setPermission}} on an inode
with an ACL, then it will convert the client-supplied {{FsPermission}} to a
minimal ACL (containing exactly the 3 base entries), and then call
{{AclTransformation#mergeAclEntries}} to combine that minimal ACL with the
existing ACL. Additionally, {{FSNamesystem#removeAclEntries}},
{{FSNamesystem#removeDefaultAcl}}, and {{FSNamesystem#setAcl}} might result in
reducing back to a minimal ACL that can be represented as a 16-bit
{{FsPermission}}. We'll detect these scenarios and handle it by removing the
ACL feature from the inode. That's a summary of everything I can think of
right now related to how these functions will be used.
> Implement logic for modification of ACLs.
> -----------------------------------------
>
> Key: HDFS-5673
> URL: https://issues.apache.org/jira/browse/HDFS-5673
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Affects Versions: HDFS ACLs (HDFS-4685)
> Reporter: Chris Nauroth
> Assignee: Chris Nauroth
> Attachments: HDFS-5673.1.patch, HDFS-5673.2.patch, HDFS-5673.3.patch,
> HDFS-5673.4.patch
>
>
> This patch will include the core logic for modification of ACLs. This
> includes support for all user-facing APIs that modify ACLs. This will cover
> access ACLs, default ACLs, automatic mask calculations, automatic inference
> of unprovided default ACL entries, and validation to prevent creation of an
> invalid ACL.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
