Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      UNIX-style sticky bit implemented for HDFS directories. When the sticky bit is set on a directory, files in that directory may be deleted or renamed only by a superuser or the file's owner.

      Description

      Our users (especially Pig) heavily use /tmp for temporary storage.
      Permission are set to 777.

      However, this means any users can rename and also remove (by moving to .Trash) other users directories and files.
      It would be nice if we can have a sticky bit like unix.

      Copy&Pasted from manpage.

      STICKY DIRECTORIES
      When the sticky bit is set on a directory, files in that directory may be unlinked or renamed only by
      root or their owner. Without the sticky bit, anyone able to write to the directory can delete or rename
      files. The sticky bit is commonly found on directories, such as /tmp, that are world-writable.

      1. HADOOP-3953.patch
        39 kB
        Jakob Homan
      2. HADOOP-3953.patch
        42 kB
        Jakob Homan
      3. HADOOP-3953.patch
        62 kB
        Jakob Homan
      4. HADOOP-3953.patch
        55 kB
        Jakob Homan

        Issue Links

          Activity

          Hide
          Hemanth Yamijala added a comment -

          +1. When working on HOD, it seemed like this would be very useful in some cases where we wanted shared space for all users.

          Show
          Hemanth Yamijala added a comment - +1. When working on HOD, it seemed like this would be very useful in some cases where we wanted shared space for all users.
          Hide
          Allen Wittenauer added a comment -

          We just had a user wipe out /tmp.

          When are we getting this capability?

          Show
          Allen Wittenauer added a comment - We just had a user wipe out /tmp. When are we getting this capability?
          Hide
          Jakob Homan added a comment -

          To keep Unix-like semantics, it seems reasonable to be able to set the sticky bit on a file (compared to a directory), but that doing so will have no effect on the file. Unless anyone has a good reason to deviate?

          Show
          Jakob Homan added a comment - To keep Unix-like semantics, it seems reasonable to be able to set the sticky bit on a file (compared to a directory), but that doing so will have no effect on the file. Unless anyone has a good reason to deviate?
          Hide
          Arun C Murthy added a comment -

          To keep Unix-like semantics, it seems reasonable to be able to set the sticky bit on a file (compared to a directory), but that doing so will have no effect on the file. Unless anyone has a good reason to deviate?

          +1 with the caveat that we heavily document the meaning of sticky bit for files.

          Show
          Arun C Murthy added a comment - To keep Unix-like semantics, it seems reasonable to be able to set the sticky bit on a file (compared to a directory), but that doing so will have no effect on the file. Unless anyone has a good reason to deviate? +1 with the caveat that we heavily document the meaning of sticky bit for files.
          Hide
          Jakob Homan added a comment -

          Here's an initial patch for review. This includes the sticky bit implementation and unit tests. I still need to go through the user documentation and update that; I'll do that now and update the patch shortly with just the documentation changes.

          Show
          Jakob Homan added a comment - Here's an initial patch for review. This includes the sticky bit implementation and unit tests. I still need to go through the user documentation and update that; I'll do that now and update the patch shortly with just the documentation changes.
          Hide
          Jakob Homan added a comment -

          Updated user docs and shell help. Ready for review.

          Show
          Jakob Homan added a comment - Updated user docs and shell help. Ready for review.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          patch looks good to me. Only one nit: FsPermission.set(FsAction u, FsAction g, FsAction o) is not useful anymore, please remove it.

          Show
          Tsz Wo Nicholas Sze added a comment - patch looks good to me. Only one nit: FsPermission.set(FsAction u, FsAction g, FsAction o) is not useful anymore, please remove it.
          Hide
          Jakob Homan added a comment -

          Updated with Nicholas' suggestions.

               [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 8 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 Eclipse classpath. The patch retains Eclipse classpath integrity.
          
          Show
          Jakob Homan added a comment - Updated with Nicholas' suggestions. [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 8 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 Eclipse classpath. The patch retains Eclipse classpath integrity.
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • The changes in FSDirectory should be reverted since the case is already taken care in INodeFile.
          • For the unit tests, leaving empty catch block is generally a bad practice since there might be some unexpected exceptions. For example, in the codes below, it is correct only if ioe is a sticky bit related exception. It is incorrect if some other IOException like FileNotFoundException is thrown.
            +      try {
            +        hdfs.rename(file, new Path(tmpPath2, "renamed"));
            +        fail("Shouldn't be able to rename someone else's file with SB on");
            +      } catch (IOException ioe) {
            +        // Correct
            +      }
            

            It would be great if you can combine some of them together, so that they take less execution time. I am fine if you don't want to change the tests although I strongly recommend to do so.

          Sorry for not seeing these problems in my previous review.

          Show
          Tsz Wo Nicholas Sze added a comment - The changes in FSDirectory should be reverted since the case is already taken care in INodeFile. For the unit tests, leaving empty catch block is generally a bad practice since there might be some unexpected exceptions. For example, in the codes below, it is correct only if ioe is a sticky bit related exception. It is incorrect if some other IOException like FileNotFoundException is thrown. + try { + hdfs.rename(file, new Path(tmpPath2, "renamed" )); + fail( "Shouldn't be able to rename someone else 's file with SB on" ); + } catch (IOException ioe) { + // Correct + } It would be great if you can combine some of them together, so that they take less execution time. I am fine if you don't want to change the tests although I strongly recommend to do so. Sorry for not seeing these problems in my previous review.
          Hide
          Jakob Homan added a comment -
          • Addressed Nicholas' comments
          • Refined chmod processing
          • Tightened up testing
                 [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 10 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 Eclipse classpath. The patch retains Eclipse classpath integrity.
            
          Show
          Jakob Homan added a comment - Addressed Nicholas' comments Refined chmod processing Tightened up testing [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 10 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 Eclipse classpath. The patch retains Eclipse classpath integrity.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Please post the local unit test results when they are ready.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good. Please post the local unit test results when they are ready.
          Hide
          Jakob Homan added a comment -

          All local unit tests passed except the known-failure documented in HADOOP-4907.

          Show
          Jakob Homan added a comment - All local unit tests passed except the known-failure documented in HADOOP-4907 .
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I just committed this. Thanks, Jakob!

          This needs release note. Please fill it out.

          Show
          Tsz Wo Nicholas Sze added a comment - I just committed this. Thanks, Jakob! This needs release note. Please fill it out.
          Hide
          Raghu Angadi added a comment -

          Jakob, release notes sounds much scarier than it should be. Just selecting an incompatible flag is enough, I think.

          Show
          Raghu Angadi added a comment - Jakob, release notes sounds much scarier than it should be. Just selecting an incompatible flag is enough, I think.
          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
          Benoy Antony added a comment -

          FilePermissionTests

          Show
          Benoy Antony added a comment - FilePermissionTests
          Hide
          Benoy Antony added a comment -

          Stickytests

          Show
          Benoy Antony added a comment - Stickytests

            People

            • Assignee:
              Jakob Homan
              Reporter:
              Koji Noguchi
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development