Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support _FILE suffixed env vars in Docker entrypoint #47573

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
05a0133
Support ELASTIC_PASSWORD_FILE in Docker entrypoint
pugnascotia Oct 2, 2019
65974f9
Support all envs vars with a _FILE suffix
pugnascotia Oct 2, 2019
2b90d97
Include _FILE names when checking files in Docker
pugnascotia Oct 8, 2019
1f03dad
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Oct 8, 2019
7e361ff
Trying to write tests
pugnascotia Oct 9, 2019
64175d2
Fix up new Docker test
pugnascotia Oct 9, 2019
9b940cf
Revert accidental changes
pugnascotia Oct 9, 2019
7ca020c
Tweaks
pugnascotia Oct 9, 2019
21c1104
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 5, 2019
e564576
Post-merge fixes
pugnascotia Nov 5, 2019
6f1cf82
Check env var file permissions
pugnascotia Nov 5, 2019
8562703
Capture more logging on failure
pugnascotia Nov 6, 2019
7f616bd
Split env var file tests
pugnascotia Nov 8, 2019
0162ef2
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 10, 2019
052d46f
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 11, 2019
ebce9ef
Address review feedback
pugnascotia Nov 11, 2019
4054f2d
Tighter checks for _FILE vs regular vars
pugnascotia Nov 11, 2019
0a07f70
Update docs for _FILE variables
pugnascotia Nov 11, 2019
2a48087
Docs tweaks
pugnascotia Nov 11, 2019
3a778a7
Address further docs review feedback
pugnascotia Nov 12, 2019
4ac0a9e
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions distribution/docker/src/docker/bin/docker-entrypoint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,38 @@ if [[ "$1" != "eswrapper" ]]; then
fi
fi

# 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.
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 [[ -n "${!VAR_NAME}" ]]; then
echo "ERROR: Both $VAR_NAME_FILE and $VAR_NAME are set. These are mutually exclusive." >&2
dliappis marked this conversation as resolved.
Show resolved Hide resolved
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

if [[ ! -r "${!VAR_NAME_FILE}" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth combining the checks on permissions into a single one.
We could get into the situation when we tell the user it's not readable, then we tell him it's too broadly readable.

echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE is not readable" >&2
exit 1
fi

echo "Setting $VAR_NAME from $VAR_NAME_FILE at ${!VAR_NAME_FILE}" >&2
export "$VAR_NAME"="$(cat ${!VAR_NAME_FILE})"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be stripping a possible newline at the end of the file here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newline is already stripped thanks to bash. An earlier implementation read the file without using cat, but didn't strip the newline.


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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.Matchers.emptyString;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assume.assumeTrue;

@Ignore("https://github.com/elastic/elasticsearch/issues/47639")
Expand Down Expand Up @@ -166,7 +167,8 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception {

// Restart the container
removeContainer();
runContainer(distribution(), tempConf, Map.of(
final Map<Path, Path> volumes = Map.of(tempConf, Path.of("/usr/share/elasticsearch/config"));
runContainer(distribution(), volumes, Map.of(
"ES_JAVA_OPTS", "-XX:-UseCompressedOops"
));

Expand All @@ -180,6 +182,60 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception {
}
}

/**
* Check that environment variables can be populated by setting variables with the suffix "_FILE",
* which point to files that hold the required values.
*/
public void test80SetEnvironmentVariablesUsingFiles() throws Exception {
final String xpackPassword = "hunter2";
final Path secretsDir = getTempDir().resolve("secrets");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would pull this into an instance variable and use @Before and @After to set it and clean up the dir.
It's more readable and less repetitive avoiding the try, finally is particularly beneficial imho.

final String optionsFilename = "esJavaOpts.txt";
final String passwordFilename = "password.txt";

try {
mkdir(secretsDir);

// ES_JAVA_OPTS_FILE
Files.writeString(secretsDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n");

// ELASTIC_PASSWORD_FILE
Files.writeString(secretsDir.resolve(passwordFilename), xpackPassword + "\n");

// Make the temp directory and contents accessible when bind-mounted
Files.setPosixFilePermissions(secretsDir, fromString("rwxrwxrwx"));

Map<String, String> envVars = Map.of(
"ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename,
"ELASTIC_PASSWORD_FILE", "/run/secrets/" + passwordFilename,
// Enable security so that we can test that the password has been used
"xpack.security.enabled", "true"
);

final Map<Path, Path> volumes = Map.of(secretsDir, Path.of("/run/secrets"));

// Restart the container
runContainer(distribution(), volumes, envVars);

// If we configured security correctly, then this call will only work if we specify the correct credentials.
waitForElasticsearch("green", null, installation, "elastic", "hunter2");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to wrap the exception here to add some additional context.
Imagine this fails 6 months from now, will it be obvious from the failure that the passwrod was not applied or will it look as if elasticsearch didn't start ? I'm not sure what the error looks like, guess it's better if it's an authentication failure, but the additional context certainly doesn't hurt.


// Try to call `/_nodes` first without authentication, and ensure it's rejected.
// We don't use the `makeRequest()` helper as it checks the status code.
final int statusCode = Request.Get("http://localhost:9200/_nodes").execute().returnResponse().getStatusLine().getStatusCode();
assertThat("Expected server to require authentication", statusCode, equalTo(401));

// Now try again, but with credentials, to check that ES_JAVA_OPTS_FILE took effect
final String nodesResponse = makeRequest(
Request.Get("http://localhost:9200/_nodes"),
"elastic",
xpackPassword);

assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\""));
} finally {
rm(secretsDir);
}
}

/**
* Check whether the elasticsearch-certutil tool has been shipped correctly,
* and if present then it can execute.
Expand Down
48 changes: 29 additions & 19 deletions qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import java.nio.file.Path;
import java.nio.file.attribute.PosixFilePermission;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -80,18 +79,22 @@ public static void ensureImageIsLoaded(Distribution distribution) {
* @param distribution details about the docker image being tested.
*/
public static Installation runContainer(Distribution distribution) throws Exception {
return runContainer(distribution, null, Collections.emptyMap());
return runContainer(distribution, null, null);
}

/**
* Runs an Elasticsearch Docker container, with options for overriding the config directory
* through a bind mount, and passing additional environment variables.
*
* @param distribution details about the docker image being tested.
* @param configPath the path to the config to bind mount, or null
* @param envVars environment variables to set when running the container
* @param volumes a map that declares any volume mappings to apply, or null
* @param envVars environment variables to set when running the container, or null
*/
public static Installation runContainer(Distribution distribution, Path configPath, Map<String,String> envVars) throws Exception {
public static Installation runContainer(
Distribution distribution,
Map<Path, Path> volumes,
Map<String,String> envVars
) {
removeContainer();

final List<String> args = new ArrayList<>();
Expand All @@ -104,7 +107,9 @@ public static Installation runContainer(Distribution distribution, Path configPa
// Run the container in the background
args.add("--detach");

envVars.forEach((key, value) -> args.add("--env " + key + "=\"" + value + "\""));
if (envVars != null) {
envVars.forEach((key, value) -> args.add("--env " + key + "=\"" + value + "\""));
}

// The container won't run without configuring discovery
args.add("--env discovery.type=single-node");
Expand All @@ -113,15 +118,15 @@ public static Installation runContainer(Distribution distribution, Path configPa
args.add("--publish 9200:9200");
args.add("--publish 9300:9300");

if (configPath != null) {
// Bind-mount the config dir, if specified
args.add("--volume \"" + configPath + ":/usr/share/elasticsearch/config\"");
// Bind-mount any volumes
if (volumes != null) {
volumes.forEach((localPath, containerPath) -> args.add("--volume \"" + localPath + ":" + containerPath + "\""));
}

args.add(distribution.flavor.name + ":test");

final String command = String.join(" ", args);
logger.debug("Running command: " + command);
logger.info("Running command: " + command);
containerId = sh.run(command).stdout.trim();

waitForElasticsearchToStart();
Expand All @@ -133,22 +138,27 @@ public static Installation runContainer(Distribution distribution, Path configPa
* Waits for the Elasticsearch process to start executing in the container.
* This is called every time a container is started.
*/
private static void waitForElasticsearchToStart() throws InterruptedException {
private static void waitForElasticsearchToStart() {
boolean isElasticsearchRunning = false;
int attempt = 0;

do {
String psOutput = dockerShell.run("ps ax").stdout;
try {
String psOutput = dockerShell.run("ps ax").stdout;

if (psOutput.contains("/usr/share/elasticsearch/jdk/bin/java -X")) {
isElasticsearchRunning = true;
break;
}
if (psOutput.contains("/usr/share/elasticsearch/jdk/bin/java -X")) {
isElasticsearchRunning = true;
break;
}

Thread.sleep(1000);
Thread.sleep(1000);
}
catch (Exception e) {
logger.debug("Caught exception while waiting for ES to start", e);
}
} while (attempt++ < 5);

if (!isElasticsearchRunning) {
if (isElasticsearchRunning == false) {
final String logs = sh.run("docker logs " + containerId).stdout;
fail("Elasticsearch container did start successfully.\n\n" + logs);
}
Expand All @@ -167,7 +177,7 @@ public static void removeContainer() {

if (result.isSuccess() == false) {
// I'm not sure why we're already removing this container, but that's OK.
if (result.stderr.contains("removal of container " + " is already in progress") == false) {
if (result.stderr.contains("removal of container " + containerId + " is already in progress") == false) {
throw new RuntimeException(
"Command was not successful: [" + command + "] result: " + result.toString());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@

package org.elasticsearch.packaging.util;

import org.apache.http.HttpHost;
import org.apache.http.HttpResponse;
import org.apache.http.client.fluent.Executor;
import org.apache.http.client.fluent.Request;
import org.apache.http.entity.ContentType;
import org.apache.http.util.EntityUtils;
Expand All @@ -35,31 +37,58 @@

public class ServerUtils {

protected static final Logger logger = LogManager.getLogger(ServerUtils.class);
private static final Logger logger = LogManager.getLogger(ServerUtils.class);

private static final long waitTime = TimeUnit.SECONDS.toMillis(60);
private static final long timeoutLength = TimeUnit.SECONDS.toMillis(10);

public static void waitForElasticsearch(Installation installation) throws IOException {
waitForElasticsearch("green", null, installation);
waitForElasticsearch("green", null, installation, null, null);
}

public static void waitForElasticsearch(String status, String index, Installation installation) throws IOException {
/**
* Executes the supplied request, optionally applying HTTP basic auth if the
* username and pasword field are supplied.
* @param request the request to execute
* @param username the username to supply, or null
* @param password the password to supply, or null
* @return the response from the server
* @throws IOException if an error occurs
*/
private static HttpResponse execute(Request request, String username, String password) throws IOException {
final Executor executor = Executor.newInstance();

if (username != null && password != null) {
executor.auth(username, password);
executor.authPreemptive(new HttpHost("localhost", 9200));
}

return executor.execute(request).returnResponse();
}

public static void waitForElasticsearch(
String status,
String index,
Installation installation,
String username,
String password
) throws IOException {

Objects.requireNonNull(status);

// we loop here rather than letting httpclient handle retries so we can measure the entire waiting time
final long startTime = System.currentTimeMillis();
long timeElapsed = 0;
boolean started = false;
Exception lastException = null;

while (started == false && timeElapsed < waitTime) {
try {

final HttpResponse response = Request.Get("http://localhost:9200/_cluster/health")
final Request request = Request.Get("http://localhost:9200/_cluster/health")
.connectTimeout((int) timeoutLength)
.socketTimeout((int) timeoutLength)
.execute()
.returnResponse();
.socketTimeout((int) timeoutLength);

final HttpResponse response = execute(request, username, password);

if (response.getStatusLine().getStatusCode() >= 300) {
final String statusLine = response.getStatusLine().toString();
Expand All @@ -70,7 +99,8 @@ public static void waitForElasticsearch(String status, String index, Installatio
started = true;

} catch (IOException e) {
logger.info("Got exception when waiting for cluster health", e);
lastException = e;
logger.debug("Got exception when waiting for cluster health", e);
}

timeElapsed = System.currentTimeMillis() - startTime;
Expand All @@ -80,7 +110,7 @@ public static void waitForElasticsearch(String status, String index, Installatio
if (installation != null) {
FileUtils.logAllLogs(installation.logs, logger);
}
throw new RuntimeException("Elasticsearch did not start");
throw new RuntimeException("Elasticsearch did not start", lastException);
}

final String url;
Expand All @@ -91,7 +121,7 @@ public static void waitForElasticsearch(String status, String index, Installatio

}

final String body = makeRequest(Request.Get(url));
final String body = makeRequest(Request.Get(url), username, password);
assertThat("cluster health response must contain desired status", body, containsString(status));
}

Expand All @@ -111,14 +141,17 @@ public static void runElasticsearchTests() throws IOException {
}

public static String makeRequest(Request request) throws IOException {
final HttpResponse response = request.execute().returnResponse();
return makeRequest(request, null, null);
}

public static String makeRequest(Request request, String username, String password) throws IOException {
final HttpResponse response = execute(request, username, password);
final String body = EntityUtils.toString(response.getEntity());

if (response.getStatusLine().getStatusCode() >= 300) {
throw new RuntimeException("Request failed:\n" + response.getStatusLine().toString() + "\n" + body);
}

return body;

}
}