Refactor environment variable processing for Docker (#50221)

Backport of #49612.

The current Docker entrypoint script picks up environment variables and
translates them into -E command line arguments. However, since any tool
executes via `docker exec` doesn't run the entrypoint, it results in
a poorer user experience.

Therefore, refactor the env var handling so that the -E options are
generated in `elasticsearch-env`. These have to be appended to any
existing command arguments, since some CLI tools have subcommands and
-E arguments must come after the subcommand.

Also extract the support for `_FILE` env vars into a separate script, so
that it can be called from more than once place (the behaviour is
idempotent).

Finally, add noop -E handling to CronEvalTool for parity, and support
`-E` in MultiCommand before subcommands.

distribution/docker/src/docker/Dockerfile

@@ -29,7 +29,7 @@ ${source_elasticsearch}
 
 RUN tar zxf /opt/${elasticsearch} --strip-components=1
 RUN grep ES_DISTRIBUTION_TYPE=tar /usr/share/elasticsearch/bin/elasticsearch-env \
-    && sed -ie 's/ES_DISTRIBUTION_TYPE=tar/ES_DISTRIBUTION_TYPE=docker/' /usr/share/elasticsearch/bin/elasticsearch-env
+    && sed -i -e 's/ES_DISTRIBUTION_TYPE=tar/ES_DISTRIBUTION_TYPE=docker/' /usr/share/elasticsearch/bin/elasticsearch-env
 RUN mkdir -p config data logs
 RUN chmod 0775 config data logs
 COPY config/elasticsearch.yml config/log4j2.properties config/
@@ -46,7 +46,7 @@ FROM ${base_image}
 ENV ELASTIC_CONTAINER true
 
 RUN for iter in {1..10}; do ${package_manager} update --setopt=tsflags=nodocs -y && \
-    ${package_manager} install --setopt=tsflags=nodocs -y nc shadow-utils && \
+    ${package_manager} install --setopt=tsflags=nodocs -y nc shadow-utils zip unzip && \
     ${package_manager} clean all && exit_code=0 && break || exit_code=\$? && echo "${package_manager} error: retry \$iter in 10s" && sleep 10; done; \
     (exit \$exit_code)
 

distribution/docker/src/docker/bin/docker-entrypoint.sh

@@ -42,71 +42,11 @@ fi
 # contents, and setting an environment variable with the suffix _FILE to
 # point to it. This can be used to provide secrets to a container, without
 # the values being specified explicitly when running the container.
-for VAR_NAME_FILE in $(env | cut -f1 -d= | grep '_FILE$'); do
-  if [[ -n "$VAR_NAME_FILE" ]]; then
-    VAR_NAME="${VAR_NAME_FILE%_FILE}"
-
-    if env | grep "^${VAR_NAME}="; then
-      echo "ERROR: Both $VAR_NAME_FILE and $VAR_NAME are set. These are mutually exclusive." >&2
-      exit 1
-    fi
-
-    if [[ ! -e "${!VAR_NAME_FILE}" ]]; then
-      echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE does not exist" >&2
-      exit 1
-    fi
-
-    FILE_PERMS="$(stat -c '%a' ${!VAR_NAME_FILE})"
-
-    if [[ "$FILE_PERMS" != "400" && "$FILE_PERMS" != 600 ]]; then
-        echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE must have file permissions 400 or 600, but actually has: $FILE_PERMS" >&2
-        exit 1
-    fi
-
-    echo "Setting $VAR_NAME from $VAR_NAME_FILE at ${!VAR_NAME_FILE}" >&2
-    export "$VAR_NAME"="$(cat ${!VAR_NAME_FILE})"
-
-    unset VAR_NAME
-    # Unset the suffixed environment variable
-    unset "$VAR_NAME_FILE"
-  fi
-done
-
-# Parse Docker env vars to customize Elasticsearch
 #
-# e.g. Setting the env var cluster.name=testcluster
+# This is also sourced in elasticsearch-env, and is only needed here
-#
+# as well because we use ELASTIC_PASSWORD below. Sourcing this script
-# will cause Elasticsearch to be invoked with -Ecluster.name=testcluster
+# is idempotent.
-#
+source /usr/share/elasticsearch/bin/elasticsearch-env-from-file
-# see https://www.elastic.co/guide/en/elasticsearch/reference/current/settings.html#_setting_default_settings
-
-declare -a es_opts
-
-while IFS='=' read -r envvar_key envvar_value
-do
-  # Elasticsearch settings need to have at least two dot separated lowercase
-  # words, e.g. `cluster.name`
-  if [[ "$envvar_key" =~ ^[a-z0-9_]+\.[a-z0-9_]+ ]]; then
-    if [[ ! -z $envvar_value ]]; then
-      es_opt="-E${envvar_key}=${envvar_value}"
-      es_opts+=("${es_opt}")
-    fi
-  fi
-done < <(env)
-
-# The virtual file /proc/self/cgroup should list the current cgroup
-# membership. For each hierarchy, you can follow the cgroup path from
-# this file to the cgroup filesystem (usually /sys/fs/cgroup/) and
-# introspect the statistics for the cgroup for the given
-# hierarchy. Alas, Docker breaks this by mounting the container
-# statistics at the root while leaving the cgroup paths as the actual
-# paths. Therefore, Elasticsearch provides a mechanism to override
-# reading the cgroup path from /proc/self/cgroup and instead uses the
-# cgroup path defined the JVM system property
-# es.cgroups.hierarchy.override. Therefore, we set this value here so
-# that cgroup statistics are available for the container this process
-# will run in.
-export ES_JAVA_OPTS="-Des.cgroups.hierarchy.override=/ $ES_JAVA_OPTS"
 
 if [[ -f bin/elasticsearch-users ]]; then
   # Check for the ELASTIC_PASSWORD environment variable to set the
@@ -130,4 +70,4 @@ if [[ "$(id -u)" == "0" ]]; then
   fi
 fi
 
-run_as_other_user_if_needed /usr/share/elasticsearch/bin/elasticsearch "${es_opts[@]}"
+run_as_other_user_if_needed /usr/share/elasticsearch/bin/elasticsearch

distribution/src/bin/elasticsearch-env

@@ -86,4 +86,51 @@ ES_DISTRIBUTION_FLAVOR=${es.distribution.flavor}
 ES_DISTRIBUTION_TYPE=${es.distribution.type}
 ES_BUNDLED_JDK=${es.bundled_jdk}
 
+if [[ "$ES_DISTRIBUTION_TYPE" == "docker" ]]; then
+  # Allow environment variables to be set by creating a file with the
+  # contents, and setting an environment variable with the suffix _FILE to
+  # point to it. This can be used to provide secrets to a container, without
+  # the values being specified explicitly when running the container.
+  source "$ES_HOME/bin/elasticsearch-env-from-file"
+
+  # Parse Docker env vars to customize Elasticsearch
+  #
+  # e.g. Setting the env var cluster.name=testcluster
+  #
+  # will cause Elasticsearch to be invoked with -Ecluster.name=testcluster
+  #
+  # see https://www.elastic.co/guide/en/elasticsearch/reference/current/settings.html#_setting_default_settings
+
+  declare -a es_arg_array
+
+  while IFS='=' read -r envvar_key envvar_value
+  do
+    # Elasticsearch settings need to have at least two dot separated lowercase
+    # words, e.g. `cluster.name`
+    if [[ "$envvar_key" =~ ^[a-z0-9_]+\.[a-z0-9_]+ ]]; then
+      if [[ ! -z $envvar_value ]]; then
+        es_opt="-E${envvar_key}=${envvar_value}"
+        es_arg_array+=("${es_opt}")
+      fi
+    fi
+  done < <(env)
+
+  # Reset the positional parameters to the es_arg_array values and any existing positional params
+  set -- "$@" "${es_arg_array[@]}"
+
+  # The virtual file /proc/self/cgroup should list the current cgroup
+  # membership. For each hierarchy, you can follow the cgroup path from
+  # this file to the cgroup filesystem (usually /sys/fs/cgroup/) and
+  # introspect the statistics for the cgroup for the given
+  # hierarchy. Alas, Docker breaks this by mounting the container
+  # statistics at the root while leaving the cgroup paths as the actual
+  # paths. Therefore, Elasticsearch provides a mechanism to override
+  # reading the cgroup path from /proc/self/cgroup and instead uses the
+  # cgroup path defined the JVM system property
+  # es.cgroups.hierarchy.override. Therefore, we set this value here so
+  # that cgroup statistics are available for the container this process
+  # will run in.
+  export ES_JAVA_OPTS="-Des.cgroups.hierarchy.override=/ $ES_JAVA_OPTS"
+fi
+
 cd "$ES_HOME"

distribution/src/bin/elasticsearch-env-from-file

@@ -0,0 +1,42 @@
+#!/bin/bash
+
+set -e -o pipefail
+
+# Allow environment variables to be set by creating a file with the
+# contents, and setting an environment variable with the suffix _FILE to
+# point to it. This can be used to provide secrets to a container, without
+# the values being specified explicitly when running the container.
+#
+# This script is intended to be sourced, not executed, and modifies the
+# environment.
+
+for VAR_NAME_FILE in $(env | cut -f1 -d= | grep '_FILE$'); do
+  if [[ -n "$VAR_NAME_FILE" ]]; then
+    VAR_NAME="${VAR_NAME_FILE%_FILE}"
+
+    if env | grep "^${VAR_NAME}="; then
+      echo "ERROR: Both $VAR_NAME_FILE and $VAR_NAME are set. These are mutually exclusive." >&2
+      exit 1
+    fi
+
+    if [[ ! -e "${!VAR_NAME_FILE}" ]]; then
+      echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE does not exist" >&2
+      exit 1
+    fi
+
+    FILE_PERMS="$(stat -c '%a' ${!VAR_NAME_FILE})"
+
+    if [[ "$FILE_PERMS" != "400" && "$FILE_PERMS" != 600 ]]; then
+        echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE must have file permissions 400 or 600, but actually has: $FILE_PERMS" >&2
+        exit 1
+    fi
+
+    echo "Setting $VAR_NAME from $VAR_NAME_FILE at ${!VAR_NAME_FILE}" >&2
+    export "$VAR_NAME"="$(cat ${!VAR_NAME_FILE})"
+
+    unset VAR_NAME
+    # Unset the suffixed environment variable
+    unset "$VAR_NAME_FILE"
+  fi
+done
+

libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java

@@ -21,11 +21,14 @@ package org.elasticsearch.cli;
 
 import joptsimple.NonOptionArgumentSpec;
 import joptsimple.OptionSet;
+import joptsimple.OptionSpec;
+import joptsimple.util.KeyValuePair;
 import org.elasticsearch.core.internal.io.IOUtils;
 
 import java.io.IOException;
-import java.util.Arrays;
+import java.util.ArrayList;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -36,6 +39,7 @@ public class MultiCommand extends Command {
     protected final Map<String, Command> subcommands = new LinkedHashMap<>();
 
     private final NonOptionArgumentSpec<String> arguments = parser.nonOptions("command");
+    private final OptionSpec<KeyValuePair> settingOption;
 
     /**
      * Construct the multi-command with the specified command description and runnable to execute before main is invoked.
@@ -45,6 +49,7 @@ public class MultiCommand extends Command {
      */
     public MultiCommand(final String description, final Runnable beforeMain) {
         super(description, beforeMain);
+        this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
         parser.posixlyCorrect(true);
     }
 
@@ -66,15 +71,24 @@ public class MultiCommand extends Command {
         if (subcommands.isEmpty()) {
             throw new IllegalStateException("No subcommands configured");
         }
-        String[] args = arguments.values(options).toArray(new String[0]);
+
-        if (args.length == 0) {
+        // .values(...) returns an unmodifiable list
+        final List<String> args = new ArrayList<>(arguments.values(options));
+        if (args.isEmpty()) {
             throw new UserException(ExitCodes.USAGE, "Missing command");
         }
-        Command subcommand = subcommands.get(args[0]);
+
+        String subcommandName = args.remove(0);
+        Command subcommand = subcommands.get(subcommandName);
         if (subcommand == null) {
-            throw new UserException(ExitCodes.USAGE, "Unknown command [" + args[0] + "]");
+            throw new UserException(ExitCodes.USAGE, "Unknown command [" + subcommandName + "]");
         }
-        subcommand.mainWithoutErrorHandling(Arrays.copyOfRange(args, 1, args.length), terminal);
+
+        for (final KeyValuePair pair : this.settingOption.values(options)) {
+            args.add("-E" + pair);
+        }
+
+        subcommand.mainWithoutErrorHandling(args.toArray(new String[0]), terminal);
     }
 
     @Override

qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java

@@ -19,31 +19,6 @@
 
 package org.elasticsearch.packaging.test;
 
-import com.fasterxml.jackson.databind.JsonNode;
-import org.apache.http.client.fluent.Request;
-import org.elasticsearch.packaging.util.Distribution;
-import org.elasticsearch.packaging.util.Docker.DockerShell;
-import org.elasticsearch.packaging.util.Installation;
-import org.elasticsearch.packaging.util.Platforms;
-import org.elasticsearch.packaging.util.ServerUtils;
-import org.elasticsearch.packaging.util.Shell.Result;
-import org.junit.After;
-import org.junit.AfterClass;
-import org.junit.Before;
-import org.junit.BeforeClass;
-
-import java.io.IOException;
-import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.Paths;
-import java.util.Arrays;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-import java.util.stream.Collectors;
-
 import static java.nio.file.attribute.PosixFilePermissions.fromString;
 import static java.util.Collections.singletonMap;
 import static org.elasticsearch.packaging.util.Docker.assertPermissionsAndOwnership;
@@ -79,6 +54,33 @@ import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assume.assumeTrue;
 
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.http.client.fluent.Request;
+import org.elasticsearch.packaging.util.Distribution;
+import org.elasticsearch.packaging.util.Docker.DockerShell;
+import org.elasticsearch.packaging.util.Installation;
+import org.elasticsearch.packaging.util.Platforms;
+import org.elasticsearch.packaging.util.ServerUtils;
+import org.elasticsearch.packaging.util.Shell.Result;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+
+import com.fasterxml.jackson.databind.JsonNode;
+
 public class DockerTests extends PackagingTestCase {
     protected DockerShell sh;
     private Path tempDir;
@@ -391,6 +393,27 @@ public class DockerTests extends PackagingTestCase {
         );
     }
 
+    /**
+     * Check that environment variables are translated to -E options even for commands invoked under
+     * `docker exec`, where the Docker image's entrypoint is not executed.
+     */
+    public void test83EnvironmentVariablesAreRespectedUnderDockerExec() {
+        // This test relies on a CLI tool attempting to connect to Elasticsearch, and the
+        // tool in question is only in the default distribution.
+        assumeTrue(distribution.isDefault());
+
+        runContainer(distribution(), null, Collections.singletonMap("http.host", "this.is.not.valid"));
+
+        // This will fail if the env var above is passed as a -E argument
+        final Result result = sh.runIgnoreExitCode("elasticsearch-setup-passwords auto");
+
+        assertFalse("elasticsearch-setup-passwords command should have failed", result.isSuccess());
+        assertThat(
+            result.stdout,
+            containsString("java.net.UnknownHostException: this.is.not.valid: Name or service not known")
+        );
+    }
+
     /**
      * Check whether the elasticsearch-certutil tool has been shipped correctly,
      * and if present then it can execute.

qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java

@@ -46,10 +46,11 @@ import static org.elasticsearch.packaging.util.FileMatcher.p770;
 import static org.elasticsearch.packaging.util.FileMatcher.p775;
 import static org.elasticsearch.packaging.util.FileUtils.getCurrentVersion;
 import static org.elasticsearch.packaging.util.ServerUtils.makeRequest;
-import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 /**
@@ -276,7 +277,7 @@ public class Docker {
         protected String[] getScriptCommand(String script) {
             assert containerId != null;
 
-            return super.getScriptCommand("docker exec " + "--user elasticsearch:root " + "--tty " + containerId + " " + script);
+            return super.getScriptCommand("docker exec --user elasticsearch:root --tty " + containerId + " " + script);
         }
     }
 
@@ -438,7 +439,6 @@ public class Docker {
             "elasticsearch",
             "elasticsearch-cli",
             "elasticsearch-env",
-            "elasticsearch-enve",
             "elasticsearch-keystore",
             "elasticsearch-node",
             "elasticsearch-plugin",
@@ -446,6 +446,14 @@ public class Docker {
         ).forEach(executable -> assertPermissionsAndOwnership(es.bin(executable), p755));
 
         Stream.of("LICENSE.txt", "NOTICE.txt", "README.textile").forEach(doc -> assertPermissionsAndOwnership(es.home.resolve(doc), p644));
+
+        // These are installed to help users who are working with certificates.
+        Stream.of("zip", "unzip").forEach(cliPackage -> {
+            // We could run `yum list installed $pkg` but that causes yum to call out to the network.
+            // rpm does the job just as well.
+            final Shell.Result result = dockerShell.runIgnoreExitCode("rpm -q " + cliPackage);
+            assertTrue(cliPackage + " ought to be installed. " + result, result.isSuccess());
+        });
     }
 
     private static void verifyDefaultInstallation(Installation es) {

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

@@ -19,12 +19,17 @@
 
 package org.elasticsearch.cli;
 
+import joptsimple.ArgumentAcceptingOptionSpec;
 import joptsimple.OptionSet;
+import joptsimple.util.KeyValuePair;
 import org.junit.Before;
 
 import java.io.IOException;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static org.hamcrest.Matchers.containsString;
+
 public class MultiCommandTests extends CommandTestCase {
 
     static class DummyMultiCommand extends MultiCommand {
@@ -32,8 +37,7 @@ public class MultiCommandTests extends CommandTestCase {
         final AtomicBoolean closed = new AtomicBoolean();
 
         DummyMultiCommand() {
-            super("A dummy multi command", () -> {
+            super("A dummy multi command", () -> {});
-            });
         }
 
         @Override
@@ -75,7 +79,23 @@ public class MultiCommandTests extends CommandTestCase {
         }
     }
 
-    DummyMultiCommand multiCommand;
+    static class DummySettingsSubCommand extends DummySubCommand {
+        private final ArgumentAcceptingOptionSpec<KeyValuePair> settingOption;
+
+        DummySettingsSubCommand() {
+            super();
+            this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class);
+        }
+
+        @Override
+        protected void execute(Terminal terminal, OptionSet options) throws Exception {
+            final List<KeyValuePair> values = this.settingOption.values(options);
+            terminal.println("Settings: " + values);
+            super.execute(terminal, options);
+        }
+    }
+
+    private DummyMultiCommand multiCommand;
 
     @Before
     public void setupCommand() {
@@ -87,27 +107,21 @@ public class MultiCommandTests extends CommandTestCase {
         return multiCommand;
     }
 
-    public void testNoCommandsConfigured() throws Exception {
+    public void testNoCommandsConfigured() {
-        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+        IllegalStateException e = expectThrows(IllegalStateException.class, this::execute);
-            execute();
-        });
         assertEquals("No subcommands configured", e.getMessage());
     }
 
-    public void testUnknownCommand() throws Exception {
+    public void testUnknownCommand() {
         multiCommand.subcommands.put("something", new DummySubCommand());
-        UserException e = expectThrows(UserException.class, () -> {
+        UserException e = expectThrows(UserException.class, () -> execute("somethingelse"));
-            execute("somethingelse");
-        });
         assertEquals(ExitCodes.USAGE, e.exitCode);
         assertEquals("Unknown command [somethingelse]", e.getMessage());
     }
 
-    public void testMissingCommand() throws Exception {
+    public void testMissingCommand() {
         multiCommand.subcommands.put("command1", new DummySubCommand());
-        UserException e = expectThrows(UserException.class, () -> {
+        UserException e = expectThrows(UserException.class, this::execute);
-            execute();
-        });
         assertEquals(ExitCodes.USAGE, e.exitCode);
         assertEquals("Missing command", e.getMessage());
     }
@@ -121,6 +135,19 @@ public class MultiCommandTests extends CommandTestCase {
         assertTrue(output, output.contains("command2"));
     }
 
+    /**
+     * Check that if -E arguments are passed to the main command, then they are accepted
+     * and passed on to the subcommand.
+     */
+    public void testSettingsOnMainCommand() throws Exception {
+        multiCommand.subcommands.put("command1", new DummySettingsSubCommand());
+        execute("-Esetting1=value1", "-Esetting2=value2", "command1", "otherArg");
+
+        String output = terminal.getOutput();
+        assertThat(output, containsString("Settings: [setting1=value1, setting2=value2]"));
+        assertThat(output, containsString("Arguments: [otherArg]"));
+    }
+
     public void testSubcommandHelp() throws Exception {
         multiCommand.subcommands.put("command1", new DummySubCommand());
         multiCommand.subcommands.put("command2", new DummySubCommand());

x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc

@@ -71,7 +71,6 @@ services:
     image: {docker-image}
     command: >
       bash -c '
-        yum install -y -q -e 0 unzip;
         if [[ ! -f /certs/bundle.zip ]]; then
           bin/elasticsearch-certutil cert --silent --pem --in config/certificates/instances.yml -out /certs/bundle.zip;
           unzip /certs/bundle.zip -d /certs; <1>
@@ -206,9 +205,6 @@ WARNING: Windows users not running PowerShell will need to remove `\` and join l
 ----
 docker exec es01 /bin/bash -c "bin/elasticsearch-setup-passwords \
 auto --batch \
--Expack.security.http.ssl.certificate=certificates/es01/es01.crt \
--Expack.security.http.ssl.certificate_authorities=certificates/ca/ca.crt \
--Expack.security.http.ssl.key=certificates/es01/es01.key \
 --url https://localhost:9200"
 ----
 --

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java

@@ -44,6 +44,8 @@ public class CronEvalTool extends LoggingAwareCommand {
             "The number of future times this expression will be triggered")
             .withRequiredArg().ofType(Integer.class).defaultsTo(10);
         this.arguments = parser.nonOptions("expression");
+
+        parser.accepts("E", "Unused. Only for compatibility with other CLI tools.").withRequiredArg();
     }
 
     @Override
@@ -56,7 +58,7 @@ public class CronEvalTool extends LoggingAwareCommand {
         execute(terminal, args.get(0), count);
     }
 
-    void execute(Terminal terminal, String expression, int count) throws Exception {
+    private void execute(Terminal terminal, String expression, int count) throws Exception {
         Cron.validate(expression);
         terminal.println("Valid!");