diff --git a/docs/root/configuration/operations/runtime.rst b/docs/root/configuration/operations/runtime.rst index 40dd2af9ab5c..47ebcfe7400c 100644 --- a/docs/root/configuration/operations/runtime.rst +++ b/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 ` is represented with via its `canonical JSON encoding `_. @@ -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 diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index a40cd980aca3..969be0791e53 100644 --- a/docs/root/intro/version_history.rst +++ b/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 ` and :ref:`per-worker watchdog stats ` to help diagnosing event diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index f850ec18fde4..e4ca1ed18a7d 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -54,6 +54,7 @@ class Snapshot { struct Entry { std::string raw_string_value_; absl::optional uint_value_; + absl::optional double_value_; absl::optional fractional_percent_value_; absl::optional bool_value_; }; @@ -186,7 +187,8 @@ 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. @@ -194,6 +196,15 @@ class Snapshot { */ 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. diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 220e4305571e..3d0a721c6aa1 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/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; diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 9b3eb6a50ce1..f5276961886d 100644 --- a/source/common/runtime/runtime_impl.h +++ b/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& 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::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 layers_; diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 7b06db0f9cdd..ef5428e19ea9 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/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(&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(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(&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()); } diff --git a/test/common/runtime/test_data/root/envoy/file_with_double b/test/common/runtime/test_data/root/envoy/file_with_double new file mode 100644 index 000000000000..3c8ce91a4696 --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file_with_double @@ -0,0 +1 @@ +23.2 diff --git a/test/common/runtime/test_data/root/envoy/file_with_double_comment b/test/common/runtime/test_data/root/envoy/file_with_double_comment new file mode 100644 index 000000000000..3ea19b108ca2 --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file_with_double_comment @@ -0,0 +1,2 @@ +# Here's a comment! +2.718 diff --git a/test/common/runtime/test_data/root/envoy/file_with_double_newlines b/test/common/runtime/test_data/root/envoy/file_with_double_newlines new file mode 100644 index 000000000000..8699ce764422 --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file_with_double_newlines @@ -0,0 +1,4 @@ +3.141 + + + diff --git a/test/common/runtime/test_data/root/envoy/file_with_large_integer b/test/common/runtime/test_data/root/envoy/file_with_large_integer new file mode 100644 index 000000000000..00e88310041a --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file_with_large_integer @@ -0,0 +1,2 @@ +# 2^64 * 10 +184467440737095516160 diff --git a/test/common/runtime/test_data/root/envoy/file_with_negative_double b/test/common/runtime/test_data/root/envoy/file_with_negative_double new file mode 100644 index 000000000000..50c9d06aa52c --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file_with_negative_double @@ -0,0 +1 @@ +-4.2 diff --git a/test/common/runtime/test_data/root/envoy/file_with_words b/test/common/runtime/test_data/root/envoy/file_with_words new file mode 100644 index 000000000000..a8287c64e148 --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file_with_words @@ -0,0 +1 @@ +bogus string diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index 388d789a8940..8e55cc6732d3 100644 --- a/test/mocks/runtime/mocks.h +++ b/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&()); }; diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 38b87e72fb7b..f6636b7b6f1c 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -947,11 +947,11 @@ TEST_P(AdminInstanceTest, Runtime) { Runtime::MockLoader loader; auto layer1 = std::make_unique>(); auto layer2 = std::make_unique>(); - 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));