Hadoop Common
  1. Hadoop Common
  2. HADOOP-6234

Permission configuration files should use octal and symbolic

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      New configuration option dfs.umaskmode sets umask with octal or symbolic value.

      Description

      Currently, the settings for the default umask in Hadoop configuration files require the input format be in decimal. Considering that every admin in the world thinks of permissions in octal and/or symbolic format, the config files should really use those two formats and drop decimal.

      [... and, yes, I'm aware this breaks backwards compatibility. But in this case, I think that is perfectly acceptable.]

      1. COMMON-126.patch
        20 kB
        Jakob Homan
      2. COMMON-6234.patch
        23 kB
        Jakob Homan
      3. COMMON-6234.rel20.patch
        22 kB
        Suresh Srinivas
      4. COMMON-6234.rel20.patch
        22 kB
        Suresh Srinivas
      5. COMMON-6234.rel20.1.patch
        21 kB
        Suresh Srinivas

        Issue Links

          Activity

          Hide
          Jakob Homan added a comment -

          updated patch looks good. +1.

          Show
          Jakob Homan added a comment - updated patch looks good. +1.
          Hide
          Suresh Srinivas added a comment -

          For 0.20 patch, the problem described in HDFS-760 is not relevant. On trunk we use a different backward compatibility mechanism to map deprecated configuration key to new configuration key, using DeprecatedKeyInfo. That patch has its own mechanism to provide backward compatibility.

          Show
          Suresh Srinivas added a comment - For 0.20 patch, the problem described in HDFS-760 is not relevant. On trunk we use a different backward compatibility mechanism to map deprecated configuration key to new configuration key, using DeprecatedKeyInfo. That patch has its own mechanism to provide backward compatibility.
          Hide
          Suresh Srinivas added a comment -

          New patch that addresses Jakob's comments.

          Show
          Suresh Srinivas added a comment - New patch that addresses Jakob's comments.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          For the 0.20 patch, we should test whether it has the problem described in HDFS-760.

          Show
          Tsz Wo Nicholas Sze added a comment - For the 0.20 patch, we should test whether it has the problem described in HDFS-760 .
          Hide
          Jakob Homan added a comment -

          Patch for 20 looks good. Essentially it is octal/symbolic umask settings in a universe where sticky bits don't exist.

          Comments:

          • Line 379 - remove reference to t in comment
          • Line 526 - Add back reference to X being prohibited
          • Line 552 - Extraneous import of FSNamesystem

          These are minor and otherwise +1.

          Show
          Jakob Homan added a comment - Patch for 20 looks good. Essentially it is octal/symbolic umask settings in a universe where sticky bits don't exist. Comments: Line 379 - remove reference to t in comment Line 526 - Add back reference to X being prohibited Line 552 - Extraneous import of FSNamesystem These are minor and otherwise +1.
          Hide
          Suresh Srinivas added a comment -

          attaching a patch for branch 0.20. This patch has some differences from the 0.21 patch. The changes related to sticky bits had to be removed since 0.20 does not have sticky bit feature.

          Show
          Suresh Srinivas added a comment - attaching a patch for branch 0.20. This patch has some differences from the 0.21 patch. The changes related to sticky bits had to be removed since 0.20 does not have sticky bit feature.
          Hide
          Robert Chansler added a comment -

          Editorial pass over all release notes prior to publication of 0.21.

          Show
          Robert Chansler added a comment - Editorial pass over all release notes prior to publication of 0.21.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #21 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/21/)

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h5.grid.sp2.yahoo.net #21 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h5.grid.sp2.yahoo.net/21/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #90 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/90/)
          . Add new option dfs.umaskmode to set umask in configuration to use octal or symbolic instead of decimal. Contributed by Jakob Homan.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #90 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/90/ ) . Add new option dfs.umaskmode to set umask in configuration to use octal or symbolic instead of decimal. Contributed by Jakob Homan.
          Hide
          Hudson added a comment -

          Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #5 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/5/)
          . Updated hadoop-core and test jars to propagate new option dfs.umaskmode in configuration. Contributed by Jakob Homan.

          Show
          Hudson added a comment - Integrated in Hdfs-Patch-h2.grid.sp2.yahoo.net #5 (See http://hudson.zones.apache.org/hudson/job/Hdfs-Patch-h2.grid.sp2.yahoo.net/5/ ) . Updated hadoop-core and test jars to propagate new option dfs.umaskmode in configuration. Contributed by Jakob Homan.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #23 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/23/)
          . Updated hadoop-core and test jars to propagate new option dfs.umaskmode in configuration. Contributed by Jakob Homan.

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #23 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/23/ ) . Updated hadoop-core and test jars to propagate new option dfs.umaskmode in configuration. Contributed by Jakob Homan.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #22 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/22/)
          . Add new option dfs.umaskmode to set umask in configuration to use octal or symbolic instead of decimal. Contributed by Jakob Homan.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #22 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/22/ ) . Add new option dfs.umaskmode to set umask in configuration to use octal or symbolic instead of decimal. Contributed by Jakob Homan.
          Hide
          Suresh Srinivas added a comment -

          Committed the change. Thanks Jakob.

          Show
          Suresh Srinivas added a comment - Committed the change. Thanks Jakob.
          Hide
          Kan Zhang added a comment -

          +1 for the patch.

          Show
          Kan Zhang added a comment - +1 for the patch.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12418574/COMMON-6234.patch
          against trunk revision 810756.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12418574/COMMON-6234.patch against trunk revision 810756. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/17/console This message is automatically generated.
          Hide
          Jakob Homan added a comment -

          submitting patch, although already ran unit tests and test-patch.

          Show
          Jakob Homan added a comment - submitting patch, although already ran unit tests and test-patch.
          Hide
          Jakob Homan added a comment -

          Finished patch. Rather than wait for 6105, handcoded check for prior value. When 6105 is committed, will update to use its functionality.

          Compared to previous patch, provided backwards compatibility, updated documentation and split one test into three.

          Of note, HDFS needs to be updated due to this patch and will open a new JIRA for that now.

          Passes all unit tests.

          Test-patch:

          [exec] +1 overall.  
          [exec] 
          [exec]     +1 @author.  The patch does not contain any @author tags.
          [exec] 
          [exec]     +1 tests included.  The patch appears to include 3 new or modified tests.
          [exec] 
          [exec]     +1 javadoc.  The javadoc tool did not generate any warning messages.
          [exec] 
          [exec]     +1 javac.  The applied patch does not increase the total number of javac compiler warnings.
          [exec] 
          [exec]     +1 findbugs.  The patch does not introduce any new Findbugs warnings.
          [exec] 
          [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
          
          Show
          Jakob Homan added a comment - Finished patch. Rather than wait for 6105, handcoded check for prior value. When 6105 is committed, will update to use its functionality. Compared to previous patch, provided backwards compatibility, updated documentation and split one test into three. Of note, HDFS needs to be updated due to this patch and will open a new JIRA for that now. Passes all unit tests. Test-patch: [exec] +1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] +1 tests included. The patch appears to include 3 new or modified tests. [exec] [exec] +1 javadoc. The javadoc tool did not generate any warning messages. [exec] [exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings. [exec] [exec] +1 findbugs. The patch does not introduce any new Findbugs warnings. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
          Hide
          Jakob Homan added a comment -

          Moved to Common, where the affected files actually reside

          Show
          Jakob Homan added a comment - Moved to Common, where the affected files actually reside
          Hide
          Jakob Homan added a comment -

          Done with most of the work. I very much would like to use HADOOP-6105, but it's not yet committed. I'll be out of town until next Wednesday, so attaching prelim patch for review and hopefully 6105 will be ready to go when I get back.

          Patch:

          • Creates a new config option, dfs.umaskmode, which will take either an octal or symbolic umask value.
          • Separates out the permission parsing from the shell code. This allows us to use the exact same code for parsing in both places.
          • Does not change external view of any of the permission processing
          • Creates a new PermissionParser and a package-private UmaskParser to handle the subtle differences between what regular permissions and umask permissions allow
          • Creates new unit tests to verify we're getting the right value from the conf file. Most of the parsing code testing is free, since we're now using common code in all the parsing.

          Still need to:

          • Use 6105 to handle old key
          • Test against hdfs
          Show
          Jakob Homan added a comment - Done with most of the work. I very much would like to use HADOOP-6105 , but it's not yet committed. I'll be out of town until next Wednesday, so attaching prelim patch for review and hopefully 6105 will be ready to go when I get back. Patch: Creates a new config option, dfs.umaskmode, which will take either an octal or symbolic umask value. Separates out the permission parsing from the shell code. This allows us to use the exact same code for parsing in both places. Does not change external view of any of the permission processing Creates a new PermissionParser and a package-private UmaskParser to handle the subtle differences between what regular permissions and umask permissions allow Creates new unit tests to verify we're getting the right value from the conf file. Most of the parsing code testing is free, since we're now using common code in all the parsing. Still need to: Use 6105 to handle old key Test against hdfs
          Hide
          Allen Wittenauer added a comment -

          this is one of those things where i think it is acceptable to break existing users, given proper release notes and enough warning. really. if we are willing to break api's then breaking something as relatively minor as permissions in the conf file does not seem like that big of deal.

          perhaps this will be the jira that finally provides 'upgrade' notes instead of just 'release' notes, like most real world operating systems.

          Show
          Allen Wittenauer added a comment - this is one of those things where i think it is acceptable to break existing users, given proper release notes and enough warning. really. if we are willing to break api's then breaking something as relatively minor as permissions in the conf file does not seem like that big of deal. perhaps this will be the jira that finally provides 'upgrade' notes instead of just 'release' notes, like most real world operating systems.
          Hide
          Jakob Homan added a comment -

          I'd guess the massive confusion at this point would be that we're using decimal rather than octal, but yeah, changing at this point cold-turkey would probably just confuse a our established users rather than our new users. My concern over using a lead 0 is that li/unix only uses octal for chmod, whether or not a leading 0 is specified. I'd like to end up with something that matches that behavior.
          In that case, the best thing is probably to deprecate the current key and introduce a new one with octal/symbolic semantics. That's the approach I'll plan on using. Too bad HADOOP-6105 isn't up and runnning yet.

          Show
          Jakob Homan added a comment - I'd guess the massive confusion at this point would be that we're using decimal rather than octal, but yeah, changing at this point cold-turkey would probably just confuse a our established users rather than our new users. My concern over using a lead 0 is that li/unix only uses octal for chmod, whether or not a leading 0 is specified. I'd like to end up with something that matches that behavior. In that case, the best thing is probably to deprecate the current key and introduce a new one with octal/symbolic semantics. That's the approach I'll plan on using. Too bad HADOOP-6105 isn't up and runnning yet.
          Hide
          Owen O'Malley added a comment -

          No, it isn't acceptable to break backwards compatability.

          Symbolic is ok, since it isn't ambiguous. To use straight octal we either need a flag like a leading o or use a new attribute and handle the old attribute in decimal.

          My other concern in this area is against supporting octal via a leading 0 on all integer attributes. That will lead to massive confusion, in my opinion.

          Show
          Owen O'Malley added a comment - No, it isn't acceptable to break backwards compatability. Symbolic is ok, since it isn't ambiguous. To use straight octal we either need a flag like a leading o or use a new attribute and handle the old attribute in decimal. My other concern in this area is against supporting octal via a leading 0 on all integer attributes. That will lead to massive confusion, in my opinion.

            People

            • Assignee:
              Jakob Homan
              Reporter:
              Allen Wittenauer
            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development