diff options
author | HÃ¥kon Hallingstad <hakon@verizonmedia.com> | 2020-04-15 17:03:26 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-04-15 17:03:26 +0200 |
commit | 1eecdac6160ea9f7ed6de5e3fc478bc211561dc2 (patch) | |
tree | 5e9fc0a7b6735521c3d47d4ce9b4ee1a113b23e7 /controller-api | |
parent | 7c1004ffb82e0512278826d24edf9dbdc91186e1 (diff) | |
parent | 4422d42987bb9aefa92abd4cb4135798479edb6a (diff) |
Merge pull request #12882 from vespa-engine/hakonhall/validate-deserialization-of-flag-data
Validate deserialization of flag data
Diffstat (limited to 'controller-api')
5 files changed, 94 insertions, 1 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..6604da144b9 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,7 +1,11 @@ // 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.SystemName; +import com.yahoo.text.JSON; import com.yahoo.vespa.flags.FlagId; import com.yahoo.vespa.flags.json.FlagData; @@ -38,6 +42,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) { @@ -151,12 +157,34 @@ public class SystemFlagsDataArchive { 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(); + String normalizedRawData = removeCommentsFromJson(rawData); + if (!JSON.equals(serializedData, normalizedRawData)) { + throw new IllegalArgumentException(filePath + " contains unknown non-comment fields: " + + "was deserialized to " + serializedData); } } builder.addFile(filename, flagData); } + static String removeCommentsFromJson(String json) { + JsonNode jsonNode = uncheck(() -> mapper.readTree(json)); + removeComments(jsonNode); + return jsonNode.toString(); + } + + private static void removeComments(JsonNode node) { + if (node instanceof ObjectNode) { + ObjectNode objectNode = (ObjectNode) node; + objectNode.remove("comment"); + } + + node.forEach(SystemFlagsDataArchive::removeComments); + } + private static String toFilePath(FlagId flagId, String filename) { return "flags/" + flagId.toString() + "/" + filename; } 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..5ff1cbf7530 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,7 @@ import java.util.Set; import static java.util.stream.Collectors.toList; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.assertTrue; /** * @author bjorncs @@ -104,6 +106,45 @@ 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: was deserialized to {\"id\":\"my-test-flag\",\"rules\":[{\"value\":\"default\"}]}"); + 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.removeCommentsFromJson("{\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" + + "}"))); + } + 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" } ] |