Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-5739

ACL RPC must allow null name or unspecified permissions in ACL entries.

    Details

      Description

      Currently, the ACL RPC defines ACL entries with required fields for name and permissions. These fields actually need to be optional. The name can be null to represent unnamed ACL entries, such as the file owner or mask. Permissions can be null when passed in an ACL spec to remove ACL entries via FileSystem#removeAclEntries.

      1. HDFS-5739.2.patch
        5 kB
        Chris Nauroth
      2. HDFS-5739.1.patch
        4 kB
        Chris Nauroth

        Issue Links

          Activity

          Chris Nauroth created issue -
          Chris Nauroth made changes -
          Field Original Value New Value
          Link This issue relates to HDFS-4685 [ HDFS-4685 ]
          Chris Nauroth made changes -
          Link This issue is broken by HDFS-5596 [ HDFS-5596 ]
          Chris Nauroth made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Chris Nauroth added a comment -

          This patch switches the fields to optional in the protobuf spec, updates the translation logic in PBHelper and expands on the tests in TestPBHelper to cover these cases.

          Show
          Chris Nauroth added a comment - This patch switches the fields to optional in the protobuf spec, updates the translation logic in PBHelper and expands on the tests in TestPBHelper to cover these cases.
          Chris Nauroth made changes -
          Attachment HDFS-5739.1.patch [ 12622026 ]
          Hide
          Haohui Mai added a comment -

          The name parts looks good.

          Since

          {AclEntry#permissions}

          is a enum, from a semantic point of view I would prefer that it is non nullable. Is it possible to simply ignore the value in removeAclEntries?

          Show
          Haohui Mai added a comment - The name parts looks good. Since {AclEntry#permissions} is a enum, from a semantic point of view I would prefer that it is non nullable. Is it possible to simply ignore the value in removeAclEntries ?
          Hide
          Chris Nauroth added a comment -

          Thanks for the review, Haohui. I'm attaching patch version 2 to show what this looks like when we keep permissions required.

          Is it possible to simply ignore the value in removeAclEntries?

          Yes, the logic currently ignores it. If we wanted to strictly match existing implementations like Linux, then we would actually send an error back to the user if they tried to specify permissions in a remove call. I don't know that we need to be rigid about that, and we could always choose to implement that check at the CLI layer if we want it, so I'm fine with this approach.

          The effect of this is that protobuf will default initialize the enum field to the 0'th element (NONE) on conversion from proto to model. For symmetry, this patch adds the corresponding logic in the conversion from model to proto too.

          Show
          Chris Nauroth added a comment - Thanks for the review, Haohui. I'm attaching patch version 2 to show what this looks like when we keep permissions required. Is it possible to simply ignore the value in removeAclEntries? Yes, the logic currently ignores it. If we wanted to strictly match existing implementations like Linux, then we would actually send an error back to the user if they tried to specify permissions in a remove call. I don't know that we need to be rigid about that, and we could always choose to implement that check at the CLI layer if we want it, so I'm fine with this approach. The effect of this is that protobuf will default initialize the enum field to the 0'th element (NONE) on conversion from proto to model. For symmetry, this patch adds the corresponding logic in the conversion from model to proto too.
          Chris Nauroth made changes -
          Attachment HDFS-5739.2.patch [ 12622043 ]
          Hide
          Haohui Mai added a comment -

          I think that it is fine to check it at the CLI layer. +1 on the v2 patch.

          Show
          Haohui Mai added a comment - I think that it is fine to check it at the CLI layer. +1 on the v2 patch.
          Chris Nauroth made changes -
          Summary ACL RPC must allow null name or null permissions in ACL entries. ACL RPC must allow null name or unspecified permissions in ACL entries.
          Hide
          Chris Nauroth added a comment -

          I committed the v2 patch to the HDFS-4685 feature branch. Thanks again for the review, Haohui.

          Show
          Chris Nauroth added a comment - I committed the v2 patch to the HDFS-4685 feature branch. Thanks again for the review, Haohui.
          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