Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-899

When using LinuxTaskController, localized files may become accessible to unintended users if permissions are misconfigured.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: tasktracker
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      Added configuration "mapreduce.tasktracker.group", a group name to which TaskTracker belongs. When LinuxTaskController is used, task-controller binary's group owner should be this group. The same should be specified in task-controller.cfg also.
      Show
      Added configuration "mapreduce.tasktracker.group", a group name to which TaskTracker belongs. When LinuxTaskController is used, task-controller binary's group owner should be this group. The same should be specified in task-controller.cfg also.

      Description

      To enforce the accessibility of job files to only the job-owner and the TaskTracker, as per MAPREDUCE-842, it is trusted that the setuid/setgid linux TaskController binary is group owned by a special group to which only TaskTracker belongs and not just any group to which TT belongs. If the trust is broken, possibly due to misconfiguration by admins, the local files become accessible to unintended users, yet giving false sense of security to the admins.

      1. testplan.txt
        1 kB
        Amareshwari Sriramadasu
      2. patch-899-7.txt
        25 kB
        Hemanth Yamijala
      3. patch-899-6.txt
        25 kB
        Amareshwari Sriramadasu
      4. patch-899-5.txt
        26 kB
        Amareshwari Sriramadasu
      5. patch-899-4.txt
        27 kB
        Amareshwari Sriramadasu
      6. patch-899-3.txt
        28 kB
        Amareshwari Sriramadasu
      7. patch-899-2.txt
        25 kB
        Amareshwari Sriramadasu
      8. patch-899-1.txt
        21 kB
        Amareshwari Sriramadasu
      9. patch-899.txt
        10 kB
        Amareshwari Sriramadasu
      10. mr-899-20.patch
        25 kB
        Hemanth Yamijala
      11. MAPREDUCE-899-20090828.txt
        9 kB
        Vinod Kumar Vavilapalli

        Issue Links

          Activity

          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Tom White made changes -
          Fix Version/s 0.21.0 [ 12314045 ]
          Fix Version/s 0.22.0 [ 12314184 ]
          Hemanth Yamijala made changes -
          Attachment mr-899-20.patch [ 12437670 ]
          Hide
          Hemanth Yamijala added a comment -

          Patch for earlier version of hadoop. Not for commit here.

          Show
          Hemanth Yamijala added a comment - Patch for earlier version of hadoop. Not for commit here.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #225 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/225/)

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #225 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/225/ )
          Amareshwari Sriramadasu made changes -
          Release Note Added configuration "mapreduce.tasktracker.group", a group name to which TaskTracker belongs. When LinuxTaskController is used, task-controller binary's group owner should be this group. The same should be specified in task-controller.cfg also.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #222 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/222/)
          . Modified LinuxTaskController to check that task-controller has right permissions and ownership before performing any actions. Contributed by Amareshwari Sriramadasu.

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #222 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk/222/ ) . Modified LinuxTaskController to check that task-controller has right permissions and ownership before performing any actions. Contributed by Amareshwari Sriramadasu.
          Hemanth Yamijala made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Incompatible change, Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Hemanth Yamijala added a comment -

          Given the test failures are unrelated, I committed this. Thanks, Amareshwari ! Can you please update the release notes ?

          Show
          Hemanth Yamijala added a comment - Given the test failures are unrelated, I committed this. Thanks, Amareshwari ! Can you please update the release notes ?
          Hide
          Hemanth Yamijala added a comment -

          I filed HADOOP-6528 to track the Jetty server problem. I am not sure if the second test failure is also indicating an issue with incorrect cleanup code in the testcase or MiniDFSCluster.

          Show
          Hemanth Yamijala added a comment - I filed HADOOP-6528 to track the Jetty server problem. I am not sure if the second test failure is also indicating an issue with incorrect cleanup code in the testcase or MiniDFSCluster.
          Hide
          Hemanth Yamijala added a comment -

          Looking at the failed test results, I think we have hit HADOOP-4744 again, this time though with NameNode.

          We have these in the logs:

          2010-01-30 16:02:53,658 INFO http.HttpServer (HttpServer.java:start(442)) - Port returned by webServer.getConnectors()[0].getLocalPort() before open() is -1. Opening the listener on 0
          2010-01-30 16:02:53,659 INFO http.HttpServer (HttpServer.java:start(447)) - listener.getLocalPort() returned 38815 webServer.getConnectors()[0].getLocalPort() returned 38815
          2010-01-30 16:02:53,659 INFO http.HttpServer (HttpServer.java:start(480)) - Jetty bound to port 38815

          But then, the test itself fails like this:

          java.lang.IllegalArgumentException: port out of range:-1
          	at java.net.InetSocketAddress.<init>(InetSocketAddress.java:118)
          	at org.apache.hadoop.hdfs.server.namenode.NameNode.startHttpServer(NameNode.java:377)
          	at org.apache.hadoop.hdfs.server.namenode.NameNode.activate(NameNode.java:317)
          	at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:308)
          	at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:421)
          	at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:410)
          	at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1261)
          	at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:278)
          	at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:127)
          	at org.apache.hadoop.mapred.ClusterMapReduceTestCase.startCluster(ClusterMapReduceTestCase.java:81)
          	at org.apache.hadoop.mapred.ClusterMapReduceTestCase.setUp(ClusterMapReduceTestCase.java:56)
          

          Looking at the code in NameNode.startHttpServer, it looks like httpServer.start() executed correctly, as indicated by the log line, "listener.getLocalPort() returned 38815" above. Then httpServer.getPort() seems to have returned -1. This is the same scenario that was described in HADOOP-4744 and was worked around in Map/Reduce by failing the TaskTracker when we hit this condition. We need a fix in Jetty or a workaround like in this in DFS also to avoid the problem. I will open a separate JIRA for this.

          The second testcase, IMO failed because the first did not finish correctly in this case. Since in ClusterMapReduceTest, the dfsCluster is not initialized correctly, I suppose a partial setup has happened, but the corresponding cleanup did not, because ClusterMapReduceTest.teardown() would not have done anything. And this resulted in the second test to fail.

          Either way both test failures are unrelated to this patch.

          Show
          Hemanth Yamijala added a comment - Looking at the failed test results, I think we have hit HADOOP-4744 again, this time though with NameNode. We have these in the logs: 2010-01-30 16:02:53,658 INFO http.HttpServer (HttpServer.java:start(442)) - Port returned by webServer.getConnectors() [0] .getLocalPort() before open() is -1. Opening the listener on 0 2010-01-30 16:02:53,659 INFO http.HttpServer (HttpServer.java:start(447)) - listener.getLocalPort() returned 38815 webServer.getConnectors() [0] .getLocalPort() returned 38815 2010-01-30 16:02:53,659 INFO http.HttpServer (HttpServer.java:start(480)) - Jetty bound to port 38815 But then, the test itself fails like this: java.lang.IllegalArgumentException: port out of range:-1 at java.net.InetSocketAddress.<init>(InetSocketAddress.java:118) at org.apache.hadoop.hdfs.server.namenode.NameNode.startHttpServer(NameNode.java:377) at org.apache.hadoop.hdfs.server.namenode.NameNode.activate(NameNode.java:317) at org.apache.hadoop.hdfs.server.namenode.NameNode.initialize(NameNode.java:308) at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:421) at org.apache.hadoop.hdfs.server.namenode.NameNode.<init>(NameNode.java:410) at org.apache.hadoop.hdfs.server.namenode.NameNode.createNameNode(NameNode.java:1261) at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:278) at org.apache.hadoop.hdfs.MiniDFSCluster.<init>(MiniDFSCluster.java:127) at org.apache.hadoop.mapred.ClusterMapReduceTestCase.startCluster(ClusterMapReduceTestCase.java:81) at org.apache.hadoop.mapred.ClusterMapReduceTestCase.setUp(ClusterMapReduceTestCase.java:56) Looking at the code in NameNode.startHttpServer, it looks like httpServer.start() executed correctly, as indicated by the log line, "listener.getLocalPort() returned 38815" above. Then httpServer.getPort() seems to have returned -1. This is the same scenario that was described in HADOOP-4744 and was worked around in Map/Reduce by failing the TaskTracker when we hit this condition. We need a fix in Jetty or a workaround like in this in DFS also to avoid the problem. I will open a separate JIRA for this. The second testcase, IMO failed because the first did not finish correctly in this case. Since in ClusterMapReduceTest, the dfsCluster is not initialized correctly, I suppose a partial setup has happened, but the corresponding cleanup did not, because ClusterMapReduceTest.teardown() would not have done anything. And this resulted in the second test to fail. Either way both test failures are unrelated to this 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/12431865/patch-899-7.txt
          against trunk revision 904717.

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

          +1 tests included. The patch appears to include 12 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 failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/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/12431865/patch-899-7.txt against trunk revision 904717. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/419/console This message is automatically generated.
          Hemanth Yamijala made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hemanth Yamijala added a comment -

          Running through Hudson...

          Show
          Hemanth Yamijala added a comment - Running through Hudson...
          Hemanth Yamijala made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hemanth Yamijala made changes -
          Attachment patch-899-7.txt [ 12431865 ]
          Hide
          Hemanth Yamijala added a comment -

          I reviewed the last patch. It was fine except for some minor nits which I've corrected in the attached patch (patch-899-7.txt). The specific changes are:

          • Set errno to 0 before use in check_taskcontroller_permissions. This is required as per contract of errno.
          • Fixed a typo in documentation of mapred-default.xml
          • Fixed an extraneous comma in task-controller.h.

          I ran most of the tests documented in the testplan by Amareshwari and they passed.

          Show
          Hemanth Yamijala added a comment - I reviewed the last patch. It was fine except for some minor nits which I've corrected in the attached patch (patch-899-7.txt). The specific changes are: Set errno to 0 before use in check_taskcontroller_permissions. This is required as per contract of errno. Fixed a typo in documentation of mapred-default.xml Fixed an extraneous comma in task-controller.h. I ran most of the tests documented in the testplan by Amareshwari and they passed.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431772/testplan.txt
          against trunk revision 904491.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/295/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/12431772/testplan.txt against trunk revision 904491. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/295/console This message is automatically generated.
          Amareshwari Sriramadasu made changes -
          Attachment testplan.txt [ 12431772 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          I'd request you to document as a comment on this JIRA the various manual tests you have run with different binary permissions / ownership.

          Attaching the testplan.

          Ran all LinuxTaskController tests. Test failures are all known.

          Show
          Amareshwari Sriramadasu added a comment - I'd request you to document as a comment on this JIRA the various manual tests you have run with different binary permissions / ownership. Attaching the testplan. Ran all LinuxTaskController tests. Test failures are all known.
          Amareshwari Sriramadasu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Amareshwari Sriramadasu made changes -
          Attachment patch-899-6.txt [ 12431769 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Patch with comments addressed.

          The wait introduced for in the ClusterWithLinuxTaskController does not seem required, because doesn't MiniMRCluster itself wait for clusters to join ?

          Verified. The new code is not required. If there are any timeouts because trackers could not come up, MAPREDUCE-1366 should address that.

          Show
          Amareshwari Sriramadasu added a comment - Patch with comments addressed. The wait introduced for in the ClusterWithLinuxTaskController does not seem required, because doesn't MiniMRCluster itself wait for clusters to join ? Verified. The new code is not required. If there are any timeouts because trackers could not come up, MAPREDUCE-1366 should address that.
          Amareshwari Sriramadasu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Hemanth Yamijala added a comment -

          A few minor comments:

          • Remove unused variables - tt_uid and tt_gid and associated code.
          • It will be good to check for user, group first and then permissions. It's a more natural code ordering than what exists currently, which is user, permissions, then group.
          • Rather than check errno as return of getgrgid, it seems more conventional to check for return value being null. This is what man states anyway for return values of getgrgid. It was not clear to me that errno will be set to non-zero if the GID does not exist. However note that errno can still be captured and used for strerror.
          • Should we rename mapreduce.tasktracker.taskcontroller.group to a more generic name and purpose like mapreduce.tasktracker.group. We could document that this is the group to which the tasktracker belongs and qualify that if LinuxTaskController is used, it takes more meaning - in that it should be the group owner of the binary etc.
          • The wait introduced for in the ClusterWithLinuxTaskController does not seem required, because doesn't MiniMRCluster itself wait for clusters to join ? Can you please check if the timeout code you added actually helps ?

          Thinking about tests, I think it is difficult to actually test all the paths verified by the C code without mocking the system calls or building different binary executables and setting them up with different permissions. While we work out details to improve the way we run LinuxTaskController tests - MAPREDUCE-1429, MAPREDUCE-1300 - I'd request you to document as a comment on this JIRA the various manual tests you have run with different binary permissions / ownership.

          Show
          Hemanth Yamijala added a comment - A few minor comments: Remove unused variables - tt_uid and tt_gid and associated code. It will be good to check for user, group first and then permissions. It's a more natural code ordering than what exists currently, which is user, permissions, then group. Rather than check errno as return of getgrgid, it seems more conventional to check for return value being null. This is what man states anyway for return values of getgrgid. It was not clear to me that errno will be set to non-zero if the GID does not exist. However note that errno can still be captured and used for strerror. Should we rename mapreduce.tasktracker.taskcontroller.group to a more generic name and purpose like mapreduce.tasktracker.group. We could document that this is the group to which the tasktracker belongs and qualify that if LinuxTaskController is used, it takes more meaning - in that it should be the group owner of the binary etc. The wait introduced for in the ClusterWithLinuxTaskController does not seem required, because doesn't MiniMRCluster itself wait for clusters to join ? Can you please check if the timeout code you added actually helps ? Thinking about tests, I think it is difficult to actually test all the paths verified by the C code without mocking the system calls or building different binary executables and setting them up with different permissions. While we work out details to improve the way we run LinuxTaskController tests - MAPREDUCE-1429 , MAPREDUCE-1300 - I'd request you to document as a comment on this JIRA the various manual tests you have run with different binary permissions / ownership.
          Hide
          Amareshwari Sriramadasu added a comment -

          Ran all tests with LinuxTaskController. Test failures were all known ( MAPREDUCE-1421 and MAPREDUCE-1322)

          Show
          Amareshwari Sriramadasu added a comment - Ran all tests with LinuxTaskController. Test failures were all known ( MAPREDUCE-1421 and MAPREDUCE-1322 )
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431651/patch-899-5.txt
          against trunk revision 903563.

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

          +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/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/12431651/patch-899-5.txt against trunk revision 903563. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/293/console This message is automatically generated.
          Amareshwari Sriramadasu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Amareshwari Sriramadasu made changes -
          Attachment patch-899-5.txt [ 12431651 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Patch modifying the permission checks as suggested.

          Show
          Amareshwari Sriramadasu added a comment - Patch modifying the permission checks as suggested.
          Amareshwari Sriramadasu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431637/patch-899-4.txt
          against trunk revision 903563.

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

          +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/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/12431637/patch-899-4.txt against trunk revision 903563. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/412/console This message is automatically generated.
          Hide
          Hemanth Yamijala added a comment -

          I think we can simplify the approach still.

          Please note that on other JIRAs, we have assumed that the task-controller binary would be set up with permissions and ownership that prevent misuse. Specifically, the binary would be setuid/setgid executable, owned by root/a special group, and importantly, other users will not have any permissions. I had proposed on this JIRA earlier on that we trust administrators to setup the special group correctly and specify it in the taskcontroller.cfg file.

          Following up on this, I think if we verify that only root and the admin specified group can execute the file in a setuid mode, then I think we are pretty much done. We should specifically check that others cannot execute the file. Can we change the approach in the patch to match this ? Any concerns ?

          Show
          Hemanth Yamijala added a comment - I think we can simplify the approach still. Please note that on other JIRAs, we have assumed that the task-controller binary would be set up with permissions and ownership that prevent misuse. Specifically, the binary would be setuid/setgid executable, owned by root/a special group, and importantly, other users will not have any permissions. I had proposed on this JIRA earlier on that we trust administrators to setup the special group correctly and specify it in the taskcontroller.cfg file. Following up on this, I think if we verify that only root and the admin specified group can execute the file in a setuid mode, then I think we are pretty much done. We should specifically check that others cannot execute the file. Can we change the approach in the patch to match this ? Any concerns ?
          Amareshwari Sriramadasu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Amareshwari Sriramadasu made changes -
          Attachment patch-899-4.txt [ 12431637 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Patch updated to trunnk.

          All LinuxTaskController tests passed except TestDebugScrpitWithLinuxTaskController, TestJobExecutionAsDifferentUser (due to MAPREDUCE-1421) and TestStreamingAsDifferentUser (MAPREDUCE-1322).

          Show
          Amareshwari Sriramadasu added a comment - Patch updated to trunnk. All LinuxTaskController tests passed except TestDebugScrpitWithLinuxTaskController, TestJobExecutionAsDifferentUser (due to MAPREDUCE-1421 ) and TestStreamingAsDifferentUser ( MAPREDUCE-1322 ).
          Amareshwari Sriramadasu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431525/patch-899-3.txt
          against trunk revision 903544.

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

          +1 tests included. The patch appears to include 15 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 failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/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/12431525/patch-899-3.txt against trunk revision 903544. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 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 failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/410/console This message is automatically generated.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          +1 for the latest patch.

          Show
          Vinod Kumar Vavilapalli added a comment - +1 for the latest patch.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          We should verify MAPREDUCE-981 after this issue is fixed and close that one as duplicate accordingly.

          Show
          Vinod Kumar Vavilapalli added a comment - We should verify MAPREDUCE-981 after this issue is fixed and close that one as duplicate accordingly.
          Vinod Kumar Vavilapalli made changes -
          Link This issue relates to MAPREDUCE-981 [ MAPREDUCE-981 ]
          Amareshwari Sriramadasu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Amareshwari Sriramadasu made changes -
          Attachment patch-899-3.txt [ 12431525 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Patch updating the documentation as Vinod suggested.

          Ran ant docs on my machine. It succeeded.

          Show
          Amareshwari Sriramadasu added a comment - Patch updating the documentation as Vinod suggested. Ran ant docs on my machine. It succeeded.
          Amareshwari Sriramadasu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Vinod Kumar Vavilapalli added a comment -

          The code changes all look good now. Only minor finishing touches needed for the documentation:

          • mapreduce.tasktracker.taskcontroller.group is an unusual configuration item documented in mapred-default.xml. It is not directly used by the TT daemon itself, but instead by the task-controller binary and that to only when LinuxTaskController is configured for the TT. I think we should state this clearly in mapred-default.xml. Also we should point to the taskcontroller.cfg file where it is actually needed to be configured.
          • cluster_setup.xml changes also need some update. We should say that mapreduce.tasktracker.taskcontroller.group needs to be configured in mapred-site.xml and taskcontroller.cfg. For better organization, we may move taskcontroller.cfg to a sub-section and refer to that. Also the special group may now contain other members like admin users besides TT unlike before. Though having job-submitters themselves as other members will compromise security. We should change the description to reflect this. The example that follows should also be inline with this. Minor typo in mapreduce.tasktracker.taskcontroller.group description in taskcontroller.cfg section - it should be "TaskTracker should belong to this group"
          Show
          Vinod Kumar Vavilapalli added a comment - The code changes all look good now. Only minor finishing touches needed for the documentation: mapreduce.tasktracker.taskcontroller.group is an unusual configuration item documented in mapred-default.xml. It is not directly used by the TT daemon itself, but instead by the task-controller binary and that to only when LinuxTaskController is configured for the TT. I think we should state this clearly in mapred-default.xml. Also we should point to the taskcontroller.cfg file where it is actually needed to be configured. cluster_setup.xml changes also need some update. We should say that mapreduce.tasktracker.taskcontroller.group needs to be configured in mapred-site.xml and taskcontroller.cfg. For better organization, we may move taskcontroller.cfg to a sub-section and refer to that. Also the special group may now contain other members like admin users besides TT unlike before. Though having job-submitters themselves as other members will compromise security. We should change the description to reflect this. The example that follows should also be inline with this. Minor typo in mapreduce.tasktracker.taskcontroller.group description in taskcontroller.cfg section - it should be "TaskTracker should belong to this group"
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12430851/patch-899-2.txt
          against trunk revision 901350.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/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/12430851/patch-899-2.txt against trunk revision 901350. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/399/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12430851/patch-899-2.txt
          against trunk revision 900815.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 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 failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/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/12430851/patch-899-2.txt against trunk revision 900815. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 15 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 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 failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/398/console This message is automatically generated.
          Amareshwari Sriramadasu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Ran all LinuxTaskController tests. All of them passed except TestStreamingAsDifferentUser (due to MAPREDUCE-1322)

          Show
          Amareshwari Sriramadasu added a comment - Ran all LinuxTaskController tests. All of them passed except TestStreamingAsDifferentUser (due to MAPREDUCE-1322 )
          Amareshwari Sriramadasu made changes -
          Attachment patch-899-2.txt [ 12430851 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Patch incorporating most of the review comments.

          TTConfig.TT_TASK_CONTROLLER_GROUP seems to be only for internal use.

          I dont think it is only for internal use. Though currently it is read only in TaskController, I think it is like any other configuration. After MAPREDUCE-849, it should to be categorized into one the categories : Cluster/JobTracker/TaskTracker/Client/Job. So, this goes into TaskTracker. Does that make sense?

          taskcontroller.cfg: Can you modify the comment for mapreduce.tasktracker.taskcontroller.group to be "The group owner of the task controller binary"

          With the above explanation, I think this change is not needed. The comment is in consistent with other config comments.

          Instead of giving the default value of System.getProperty(TASKCONTROLLER_PATH)+ "/task-controller" to taskControllerExePath, you can use setTaskControllerExe() whenever needed.

          Now that TaskTracker constructor (which calls TaskController.setup()) requires this value to be set, we cannot postpone setting of taskControllerExePath after the constructor (as earlier). Since default value can be constructed statically, there is no harm setting the default value for taskControllerExePath.

          For general ClusterWithLinuxTaskController tests, we can disable the binary checks by overriding LinuxTaskController.setup() method and can then simply generate the configuration file after the cluster starts so that further checks done when tasks are launched can go smoothly.

          I have done this by overriding LinuxTaskController.setup() method to set set task-controller group and writing the configuration file and then calling super.setup().

          To verify the checks during TT startup, we can add simple unit tests which just start the TT with/without group name and with invalid group name.

          Added this unit test.

          Show
          Amareshwari Sriramadasu added a comment - Patch incorporating most of the review comments. TTConfig.TT_TASK_CONTROLLER_GROUP seems to be only for internal use. I dont think it is only for internal use. Though currently it is read only in TaskController, I think it is like any other configuration. After MAPREDUCE-849 , it should to be categorized into one the categories : Cluster/JobTracker/TaskTracker/Client/Job. So, this goes into TaskTracker. Does that make sense? taskcontroller.cfg: Can you modify the comment for mapreduce.tasktracker.taskcontroller.group to be "The group owner of the task controller binary" With the above explanation, I think this change is not needed. The comment is in consistent with other config comments. Instead of giving the default value of System.getProperty(TASKCONTROLLER_PATH)+ "/task-controller" to taskControllerExePath, you can use setTaskControllerExe() whenever needed. Now that TaskTracker constructor (which calls TaskController.setup()) requires this value to be set, we cannot postpone setting of taskControllerExePath after the constructor (as earlier). Since default value can be constructed statically, there is no harm setting the default value for taskControllerExePath. For general ClusterWithLinuxTaskController tests, we can disable the binary checks by overriding LinuxTaskController.setup() method and can then simply generate the configuration file after the cluster starts so that further checks done when tasks are launched can go smoothly. I have done this by overriding LinuxTaskController.setup() method to set set task-controller group and writing the configuration file and then calling super.setup(). To verify the checks during TT startup, we can add simple unit tests which just start the TT with/without group name and with invalid group name. Added this unit test.
          Amareshwari Sriramadasu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Looked at the patch. Some comments I have:

          • main.c : We should check errno after the statement (+131) int ngroups = getgroups (0, NULL); and return immediately on error. If there is no error, we should (of course) reset errno to zero for further ops.
          • TTConfig.TT_TASK_CONTROLLER_GROUP seems to be only for internal use. In that case, let's not even put it in there and instead put it possibly in ClusterWithLinuxTaskController. Correspondingly we will not need the documentation changes in mapred-default.xml
          • We should also update the forrest documentation regarding this change.

          ClusterWithLinuxTaskController.java

          • createTaskControllerConf() can either (1) take the group name itself as an argument instead of a whole conf object or (2) only take a conf object and extract both local-dirs and group name from the conf object.
          • Instead of giving the default value of System.getProperty(TASKCONTROLLER_PATH)+ "/task-controller" to taskControllerExePath, you can use setTaskControllerExe() whenever needed.
          • As for the changes to generate the taskcontroller.cfg file here and in MiniMRCluster, I think there is a lot easier way of doing this which avoids the changes to MiniMRCluster
            • For general ClusterWithLinuxTaskController tests, we can disable the binary checks by overriding LinuxTaskController.setup() method and can then simply generate the configuration file after the cluster starts so that further checks done when tasks are launched can go smoothly.
            • The changes to the tests in the current patch already test that the checks are done successfully during task startup. To verify the checks during TT startup, we can add simple unit tests which just start the TT with/without group name and with invalid group name.
              Does that work?
          • Currently, number of TTs is a final constant 1 in this class. It doesn't really suport more nodes in that each TT will then need its own copy of binary file. This is because a binary is tied to a config file. So the multiple TTs case won't work with multiple configs but a single binary. Should we copy the binary to each TT's space and generate multiple configs? Or leave it as is and comment on NUMBER_OF_NODES saying to change it to more than 1, we will need more changes?

          Nits:

          • taskcontroller.cfg: Can you modify the comment for mapreduce.tasktracker.taskcontroller.group to be "The group owner of the task controller binary"?
          • LinuxTaskController: In setp() method, can you change the log statement @114 to warn level and enhance the log statement too saying setup() failed because of invalid permissions/ownership on the binary?
          Show
          Vinod Kumar Vavilapalli added a comment - Looked at the patch. Some comments I have: main.c : We should check errno after the statement (+131) int ngroups = getgroups (0, NULL); and return immediately on error. If there is no error, we should (of course) reset errno to zero for further ops. TTConfig.TT_TASK_CONTROLLER_GROUP seems to be only for internal use. In that case, let's not even put it in there and instead put it possibly in ClusterWithLinuxTaskController . Correspondingly we will not need the documentation changes in mapred-default.xml We should also update the forrest documentation regarding this change. ClusterWithLinuxTaskController.java createTaskControllerConf() can either (1) take the group name itself as an argument instead of a whole conf object or (2) only take a conf object and extract both local-dirs and group name from the conf object. Instead of giving the default value of System.getProperty(TASKCONTROLLER_PATH)+ "/task-controller" to taskControllerExePath , you can use setTaskControllerExe() whenever needed. As for the changes to generate the taskcontroller.cfg file here and in MiniMRCluster , I think there is a lot easier way of doing this which avoids the changes to MiniMRCluster For general ClusterWithLinuxTaskController tests, we can disable the binary checks by overriding LinuxTaskController.setup() method and can then simply generate the configuration file after the cluster starts so that further checks done when tasks are launched can go smoothly. The changes to the tests in the current patch already test that the checks are done successfully during task startup. To verify the checks during TT startup, we can add simple unit tests which just start the TT with/without group name and with invalid group name. Does that work? Currently, number of TTs is a final constant 1 in this class. It doesn't really suport more nodes in that each TT will then need its own copy of binary file. This is because a binary is tied to a config file. So the multiple TTs case won't work with multiple configs but a single binary. Should we copy the binary to each TT's space and generate multiple configs? Or leave it as is and comment on NUMBER_OF_NODES saying to change it to more than 1, we will need more changes? Nits: taskcontroller.cfg: Can you modify the comment for mapreduce.tasktracker.taskcontroller.group to be "The group owner of the task controller binary"? LinuxTaskController: In setp() method, can you change the log statement @114 to warn level and enhance the log statement too saying setup() failed because of invalid permissions/ownership on the binary?
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12429436/patch-899-1.txt
          against trunk revision 895914.

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

          +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/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/12429436/patch-899-1.txt against trunk revision 895914. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/356/console This message is automatically generated.
          Amareshwari Sriramadasu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Assignee Vinod K V [ vinodkv ] Amareshwari Sriramadasu [ amareshwari ]
          Fix Version/s 0.22.0 [ 12314184 ]
          Amareshwari Sriramadasu made changes -
          Attachment patch-899-1.txt [ 12429436 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Patch updated to trunk. Earlier patch had bug in LinuxTaskController tests, fixed it. Ran all LinuxTaskController tests and all of them passed (used available patches for MAPREDUCE-1322 and MAPREDUCE-1186 for failing tests)

          Show
          Amareshwari Sriramadasu added a comment - Patch updated to trunk. Earlier patch had bug in LinuxTaskController tests, fixed it. Ran all LinuxTaskController tests and all of them passed (used available patches for MAPREDUCE-1322 and MAPREDUCE-1186 for failing tests)
          Amareshwari Sriramadasu made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12428904/patch-899.txt
          against trunk revision 893469.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +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 failed core unit tests.

          -1 contrib tests. The patch failed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/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/12428904/patch-899.txt against trunk revision 893469. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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 failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/341/console This message is automatically generated.
          Amareshwari Sriramadasu made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Amareshwari Sriramadasu made changes -
          Attachment patch-899.txt [ 12428904 ]
          Hide
          Amareshwari Sriramadasu added a comment -

          Attaching patch after working on Hemanth's suggestion.

          I tested the patch verifying all the conditions.
          Trying to see I can write a testcase. But the patch itself is a working patch and is up for review.

          Show
          Amareshwari Sriramadasu added a comment - Attaching patch after working on Hemanth's suggestion. I tested the patch verifying all the conditions. Trying to see I can write a testcase. But the patch itself is a working patch and is up for review.
          Hide
          Hemanth Yamijala added a comment -

          Hmm. There are assumptions we are making about the setup that may/may not be true in practise. For e.g, admins may assume that more than one special user could be part of the special group to which the task-controller group ownership belongs - say 'hdfs' and 'mapreduce' are two user accounts belonging to special group 'hadoop'. Isn't it simpler to configure the special group's name in task-controller.cfg and check against it ? We can then ensure the task-controller.cfg file has right ownership and permissions to prevent misuse and assume whatever is there in the file is valid.

          Show
          Hemanth Yamijala added a comment - Hmm. There are assumptions we are making about the setup that may/may not be true in practise. For e.g, admins may assume that more than one special user could be part of the special group to which the task-controller group ownership belongs - say 'hdfs' and 'mapreduce' are two user accounts belonging to special group 'hadoop'. Isn't it simpler to configure the special group's name in task-controller.cfg and check against it ? We can then ensure the task-controller.cfg file has right ownership and permissions to prevent misuse and assume whatever is there in the file is valid.
          Vinod Kumar Vavilapalli made changes -
          Attachment MAPREDUCE-899-20090828.txt [ 12417972 ]
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Attaching a patch that does the following:

          • Put in checks in taskcontroller to make sure that permissions are properly set.. For this, we check whether task-controller binary is
            • set setuid and setgid bits
            • user-owned by root
            • group-owned by a special group to which only TT is a member. To check this, we
              • scan the entry in group database for the special group and make sure it has only one member which is the tt_user
              • scan all the entries in passwd database and make sure that only tt_user has the special group as its primary group.
          • The checks are made whenever task-controller binary is used in various operations like initialize_job, intialize_task etc.
          • The check is also made during TT start up so as to fail early in case. This is done by a plain run of the task-controller binary.

          No new tests are included, existing tests test this issue when combined with various combinations of ownership on the binary.

          Show
          Vinod Kumar Vavilapalli added a comment - Attaching a patch that does the following: Put in checks in taskcontroller to make sure that permissions are properly set.. For this, we check whether task-controller binary is set setuid and setgid bits user-owned by root group-owned by a special group to which only TT is a member. To check this, we scan the entry in group database for the special group and make sure it has only one member which is the tt_user scan all the entries in passwd database and make sure that only tt_user has the special group as its primary group. The checks are made whenever task-controller binary is used in various operations like initialize_job, intialize_task etc. The check is also made during TT start up so as to fail early in case. This is done by a plain run of the task-controller binary. No new tests are included, existing tests test this issue when combined with various combinations of ownership on the binary.
          Vinod Kumar Vavilapalli made changes -
          Field Original Value New Value
          Assignee Vinod K V [ vinodkv ]
          Hide
          Vinod Kumar Vavilapalli added a comment -

          There are two possible solutions:

          • Let the onus be on the administrators to be extra careful while configuring. Code is left as is and simple but the main disadvantage is that it gives a false sense of security and security holes might not be discovered until later.
          • Put explicit changes in the TaskTracker to shout aloud in case of misconfiguration of these permissions. If needed, special ant arguments might be added to disable these checks while testing. Though adds a bit of new code, it is advantageous in that administrators are informed very early.

          After discussing with Rajiv/Hemanth/Sreekanth, the later is agreed upon.

          Show
          Vinod Kumar Vavilapalli added a comment - There are two possible solutions: Let the onus be on the administrators to be extra careful while configuring. Code is left as is and simple but the main disadvantage is that it gives a false sense of security and security holes might not be discovered until later. Put explicit changes in the TaskTracker to shout aloud in case of misconfiguration of these permissions. If needed, special ant arguments might be added to disable these checks while testing. Though adds a bit of new code, it is advantageous in that administrators are informed very early. After discussing with Rajiv/Hemanth/Sreekanth, the later is agreed upon.
          Vinod Kumar Vavilapalli created issue -

            People

            • Assignee:
              Amareshwari Sriramadasu
              Reporter:
              Vinod Kumar Vavilapalli
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development