diff options
Diffstat (limited to 'controller-api')
5 files changed, 220 insertions, 2 deletions
diff --git a/controller-api/pom.xml b/controller-api/pom.xml index 5c744db2d6d..5d0bbff1c62 100644 --- a/controller-api/pom.xml +++ b/controller-api/pom.xml @@ -53,6 +53,13 @@ <version>${project.version}</version> </dependency> + <dependency> + <groupId>com.yahoo.vespa</groupId> + <artifactId>vespajlib</artifactId> + <scope>provided</scope> + <version>${project.version}</version> + </dependency> + <!-- compile --> <dependency> 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 0cb4baab790..1e42efdd256 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 @@ -1,8 +1,16 @@ // Copyright 2019 Oath Inc. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. 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; @@ -18,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; @@ -38,6 +49,8 @@ import static com.yahoo.yolean.Exceptions.uncheck; */ public class SystemFlagsDataArchive { + private static final ObjectMapper mapper = new ObjectMapper(); + private final Map<FlagId, Map<String, FlagData>> files; private SystemFlagsDataArchive(Map<FlagId, Map<String, FlagData>> files) { @@ -147,16 +160,65 @@ 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'", - flagData.id(), directoryDeducedFlagId.toString())); + flagData.id(), directoryDeducedFlagId.toString())); + } + + String serializedData = flagData.serializeToJson(); + if (!JSON.equals(serializedData, normalizedRawData)) { + throw new IllegalArgumentException(filePath + " contains unknown non-comment fields: " + + "after removing any comment fields the JSON is:\n " + + normalizedRawData + + "\nbut deserializing this ended up with a JSON that are missing some of the fields:\n " + + serializedData + + "\nSee https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax"); } } builder.addFile(filename, flagData); } + static String normalizeJson(String json) { + JsonNode root = uncheck(() -> mapper.readTree(json)); + removeCommentsRecursively(root); + verifyValues(root); + return root.toString(); + } + + 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::removeCommentsRecursively); + } + private static String toFilePath(FlagId flagId, String filename) { return "flags/" + flagId.toString() + "/" + filename; } @@ -178,4 +240,40 @@ 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))); + } + } + + /** 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 6e78b8da91c..35f6794e28d 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 @@ -6,6 +6,7 @@ import com.yahoo.config.provision.Environment; import com.yahoo.config.provision.RegionName; import com.yahoo.config.provision.SystemName; import com.yahoo.config.provision.zone.ZoneId; +import com.yahoo.text.JSON; import com.yahoo.vespa.athenz.api.AthenzService; import com.yahoo.vespa.flags.FetchVector; import com.yahoo.vespa.flags.FlagId; @@ -32,6 +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 @@ -104,6 +108,98 @@ public class SystemFlagsDataArchiveTest { archive.validateAllFilesAreForTargets(SystemName.main, Set.of(mainControllerTarget, prodUsWestCfgTarget)); } + @Test + public void throws_on_unknown_field() { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage( + "flags/my-test-flag/main.prod.us-west-1.json contains unknown non-comment fields: after removing any comment fields the JSON is:\n" + + " {\"id\":\"my-test-flag\",\"rules\":[{\"condition\":[{\"type\":\"whitelist\",\"dimension\":\"hostname\",\"values\":[\"foo.com\"]}],\"value\":\"default\"}]}\n" + + "but deserializing this ended up with a JSON that are missing some of the fields:\n" + + " {\"id\":\"my-test-flag\",\"rules\":[{\"value\":\"default\"}]}\n" + + "See https://git.ouroath.com/vespa/hosted-feature-flags for more info on the JSON syntax"); + SystemFlagsDataArchive.fromDirectory(Paths.get("src/test/resources/system-flags-with-unknown-field-name/")); + } + + @Test + public void remove_comments() { + assertTrue(JSON.equals("{\n" + + " \"a\": {\n" + + " \"b\": 1\n" + + " },\n" + + " \"list\": [\n" + + " {\n" + + " \"c\": 2\n" + + " },\n" + + " {\n" + + " }\n" + + " ]\n" + + "}", + SystemFlagsDataArchive.normalizeJson("{\n" + + " \"comment\": \"comment a\",\n" + + " \"a\": {\n" + + " \"comment\": \"comment b\",\n" + + " \"b\": 1\n" + + " },\n" + + " \"list\": [\n" + + " {\n" + + " \"comment\": \"comment c\",\n" + + " \"c\": 2\n" + + " },\n" + + " {\n" + + " \"comment\": \"comment d\"\n" + + " }\n" + + " ]\n" + + "}"))); + } + + @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"); diff --git a/controller-api/src/test/resources/system-flags-with-unknown-field-name/flags/my-test-flag/main.prod.us-west-1.json b/controller-api/src/test/resources/system-flags-with-unknown-field-name/flags/my-test-flag/main.prod.us-west-1.json new file mode 100644 index 00000000000..c41083fc7ab --- /dev/null +++ b/controller-api/src/test/resources/system-flags-with-unknown-field-name/flags/my-test-flag/main.prod.us-west-1.json @@ -0,0 +1,15 @@ +{ + "id" : "my-test-flag", + "rules" : [ + { + "condition": [ + { + "type": "whitelist", + "dimension": "hostname", + "values": ["foo.com"] + } + ], + "value" : "default" + } + ] +} diff --git a/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.json b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.json index cef75be02b7..29160dc0081 100644 --- a/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.json +++ b/controller-api/src/test/resources/system-flags/flags/flag-with-empty-data/main.json @@ -1,7 +1,9 @@ { + "comment": "comment a", "id" : "flag-with-empty-data", "rules" : [ { + "comment": "comment b", "value" : "main" } ] |