Hadoop Common
  1. Hadoop Common
  2. HADOOP-10213

Fix bugs parsing ACL spec in FsShell setfacl.

    Details

    • Type: Bug Bug
    • 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

      When calling setfacl -x to remove ACL entries, it does not make sense for the entries in the ACL spec to contain permissions. The permissions should be unspecified, and the CLI should return an error if the user attempts to provide permissions.

      1. HADOOP-10213.patch
        4 kB
        Vinayakumar B
      2. HADOOP-10213.patch
        3 kB
        Vinayakumar B
      3. HADOOP-10213.patch
        8 kB
        Vinayakumar B
      4. HADOOP-10213.patch
        10 kB
        Vinayakumar B
      5. HADOOP-10213.patch
        11 kB
        Vinayakumar B

        Issue Links

          Activity

          Chris Nauroth created issue -
          Chris Nauroth made changes -
          Field Original Value New Value
          Link This issue relates to HADOOP-10187 [ HADOOP-10187 ]
          Chris Nauroth made changes -
          Link This issue relates to HADOOP-10184 [ HADOOP-10184 ]
          Hide
          Chris Nauroth added a comment -

          Here is an example on Linux showing rejection of an attempt to specify permissions in an ACL spec with setfacl -x:

          > setfacl -x user:bruce:rw- file2
          setfacl: Option -x: Invalid argument near character 12
          

          Both of the following are correct syntax:

          setfacl -x user:bruce file2
          setfacl -x user:bruce: file2
          

          Vinayakumar B, if you're interested in taking this, would you please assign the issue to yourself? Thanks!

          Show
          Chris Nauroth added a comment - Here is an example on Linux showing rejection of an attempt to specify permissions in an ACL spec with setfacl -x: > setfacl -x user:bruce:rw- file2 setfacl: Option -x: Invalid argument near character 12 Both of the following are correct syntax: setfacl -x user:bruce file2 setfacl -x user:bruce: file2 Vinayakumar B , if you're interested in taking this, would you please assign the issue to yourself? Thanks!
          Hide
          Vinayakumar B added a comment -

          Thanks chris. I will make necessary changes and post a patch soon..

          Show
          Vinayakumar B added a comment - Thanks chris. I will make necessary changes and post a patch soon..
          Vinayakumar B made changes -
          Assignee Vinay [ vinayrpet ]
          Hide
          Vinayakumar B added a comment -

          Attached the patch for validation for -x.

          Also removed the permissions from equals() and hashCode() as AclEntry always may not carry permissions.

          Show
          Vinayakumar B added a comment - Attached the patch for validation for -x. Also removed the permissions from equals() and hashCode() as AclEntry always may not carry permissions.
          Vinayakumar B made changes -
          Attachment HADOOP-10213.patch [ 12622153 ]
          Hide
          Vinayakumar B added a comment -

          Please review

          Show
          Vinayakumar B added a comment - Please review
          Hide
          Chris Nauroth added a comment -

          Hi, Vinay. This looks good, but I think we'll need to revert the AclEntry portion of the change. There are various unit tests that rely on assertEquals or assertArrayEquals to check that the correct ACL was applied to a file. With this change, those assertEquals calls would pass even if the permissions inside the ACL entries were incorrect.

          Even putting aside tests, this is a public user-facing class, and callers likely would find it surprising if "user:bruce:rwx" and "user:bruce:---" were considered equal.

          Show
          Chris Nauroth added a comment - Hi, Vinay. This looks good, but I think we'll need to revert the AclEntry portion of the change. There are various unit tests that rely on assertEquals or assertArrayEquals to check that the correct ACL was applied to a file. With this change, those assertEquals calls would pass even if the permissions inside the ACL entries were incorrect. Even putting aside tests, this is a public user-facing class, and callers likely would find it surprising if "user:bruce:rwx" and "user:bruce:---" were considered equal.
          Hide
          Chris Nauroth added a comment -

          One more note on the above. The implementation of AclEntry#equals is using Guava's Objects#equal. That method returns true if both instances are null, so I think that addresses your concern stated earlier about correct behavior if the AclEntry does not carry permissions. Objects#hashCode is null-safe too.

          Show
          Chris Nauroth added a comment - One more note on the above. The implementation of AclEntry#equals is using Guava's Objects#equal . That method returns true if both instances are null, so I think that addresses your concern stated earlier about correct behavior if the AclEntry does not carry permissions. Objects#hashCode is null-safe too.
          Hide
          Vinayakumar B added a comment -

          Hi, Vinay. This looks good, but I think we'll need to revert the AclEntry portion of the change. There are various unit tests that rely on assertEquals or assertArrayEquals to check that the correct ACL was applied to a file. With this change, those assertEquals calls would pass even if the permissions inside the ACL entries were incorrect. Even putting aside tests, this is a public user-facing class, and callers likely would find it surprising if "user:bruce:rwx" and "user:bruce:---" were considered equal.

          With this reason only I have earlier included permissions also from command line for -x.
          But in this case, say permissions are not passed from the commandline, but the ACLEntries contain ACL for same user/group with some permissions. In this case, permissions will differ and objects also will differ.
          In general, there will be only one ACL entry per user/group in each type no matter what are the permissions. I agree that we cannot consider "user:bruce:rwx" and "user:bruce:---" as equal, but also both these entries cannot be present in list of ACL entries right?

          So my preference is that we need to check for permissions separately whenever necessary, instead of including in equals() and hashCode().
          What you say?

          Show
          Vinayakumar B added a comment - Hi, Vinay. This looks good, but I think we'll need to revert the AclEntry portion of the change. There are various unit tests that rely on assertEquals or assertArrayEquals to check that the correct ACL was applied to a file. With this change, those assertEquals calls would pass even if the permissions inside the ACL entries were incorrect. Even putting aside tests, this is a public user-facing class, and callers likely would find it surprising if "user:bruce:rwx" and "user:bruce:---" were considered equal. With this reason only I have earlier included permissions also from command line for -x. But in this case, say permissions are not passed from the commandline, but the ACLEntries contain ACL for same user/group with some permissions. In this case, permissions will differ and objects also will differ. In general, there will be only one ACL entry per user/group in each type no matter what are the permissions. I agree that we cannot consider "user:bruce:rwx" and "user:bruce:---" as equal, but also both these entries cannot be present in list of ACL entries right? So my preference is that we need to check for permissions separately whenever necessary, instead of including in equals() and hashCode(). What you say?
          Hide
          Chris Nauroth added a comment -

          In general, there will be only one ACL entry per user/group in each type no matter what are the permissions. I agree that we cannot consider "user:bruce:rwx" and "user:bruce:---" as equal, but also both these entries cannot be present in list of ACL entries right?

          This is correct. It doesn't make sense to have 2 different ACL entries with the same scope + type + name. We already enforce this on the NameNode side in AclTransformation#buildAndValidateAcl. The validation logic rejects duplicate ACL entries, where two entries are considered duplicate if they have the same scope + type + name (even if the permissions are different). If a user supplies duplicates like this, then we'll return an error. This is regardless of whether the request comes in through the CLI, another interface, or programmatically through FileSystem method calls. The enforcement is centralized on the server side.

          So my preference is that we need to check for permissions separately whenever necessary, instead of including in equals() and hashCode().

          Considering that the NameNode is enforcing the required uniqueness on the server side, let's not remove permissions from consideration in equals and hashCode. There are 2 reasons for this:

          1. As mentioned above, it's a public user-facing class, and most callers will have an intuition that equals and hashCode consider all data members.
          2. I'm also anticipating that we'll want to include perms when the NameNode starts de-duplicating ACLs via the Global ACL Set as described in the design doc (HDFS-5620). The implementation of this is likely to be sensitive to equals and hashCode, and we wouldn't want it to de-duplicate "user:bruce:---" based on seeing that "user:bruce:rwx" already exists in the live set. If we remove perms from the AclEntry implementation here, then the NameNode is likely going to have to translate everything to some kind of alternative internal class with an equals and hashCode that includes perms. This would require additional awkward data copying code.
          Show
          Chris Nauroth added a comment - In general, there will be only one ACL entry per user/group in each type no matter what are the permissions. I agree that we cannot consider "user:bruce:rwx" and "user:bruce:---" as equal, but also both these entries cannot be present in list of ACL entries right? This is correct. It doesn't make sense to have 2 different ACL entries with the same scope + type + name. We already enforce this on the NameNode side in AclTransformation#buildAndValidateAcl . The validation logic rejects duplicate ACL entries, where two entries are considered duplicate if they have the same scope + type + name (even if the permissions are different). If a user supplies duplicates like this, then we'll return an error. This is regardless of whether the request comes in through the CLI, another interface, or programmatically through FileSystem method calls. The enforcement is centralized on the server side. So my preference is that we need to check for permissions separately whenever necessary, instead of including in equals() and hashCode(). Considering that the NameNode is enforcing the required uniqueness on the server side, let's not remove permissions from consideration in equals and hashCode . There are 2 reasons for this: As mentioned above, it's a public user-facing class, and most callers will have an intuition that equals and hashCode consider all data members. I'm also anticipating that we'll want to include perms when the NameNode starts de-duplicating ACLs via the Global ACL Set as described in the design doc ( HDFS-5620 ). The implementation of this is likely to be sensitive to equals and hashCode , and we wouldn't want it to de-duplicate "user:bruce:---" based on seeing that "user:bruce:rwx" already exists in the live set. If we remove perms from the AclEntry implementation here, then the NameNode is likely going to have to translate everything to some kind of alternative internal class with an equals and hashCode that includes perms. This would require additional awkward data copying code.
          Hide
          Vinayakumar B added a comment -

          Ok. If the validation of the acl is already done for duplicate then no problem. Only thing I am concerned about is while implementation removal of acl entry proper ack entry should be found regardless of the permission.
          I will post a patch soon by removing the modifications to AclEntry

          Show
          Vinayakumar B added a comment - Ok. If the validation of the acl is already done for duplicate then no problem. Only thing I am concerned about is while implementation removal of acl entry proper ack entry should be found regardless of the permission. I will post a patch soon by removing the modifications to AclEntry
          Hide
          Chris Nauroth added a comment -

          Thanks, Vinay.

          Only thing I am concerned about is while implementation removal of acl entry proper ack entry should be found regardless of the permission.

          Yes, this is important too. This is also handled server-side in the NameNode. The relevant code is in AclTransformation#filterAclEntriesByAclSpec, which also uses a helper method AclTransformation#ValidatedAclSpec#containsKey. This method finds an entry in the ACL spec by a key consisting of scope + type + name (but not permission).

          Show
          Chris Nauroth added a comment - Thanks, Vinay. Only thing I am concerned about is while implementation removal of acl entry proper ack entry should be found regardless of the permission. Yes, this is important too. This is also handled server-side in the NameNode. The relevant code is in AclTransformation#filterAclEntriesByAclSpec , which also uses a helper method AclTransformation#ValidatedAclSpec#containsKey . This method finds an entry in the ACL spec by a key consisting of scope + type + name (but not permission).
          Hide
          Vinayakumar B added a comment -

          Reverted the changes in AclEntry equals() and hashCode().
          Sorry for the noise. : )
          Please review

          Show
          Vinayakumar B added a comment - Reverted the changes in AclEntry equals() and hashCode(). Sorry for the noise. : ) Please review
          Vinayakumar B made changes -
          Attachment HADOOP-10213.patch [ 12623388 ]
          Hide
          Chris Nauroth added a comment -

          No worries. The patch looks good. Thanks!

          I have one more question for you. Sachin Jose has provided a WebHDFS patch on HDFS-5608, and it needs the same logic for parsing an ACL spec from a query parameter. I'm thinking that we could refactor your AclCommands#SetfaclCommand#parseAclSpec method to a public static method on AclEntry, so that WebHDFS can reuse the parsing logic instead of duplicating it. It would look something like this:

            /**
             * Parses a string representation of an ACL spec into a list of AclEntry
             * objects.  Example: "user::rwx,user:foo:rw-,group::r--,other::---"
             *
             * @param aclSpec String ACL spec to parse
             * @return List<AclEntry> containing parsed ACL spec
             */
            public static List<AclEntry> parseAclSpec(String aclSpec) {
              ...
            }
          

          Do you think that makes sense? If so, would you want to do that refactoring as part of this patch?

          Show
          Chris Nauroth added a comment - No worries. The patch looks good. Thanks! I have one more question for you. Sachin Jose has provided a WebHDFS patch on HDFS-5608 , and it needs the same logic for parsing an ACL spec from a query parameter. I'm thinking that we could refactor your AclCommands#SetfaclCommand#parseAclSpec method to a public static method on AclEntry , so that WebHDFS can reuse the parsing logic instead of duplicating it. It would look something like this: /** * Parses a string representation of an ACL spec into a list of AclEntry * objects. Example: "user::rwx,user:foo:rw-,group::r--,other::---" * * @param aclSpec String ACL spec to parse * @ return List<AclEntry> containing parsed ACL spec */ public static List<AclEntry> parseAclSpec( String aclSpec) { ... } Do you think that makes sense? If so, would you want to do that refactoring as part of this patch?
          Hide
          Vinayakumar B added a comment -

          Attached the refactored patch to separate parse logic to static method in AclEntry.
          Please review

          Show
          Vinayakumar B added a comment - Attached the refactored patch to separate parse logic to static method in AclEntry. Please review
          Vinayakumar B made changes -
          Attachment HADOOP-10213.patch [ 12623586 ]
          Hide
          Chris Nauroth added a comment -

          The refactoring change looks good. Thanks!

          I've discovered a few other small problems while running a dev build of the HDFS-4685 branch plus some additional uncommitted patches. Can we address the following as part of this patch? I think the changes will be small.

          1. I noticed that setfacl wasn't actually doing anything. This is because it was popping the path argument from the list, and this caused the base classes to think there was no path to process. To fix this, please 1) remove the SetfaclCommand#path member variable, 2) in SetfaclCommand#processOptions remove the line that deleted from the args list and assigned to path, 3) in SetfaclCommand#processPath, you can use item.path instead of the old path member variable.
          2. I noticed that parseAclSpec was failing for comma-separated lists of multiple ACL entries. The reason for this is that within the for loop, it's calling aclSpec.split, which splits the whole ACL spec. Instead, it should be calling aclStr.split to split just the single ACL entry.
          3. Let's add a test to TestAclCommands for passing a comma-separated list to cover the above change.
          Show
          Chris Nauroth added a comment - The refactoring change looks good. Thanks! I've discovered a few other small problems while running a dev build of the HDFS-4685 branch plus some additional uncommitted patches. Can we address the following as part of this patch? I think the changes will be small. I noticed that setfacl wasn't actually doing anything. This is because it was popping the path argument from the list, and this caused the base classes to think there was no path to process. To fix this, please 1) remove the SetfaclCommand#path member variable, 2) in SetfaclCommand#processOptions remove the line that deleted from the args list and assigned to path , 3) in SetfaclCommand#processPath , you can use item.path instead of the old path member variable. I noticed that parseAclSpec was failing for comma-separated lists of multiple ACL entries. The reason for this is that within the for loop, it's calling aclSpec.split , which splits the whole ACL spec. Instead, it should be calling aclStr.split to split just the single ACL entry. Let's add a test to TestAclCommands for passing a comma-separated list to cover the above change.
          Hide
          Vinayakumar B added a comment -

          Thanks Chris for checking.
          Corrected all your comments and attached the patch. Please review

          Show
          Vinayakumar B added a comment - Thanks Chris for checking. Corrected all your comments and attached the patch. Please review
          Vinayakumar B made changes -
          Attachment HADOOP-10213.patch [ 12623786 ]
          Hide
          Chris Nauroth added a comment -

          Thanks for making those changes. I found one more thing. AclEntry#parseAclSpec is setting the name of the AclEntry to an empty String when the name is unspecified in the input string. This can cause validation failures on the server side when we try to check that the caller didn't specify a name for entries that do not permit a name. For example, the other entry can never have a name, so the NameNode will throw an exception back to the client if it tries to do something like setfacl -m other:vinay:rwx. I'm pasting below a small diff that fixes the problem. Could you please incorporate this into your patch and add another test in TestAclCommands to cover this? Thanks again!

          diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java b/hadoo
          index 6143ef8..7de6115 100644
          --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java
          +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java
          @@ -249,7 +249,10 @@ private AclEntry(AclEntryType type, String name, FsAction permission, AclEntrySc
                       "Invalid type of acl in <aclSpec> :" + aclStr);
                 }
           
          -      builder.setName(split[index++]);
          +      String name = split[index++];
          +      if (!name.isEmpty()) {
          +        builder.setName(name);
          +      }
           
                 if (expectedAclSpecLength == 3) {
                   String permission = split[index++];
          
          Show
          Chris Nauroth added a comment - Thanks for making those changes. I found one more thing. AclEntry#parseAclSpec is setting the name of the AclEntry to an empty String when the name is unspecified in the input string. This can cause validation failures on the server side when we try to check that the caller didn't specify a name for entries that do not permit a name. For example, the other entry can never have a name, so the NameNode will throw an exception back to the client if it tries to do something like setfacl -m other:vinay:rwx. I'm pasting below a small diff that fixes the problem. Could you please incorporate this into your patch and add another test in TestAclCommands to cover this? Thanks again! diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java b/hadoo index 6143ef8..7de6115 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/permission/AclEntry.java @@ -249,7 +249,10 @@ private AclEntry(AclEntryType type, String name, FsAction permission, AclEntrySc "Invalid type of acl in <aclSpec> :" + aclStr); } - builder.setName(split[index++]); + String name = split[index++]; + if (!name.isEmpty()) { + builder.setName(name); + } if (expectedAclSpecLength == 3) { String permission = split[index++];
          Hide
          Chris Nauroth added a comment -

          Updating summary to show expanded scope of the patch.

          Show
          Chris Nauroth added a comment - Updating summary to show expanded scope of the patch.
          Chris Nauroth made changes -
          Summary setfacl -x should reject attempts to include permissions in the ACL spec. Fix bugs parsing ACL spec in FsShell setfacl.
          Hide
          Vinayakumar B added a comment -

          Attaching the updated patch

          Show
          Vinayakumar B added a comment - Attaching the updated patch
          Vinayakumar B made changes -
          Attachment HADOOP-10213.patch [ 12623901 ]
          Hide
          Chris Nauroth added a comment -

          +1 for the patch. Thanks for fixing all of this, Vinay. I've committed it to the feature branch.

          Show
          Chris Nauroth added a comment - +1 for the patch. Thanks for fixing all of this, Vinay. I've committed it to the feature branch.
          Chris Nauroth made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Hadoop Flags Reviewed [ 10343 ]
          Fix Version/s HDFS ACLs (HDFS-4685) [ 12325672 ]
          Resolution Fixed [ 1 ]
          Hide
          Vinayakumar B added a comment -

          Thanks Chris for the detailed reviews and commit.

          Show
          Vinayakumar B added a comment - Thanks Chris for the detailed reviews and commit.
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          11d 18h 56m 1 Chris Nauroth 20/Jan/14 18:03

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development