Merge pull request #12882 from vespa-engine/hakonhall/validate-deserialization-of-flag-data

Validate deserialization of flag data

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"
     }
   ]