[COPS-5211] Fix marathon constraint parser bug (#3160)

* Upgrade jackson to 2.9 to use DeserializationFeature.FAIL_ON_TRAILING_TOKENS * Add/update unit tests
d2iq-archive · Aug 21, 2019 · 240a163 · 240a163
1 parent 2128302
commit 240a163
Show file tree

Hide file tree

Showing 9 changed files with 52 additions and 41 deletions.
diff --git a/sdk/scheduler/build.gradle b/sdk/scheduler/build.gradle
@@ -111,7 +111,7 @@ configurations {
 
 ext {
     // Only include version numbers here if it'd be redundant to repeat them below:
-    jacksonVer = "2.6.7"
+    jacksonVer = "2.9.9"
     curatorVer = "4.0.1"
     httpClientVer = "4.5.2"
     jerseyVer = "2.23"
@@ -125,7 +125,7 @@ dependencies {
     compile "com.fasterxml.jackson.datatype:jackson-datatype-json-org:${jacksonVer}"
     compile "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:${jacksonVer}"
     compile "com.fasterxml.jackson.core:jackson-databind:${jacksonVer}"
-    compile 'com.hubspot.jackson:jackson-datatype-protobuf:0.9.9-preJackson2.7-proto3'
+    compile 'com.hubspot.jackson:jackson-datatype-protobuf:0.9.11-jackson2.9'
     compile 'com.googlecode.protobuf-java-format:protobuf-java-format:1.4'
     compile 'com.github.spullara.mustache.java:compiler:0.9.2'
     compile 'commons-codec:commons-codec:1.11'

diff --git a/...r/src/main/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParser.java b/...r/src/main/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParser.java
@@ -3,6 +3,7 @@
 import com.mesosphere.sdk.offer.LoggingUtils;
 
 import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.annotations.VisibleForTesting;
 import jersey.repackaged.com.google.common.collect.Lists;
@@ -80,7 +81,7 @@ public static PlacementRule parse(String podName, String marathonConstraints) {
       return new AndRule(rowRules);
 
     } catch (IOException e) {
-      LOGGER.error("Failed to parse marathon constraints", podName, marathonConstraints);
+      LOGGER.error("Failed to parse marathon constraints [{}] for {}", marathonConstraints, podName);
       return new InvalidPlacementRule(marathonConstraints, e.getMessage());
     }
   }
@@ -126,6 +127,8 @@ private static PlacementRule parseRow(
   @VisibleForTesting
   static List<List<String>> splitConstraints(String marathonConstraints) {
     ObjectMapper mapper = new ObjectMapper();
+    // Always parse the string in full. Leftover trailing tokens result in incomplete (and may be invalid) rules.
+    mapper.enable(DeserializationFeature.FAIL_ON_TRAILING_TOKENS);
     // The marathon doc uses a format like: '[["a", "b", "c"], ["d", "e"]]'
     // Meanwhile the marathon web interface uses a format like: 'a:b:c,d:e'
     try {

diff --git a/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/DefaultPodSpec.java b/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/DefaultPodSpec.java
@@ -575,13 +575,10 @@ public Builder seccompProfileName(String seccompProfileName) {
     }
 
     /**
-     * Returns a {@code DefaultPodSpec} built from the parameters previously set.
-     *
      * @return a {@code DefaultPodSpec} built with parameters of this {@code DefaultPodSpec.Builder}
      */
     public DefaultPodSpec build() {
-      DefaultPodSpec defaultPodSpec = new DefaultPodSpec(this);
-      return defaultPodSpec;
+      return new DefaultPodSpec(this);
     }
   }
 }
diff --git a/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawNetwork.java b/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawNetwork.java
@@ -31,7 +31,7 @@ private RawNetwork(
    * Included so that we support empty network specifications (e.g. a network of {@code networks: dcos:}).
    */
   @JsonCreator
-  private RawNetwork(String ignored) {
+  private RawNetwork() {
     this(Collections.emptyList(), Collections.emptyList(), "");
   }
 

diff --git a/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawPod.java b/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawPod.java
@@ -4,6 +4,8 @@
 
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.annotation.JsonSetter;
+import com.fasterxml.jackson.annotation.Nulls;
 import org.apache.commons.collections.CollectionUtils;
 
 import java.util.Collection;
@@ -22,7 +24,8 @@ public final class RawPod {
 
   private final String image;
 
-  private final WriteOnceLinkedHashMap<String, RawNetwork> networks;
+  @JsonSetter(contentNulls = Nulls.AS_EMPTY)
+  private WriteOnceLinkedHashMap<String, RawNetwork> networks;
 
   private final WriteOnceLinkedHashMap<String, RawRLimit> rlimits;
 
@@ -56,7 +59,6 @@ private RawPod(
       @JsonProperty("placement") String placement,
       @JsonProperty("count") Integer count,
       @JsonProperty("image") String image,
-      @JsonProperty("networks") WriteOnceLinkedHashMap<String, RawNetwork> networks,
       @JsonProperty("rlimits") WriteOnceLinkedHashMap<String, RawRLimit> rlimits,
       @JsonProperty("uris") Collection<String> uris,
       @JsonProperty("tasks") WriteOnceLinkedHashMap<String, RawTask> tasks,
@@ -73,7 +75,6 @@ private RawPod(
     this.placement = placement;
     this.count = count;
     this.image = image;
-    this.networks = networks == null ? new WriteOnceLinkedHashMap<>() : networks;
     this.rlimits = rlimits == null ? new WriteOnceLinkedHashMap<>() : rlimits;
     this.uris = uris;
     this.tasks = tasks;

diff --git a/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawServiceSpec.java b/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawServiceSpec.java
@@ -29,6 +29,7 @@ public final class RawServiceSpec {
   static {
     // If the user provides duplicate fields (e.g. 'count' twice), throw an error instead of silently dropping data:
     YAML_MAPPER.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION);
+    YAML_MAPPER.enable(JsonParser.Feature.ALLOW_YAML_COMMENTS);
   }
 
   private final String name;

diff --git a/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/YAMLToInternalMappers.java b/sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/YAMLToInternalMappers.java
@@ -300,9 +300,8 @@ private static PodSpec convertPod(
     WriteOnceLinkedHashMap<String, RawNetwork> rawNetworks = rawPod.getNetworks();
     final Collection<NetworkSpec> networks = new ArrayList<>();
     if (MapUtils.isNotEmpty(rawNetworks)) {
-      networks.addAll(rawNetworks.entrySet().stream()
-          .map(rawNetworkEntry -> {
-            String networkName = rawNetworkEntry.getKey();
+      networks.addAll(rawNetworks.keySet().stream()
+          .map(networkName -> {
             DcosConstants.warnIfUnsupportedNetwork(networkName);
             networkNames.add(networkName);
             RawNetwork rawNetwork = rawNetworks.get(networkName);

diff --git a/...c/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java b/...c/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java
@@ -2,7 +2,6 @@
 
 import org.junit.Test;
 
-import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 
@@ -16,7 +15,7 @@ public class MarathonConstraintParserTest {
     static final String POD_NAME = "hello";
 
     @Test
-    public void testSplitConstraints() throws IOException {
+    public void testSplitConstraints() {
         assertEquals(Arrays.asList(Arrays.asList("")),
                 MarathonConstraintParser.splitConstraints(""));
         assertEquals(Arrays.asList(Arrays.asList("a")),
@@ -41,7 +40,7 @@ public void testSplitConstraints() throws IOException {
     }
 
     @Test
-    public void testUniqueOperator() throws IOException {
+    public void testUniqueOperator() {
         // example from marathon documentation:
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['hostname', 'UNIQUE']]")).toString();
         assertEquals("MaxPerHostnameRule{max=1, task-filter=RegexMatcher{pattern='hello-.*'}}", constraintStr);
@@ -56,7 +55,7 @@ public void testUniqueOperator() throws IOException {
     }
 
     @Test
-    public void testClusterOperator() throws IOException {
+    public void testClusterOperator() {
         // example from marathon documentation:
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'CLUSTER', 'rack-1']]")).toString();
         assertEquals("AttributeRule{matcher=ExactMatcher{str='rack-id:rack-1'}}", constraintStr);
@@ -71,7 +70,7 @@ public void testClusterOperator() throws IOException {
     }
 
     @Test
-    public void testGroupByOperator() throws IOException {
+    public void testGroupByOperator() {
         // example from marathon documentation:
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'GROUP_BY']]")).toString();
         assertEquals("RoundRobinByAttributeRule{attribute=rack-id, attribute-count=Optional.empty, task-filter=RegexMatcher{pattern='hello-.*'}}", constraintStr);
@@ -101,7 +100,7 @@ public void testGroupByOperator() throws IOException {
     }
 
     @Test
-    public void testLikeOperator() throws IOException {
+    public void testLikeOperator() {
         // example from marathon documentation:
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'LIKE', 'rack-[1-3]']]")).toString();
         assertEquals("AttributeRule{matcher=RegexMatcher{pattern='rack-id:rack-[1-3]'}}", constraintStr);
@@ -115,7 +114,7 @@ public void testLikeOperator() throws IOException {
     }
 
     @Test
-    public void testIsOperator() throws IOException {
+    public void testIsOperator() {
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['foo', 'IS', 'bar']]")).toString();
         assertEquals("AttributeRule{matcher=ExactMatcher{str='foo:bar'}}", constraintStr);
         assertEquals(constraintStr, MarathonConstraintParser.parse(POD_NAME, unescape("['foo', 'IS', 'bar']")).toString());
@@ -138,7 +137,7 @@ public void testIsOperator() throws IOException {
     }
 
     @Test
-    public void testUnlikeOperator() throws IOException {
+    public void testUnlikeOperator() {
         // example from marathon documentation:
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'UNLIKE', 'rack-[7-9]']]")).toString();
         assertEquals("NotRule{rule=AttributeRule{matcher=RegexMatcher{pattern='rack-id:rack-[7-9]'}}}", constraintStr);
@@ -152,7 +151,7 @@ public void testUnlikeOperator() throws IOException {
     }
 
     @Test
-    public void testMaxPerOperator() throws IOException {
+    public void testMaxPerOperator() {
         // example from marathon documentation:
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'MAX_PER', '2']]")).toString();
         assertEquals("AndRule{rules=[AttributeRule{matcher=RegexMatcher{pattern='rack-id:.*'}}, " +
@@ -167,7 +166,7 @@ public void testMaxPerOperator() throws IOException {
     }
 
     @Test
-    public void testManyOperators() throws IOException {
+    public void testManyOperators() {
         String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape(
                 "[['hostname', 'UNIQUE'], "
                         + "['rack-id', 'CLUSTER', 'rack-1'], "
@@ -194,79 +193,90 @@ public void testManyOperators() throws IOException {
     }
 
     @Test
-    public void testEscapedCommaRegex() throws IOException {
+    public void testEscapedCommaRegex() {
         assertEquals("AttributeRule{matcher=RegexMatcher{pattern='rack-id:rack-{1,3}'}}",
                 MarathonConstraintParser.parse(POD_NAME, "rack-id:LIKE:rack-{1\\,3}").toString());
     }
 
     @Test
-    public void testEscapedColonRegex() throws IOException {
+    public void testEscapedColonRegex() {
         assertEquals("AttributeRule{matcher=RegexMatcher{pattern='rack-id:foo:bar:baz'}}",
                 MarathonConstraintParser.parse(POD_NAME, "rack-id:LIKE:foo\\:bar\\:baz").toString());
     }
 
     @Test
-    public void testEmptyConstraint() throws IOException {
+    public void testEmptyConstraint() {
         assertEquals("PassthroughRule{}", MarathonConstraintParser.parse(POD_NAME, "").toString());
     }
 
     @Test
-    public void testEmptyArrayConstraint() throws IOException {
+    public void testEmptyArrayConstraint() {
         assertEquals("PassthroughRule{}", MarathonConstraintParser.parse(POD_NAME, "[]").toString());
     }
 
     @Test
-    public void testOverEscapedConstraintIsInvalid() throws IOException {
+    public void testOverEscapedConstraintIsInvalid() {
         assertTrue("too many \\'s", isInvalidConstraints("[[\\\"hostname\\\",\\\"MAX_PER\\\",\\\"1\\\"]]"));
     }
 
     @Test
-    public void testBadListConstraint() throws IOException {
+    public void testBadListConstraint() {
         assertTrue("missing ']]'", isInvalidConstraints(unescape("[['rack-id', 'MAX_PER', '2'")));
     }
 
     @Test
-    public void testBadFlatConstraint() throws IOException {
+    public void testBadFlatConstraint() {
         assertTrue("Missing last element", isInvalidConstraints("rack-id:MAX_PER:,"));
     }
 
     @Test
-    public void testBadParamGroupBy() throws IOException {
+    public void testBadParamGroupBy() {
         assertTrue("Expected integer", isInvalidConstraints("rack-id:GROUP_BY:foo"));
     }
 
     @Test
-    public void testBadParamMaxPer() throws IOException {
+    public void testBadParamMaxPer() {
         assertTrue("Expected integer", isInvalidConstraints("rack-id:MAX_PER:foo"));
     }
 
     @Test
-    public void testMissingParamCluster() throws IOException {
+    public void testExtraTokens() {
+        for (String constraint: Arrays.asList(
+                unescape("['hostname','UNIQUE'],[['hostname','LIKE','10.0.3.6']"),
+                unescape("['hostname','UNIQUE'],["),
+                unescape("[['hostname','UNIQUE'],['hostname','LIKE','10.0.3.6']"),
+                unescape("[['hostname','UNIQUE']],"))) {
+            assertTrue("Trailing tokens", isInvalidConstraints(constraint));
+        }
+    }
+
+    @Test
+    public void testMissingParamCluster() {
         assertTrue("Expected parameter", isInvalidConstraints("rack-id:CLUSTER"));
     }
 
     @Test
-    public void testMissingParamLike() throws IOException {
+    public void testMissingParamLike() {
         assertTrue("Expected parameter", isInvalidConstraints("rack-id:LIKE"));
     }
 
     @Test
-    public void testMissingParamUnlike() throws IOException {
+    public void testMissingParamUnlike() {
         assertTrue("Expected parameter", isInvalidConstraints("rack-id:UNLIKE"));
     }
 
     @Test
-    public void testMissingParamMaxPer() throws IOException {
+    public void testMissingParamMaxPer() {
         assertTrue("Expected parameter", isInvalidConstraints("rack-id:MAX_PER"));
     }
 
     @Test
-    public void testUnknownCommand() throws IOException {
+    public void testUnknownCommand() {
         assertTrue(isInvalidConstraints("rack-id:FOO:foo"));
     }
 
     @Test
-    public void testTooManyElemenents() throws IOException {
+    public void testTooManyElemenents() {
         assertTrue(isInvalidConstraints("rack-id:LIKE:foo:bar"));
     }
 
@@ -310,7 +320,7 @@ private static String unescape(String s) {
         return s.replace('\'', '"');
     }
 
-    private boolean isInvalidConstraints(String constraints) throws IOException {
+    private boolean isInvalidConstraints(String constraints) {
         return MarathonConstraintParser.parse(POD_NAME, constraints) instanceof InvalidPlacementRule;
     }
 }
diff --git a/sdk/scheduler/src/test/resources/invalid-image-null.yml b/sdk/scheduler/src/test/resources/invalid-image-null.yml
@@ -2,7 +2,7 @@ name: "invalid-image-null-test"
 pods:
   server:
     count: 1
-    image:
+    image: ""
     tasks:
       server:
         goal: RUNNING