From a2a2ee7af867c8c08af0141a8f7a0a728d9dae0e Mon Sep 17 00:00:00 2001 From: Bernd Ahlers Date: Fri, 7 Apr 2023 12:49:40 +0200 Subject: [PATCH] Don't pass JAVA_HOME environment variable to OpenSearch process (#15166) The OpenSearch process should use its bundled JVM version. --- .../management/CommandLineProcess.java | 32 +++++++++++++++++-- .../management/CommandLineProcessTest.java | 32 ++++++++++++++++++- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/data-node/src/main/java/org/graylog/datanode/management/CommandLineProcess.java b/data-node/src/main/java/org/graylog/datanode/management/CommandLineProcess.java index e965ded2bd..b1bb9e730e 100644 --- a/data-node/src/main/java/org/graylog/datanode/management/CommandLineProcess.java +++ b/data-node/src/main/java/org/graylog/datanode/management/CommandLineProcess.java @@ -16,6 +16,7 @@ */ package org.graylog.datanode.management; +import com.google.common.annotations.VisibleForTesting; import org.apache.commons.exec.CommandLine; import org.apache.commons.exec.ExecuteWatchdog; import org.apache.commons.exec.PumpStreamHandler; @@ -26,25 +27,44 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.nio.file.Path; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.function.Supplier; +import java.util.stream.Collectors; + +import static java.util.function.Predicate.not; class CommandLineProcess { + private static final String JAVA_HOME_ENV = "JAVA_HOME"; private final Path executable; private final List arguments; private final ProcessListener listener; + private final Supplier executorSupplier; + private final Supplier> environmentSupplier; private final ExecuteWatchdog watchDog; private Process process; public CommandLineProcess(Path executable, List arguments, ProcessListener listener) { + this(executable, arguments, listener, ProcessProvidingExecutor::new, System::getenv); + } + + @VisibleForTesting + CommandLineProcess(Path executable, + List arguments, + ProcessListener listener, + Supplier executorSupplier, + Supplier> environmentSupplier) { this.executable = executable; this.arguments = arguments; this.listener = listener; + this.environmentSupplier = environmentSupplier; + this.executorSupplier = executorSupplier; this.watchDog = new ExecuteWatchdog(ExecuteWatchdog.INFINITE_TIMEOUT); } @@ -58,12 +78,12 @@ class CommandLineProcess { arguments.forEach(it -> cmdLine.addArgument(it, true)); - ProcessProvidingExecutor executor = new ProcessProvidingExecutor(); + ProcessProvidingExecutor executor = executorSupplier.get(); //executor.setStreamHandler(logs); executor.setStreamHandler(new PumpStreamHandler(new LoggingOutputStream(listener::onStdOut), new LoggingOutputStream(listener::onStdErr))); executor.setWatchdog(watchDog); - executor.execute(cmdLine, listener); + executor.execute(cmdLine, cleanEnvironment(environmentSupplier.get()), listener); try { this.process = executor.getProcess().get(30, TimeUnit.SECONDS); listener.onStart(); @@ -74,9 +94,17 @@ class CommandLineProcess { } } + private Map cleanEnvironment(Map env) { + return env.entrySet().stream() + // Remove JAVA_HOME from environment because OpenSearch should use its bundled JVM. + .filter(not(entry -> JAVA_HOME_ENV.equals(entry.getKey()))) + .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + } + public void stop() { this.watchDog.destroyProcess(); } + /** * "Do not rely on the undelying process if not necessary" */ diff --git a/data-node/src/test/java/org/graylog/datanode/management/CommandLineProcessTest.java b/data-node/src/test/java/org/graylog/datanode/management/CommandLineProcessTest.java index 339a7b6e8d..9ddac53dc4 100644 --- a/data-node/src/test/java/org/graylog/datanode/management/CommandLineProcessTest.java +++ b/data-node/src/test/java/org/graylog/datanode/management/CommandLineProcessTest.java @@ -21,13 +21,16 @@ import com.github.rholder.retry.Retryer; import com.github.rholder.retry.RetryerBuilder; import com.github.rholder.retry.StopStrategies; import com.github.rholder.retry.WaitStrategies; +import org.apache.commons.exec.CommandLine; import org.apache.commons.exec.ExecuteException; import org.assertj.core.api.Assertions; +import org.graylog.datanode.ProcessProvidingExecutor; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.File; import java.io.IOException; import java.net.URISyntaxException; import java.net.URL; @@ -37,6 +40,7 @@ import java.nio.file.attribute.PosixFilePermissions; import java.util.Collections; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; @@ -89,7 +93,14 @@ class CommandLineProcessTest { LOG.info("On process failed:", e); } }; - final CommandLineProcess process = new CommandLineProcess(binPath, Collections.emptyList(), listener); + final TestExecutor testExecutor = new TestExecutor(); + final CommandLineProcess process = new CommandLineProcess( + binPath, + Collections.emptyList(), + listener, + () -> testExecutor, + () -> Map.of("USER", "test", "JAVA_HOME", "/path/to/jre") + ); process.start(); waitTillLogsAreAvailable(stdout, 3); @@ -106,6 +117,8 @@ class CommandLineProcessTest { .hasSize(1) .contains("This message goes to stderr"); + Assertions.assertThat(testExecutor.getEnvironment()) + .doesNotContainKey("JAVA_HOME"); } private void waitTillLogsAreAvailable(List logs, int expectedLinesCount) throws ExecutionException, RetryException { @@ -155,4 +168,21 @@ class CommandLineProcessTest { Assertions.assertThat(exitCode).isEqualTo(143); } + + /** + * Implementation that allows access to the environment argument. + */ + private static class TestExecutor extends ProcessProvidingExecutor { + private Map environment = Map.of(); + + @Override + protected Process launch(CommandLine command, Map env, File dir) throws IOException { + this.environment = env; + return super.launch(command, env, dir); + } + + public Map getEnvironment() { + return environment; + } + } }