Move docker env var settings handling out of bash (#85913)

In docker ES allows settings to be set via environment variables. This
is currently handled in complex bash logic. This commit moves that logic
into EnvironmentAwareCommand where the rest of the initial settings are
found.

relates #85758

distribution/src/bin/elasticsearch-env

@@ -92,57 +92,6 @@ ES_PATH_CONF=`cd "$ES_PATH_CONF"; pwd`
 ES_DISTRIBUTION_TYPE=@es.distribution.type@
 
 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 or ES_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
-
-  containsElement () {
-    local e match="$1"
-    shift
-    for e; do [[ "$e" == "$match" ]] && return 0; done
-    return 1
-  }
-
-  # Elasticsearch settings need to either:
-  # a. have at least two dot separated lower case words, e.g. `cluster.name`, or
-  while IFS='=' read -r envvar_key envvar_value; do
-    es_opt=""
-    if [[ -n "$envvar_value" ]]; then
-      es_opt="-E${envvar_key}=${envvar_value}"
-    fi
-    if [[ ! -z "${es_opt}" ]] && ! containsElement "${es_opt}" "$@" ; then
-      es_arg_array+=("${es_opt}")
-    fi
-  done <<< "$(env | grep -E '^[-a-z0-9_]+(\.[-a-z0-9_]+)+=')"
-
-  # b. be upper cased with underscore separators and prefixed with `ES_SETTING_`, e.g. `ES_SETTING_CLUSTER_NAME`.
-  #    Underscores in setting names are escaped by writing them as a double-underscore e.g. "__"
-  while IFS='=' read -r envvar_key envvar_value; do
-    es_opt=""
-    if [[ -n "$envvar_value" ]]; then
-      # The long-hand sed `y` command works in any sed variant.
-      envvar_key="$(echo "$envvar_key" | sed -e 's/^ES_SETTING_//; s/_/./g ; s/\.\./_/g; y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/' )"
-      es_opt="-E${envvar_key}=${envvar_value}"
-    fi
-    if [[ ! -z "${es_opt}" ]] && ! containsElement "${es_opt}" "$@" ; then
-      es_arg_array+=("${es_opt}")
-    fi
-  done <<< "$(env | grep -E '^ES_SETTING(_{1,2}[A-Z]+)+=')"
-
-  # 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

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

@@ -22,6 +22,7 @@ import java.security.PrivilegedAction;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Properties;
 import java.util.stream.Collectors;
 
@@ -55,8 +56,8 @@ public abstract class Command implements Closeable {
      */
     public Command(final String description) {
         this.description = description;
-        this.sysprops = captureSystemProperties();
+        this.sysprops = Objects.requireNonNull(captureSystemProperties());
-        this.envVars = captureEnvironmentVariables();
+        this.envVars = Objects.requireNonNull(captureEnvironmentVariables());
     }
 
     private Thread shutdownHookThread;

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

@@ -71,7 +71,6 @@ import static org.elasticsearch.packaging.util.docker.Docker.verifyContainerInst
 import static org.elasticsearch.packaging.util.docker.Docker.waitForElasticsearch;
 import static org.elasticsearch.packaging.util.docker.DockerFileMatcher.file;
 import static org.elasticsearch.packaging.util.docker.DockerRun.builder;
-import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.arrayContaining;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
@@ -754,86 +753,6 @@ public class DockerTests extends PackagingTestCase {
         assertThat(result.stdout(), containsString("java.net.UnknownHostException: this.is.not.valid"));
     }
 
-    /**
-     * Check that settings are applied when they are supplied as environment variables with names that are:
-     * <ul>
-     *     <li>Prefixed with {@code ES_SETTING_}</li>
-     *     <li>All uppercase</li>
-     *     <li>Dots (periods) are converted to underscores</li>
-     *     <li>Underscores in setting names are escaped by doubling them</li>
-     * </ul>
-     */
-    public void test086EnvironmentVariablesInSnakeCaseAreTranslated() {
-        // Note the double-underscore in the var name here, which retains the underscore in translation
-        installation = runContainer(distribution(), builder().envVar("ES_SETTING_XPACK_SECURITY_FIPS__MODE_ENABLED", "false"));
-
-        final Optional<String> commandLine = sh.run("bash -c 'COLUMNS=2000 ps ax'")
-            .stdout()
-            .lines()
-            .filter(line -> line.contains("org.elasticsearch.bootstrap.Elasticsearch"))
-            .findFirst();
-
-        assertThat(commandLine.isPresent(), equalTo(true));
-
-        assertThat(commandLine.get(), containsString("-Expack.security.fips_mode.enabled=false"));
-    }
-
-    /**
-     * Check that environment variables that do not match the criteria for translation to settings are ignored.
-     */
-    public void test087EnvironmentVariablesInIncorrectFormatAreIgnored() {
-        installation = runContainer(
-            distribution(),
-            builder()
-                // No ES_SETTING_ prefix
-                .envVar("XPACK_SECURITY_FIPS__MODE_ENABLED", "false")
-                // Incomplete prefix
-                .envVar("ES_XPACK_SECURITY_FIPS__MODE_ENABLED", "false")
-                // Not underscore-separated
-                .envVar("ES.SETTING.XPACK.SECURITY.FIPS_MODE.ENABLED", "false")
-                // Not uppercase
-                .envVar("es_setting_xpack_security_fips__mode_enabled", "false")
-        );
-
-        final Optional<String> commandLine = sh.run("bash -c 'COLUMNS=2000 ps ax'")
-            .stdout()
-            .lines()
-            .filter(line -> line.contains("org.elasticsearch.bootstrap.Elasticsearch"))
-            .findFirst();
-
-        assertThat(commandLine.isPresent(), equalTo(true));
-
-        assertThat(commandLine.get(), not(containsString("-Expack.security.fips_mode.enabled=false")));
-    }
-
-    /**
-     * Check that settings are applied when they are supplied as environment variables with names that:
-     * <ul>
-     *     <li>Consist only of lowercase letters, numbers, underscores and hyphens</li>
-     *     <li>Separated by periods</li>
-     * </ul>
-     */
-    public void test088EnvironmentVariablesInDottedFormatArePassedThrough() {
-        // Note the double-underscore in the var name here, which retains the underscore in translation
-        installation = runContainer(
-            distribution(),
-            builder().envVar("xpack.security.fips_mode.enabled", "false").envVar("http.cors.allow-methods", "GET")
-        );
-
-        final Optional<String> commandLine = sh.run("bash -c 'COLUMNS=2000 ps ax'")
-            .stdout()
-            .lines()
-            .filter(line -> line.contains("org.elasticsearch.bootstrap.Elasticsearch"))
-            .findFirst();
-
-        assertThat(commandLine.isPresent(), equalTo(true));
-
-        assertThat(
-            commandLine.get(),
-            allOf(containsString("-Expack.security.fips_mode.enabled=false"), containsString("-Ehttp.cors.allow-methods=GET"))
-        );
-    }
-
     /**
      * 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/ServerUtils.java

@@ -39,7 +39,6 @@ import java.security.cert.CertificateFactory;
 import java.security.cert.X509Certificate;
 import java.util.List;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -77,16 +76,14 @@ public class ServerUtils {
             String configFile = Files.readString(configFilePath, StandardCharsets.UTF_8);
             securityEnabled = configFile.contains(SECURITY_DISABLED) == false;
         } else {
-            final Optional<String> commandLine = dockerShell.run("bash -c 'COLUMNS=2000 ps ax'")
+            securityEnabled = dockerShell.run("env")
                 .stdout()
                 .lines()
-                .filter(line -> line.contains("org.elasticsearch.bootstrap.Elasticsearch"))
+                .filter(each -> each.startsWith("xpack.security.enabled"))
-                .findFirst();
+                .findFirst()
-            if (commandLine.isPresent() == false) {
+                .map(line -> Boolean.parseBoolean(line.split("=")[1]))
-                throw new RuntimeException("Installation distribution is docker but a docker container is not running");
-            }
                 // security is enabled by default, the only way for it to be disabled is to be explicitly disabled
-            securityEnabled = commandLine.get().contains("-Expack.security.enabled=false") == false;
+                .orElse(true);
         }
 
         if (securityEnabled) {

server/src/main/java/org/elasticsearch/common/cli/EnvironmentAwareCommand.java

@@ -12,6 +12,7 @@ import joptsimple.OptionSet;
 import joptsimple.OptionSpec;
 import joptsimple.util.KeyValuePair;
 
+import org.elasticsearch.Build;
 import org.elasticsearch.cli.Command;
 import org.elasticsearch.cli.ExitCodes;
 import org.elasticsearch.cli.Terminal;
@@ -26,10 +27,14 @@ import java.nio.file.Paths;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
+import java.util.regex.Pattern;
 
 /** A cli command which requires an {@link org.elasticsearch.env.Environment} to use current paths and settings. */
 public abstract class EnvironmentAwareCommand extends Command {
 
+    private static final String DOCKER_UPPERCASE_SETTING_PREFIX = "ES_SETTING_";
+    private static final Pattern DOCKER_LOWERCASE_SETTING_REGEX = Pattern.compile("[-a-z0-9_]+(\\.[-a-z0-9_]+)+");
+
     private final OptionSpec<KeyValuePair> settingOption;
 
     /**
@@ -48,6 +53,27 @@ public abstract class EnvironmentAwareCommand extends Command {
         execute(terminal, options, createEnv(options));
     }
 
+    private void putDockerEnvSettings(Map<String, String> settings, Map<String, String> envVars) {
+        for (var envVar : envVars.entrySet()) {
+            String key = envVar.getKey();
+            if (DOCKER_LOWERCASE_SETTING_REGEX.matcher(key).matches()) {
+                // all lowercase, like cluster.name, so just put directly
+                settings.put(key, envVar.getValue());
+            } else if (key.startsWith(DOCKER_UPPERCASE_SETTING_PREFIX)) {
+                // remove prefix
+                key = key.substring(DOCKER_UPPERCASE_SETTING_PREFIX.length());
+                // insert dots for underscores
+                key = key.replace('_', '.');
+                // unescape double dots, which were originally double underscores
+                key = key.replace("..", "_");
+                // lowercase the whole thing
+                key = key.toLowerCase(Locale.ROOT);
+
+                settings.put(key, envVar.getValue());
+            }
+        }
+    }
+
     /** Create an {@link Environment} for the command to use. Overrideable for tests. */
     protected Environment createEnv(OptionSet options) throws UserException {
         final Map<String, String> settings = new HashMap<>();
@@ -68,6 +94,10 @@ public abstract class EnvironmentAwareCommand extends Command {
             settings.put(kvp.key, kvp.value);
         }
 
+        if (getBuildType() == Build.Type.DOCKER) {
+            putDockerEnvSettings(settings, envVars);
+        }
+
         putSystemPropertyIfSettingIsMissing(sysprops, settings, "path.data", "es.path.data");
         putSystemPropertyIfSettingIsMissing(sysprops, settings, "path.home", "es.path.home");
         putSystemPropertyIfSettingIsMissing(sysprops, settings, "path.logs", "es.path.logs");
@@ -85,6 +115,11 @@ public abstract class EnvironmentAwareCommand extends Command {
         );
     }
 
+    // protected to allow tests to override
+    protected Build.Type getBuildType() {
+        return Build.CURRENT.type();
+    }
+
     @SuppressForbidden(reason = "need path to construct environment")
     private static Path getConfigPath(final String pathConf) {
         return Paths.get(pathConf);

server/src/test/java/org/elasticsearch/common/cli/EnvironmentAwareCommandTests.java

@@ -0,0 +1,142 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.common.cli;
+
+import joptsimple.OptionSet;
+
+import org.elasticsearch.Build;
+import org.elasticsearch.cli.Command;
+import org.elasticsearch.cli.CommandTestCase;
+import org.elasticsearch.cli.Terminal;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.env.Environment;
+import org.junit.Before;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Consumer;
+
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+
+public class EnvironmentAwareCommandTests extends CommandTestCase {
+
+    private Build.Type buildType;
+    private Map<String, String> mockEnvVars;
+    private Consumer<Environment> callback;
+
+    @Before
+    public void resetHooks() {
+        buildType = Build.Type.TAR;
+        mockEnvVars = new HashMap<>();
+        callback = null;
+    }
+
+    @Override
+    protected Command newCommand() {
+        return new EnvironmentAwareCommand("test command") {
+            @Override
+            protected void execute(Terminal terminal, OptionSet options, Environment env) {
+                if (callback != null) {
+                    callback.accept(env);
+                }
+            }
+
+            @Override
+            protected Map<String, String> captureSystemProperties() {
+                return mockSystemProperties(createTempDir());
+            }
+
+            @Override
+            protected Map<String, String> captureEnvironmentVariables() {
+                return mockEnvVars;
+            }
+
+            @Override
+            protected Build.Type getBuildType() {
+                return buildType;
+            }
+        };
+    }
+
+    // Check that for non-Docker, environment variables are not translated into settings
+    public void testNonDockerEnvVarSettingsIgnored() throws Exception {
+        mockEnvVars.put("ES_SETTING_FOO_BAR", "baz");
+        mockEnvVars.put("some.setting", "1");
+        callback = env -> {
+            Settings settings = env.settings();
+            assertThat(settings.hasValue("foo.bar"), is(false));
+            assertThat(settings.hasValue("some.settings"), is(false));
+        };
+        execute();
+    }
+
+    // Check that for Docker, environment variables that do not match the criteria for translation to settings are ignored.
+    public void testDockerEnvVarSettingsIgnored() throws Exception {
+        // No ES_SETTING_ prefix
+        mockEnvVars.put("XPACK_SECURITY_FIPS__MODE_ENABLED", "false");
+        // Incomplete prefix
+        mockEnvVars.put("ES_XPACK_SECURITY_FIPS__MODE_ENABLED", "false");
+        // Not underscore-separated
+        mockEnvVars.put("ES.SETTING.XPACK.SECURITY.FIPS_MODE.ENABLED", "false");
+        // Not uppercase
+        mockEnvVars.put("es_setting_xpack_security_fips__mode_enabled", "false");
+        // single word is not translated, it must contain a dot
+        mockEnvVars.put("singleword", "value");
+        // any uppercase letters cause the var to be ignored
+        mockEnvVars.put("setting.Ignored", "value");
+        callback = env -> {
+            Settings settings = env.settings();
+            assertThat(settings.hasValue("xpack.security.fips_mode.enabled"), is(false));
+            assertThat(settings.hasValue("singleword"), is(false));
+            assertThat(settings.hasValue("setting.Ignored"), is(false));
+            assertThat(settings.hasValue("setting.ignored"), is(false));
+        };
+        execute();
+    }
+
+    // Check that for Docker builds, various env vars are translated correctly to settings
+    public void testDockerEnvVarSettingsTranslated() throws Exception {
+        buildType = Build.Type.DOCKER;
+        // normal setting with a dot
+        mockEnvVars.put("ES_SETTING_SIMPLE_SETTING", "value");
+        // double underscore is translated to literal underscore
+        mockEnvVars.put("ES_SETTING_UNDERSCORE__HERE", "value");
+        // literal underscore and a dot
+        mockEnvVars.put("ES_SETTING_UNDERSCORE__DOT_BAZ", "value");
+        // two literal underscores
+        mockEnvVars.put("ES_SETTING_DOUBLE____UNDERSCORE", "value");
+        // literal underscore followed by a dot (not valid setting, but translated nonetheless
+        mockEnvVars.put("ES_SETTING_TRIPLE___BAZ", "value");
+        // lowercase
+        mockEnvVars.put("lowercase.setting", "value");
+        callback = env -> {
+            Settings settings = env.settings();
+            assertThat(settings.get("simple.setting"), equalTo("value"));
+            assertThat(settings.get("underscore_here"), equalTo("value"));
+            assertThat(settings.get("underscore_dot.baz"), equalTo("value"));
+            assertThat(settings.get("triple_.baz"), equalTo("value"));
+            assertThat(settings.get("double__underscore"), equalTo("value"));
+            assertThat(settings.get("lowercase.setting"), equalTo("value"));
+        };
+        execute();
+    }
+
+    // Check that for Docker builds, env vars takes precedence over settings on the command line.
+    public void testDockerEnvVarSettingsOverrideCommandLine() throws Exception {
+        // docker env takes precedence over settings on the command line
+        buildType = Build.Type.DOCKER;
+        mockEnvVars.put("ES_SETTING_SIMPLE_SETTING", "override");
+        callback = env -> {
+            Settings settings = env.settings();
+            assertThat(settings.get("simple.setting"), equalTo("override"));
+        };
+        execute("-Esimple.setting=original");
+    }
+}