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

Validate deserialization of flag data

13 files changed, 271 insertions, 9 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"
     }
   ]
diff --git a/vespajlib/abi-spec.json b/vespajlib/abi-spec.json
index 5cddc82d05a..d9467a41f78 100644
--- a/vespajlib/abi-spec.json
+++ b/vespajlib/abi-spec.json
@@ -2863,7 +2863,8 @@
     ],
     "methods": [
       "public static java.lang.String encode(java.util.Map)",
-      "public static java.lang.String escape(java.lang.String)"
+      "public static java.lang.String escape(java.lang.String)",
+      "public static boolean equals(java.lang.String, java.lang.String)"
     ],
     "fields": []
   },
diff --git a/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java b/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java
index f199fefd185..f677ae23a45 100644
--- a/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java
+++ b/vespajlib/src/main/java/com/yahoo/slime/JsonDecoder.java
@@ -3,10 +3,8 @@ package com.yahoo.slime;
 
 import com.yahoo.text.Text;
 import com.yahoo.text.Utf8;
-import org.w3c.dom.CharacterData;
 
 import java.io.ByteArrayOutputStream;
-import java.nio.charset.StandardCharsets;
 
 /**
  * A port of the C++ json decoder intended to be fast.
@@ -47,6 +45,17 @@ public class JsonDecoder {
         return slime;
     }
 
+    /** Decode bytes as a UTF-8 JSON into Slime, or throw {@link JsonParseException} on invalid JSON. */
+    public Slime decodeOrThrow(Slime slime, byte[] bytes) {
+        in = new BufferedInput(bytes);
+        next();
+        decodeValue(slimeInserter.adjust(slime));
+        if (in.failed()) {
+            throw new JsonParseException(in);
+        }
+        return slime;
+    }
+
     private void decodeValue(Inserter inserter) {
         skipWhiteSpace();
         switch (c) {
diff --git a/vespajlib/src/main/java/com/yahoo/slime/JsonParseException.java b/vespajlib/src/main/java/com/yahoo/slime/JsonParseException.java
new file mode 100644
index 00000000000..6c42f7d38c1
--- /dev/null
+++ b/vespajlib/src/main/java/com/yahoo/slime/JsonParseException.java
@@ -0,0 +1,23 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.slime;
+
+/**
+ * @author hakonhall
+ */
+public class JsonParseException extends RuntimeException {
+
+    private static final long serialVersionUID = 1586949558L;
+
+    private final BufferedInput input;
+
+    JsonParseException(BufferedInput input) {
+        super(input.getErrorMessage());
+        this.input = input;
+    }
+
+    public byte[] getOffendingBytes() {
+        // potentially expensive array copy
+        return input.getOffending();
+    }
+
+}
diff --git a/vespajlib/src/main/java/com/yahoo/slime/Slime.java b/vespajlib/src/main/java/com/yahoo/slime/Slime.java
index 8357e3035c0..83934e0c206 100644
--- a/vespajlib/src/main/java/com/yahoo/slime/Slime.java
+++ b/vespajlib/src/main/java/com/yahoo/slime/Slime.java
@@ -159,11 +159,10 @@ public final class Slime {
     }
 
     /**
-     * Tests whether this is equal to Inspector.
+     * Tests whether the two Inspectors are equal.
      *
-     * Since equality of two Inspectors is subtle, {@link Object#equals(Object)} is not used.
+     * <p>Since equality of two Inspectors is subtle, {@link Object#equals(Object)} is not used.</p>
      *
-     * @param that inspector.
      * @return true if they are equal.
      */
     public boolean equalTo(Slime that) {
diff --git a/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java b/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java
index 4d0b3a8ff2e..2ed7331a60c 100644
--- a/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java
+++ b/vespajlib/src/main/java/com/yahoo/slime/SlimeUtils.java
@@ -104,6 +104,17 @@ public class SlimeUtils {
         return jsonToSlime(json.getBytes(StandardCharsets.UTF_8));
     }
 
+    /** Throws {@link JsonParseException} on invalid JSON. */
+    public static Slime jsonToSlimeOrThrow(String json) {
+        return jsonToSlimeOrThrow(json.getBytes(StandardCharsets.UTF_8));
+    }
+
+    public static Slime jsonToSlimeOrThrow(byte[] json) {
+        Slime slime = new Slime();
+        new JsonDecoder().decodeOrThrow(slime, json);
+        return slime;
+    }
+
     public static Optional<String> optionalString(Inspector inspector) {
         return Optional.of(inspector.asString()).filter(s -> !s.isEmpty());
     }
diff --git a/vespajlib/src/main/java/com/yahoo/text/JSON.java b/vespajlib/src/main/java/com/yahoo/text/JSON.java
index cfff16c9aba..11e5be9328e 100644
--- a/vespajlib/src/main/java/com/yahoo/text/JSON.java
+++ b/vespajlib/src/main/java/com/yahoo/text/JSON.java
@@ -1,10 +1,13 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.text;
 
+import com.yahoo.slime.Slime;
+import com.yahoo.slime.SlimeUtils;
+
 import java.util.Map;
 
 /**
- * Static methods for working with the map textual format which is parsed by {@link MapParser}
+ * Static methods for working with JSON.
  *
  * @author bratseth
  */
@@ -56,4 +59,19 @@ public final class JSON {
         return b != null ? b.toString() : s;
     }
 
+    /**
+     * Test whether two JSON strings are equal, e.g. the order of fields in an object is irrelevant.
+     *
+     * <p>When comparing two numbers of the two JSON strings, the result is only guaranteed to be
+     * correct if (a) both are integers (without fraction and exponent) and each fits in a long, or
+     * (b) both are non-integers, are syntactically identical, and fits in a double.</p>
+     *
+     * @throws RuntimeException on invalid JSON
+     */
+    public static boolean equals(String left, String right) {
+        Slime leftSlime = SlimeUtils.jsonToSlimeOrThrow(left);
+        Slime rightSlime = SlimeUtils.jsonToSlimeOrThrow(right);
+        return leftSlime.equalTo(rightSlime);
+    }
+
 }
diff --git a/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java b/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java
index 8b13ee74aed..237b1575bfb 100644
--- a/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java
+++ b/vespajlib/src/test/java/com/yahoo/slime/SlimeUtilsTest.java
@@ -7,8 +7,10 @@ import org.junit.Test;
 import java.io.IOException;
 
 import static org.hamcrest.core.Is.is;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * @author Ulf Lilleengen
@@ -78,4 +80,21 @@ public class SlimeUtilsTest {
         assertTrue(slime.get().field("bar").valid());
     }
 
+    @Test
+    public void test_json_to_slime_or_throw() {
+        Slime slime = SlimeUtils.jsonToSlimeOrThrow("{\"foo\":\"foobie\",\"bar\":{}}");
+        assertThat(slime.get().field("foo").asString(), is("foobie"));
+        assertTrue(slime.get().field("bar").valid());
+    }
+
+    @Test
+    public void test_invalid_json() {
+        try {
+            SlimeUtils.jsonToSlimeOrThrow("foo");
+            fail();
+        } catch (RuntimeException e) {
+            assertEquals("Unexpected character 'o'", e.getMessage());
+        }
+    }
+
 }
diff --git a/vespajlib/src/test/java/com/yahoo/text/JSONTest.java b/vespajlib/src/test/java/com/yahoo/text/JSONTest.java
index 22174761571..fbd0f9d0403 100644
--- a/vespajlib/src/test/java/com/yahoo/text/JSONTest.java
+++ b/vespajlib/src/test/java/com/yahoo/text/JSONTest.java
@@ -2,11 +2,16 @@
 package com.yahoo.text;
 
 import org.junit.Test;
-import static org.junit.Assert.assertEquals;
 
 import java.util.LinkedHashMap;
 import java.util.Map;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
 /**
  * @author bratseth
  */
@@ -22,4 +27,88 @@ public class JSONTest {
         assertEquals("{\"a \\\"key\\\"\":3,\"key2\":\"value\",\"key3\":3.3}", JSON.encode(map));
     }
 
+    @Test
+    public void testEquals() {
+        assertTrue(JSON.equals("{}", "{}"));
+
+        // Whitespace is irrelevant
+        assertTrue(JSON.equals("{}", "\n{ }"));
+
+        // Order of fields in object is irrelevant
+        assertTrue(JSON.equals("{\"a\":0, \"c\":1}", "{\"c\":1, \"a\":0}"));
+
+        // Object equality is not using subset
+        assertFalse(JSON.equals("{\"a\":0}", "{\"a\":0, \"b\":0}"));
+        assertFalse(JSON.equals("{\"a\":0, \"b\":0}", "{\"a\":0}"));
+
+        // Order of elements of array is significant
+        assertFalse(JSON.equals("[\"a\",\"b\"]", "[\"b\",\"a\"]"));
+
+        // Verify null-valued fields are not ignored
+        assertFalse(JSON.equals("{\"a\":null}", "{}"));
+
+        // Current impl uses BigInteger if integer doesn't fit in a long.
+        assertEquals(9223372036854775807L, Long.MAX_VALUE);
+        assertTrue(JSON.equals("{\"a\": 9223372036854775807}", "{\"a\": 9223372036854775807}"));
+
+        // double 1.0 and int 1 are different
+        assertTrue(JSON.equals("{\"a\": 1}", "{\"a\": 1}"));
+        assertTrue(JSON.equals("{\"a\": 1.0}", "{\"a\": 1.0}"));
+        assertFalse(JSON.equals("{\"a\": 1.0}", "{\"a\": 1}"));
+
+        // Double-precision on numbers. Constant from Math.E.
+        assertTrue(JSON.equals("{\"e\": 2.71828182845904}", "{\"e\": 2.71828182845904}"));
+
+        // Double.MAX_VALUE is 1.7976931348623157e+308
+        assertTrue(JSON.equals("{\"e\": 1.7976931348623156e+308}", "{\"e\": 1.7976931348623156e+308}"));
+
+        // Justification of above float values
+        double e1 = 2.7182818284590452354;
+        double e2 = 2.718281828459045;
+        double e3 = 2.71828182845904;
+        assertEquals(e1, Math.E, -1);
+        assertEquals(e1, e2, -1);
+        assertNotEquals(e1, e3, -1);
+
+        // Invalid JSON throws RuntimeException
+        assertRuntimeException(() -> JSON.equals("", "{}"));
+        assertRuntimeException(() -> JSON.equals("{}", ""));
+        assertRuntimeException(() -> JSON.equals("{", "{}"));
+        assertRuntimeException(() -> JSON.equals("{}", "{"));
+    }
+
+    @Test
+    public void implementationSpecificEqualsBehavior() {
+        // Exception thrown if outside a long
+        assertTrue(                  JSON.equals("{\"a\": 9223372036854775807}", "{\"a\": 9223372036854775807}"));
+        assertRuntimeException(() -> JSON.equals("{\"a\": 9223372036854775808}", "{\"a\": 9223372036854775808}"));
+
+        // Infinity if floating point number outside of double, and hence equal
+        assertTrue(JSON.equals("{\"a\": 2.7976931348623158e+308}", "{\"a\": 2.7976931348623158e+308}"));
+
+        // Ignores extraneous precision
+        assertTrue(JSON.equals( "{\"e\": 2.7182818284590452354}",
+                                 "{\"e\": 2.7182818284590452354}"));
+        assertTrue(JSON.equals( "{\"e\": 2.7182818284590452354}",
+                                 "{\"e\": 2.7182818284590452355}"));
+        assertFalse(JSON.equals("{\"e\": 2.7182818284590452354}",
+                                 "{\"e\": 2.71828182845904}"));
+
+        // Comparing equal but syntactically different numbers
+        assertFalse(JSON.equals("{\"a\": 1.0}", "{\"a\":1}"));
+        assertTrue(JSON.equals("{\"a\": 1.0}", "{\"a\":1.00}"));
+        assertTrue(JSON.equals("{\"a\": 1.0}", "{\"a\":1.0000000000000000000000000000}"));
+        assertTrue(JSON.equals("{\"a\": 10.0}", "{\"a\":1e1}"));
+        assertTrue(JSON.equals("{\"a\": 1.2}", "{\"a\":12e-1}"));
+    }
+
+    private static void assertRuntimeException(Runnable runnable) {
+        try {
+            runnable.run();
+            fail("Expected RuntimeException to be thrown, but no exception was thrown");
+        } catch (RuntimeException e) {
+            // OK
+        }
+    }
+
 }