diff options
author | Håkon Hallingstad <hakon@verizonmedia.com> | 2020-04-17 09:35:30 +0200 |
---|---|---|
committer | Håkon Hallingstad <hakon@verizonmedia.com> | 2020-04-17 09:35:30 +0200 |
commit | fd3bb4882effd8687c524eb8b7b9853fda7baa4a (patch) | |
tree | dc17014b722183c395ec235de473c6cb9c2c2db1 /controller-api | |
parent | 8623bb270146aacc628912a71b4cbdd7fc814951 (diff) |
Validate application ID and node type in flag conditions
Diffstat (limited to 'controller-api')
2 files changed, 129 insertions, 9 deletions
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java index 6604da144b9..d7d3f577990 100644 --- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java +++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchive.java @@ -4,9 +4,13 @@ package com.yahoo.vespa.hosted.controller.api.systemflags.v1; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.node.ObjectNode; +import com.yahoo.config.provision.ApplicationId; +import com.yahoo.config.provision.NodeType; import com.yahoo.config.provision.SystemName; import com.yahoo.text.JSON; +import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.FlagId; +import com.yahoo.vespa.flags.json.DimensionHelper; import com.yahoo.vespa.flags.json.FlagData; import java.io.BufferedInputStream; @@ -22,8 +26,11 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.TreeMap; +import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.zip.ZipEntry; @@ -153,7 +160,8 @@ public class SystemFlagsDataArchive { if (rawData.isBlank()) { flagData = new FlagData(directoryDeducedFlagId); } else { - flagData = FlagData.deserialize(rawData); + String normalizedRawData = normalizeJson(rawData); + flagData = FlagData.deserialize(normalizedRawData); if (!directoryDeducedFlagId.equals(flagData.id())) { throw new IllegalArgumentException( String.format("Flag data file with flag id '%s' in directory for '%s'", @@ -161,7 +169,6 @@ public class SystemFlagsDataArchive { } String serializedData = flagData.serializeToJson(); - String normalizedRawData = removeCommentsFromJson(rawData); if (!JSON.equals(serializedData, normalizedRawData)) { throw new IllegalArgumentException(filePath + " contains unknown non-comment fields: " + "was deserialized to " + serializedData); @@ -170,19 +177,42 @@ public class SystemFlagsDataArchive { builder.addFile(filename, flagData); } - static String removeCommentsFromJson(String json) { - JsonNode jsonNode = uncheck(() -> mapper.readTree(json)); - removeComments(jsonNode); - return jsonNode.toString(); + static String normalizeJson(String json) { + JsonNode root = uncheck(() -> mapper.readTree(json)); + removeCommentsRecursively(root); + verifyValues(root); + return root.toString(); } - private static void removeComments(JsonNode node) { + private static void verifyValues(JsonNode root) { + var cursor = new JsonAccessor(root); + cursor.get("rules").forEachArrayElement(rule -> rule.get("conditions").forEachArrayElement(condition -> { + var dimension = condition.get("dimension"); + if (dimension.isEqualTo(DimensionHelper.toWire(FetchVector.Dimension.APPLICATION_ID))) { + condition.get("values").forEachArrayElement(conditionValue -> { + String applicationIdString = conditionValue.asString() + .orElseThrow(() -> new IllegalArgumentException("Non-string application ID: " + conditionValue)); + // Throws exception if not recognized + ApplicationId.fromSerializedForm(applicationIdString); + }); + } else if (dimension.isEqualTo(DimensionHelper.toWire(FetchVector.Dimension.NODE_TYPE))) { + condition.get("values").forEachArrayElement(conditionValue -> { + String nodeTypeString = conditionValue.asString() + .orElseThrow(() -> new IllegalArgumentException("Non-string node type: " + conditionValue)); + // Throws exception if not recognized + NodeType.valueOf(nodeTypeString); + }); + } + })); + } + + private static void removeCommentsRecursively(JsonNode node) { if (node instanceof ObjectNode) { ObjectNode objectNode = (ObjectNode) node; objectNode.remove("comment"); } - node.forEach(SystemFlagsDataArchive::removeComments); + node.forEach(SystemFlagsDataArchive::removeCommentsRecursively); } private static String toFilePath(FlagId flagId, String filename) { @@ -206,4 +236,44 @@ public class SystemFlagsDataArchive { } } + + private static class JsonAccessor { + private final JsonNode jsonNode; + + public JsonAccessor(JsonNode jsonNode) { + this.jsonNode = jsonNode; + } + + public JsonAccessor get(String fieldName) { + if (jsonNode == null) { + return this; + } else { + return new JsonAccessor(jsonNode.get(fieldName)); + } + } + + public Optional<String> asString() { + return jsonNode != null && jsonNode.isTextual() ? Optional.of(jsonNode.textValue()) : Optional.empty(); + } + + public void forEachArrayElement(Consumer<JsonAccessor> consumer) { + if (jsonNode != null && jsonNode.isArray()) { + jsonNode.forEach(jsonNodeElement -> consumer.accept(new JsonAccessor(jsonNodeElement))); + } + } + + public void removeCommentRecursively() { + + } + + /** Returns true if this (JsonNode) is a string and equal to value. */ + public boolean isEqualTo(String value) { + return jsonNode != null && jsonNode.isTextual() && Objects.equals(jsonNode.textValue(), value); + } + + @Override + public String toString() { + return jsonNode == null ? "undefined" : jsonNode.toString(); + } + } } diff --git a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java index 5ff1cbf7530..5f803efb9e9 100644 --- a/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java +++ b/controller-api/src/test/java/com/yahoo/vespa/hosted/controller/api/systemflags/v1/SystemFlagsDataArchiveTest.java @@ -33,7 +33,9 @@ import java.util.Set; import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author bjorncs @@ -127,7 +129,7 @@ public class SystemFlagsDataArchiveTest { " }\n" + " ]\n" + "}", - SystemFlagsDataArchive.removeCommentsFromJson("{\n" + + SystemFlagsDataArchive.normalizeJson("{\n" + " \"comment\": \"comment a\",\n" + " \"a\": {\n" + " \"comment\": \"comment b\",\n" + @@ -145,6 +147,54 @@ public class SystemFlagsDataArchiveTest { "}"))); } + @Test + public void normalize_json_fail_on_invalid_application() { + try { + SystemFlagsDataArchive.normalizeJson("{\n" + + " \"id\": \"foo\",\n" + + " \"rules\": [\n" + + " {\n" + + " \"conditions\": [\n" + + " {\n" + + " \"type\": \"whitelist\",\n" + + " \"dimension\": \"application\",\n" + + " \"values\": [ \"a.b.c\" ]\n" + + " }\n" + + " ],\n" + + " \"value\": true\n" + + " }\n" + + " ]\n" + + "}\n"); + fail(); + } catch (IllegalArgumentException e) { + assertEquals("Application ids must be on the form tenant:application:instance, but was a.b.c", e.getMessage()); + } + } + + @Test + public void normalize_json_fail_on_invalid_node_type() { + try { + SystemFlagsDataArchive.normalizeJson("{\n" + + " \"id\": \"foo\",\n" + + " \"rules\": [\n" + + " {\n" + + " \"conditions\": [\n" + + " {\n" + + " \"type\": \"whitelist\",\n" + + " \"dimension\": \"node-type\",\n" + + " \"values\": [ \"footype\" ]\n" + + " }\n" + + " ],\n" + + " \"value\": true\n" + + " }\n" + + " ]\n" + + "}\n"); + fail(); + } catch (IllegalArgumentException e) { + assertEquals("No enum constant com.yahoo.config.provision.NodeType.footype", e.getMessage()); + } + } + private static void assertArchiveReturnsCorrectTestFlagDataForTarget(SystemFlagsDataArchive archive) { assertFlagDataHasValue(archive, MY_TEST_FLAG, mainControllerTarget, "main.controller"); assertFlagDataHasValue(archive, MY_TEST_FLAG, prodUsWestCfgTarget, "main.prod.us-west-1"); |