Move docker env var settings handling out of bash #85913
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 elastic#85758
rjernst
added
>refactoring
:Core/Infra/CLI
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Previously, in order to determine whether security had been disabled via environment variables in Docker, we were able to check the command line switches to the Elasticsearch process. That doesn't work now because Elasticsearch processes the environment variables itself. Therefore, change the check to look at the environment instead.
pugnascotia
reviewed
Apr 15, 2022
| @@ -78,7 +78,7 @@ public class Docker { | |||
| * but that appeared to cause problems with repeatedly destroying and recreating containers with | |||
| * the same name. | |||
| */ | |||
| static String containerId = null; | |||
| public static String containerId = null; | |||
There was a problem hiding this comment.
I don't think we need this now?
pugnascotia
reviewed
Apr 15, 2022
| }; | ||
| } | ||
|
|
||
| public void testNonDockerEnvVarsIgnored() throws Exception { |
There was a problem hiding this comment.
Suggested change
| public void testNonDockerEnvVarsIgnored() throws Exception { | |
| /** | |
| * Check that env vars are not respected when the build type is not Docker. | |
| */ | |
| public void testNonDockerBuildIgnoresEnvVars() throws Exception { |
pugnascotia
reviewed
Apr 15, 2022
| execute(); | ||
| } | ||
|
|
||
| public void testDockerEnvSettings() throws Exception { |
There was a problem hiding this comment.
Suggested change
| public void testDockerEnvSettings() throws Exception { | |
| /** | |
| * Check that when the build type is Docker, then the different supported styles of passing settings | |
| * via env vars are handled. | |
| */ | |
| public void testDockerBuildRespectsEnvVars() throws Exception { |
pugnascotia
reviewed
Apr 15, 2022
| mockEnvVars.put("setting.Ignored", "baz"); | ||
| callback = env -> { | ||
| Settings settings = env.settings(); | ||
| assertThat(settings.hasValue("simple.setting"), is(true)); |
There was a problem hiding this comment.
What do you think about also checking that the settings have the correct values as well?
pugnascotia
reviewed
Apr 15, 2022
Comment on lines
101
to
102
| public void testDockerEnvSettingsOverride() throws Exception { | ||
| // docker env takes precedence over settings on the command line |
There was a problem hiding this comment.
Suggested change
| public void testDockerEnvSettingsOverride() throws Exception { | |
| // docker env takes precedence over settings on the command line | |
| /** | |
| * Check that with the Docker build, env vars takes precedence over settings on the command line. | |
| */ | |
| public void testDockerBuildEnvVarsOverrideCommandLine() throws Exception { |
pugnascotia
reviewed
Apr 15, 2022
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.hamcrest.Matchers.is; | ||
|
|
||
| public class EnvironmentAwareCommandTests extends CommandTestCase { |
There was a problem hiding this comment.
Can we also please replicate DockerTests.test087EnvironmentVariablesInIncorrectFormatAreIgnored()? It checks that env vars, which look like maybe they ought to be respected, are ignored due to being malformed.
You'll also need to remove these tests from DockerTests, because they rely on being able to inspect the Elasticsearch command line for options:
test086EnvironmentVariablesInSnakeCaseAreTranslatedtest087EnvironmentVariablesInIncorrectFormatAreIgnoredtest088EnvironmentVariablesInDottedFormatArePassedThrough
pugnascotia
requested changes
Apr 15, 2022
|
Thanks Rory! I think I have a addressed all your comments. I normally don't like javadoc comments on test methods and having super descriptive names (which are actually harder to read IMO becauseOfTheLongRunningCamelCaseThatIsHardToVisuallyParse ;) But I added some simple comments to each method to describe what they are checking and standardized on method naming. |
We were checked whether security was enabled or disabled in the environment by running the `env` command and passing it through `grep`. Unforunately, and somewhat obviously, `grep` exits non-zero if it doesn't find a match, which causes `Shell.run` to throw and exception. We could fix it with `runScriptIgnoreExitCode` instead of `run`, but instead I chose just fetch all the `env` values and check them in the Java code.
|
Thanks for the docker test fixes Rory. I think this is ready for a final review now that all tests are passing. |
pugnascotia
approved these changes
Apr 18, 2022
weizijun
added a commit
to weizijun/elasticsearch
that referenced
this pull request
Apr 21, 2022
* master: (104 commits) fix: ordering terms aggregation on top metrics null values (elastic#85774) Fix up whitespace error introduced in elastic#85948 More docs re. removing cluster.initial_master_nodes (elastic#85948) [Test] Remove API key methods from HLRC (elastic#85802) Remove references to bootstrap.system_call_filter (elastic#85964) Move docker cgroup override to SystemJvmOptions (elastic#85960) Add connection accounting tests (elastic#85966) Remove MacOS from platform support testing matrix Remove custom KnnVectorFieldExistsQuery (elastic#85945) Relax data path deprecations from critical to warn (elastic#85952) Remove hppc from some "common" classes (elastic#85957) Move docker env var settings handling out of bash (elastic#85913) Remove hppc from task manager (elastic#85889) [ML] rename trained model allocations to assignments (elastic#85503) Remove hppc from multi*shard request and responses (elastic#85888) Consolidating logging initialization in cli launcher (elastic#85920) Convert license tools to use unified cli entrypoint (elastic#85919) Add noop detection to node shutdown actions (elastic#85914) Adjust SQL expended test output TSDB: Add timestamp provider to AggregationExecutionContext (elastic#85850) ... # Conflicts: # server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionContext.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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