Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: HDFS ACLs (HDFS-4685)
    • Fix Version/s: HDFS ACLs (HDFS-4685)
    • Component/s: namenode
    • Labels:
      None

      Description

      Implement and test handling of default ACLs within NameNode.

      1. HDFS-5616.1.patch
        37 kB
        Chris Nauroth
      2. HDFS-5616.2.patch
        51 kB
        Chris Nauroth
      3. HDFS-5616.3.patch
        51 kB
        Chris Nauroth

        Issue Links

          Activity

          Chris Nauroth created issue -
          Chris Nauroth made changes -
          Field Original Value New Value
          Affects Version/s HDFS ACLs (HDFS-4685) [ 12325671 ]
          Target Version/s HDFS ACLs (HDFS-4685) [ 12325671 ]
          Chris Nauroth made changes -
          Assignee Chris Nauroth [ cnauroth ]
          Chris Nauroth made changes -
          Link This issue is required by HDFS-5827 [ HDFS-5827 ]
          Chris Nauroth made changes -
          Link This issue is related to HDFS-5702 [ HDFS-5702 ]
          Vinayakumar B made changes -
          Link This issue duplicates HDFS-5827 [ HDFS-5827 ]
          Hide
          Chris Nauroth added a comment -

          I'm attaching the patch that implements copying the parent default ACL defined on a directory to its newly created child files and directories. Haohui Mai, would you please review?

          Summary of changes:

          1. AclStorage: Implemented copyINodeDefaultAcl and refactored to share common code with the existing updateINodeAcl method.
          2. FSDirectory: Call AclStorage#copyINodeDefaultAcl when needed, and log OP_SET_ACL for a mkdir that receives a copy of a default ACL.
          3. FSEditLogOp: Guarantee that the ACL bit is masked off for serialization of OP_ADD and OP_MKDIR. The comments fully explain the reason for this.
          4. FSNamesystem: Log OP_SET_ACL for a new file that receives a copy of a default ACL.
          5. ScopedAclEntries: I noticed that it was possible for callers to get null returned from getAccessEntries, which contradicts the statement in the JavaDocs that it would return an empty list. This isn't causing any real problems right now, but I'd like to fix it for consistency.
          6. FSAclBaseTest: Added new tests for various cases of the default ACL getting copied to new children.
          7. TestFSImageWithAcl: Added new tests asserting that ACLs copied from a default ACL remain persistent in fsimage and edits.
          8. TestWebHDFSAcl: We need to skip one of the new tests defined in FSAclBaseTest when running it against WebHdfsFileSystem, because WebHdfsFileSystem does not currently resolve symlinks. This is a known issue unrelated to ACLs.
          9. testAclCLI.xml: We previously wrote some CLI tests related to default ACL handling, and committed them knowing they'd be broken until default ACL handling was implemented. For one of these tests, the assertions were just slightly off. The group perms get inherited as-is, and then the mask is responsible for restricting it.
          Show
          Chris Nauroth added a comment - I'm attaching the patch that implements copying the parent default ACL defined on a directory to its newly created child files and directories. Haohui Mai , would you please review? Summary of changes: AclStorage : Implemented copyINodeDefaultAcl and refactored to share common code with the existing updateINodeAcl method. FSDirectory : Call AclStorage#copyINodeDefaultAcl when needed, and log OP_SET_ACL for a mkdir that receives a copy of a default ACL. FSEditLogOp : Guarantee that the ACL bit is masked off for serialization of OP_ADD and OP_MKDIR . The comments fully explain the reason for this. FSNamesystem : Log OP_SET_ACL for a new file that receives a copy of a default ACL. ScopedAclEntries : I noticed that it was possible for callers to get null returned from getAccessEntries , which contradicts the statement in the JavaDocs that it would return an empty list. This isn't causing any real problems right now, but I'd like to fix it for consistency. FSAclBaseTest : Added new tests for various cases of the default ACL getting copied to new children. TestFSImageWithAcl : Added new tests asserting that ACLs copied from a default ACL remain persistent in fsimage and edits. TestWebHDFSAcl : We need to skip one of the new tests defined in FSAclBaseTest when running it against WebHdfsFileSystem , because WebHdfsFileSystem does not currently resolve symlinks. This is a known issue unrelated to ACLs. testAclCLI.xml : We previously wrote some CLI tests related to default ACL handling, and committed them knowing they'd be broken until default ACL handling was implemented. For one of these tests, the assertions were just slightly off. The group perms get inherited as-is, and then the mask is responsible for restricting it.
          Chris Nauroth made changes -
          Attachment HDFS-5616.1.patch [ 12627211 ]
          Chris Nauroth made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Haohui Mai added a comment -

          A couple comments:

          copyINodeDefaultAcl(INodeDirectory parent, INode child)
          

          You can pass in child only, and get its parent via child.getParent(). You might also want to refactor the code to create a path for early return. For example:

          if (!child.isFile() || !child.isDirectory())
            return;
          

          That gives a clear view of the pre-conditions of the function.

          You might want to create some utility functions like isMinimalAcl() to simplify the code. I think it can be addressed in a separate jira.

          One thing that I'm unclear is that the rationales on masking ACL bit on OP_ADD and OP_MKDIR. It seems to me that with default ACLs, an add operation will correspond to two edit logs entries. (an OP_ADD and an OP_SET_ACL). My concern is that the operation is no longer atomic. If the edit log truncates right before OP_SET_ACL, the semantic of default ACLs is lost. I'm wondering whether we should create new ops to maintain the atomic guarantee.

          Show
          Haohui Mai added a comment - A couple comments: copyINodeDefaultAcl(INodeDirectory parent, INode child) You can pass in child only, and get its parent via child.getParent() . You might also want to refactor the code to create a path for early return. For example: if (!child.isFile() || !child.isDirectory()) return ; That gives a clear view of the pre-conditions of the function. You might want to create some utility functions like isMinimalAcl() to simplify the code. I think it can be addressed in a separate jira. One thing that I'm unclear is that the rationales on masking ACL bit on OP_ADD and OP_MKDIR . It seems to me that with default ACLs, an add operation will correspond to two edit logs entries. (an OP_ADD and an OP_SET_ACL ). My concern is that the operation is no longer atomic. If the edit log truncates right before OP_SET_ACL , the semantic of default ACLs is lost. I'm wondering whether we should create new ops to maintain the atomic guarantee.
          Hide
          Chris Nauroth added a comment -

          Thanks, Haohui. These are good suggestions, and I'll incorporate them into a v2 patch here.

          If the edit log truncates right before OP_SET_ACL, the semantic of default ACLs is lost.

          In an earlier version of this code, I changed OP_ADD and OP_MKDIR to optionally encode the ACL entries. That would have made it atomic like you asked. Then, I switched to this two-op combo, because I thought it might be helpful to use hdfs namenode -recover to get back the file even if its OP_SET_ACL got truncated/corrupted.

          However, now I'm doubting that decision. If we recover the file without all of its security restrictions in place, then that might not be such a helpful thing depending on the sensitivity of the data. I'll resurrect the former version of the code when I post v2 so that we have a guarantee of atomicity.

          Show
          Chris Nauroth added a comment - Thanks, Haohui. These are good suggestions, and I'll incorporate them into a v2 patch here. If the edit log truncates right before OP_SET_ACL, the semantic of default ACLs is lost. In an earlier version of this code, I changed OP_ADD and OP_MKDIR to optionally encode the ACL entries. That would have made it atomic like you asked. Then, I switched to this two-op combo, because I thought it might be helpful to use hdfs namenode -recover to get back the file even if its OP_SET_ACL got truncated/corrupted. However, now I'm doubting that decision. If we recover the file without all of its security restrictions in place, then that might not be such a helpful thing depending on the sensitivity of the data. I'll resurrect the former version of the code when I post v2 so that we have a guarantee of atomicity.
          Hide
          Chris Nauroth added a comment -

          I'm attaching a v2 patch that addresses the code review feedback and changes the edit logging to embed the ACL entries directly into the OP_ADD or OP_MKDIR. It's an optional field only used when the new file/directory has received a copy of a default ACL. I used AclFsImageProto for this, because I just needed the list of ACL entries and not the source.

          Show
          Chris Nauroth added a comment - I'm attaching a v2 patch that addresses the code review feedback and changes the edit logging to embed the ACL entries directly into the OP_ADD or OP_MKDIR . It's an optional field only used when the new file/directory has received a copy of a default ACL. I used AclFsImageProto for this, because I just needed the list of ACL entries and not the source.
          Chris Nauroth made changes -
          Attachment HDFS-5616.2.patch [ 12627303 ]
          Hide
          Haohui Mai added a comment -
                                                                                                                                                                                                                   
          +      } else if (type == AclEntryType.MASK) {                                                                                                                                                                 
          +        permission = entry.getPermission().and(childPerm.getGroupAction());                                                                                                                                   
          

          It looks inconsistent with other branches. Is it intentional?

                                                                                                                                                                                                                   
          +    return new AclFeature(Collections.unmodifiableList(featureEntries));                                                                                                                                      
          

          I think this is an issue might worth revisiting. Currently we place a List<AclEntry> in AclFeature, which does not capture the fact that the list should be immutable. A better approach is to change \
          List<AclEntry> to ImmuableList<AclEntry> in the AclFeature class and related functions to capture this fact. However, I think you can address it in a separate jira.

          Show
          Haohui Mai added a comment - + } else if (type == AclEntryType.MASK) { + permission = entry.getPermission().and(childPerm.getGroupAction()); It looks inconsistent with other branches. Is it intentional? + return new AclFeature(Collections.unmodifiableList(featureEntries)); I think this is an issue might worth revisiting. Currently we place a List<AclEntry> in AclFeature , which does not capture the fact that the list should be immutable. A better approach is to change \ List<AclEntry> to ImmuableList<AclEntry> in the AclFeature class and related functions to capture this fact. However, I think you can address it in a separate jira.
          Hide
          Chris Nauroth added a comment -

          It looks inconsistent with other branches. Is it intentional?

          This is intentional. With an extended ACL, on child creation, the group bits from the mode parameter are a further filter on the permission bits of the mask entry received from the parent's default ACL.

          A better approach is to change List<AclEntry> to ImmutableList<AclEntry> in the AclFeature class...

          It's a good idea. It turns out I've got the same thing in my TODO list, but hadn't entered it in jira yet. I just filed HDFS-5908 for it.

          Show
          Chris Nauroth added a comment - It looks inconsistent with other branches. Is it intentional? This is intentional. With an extended ACL, on child creation, the group bits from the mode parameter are a further filter on the permission bits of the mask entry received from the parent's default ACL. A better approach is to change List<AclEntry> to ImmutableList<AclEntry> in the AclFeature class... It's a good idea. It turns out I've got the same thing in my TODO list, but hadn't entered it in jira yet. I just filed HDFS-5908 for it.
          Hide
          Haohui Mai added a comment -

          This is intentional. With an extended ACL, on child creation, the group bits from the mode parameter are a further filter on the permission bits of the mask entry received from the parent's default ACL.

          It might be worthwhile to leave a one-liner comment then. +1 after addressing it.

          Show
          Haohui Mai added a comment - This is intentional. With an extended ACL, on child creation, the group bits from the mode parameter are a further filter on the permission bits of the mask entry received from the parent's default ACL. It might be worthwhile to leave a one-liner comment then. +1 after addressing it.
          Hide
          Chris Nauroth added a comment -

          Here is patch version 3 adding the comment. Thank you, Haohui.

          Show
          Chris Nauroth added a comment - Here is patch version 3 adding the comment. Thank you, Haohui.
          Chris Nauroth made changes -
          Attachment HDFS-5616.3.patch [ 12627679 ]
          Chris Nauroth made changes -
          Link This issue is required by HDFS-5899 [ HDFS-5899 ]
          Hide
          Haohui Mai added a comment -

          Looks good to me. +1

          Show
          Haohui Mai added a comment - Looks good to me. +1
          Hide
          Chris Nauroth added a comment -

          Thanks for another helpful code review, Haohui. I've committed this to the HDFS-4685 feature branch.

          Show
          Chris Nauroth added a comment - Thanks for another helpful code review, Haohui. I've committed this to the HDFS-4685 feature branch.
          Chris Nauroth made changes -
          Status In Progress [ 3 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s HDFS ACLs (HDFS-4685) [ 12325671 ]
          Resolution Fixed [ 1 ]

            People

            • Assignee:
              Chris Nauroth
              Reporter:
              Chris Nauroth
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development