Validate JVM options

2 files changed, 54 insertions, 12 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
index c8b2197fc29..70d01f104dd 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/xml/ContainerModelBuilder.java
@@ -101,7 +101,6 @@ import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
 import java.util.function.Consumer;
-import java.util.logging.Level;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
@@ -1071,18 +1070,20 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
 
     private static class JvmOptions {
 
+        private static final Pattern validPattern = Pattern.compile("-[a-zA-z0-9=:]+");
+
         private final ContainerCluster<?> cluster;
         private final Element nodesElement;
         private final DeployLogger logger;
-        private final boolean isHosted;
         private final boolean legacyOptions;
+        private final boolean failDeploymentWithInvalidJvmOptions;
 
         public JvmOptions(ContainerCluster<?> cluster, Element nodesElement, DeployState deployState, boolean legacyOptions) {
             this.cluster = cluster;
             this.nodesElement = nodesElement;
             this.logger = deployState.getDeployLogger();
-            this.isHosted = deployState.isHosted();
             this.legacyOptions = legacyOptions;
+            this.failDeploymentWithInvalidJvmOptions = deployState.featureFlags().failDeploymentWithInvalidJvmOptions();
         }
 
         String build() {
@@ -1093,7 +1094,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             if (jvmElement == null) return "";
             String jvmOptions = jvmElement.getAttribute(VespaDomBuilder.OPTIONS);
             if (jvmOptions == null) return "";
-            log(jvmOptions);
+            validateJvmOptions(jvmOptions);
             return jvmOptions;
         }
 
@@ -1101,7 +1102,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             String jvmOptions;
             if (nodesElement.hasAttribute(VespaDomBuilder.JVM_OPTIONS)) {
                 jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVM_OPTIONS);
-                log(jvmOptions);
+                validateJvmOptions(jvmOptions);
                 if (nodesElement.hasAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME)) {
                     String jvmArgs = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME);
                     throw new IllegalArgumentException("You have specified both jvm-options='" + jvmOptions + "'" +
@@ -1111,7 +1112,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                 }
             } else {
                 jvmOptions = nodesElement.getAttribute(VespaDomBuilder.JVMARGS_ATTRIB_NAME);
-                log(jvmOptions);
+                validateJvmOptions(jvmOptions);
                 if (incompatibleGCOptions(jvmOptions)) {
                     logger.logApplicationPackage(WARNING, "You need to move your GC-related options from deprecated 'jvmargs' to 'gc-options' in 'jvm' element." +
                             " See https://docs.vespa.ai/en/reference/services-container.html#jvm");
@@ -1121,9 +1122,21 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             return jvmOptions;
         }
 
-        private void log(String jvmOptions) {
-            if (isHosted && jvmOptions != null && !jvmOptions.isEmpty())
-                logger.logApplicationPackage(Level.INFO, "JVM options from services.xml: " + jvmOptions);
+        private void validateJvmOptions(String jvmOptions) {
+            if (jvmOptions == null || jvmOptions.isEmpty()) return;
+
+            String[] optionList = jvmOptions.split(" ");
+            List<String> invalidOptions = Arrays.stream(optionList)
+                                                .filter(option -> !option.isEmpty())
+                                                .filter(option -> !Pattern.matches(validPattern.pattern(), option))
+                                                .sorted()
+                                                .collect(Collectors.toList());
+
+            String message = "Invalid JVM options in services.xml: " + String.join(",", invalidOptions);
+            if (failDeploymentWithInvalidJvmOptions)
+                throw new IllegalArgumentException(message);
+            else
+                logger.logApplicationPackage(WARNING, message);
         }
     }
 
@@ -1182,7 +1195,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             if (options.isEmpty()) return;
 
             Collections.sort(options);
-            String message = "Invalid JVM GC options from services.xml: " + String.join(",", options);
+            String message = "Invalid JVM GC options in services.xml: " + String.join(",", options);
             if (failDeploymentWithInvalidJvmOptions)
                 throw new IllegalArgumentException(message);
             else
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java
index ec3b9386dad..4518f0eb605 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/xml/JvmOptionsTest.java
@@ -181,7 +181,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
                     "-XX:+ParallelGCThreads=8 foo     bar");
             fail();
         } catch (IllegalArgumentException e) {
-            assertTrue(e.getMessage().contains("Invalid JVM GC options from services.xml: bar,foo"));
+            assertTrue(e.getMessage().contains("Invalid JVM GC options in services.xml: bar,foo"));
         }
     }
 
@@ -200,7 +200,7 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
         Pair<Level, String> firstOption = logger.msgs.get(0);
         assertEquals(Level.WARNING, firstOption.getFirst());
         assertEquals("Invalid JVM " + (optionName.equals("gc-options") ? "GC " : "") +
-                             "options from services.xml: " + String.join(",", strings), firstOption.getSecond());
+                             "options in services.xml: " + String.join(",", strings), firstOption.getSecond());
     }
 
     private void buildModelWithJvmOptions(boolean isHosted, TestLogger logger, String optionName, String override) throws IOException, SAXException {
@@ -225,8 +225,37 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
 
     @Test
     public void requireThatJvmOptionsAreLogged()  throws IOException, SAXException {
+        verifyLoggingOfJvmOptions(true,
+                                  "options",
+                                  "-Xms2G foo     bar",
+                                  "foo", "bar");
+        verifyLoggingOfJvmOptions(true,
+                                  "options",
+                                  "$(touch /tmp/hello-from-gc-options)",
+                                  "$(touch", "/tmp/hello-from-gc-options)");
+
+        verifyLoggingOfJvmOptions(false,
+                                  "options",
+                                  "$(touch /tmp/hello-from-gc-options)",
+                                  "$(touch", "/tmp/hello-from-gc-options)");
+
+        // Valid options, should not log anything
         verifyLoggingOfJvmOptions(true, "options", "-Xms2G");
+        verifyLoggingOfJvmOptions(true, "options", "-verbose:gc");
         verifyLoggingOfJvmOptions(false, "options", "-Xms2G");
     }
 
+    @Test
+    public void requireThatInvalidJvmOptionsFailDeployment() throws IOException, SAXException {
+        try {
+            buildModelWithJvmOptions(new TestProperties().setHostedVespa(true).failDeploymentWithInvalidJvmOptions(true),
+                                     new TestLogger(),
+                                     "options",
+                                     "-Xms2G foo     bar");
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("Invalid JVM options in services.xml: bar,foo"));
+        }
+    }
+
 }