From 133ebf224ad12e98cfec5b6075eb3056c18d3a2c Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 22 Nov 2019 09:57:35 +0000 Subject: [PATCH 01/13] Refactor env var processing Closes #45223. 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, change `CronEvalTool` to extend `EnvironmentAwareCommand`, so that it can allow (but ignore) -E commands. This also brings the tool in line with our other CLI tools. --- distribution/docker/src/docker/Dockerfile | 2 +- .../src/docker/bin/docker-entrypoint.sh | 57 ++----------------- distribution/src/bin/elasticsearch-env | 34 +++++++++++ .../src/bin/elasticsearch-env-from-file | 42 ++++++++++++++ .../packaging/test/DockerTests.java | 17 ++++++ .../trigger/schedule/tool/CronEvalTool.java | 17 ++++-- 6 files changed, 112 insertions(+), 57 deletions(-) create mode 100644 distribution/src/bin/elasticsearch-env-from-file diff --git a/distribution/docker/src/docker/Dockerfile b/distribution/docker/src/docker/Dockerfile index 0b36b29d8e22a..408f4c567b4eb 100644 --- a/distribution/docker/src/docker/Dockerfile +++ b/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/ diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index d95c451dead9d..11774ae6878cf 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -42,58 +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 -# -# 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_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`, except for `processors` which we handle - # specially - 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) +# This is also sourced in elasticsearch-env, and is only needed here +# as well because we use ELASTIC_PASSWORD below. Sourcing this script +# is idempotent. +source /usr/share/elasticsearch/bin/elasticsearch-env-from-file # The virtual file /proc/self/cgroup should list the current cgroup # membership. For each hierarchy, you can follow the cgroup path from @@ -131,4 +84,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 diff --git a/distribution/src/bin/elasticsearch-env b/distribution/src/bin/elasticsearch-env index 867e19b3edd6b..33a755a1e84c8 100644 --- a/distribution/src/bin/elasticsearch-env +++ b/distribution/src/bin/elasticsearch-env @@ -86,4 +86,38 @@ 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`, except for `processors` which we handle + # specially + 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[@]}" +fi + cd "$ES_HOME" diff --git a/distribution/src/bin/elasticsearch-env-from-file b/distribution/src/bin/elasticsearch-env-from-file new file mode 100644 index 0000000000000..fd5326afcc6b7 --- /dev/null +++ b/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 + diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 997d4b9758684..f3f0fc42c8e6a 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -306,6 +306,23 @@ public void test82EnvironmentVariablesUsingFilesHaveCorrectPermissions() throws ); } + /** + * 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() { + runContainer(distribution(), null, Map.of("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. diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index 28b2a363ceba3..367234102de3a 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -7,11 +7,12 @@ import joptsimple.OptionSet; import joptsimple.OptionSpec; +import org.elasticsearch.cli.EnvironmentAwareCommand; import org.elasticsearch.cli.ExitCodes; -import org.elasticsearch.cli.LoggingAwareCommand; import org.elasticsearch.cli.Terminal; import org.elasticsearch.cli.UserException; import org.elasticsearch.common.time.DateFormatter; +import org.elasticsearch.env.Environment; import org.elasticsearch.xpack.core.scheduler.Cron; import java.time.Instant; @@ -22,7 +23,12 @@ import java.util.List; import java.util.Locale; -public class CronEvalTool extends LoggingAwareCommand { +/* + * This class extends EnvironmentAwareCommand so that -E options are accepted, + * even though they are unused. This makes the CLI interface the same as the + * other CLI tools. + */ +public class CronEvalTool extends EnvironmentAwareCommand { public static void main(String[] args) throws Exception { exit(new CronEvalTool().main(args, Terminal.DEFAULT)); @@ -46,8 +52,11 @@ public static void main(String[] args) throws Exception { this.arguments = parser.nonOptions("expression"); } + /* + * env is not used here, but is part of the EnvironmentAwareCommand contract. + */ @Override - protected void execute(Terminal terminal, OptionSet options) throws Exception { + protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception { int count = countOption.value(options); List args = arguments.values(options); if (args.size() != 1) { @@ -56,7 +65,7 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception { 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!"); From 2261af3597f42a7afe80af74677b45b58ef8c91d Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 27 Nov 2019 09:38:50 +0000 Subject: [PATCH 02/13] Remove outdated comment --- distribution/src/bin/elasticsearch-env | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/distribution/src/bin/elasticsearch-env b/distribution/src/bin/elasticsearch-env index 33a755a1e84c8..64a5668ab9ffe 100644 --- a/distribution/src/bin/elasticsearch-env +++ b/distribution/src/bin/elasticsearch-env @@ -106,8 +106,7 @@ if [[ "$ES_DISTRIBUTION_TYPE" == "docker" ]]; then 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`, except for `processors` which we handle - # specially + # 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}" From f92910589fccc1d177041ed3fceb61f5587ed52a Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 27 Nov 2019 11:58:00 +0000 Subject: [PATCH 03/13] Revert CronEvalTool superclass change Changing `CronEvalTool` to extend `EnvironmentAwareCommand` broke the unit tests, and after further reflection I decided that this change was lazy. Actually, the tool should just accept `-E` params but do nothing with them, instead of pulling in all the reset of the env code. --- .../trigger/schedule/tool/CronEvalTool.java | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index 367234102de3a..be1b12b9e4f2d 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -7,12 +7,12 @@ import joptsimple.OptionSet; import joptsimple.OptionSpec; -import org.elasticsearch.cli.EnvironmentAwareCommand; +import joptsimple.util.KeyValuePair; import org.elasticsearch.cli.ExitCodes; +import org.elasticsearch.cli.LoggingAwareCommand; import org.elasticsearch.cli.Terminal; import org.elasticsearch.cli.UserException; import org.elasticsearch.common.time.DateFormatter; -import org.elasticsearch.env.Environment; import org.elasticsearch.xpack.core.scheduler.Cron; import java.time.Instant; @@ -23,12 +23,7 @@ import java.util.List; import java.util.Locale; -/* - * This class extends EnvironmentAwareCommand so that -E options are accepted, - * even though they are unused. This makes the CLI interface the same as the - * other CLI tools. - */ -public class CronEvalTool extends EnvironmentAwareCommand { +public class CronEvalTool extends LoggingAwareCommand { public static void main(String[] args) throws Exception { exit(new CronEvalTool().main(args, Terminal.DEFAULT)); @@ -50,13 +45,12 @@ public static void main(String[] args) throws Exception { "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().ofType(KeyValuePair.class); } - /* - * env is not used here, but is part of the EnvironmentAwareCommand contract. - */ @Override - protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception { + protected void execute(Terminal terminal, OptionSet options) throws Exception { int count = countOption.value(options); List args = arguments.values(options); if (args.size() != 1) { From 3f7d28711c59aa93c5a05226e356beeb0810ad36 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 27 Nov 2019 15:57:07 +0000 Subject: [PATCH 04/13] Fix test after removing elasticsearch-enve --- qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java | 1 - 1 file changed, 1 deletion(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index 8489ef42ea83c..8628d922fdab0 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -364,7 +364,6 @@ private static void verifyOssInstallation(Installation es) { "elasticsearch", "elasticsearch-cli", "elasticsearch-env", - "elasticsearch-enve", "elasticsearch-keystore", "elasticsearch-node", "elasticsearch-plugin", From fcec60d1f1c07df87de2d1266a264d469bc20a59 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 28 Nov 2019 14:23:07 +0000 Subject: [PATCH 05/13] Allow -E in MultiCommand tools --- libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java | 3 +++ .../xpack/watcher/trigger/schedule/tool/CronEvalTool.java | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java b/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java index bcc75a2d1be12..7e1bcf35067be 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java @@ -45,6 +45,9 @@ public class MultiCommand extends Command { */ public MultiCommand(final String description, final Runnable beforeMain) { super(description, beforeMain); + // Accepting -E here does not prevent subcommands receiving -E arguments, since we also set `posixlyCorrect` below. + // This stops option parsing when the first non-option is encountered. + parser.accepts("E", "Unused. Pass to a subcommand instead").withRequiredArg(); parser.posixlyCorrect(true); } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index be1b12b9e4f2d..0573b33d28df1 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -46,7 +46,7 @@ public static void main(String[] args) throws Exception { .withRequiredArg().ofType(Integer.class).defaultsTo(10); this.arguments = parser.nonOptions("expression"); - parser.accepts("E", "Unused. Only for compatibility with other CLI tools.").withRequiredArg().ofType(KeyValuePair.class); + parser.accepts("E", "Unused. Only for compatibility with other CLI tools.").withRequiredArg(); } @Override From 31e4679e35a311e700e79b9b937420546e104901 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 29 Nov 2019 10:27:33 +0000 Subject: [PATCH 06/13] Pass along -E arguments to subcommands Having MultiCommand silently accept and ignore -E arguments could be confusing, so instead pass those arguments along to subcommands. --- .../org/elasticsearch/cli/MultiCommand.java | 29 +++++++--- .../elasticsearch/cli/MultiCommandTests.java | 57 ++++++++++++++----- 2 files changed, 62 insertions(+), 24 deletions(-) diff --git a/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java b/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java index 7e1bcf35067be..e33598638a936 100644 --- a/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java +++ b/libs/cli/src/main/java/org/elasticsearch/cli/MultiCommand.java @@ -21,11 +21,14 @@ 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 subcommands = new LinkedHashMap<>(); private final NonOptionArgumentSpec arguments = parser.nonOptions("command"); + private final OptionSpec settingOption; /** * Construct the multi-command with the specified command description and runnable to execute before main is invoked. @@ -45,9 +49,7 @@ public class MultiCommand extends Command { */ public MultiCommand(final String description, final Runnable beforeMain) { super(description, beforeMain); - // Accepting -E here does not prevent subcommands receiving -E arguments, since we also set `posixlyCorrect` below. - // This stops option parsing when the first non-option is encountered. - parser.accepts("E", "Unused. Pass to a subcommand instead").withRequiredArg(); + this.settingOption = parser.accepts("E", "Configure a setting").withRequiredArg().ofType(KeyValuePair.class); parser.posixlyCorrect(true); } @@ -69,15 +71,24 @@ protected void execute(Terminal terminal, OptionSet options) throws Exception { 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 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 diff --git a/server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java b/server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java index 41fe851ed2561..38c0edaee801e 100644 --- a/server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java +++ b/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 @@ static class DummyMultiCommand extends MultiCommand { final AtomicBoolean closed = new AtomicBoolean(); DummyMultiCommand() { - super("A dummy multi command", () -> { - }); + super("A dummy multi command", () -> {}); } @Override @@ -75,7 +79,23 @@ public void close() throws IOException { } } - DummyMultiCommand multiCommand; + static class DummySettingsSubCommand extends DummySubCommand { + private final ArgumentAcceptingOptionSpec 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 values = this.settingOption.values(options); + terminal.println("Settings: " + values); + super.execute(terminal, options); + } + } + + private DummyMultiCommand multiCommand; @Before public void setupCommand() { @@ -87,27 +107,21 @@ protected Command newCommand() { return multiCommand; } - public void testNoCommandsConfigured() throws Exception { - IllegalStateException e = expectThrows(IllegalStateException.class, () -> { - execute(); - }); + public void testNoCommandsConfigured() { + IllegalStateException e = expectThrows(IllegalStateException.class, this::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, () -> { - execute("somethingelse"); - }); + UserException e = expectThrows(UserException.class, () -> 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, () -> { - execute(); - }); + UserException e = expectThrows(UserException.class, this::execute); assertEquals(ExitCodes.USAGE, e.exitCode); assertEquals("Missing command", e.getMessage()); } @@ -121,6 +135,19 @@ public void testHelp() throws Exception { 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()); From 4bf7ba5468fea5c79bf615d96adee9b468b1a2c0 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 29 Nov 2019 10:43:12 +0000 Subject: [PATCH 07/13] Checkstyle --- .../xpack/watcher/trigger/schedule/tool/CronEvalTool.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java index 0573b33d28df1..565bae15ea998 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java @@ -7,7 +7,6 @@ import joptsimple.OptionSet; import joptsimple.OptionSpec; -import joptsimple.util.KeyValuePair; import org.elasticsearch.cli.ExitCodes; import org.elasticsearch.cli.LoggingAwareCommand; import org.elasticsearch.cli.Terminal; From 9061828c234ec01e668e53bcea255157d554fb23 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 29 Nov 2019 11:47:23 +0000 Subject: [PATCH 08/13] Restrict new packaging test to default distro --- .../java/org/elasticsearch/packaging/test/DockerTests.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 628ade28ba442..570b3e6d4643b 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -334,6 +334,10 @@ public void test82EnvironmentVariablesUsingFilesHaveCorrectPermissions() throws * `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, Map.of("http.host", "this.is.not.valid")); // This will fail if the env var above is passed as a -E argument From e43283ec1f31cedd81188d62c608f79f28dcc6be Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Tue, 3 Dec 2019 11:16:28 +0000 Subject: [PATCH 09/13] Override ES cgroup setting in elasticsearch-env Although most environment variable handling related to Docker has already been moved from the entrypoint script to `elasticsearch-env`, there still remaining the setting of `ES_JAVA_OPTS` to override the cgroup hierarchy setting. Move this to the env script as well, so that Docker env var handling is done in one place. --- .../docker/src/docker/bin/docker-entrypoint.sh | 14 -------------- distribution/src/bin/elasticsearch-env | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index 11774ae6878cf..0366060257b2c 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -48,20 +48,6 @@ fi # is idempotent. source /usr/share/elasticsearch/bin/elasticsearch-env-from-file -# 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 # bootstrap password for Security. diff --git a/distribution/src/bin/elasticsearch-env b/distribution/src/bin/elasticsearch-env index 64a5668ab9ffe..cbdfbf8facb5c 100644 --- a/distribution/src/bin/elasticsearch-env +++ b/distribution/src/bin/elasticsearch-env @@ -117,6 +117,20 @@ if [[ "$ES_DISTRIBUTION_TYPE" == "docker" ]]; then # 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" From 35487e9318766f348a30996e57157f7e28e4162c Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 5 Dec 2019 12:00:55 +0000 Subject: [PATCH 10/13] Remove redundant -E flags from docker exec call --- .../securing-communications/configuring-tls-docker.asciidoc | 3 --- 1 file changed, 3 deletions(-) diff --git a/x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc b/x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc index ec00220ab1d75..692d2d2c6b7be 100644 --- a/x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc +++ b/x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc @@ -206,9 +206,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" ---- -- From c25e6ea90e51118bd24b448944bd339d04eb4a7d Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Mon, 9 Dec 2019 09:54:59 +0000 Subject: [PATCH 11/13] Add zip / unzip to the Docker image --- distribution/docker/src/docker/Dockerfile | 2 +- .../securing-communications/configuring-tls-docker.asciidoc | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/distribution/docker/src/docker/Dockerfile b/distribution/docker/src/docker/Dockerfile index 93d70e83dbfff..af84595a0f659 100644 --- a/distribution/docker/src/docker/Dockerfile +++ b/distribution/docker/src/docker/Dockerfile @@ -14,7 +14,7 @@ FROM ${base_image} AS builder RUN for iter in {1..10}; do ${package_manager} update --setopt=tsflags=nodocs -y && \ - ${package_manager} install --setopt=tsflags=nodocs -y gzip shadow-utils tar && \ + ${package_manager} install --setopt=tsflags=nodocs -y gzip shadow-utils tar unzip zip && \ ${package_manager} clean all && exit_code=0 && break || exit_code=\$? && echo "${package_manager} error: retry \$iter in 10s" && sleep 10; done; \ (exit \$exit_code) diff --git a/x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc b/x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc index 692d2d2c6b7be..ac4cbcc40053a 100644 --- a/x-pack/docs/en/security/securing-communications/configuring-tls-docker.asciidoc +++ b/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> From 6064ed166b8316048c44fba76663eb5748ca30d6 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 12 Dec 2019 14:32:48 +0000 Subject: [PATCH 12/13] Check that zip and unzip are installed in Docker --- distribution/docker/src/docker/Dockerfile | 4 ++-- .../org/elasticsearch/packaging/util/Docker.java | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/distribution/docker/src/docker/Dockerfile b/distribution/docker/src/docker/Dockerfile index af84595a0f659..0f9087b143e3d 100644 --- a/distribution/docker/src/docker/Dockerfile +++ b/distribution/docker/src/docker/Dockerfile @@ -14,7 +14,7 @@ FROM ${base_image} AS builder RUN for iter in {1..10}; do ${package_manager} update --setopt=tsflags=nodocs -y && \ - ${package_manager} install --setopt=tsflags=nodocs -y gzip shadow-utils tar unzip zip && \ + ${package_manager} install --setopt=tsflags=nodocs -y gzip shadow-utils tar && \ ${package_manager} clean all && exit_code=0 && break || exit_code=\$? && echo "${package_manager} error: retry \$iter in 10s" && sleep 10; done; \ (exit \$exit_code) @@ -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) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index ffedbc14bc60e..f5185e4293aac 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -32,6 +32,7 @@ import java.nio.file.attribute.PosixFileAttributes; import java.nio.file.attribute.PosixFilePermission; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -46,10 +47,12 @@ 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.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; /** @@ -276,7 +279,7 @@ public static class DockerShell extends Shell { 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); } } @@ -445,6 +448,14 @@ private static void verifyOssInstallation(Installation es) { ).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(cliTool -> { + // `which` isn't installed in our image, so instead just try to call the command. + final Shell.Result result = dockerShell.runIgnoreExitCode(cliTool + " -h"); + + assertTrue(cliTool + " ought to be installed. " + result, result.isSuccess()); + }); } private static void verifyDefaultInstallation(Installation es) { From 2c61f08eca4b9d619d8cd6a3463d0c58e4dd4dc6 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 13 Dec 2019 10:21:35 +0000 Subject: [PATCH 13/13] Use rpm to check for package installation --- .../org/elasticsearch/packaging/util/Docker.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index f5185e4293aac..313060cdd3989 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -32,7 +32,6 @@ import java.nio.file.attribute.PosixFileAttributes; import java.nio.file.attribute.PosixFilePermission; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -50,7 +49,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -450,11 +448,11 @@ private static void verifyOssInstallation(Installation es) { 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(cliTool -> { - // `which` isn't installed in our image, so instead just try to call the command. - final Shell.Result result = dockerShell.runIgnoreExitCode(cliTool + " -h"); - - assertTrue(cliTool + " ought to be installed. " + result, result.isSuccess()); + 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()); }); }