Hadoop Common
  1. Hadoop Common
  2. HADOOP-10184 Hadoop Common changes required to support HDFS ACLs.
  3. HADOOP-10187

FsShell CLI: add getfacl and setfacl with minimal support for getting and setting ACLs.

    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: tools
    • Labels:
      None

      Description

      Implement and test FsShell CLI commands for getfacl and setfacl.

      1. HADOOP-10187.patch
        14 kB
        Vinayakumar B
      2. HDFS-5600.patch
        14 kB
        Vinayakumar B
      3. HDFS-5600.patch
        13 kB
        Vinayakumar B
      4. HDFS-5600.patch
        13 kB
        Vinayakumar B

        Issue Links

          Activity

          Hide
          Vinayakumar B added a comment -

          Attaching a patch for FsShell CLI implementations.
          Tests needs to be updated after namenode implementations.

          Show
          Vinayakumar B added a comment - Attaching a patch for FsShell CLI implementations. Tests needs to be updated after namenode implementations.
          Hide
          Chris Nauroth added a comment -

          Hi, Vinayakumar B. Thank you again for volunteering to do this. I'll keep you informed as the rest of the implementation comes together and you'll be able to test the CLI end-to-end. Meanwhile, here are a couple of comments about the current patch that we'll want to address:

          1. FsCommand: minor: It looks like there is precedent for listing the command classes in alphabetical order in registerCommands, so let's put AclCommands at the top. (This is just a style nit. It doesn't change functionality, because the help output always sorts the command names.)
          2. FsAction#getFSAction: This class has a cached copy of all values in the vals static member, which you can reuse here instead of making a separate call to FsAction#values. In the Javadoc, I wasn't sure what "3 String representation" meant. Was that supposed to be "3-character String representation"? Let's also have the Javadoc mention that it returns null if not found.
          3. Thanks for getting started on tests to cover the validation logic. Eventually, I think we'll want something like the other HDFS CLI XML-based tests, i.e. org.apache.hadoop.cli.TestAclCLI and a corresponding testAclCLIConf.xml.
          4. AclCommands: The short USAGE string literals should not have newline characters at the end. Right now, when I run "hdfs dfs", I see the getfacl and setfacl command usage slightly misformatted across multiple lines because of this. Also, setfacl has an internal new line separating the 2 variants which further misformats the output. We might need to separate the 2 variants with a comma or find some other clever way to pack the usage onto a single line.
          5. AclCommands: Instead of calling System.out.println, please call out.println. out is a PrintStream inherited from the Command base class.
          6. AclCommands#processPath: getfacl also prints a file's special flags. In HDFS, the only special flag we implement is sticky bit, so let's print that information. We already made sticky bit available on the AclStatus object. Here is example output from getfacl on a sticky directory on Linux. The "flags" line should only be printed if the file has sticky bit:
            mkdir sticky
            chmod 01777 sticky
            getfacl sticky
            
            # file: sticky/
            # owner: cnauroth
            # group: cnauroth
            # flags: --t
            user::rwx
            group::rwx
            other::rwx
            
          7. For setfacl, we won't have support for the -d flag (at least not yet). There won't be support for it at the API level, so please remove that option from the CLI.
          8. AclCommands#SetfaclCommand#processOptions: I found it challenging to read the conditional that checks for conflicting options (though I think your logic is correct). Can you please try reformatting or perhaps introducing some helper methods to improve readability?
          9. AclCommands#SetfaclCommand#parseAclSpec: The CLI needs to be able to accept a mix of access ACL entries and default ACL entries in a single acl_spec. Default ACL entries begin with "default:". Access ACL entries omit this. For example:
            setfacl -m user:vinay:rw-,default:user:cnauroth:r-- /file1
            

            This means that the parsing logic needs to handle a split of length 3 or 4 (not just 3). If the length is 4, then the first element must be "default", and the parser handles this by setting default scope on the entry builder.

          10. AclCommands#SetfaclCommand#parseAclSpec: Optional: You might be able to simplify parsing of type by calling Enum.valueOf(AclEntryType.class, type). It might make error handling a little different though, so let me know what you think.
          Show
          Chris Nauroth added a comment - Hi, Vinayakumar B . Thank you again for volunteering to do this. I'll keep you informed as the rest of the implementation comes together and you'll be able to test the CLI end-to-end. Meanwhile, here are a couple of comments about the current patch that we'll want to address: FsCommand : minor: It looks like there is precedent for listing the command classes in alphabetical order in registerCommands , so let's put AclCommands at the top. (This is just a style nit. It doesn't change functionality, because the help output always sorts the command names.) FsAction#getFSAction : This class has a cached copy of all values in the vals static member, which you can reuse here instead of making a separate call to FsAction#values . In the Javadoc, I wasn't sure what "3 String representation" meant. Was that supposed to be "3-character String representation"? Let's also have the Javadoc mention that it returns null if not found. Thanks for getting started on tests to cover the validation logic. Eventually, I think we'll want something like the other HDFS CLI XML-based tests, i.e. org.apache.hadoop.cli.TestAclCLI and a corresponding testAclCLIConf.xml . AclCommands : The short USAGE string literals should not have newline characters at the end. Right now, when I run "hdfs dfs", I see the getfacl and setfacl command usage slightly misformatted across multiple lines because of this. Also, setfacl has an internal new line separating the 2 variants which further misformats the output. We might need to separate the 2 variants with a comma or find some other clever way to pack the usage onto a single line. AclCommands : Instead of calling System.out.println , please call out.println . out is a PrintStream inherited from the Command base class. AclCommands#processPath : getfacl also prints a file's special flags. In HDFS, the only special flag we implement is sticky bit, so let's print that information. We already made sticky bit available on the AclStatus object. Here is example output from getfacl on a sticky directory on Linux. The "flags" line should only be printed if the file has sticky bit: mkdir sticky chmod 01777 sticky getfacl sticky # file: sticky/ # owner: cnauroth # group: cnauroth # flags: --t user::rwx group::rwx other::rwx For setfacl, we won't have support for the -d flag (at least not yet). There won't be support for it at the API level, so please remove that option from the CLI. AclCommands#SetfaclCommand#processOptions : I found it challenging to read the conditional that checks for conflicting options (though I think your logic is correct). Can you please try reformatting or perhaps introducing some helper methods to improve readability? AclCommands#SetfaclCommand#parseAclSpec : The CLI needs to be able to accept a mix of access ACL entries and default ACL entries in a single acl_spec. Default ACL entries begin with "default:". Access ACL entries omit this. For example: setfacl -m user:vinay:rw-, default :user:cnauroth:r-- /file1 This means that the parsing logic needs to handle a split of length 3 or 4 (not just 3). If the length is 4, then the first element must be "default", and the parser handles this by setting default scope on the entry builder. AclCommands#SetfaclCommand#parseAclSpec : Optional: You might be able to simplify parsing of type by calling Enum.valueOf(AclEntryType.class, type) . It might make error handling a little different though, so let me know what you think.
          Hide
          Vinayakumar B added a comment -

          Thanks Chris for the detailed review and comments.

          I have tried to address all your comments.

          Regarding XML based tests, started working on it. Will update in a day or two as I am on travel as of now.

          please have a look at updated patch. Thanks again

          Show
          Vinayakumar B added a comment - Thanks Chris for the detailed review and comments. I have tried to address all your comments. Regarding XML based tests, started working on it. Will update in a day or two as I am on travel as of now. please have a look at updated patch. Thanks again
          Hide
          Uma Maheswara Rao G added a comment -

          Thanks Vinay for working on this. I agree to wait until completion of core implementation to test end to end functionality with CLI.
          For xml based tests you may want to see CLITestHelper. and you can also see TestCacheAdminCLI for example.

          Please change getFSAction --> getFsAction

          Chris Nauroth
          I think we have to raise separate umbrella JIRA for Common side for ACL changes and link to this parent JIRA of HDFS side changes?

          Show
          Uma Maheswara Rao G added a comment - Thanks Vinay for working on this. I agree to wait until completion of core implementation to test end to end functionality with CLI. For xml based tests you may want to see CLITestHelper. and you can also see TestCacheAdminCLI for example. Please change getFSAction --> getFsAction Chris Nauroth I think we have to raise separate umbrella JIRA for Common side for ACL changes and link to this parent JIRA of HDFS side changes?
          Hide
          Vinayakumar B added a comment -

          Attaching the updated patch.
          Addressed Uma's comment.
          Updated stickybit printing based on OTHER's permission.

          Show
          Vinayakumar B added a comment - Attaching the updated patch. Addressed Uma's comment. Updated stickybit printing based on OTHER's permission.
          Hide
          Chris Nauroth added a comment -

          Both Vinayakumar B and Uma Maheswara Rao G requested a separate umbrella jira for Hadoop Common changes related to ACLs, so I've created HADOOP-10184 and converted a few issues to be sub-tasks of that. The FsShell CLI is now tracked as HADOOP-10187.

          Show
          Chris Nauroth added a comment - Both Vinayakumar B and Uma Maheswara Rao G requested a separate umbrella jira for Hadoop Common changes related to ACLs, so I've created HADOOP-10184 and converted a few issues to be sub-tasks of that. The FsShell CLI is now tracked as HADOOP-10187 .
          Hide
          Chris Nauroth added a comment -

          Vinay, thanks for incorporating the last round of feedback. Here are just 2 more comments. After this, you'll need to wait for some other patches to land before you can do end-to-end testing.

          1. FsCommand: Let's mention in the JavaDoc that it returns null if not found.
          2. AclCommands#GetfaclCommand#processPath: Inside the loop, in addition to checking the entry type, we'll also need to check that the scope is AclEntryScope#ACCESS. (It would be incorrect to change the sticky flag for a default ACL entry that turns off execute permission.)
          Show
          Chris Nauroth added a comment - Vinay, thanks for incorporating the last round of feedback. Here are just 2 more comments. After this, you'll need to wait for some other patches to land before you can do end-to-end testing. FsCommand : Let's mention in the JavaDoc that it returns null if not found. AclCommands#GetfaclCommand#processPath : Inside the loop, in addition to checking the entry type, we'll also need to check that the scope is AclEntryScope#ACCESS . (It would be incorrect to change the sticky flag for a default ACL entry that turns off execute permission.)
          Hide
          Vinayakumar B added a comment -

          Updated the patch

          FsCommand: Let's mention in the JavaDoc that it returns null if not found.

          I didn't get this. Did you mean FsAction#getFsAction()? I have updated the Javadoc for this.

          Also moved TestAclCommands to common code.
          End-to-end xml-based test will be added in hdfs later. I will file a Jira for this.

          Show
          Vinayakumar B added a comment - Updated the patch FsCommand: Let's mention in the JavaDoc that it returns null if not found. I didn't get this. Did you mean FsAction#getFsAction() ? I have updated the Javadoc for this. Also moved TestAclCommands to common code. End-to-end xml-based test will be added in hdfs later. I will file a Jira for this.
          Hide
          Vinayakumar B added a comment -

          Filed HDFS-5702 to track e2e tests

          Show
          Vinayakumar B added a comment - Filed HDFS-5702 to track e2e tests
          Hide
          Chris Nauroth added a comment -

          Sorry my last comment wasn't clear, but you're correct. Thank you for adding that to the Javadoc.

          Thanks also for filing HDFS-5702. With that part separated, I think we can commit this patch to the feature branch. I'm planning to do a final review pass and commit later today.

          Show
          Chris Nauroth added a comment - Sorry my last comment wasn't clear, but you're correct. Thank you for adding that to the Javadoc. Thanks also for filing HDFS-5702 . With that part separated, I think we can commit this patch to the feature branch. I'm planning to do a final review pass and commit later today.
          Hide
          Chris Nauroth added a comment -

          +1 for the patch. I committed this to the HDFS-4685 feature branch. Vinay, thank you for the contribution. Uma, thank you for help with code review.

          Show
          Chris Nauroth added a comment - +1 for the patch. I committed this to the HDFS-4685 feature branch. Vinay, thank you for the contribution. Uma, thank you for help with code review.

            People

            • Assignee:
              Vinayakumar B
              Reporter:
              Chris Nauroth
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development