Hadoop Common
  1. Hadoop Common
  2. HADOOP-10277

setfacl -x fails to parse ACL spec if trying to remove the mask entry.

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

      Description

      You should be able to use setfacl -x to remove the mask entry (if also removing all other extended ACL entries). Right now, this causes a failure to parse the ACL spec due to a bug in AclEntry#parseAclSpec.

      1. HADOOP-10277.patch
        8 kB
        Vinayakumar B
      2. HADOOP-10277.1.patch
        5 kB
        Chris Nauroth

        Issue Links

          Activity

          Hide
          Chris Nauroth added a comment -

          Thank you to Sachin Jose and R J for reporting this bug.

          Show
          Chris Nauroth added a comment - Thank you to Sachin Jose and R J for reporting this bug.
          Hide
          Vinayakumar B added a comment -

          Hi Chris,
          I got the issue.
          Issue was String.split() will not add empty trailing strings to parsed array.
          This can be fixed.

          But my doubt is, whether removing the mask is allowed ?

          My setfacl throws error when I try to remove mask entry in my suse linux box

          vinay@host-10-18-40-99:~> setfacl -x mask:: testAcl/
          setfacl: testAcl/: Malformed access ACL `user::rwx,user:vinay:r--,group::r-x,group:users:r-x,other::r-x': Missing or wrong entry at entry 5
          vinay@host-10-18-40-99:~> setfacl -x mask testAcl/
          setfacl: testAcl/: Malformed access ACL `user::rwx,user:vinay:r--,group::r-x,group:users:r-x,other::r-x': Missing or wrong entry at entry 5

          Please validate whether this is correct behaviour or whether we need to support removal of mask entries

          Show
          Vinayakumar B added a comment - Hi Chris, I got the issue. Issue was String.split() will not add empty trailing strings to parsed array. This can be fixed. But my doubt is, whether removing the mask is allowed ? My setfacl throws error when I try to remove mask entry in my suse linux box vinay@host-10-18-40-99:~> setfacl -x mask:: testAcl/ setfacl: testAcl/: Malformed access ACL `user::rwx,user:vinay:r--,group::r-x,group:users:r-x,other::r-x': Missing or wrong entry at entry 5 vinay@host-10-18-40-99:~> setfacl -x mask testAcl/ setfacl: testAcl/: Malformed access ACL `user::rwx,user:vinay:r--,group::r-x,group:users:r-x,other::r-x': Missing or wrong entry at entry 5 Please validate whether this is correct behaviour or whether we need to support removal of mask entries
          Hide
          Chris Nauroth added a comment -

          Thanks for volunteering to take this, Vinayakumar B. I did have a patch in progress, so I'll just attach the work I've done so far and ask you if you want to do anything more with this.

          As part of this patch, I also wanted to refactor AclEntry#parseAclSpec so that the logic of parsing a single entry is in a separate public static method: AclEntry#parseAclEntry. That's going to be helpful for the HDFS-5608 WebHDFS patch. I didn't get the refactoring done yet in this version of the patch.

          But my doubt is, whether removing the mask is allowed ?

          Actually, you are correct. I was incorrect when I said earlier that you can remove the mask to trigger automatic recalculation. It is invalid to attempt to remove the mask from an ACL that needs it, however, the error should get reported from the server side instead of a failure to parse the ACL spec. We already have the code to do this in the NameNode in AclTransformation#buildAndValidateAcl. The parsing still needs to be able to parse a mask entry with no permission, because it is valid to remove the mask entry if you're also removing all other extended ACL entries (reducing it from an extended ACL back to a minimal ACL). See below for an example on Linux.

          [cnauroth@ubuntu:pts/0] acltest                                                                                     
          > getfacl file1
          # file: file1
          # owner: cnauroth
          # group: cnauroth
          user::rw-
          user:bruce:rwx			#effective:r--
          group::rw-			#effective:r--
          mask::r--
          other::r--
          
          [cnauroth@ubuntu:pts/0] acltest                                                                                     
          > setfacl -x mask:: file1
          setfacl: file1: Malformed access ACL `user::rw-,user:bruce:rwx,group::rw-,other::r--': Missing or wrong entry at entry 4
          
          [cnauroth@ubuntu:pts/0] acltest                                                                                     
          > setfacl -x mask::,user:bruce: file1
          
          [cnauroth@ubuntu:pts/0] acltest                                                                                     
          > getfacl file1
          # file: file1
          # owner: cnauroth
          # group: cnauroth
          user::rw-
          group::rw-
          other::r--
          
          Show
          Chris Nauroth added a comment - Thanks for volunteering to take this, Vinayakumar B . I did have a patch in progress, so I'll just attach the work I've done so far and ask you if you want to do anything more with this. As part of this patch, I also wanted to refactor AclEntry#parseAclSpec so that the logic of parsing a single entry is in a separate public static method: AclEntry#parseAclEntry . That's going to be helpful for the HDFS-5608 WebHDFS patch. I didn't get the refactoring done yet in this version of the patch. But my doubt is, whether removing the mask is allowed ? Actually, you are correct. I was incorrect when I said earlier that you can remove the mask to trigger automatic recalculation. It is invalid to attempt to remove the mask from an ACL that needs it, however, the error should get reported from the server side instead of a failure to parse the ACL spec. We already have the code to do this in the NameNode in AclTransformation#buildAndValidateAcl . The parsing still needs to be able to parse a mask entry with no permission, because it is valid to remove the mask entry if you're also removing all other extended ACL entries (reducing it from an extended ACL back to a minimal ACL). See below for an example on Linux. [cnauroth@ubuntu:pts/0] acltest > getfacl file1 # file: file1 # owner: cnauroth # group: cnauroth user::rw- user:bruce:rwx #effective:r-- group::rw- #effective:r-- mask::r-- other::r-- [cnauroth@ubuntu:pts/0] acltest > setfacl -x mask:: file1 setfacl: file1: Malformed access ACL `user::rw-,user:bruce:rwx,group::rw-,other::r--': Missing or wrong entry at entry 4 [cnauroth@ubuntu:pts/0] acltest > setfacl -x mask::,user:bruce: file1 [cnauroth@ubuntu:pts/0] acltest > getfacl file1 # file: file1 # owner: cnauroth # group: cnauroth user::rw- group::rw- other::r--
          Hide
          Vinayakumar B added a comment -

          Thanks Chris for clearing the doubt. I will post a patch soon for refactoring as well as the fix.

          Show
          Vinayakumar B added a comment - Thanks Chris for clearing the doubt. I will post a patch soon for refactoring as well as the fix.
          Hide
          Vinayakumar B added a comment -

          Hi Chris, Attaching the refractored patch.
          Its almost same as your initial work.
          Please review.

          Show
          Vinayakumar B added a comment - Hi Chris, Attaching the refractored patch. Its almost same as your initial work. Please review.
          Hide
          Chris Nauroth added a comment -

          +1. I committed this to the feature branch. Vinay, thank you for the patch.

          Show
          Chris Nauroth added a comment - +1. I committed this to the feature branch. Vinay, thank you for the patch.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development