fix: respect LS_JAVA_OPTS environment even when optionsfile missing (e…

…lastic#13525) * fix: respect LS_JAVA_OPTS environment even when optionsfile missing * Fixed integration tests * Added unit test to cover the fix * Wipe commented code * Removed redundant log in a path that could never be reached * Moved jvm.options checks into only one place * javaopts: provide injection point for environment string Co-authored-by: andsel <[email protected]>
AsherBond · Jan 18, 2022 · 2a248b2 · 2a248b2
1 parent d4bdcc9
commit 2a248b2
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 13 deletions.
diff --git a/logstash-core/src/main/java/org/logstash/launchers/JvmOptionsParser.java b/logstash-core/src/main/java/org/logstash/launchers/JvmOptionsParser.java
@@ -68,16 +68,16 @@ public static void main(final String[] args) throws InterruptedException, IOExce
                     "Expected two arguments specifying path to LOGSTASH_HOME and an optional LS_JVM_OPTS, but was " + Arrays.toString(args)
             );
         }
+        final String lsJavaOpts = System.getenv("LS_JAVA_OPTS");
+        handleJvmOptions(args, lsJavaOpts);
+    }
 
+    static void handleJvmOptions(String[] args, String lsJavaOpts) {
         final JvmOptionsParser parser = new JvmOptionsParser(args[0]);
         final String jvmOpts = args.length == 2 ? args[1] : null;
         try {
             Optional<Path> jvmOptions = parser.lookupJvmOptionsFile(jvmOpts);
-            if (!jvmOptions.isPresent()) {
-                System.err.println("warning: no jvm.options file found");
-                return;
-            }
-            parser.parse(jvmOptions.get());
+            parser.parseAndInjectEnvironment(jvmOptions, lsJavaOpts);
         } catch (JvmOptionsFileParserException pex) {
             System.err.printf(Locale.ROOT,
                     "encountered [%d] error%s parsing [%s]",
@@ -112,33 +112,56 @@ private Optional<Path> lookupJvmOptionsFile(String jvmOpts) {
                 .findFirst();
     }
 
-    private void parse(Path jvmOptionsFile) throws IOException, JvmOptionsFileParserException {
-        final List<String> jvmOptionsContent = parseJvmOptions(jvmOptionsFile);
-        final String lsJavaOpts = System.getenv("LS_JAVA_OPTS");
+    private void parseAndInjectEnvironment(Optional<Path> jvmOptionsFile, String lsJavaOpts) throws IOException, JvmOptionsFileParserException {
+        final List<String> jvmOptionsContent = new ArrayList<>(parseJvmOptions(jvmOptionsFile));
+
         if (lsJavaOpts != null && !lsJavaOpts.isEmpty()) {
+            if (isDebugEnabled()) {
+                System.err.println("Appending jvm options from environment LS_JAVA_OPTS");
+            }
             jvmOptionsContent.add(lsJavaOpts);
         }
+
         System.out.println(String.join(" ", jvmOptionsContent));
     }
 
-    private List<String> parseJvmOptions(Path jvmOptionsFile) throws IOException, JvmOptionsFileParserException {
-        if (!jvmOptionsFile.toFile().exists()) {
+    private List<String> parseJvmOptions(Optional<Path> jvmOptionsFile) throws IOException, JvmOptionsFileParserException {
+        if (!jvmOptionsFile.isPresent()) {
+            System.err.println("Warning: no jvm.options file found.");
             return Collections.emptyList();
         }
+        final Path optionsFilePath = jvmOptionsFile.get();
+        if (!optionsFilePath.toFile().exists()) {
+            System.err.format("Warning: jvm.options file does not exist or is not readable: `%s`\n", optionsFilePath);
+            return Collections.emptyList();
+        }
+
+        if (isDebugEnabled()) {
+            System.err.format("Processing jvm.options file at `%s`\n", optionsFilePath);
+        }
         final int majorJavaVersion = javaMajorVersion();
 
-        try (InputStream is = Files.newInputStream(jvmOptionsFile);
+        try (InputStream is = Files.newInputStream(optionsFilePath);
              Reader reader = new InputStreamReader(is, StandardCharsets.UTF_8);
              BufferedReader br = new BufferedReader(reader)
         ) {
             final ParseResult parseResults = parse(majorJavaVersion, br);
             if (parseResults.hasErrors()) {
-                throw new JvmOptionsFileParserException(jvmOptionsFile, parseResults.getInvalidLines());
+                throw new JvmOptionsFileParserException(optionsFilePath, parseResults.getInvalidLines());
             }
             return parseResults.getJvmOptions();
         }
     }
 
+    private boolean isDebugEnabled() {
+        final String debug = System.getenv("DEBUG");
+        if (debug == null) {
+            return false;
+        }
+
+        return "1".equals(debug) || Boolean.parseBoolean(debug);
+    }
+
     /**
      * Collector of parsed lines and errors.
      * */

diff --git a/logstash-core/src/test/java/org/logstash/launchers/JvmOptionsParserTest.java b/logstash-core/src/test/java/org/logstash/launchers/JvmOptionsParserTest.java
@@ -1,15 +1,57 @@
 package org.logstash.launchers;
 
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
 
 import java.io.BufferedReader;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.PrintStream;
 import java.io.StringReader;
+import java.lang.reflect.Field;
+import java.util.Map;
 
 import static org.junit.Assert.*;
 
 public class JvmOptionsParserTest {
 
+    @Rule
+    public TemporaryFolder temp = new TemporaryFolder();
+
+    private final PrintStream standardOut = System.out;
+    private final ByteArrayOutputStream outputStreamCaptor = new ByteArrayOutputStream();
+
+    @Before
+    public void setUp() {
+        System.setOut(new PrintStream(outputStreamCaptor));
+    }
+
+    @After
+    public void tearDown() {
+        System.setOut(standardOut);
+    }
+
+    @Test
+    public void test_LS_JAVA_OPTS_isUsedWhenNoJvmOptionsIsAvailable() throws IOException, InterruptedException, ReflectiveOperationException {
+        JvmOptionsParser.handleJvmOptions(new String[] {temp.toString()}, "-Xblabla");
+
+        // Verify
+        final String output = outputStreamCaptor.toString();
+        assertEquals("Output MUST contains the options present in LS_JAVA_OPTS", "-Xblabla\n", output);
+    }
+
+    @SuppressWarnings({ "unchecked" })
+    public static void updateEnv(String name, String val) throws ReflectiveOperationException {
+        Map<String, String> env = System.getenv();
+        Field field = env.getClass().getDeclaredField("m");
+        field.setAccessible(true);
+        ((Map<String, String>) field.get(env)).put(name, val);
+    }
+
+
     @Test
     public void testParseCommentLine() throws IOException {
         final BufferedReader options = asReader("# this is a comment\n-XX:+UseConcMarkSweepGC");

diff --git a/qa/integration/specs/cli/prepare_offline_pack_spec.rb b/qa/integration/specs/cli/prepare_offline_pack_spec.rb
@@ -86,7 +86,7 @@
       filters = @logstash_plugin.list(plugins_to_pack.first)
                                 .stderr_and_stdout.split("\n")
                                 .delete_if do |line|
-                                  line =~ /cext|├──|└──|logstash-integration|JAVA_OPT|fatal|^WARNING|^warning: ignoring JAVA_TOOL_OPTIONS|^OpenJDK 64-Bit Server VM warning|Option \w+ was deprecated|Using LS_JAVA_HOME defined java|Using system java: |\[\[: not found/
+                                  line =~ /cext|├──|└──|logstash-integration|JAVA_OPT|fatal|^WARNING|^warning: ignoring JAVA_TOOL_OPTIONS|^OpenJDK 64-Bit Server VM warning|Option \w+ was deprecated|Using LS_JAVA_HOME defined java|Using system java: |\[\[: not found|^Warning: no jvm.options file found|^Processing jvm.options file at/
                                 end
 
       expect(unpacked.plugins.collect(&:name)).to include(*filters)