HADOOP-13817. Add a finite shell command timeout to ShellBasedUnixGroupsMapping.

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java

@@ -517,6 +517,21 @@ public class CommonConfigurationKeysPublic {
    * <a href="{@docRoot}/../hadoop-project-dist/hadoop-common/core-default.xml">
    * core-default.xml</a>
    */
+  public static final String HADOOP_SECURITY_GROUP_SHELL_COMMAND_TIMEOUT =
+      "hadoop.security.groups.shell.groups.command.timeout";
+  /**
+   * @see
+   * <a href="{@docRoot}/../hadoop-project-dist/hadoop-common/core-default.xml">
+   * core-default.xml</a>
+   */
+  public static final long
+      HADOOP_SECURITY_GROUP_SHELL_COMMAND_TIMEOUT_DEFAULT =
+          0L;
+  /**
+   * @see
+   * <a href="{@docRoot}/../hadoop-project-dist/hadoop-common/core-default.xml">
+   * core-default.xml</a>
+   */
   public static final String  HADOOP_SECURITY_AUTHENTICATION =
     "hadoop.security.authentication";
   /**

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/ShellBasedUnixGroupsMapping.java

@@ -18,17 +18,23 @@
 package org.apache.hadoop.security;
 
 import java.io.IOException;
+import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.StringTokenizer;
+
+import com.google.common.annotations.VisibleForTesting;
 import org.apache.commons.lang.StringUtils;
-import org.apache.commons.logging.Log;
-import org.apache.commons.logging.LogFactory;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.conf.Configured;
+import org.apache.hadoop.fs.CommonConfigurationKeys;
 import org.apache.hadoop.util.Shell;
 import org.apache.hadoop.util.Shell.ExitCodeException;
 import org.apache.hadoop.util.Shell.ShellCommandExecutor;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * A simple shell-based implementation of {@link GroupMappingServiceProvider} 
@@ -37,11 +43,23 @@
  */
 @InterfaceAudience.LimitedPrivate({"HDFS", "MapReduce"})
 @InterfaceStability.Evolving
-public class ShellBasedUnixGroupsMapping
+public class ShellBasedUnixGroupsMapping extends Configured
   implements GroupMappingServiceProvider {
-
-  private static final Log LOG =
-    LogFactory.getLog(ShellBasedUnixGroupsMapping.class);
+
+  @VisibleForTesting
+  protected static final Logger LOG =
+      LoggerFactory.getLogger(ShellBasedUnixGroupsMapping.class);
+
+  private long timeout = 0L;
+
+  @Override
+  public void setConf(Configuration conf) {
+    super.setConf(conf);
+    timeout = conf.getLong(
+        CommonConfigurationKeys.HADOOP_SECURITY_GROUP_SHELL_COMMAND_TIMEOUT,
+        CommonConfigurationKeys.
+            HADOOP_SECURITY_GROUP_SHELL_COMMAND_TIMEOUT_DEFAULT);
+  }
 
   @SuppressWarnings("serial")
   private static class PartialGroupNameException extends IOException {
@@ -98,7 +116,17 @@ public void cacheGroupsAdd(List<String> groups) throws IOException {
    */
   protected ShellCommandExecutor createGroupExecutor(String userName) {
     return new ShellCommandExecutor(
-        Shell.getGroupsForUserCommand(userName), null, null, 0L);
+        getGroupsForUserCommand(userName), null, null, timeout);
+  }
+
+  /**
+   * Returns just the shell command to be used to fetch a user's groups list.
+   * This is mainly separate to make some tests easier.
+   * @param userName The username that needs to be passed into the command built
+   * @return An appropriate shell command with arguments
+   */
+  protected String[] getGroupsForUserCommand(String userName) {
+    return Shell.getGroupsForUserCommand(userName);
   }
 
   /**
@@ -109,7 +137,17 @@ protected ShellCommandExecutor createGroupExecutor(String userName) {
    */
   protected ShellCommandExecutor createGroupIDExecutor(String userName) {
     return new ShellCommandExecutor(
-        Shell.getGroupsIDForUserCommand(userName), null, null, 0L);
+        getGroupsIDForUserCommand(userName), null, null, timeout);
+  }
+
+  /**
+   * Returns just the shell command to be used to fetch a user's group IDs list.
+   * This is mainly separate to make some tests easier.
+   * @param userName The username that needs to be passed into the command built
+   * @return An appropriate shell command with arguments
+   */
+  protected String[] getGroupsIDForUserCommand(String userName) {
+    return Shell.getGroupsIDForUserCommand(userName);
   }
 
   /**
@@ -133,8 +171,26 @@ private List<String> getUnixGroups(String user) throws IOException {
         groups = resolvePartialGroupNames(user, e.getMessage(),
             executor.getOutput());
       } catch (PartialGroupNameException pge) {
-        LOG.warn("unable to return groups for user " + user, pge);
+        LOG.warn("unable to return groups for user {}", user, pge);
+        return new LinkedList<>();
+      }
+    } catch (IOException ioe) {
+      // If its a shell executor timeout, indicate so in the message
+      // but treat the result as empty instead of throwing it up,
+      // similar to how partial resolution failures are handled above
+      if (executor.isTimedOut()) {
+        LOG.warn(
+            "Unable to return groups for user '{}' as shell group lookup " +
+            "command '{}' ran longer than the configured timeout limit of " +
+            "{} seconds.",
+            user,
+            Arrays.asList(executor.getExecString()),
+            timeout
+        );
         return new LinkedList<>();
+      } else {
+        // If its not an executor timeout, we should let the caller handle it
+        throw ioe;
       }
     }
 
@@ -212,11 +268,11 @@ private List<String> resolvePartialGroupNames(String userName,
       throw new PartialGroupNameException("The user name '" + userName
           + "' is not found. " + errMessage);
     } else {
-      LOG.warn("Some group names for '" + userName + "' are not resolvable. "
-          + errMessage);
+      LOG.warn("Some group names for '{}' are not resolvable. {}",
+          userName, errMessage);
       // attempt to partially resolve group names
+      ShellCommandExecutor exec2 = createGroupIDExecutor(userName);
       try {
-        ShellCommandExecutor exec2 = createGroupIDExecutor(userName);
         exec2.execute();
         return parsePartialGroupNames(groupNames, exec2.getOutput());
       } catch (ExitCodeException ece) {
@@ -225,8 +281,16 @@ private List<String> resolvePartialGroupNames(String userName,
         throw new PartialGroupNameException("failed to get group id list for " +
         "user '" + userName + "'", ece);
       } catch (IOException ioe) {
-        throw new PartialGroupNameException("can't execute the shell command to"
-        + " get the list of group id for user '" + userName + "'", ioe);
+        String message =
+            "Can't execute the shell command to " +
+            "get the list of group id for user '" + userName + "'";
+        if (exec2.isTimedOut()) {
+          message +=
+              " because of the command taking longer than " +
+              "the configured timeout: " + timeout + " seconds";
+          throw new PartialGroupNameException(message);
+        }
+        throw new PartialGroupNameException(message, ioe);
       }
     }
   }
@@ -237,7 +301,8 @@ private List<String> resolvePartialGroupNames(String userName,
    * @param groupNames a string representing the user's group names
    * @return a linked list of group names
    */
-  private List<String> resolveFullGroupNames(String groupNames) {
+  @VisibleForTesting
+  protected List<String> resolveFullGroupNames(String groupNames) {
     StringTokenizer tokenizer =
         new StringTokenizer(groupNames, Shell.TOKEN_SEPARATOR_REGEX);
     List<String> groups = new LinkedList<String>();

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/Shell.java

@@ -945,7 +945,12 @@ public void run() {
             line = errReader.readLine();
           }
         } catch(IOException ioe) {
-          LOG.warn("Error reading the error stream", ioe);
+          // Its normal to observe a "Stream closed" I/O error on
+          // command timeouts destroying the underlying process
+          // so only log a WARN if the command didn't time out
+          if (!isTimedOut()) {
+            LOG.warn("Error reading the error stream", ioe);
+          }
         }
       }
     };
@@ -1159,6 +1164,15 @@ public ShellCommandExecutor(String[] execString, File dir,
       this.inheritParentEnv = inheritParentEnv;
     }
 
+    /**
+     * Returns the timeout value set for the executor's sub-commands.
+     * @return The timeout value in seconds
+     */
+    @VisibleForTesting
+    public long getTimeoutInterval() {
+      return timeOutInterval;
+    }
+
     /**
      * Execute the shell command.
      * @throws IOException if the command fails, or if the command is

hadoop-common-project/hadoop-common/src/main/resources/core-default.xml

@@ -187,6 +187,19 @@
   </description>
 </property>
 
+<property>
+  <name>hadoop.security.groups.shell.groups.command.timeout</name>
+  <value>0</value>
+  <description>
+    Used by the ShellBasedUnixGroupsMapping class, this property controls how
+    long to wait for the underlying shell command that is run to fetch groups.
+    Expressed in seconds, if the running command takes longer than the value
+    configured, the command is aborted and the groups resolver would return a
+    result of no groups found. A value of 0 (default) would mean an infinite
+    wait (i.e. wait until the command exits on its own).
+  </description>
+</property>
+
 <property>
   <name>hadoop.security.group.mapping.ldap.connection.timeout.ms</name>
   <value>60000</value>

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/TestGroupsCaching.java

@@ -50,7 +50,7 @@
 
 
 public class TestGroupsCaching {
-  public static final Log LOG = LogFactory.getLog(TestGroupsCaching.class);
+  public static final Log TESTLOG = LogFactory.getLog(TestGroupsCaching.class);
   private static String[] myGroups = {"grp1", "grp2"};
   private Configuration conf;
 
@@ -76,7 +76,7 @@ public static class FakeGroupMapping extends ShellBasedUnixGroupsMapping {
 
     @Override
     public List<String> getGroups(String user) throws IOException {
-      LOG.info("Getting groups for " + user);
+      TESTLOG.info("Getting groups for " + user);
       delayIfNecessary();
 
       requestCount++;
@@ -115,18 +115,18 @@ private void delayIfNecessary() {
 
     @Override
     public void cacheGroupsRefresh() throws IOException {
-      LOG.info("Cache is being refreshed.");
+      TESTLOG.info("Cache is being refreshed.");
       clearBlackList();
       return;
     }
 
     public static void clearBlackList() throws IOException {
-      LOG.info("Clearing the blacklist");
+      TESTLOG.info("Clearing the blacklist");
       blackList.clear();
     }
 
     public static void clearAll() throws IOException {
-      LOG.info("Resetting FakeGroupMapping");
+      TESTLOG.info("Resetting FakeGroupMapping");
       blackList.clear();
       allGroups.clear();
       requestCount = 0;
@@ -137,12 +137,12 @@ public static void clearAll() throws IOException {
 
     @Override
     public void cacheGroupsAdd(List<String> groups) throws IOException {
-      LOG.info("Adding " + groups + " to groups.");
+      TESTLOG.info("Adding " + groups + " to groups.");
       allGroups.addAll(groups);
     }
 
     public static void addToBlackList(String user) throws IOException {
-      LOG.info("Adding " + user + " to the blacklist");
+      TESTLOG.info("Adding " + user + " to the blacklist");
       blackList.add(user);
     }
 
@@ -226,11 +226,12 @@ public void testGroupsCaching() throws Exception {
 
     // ask for a negative entry
     try {
-      LOG.error("We are not supposed to get here." + groups.getGroups("user1").toString());
+      TESTLOG.error("We are not supposed to get here."
+          + groups.getGroups("user1").toString());
       fail();
     } catch (IOException ioe) {
       if(!ioe.getMessage().startsWith("No groups found")) {
-        LOG.error("Got unexpected exception: " + ioe.getMessage());
+        TESTLOG.error("Got unexpected exception: " + ioe.getMessage());
         fail();
       }
     }