Merge pull request #20696 from vespa-engine/hmusum/validate-jvm-options-4

Allow comma in JVM options

2 files changed, 29 insertions, 7 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 657eb6a29e7..e961b39ff8e 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
@@ -1061,15 +1061,22 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
         return CONTAINER_TAG.equals(element.getTagName()) || DEPRECATED_CONTAINER_TAG.equals(element.getTagName());
     }
 
-    private static class JvmOptions {
+    /**
+     * Validates JVM options and logs a warning or fails deployment (depending on feature flag)
+     * if anyone of them has invalid syntax or is an option that is unsupported for the running system.
+     */
+     private static class JvmOptions {
 
-        private static final Pattern validPattern = Pattern.compile("-[a-zA-z0-9=:./]+");
+        private static final Pattern validPattern = Pattern.compile("-[a-zA-z0-9=:./,]+");
+        // debug port will not be available in hosted, don't allow
+        private static final Pattern invalidInHostedatttern = Pattern.compile("-Xrunjdwp:transport=.*");
 
         private final ContainerCluster<?> cluster;
         private final Element nodesElement;
         private final DeployLogger logger;
         private final boolean legacyOptions;
         private final boolean failDeploymentWithInvalidJvmOptions;
+        private final boolean isHosted;
 
         public JvmOptions(ContainerCluster<?> cluster, Element nodesElement, DeployState deployState, boolean legacyOptions) {
             this.cluster = cluster;
@@ -1077,6 +1084,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             this.logger = deployState.getDeployLogger();
             this.legacyOptions = legacyOptions;
             this.failDeploymentWithInvalidJvmOptions = deployState.featureFlags().failDeploymentWithInvalidJvmOptions();
+            this.isHosted = deployState.isHosted();
         }
 
         String build() {
@@ -1086,7 +1094,7 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
             Element jvmElement = XML.getChild(nodesElement, "jvm");
             if (jvmElement == null) return "";
             String jvmOptions = jvmElement.getAttribute(VespaDomBuilder.OPTIONS);
-            if (jvmOptions == null) return "";
+            if (jvmOptions.isEmpty()) return "";
             validateJvmOptions(jvmOptions);
             return jvmOptions;
         }
@@ -1135,6 +1143,12 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
                                                 .filter(option -> !Pattern.matches(validPattern.pattern(), option))
                                                 .sorted()
                                                 .collect(Collectors.toList());
+            if (isHosted)
+                invalidOptions.addAll(Arrays.stream(optionList)
+                                            .filter(option -> !option.isEmpty())
+                                            .filter(option -> Pattern.matches(invalidInHostedatttern.pattern(), option))
+                                            .sorted()
+                                            .collect(Collectors.toList()));
 
             if (invalidOptions.isEmpty()) return;
 
@@ -1147,8 +1161,9 @@ public class ContainerModelBuilder extends ConfigModelBuilder<ContainerModel> {
     }
 
     /**
-     * Validates JVM GC options and logs a warning if anyone of them has invalid syntax or is an option
-     * that is unsupported for the running system (e.g. uses CMS options for hosted Vespa, which uses JDK 17)
+     * Validates JVM GC options and logs a warning or fails deployment (depending on feature flag)
+     * if anyone of them has invalid syntax or is an option that is unsupported for the running system
+     * (e.g. uses CMS options for hosted Vespa, which uses JDK 17).
      */
     private static class JvmGcOptions {
 
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 96fbce03e88..ba27deedb61 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
@@ -196,13 +196,15 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
         List<String> strings = Arrays.asList(invalidOptions.clone());
         // Verify that nothing is logged if there are no invalid options
         if (strings.isEmpty()) {
-            assertEquals(0, logger.msgs.size());
+            assertEquals(logger.msgs.size() > 0 ? logger.msgs.get(0).getSecond() : "", 0, logger.msgs.size());
             return;
         }
 
-        Collections.sort(strings);
+        assertTrue("Expected 1 or more log messages for invalid JM options, got none", logger.msgs.size() > 0);
         Pair<Level, String> firstOption = logger.msgs.get(0);
         assertEquals(Level.WARNING, firstOption.getFirst());
+
+        Collections.sort(strings);
         assertEquals("Invalid JVM " + (optionName.equals("gc-options") ? "GC " : "") +
                              "options in services.xml: " + String.join(",", strings), firstOption.getSecond());
     }
@@ -238,6 +240,11 @@ public class JvmOptionsTest extends ContainerModelBuilderTestBase {
                                   "$(touch /tmp/hello-from-gc-options)",
                                   "$(touch", "/tmp/hello-from-gc-options)");
 
+        verifyLoggingOfJvmOptions(true,
+                                  "options",
+                                  "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005",
+                                  "-Xrunjdwp:transport=dt_socket,server=y,suspend=n,address=5005");
+
         verifyLoggingOfJvmOptions(false,
                                   "options",
                                   "$(touch /tmp/hello-from-gc-options)",