Sticky bit for directories

• Key: HADOOP-3953
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Jakob Homan
• Reporter: Koji Noguchi
• Fix Version/s: 0.21.0

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.

Comments

[Hemanth Yamijala]

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

[Allen Wittenauer]

We just had a user wipe out /tmp.

When are we getting this capability?

[Jakob Homan]

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?

[Arun C Murthy]

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.

[Jakob Homan]

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.

[Jakob Homan]

Updated user docs and shell help. Ready for review.

[Tsz Wo (Nicholas), SZE]

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

[Jakob Homan]

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.

[Tsz Wo (Nicholas), SZE]

• 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.

[Jakob Homan]

• 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.
  

[Tsz Wo (Nicholas), SZE]

+1 patch looks good.

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

[Jakob Homan]

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

[Tsz Wo (Nicholas), SZE]

I just committed this. Thanks, Jakob!

This needs release note. Please fill it out.

[Raghu Angadi]

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

[Robert Chansler]

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