Hadoop Common
  1. Hadoop Common
  2. HADOOP-6864

Provide a JNI-based implementation of ShellBasedUnixGroupsNetgroupMapping (implementation of GroupMappingServiceProvider)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.23.0
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Incompatible change, Reviewed

      Description

      The netgroups implementation of GroupMappingServiceProvider (see ShellBasedUnixGroupsNetgroupMapping.java) does a fork of a unix command to get the netgroups of a user. Since the group resolution happens in the servers, this might be costly. This jira aims at providing a JNI-based implementation for GroupMappingServiceProvider.

      Note that this is similar to what https://issues.apache.org/jira/browse/HADOOP-6818 does for implementation of GroupMappingServiceProvider that supports only unix groups.

      1. HADOOP-6864-0.22-3.patch
        549 kB
        Erik Steffl
      2. HADOOP-6864-0.22-2.patch
        549 kB
        Erik Steffl
      3. HADOOP-6864-0.22-1.patch
        549 kB
        Erik Steffl
      4. HADOOP-6864-0.22.patch
        43 kB
        Erik Steffl
      5. HADOOP-6864-0.20.1xx-1.patch
        19 kB
        Erik Steffl
      6. HADOOP-6864-0.20.1xx.patch
        18 kB
        Erik Steffl

        Issue Links

          Activity

          carltone made changes -
          Link This issue relates to HADOOP-10194 [ HADOOP-10194 ]
          Jeff Hammerbacher made changes -
          Link This issue relates to HADOOP-7147 [ HADOOP-7147 ]
          Arun C Murthy made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Aaron T. Myers made changes -
          Link This issue is duplicated by HADOOP-7033 [ HADOOP-7033 ]
          Todd Lipcon made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Hadoop Flags [Incompatible change] [Incompatible change, Reviewed]
          Resolution Fixed [ 1 ]
          Hide
          Todd Lipcon added a comment -

          This was committed a while back but never marked resolved.

          Show
          Todd Lipcon added a comment - This was committed a while back but never marked resolved.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #580 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/580/)
          HADOOP-6864. Moving CHANGES.txt record to the right place - trunk changes

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #580 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/580/ ) HADOOP-6864 . Moving CHANGES.txt record to the right place - trunk changes
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #479 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/479/)
          HADOOP-6864. Moving CHANGES.txt record to the right place - trunk changes

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #479 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/479/ ) HADOOP-6864 . Moving CHANGES.txt record to the right place - trunk changes
          Hide
          Boris Shkolnik added a comment -

          Thanks Todd.
          Since I don't know if it ever will be moved to 0.22, i've modified CHANGES.txt, by moving the line into the trunk area.

          Committed to trunk.

          Show
          Boris Shkolnik added a comment - Thanks Todd. Since I don't know if it ever will be moved to 0.22, i've modified CHANGES.txt, by moving the line into the trunk area. Committed to trunk.
          Jeff Hammerbacher made changes -
          Link This issue relates to HADOOP-6855 [ HADOOP-6855 ]
          Hide
          Todd Lipcon added a comment -

          Note that this was committed to trunk and not branch-0.22, but it's in CHANGES.txt for 0.22. Either CHANGES.txt should get updated, or it should get committed to 0.22 as well (in which case HDFS-1559 and MAPREDUCE-2232 also need to go on their respective release branches)

          Show
          Todd Lipcon added a comment - Note that this was committed to trunk and not branch-0.22, but it's in CHANGES.txt for 0.22. Either CHANGES.txt should get updated, or it should get committed to 0.22 as well (in which case HDFS-1559 and MAPREDUCE-2232 also need to go on their respective release branches)
          Todd Lipcon made changes -
          Link This issue breaks MAPREDUCE-2232 [ MAPREDUCE-2232 ]
          Todd Lipcon made changes -
          Link This issue breaks HDFS-1559 [ HDFS-1559 ]
          Todd Lipcon made changes -
          Hadoop Flags [Incompatible change]
          Assignee Boris Shkolnik [ boryas ]
          Fix Version/s 0.23.0 [ 12315569 ]
          Fix Version/s 0.22.0 [ 12314296 ]
          Hide
          Todd Lipcon added a comment -

          Marking incompatible change since this adds a new method to the GroupMappingServiceProvider interface, edited fix version to 0.23 since this seems to have been committed to trunk but not 0.22.

          Show
          Todd Lipcon added a comment - Marking incompatible change since this adds a new method to the GroupMappingServiceProvider interface, edited fix version to 0.23 since this seems to have been committed to trunk but not 0.22.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #555 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/555/)
          HADOOP-6864. Provide a JNI-based implementation of ShellBasedUnixGroupsNetgroupMapping (implementation of GroupMappingServiceProvider)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #555 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/555/ ) HADOOP-6864 . Provide a JNI-based implementation of ShellBasedUnixGroupsNetgroupMapping (implementation of GroupMappingServiceProvider)
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #460 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/460/)
          HADOOP-6864. Provide a JNI-based implementation of ShellBasedUnixGroupsNetgroupMapping (implementation of GroupMappingServiceProvider)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #460 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/460/ ) HADOOP-6864 . Provide a JNI-based implementation of ShellBasedUnixGroupsNetgroupMapping (implementation of GroupMappingServiceProvider)
          Hide
          Boris Shkolnik added a comment -

          committed to trunk. Thanks Erik.

          Show
          Boris Shkolnik added a comment - committed to trunk. Thanks Erik.
          Hide
          Erik Steffl added a comment -

          HADOOP-6864-0.22-3.patch also applies to common-trunk:

          • test-patch +1 (no minus ones)
          • unit test passes (TestAccessControlList using both implementations classes ShellBasedUnixGroupsNetgroupMapping and JniBasedUnixGroupsNetgroupMapping)
          • ant test succeeds

          FYI:

          Suspicious but unrelated test failures:

          • TestTFileByteArrays
          • TestTFileJClassComparatorByteArrays

          These tests fail only if native-compile is done, with or without patch. As far as I can tell this is not related to patch but maybe we should take a look at the tests and see why they fail if native code is compiled.

          Show
          Erik Steffl added a comment - HADOOP-6864 -0.22-3.patch also applies to common-trunk: test-patch +1 (no minus ones) unit test passes (TestAccessControlList using both implementations classes ShellBasedUnixGroupsNetgroupMapping and JniBasedUnixGroupsNetgroupMapping) ant test succeeds FYI: Suspicious but unrelated test failures: TestTFileByteArrays TestTFileJClassComparatorByteArrays These tests fail only if native-compile is done, with or without patch. As far as I can tell this is not related to patch but maybe we should take a look at the tests and see why they fail if native code is compiled.
          Hide
          Erik Steffl added a comment -

          HADOOP-6864-0.22-3.patch fixes test-patch issues:

          • member functions in NetgroupCache are static now (they only access static members) - per findbug findings
          • Groups.getUserToGroupsMappingService is marked synchronized - per findbug findings

          Patch-test output:

          [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 4 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 release audit. The applied patch does not increase the total number of release audit warnings.
          [exec]
          [exec] +1 system test framework. The patch passed system test framework compile.

          Show
          Erik Steffl added a comment - HADOOP-6864 -0.22-3.patch fixes test-patch issues: member functions in NetgroupCache are static now (they only access static members) - per findbug findings Groups.getUserToGroupsMappingService is marked synchronized - per findbug findings Patch-test output: [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 4 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 release audit. The applied patch does not increase the total number of release audit warnings. [exec] [exec] +1 system test framework. The patch passed system test framework compile.
          Erik Steffl made changes -
          Attachment HADOOP-6864-0.22-3.patch [ 12466848 ]
          Hide
          Boris Shkolnik added a comment -

          +1

          Show
          Boris Shkolnik added a comment - +1
          Hide
          Erik Steffl added a comment -

          HADOOP-6864-0.22-2.patch changes the log messages to format LOG.info(msg, e)

          Show
          Erik Steffl added a comment - HADOOP-6864 -0.22-2.patch changes the log messages to format LOG.info(msg, e)
          Erik Steffl made changes -
          Attachment HADOOP-6864-0.22-2.patch [ 12466411 ]
          Hide
          Boris Shkolnik added a comment -

          When you are adding an exception to LOG.info(), don't do "+ e", you can use LOG.info(msg, e) method.

          Show
          Boris Shkolnik added a comment - When you are adding an exception to LOG.info(), don't do "+ e", you can use LOG.info(msg, e) method.
          Show
          Erik Steffl added a comment - HADOOP-6864 -0.22-1.patch applies cleanly to current 0.22 branch and addresses issues in review https://issues.apache.org/jira/browse/HADOOP-6864?focusedCommentId=12971869&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12971869
          Erik Steffl made changes -
          Attachment HADOOP-6864-0.22-1.patch [ 12466356 ]
          Hide
          Boris Shkolnik added a comment -

          Reviewing additional changes from 0.20 patch
          1. use constant for "hadoop.security.group.mapping"
          2. test before and after groups.refresh(); (create a separate validate method).
          +1 otherwise.

          NOTE.
          This patch needs to be run MANUALLY by setting up unix files (/etc/netgroup and so on). For details please see the comments in the test itself.

          Show
          Boris Shkolnik added a comment - Reviewing additional changes from 0.20 patch 1. use constant for "hadoop.security.group.mapping" 2. test before and after groups.refresh(); (create a separate validate method). +1 otherwise. NOTE. This patch needs to be run MANUALLY by setting up unix files (/etc/netgroup and so on). For details please see the comments in the test itself.
          Show
          Erik Steffl added a comment - Re https://issues.apache.org/jira/browse/HADOOP-6864?focusedCommentId=12931296&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#action_12931296 Sorry for confusion, HADOOP-6884 should have been HADOOP-6864 . The rest of the comment is valid.
          Erik Steffl made changes -
          Attachment HADOOP-6884-0.22-1.patch [ 12459416 ]
          Erik Steffl made changes -
          Attachment HADOOP-6864-0.22.patch [ 12459417 ]
          Hide
          Erik Steffl added a comment -

          HADOOP-6884-0.22-1.patch for trunk:

          • combines HADOOP-6855 0.20 and HADOOP-6884 0.20 patches (i.e. both shell and JNI implementations)
          • implements suggestion in HADOOP-6878
          • has improved test, see instructions in TestAccessControlList
          • adds support for new API in AccessControlList (addGroup)
          Show
          Erik Steffl added a comment - HADOOP-6884 -0.22-1.patch for trunk: combines HADOOP-6855 0.20 and HADOOP-6884 0.20 patches (i.e. both shell and JNI implementations) implements suggestion in HADOOP-6878 has improved test, see instructions in TestAccessControlList adds support for new API in AccessControlList (addGroup)
          Erik Steffl made changes -
          Attachment HADOOP-6884-0.22-1.patch [ 12459416 ]
          Erik Steffl made changes -
          Attachment HADOOP-6864-0.20.1xx-1.patch [ 12450342 ]
          Hide
          Erik Steffl added a comment -

          HADOOP-6864-0.20.1xx-1.patch addresses the comments in review.

          Testing: since it depends on system setup it's not completely automatic, the way I tested it I created a copy of TestAccessControlList and set it up to use JniBasedUnixGroupsNetgroupMapping for group mapping and set up /etc/netgroup accordingly (essentially same as what ShellBasedUnixGroupsNetgroupMappingTestWrapper.java returns).

          Note that /etc/nsswitch.conf must have line 'netgroup: files' in it for the settings in /etc/netgroup to be used.

          Show
          Erik Steffl added a comment - HADOOP-6864 -0.20.1xx-1.patch addresses the comments in review. Testing: since it depends on system setup it's not completely automatic, the way I tested it I created a copy of TestAccessControlList and set it up to use JniBasedUnixGroupsNetgroupMapping for group mapping and set up /etc/netgroup accordingly (essentially same as what ShellBasedUnixGroupsNetgroupMappingTestWrapper.java returns). Note that /etc/nsswitch.conf must have line 'netgroup: files' in it for the settings in /etc/netgroup to be used.
          Hide
          Boris Shkolnik added a comment -

          1. put a comment before (and other places, including C code)
          users = getUsersForNetgroupJNI(netgroup.substring(1));
          2. instead Arrays.asList(new String[0]); - just construct an empty list.
          3. Please put a disclaimer that JNI implementation returns only groups which appear in the ACL
          4. synchronize excess to the JNI functionality
          5. Please test valid and invalid cases (empty or non-existing groups)..
          6. Please test again

          otherwise +1

          Show
          Boris Shkolnik added a comment - 1. put a comment before (and other places, including C code) users = getUsersForNetgroupJNI(netgroup.substring(1)); 2. instead Arrays.asList(new String [0] ); - just construct an empty list. 3. Please put a disclaimer that JNI implementation returns only groups which appear in the ACL 4. synchronize excess to the JNI functionality 5. Please test valid and invalid cases (empty or non-existing groups).. 6. Please test again otherwise +1
          Erik Steffl made changes -
          Attachment HADOOP-6864-0.20.1xx.patch [ 12450231 ]
          Hide
          Erik Steffl added a comment -

          JNA is licenced under Lesser General Public License (LGPL v. 2.1) which is not allowed to be used in Apache projects, see http://www.apache.org/legal/resolved.html

          Show
          Erik Steffl added a comment - JNA is licenced under Lesser General Public License (LGPL v. 2.1) which is not allowed to be used in Apache projects, see http://www.apache.org/legal/resolved.html
          Jeff Hammerbacher made changes -
          Field Original Value New Value
          Link This issue is related to HADOOP-6818 [ HADOOP-6818 ]
          Hide
          Boris Shkolnik added a comment -

          please review the possibility of using JNA instead of JNI (https://jna.dev.java.net/).

          Show
          Boris Shkolnik added a comment - please review the possibility of using JNA instead of JNI ( https://jna.dev.java.net/ ).
          Erik Steffl created issue -

            People

            • Assignee:
              Boris Shkolnik
              Reporter:
              Erik Steffl
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development