From ed41752f2d58998b3bc31377ce0b4b19e1d384af Mon Sep 17 00:00:00 2001 From: Tarun Gupta Akirala Date: Tue, 20 Aug 2019 19:37:03 -0700 Subject: [PATCH 1/5] Upgrade jackson to 2.9 to use DeserializationFeature.FAIL_ON_TRAILING_TOKENS --- sdk/scheduler/build.gradle | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/scheduler/build.gradle b/sdk/scheduler/build.gradle index 3cb78438532..bc6d09b201c 100644 --- 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' From 2c68dd161b5c70c1e5a1522d87833d79c6f2d5a9 Mon Sep 17 00:00:00 2001 From: Tarun Gupta Akirala Date: Tue, 20 Aug 2019 19:51:16 -0700 Subject: [PATCH 2/5] Use new jackson 2.9 methods in source code --- .../offer/evaluate/placement/MarathonConstraintParser.java | 5 ++++- .../com/mesosphere/sdk/specification/DefaultPodSpec.java | 5 +---- .../com/mesosphere/sdk/specification/yaml/RawNetwork.java | 2 +- .../java/com/mesosphere/sdk/specification/yaml/RawPod.java | 7 ++++--- .../mesosphere/sdk/specification/yaml/RawServiceSpec.java | 2 ++ .../sdk/specification/yaml/YAMLToInternalMappers.java | 5 ++--- 6 files changed, 14 insertions(+), 12 deletions(-) diff --git a/sdk/scheduler/src/main/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParser.java b/sdk/scheduler/src/main/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParser.java index b79acbcd10a..90d9a7f2439 100644 --- a/sdk/scheduler/src/main/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParser.java +++ b/sdk/scheduler/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> 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 index 88cc9a99a4c..7c96bea1aab 100644 --- 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 index cb8b38a7180..1567aeadc42 100644 --- 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 index 4f7e376cf1c..745ee5dc45a 100644 --- 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 networks; + @JsonSetter(contentNulls = Nulls.AS_EMPTY) + private WriteOnceLinkedHashMap networks; private final WriteOnceLinkedHashMap rlimits; @@ -56,7 +59,6 @@ private RawPod( @JsonProperty("placement") String placement, @JsonProperty("count") Integer count, @JsonProperty("image") String image, - @JsonProperty("networks") WriteOnceLinkedHashMap networks, @JsonProperty("rlimits") WriteOnceLinkedHashMap rlimits, @JsonProperty("uris") Collection uris, @JsonProperty("tasks") WriteOnceLinkedHashMap 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 index 59550e19791..53499bafc5d 100644 --- 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 @@ -1,5 +1,6 @@ package com.mesosphere.sdk.specification.yaml; +import com.fasterxml.jackson.dataformat.yaml.YAMLMapper; import com.mesosphere.sdk.offer.LoggingUtils; import com.fasterxml.jackson.annotation.JsonCreator; @@ -29,6 +30,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 index 2f023a1fca2..3a811ea454b 100644 --- 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 rawNetworks = rawPod.getNetworks(); final Collection 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); From a064c123728006705ebd9a34d893eb81381cf5a9 Mon Sep 17 00:00:00 2001 From: Tarun Gupta Akirala Date: Tue, 20 Aug 2019 19:52:30 -0700 Subject: [PATCH 3/5] Add/update unit tests --- .../MarathonConstraintParserTest.java | 62 +++++++++++-------- .../specification/DefaultServiceSpecTest.java | 1 + 2 files changed, 38 insertions(+), 25 deletions(-) diff --git a/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java b/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java index 3fae8bf41e3..f76402d5b2f 100644 --- a/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java +++ b/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java @@ -5,6 +5,7 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collections; +import java.util.List; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; @@ -16,7 +17,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 +42,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 +57,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 +72,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 +102,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 +116,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 +139,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 +153,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 +168,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 +195,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 +322,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/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java b/sdk/scheduler/src/test/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java index 1a15a3eb90b..6f74697c5d3 100644 --- a/sdk/scheduler/src/test/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java +++ b/sdk/scheduler/src/test/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java @@ -5,6 +5,7 @@ import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.exc.InvalidNullException; import com.fasterxml.jackson.databind.module.SimpleModule; import com.google.common.collect.Iterables; import com.mesosphere.sdk.config.SerializationUtils; From 99ca100cb8900b15fdd29d095dcd3180149d47f3 Mon Sep 17 00:00:00 2001 From: Tarun Gupta Akirala Date: Tue, 20 Aug 2019 19:52:58 -0700 Subject: [PATCH 4/5] Add/update unit test resource files --- sdk/scheduler/src/test/resources/invalid-image-null.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/scheduler/src/test/resources/invalid-image-null.yml b/sdk/scheduler/src/test/resources/invalid-image-null.yml index 38d050f51f6..a2fc57c20c7 100644 --- 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 From bbffaccecb2166936cb980434f3335b5cd79eae4 Mon Sep 17 00:00:00 2001 From: Tarun Gupta Akirala Date: Tue, 20 Aug 2019 21:53:04 -0700 Subject: [PATCH 5/5] Fix linter errors --- .../com/mesosphere/sdk/specification/yaml/RawServiceSpec.java | 1 - .../offer/evaluate/placement/MarathonConstraintParserTest.java | 2 -- .../mesosphere/sdk/specification/DefaultServiceSpecTest.java | 1 - 3 files changed, 4 deletions(-) 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 index 53499bafc5d..28b65fd1ad0 100644 --- 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 @@ -1,6 +1,5 @@ package com.mesosphere.sdk.specification.yaml; -import com.fasterxml.jackson.dataformat.yaml.YAMLMapper; import com.mesosphere.sdk.offer.LoggingUtils; import com.fasterxml.jackson.annotation.JsonCreator; diff --git a/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java b/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java index f76402d5b2f..2141751ffa6 100644 --- a/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java +++ b/sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java @@ -2,10 +2,8 @@ import org.junit.Test; -import java.io.IOException; import java.util.Arrays; import java.util.Collections; -import java.util.List; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; diff --git a/sdk/scheduler/src/test/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java b/sdk/scheduler/src/test/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java index 6f74697c5d3..1a15a3eb90b 100644 --- a/sdk/scheduler/src/test/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java +++ b/sdk/scheduler/src/test/java/com/mesosphere/sdk/specification/DefaultServiceSpecTest.java @@ -5,7 +5,6 @@ import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.exc.InvalidNullException; import com.fasterxml.jackson.databind.module.SimpleModule; import com.google.common.collect.Iterables; import com.mesosphere.sdk.config.SerializationUtils;