From 604414a23c3a4f12de05d6808a45db83a569198c Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Tue, 21 Dec 2021 10:39:15 +0100 Subject: Validate JVM options --- .../model/container/xml/ContainerModelBuilder.java | 33 +++++++++++++++------- .../vespa/model/container/xml/JvmOptionsTest.java | 33 ++++++++++++++++++++-- 2 files changed, 54 insertions(+), 12 deletions(-) (limited to 'config-model') 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 { 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 { 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 { 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 { } } 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 { 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 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 { 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 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")); + } + } + } -- cgit v1.2.3