Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: security, test
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Fixed JobTracker to use it's own credentials instead of the job's credentials for accessing mapred.system.dir. Also added APIs in the JobTracker to get the FileSystem objects as per the JobTracker's configuration.

      Description

      While running TestMiniMRWithDFSWithDistinctUsers, I used this patch to test the ugi checks

      Index: src/hdfs/org/apache/hadoop/hdfs/server/namenode/PermissionChecker.java
      ===================================================================
      --- src/hdfs/org/apache/hadoop/hdfs/server/namenode/PermissionChecker.java	(revision 768189)
      +++ src/hdfs/org/apache/hadoop/hdfs/server/namenode/PermissionChecker.java	(working copy)
      @@ -40,6 +40,7 @@
           if (LOG.isDebugEnabled()) {
             LOG.debug("ugi=" + ugi);
           }
      +    LOG.info("ugi=" + ugi);
       
           if (ugi != null) {
             user = ugi.getUserName();
      

      While initializing a job, the ugi information should point to jobtracker as jobtracker does a dfs read. But today we will see that the log shows pi as the caller instead of the jobtracker.

      1. HADOOP-5737-v1.3.patch
        4 kB
        Amar Kamat
      2. HADOOP-5737-v1.5.patch
        10 kB
        Amar Kamat
      3. HADOOP-5737-v1.7.patch
        19 kB
        Amar Kamat
      4. HADOOP-5737-y20.patch
        10 kB
        Hemanth Yamijala

        Issue Links

          Activity

          Hide
          Arun C Murthy added a comment -

          Does this anomaly happen in all test-cases? Or only in TestMiniMRWithDFSWithDistinctUsers?

          Show
          Arun C Murthy added a comment - Does this anomaly happen in all test-cases? Or only in TestMiniMRWithDFSWithDistinctUsers?
          Hide
          Arun C Murthy added a comment -

          Also, another thing to check is whether it happens only in when using the Mini* clusters, and not in single-node cluster etc.

          Show
          Arun C Murthy added a comment - Also, another thing to check is whether it happens only in when using the Mini* clusters, and not in single-node cluster etc.
          Hide
          Amar Kamat added a comment -

          Does this anomaly happen in all test-cases? Or only in TestMiniMRWithDFSWithDistinctUsers?

          This happens whenever the users for jobtracker and job are different. For example, the jobtracker runs as foo and a user bar submits a job. So what happens in these testcase is that

          1. user bar submits a job
          2. jobtracker (running as foo) tries to do a job.init()
          3. fs does read-permission check and uses ugi = UserGroupInformation.getCurrentUGI() which points to user bar.

          Ideally the ugi should point to jobtracker's user.

          Also, another thing to check is whether it happens only in when using the Mini* clusters, and not in single-node cluster etc.

          Arun this is a bug with the testcase only. I will still try it out on a single node cluster.

          Show
          Amar Kamat added a comment - Does this anomaly happen in all test-cases? Or only in TestMiniMRWithDFSWithDistinctUsers? This happens whenever the users for jobtracker and job are different. For example, the jobtracker runs as foo and a user bar submits a job. So what happens in these testcase is that user bar submits a job jobtracker (running as foo) tries to do a job.init() fs does read-permission check and uses ugi = UserGroupInformation.getCurrentUGI() which points to user bar. Ideally the ugi should point to jobtracker's user. Also, another thing to check is whether it happens only in when using the Mini* clusters, and not in single-node cluster etc. Arun this is a bug with the testcase only. I will still try it out on a single node cluster.
          Hide
          Amar Kamat added a comment -

          Attaching a patch that uses jobtracker's credentials for contacting the namenode instead of using job's credentials. This patch solves the issue. Result of test-patch

          [exec] -1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     -1 tests included.  The patch doesn't appear to include any new or modified tests.
               [exec]                         Please justify why no tests are needed for this patch.
               [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.
               [exec] 
               [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
          

          Running ant test now.

          Show
          Amar Kamat added a comment - Attaching a patch that uses jobtracker's credentials for contacting the namenode instead of using job's credentials. This patch solves the issue. Result of test-patch [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no tests are needed for this patch. [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. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Running ant test now.
          Hide
          Amar Kamat added a comment -

          Ant tests passed on my box.

          Show
          Amar Kamat added a comment - Ant tests passed on my box.
          Hide
          Arun C Murthy added a comment -

          The patch looks fine. I'd rather have a JobTracker.getFileSystem() and JobTracker.getFileSystem(Path) than spread the code all over... please document the rationale for these apis too for posterity.

          Show
          Arun C Murthy added a comment - The patch looks fine. I'd rather have a JobTracker.getFileSystem() and JobTracker.getFileSystem(Path) than spread the code all over... please document the rationale for these apis too for posterity.
          Hide
          Amar Kamat added a comment -

          Hey Arun thanks for looking. JobInProgress requires access to JobTracker's conf for

          1. Making connections to namenode
          2. Passing it to other classes like CleanupQueue etc.

          For case #1, JobTracker.getFileSystem() makes sense but for case #2, its better to pass the conf. Hence the getConf() api.

          please document the rationale for these apis too for posterity.

          If we keep the getConf() api, do we need any documentation for this?

          Show
          Amar Kamat added a comment - Hey Arun thanks for looking. JobInProgress requires access to JobTracker's conf for Making connections to namenode Passing it to other classes like CleanupQueue etc. For case #1, JobTracker.getFileSystem() makes sense but for case #2, its better to pass the conf. Hence the getConf() api. please document the rationale for these apis too for posterity. If we keep the getConf() api, do we need any documentation for this?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > While running TestMiniMRWithDFSWithDistinctUsers, I used this patch to test the ugi checks ...
          You may set the LOG to debug level instead of changing the code.

          Show
          Tsz Wo Nicholas Sze added a comment - > While running TestMiniMRWithDFSWithDistinctUsers , I used this patch to test the ugi checks ... You may set the LOG to debug level instead of changing the code.
          Hide
          Arun C Murthy added a comment -

          For case #1, JobTracker.getFileSystem() makes sense but for case #2, its better to pass the conf.

          How is it better to pass the conf? JobTracker.getFileSystem(path) would take the path, resolve it against it's own conf and return the FileSystem - that way it's more explicit that you need the JobTracker's view of the filesytem. getConf() is too generic and it's too easy to forget to call getConf() in the future etc.

          Show
          Arun C Murthy added a comment - For case #1, JobTracker.getFileSystem() makes sense but for case #2, its better to pass the conf. How is it better to pass the conf? JobTracker.getFileSystem(path) would take the path, resolve it against it's own conf and return the FileSystem - that way it's more explicit that you need the JobTracker's view of the filesytem. getConf() is too generic and it's too easy to forget to call getConf() in the future etc.
          Hide
          Amar Kamat added a comment -

          Attaching a patch the reduces code duplication. I think its ok to pass open up jobtracker conf as only jip requires it now. JobHistory already has it. To pass the jobtracker's fs to JobHistory, we need to pass the jobtracker handle to JobHistory. Result of test-patch

           [exec] -1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     -1 tests included.  The patch doesn't appear to include any new or modified tests.
               [exec]                         Please justify why no tests are needed for this patch.
               [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.
               [exec] 
               [exec]     +1 release audit.  The applied patch does not increase the total number of release audit warnings.
          

          Ant tests passed on my box.

          Show
          Amar Kamat added a comment - Attaching a patch the reduces code duplication. I think its ok to pass open up jobtracker conf as only jip requires it now. JobHistory already has it. To pass the jobtracker's fs to JobHistory, we need to pass the jobtracker handle to JobHistory. Result of test-patch [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no tests are needed for this patch. [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. [exec] [exec] +1 release audit. The applied patch does not increase the total number of release audit warnings. Ant tests passed on my box.
          Hide
          Devaraj Das added a comment -

          I just looked at the patch and it seems okay to me. Passing the JobTracker object to JobHistory seems less preferable when things can work without that change.

          Show
          Devaraj Das added a comment - I just looked at the patch and it seems okay to me. Passing the JobTracker object to JobHistory seems less preferable when things can work without that change.
          Hide
          Amar Kamat added a comment -

          Attaching a patch incorporating Arun's comment. Result of test-patch

          [exec] -1 overall.  
               [exec] 
               [exec]     +1 @author.  The patch does not contain any @author tags.
               [exec] 
               [exec]     -1 tests included.  The patch doesn't appear to include any new or modified tests.
               [exec]                         Please justify why no tests are needed for this patch.
               [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.
               [exec] 
               [exec]     -1 release audit.  The applied patch generated 484 release audit warnings (more than the trunk's current 483 warnings).
          

          No extra files are added. Release audit warning is because of change to a public api (JobHistory.init()).
          Running ant test now.

          Show
          Amar Kamat added a comment - Attaching a patch incorporating Arun's comment. Result of test-patch [exec] -1 overall. [exec] [exec] +1 @author. The patch does not contain any @author tags. [exec] [exec] -1 tests included. The patch doesn't appear to include any new or modified tests. [exec] Please justify why no tests are needed for this patch. [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. [exec] [exec] -1 release audit. The applied patch generated 484 release audit warnings (more than the trunk's current 483 warnings). No extra files are added. Release audit warning is because of change to a public api (JobHistory.init()). Running ant test now.
          Hide
          Amar Kamat added a comment -

          Ant tests passed on my box.

          Show
          Amar Kamat added a comment - Ant tests passed on my box.
          Hide
          Arun C Murthy added a comment -

          +1

          Show
          Arun C Murthy added a comment - +1
          Hide
          Devaraj Das added a comment -

          I just committed this. Thanks, Amar!

          Show
          Devaraj Das added a comment - I just committed this. Thanks, Amar!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-trunk #834 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/834/)
          . Fixes a problem in the way the JobTracker used to talk to other daemons like the NameNode to get the job's files. Also adds APIs in the JobTracker to get the FileSystem objects as per the JobTracker's configuration. Contributed by Amar Kamat.

          Show
          Hudson added a comment - Integrated in Hadoop-trunk #834 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/834/ ) . Fixes a problem in the way the JobTracker used to talk to other daemons like the NameNode to get the job's files. Also adds APIs in the JobTracker to get the FileSystem objects as per the JobTracker's configuration. Contributed by Amar Kamat.
          Hide
          Hemanth Yamijala added a comment -

          Patch for Hadoop .20, not for commit.

          Show
          Hemanth Yamijala added a comment - Patch for Hadoop .20, not for commit.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Updated release note from the commit message.

          Show
          Vinod Kumar Vavilapalli added a comment - Updated release note from the commit message.

            People

            • Assignee:
              Amar Kamat
              Reporter:
              Amar Kamat
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development