Skip to content
This repository was archived by the owner on Oct 23, 2024. It is now read-only.

Commit 5edb2dd

Browse files
takiralatakirala
committed
[COPS-5211] Fix marathon constraint parser bug (#3160)
* Upgrade jackson to 2.9 to use DeserializationFeature.FAIL_ON_TRAILING_TOKENS * Add/update unit tests
1 parent 64dbdea commit 5edb2dd

9 files changed

Lines changed: 52 additions & 41 deletions

File tree

‎sdk/scheduler/build.gradle‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ configurations {
111111

112112
ext {
113113
// Only include version numbers here if it'd be redundant to repeat them below:
114-
jacksonVer = "2.6.7"
114+
jacksonVer = "2.9.9"
115115
curatorVer = "4.0.1"
116116
httpClientVer = "4.5.2"
117117
jerseyVer = "2.23"
@@ -125,7 +125,7 @@ dependencies {
125125
compile "com.fasterxml.jackson.datatype:jackson-datatype-json-org:${jacksonVer}"
126126
compile "com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:${jacksonVer}"
127127
compile "com.fasterxml.jackson.core:jackson-databind:${jacksonVer}"
128-
compile 'com.hubspot.jackson:jackson-datatype-protobuf:0.9.9-preJackson2.7-proto3'
128+
compile 'com.hubspot.jackson:jackson-datatype-protobuf:0.9.11-jackson2.9'
129129
compile 'com.googlecode.protobuf-java-format:protobuf-java-format:1.4'
130130
compile 'com.github.spullara.mustache.java:compiler:0.9.2'
131131
compile 'commons-codec:commons-codec:1.11'

‎sdk/scheduler/src/main/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParser.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import com.mesosphere.sdk.offer.LoggingUtils;
44

55
import com.fasterxml.jackson.core.type.TypeReference;
6+
import com.fasterxml.jackson.databind.DeserializationFeature;
67
import com.fasterxml.jackson.databind.ObjectMapper;
78
import com.google.common.annotations.VisibleForTesting;
89
import jersey.repackaged.com.google.common.collect.Lists;
@@ -80,7 +81,7 @@ public static PlacementRule parse(String podName, String marathonConstraints) {
8081
return new AndRule(rowRules);
8182

8283
} catch (IOException e) {
83-
LOGGER.error("Failed to parse marathon constraints", podName, marathonConstraints);
84+
LOGGER.error("Failed to parse marathon constraints [{}] for {}", marathonConstraints, podName);
8485
return new InvalidPlacementRule(marathonConstraints, e.getMessage());
8586
}
8687
}
@@ -126,6 +127,8 @@ private static PlacementRule parseRow(
126127
@VisibleForTesting
127128
static List<List<String>> splitConstraints(String marathonConstraints) {
128129
ObjectMapper mapper = new ObjectMapper();
130+
// Always parse the string in full. Leftover trailing tokens result in incomplete (and may be invalid) rules.
131+
mapper.enable(DeserializationFeature.FAIL_ON_TRAILING_TOKENS);
129132
// The marathon doc uses a format like: '[["a", "b", "c"], ["d", "e"]]'
130133
// Meanwhile the marathon web interface uses a format like: 'a:b:c,d:e'
131134
try {

‎sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/DefaultPodSpec.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -575,13 +575,10 @@ public Builder seccompProfileName(String seccompProfileName) {
575575
}
576576

577577
/**
578-
* Returns a {@code DefaultPodSpec} built from the parameters previously set.
579-
*
580578
* @return a {@code DefaultPodSpec} built with parameters of this {@code DefaultPodSpec.Builder}
581579
*/
582580
public DefaultPodSpec build() {
583-
DefaultPodSpec defaultPodSpec = new DefaultPodSpec(this);
584-
return defaultPodSpec;
581+
return new DefaultPodSpec(this);
585582
}
586583
}
587584
}

‎sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawNetwork.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ private RawNetwork(
3131
* Included so that we support empty network specifications (e.g. a network of {@code networks: dcos:}).
3232
*/
3333
@JsonCreator
34-
private RawNetwork(String ignored) {
34+
private RawNetwork() {
3535
this(Collections.emptyList(), Collections.emptyList(), "");
3636
}
3737

‎sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawPod.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
66
import com.fasterxml.jackson.annotation.JsonProperty;
7+
import com.fasterxml.jackson.annotation.JsonSetter;
8+
import com.fasterxml.jackson.annotation.Nulls;
79
import org.apache.commons.collections.CollectionUtils;
810

911
import java.util.Collection;
@@ -22,7 +24,8 @@ public final class RawPod {
2224

2325
private final String image;
2426

25-
private final WriteOnceLinkedHashMap<String, RawNetwork> networks;
27+
@JsonSetter(contentNulls = Nulls.AS_EMPTY)
28+
private WriteOnceLinkedHashMap<String, RawNetwork> networks;
2629

2730
private final WriteOnceLinkedHashMap<String, RawRLimit> rlimits;
2831

@@ -56,7 +59,6 @@ private RawPod(
5659
@JsonProperty("placement") String placement,
5760
@JsonProperty("count") Integer count,
5861
@JsonProperty("image") String image,
59-
@JsonProperty("networks") WriteOnceLinkedHashMap<String, RawNetwork> networks,
6062
@JsonProperty("rlimits") WriteOnceLinkedHashMap<String, RawRLimit> rlimits,
6163
@JsonProperty("uris") Collection<String> uris,
6264
@JsonProperty("tasks") WriteOnceLinkedHashMap<String, RawTask> tasks,
@@ -73,7 +75,6 @@ private RawPod(
7375
this.placement = placement;
7476
this.count = count;
7577
this.image = image;
76-
this.networks = networks == null ? new WriteOnceLinkedHashMap<>() : networks;
7778
this.rlimits = rlimits == null ? new WriteOnceLinkedHashMap<>() : rlimits;
7879
this.uris = uris;
7980
this.tasks = tasks;

‎sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/RawServiceSpec.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ public final class RawServiceSpec {
2929
static {
3030
// If the user provides duplicate fields (e.g. 'count' twice), throw an error instead of silently dropping data:
3131
YAML_MAPPER.enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION);
32+
YAML_MAPPER.enable(JsonParser.Feature.ALLOW_YAML_COMMENTS);
3233
}
3334

3435
private final String name;

‎sdk/scheduler/src/main/java/com/mesosphere/sdk/specification/yaml/YAMLToInternalMappers.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -300,9 +300,8 @@ private static PodSpec convertPod(
300300
WriteOnceLinkedHashMap<String, RawNetwork> rawNetworks = rawPod.getNetworks();
301301
final Collection<NetworkSpec> networks = new ArrayList<>();
302302
if (MapUtils.isNotEmpty(rawNetworks)) {
303-
networks.addAll(rawNetworks.entrySet().stream()
304-
.map(rawNetworkEntry -> {
305-
String networkName = rawNetworkEntry.getKey();
303+
networks.addAll(rawNetworks.keySet().stream()
304+
.map(networkName -> {
306305
DcosConstants.warnIfUnsupportedNetwork(networkName);
307306
networkNames.add(networkName);
308307
RawNetwork rawNetwork = rawNetworks.get(networkName);

‎sdk/scheduler/src/test/java/com/mesosphere/sdk/offer/evaluate/placement/MarathonConstraintParserTest.java‎

Lines changed: 36 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import org.junit.Test;
44

5-
import java.io.IOException;
65
import java.util.Arrays;
76
import java.util.Collections;
87

@@ -16,7 +15,7 @@ public class MarathonConstraintParserTest {
1615
static final String POD_NAME = "hello";
1716

1817
@Test
19-
public void testSplitConstraints() throws IOException {
18+
public void testSplitConstraints() {
2019
assertEquals(Arrays.asList(Arrays.asList("")),
2120
MarathonConstraintParser.splitConstraints(""));
2221
assertEquals(Arrays.asList(Arrays.asList("a")),
@@ -41,7 +40,7 @@ public void testSplitConstraints() throws IOException {
4140
}
4241

4342
@Test
44-
public void testUniqueOperator() throws IOException {
43+
public void testUniqueOperator() {
4544
// example from marathon documentation:
4645
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['hostname', 'UNIQUE']]")).toString();
4746
assertEquals("MaxPerHostnameRule{max=1, task-filter=RegexMatcher{pattern='hello-.*'}}", constraintStr);
@@ -56,7 +55,7 @@ public void testUniqueOperator() throws IOException {
5655
}
5756

5857
@Test
59-
public void testClusterOperator() throws IOException {
58+
public void testClusterOperator() {
6059
// example from marathon documentation:
6160
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'CLUSTER', 'rack-1']]")).toString();
6261
assertEquals("AttributeRule{matcher=ExactMatcher{str='rack-id:rack-1'}}", constraintStr);
@@ -71,7 +70,7 @@ public void testClusterOperator() throws IOException {
7170
}
7271

7372
@Test
74-
public void testGroupByOperator() throws IOException {
73+
public void testGroupByOperator() {
7574
// example from marathon documentation:
7675
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'GROUP_BY']]")).toString();
7776
assertEquals("RoundRobinByAttributeRule{attribute=rack-id, attribute-count=Optional.empty, task-filter=RegexMatcher{pattern='hello-.*'}}", constraintStr);
@@ -101,7 +100,7 @@ public void testGroupByOperator() throws IOException {
101100
}
102101

103102
@Test
104-
public void testLikeOperator() throws IOException {
103+
public void testLikeOperator() {
105104
// example from marathon documentation:
106105
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'LIKE', 'rack-[1-3]']]")).toString();
107106
assertEquals("AttributeRule{matcher=RegexMatcher{pattern='rack-id:rack-[1-3]'}}", constraintStr);
@@ -115,7 +114,7 @@ public void testLikeOperator() throws IOException {
115114
}
116115

117116
@Test
118-
public void testIsOperator() throws IOException {
117+
public void testIsOperator() {
119118
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['foo', 'IS', 'bar']]")).toString();
120119
assertEquals("AttributeRule{matcher=ExactMatcher{str='foo:bar'}}", constraintStr);
121120
assertEquals(constraintStr, MarathonConstraintParser.parse(POD_NAME, unescape("['foo', 'IS', 'bar']")).toString());
@@ -138,7 +137,7 @@ public void testIsOperator() throws IOException {
138137
}
139138

140139
@Test
141-
public void testUnlikeOperator() throws IOException {
140+
public void testUnlikeOperator() {
142141
// example from marathon documentation:
143142
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'UNLIKE', 'rack-[7-9]']]")).toString();
144143
assertEquals("NotRule{rule=AttributeRule{matcher=RegexMatcher{pattern='rack-id:rack-[7-9]'}}}", constraintStr);
@@ -152,7 +151,7 @@ public void testUnlikeOperator() throws IOException {
152151
}
153152

154153
@Test
155-
public void testMaxPerOperator() throws IOException {
154+
public void testMaxPerOperator() {
156155
// example from marathon documentation:
157156
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape("[['rack-id', 'MAX_PER', '2']]")).toString();
158157
assertEquals("AndRule{rules=[AttributeRule{matcher=RegexMatcher{pattern='rack-id:.*'}}, " +
@@ -167,7 +166,7 @@ public void testMaxPerOperator() throws IOException {
167166
}
168167

169168
@Test
170-
public void testManyOperators() throws IOException {
169+
public void testManyOperators() {
171170
String constraintStr = MarathonConstraintParser.parse(POD_NAME, unescape(
172171
"[['hostname', 'UNIQUE'], "
173172
+ "['rack-id', 'CLUSTER', 'rack-1'], "
@@ -194,79 +193,90 @@ public void testManyOperators() throws IOException {
194193
}
195194

196195
@Test
197-
public void testEscapedCommaRegex() throws IOException {
196+
public void testEscapedCommaRegex() {
198197
assertEquals("AttributeRule{matcher=RegexMatcher{pattern='rack-id:rack-{1,3}'}}",
199198
MarathonConstraintParser.parse(POD_NAME, "rack-id:LIKE:rack-{1\\,3}").toString());
200199
}
201200

202201
@Test
203-
public void testEscapedColonRegex() throws IOException {
202+
public void testEscapedColonRegex() {
204203
assertEquals("AttributeRule{matcher=RegexMatcher{pattern='rack-id:foo:bar:baz'}}",
205204
MarathonConstraintParser.parse(POD_NAME, "rack-id:LIKE:foo\\:bar\\:baz").toString());
206205
}
207206

208207
@Test
209-
public void testEmptyConstraint() throws IOException {
208+
public void testEmptyConstraint() {
210209
assertEquals("PassthroughRule{}", MarathonConstraintParser.parse(POD_NAME, "").toString());
211210
}
212211

213212
@Test
214-
public void testEmptyArrayConstraint() throws IOException {
213+
public void testEmptyArrayConstraint() {
215214
assertEquals("PassthroughRule{}", MarathonConstraintParser.parse(POD_NAME, "[]").toString());
216215
}
217216

218217
@Test
219-
public void testOverEscapedConstraintIsInvalid() throws IOException {
218+
public void testOverEscapedConstraintIsInvalid() {
220219
assertTrue("too many \\'s", isInvalidConstraints("[[\\\"hostname\\\",\\\"MAX_PER\\\",\\\"1\\\"]]"));
221220
}
222221

223222
@Test
224-
public void testBadListConstraint() throws IOException {
223+
public void testBadListConstraint() {
225224
assertTrue("missing ']]'", isInvalidConstraints(unescape("[['rack-id', 'MAX_PER', '2'")));
226225
}
227226

228227
@Test
229-
public void testBadFlatConstraint() throws IOException {
228+
public void testBadFlatConstraint() {
230229
assertTrue("Missing last element", isInvalidConstraints("rack-id:MAX_PER:,"));
231230
}
232231

233232
@Test
234-
public void testBadParamGroupBy() throws IOException {
233+
public void testBadParamGroupBy() {
235234
assertTrue("Expected integer", isInvalidConstraints("rack-id:GROUP_BY:foo"));
236235
}
237236

238237
@Test
239-
public void testBadParamMaxPer() throws IOException {
238+
public void testBadParamMaxPer() {
240239
assertTrue("Expected integer", isInvalidConstraints("rack-id:MAX_PER:foo"));
241240
}
242241

243242
@Test
244-
public void testMissingParamCluster() throws IOException {
243+
public void testExtraTokens() {
244+
for (String constraint: Arrays.asList(
245+
unescape("['hostname','UNIQUE'],[['hostname','LIKE','10.0.3.6']"),
246+
unescape("['hostname','UNIQUE'],["),
247+
unescape("[['hostname','UNIQUE'],['hostname','LIKE','10.0.3.6']"),
248+
unescape("[['hostname','UNIQUE']],"))) {
249+
assertTrue("Trailing tokens", isInvalidConstraints(constraint));
250+
}
251+
}
252+
253+
@Test
254+
public void testMissingParamCluster() {
245255
assertTrue("Expected parameter", isInvalidConstraints("rack-id:CLUSTER"));
246256
}
247257

248258
@Test
249-
public void testMissingParamLike() throws IOException {
259+
public void testMissingParamLike() {
250260
assertTrue("Expected parameter", isInvalidConstraints("rack-id:LIKE"));
251261
}
252262

253263
@Test
254-
public void testMissingParamUnlike() throws IOException {
264+
public void testMissingParamUnlike() {
255265
assertTrue("Expected parameter", isInvalidConstraints("rack-id:UNLIKE"));
256266
}
257267

258268
@Test
259-
public void testMissingParamMaxPer() throws IOException {
269+
public void testMissingParamMaxPer() {
260270
assertTrue("Expected parameter", isInvalidConstraints("rack-id:MAX_PER"));
261271
}
262272

263273
@Test
264-
public void testUnknownCommand() throws IOException {
274+
public void testUnknownCommand() {
265275
assertTrue(isInvalidConstraints("rack-id:FOO:foo"));
266276
}
267277

268278
@Test
269-
public void testTooManyElemenents() throws IOException {
279+
public void testTooManyElemenents() {
270280
assertTrue(isInvalidConstraints("rack-id:LIKE:foo:bar"));
271281
}
272282

@@ -310,7 +320,7 @@ private static String unescape(String s) {
310320
return s.replace('\'', '"');
311321
}
312322

313-
private boolean isInvalidConstraints(String constraints) throws IOException {
323+
private boolean isInvalidConstraints(String constraints) {
314324
return MarathonConstraintParser.parse(POD_NAME, constraints) instanceof InvalidPlacementRule;
315325
}
316326
}

‎sdk/scheduler/src/test/resources/invalid-image-null.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ name: "invalid-image-null-test"
22
pods:
33
server:
44
count: 1
5-
image:
5+
image: ""
66
tasks:
77
server:
88
goal: RUNNING

0 commit comments

Comments
 (0)