Refactor environment variable processing for Docker #49612

pugnascotia · 2019-11-26T21:26:58Z

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

This PR refactors 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.

Closes elastic#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.

elasticmachine · 2019-11-26T21:27:01Z

Pinging @elastic/es-core-infra (:Core/Infra/Packaging)

distribution/docker/src/docker/Dockerfile

dliappis

LGTM! Left a minor comment.

distribution/docker/src/docker/Dockerfile

distribution/src/bin/elasticsearch-env

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.

pugnascotia · 2019-11-27T12:01:41Z

So 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. This also means that the tool's help text documents the fact that the `-E options are unused, instead of implying that they do something.

pugnascotia · 2019-11-28T14:24:42Z

I've updated MultiCommand to also accept -E arguments, since otherwise e.g. ./elasticsearch-node --help fails when it has -E args appends. The extra handling does nothing, and does not prevent -E` arguments later in the command line from being picked up correctly.

pugnascotia · 2019-11-28T14:26:25Z

@tvernum I'd appreciate your thoughts on these changes if have capacity.

tvernum · 2019-11-29T00:45:19Z

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

@@ -45,6 +45,9 @@
     */
    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();


I think this is a problem.
As you know, it's natural for people to expect that passing -E as the first argument to a multi-command will behave correctly. Currently they get an error, and have to work out what they've done wrong.

With this change they will no longer get an error, they will just get something that silently ignores their parameter. This is surprising and trappy behaviour.

If we want to do this, then I think we need to either:

make it an error to pass -E and also call a subcommand (so --help -E var.from.docker=true works, but -E var=true auto does not); OR

actually pass the -E values into the subcommand like people expect.

Unless I'm misunderstanding the behaviour here, and this already does (2), in which case can we make the comment more explicit?

No, you're quite right. I was thinking primarily of the Docker case, but it would be much better if -E was respected wherever it is supplied. I'll look at this again.

@tvernum I've addressed this in 31e4679 by passing along -E args to subcommands.

Having MultiCommand silently accept and ignore -E arguments could be confusing, so instead pass those arguments along to subcommands.

…v-var-handling

pugnascotia · 2019-11-29T13:26:15Z

@elasticmachine run elasticsearch-ci/packaging-matrix

…v-var-handling

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.

dliappis

LGTM for the change in e43283e

pugnascotia · 2019-12-03T14:00:15Z

@jkakavas would you have time to look over the Java parts of this PR from a security POV?

dliappis · 2019-12-03T17:40:57Z

@pugnascotia It dawned on me that with this PR we should adjust also the instructions in step 4. of configuring-tls-on-docker. It runs elasticsearch-setup-passwords via docker exec so as they are now they explicitly execute the command with -E ....

In a separate PR we can adjust the same instructions in step 3 of the stack-docs.

tvernum

LGTM

pugnascotia · 2019-12-05T12:01:38Z

@dliappis I updated the docs in 35487e9.

dliappis

Thanks! While digging into this I realized there are some changes in the images (UBI) which make one docker-compose file in this doc file non functional.

At some point we need docs tests for this, but I left a comment that IMHO we should bundle unzip in all our images (given the need to deal with artifacts from cli tools) and simplify the create-certs.yml compose file in the docs.

dliappis · 2019-12-09T08:12:55Z

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 \


Thanks! This works, however -- at least this PR -- includes the UBI work and earlier in this file we rely on yum install unzip. In order to simply things, given that unzip is a fairly common requirement for handling certs, what if we include unzip in our Dockerfile i.e.bundle it in all Elasticsearch images and remove it from the the mentioned line above, so that examples and users don't have to pull it?

That makes sense to me, and we already include gzip.

Great. Maybe, esp. given that we have abstracted the package manager invocation now, we could test for the present of unzip / zip in the path in our test cases, if it's not too complex?

Good call on the test - I was installing the CLI tools in the wrong part of the Dockefile!

…v-var-handling

dliappis

LGTM

…v-var-handling

dliappis

Great catch! Left a comment

dliappis · 2019-12-12T17:57:39Z

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

+        // 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");


In bash you can also rely on type -P <thefile> if you don't have which to ensure the file is in $PATH:

e.g. on the Ubi image:

[elasticsearch@0f6edc7f150a ~]$ type -P zip [elasticsearch@0f6edc7f150a ~]$ echo $? 1 [elasticsearch@0f6edc7f150a ~]$ type -P ls /usr/bin/ls [elasticsearch@0f6edc7f150a ~]$ echo $? 0

type -P would work, but as it's a bash builtin, I'd have to run it through bash. That's a actually a bit cumbersome, because we're already running "bash -c 'docker blah blah commmand'". Instead, I'm now using rpm to check if the package is installed.

Closes elastic#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, add noop -E handling to CronEvalTool for parity, and support `-E` in MultiCommand before subcommands.

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.

Closes elastic#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, add noop -E handling to CronEvalTool for parity, and support `-E` in MultiCommand before subcommands.

pugnascotia added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v8.0.0 labels Nov 26, 2019

pugnascotia requested review from dliappis and tvernum November 26, 2019 21:26

pugnascotia commented Nov 26, 2019

View reviewed changes

distribution/docker/src/docker/Dockerfile Show resolved Hide resolved

pugnascotia mentioned this pull request Nov 26, 2019

Make docker environment variable parsing available in CLI tools #45223

Closed

dliappis approved these changes Nov 27, 2019

View reviewed changes

distribution/docker/src/docker/Dockerfile Show resolved Hide resolved

distribution/src/bin/elasticsearch-env Outdated Show resolved Hide resolved

pugnascotia added 2 commits November 27, 2019 09:38

Remove outdated comment

2261af3

pugnascotia added 2 commits November 27, 2019 15:57

Fix test after removing elasticsearch-enve

3f7d287

Allow -E in MultiCommand tools

fcec60d

pugnascotia requested a review from dliappis November 28, 2019 14:24

tvernum reviewed Nov 29, 2019

View reviewed changes

Pass along -E arguments to subcommands

31e4679

Having MultiCommand silently accept and ignore -E arguments could be confusing, so instead pass those arguments along to subcommands.

pugnascotia requested a review from tvernum November 29, 2019 10:30

pugnascotia added 3 commits November 29, 2019 10:43

Checkstyle

4bf7ba5

Merge remote-tracking branch 'upstream/master' into 45223-refactor-en…

0d8b8dd

…v-var-handling

Restrict new packaging test to default distro

9061828

pugnascotia added 2 commits December 3, 2019 11:11

Merge remote-tracking branch 'upstream/master' into 45223-refactor-en…

569841a

…v-var-handling

dliappis approved these changes Dec 3, 2019

View reviewed changes

pugnascotia requested a review from jkakavas December 3, 2019 13:59

tvernum approved these changes Dec 5, 2019

View reviewed changes

pugnascotia mentioned this pull request Dec 5, 2019

Refactor CLI classes so everything is environment aware #49858

Closed

Remove redundant -E flags from docker exec call

35487e9

pugnascotia requested review from dliappis and removed request for jkakavas December 5, 2019 12:01

dliappis requested changes Dec 9, 2019

View reviewed changes

Add zip / unzip to the Docker image

c25e6ea

pugnascotia requested a review from dliappis December 9, 2019 09:55

Merge remote-tracking branch 'upstream/master' into 45223-refactor-en…

af04f13

…v-var-handling

dliappis approved these changes Dec 9, 2019

View reviewed changes

pugnascotia added 2 commits December 12, 2019 12:47

Merge remote-tracking branch 'upstream/master' into 45223-refactor-en…

329d7f9

…v-var-handling

Check that zip and unzip are installed in Docker

6064ed1

pugnascotia requested a review from dliappis December 12, 2019 14:33

dliappis reviewed Dec 12, 2019

View reviewed changes

Use rpm to check for package installation

2c61f08

pugnascotia requested a review from dliappis December 13, 2019 10:23

pugnascotia merged commit 9f069f7 into elastic:master Dec 16, 2019

pugnascotia deleted the 45223-refactor-env-var-handling branch December 16, 2019 09:57

pugnascotia mentioned this pull request Dec 16, 2019

Refactor environment variable processing for Docker #50221

Merged

This was referenced Mar 25, 2020

Monkey patch elasticsearch-env file so it parses on busybox. openzipkin/zipkin#3044

Merged

Reduce the surface area of the Elasticsearch Docker image #51670

Closed

pugnascotia added the v7.6.0 label Jun 1, 2020

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor environment variable processing for Docker #49612

Refactor environment variable processing for Docker #49612

pugnascotia commented Nov 26, 2019 •

edited

Loading

elasticmachine commented Nov 26, 2019

dliappis left a comment

pugnascotia commented Nov 27, 2019

pugnascotia commented Nov 28, 2019

pugnascotia commented Nov 28, 2019

tvernum Nov 29, 2019

tvernum Nov 29, 2019

pugnascotia Nov 29, 2019

pugnascotia Nov 29, 2019

pugnascotia commented Nov 29, 2019

dliappis left a comment

pugnascotia commented Dec 3, 2019

dliappis commented Dec 3, 2019 •

edited

Loading

tvernum left a comment

pugnascotia commented Dec 5, 2019

dliappis left a comment

dliappis Dec 9, 2019

pugnascotia Dec 9, 2019

dliappis Dec 9, 2019

pugnascotia Dec 12, 2019

dliappis left a comment

dliappis left a comment

dliappis Dec 12, 2019

pugnascotia Dec 13, 2019

Refactor environment variable processing for Docker #49612

Refactor environment variable processing for Docker #49612

Conversation

pugnascotia commented Nov 26, 2019 • edited Loading

elasticmachine commented Nov 26, 2019

dliappis left a comment

Choose a reason for hiding this comment

pugnascotia commented Nov 27, 2019

pugnascotia commented Nov 28, 2019

pugnascotia commented Nov 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugnascotia commented Nov 29, 2019

dliappis left a comment

Choose a reason for hiding this comment

pugnascotia commented Dec 3, 2019

dliappis commented Dec 3, 2019 • edited Loading

tvernum left a comment

Choose a reason for hiding this comment

pugnascotia commented Dec 5, 2019

dliappis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis left a comment

Choose a reason for hiding this comment

dliappis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pugnascotia commented Nov 26, 2019 •

edited

Loading

dliappis commented Dec 3, 2019 •

edited

Loading