From ea5e7a3565b00457bdbb5a153dd3c1fc763761c6 Mon Sep 17 00:00:00 2001
From: Rob Bavey <rob.bavey@elastic.co>
Date: Wed, 2 Mar 2022 17:26:20 -0500
Subject: [PATCH] Move mandatory JVM options out of `jvm.options` and into
 JvmOptionsParser

Certain values for the JVM are mandatory for Logstash to function correctly. Rather than
leave them in config/jvm.options where they can be updated, and can cause upgrade issues
for users when we add new mandatory options, move them into code where they cannot be
changed
---
 config/jvm.options                            |  16 +-
 .../logstash/launchers/JvmOptionsParser.java  | 178 ++++++++++--------
 .../launchers/JvmOptionsParserTest.java       |  23 ++-
 3 files changed, 125 insertions(+), 92 deletions(-)

diff --git a/config/jvm.options b/config/jvm.options
index 842a302dde3..cab8f03c338 100644
--- a/config/jvm.options
+++ b/config/jvm.options
@@ -49,8 +49,6 @@
 -Djruby.compile.invokedynamic=true
 # Force Compilation
 -Djruby.jit.threshold=0
-# Make sure joni regexp interruptability is enabled
--Djruby.regexp.interruptible=true
 
 ## heap dumps
 
@@ -73,16 +71,4 @@
 -Djava.security.egd=file:/dev/urandom
 
 # Copy the logging context from parent threads to children
--Dlog4j2.isThreadContextMapInheritable=true
-
-11-:--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
-11-:--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED
-11-:--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
-11-:--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED
-11-:--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED
-
-11-:--add-opens=java.base/java.security=ALL-UNNAMED
-11-:--add-opens=java.base/java.io=ALL-UNNAMED
-11-:--add-opens=java.base/java.nio.channels=ALL-UNNAMED
-11-:--add-opens=java.base/sun.nio.ch=ALL-UNNAMED
-11-:--add-opens=java.management/sun.management=ALL-UNNAMED
+-Dlog4j2.isThreadContextMapInheritable=true
\ No newline at end of file
diff --git a/logstash-core/src/main/java/org/logstash/launchers/JvmOptionsParser.java b/logstash-core/src/main/java/org/logstash/launchers/JvmOptionsParser.java
index 04958cd6ccc..f1d17b5b0dc 100644
--- a/logstash-core/src/main/java/org/logstash/launchers/JvmOptionsParser.java
+++ b/logstash-core/src/main/java/org/logstash/launchers/JvmOptionsParser.java
@@ -14,14 +14,17 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 
 /**
@@ -29,6 +32,20 @@
  * */
 public class JvmOptionsParser {
 
+    private static final String[] MANDATORY_JVM_OPTIONS = new String[]{
+            "-Djruby.regexp.interruptible=true",
+            "16-:--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
+            "16-:--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED",
+            "16-:--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED",
+            "16-:--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
+            "16-:--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
+            "11-:--add-opens=java.base/java.security=ALL-UNNAMED",
+            "11-:--add-opens=java.base/java.io=ALL-UNNAMED",
+            "11-:--add-opens=java.base/java.nio.channels=ALL-UNNAMED",
+            "11-:--add-opens=java.base/sun.nio.ch=ALL-UNNAMED",
+            "11-:--add-opens=java.management/sun.management=ALL-UNNAMED"
+    };
+
     static class JvmOptionsFileParserException extends Exception {
 
         private static final long serialVersionUID = 2446165130736962758L;
@@ -71,8 +88,7 @@ public static void main(final String[] args) throws InterruptedException, IOExce
             );
         }
         bailOnOldJava();
-        final String lsJavaOpts = System.getenv("LS_JAVA_OPTS");
-        handleJvmOptions(args, lsJavaOpts);
+        handleJvmOptions(args, System.getenv("LS_JAVA_OPTS"));
     }
 
     static void bailOnOldJava(){
@@ -93,7 +109,7 @@ static void handleJvmOptions(String[] args, String lsJavaOpts) {
         final String jvmOpts = args.length == 2 ? args[1] : null;
         try {
             Optional<Path> jvmOptions = parser.lookupJvmOptionsFile(jvmOpts);
-            parser.parseAndInjectEnvironment(jvmOptions, lsJavaOpts);
+            parser.handleJvmOptions(jvmOptions, lsJavaOpts);
         } catch (JvmOptionsFileParserException pex) {
             System.err.printf(Locale.ROOT,
                     "encountered [%d] error%s parsing [%s]",
@@ -128,20 +144,33 @@ private Optional<Path> lookupJvmOptionsFile(String jvmOpts) {
                 .findFirst();
     }
 
-    private void parseAndInjectEnvironment(Optional<Path> jvmOptionsFile, String lsJavaOpts) throws IOException, JvmOptionsFileParserException {
-        final List<String> jvmOptionsContent = new ArrayList<>(parseJvmOptions(jvmOptionsFile));
+    private void handleJvmOptions(Optional<Path> jvmOptionsFile, String lsJavaOpts) throws IOException, JvmOptionsFileParserException {
+        int javaMajorVersion = javaMajorVersion();
+
+        // Add JVM Options from config/jvm.options
+        final Set<String> jvmOptionsContent = new LinkedHashSet<>(getJvmOptionsFromFile(jvmOptionsFile, javaMajorVersion));
 
+        // Add JVM Options from LS_JAVA_OPTS
         if (lsJavaOpts != null && !lsJavaOpts.isEmpty()) {
             if (isDebugEnabled()) {
                 System.err.println("Appending jvm options from environment LS_JAVA_OPTS");
             }
             jvmOptionsContent.add(lsJavaOpts);
         }
+        // Set mandatory JVM options
+        jvmOptionsContent.addAll(getMandatoryJvmOptions(javaMajorVersion));
 
         System.out.println(String.join(" ", jvmOptionsContent));
     }
 
-    private List<String> parseJvmOptions(Optional<Path> jvmOptionsFile) throws IOException, JvmOptionsFileParserException {
+     static List<String> getMandatoryJvmOptions(int javaMajorVersion){
+          return Arrays.stream(MANDATORY_JVM_OPTIONS)
+                  .map(option -> jvmOptionFromLine(javaMajorVersion, option))
+                  .flatMap(Optional::stream)
+                  .collect(Collectors.toList());
+    }
+
+    private List<String> getJvmOptionsFromFile(final Optional<Path> jvmOptionsFile, final int javaMajorVersion) throws IOException, JvmOptionsFileParserException {
         if (!jvmOptionsFile.isPresent()) {
             System.err.println("Warning: no jvm.options file found.");
             return Collections.emptyList();
@@ -155,13 +184,11 @@ private List<String> parseJvmOptions(Optional<Path> jvmOptionsFile) throws IOExc
         if (isDebugEnabled()) {
             System.err.format("Processing jvm.options file at `%s`\n", optionsFilePath);
         }
-        final int majorJavaVersion = javaMajorVersion();
-
         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);
+            final ParseResult parseResults = parse(javaMajorVersion, br);
             if (parseResults.hasErrors()) {
                 throw new JvmOptionsFileParserException(optionsFilePath, parseResults.getInvalidLines());
             }
@@ -209,7 +236,36 @@ public List<String> getJvmOptions() {
     private static final Pattern OPTION_DEFINITION = Pattern.compile("((?<start>\\d+)(?<range>-)?(?<end>\\d+)?:)?(?<option>-.*)$");
 
     /**
-     * Parse the line-delimited JVM options from the specified buffered reader for the specified Java major version.
+     *
+     * If the version syntax specified on a line matches the specified JVM options, the JVM option callback will be invoked with the JVM
+     * option. If the line does not match the specified syntax for the JVM options, the invalid line callback will be invoked with the
+     * contents of the entire line.
+     *
+     * @param javaMajorVersion the Java major version to match JVM options against
+     * @param br the buffered reader to read line-delimited JVM options from
+     * @return the admitted options lines respecting the javaMajorVersion and the error lines
+     * @throws IOException if an I/O exception occurs reading from the buffered reader
+     */
+    static ParseResult parse(final int javaMajorVersion, final BufferedReader br) throws IOException {
+        final ParseResult result = new ParseResult();
+        int lineNumber = 0;
+        while (true) {
+            final String line = br.readLine();
+            lineNumber++;
+            if (line == null) {
+                break;
+            }
+            try{
+                jvmOptionFromLine(javaMajorVersion, line).ifPresent(result::appendOption);
+            } catch (IllegalArgumentException e){
+                result.appendError(lineNumber, line);
+            };
+        }
+        return result;
+    }
+
+    /**
+     * Parse the line-delimited JVM options from the specified string for the specified Java major version.
      * Valid JVM options are:
      * <ul>
      *     <li>
@@ -256,82 +312,52 @@ public List<String> getJvmOptions() {
      *         {@code 9-10:-Xlog:age*=trace,gc*,safepoint:file=logs/gc.log:utctime,pid,tags:filecount=32,filesize=64m}
      *     </li>
      * </ul>
-     *
-     * If the version syntax specified on a line matches the specified JVM options, the JVM option callback will be invoked with the JVM
-     * option. If the line does not match the specified syntax for the JVM options, the invalid line callback will be invoked with the
-     * contents of the entire line.
-     *
-     * @param javaMajorVersion the Java major version to match JVM options against
-     * @param br the buffered reader to read line-delimited JVM options from
-     * @return the admitted options lines respecting the javaMajorVersion and the error lines
-     * @throws IOException if an I/O exception occurs reading from the buffered reader
+
+     * @param javaMajorVersion
+     * @param line
+     * @return Returns an Optional containing a string if the line contains a valid option for the specified Java
+     *         version and empty otherwise
      */
-    static ParseResult parse(final int javaMajorVersion, final BufferedReader br) throws IOException {
-        final ParseResult result = new ParseResult();
-        int lineNumber = 0;
-        while (true) {
-            final String line = br.readLine();
-            lineNumber++;
-            if (line == null) {
-                break;
-            }
-            if (line.startsWith("#")) {
-                // lines beginning with "#" are treated as comments
-                continue;
-            }
-            if (line.matches("\\s*")) {
-                // skip blank lines
-                continue;
-            }
-            final Matcher matcher = OPTION_DEFINITION.matcher(line);
-            if (matcher.matches()) {
-                final String start = matcher.group("start");
-                final String end = matcher.group("end");
-                if (start == null) {
-                    // no range present, unconditionally apply the JVM option
-                    result.appendOption(line);
+    private static Optional<String> jvmOptionFromLine(final int javaMajorVersion, final String line){
+        if (line.startsWith("#") || line.matches("\\s*")) {
+            // Skip comments and blank lines
+            return Optional.empty();
+        }
+        final Matcher matcher = OPTION_DEFINITION.matcher(line);
+        if (matcher.matches()) {
+            final String start = matcher.group("start");
+            final String end = matcher.group("end");
+            if (start == null) {
+                // no range present, unconditionally apply the JVM option
+                return Optional.of(line);
+            } else {
+                final int lower = Integer.parseInt(start);
+                final int upper;
+                if (matcher.group("range") == null) {
+                    // no range is present, apply the JVM option to the specified major version only
+                    upper = lower;
+                } else if (end == null) {
+                    // a range of the form \\d+- is present, apply the JVM option to all major versions larger than the specified one
+                    upper = Integer.MAX_VALUE;
                 } else {
-                    final int lower;
-                    try {
-                        lower = Integer.parseInt(start);
-                    } catch (final NumberFormatException e) {
-                        result.appendError(lineNumber, line);
-                        continue;
-                    }
-                    final int upper;
-                    if (matcher.group("range") == null) {
-                        // no range is present, apply the JVM option to the specified major version only
-                        upper = lower;
-                    } else if (end == null) {
-                        // a range of the form \\d+- is present, apply the JVM option to all major versions larger than the specified one
-                        upper = Integer.MAX_VALUE;
-                    } else {
-                        // a range of the form \\d+-\\d+ is present, apply the JVM option to the specified range of major versions
-                        try {
-                            upper = Integer.parseInt(end);
-                        } catch (final NumberFormatException e) {
-                            result.appendError(lineNumber, line);
-                            continue;
-                        }
-                        if (upper < lower) {
-                            result.appendError(lineNumber, line);
-                            continue;
-                        }
-                    }
-                    if (lower <= javaMajorVersion && javaMajorVersion <= upper) {
-                        result.appendOption(matcher.group("option"));
+                        upper = Integer.parseInt(end);
+                    if (upper < lower) {
+                        throw new IllegalArgumentException("Upper bound must be greater than lower bound");
                     }
                 }
-            } else {
-                result.appendError(lineNumber, line);
+                if (lower <= javaMajorVersion && javaMajorVersion <= upper) {
+                    return Optional.of(matcher.group("option"));
+                }
             }
+        } else {
+            throw new IllegalArgumentException("Illegal JVM Option");
         }
-        return result;
+        return Optional.empty();
     }
 
     private static final Pattern JAVA_VERSION = Pattern.compile("^(?:1\\.)?(?<javaMajorVersion>\\d+)(?:\\.\\d+)?$");
 
-    private int javaMajorVersion() {
+    public static int javaMajorVersion() {
         final String specVersion = System.getProperty("java.specification.version");
         final Matcher specVersionMatcher = JAVA_VERSION.matcher(specVersion);
         if (!specVersionMatcher.matches()) {
diff --git a/logstash-core/src/test/java/org/logstash/launchers/JvmOptionsParserTest.java b/logstash-core/src/test/java/org/logstash/launchers/JvmOptionsParserTest.java
index 109e025b3da..d271194db1f 100644
--- a/logstash-core/src/test/java/org/logstash/launchers/JvmOptionsParserTest.java
+++ b/logstash-core/src/test/java/org/logstash/launchers/JvmOptionsParserTest.java
@@ -40,7 +40,7 @@ public void test_LS_JAVA_OPTS_isUsedWhenNoJvmOptionsIsAvailable() throws IOExcep
 
         // Verify
         final String output = outputStreamCaptor.toString();
-        assertEquals("Output MUST contains the options present in LS_JAVA_OPTS", "-Xblabla" + System.lineSeparator(), output);
+        assertTrue("Output MUST contains the options present in LS_JAVA_OPTS", output.contains("-Xblabla"));
     }
 
     @SuppressWarnings({ "unchecked" })
@@ -89,6 +89,27 @@ public void testParseOptionVersionRange() throws IOException {
         assertTrue("No option match outside the range [10-11]", res.getJvmOptions().isEmpty());
     }
 
+    @Test
+    public void testMandatoryJvmOptionApplicableJvmPresent() throws IOException{
+        assertTrue("Contains add-exports value for Java 17",
+                JvmOptionsParser.getMandatoryJvmOptions(17).contains("--add-exports=jdk.compiler/com.sun.tools.javac.api==ALL-UNNAMED"));
+    }
+
+    @Test
+    public void testMandatoryJvmOptionNonApplicableJvmNotPresent() throws IOException{
+        assertFalse("Does not contains add-exports value for Java 11",
+                JvmOptionsParser.getMandatoryJvmOptions(11).contains("--add-exports=jdk.compiler/com.sun.tools.javac.api==ALL-UNNAMED"));
+    }
+
+    @Test
+    public void testAlwaysMandatoryJvmPresent() {
+        assertTrue("Contains regexp interruptible for Java 11",
+                JvmOptionsParser.getMandatoryJvmOptions(11).contains("-Djruby.regexp.interruptible=true"));
+        assertTrue("Contains regexp interruptible for Java 17",
+                JvmOptionsParser.getMandatoryJvmOptions(17).contains("-Djruby.regexp.interruptible=true"));
+
+    }
+
     @Test
     public void testErrorLinesAreReportedCorrectly() throws IOException {
         final String jvmOptionsContent = "10-11:-XX:+UseConcMarkSweepGC" + System.lineSeparator() +