Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.17.2
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Fsck now checks permissions as directories are traversed. Any user can now use fsck, but information is provided only for directories the user has permission to read.

      Description

      Quoting from HADOOP-3222 ("fsck should require superuser privilege"),

      I agree that it makes sense to make fsck do permission checking for the nodes that it traverses. If a user does a fsck on files/directories that he/she has access to (using permissions) then that invocation of fsck should be allowed. Since "/" is usually owned by super-user, only super-user should be allowed to run fsck on "/".

      1. 4268_20081217.patch
        15 kB
        Tsz Wo Nicholas Sze
      2. 4268_20081218.patch
        18 kB
        Tsz Wo Nicholas Sze
      3. 4268_20081218b.patch
        26 kB
        Tsz Wo Nicholas Sze
      4. 4268_20081230.patch
        23 kB
        Tsz Wo Nicholas Sze
      5. HADOOP-4268-0_20.2.patch
        23 kB
        Jitendra Nath Pandey

        Issue Links

          Activity

          Koji Noguchi created issue -
          Koji Noguchi made changes -
          Field Original Value New Value
          Link This issue is related to HADOOP-3222 [ HADOOP-3222 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081217.patch: check permissions for fsck

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081217.patch: check permissions for fsck
          Tsz Wo Nicholas Sze made changes -
          Attachment 4268_20081217.patch [ 12396370 ]
          Tsz Wo Nicholas Sze made changes -
          Link This issue depends on HADOOP-4896 [ HADOOP-4896 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081218.patch: forgot to change DFSck in my previous patch. It temporarily includes the codes in HADOOP-4896. Tested this manually.

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081218.patch: forgot to change DFSck in my previous patch. It temporarily includes the codes in HADOOP-4896 . Tested this manually.
          Tsz Wo Nicholas Sze made changes -
          Attachment 4268_20081218.patch [ 12396438 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081218b.patch: added a test.

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081218b.patch: added a test.
          Tsz Wo Nicholas Sze made changes -
          Attachment 4268_20081218b.patch [ 12396443 ]
          Hide
          Konstantin Shvachko added a comment -
          1. NamenodeFsck constructor has new parameters, which can be obtained from the NameNode nn parameter during the construction. Why do you need the new parameters?
          2. I do not understand the reason for factoring out TestDFSck into a separate class especially if it is in a separate package.
            I would rather place both methods from TestDFSck back into TestFsck.
          3. I don't think introduction of a new package is justified in the case.
          4. In any case TestDFSck, runDFSck anything with DFS are bad names, we should use HDFS or nothing.
          5. testPermission() should have a Javadoc explaining what the test tests and how.
            So that one could see it from the description without going through the code.
          Show
          Konstantin Shvachko added a comment - NamenodeFsck constructor has new parameters, which can be obtained from the NameNode nn parameter during the construction. Why do you need the new parameters? I do not understand the reason for factoring out TestDFSck into a separate class especially if it is in a separate package. I would rather place both methods from TestDFSck back into TestFsck . I don't think introduction of a new package is justified in the case. In any case TestDFSck , runDFSck anything with DFS are bad names, we should use HDFS or nothing. testPermission() should have a Javadoc explaining what the test tests and how. So that one could see it from the description without going through the code.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          4268_20081230.patch: moved the new unit test to TestFsck.

          Show
          Tsz Wo Nicholas Sze added a comment - 4268_20081230.patch: moved the new unit test to TestFsck.
          Tsz Wo Nicholas Sze made changes -
          Attachment 4268_20081230.patch [ 12396928 ]
          Hide
          Konstantin Shvachko added a comment -

          +1. This looks good to me.

          Show
          Konstantin Shvachko added a comment - +1. This looks good to me.
          Hide
          Tsz Wo Nicholas Sze added a comment -
               [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 6 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
          Tsz Wo Nicholas Sze added a comment - [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 6 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 made changes -
          Fix Version/s 0.21.0 [ 12313563 ]
          Release Note Add permission checking on fsck. Before the changes, fsck invokes NameNode internal methods directly. So that any user can run fsck on any path, even for the path they do not have permission to access the files. After the changes, fsck invokes the ClientProtocol methods. Then the corresponding permission requirement for running the ClientProtocol methods will be enforced.
          Status Open [ 1 ] Patch Available [ 10002 ]
          Assignee Tsz Wo (Nicholas), SZE [ szetszwo ]
          Hadoop Flags [Incompatible change, Reviewed]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          It passed all tests locally, except for a un-related test TestMapReduceLocal. See HADOOP-4907.

          Show
          Tsz Wo Nicholas Sze added a comment - It passed all tests locally, except for a un-related test TestMapReduceLocal. See HADOOP-4907 .
          Hide
          Tsz Wo Nicholas Sze added a comment -

          I just committed this.

          Show
          Tsz Wo Nicholas Sze added a comment - I just committed this.
          Tsz Wo Nicholas Sze made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed, Incompatible change] [Incompatible change, Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #708 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/708/)
          . Change fsck to use ClientProtocol methods for enforcing permissions. (szetszwo)

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #708 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/708/ ) . Change fsck to use ClientProtocol methods for enforcing permissions. (szetszwo)
          Owen O'Malley made changes -
          Component/s dfs [ 12310710 ]
          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.
          Robert Chansler made changes -
          Release Note Add permission checking on fsck. Before the changes, fsck invokes NameNode internal methods directly. So that any user can run fsck on any path, even for the path they do not have permission to access the files. After the changes, fsck invokes the ClientProtocol methods. Then the corresponding permission requirement for running the ClientProtocol methods will be enforced. Fsck now checks permissions as directories are traversed. Any user can now use fsck, but information is provided only for directories the user has permission to read.
          Hide
          Jitendra Nath Pandey added a comment -

          Patch for Hadoop 20 is added.

          Show
          Jitendra Nath Pandey added a comment - Patch for Hadoop 20 is added.
          Jitendra Nath Pandey made changes -
          Attachment HADOOP-4268-0_20.2.patch [ 12428975 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          HADOOP-4268-0_20.2.patch looks good to me. Thanks, Jitendra!

          Show
          Tsz Wo Nicholas Sze added a comment - HADOOP-4268 -0_20.2.patch looks good to me. Thanks, Jitendra!
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Forgot to say: +1 on HADOOP-4268-0_20.2.patch.

          Show
          Tsz Wo Nicholas Sze added a comment - Forgot to say: +1 on HADOOP-4268 -0_20.2.patch.
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Gavin made changes -
          Link This issue depends on HADOOP-4896 [ HADOOP-4896 ]
          Gavin made changes -
          Link This issue depends upon HADOOP-4896 [ HADOOP-4896 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          102d 23h 35m 1 Tsz Wo Nicholas Sze 05/Jan/09 22:19
          Patch Available Patch Available Resolved Resolved
          3h 21m 1 Tsz Wo Nicholas Sze 06/Jan/09 01:40
          Resolved Resolved Closed Closed
          595d 18h 53m 1 Tom White 24/Aug/10 21:34

            People

            • Assignee:
              Tsz Wo Nicholas Sze
              Reporter:
              Koji Noguchi
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development