runtime: Add getDouble functionality to snapshot (envoyproxy#8265)

Signed-off-by: Tony Allen <tallen@lyft.com>

docs/root/configuration/operations/runtime.rst

@@ -200,7 +200,7 @@ message as a `google.protobuf.Struct
 modeling a JSON object with the following rules:
 
 * Dot separators map to tree edges.
-* Scalar leaves (integer, strings, booleans) are represented with their respective JSON type.
+* Scalar leaves (integer, strings, booleans, doubles) are represented with their respective JSON type.
 * :ref:`FractionalPercent <envoy_api_msg_type.FractionalPercent>` is represented with via its
   `canonical JSON encoding <https://developers.google.com/protocol-buffers/docs/proto3#json>`_.
 
@@ -211,6 +211,10 @@ An example representation of a setting for the *health_check.min_interval* key i
   health_check:
     min_interval: 5
 
+.. note::
+
+  Integer values that are parsed from doubles are rounded down to the nearest whole number.
+
 .. _config_runtime_comments:
 
 Comments

docs/root/intro/version_history.rst

@@ -60,6 +60,7 @@ Version history
 * router check tool: add deprecated field check.
 * router check tool: add flag for only printing results of failed tests.
 * router check tool: add support for outputting missing tests in the detailed coverage report.
+* runtime: allow for the ability to parse integers as double values and vice-versa.
 * server: added a post initialization lifecycle event, in addition to the existing startup and shutdown events.
 * server: added :ref:`per-handler listener stats <config_listener_stats_per_handler>` and
   :ref:`per-worker watchdog stats <operations_performance_watchdog>` to help diagnosing event

include/envoy/runtime/runtime.h

@@ -54,6 +54,7 @@ class Snapshot {
   struct Entry {
     std::string raw_string_value_;
     absl::optional<uint64_t> uint_value_;
+    absl::optional<double> double_value_;
     absl::optional<envoy::type::FractionalPercent> fractional_percent_value_;
     absl::optional<bool> bool_value_;
   };
@@ -186,14 +187,24 @@ class Snapshot {
   virtual const std::string& get(const std::string& key) const PURE;
 
   /**
-   * Fetch an integer runtime key.
+   * Fetch an integer runtime key. Runtime keys larger than ~2^53 may not be accurately converted
+   * into integers and will return default_value.
    * @param key supplies the key to fetch.
    * @param default_value supplies the value to return if the key does not exist or it does not
    *        contain an integer.
    * @return uint64_t the runtime value or the default value.
    */
   virtual uint64_t getInteger(const std::string& key, uint64_t default_value) const PURE;
 
+  /**
+   * Fetch a double runtime key.
+   * @param key supplies the key to fetch.
+   * @param default_value supplies the value to return if the key does not exist or it does not
+   *        contain a double.
+   * @return double the runtime value or the default value.
+   */
+  virtual double getDouble(const std::string& key, double default_value) const PURE;
+
   /**
    * Fetch the OverrideLayers that provide values in this snapshot. Layers are ordered from bottom
    * to top; for instance, the second layer's entries override the first layer's entries, and so on.

source/common/runtime/runtime_impl.cc

@@ -19,6 +19,7 @@
 #include "common/runtime/runtime_features.h"
 
 #include "absl/strings/match.h"
+#include "absl/strings/numbers.h"
 #include "openssl/rand.h"
 
 namespace Envoy {
@@ -276,6 +277,15 @@ uint64_t SnapshotImpl::getInteger(const std::string& key, uint64_t default_value
   }
 }
 
+double SnapshotImpl::getDouble(const std::string& key, double default_value) const {
+  auto entry = values_.find(key);
+  if (entry == values_.end() || !entry->second.double_value_) {
+    return default_value;
+  } else {
+    return entry->second.double_value_.value();
+  }
+}
+
 bool SnapshotImpl::getBoolean(absl::string_view key, bool& value) const {
   auto entry = values_.find(key);
   if (entry != values_.end() && entry->second.bool_value_.has_value()) {
@@ -332,10 +342,10 @@ bool SnapshotImpl::parseEntryBooleanValue(Entry& entry) {
   return false;
 }
 
-bool SnapshotImpl::parseEntryUintValue(Entry& entry) {
-  uint64_t converted_uint64;
-  if (absl::SimpleAtoi(entry.raw_string_value_, &converted_uint64)) {
-    entry.uint_value_ = converted_uint64;
+bool SnapshotImpl::parseEntryDoubleValue(Entry& entry) {
+  double converted_double;
+  if (absl::SimpleAtod(entry.raw_string_value_, &converted_double)) {
+    entry.double_value_ = converted_double;
     return true;
   }
   return false;

source/common/runtime/runtime_impl.h

@@ -92,6 +92,7 @@ class SnapshotImpl : public Snapshot,
                       uint64_t random_value) const override;
   const std::string& get(const std::string& key) const override;
   uint64_t getInteger(const std::string& key, uint64_t default_value) const override;
+  double getDouble(const std::string& key, double default_value) const override;
   const std::vector<OverrideLayerConstPtr>& getLayers() const override;
 
   static Entry createEntry(const std::string& value);
@@ -106,14 +107,21 @@ class SnapshotImpl : public Snapshot,
     if (parseEntryBooleanValue(entry)) {
       return;
     }
-    if (parseEntryUintValue(entry)) {
+
+    if (parseEntryDoubleValue(entry) && entry.double_value_ >= 0 &&
+        entry.double_value_ <= std::numeric_limits<uint64_t>::max()) {
+      // Valid uint values will always be parseable as doubles, so we assign the value to both the
+      // uint and double fields. In cases where the value is something like "3.1", we will floor the
+      // number by casting it to a uint and assigning the uint value.
+      entry.uint_value_ = entry.double_value_;
       return;
     }
+
     parseEntryFractionalPercentValue(entry);
   }
 
   static bool parseEntryBooleanValue(Entry& entry);
-  static bool parseEntryUintValue(Entry& entry);
+  static bool parseEntryDoubleValue(Entry& entry);
   static void parseEntryFractionalPercentValue(Entry& entry);
 
   const std::vector<OverrideLayerConstPtr> layers_;

test/common/runtime/runtime_impl_test.cc

@@ -138,6 +138,22 @@ class DiskLoaderImplTest : public LoaderImplTest {
   ProtobufWkt::Struct base_;
 };
 
+TEST_F(DiskLoaderImplTest, DoubleUintInteraction) {
+  setup();
+  run("test/common/runtime/test_data/current", "envoy_override");
+
+  EXPECT_EQ(2UL, loader_->snapshot().getInteger("file3", 1));
+  EXPECT_EQ(2.0, loader_->snapshot().getDouble("file3", 1.1));
+}
+
+TEST_F(DiskLoaderImplTest, DoubleUintInteractionNegatives) {
+  setup();
+  run("test/common/runtime/test_data/current", "envoy_override");
+
+  EXPECT_EQ(1, loader_->snapshot().getInteger("file_with_negative_double", 1));
+  EXPECT_EQ(-4.2, loader_->snapshot().getDouble("file_with_negative_double", 1.1));
+}
+
 TEST_F(DiskLoaderImplTest, All) {
   setup();
   run("test/common/runtime/test_data/current", "envoy_override");
@@ -152,6 +168,14 @@ TEST_F(DiskLoaderImplTest, All) {
   EXPECT_EQ(2UL, loader_->snapshot().getInteger("file3", 1));
   EXPECT_EQ(123UL, loader_->snapshot().getInteger("file4", 1));
 
+  // Double getting.
+  // Bogus string, expect default.
+  EXPECT_EQ(42.1, loader_->snapshot().getDouble("file_with_words", 42.1));
+  // Valid float string.
+  EXPECT_EQ(23.2, loader_->snapshot().getDouble("file_with_double", 1.1));
+  // Valid float string followed by newlines.
+  EXPECT_EQ(3.141, loader_->snapshot().getDouble("file_with_double_newlines", 1.1));
+
   bool value;
   const SnapshotImpl* snapshot = reinterpret_cast<const SnapshotImpl*>(&loader_->snapshot());
 
@@ -192,6 +216,7 @@ TEST_F(DiskLoaderImplTest, All) {
 
   // Files with comments.
   EXPECT_EQ(123UL, loader_->snapshot().getInteger("file5", 1));
+  EXPECT_EQ(2.718, loader_->snapshot().getDouble("file_with_double_comment", 1.1));
   EXPECT_EQ("/home#about-us", loader_->snapshot().get("file6"));
   EXPECT_EQ("", loader_->snapshot().get("file7"));
 
@@ -247,10 +272,17 @@ TEST_F(DiskLoaderImplTest, All) {
 
   EXPECT_EQ(0, store_.counter("runtime.load_error").value());
   EXPECT_EQ(1, store_.counter("runtime.load_success").value());
-  EXPECT_EQ(17, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
+  EXPECT_EQ(23, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
   EXPECT_EQ(4, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value());
 }
 
+TEST_F(DiskLoaderImplTest, UintLargeIntegerConversion) {
+  setup();
+  run("test/common/runtime/test_data/current", "envoy_override");
+
+  EXPECT_EQ(1, loader_->snapshot().getInteger("file_with_large_integer", 1));
+}
+
 TEST_F(DiskLoaderImplTest, GetLayers) {
   base_ = TestUtility::parseYaml<ProtobufWkt::Struct>(R"EOF(
     foo: whatevs
@@ -355,25 +387,35 @@ void testNewOverrides(Loader& loader, Stats::Store& store) {
   Stats::Gauge& admin_overrides_active =
       store.gauge("runtime.admin_overrides_active", Stats::Gauge::ImportMode::NeverImport);
 
-  // New string
+  // New string.
   loader.mergeValues({{"foo", "bar"}});
   EXPECT_EQ("bar", loader.snapshot().get("foo"));
   EXPECT_EQ(1, admin_overrides_active.value());
 
-  // Remove new string
+  // Remove new string.
   loader.mergeValues({{"foo", ""}});
   EXPECT_EQ("", loader.snapshot().get("foo"));
   EXPECT_EQ(0, admin_overrides_active.value());
 
-  // New integer
+  // New integer.
   loader.mergeValues({{"baz", "42"}});
   EXPECT_EQ(42, loader.snapshot().getInteger("baz", 0));
   EXPECT_EQ(1, admin_overrides_active.value());
 
-  // Remove new integer
+  // Remove new integer.
   loader.mergeValues({{"baz", ""}});
   EXPECT_EQ(0, loader.snapshot().getInteger("baz", 0));
   EXPECT_EQ(0, admin_overrides_active.value());
+
+  // New double.
+  loader.mergeValues({{"beep", "42.1"}});
+  EXPECT_EQ(42.1, loader.snapshot().getDouble("beep", 1.2));
+  EXPECT_EQ(1, admin_overrides_active.value());
+
+  // Remove new double.
+  loader.mergeValues({{"beep", ""}});
+  EXPECT_EQ(1.2, loader.snapshot().getDouble("beep", 1.2));
+  EXPECT_EQ(0, admin_overrides_active.value());
 }
 
 TEST_F(DiskLoaderImplTest, MergeValues) {
@@ -403,6 +445,16 @@ TEST_F(DiskLoaderImplTest, MergeValues) {
   EXPECT_EQ(2, loader_->snapshot().getInteger("file3", 1));
   EXPECT_EQ(0, admin_overrides_active.value());
 
+  // Override double
+  loader_->mergeValues({{"file_with_double", "42.1"}});
+  EXPECT_EQ(42.1, loader_->snapshot().getDouble("file_with_double", 1.1));
+  EXPECT_EQ(1, admin_overrides_active.value());
+
+  // Remove overridden double
+  loader_->mergeValues({{"file_with_double", ""}});
+  EXPECT_EQ(23.2, loader_->snapshot().getDouble("file_with_double", 1.1));
+  EXPECT_EQ(0, admin_overrides_active.value());
+
   // Override override string
   loader_->mergeValues({{"file1", "hello overridden override"}});
   EXPECT_EQ("hello overridden override", loader_->snapshot().get("file1"));
@@ -415,7 +467,7 @@ TEST_F(DiskLoaderImplTest, MergeValues) {
   EXPECT_EQ(0, store_.gauge("runtime.admin_overrides_active", Stats::Gauge::ImportMode::NeverImport)
                    .value());
 
-  EXPECT_EQ(11, store_.counter("runtime.load_success").value());
+  EXPECT_EQ(15, store_.counter("runtime.load_success").value());
   EXPECT_EQ(4, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value());
 }
 
@@ -498,6 +550,7 @@ TEST_F(StaticLoaderImplTest, All) {
   setup();
   EXPECT_EQ("", loader_->snapshot().get("foo"));
   EXPECT_EQ(1UL, loader_->snapshot().getInteger("foo", 1));
+  EXPECT_EQ(1.1, loader_->snapshot().getDouble("foo", 1.1));
   EXPECT_CALL(generator_, random()).WillOnce(Return(49));
   EXPECT_TRUE(loader_->snapshot().featureEnabled("foo", 50));
   testNewOverrides(*loader_, store_);
@@ -530,6 +583,8 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
       numerator: 100
       foo: bar
     empty: {}
+    file_with_words: "some words"
+    file_with_double: 23.2
   )EOF");
   setup();
 
@@ -543,6 +598,10 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
   EXPECT_EQ(2UL, loader_->snapshot().getInteger("file3", 1));
   EXPECT_EQ(123UL, loader_->snapshot().getInteger("file4", 1));
 
+  // Double getting.
+  EXPECT_EQ(1.1, loader_->snapshot().getDouble("file_with_words", 1.1));
+  EXPECT_EQ(23.2, loader_->snapshot().getDouble("file_with_double", 1.1));
+
   // Boolean getting.
   bool value;
   const SnapshotImpl* snapshot = reinterpret_cast<const SnapshotImpl*>(&loader_->snapshot());
@@ -613,7 +672,7 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
 
   EXPECT_EQ(0, store_.counter("runtime.load_error").value());
   EXPECT_EQ(1, store_.counter("runtime.load_success").value());
-  EXPECT_EQ(15, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
+  EXPECT_EQ(17, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
   EXPECT_EQ(2, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value());
 }
 

test/common/runtime/test_data/root/envoy/file_with_double

@@ -0,0 +1 @@
+23.2

test/common/runtime/test_data/root/envoy/file_with_double_comment

@@ -0,0 +1,2 @@
+# Here's a comment!
+2.718

test/common/runtime/test_data/root/envoy/file_with_double_newlines

@@ -0,0 +1,4 @@
+3.141
+
+
+

test/common/runtime/test_data/root/envoy/file_with_large_integer

@@ -0,0 +1,2 @@
+# 2^64 * 10
+184467440737095516160

test/common/runtime/test_data/root/envoy/file_with_negative_double

@@ -0,0 +1 @@
+-4.2

test/common/runtime/test_data/root/envoy/file_with_words

@@ -0,0 +1 @@
+bogus string

test/mocks/runtime/mocks.h

@@ -54,6 +54,7 @@ class MockSnapshot : public Snapshot {
                                           uint64_t random_value));
   MOCK_CONST_METHOD1(get, const std::string&(const std::string& key));
   MOCK_CONST_METHOD2(getInteger, uint64_t(const std::string& key, uint64_t default_value));
+  MOCK_CONST_METHOD2(getDouble, double(const std::string& key, double default_value));
   MOCK_CONST_METHOD0(getLayers, const std::vector<OverrideLayerConstPtr>&());
 };
 

test/server/http/admin_test.cc

@@ -947,11 +947,11 @@ TEST_P(AdminInstanceTest, Runtime) {
   Runtime::MockLoader loader;
   auto layer1 = std::make_unique<NiceMock<Runtime::MockOverrideLayer>>();
   auto layer2 = std::make_unique<NiceMock<Runtime::MockOverrideLayer>>();
-  Runtime::Snapshot::EntryMap entries2{{"string_key", {"override", {}, {}, {}}},
-                                       {"extra_key", {"bar", {}, {}, {}}}};
-  Runtime::Snapshot::EntryMap entries1{{"string_key", {"foo", {}, {}, {}}},
-                                       {"int_key", {"1", 1, {}, {}}},
-                                       {"other_key", {"bar", {}, {}, {}}}};
+  Runtime::Snapshot::EntryMap entries2{{"string_key", {"override", {}, {}, {}, {}}},
+                                       {"extra_key", {"bar", {}, {}, {}, {}}}};
+  Runtime::Snapshot::EntryMap entries1{{"string_key", {"foo", {}, {}, {}, {}}},
+                                       {"int_key", {"1", 1, {}, {}, {}}},
+                                       {"other_key", {"bar", {}, {}, {}, {}}}};
 
   ON_CALL(*layer1, name()).WillByDefault(testing::ReturnRefOfCopy(std::string{"layer1"}));
   ON_CALL(*layer1, values()).WillByDefault(testing::ReturnRef(entries1));