From ac672705a45c14d0cc96629366980e7c853e2d1c Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Fri, 16 Mar 2018 10:44:42 -0700 Subject: [PATCH 01/10] admin: add /runtime_modify endpoint and change runtime to work without fs backing Signed-off-by: James Sedgwick --- include/envoy/runtime/runtime.h | 7 + source/common/runtime/runtime_impl.cc | 159 +++++++++++++++++------ source/common/runtime/runtime_impl.h | 155 ++++++++++------------ source/server/http/admin.cc | 19 ++- source/server/http/admin.h | 2 + source/server/server.cc | 4 +- test/common/runtime/runtime_impl_test.cc | 71 ++++++++-- test/mocks/runtime/mocks.h | 1 + test/server/http/admin_test.cc | 31 +++++ 9 files changed, 304 insertions(+), 145 deletions(-) diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index 9fc1442444198..1813ddde1da29 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -135,6 +135,13 @@ class Loader { * fetched again when needed. */ virtual Snapshot& snapshot() PURE; + + /** + * Merge the given map of key-value pairs into the runtime's state. To remove a previous merge for + * a key, use an empty string as the value. + * @param values the values to merge + */ + virtual void mergeValues(const std::unordered_map& values) PURE; }; typedef std::unique_ptr LoaderPtr; diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index d6700e59b02d9..075dd5953ef8c 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -146,26 +146,26 @@ std::string RandomGeneratorImpl::uuid() { return std::string(uuid, UUID_LENGTH); } -SnapshotImpl::SnapshotImpl(const std::string& root_path, const std::string& override_path, - RuntimeStats& stats, RandomGenerator& generator, - Api::OsSysCalls& os_sys_calls) - : generator_(generator), os_sys_calls_(os_sys_calls) { - try { - walkDirectory(root_path, ""); - if (Filesystem::directoryExists(override_path)) { - walkDirectory(override_path, ""); - stats.override_dir_exists_.inc(); - } else { - stats.override_dir_not_exists_.inc(); - } +bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value, + uint64_t random_value, uint64_t num_buckets) const { + return random_value % num_buckets < std::min(getInteger(key, default_value), num_buckets); +} - stats.load_success_.inc(); - } catch (EnvoyException& e) { - stats.load_error_.inc(); - ENVOY_LOG(debug, "error creating runtime snapshot: {}", e.what()); +bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value) const { + // Avoid PNRG if we know we don't need it. + uint64_t cutoff = std::min(getInteger(key, default_value), static_cast(100)); + if (cutoff == 0) { + return false; + } else if (cutoff == 100) { + return true; + } else { + return generator_.random() % 100 < cutoff; } +} - stats.num_keys_.set(values_.size()); +bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value, + uint64_t random_value) const { + return featureEnabled(key, default_value, random_value, 100); } const std::string& SnapshotImpl::get(const std::string& key) const { @@ -190,7 +190,56 @@ const std::unordered_map& SnapshotImpl::getA return values_; } -void SnapshotImpl::walkDirectory(const std::string& path, const std::string& prefix) { +SnapshotImpl::SnapshotImpl(RandomGenerator& generator, + const std::unordered_map& values) + : SnapshotImpl(generator) { + mergeValues(values); +} + +SnapshotImpl::SnapshotImpl(RandomGenerator& generator) : generator_(generator) {} + +void SnapshotImpl::mergeValues(const std::unordered_map& values) { + for (const auto& kv : values) { + Entry entry{kv.second, absl::nullopt}; + tryConvertToInteger(entry); + values_.erase(kv.first); + values_.emplace(kv.first, std::move(entry)); + } +} + +void SnapshotImpl::tryConvertToInteger(Snapshot::Entry& entry) { + // As a perf optimization, attempt to convert the entry's string into an integer. If we don't + // succeed that's fine. + uint64_t converted; + if (StringUtil::atoul(entry.string_value_.c_str(), converted)) { + entry.uint_value_ = converted; + } +} + +DiskBackedSnapshotImpl::DiskBackedSnapshotImpl( + RandomGenerator& generator, + const std::unordered_map& additional_overrides, + const std::string& root_path, const std::string& override_path, RuntimeStats& stats, + Api::OsSysCalls& os_sys_calls) + : SnapshotImpl(generator), os_sys_calls_(os_sys_calls) { + try { + walkDirectory(root_path, ""); + if (Filesystem::directoryExists(override_path)) { + walkDirectory(override_path, ""); + stats.override_dir_exists_.inc(); + } else { + stats.override_dir_not_exists_.inc(); + } + mergeValues(additional_overrides); + } catch (EnvoyException& e) { + stats.load_error_.inc(); + ENVOY_LOG(debug, "error creating runtime snapshot: {}", e.what()); + } + + stats.num_keys_.set(values_.size()); +} + +void DiskBackedSnapshotImpl::walkDirectory(const std::string& path, const std::string& prefix) { ENVOY_LOG(debug, "walking directory: {}", path); Directory current_dir(path); while (true) { @@ -243,13 +292,7 @@ void SnapshotImpl::walkDirectory(const std::string& path, const std::string& pre entry.string_value_.append(std::string{line} + "\n"); } } - - // As a perf optimization, attempt to convert the string into an integer. If we don't - // succeed that's fine. - uint64_t converted; - if (StringUtil::atoul(entry.string_value_.c_str(), converted)) { - entry.uint_value_ = converted; - } + tryConvertToInteger(entry); // Separate erase/insert calls required due to the value type being constant; this prevents // the use of the [] operator. Can leverage insert_or_assign in C++17 in the future. @@ -259,37 +302,67 @@ void SnapshotImpl::walkDirectory(const std::string& path, const std::string& pre } } -LoaderImpl::LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, - const std::string& root_symlink_path, const std::string& subdir, - const std::string& override_dir, Stats::Store& store, - RandomGenerator& generator, Api::OsSysCallsPtr os_sys_calls) - : watcher_(dispatcher.createFilesystemWatcher()), tls_(tls.allocateSlot()), - generator_(generator), root_path_(root_symlink_path + "/" + subdir), +LoaderImpl::LoaderImpl(RandomGenerator& generator, ThreadLocal::SlotAllocator& tls) + : LoaderImpl(DoNotLoadSnapshot{}, generator, tls) { + loadNewSnapshot(); +} + +LoaderImpl::LoaderImpl(DoNotLoadSnapshot /* unused */, RandomGenerator& generator, + ThreadLocal::SlotAllocator& tls) + : generator_(generator), tls_(tls.allocateSlot()) {} + +std::unique_ptr LoaderImpl::createNewSnapshot() { + return std::make_unique(generator_, values_); +} + +void LoaderImpl::loadNewSnapshot() { + ThreadLocal::ThreadLocalObjectSharedPtr ptr = createNewSnapshot(); + tls_->set([ptr_copy = ptr](Event::Dispatcher&)->ThreadLocal::ThreadLocalObjectSharedPtr { + return ptr_copy; + }); +} + +Snapshot& LoaderImpl::snapshot() { return tls_->getTyped(); } + +void LoaderImpl::mergeValues(const std::unordered_map& values) { + for (const auto& kv : values) { + values_.erase(kv.first); + if (!kv.second.empty()) { + values_.insert(kv); + } + } + + loadNewSnapshot(); +} + +DiskBackedLoaderImpl::DiskBackedLoaderImpl(Event::Dispatcher& dispatcher, + ThreadLocal::SlotAllocator& tls, + const std::string& root_symlink_path, + const std::string& subdir, + const std::string& override_dir, Stats::Store& store, + RandomGenerator& generator, + Api::OsSysCallsPtr os_sys_calls) + : LoaderImpl(DoNotLoadSnapshot{}, generator, tls), + watcher_(dispatcher.createFilesystemWatcher()), root_path_(root_symlink_path + "/" + subdir), override_path_(root_symlink_path + "/" + override_dir), stats_(generateStats(store)), os_sys_calls_(std::move(os_sys_calls)) { watcher_->addWatch(root_symlink_path, Filesystem::Watcher::Events::MovedTo, - [this](uint32_t) -> void { onSymlinkSwap(); }); + [this](uint32_t) -> void { loadNewSnapshot(); }); - onSymlinkSwap(); + loadNewSnapshot(); } -RuntimeStats LoaderImpl::generateStats(Stats::Store& store) { +RuntimeStats DiskBackedLoaderImpl::generateStats(Stats::Store& store) { std::string prefix = "runtime."; RuntimeStats stats{ ALL_RUNTIME_STATS(POOL_COUNTER_PREFIX(store, prefix), POOL_GAUGE_PREFIX(store, prefix))}; return stats; } -void LoaderImpl::onSymlinkSwap() { - current_snapshot_.reset( - new SnapshotImpl(root_path_, override_path_, stats_, generator_, *os_sys_calls_)); - ThreadLocal::ThreadLocalObjectSharedPtr ptr_copy = current_snapshot_; - tls_->set([ptr_copy](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr { - return ptr_copy; - }); +std::unique_ptr DiskBackedLoaderImpl::createNewSnapshot() { + return std::make_unique(generator_, values_, root_path_, override_path_, + stats_, *os_sys_calls_); } -Snapshot& LoaderImpl::snapshot() { return tls_->getTyped(); } - } // namespace Runtime } // namespace Envoy diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index e08605f9edf66..19e9c2ce4a371 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -55,42 +55,47 @@ struct RuntimeStats { }; /** - * Implementation of Snapshot that reads from disk. + * Implementation of Snapshot whose source is the values map provided to its constructor. + * Used by LoaderImpl. */ -class SnapshotImpl : public Snapshot, - public ThreadLocal::ThreadLocalObject, - Logger::Loggable { +class SnapshotImpl : public Snapshot, public ThreadLocal::ThreadLocalObject { public: - SnapshotImpl(const std::string& root_path, const std::string& override_path, RuntimeStats& stats, - RandomGenerator& generator, Api::OsSysCalls& os_sys_calls); + SnapshotImpl(RandomGenerator& generator, + const std::unordered_map& values); // Runtime::Snapshot bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, - uint64_t num_buckets) const override { - return random_value % num_buckets < std::min(getInteger(key, default_value), num_buckets); - } - - bool featureEnabled(const std::string& key, uint64_t default_value) const override { - // Avoid PNRG if we know we don't need it. - uint64_t cutoff = std::min(getInteger(key, default_value), static_cast(100)); - if (cutoff == 0) { - return false; - } else if (cutoff == 100) { - return true; - } else { - return generator_.random() % 100 < cutoff; - } - } - + uint64_t num_buckets) const override; + bool featureEnabled(const std::string& key, uint64_t default_value) const override; bool featureEnabled(const std::string& key, uint64_t default_value, - uint64_t random_value) const override { - return featureEnabled(key, default_value, random_value, 100); - } - + uint64_t random_value) const override; const std::string& get(const std::string& key) const override; - uint64_t getInteger(const std::string&, uint64_t default_value) const override; + uint64_t getInteger(const std::string& key, uint64_t default_value) const override; const std::unordered_map& getAll() const override; +protected: + explicit SnapshotImpl(RandomGenerator& generator); + + static void tryConvertToInteger(Snapshot::Entry& entry); + void mergeValues(const std::unordered_map& values); + + std::unordered_map values_; + +private: + RandomGenerator& generator_; +}; + +/** + * Extension of SnapshotImpl that uses the disk as its primary source of values and SnapshotImpl's + * in-memory values as overrides. Used by DiskBackedLoaderImpl. + */ +class DiskBackedSnapshotImpl : public SnapshotImpl, Logger::Loggable { +public: + DiskBackedSnapshotImpl(RandomGenerator& generator, + const std::unordered_map& additional_overrides, + const std::string& root_path, const std::string& override_path, + RuntimeStats& stats, Api::OsSysCalls& os_sys_calls); + private: struct Directory { Directory(const std::string& path) { @@ -107,91 +112,63 @@ class SnapshotImpl : public Snapshot, void walkDirectory(const std::string& path, const std::string& prefix); - std::unordered_map values_; - RandomGenerator& generator_; Api::OsSysCalls& os_sys_calls_; }; /** - * Implementation of Loader that watches a symlink for swapping and loads a specified subdirectory - * from disk. A single snapshot is shared among all threads and referenced by shared_ptr such that + * Implementation of Loader that provides Snapshots of values added via mergeValues(). + * A single snapshot is shared among all threads and referenced by shared_ptr such that * a new runtime can be swapped in by the main thread while workers are still using the previous * version. */ class LoaderImpl : public Loader { public: - LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, - const std::string& root_symlink_path, const std::string& subdir, - const std::string& override_dir, Stats::Store& store, RandomGenerator& generator, - Api::OsSysCallsPtr os_sys_calls); + LoaderImpl(RandomGenerator& generator, ThreadLocal::SlotAllocator& tls); // Runtime::Loader Snapshot& snapshot() override; + void mergeValues(const std::unordered_map& values) override; -private: - RuntimeStats generateStats(Stats::Store& store); - void onSymlinkSwap(); +protected: + // Identical the the public constructor but does not call loadSnapshot(). Subclasses must call + // loadSnapshot() themselves to create the initial snapshot, since loadSnapshot calls the virtual + // function createNewSnapshot() and is therefore unsuitable for use in a superclass constructor. + struct DoNotLoadSnapshot {}; + LoaderImpl(DoNotLoadSnapshot /* unused */, RandomGenerator& generator, + ThreadLocal::SlotAllocator& tls); + + // Create a new Snapshot + virtual std::unique_ptr createNewSnapshot(); + // Load a new Snapshot into TLS + void loadNewSnapshot(); - Filesystem::WatcherPtr watcher_; - ThreadLocal::SlotPtr tls_; RandomGenerator& generator_; - std::string root_path_; - std::string override_path_; - std::shared_ptr current_snapshot_; - RuntimeStats stats_; - Api::OsSysCallsPtr os_sys_calls_; + std::unordered_map values_; + +private: + ThreadLocal::SlotPtr tls_; }; /** - * Null implementation of runtime if another runtime source is not configured. + * Extension of LoaderImpl that watches a symlink for swapping and loads a specified subdirectory + * from disk. Values added via mergeValues() are secondary to those loaded from disk. */ -class NullLoaderImpl : public Loader { +class DiskBackedLoaderImpl : public LoaderImpl { public: - NullLoaderImpl(RandomGenerator& generator) : snapshot_(generator) {} - - // Runtime::Loader - Snapshot& snapshot() override { return snapshot_; } + DiskBackedLoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, + const std::string& root_symlink_path, const std::string& subdir, + const std::string& override_dir, Stats::Store& store, + RandomGenerator& generator, Api::OsSysCallsPtr os_sys_calls); private: - struct NullSnapshotImpl : public Snapshot { - NullSnapshotImpl(RandomGenerator& generator) : generator_(generator) {} - - // Runtime::Snapshot - bool featureEnabled(const std::string&, uint64_t default_value, uint64_t random_value, - uint64_t num_buckets) const override { - return random_value % num_buckets < std::min(default_value, num_buckets); - } - - bool featureEnabled(const std::string& key, uint64_t default_value) const override { - if (default_value == 0) { - return false; - } else if (default_value == 100) { - return true; - } else { - return featureEnabled(key, default_value, generator_.random()); - } - } - - bool featureEnabled(const std::string& key, uint64_t default_value, - uint64_t random_value) const override { - return featureEnabled(key, default_value, random_value, 100); - } - - const std::string& get(const std::string&) const override { return EMPTY_STRING; } - - uint64_t getInteger(const std::string&, uint64_t default_value) const override { - return default_value; - } - - const std::unordered_map& getAll() const override { - return values_; - } - - RandomGenerator& generator_; - std::unordered_map values_; - }; + std::unique_ptr createNewSnapshot() override; + RuntimeStats generateStats(Stats::Store& store); - NullSnapshotImpl snapshot_; + Filesystem::WatcherPtr watcher_; + std::string root_path_; + std::string override_path_; + RuntimeStats stats_; + Api::OsSysCallsPtr os_sys_calls_; }; } // namespace Runtime diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index b71a0d9a62f87..0ce513d480b54 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -566,6 +566,21 @@ Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& res return rc; } +Http::Code AdminImpl::handlerRuntimeModify(const std::string& url, Http::HeaderMap&, + Buffer::Instance& response) { + const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); + if (params.empty()) { + response.add("usage: /runtime_modify?key1=value1&key2=value2&keyN=valueN\n"); + response.add("use an empty value to remove a previously added override"); + return Http::Code::BadRequest; + } + std::unordered_map overrides; + overrides.insert(params.begin(), params.end()); + server_.runtime().mergeValues(overrides); + response.add("OK\n"); + return Http::Code::OK; +} + const std::vector> AdminImpl::sortedRuntime( const std::unordered_map& entries) { std::vector> pairs(entries.begin(), @@ -681,7 +696,9 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof MAKE_ADMIN_HANDLER(handlerPrometheusStats), false, false}, {"/listeners", "print listener addresses", MAKE_ADMIN_HANDLER(handlerListenerInfo), false, false}, - {"/runtime", "print runtime values", MAKE_ADMIN_HANDLER(handlerRuntime), false, false}}, + {"/runtime", "print runtime values", MAKE_ADMIN_HANDLER(handlerRuntime), false, false}, + {"/runtime_modify", "modify runtime values", MAKE_ADMIN_HANDLER(handlerRuntimeModify), + false, true}}, listener_(*this, std::move(listener_scope)) { if (!address_out_path.empty()) { diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 4f5f3f13efe98..75c910eda0972 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -181,6 +181,8 @@ class AdminImpl : public Admin, Http::HeaderMap& response_headers, Buffer::Instance& response); Http::Code handlerRuntime(absl::string_view path_and_query, Http::HeaderMap& response_headers, Buffer::Instance& response); + Http::Code handlerRuntimeModify(const std::string& path_and_query, + Http::HeaderMap& response_headers, Buffer::Instance& response); class AdminListener : public Network::ListenerConfig { public: diff --git a/source/server/server.cc b/source/server/server.cc index 90c58c9ec4be4..24419af4607c4 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -289,12 +289,12 @@ Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server, ENVOY_LOG(info, "runtime override subdirectory: {}", override_subdirectory); Api::OsSysCallsPtr os_sys_calls(new Api::OsSysCallsImpl); - return Runtime::LoaderPtr{new Runtime::LoaderImpl( + return Runtime::LoaderPtr{new Runtime::DiskBackedLoaderImpl( server.dispatcher(), server.threadLocal(), config.runtime()->symlinkRoot(), config.runtime()->subdirectory(), override_subdirectory, server.stats(), server.random(), std::move(os_sys_calls))}; } else { - return Runtime::LoaderPtr{new Runtime::NullLoaderImpl(server.random())}; + return Runtime::LoaderPtr{new Runtime::LoaderImpl(server.random(), server.threadLocal())}; } } diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index c2bce70d267e6..e91a2e634b124 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -64,7 +64,7 @@ TEST(UUID, sanityCheckOfUniqueness) { EXPECT_EQ(num_of_uuids, uuids.size()); } -class RuntimeImplTest : public testing::Test { +class DiskBackedLoaderImplTest : public testing::Test { public: static void SetUpTestCase() { TestEnvironment::exec( @@ -83,8 +83,9 @@ class RuntimeImplTest : public testing::Test { void run(const std::string& primary_dir, const std::string& override_dir) { Api::OsSysCallsPtr os_sys_calls(os_sys_calls_); - loader.reset(new LoaderImpl(dispatcher, tls, TestEnvironment::temporaryPath(primary_dir), - "envoy", override_dir, store, generator, std::move(os_sys_calls))); + loader.reset(new DiskBackedLoaderImpl(dispatcher, tls, + TestEnvironment::temporaryPath(primary_dir), "envoy", + override_dir, store, generator, std::move(os_sys_calls))); } Event::MockDispatcher dispatcher; @@ -96,7 +97,7 @@ class RuntimeImplTest : public testing::Test { std::unique_ptr loader; }; -TEST_F(RuntimeImplTest, All) { +TEST_F(DiskBackedLoaderImplTest, All) { setup(); run("test/common/runtime/test_data/current", "envoy_override"); @@ -134,7 +135,7 @@ TEST_F(RuntimeImplTest, All) { EXPECT_EQ("hello override", loader->snapshot().get("file1")); } -TEST_F(RuntimeImplTest, GetAll) { +TEST_F(DiskBackedLoaderImplTest, GetAll) { setup(); run("test/common/runtime/test_data/current", "envoy_override"); @@ -160,33 +161,83 @@ TEST_F(RuntimeImplTest, GetAll) { EXPECT_TRUE(entry == values.end()); } -TEST_F(RuntimeImplTest, BadDirectory) { +TEST_F(DiskBackedLoaderImplTest, BadDirectory) { setup(); run("/baddir", "/baddir"); } -TEST_F(RuntimeImplTest, BadStat) { +TEST_F(DiskBackedLoaderImplTest, BadStat) { setup(); EXPECT_CALL(*os_sys_calls_, stat(_, _)).WillOnce(Return(-1)); run("test/common/runtime/test_data/current", "envoy_override"); EXPECT_EQ(store.counter("runtime.load_error").value(), 1); } -TEST_F(RuntimeImplTest, OverrideFolderDoesNotExist) { +TEST_F(DiskBackedLoaderImplTest, OverrideFolderDoesNotExist) { setup(); run("test/common/runtime/test_data/current", "envoy_override_does_not_exist"); EXPECT_EQ("hello", loader->snapshot().get("file1")); } -TEST(NullRuntimeImplTest, All) { +void testNewOverrides(Loader& loader) { + // New string + loader.mergeValues({{"foo", "bar"}}); + EXPECT_EQ("bar", loader.snapshot().get("foo")); + + // Remove new string + loader.mergeValues({{"foo", ""}}); + EXPECT_EQ("", loader.snapshot().get("foo")); + + // New integer + loader.mergeValues({{"baz", "42"}}); + EXPECT_EQ(42, loader.snapshot().getInteger("baz", 0)); + + // Remove new integer + loader.mergeValues({{"baz", ""}}); + EXPECT_EQ(0, loader.snapshot().getInteger("baz", 0)); +} + +TEST_F(DiskBackedLoaderImplTest, mergeValues) { + setup(); + run("test/common/runtime/test_data/current", "envoy_override"); + testNewOverrides(*loader); + + // Override string + loader->mergeValues({{"file2", "new world"}}); + EXPECT_EQ("new world", loader->snapshot().get("file2")); + + // Remove overridden string + loader->mergeValues({{"file2", ""}}); + EXPECT_EQ("world", loader->snapshot().get("file2")); + + // Override integer + loader->mergeValues({{"file3", "42"}}); + EXPECT_EQ(42, loader->snapshot().getInteger("file3", 1)); + + // Remove overridden integer + loader->mergeValues({{"file3", ""}}); + EXPECT_EQ(2, loader->snapshot().getInteger("file3", 1)); + + // Override override string + loader->mergeValues({{"file1", "hello overridden override"}}); + EXPECT_EQ("hello overridden override", loader->snapshot().get("file1")); + + // Remove overridden override string + loader->mergeValues({{"file1", ""}}); + EXPECT_EQ("hello override", loader->snapshot().get("file1")); +} + +TEST(LoaderImplTest, All) { MockRandomGenerator generator; - NullLoaderImpl loader(generator); + NiceMock tls; + LoaderImpl loader(generator, tls); EXPECT_EQ("", loader.snapshot().get("foo")); EXPECT_EQ(1UL, loader.snapshot().getInteger("foo", 1)); EXPECT_CALL(generator, random()).WillOnce(Return(49)); EXPECT_TRUE(loader.snapshot().featureEnabled("foo", 50)); EXPECT_TRUE(loader.snapshot().getAll().empty()); + testNewOverrides(loader); } } // namespace Runtime diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index e9a28be80786b..839bb597ab56b 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -43,6 +43,7 @@ class MockLoader : public Loader { ~MockLoader(); MOCK_METHOD0(snapshot, Snapshot&()); + MOCK_METHOD1(mergeValues, void(const std::unordered_map&)); testing::NiceMock snapshot_; }; diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index bc8f543eb7835..a5d12825aaa86 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -18,6 +18,7 @@ #include "test/test_common/printers.h" #include "test/test_common/utility.h" +#include "absl/strings/match.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -298,6 +299,36 @@ TEST_P(AdminInstanceTest, RuntimeBadFormat) { EXPECT_EQ("usage: /runtime?format=json\n", TestUtility::bufferToString(response)); } +TEST_P(AdminInstanceTest, RuntimeModify) { + Http::HeaderMapImpl header_map; + Buffer::OwnedImpl response; + + std::unordered_map entries; + Runtime::MockSnapshot snapshot; + Runtime::MockLoader loader; + + EXPECT_CALL(snapshot, getAll()).WillRepeatedly(testing::ReturnRef(entries)); + EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); + EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); + + std::unordered_map overrides; + overrides["foo"] = "bar"; + overrides["x"] = "42"; + overrides["nothing"] = ""; + EXPECT_CALL(loader, mergeValues(overrides)).Times(1); + EXPECT_EQ(Http::Code::OK, + admin_.runCallback("/runtime_modify?foo=bar&x=42¬hing=", header_map, response)); + EXPECT_EQ("OK\n", TestUtility::bufferToString(response)); +} + +TEST_P(AdminInstanceTest, RuntimeModifyNoArguments) { + Http::HeaderMapImpl header_map; + Buffer::OwnedImpl response; + + EXPECT_EQ(Http::Code::BadRequest, admin_.runCallback("/runtime_modify", header_map, response)); + EXPECT_TRUE(absl::StartsWith(TestUtility::bufferToString(response), "usage:")); +} + TEST(PrometheusStatsFormatter, MetricName) { std::string raw = "vulture.eats-liver"; std::string expected = "envoy_vulture_eats_liver"; From 831493eb05fb270b304e0a828b7331c852a9f824 Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Tue, 27 Mar 2018 18:20:24 -0700 Subject: [PATCH 02/10] implement override layers Signed-off-by: James Sedgwick --- include/envoy/runtime/runtime.h | 29 ++++++ source/common/runtime/runtime_impl.cc | 117 ++++++++++++----------- source/common/runtime/runtime_impl.h | 74 +++++++++----- source/server/http/admin.cc | 58 +++++++---- source/server/server.cc | 3 +- test/common/runtime/runtime_impl_test.cc | 3 +- test/mocks/runtime/mocks.h | 7 ++ test/server/http/admin_test.cc | 68 +++++-------- 8 files changed, 213 insertions(+), 146 deletions(-) diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index 1813ddde1da29..51232c244f3de 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -55,6 +55,25 @@ class Snapshot { absl::optional uint_value_; }; + /** + * A provider of runtime values. One or more of these compose the snapshot's source of values, + * where successive layers override the previous ones. + */ + class OverrideLayer { + public: + virtual ~OverrideLayer() {} + /** + * @return const std::unordered_map& the values in this layer. + */ + virtual const std::unordered_map& values() const PURE; + /** + * @return const std::string& a user-friendly alias for this layer, e.g. "admin" or "disk". + */ + virtual const std::string& name() const PURE; + }; + + typedef std::shared_ptr OverrideLayerSharedPtr; + /** * Test if a feature is enabled using the built in random generator. This is done by generating * a random number in the range 0-99 and seeing if this number is < the value stored in the @@ -116,10 +135,20 @@ class Snapshot { virtual uint64_t getInteger(const std::string& key, uint64_t default_value) const PURE; /** + * TODO: Reviewers, shall I delete getAll()? It's no longer used. * Fetch the raw runtime entries map. The map data is safe only for the lifetime of the Snapshot. * @return const std::unordered_map& the raw map of loaded values. */ virtual const std::unordered_map& getAll() 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. + * Any layer can add a key in addition to overriding keys in layers below. The layer vector is + * safe only for the lifetime of the Snapshot. + * @return const std::unordered_map& the raw map of loaded values. + */ + virtual const std::vector& getAllLayers() const PURE; }; /** diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 075dd5953ef8c..39f57e18e0b0e 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -190,56 +190,50 @@ const std::unordered_map& SnapshotImpl::getA return values_; } -SnapshotImpl::SnapshotImpl(RandomGenerator& generator, - const std::unordered_map& values) - : SnapshotImpl(generator) { - mergeValues(values); +const std::vector& SnapshotImpl::getAllLayers() const { + return layers_; } -SnapshotImpl::SnapshotImpl(RandomGenerator& generator) : generator_(generator) {} - -void SnapshotImpl::mergeValues(const std::unordered_map& values) { - for (const auto& kv : values) { - Entry entry{kv.second, absl::nullopt}; - tryConvertToInteger(entry); - values_.erase(kv.first); - values_.emplace(kv.first, std::move(entry)); +SnapshotImpl::SnapshotImpl(RandomGenerator& generator, RuntimeStats& stats, + const std::vector& layers) + : layers_{layers}, generator_{generator} { + for (const auto& layer : layers_) { + for (const auto& kv : layer->values()) { + values_.erase(kv.first); + values_.emplace(kv.first, kv.second); + } } + stats.num_keys_.set(values_.size()); } -void SnapshotImpl::tryConvertToInteger(Snapshot::Entry& entry) { +Snapshot::Entry SnapshotImpl::createEntry(const std::string& value) { + Entry entry{value, absl::nullopt}; // As a perf optimization, attempt to convert the entry's string into an integer. If we don't // succeed that's fine. uint64_t converted; if (StringUtil::atoul(entry.string_value_.c_str(), converted)) { entry.uint_value_ = converted; } + return entry; } -DiskBackedSnapshotImpl::DiskBackedSnapshotImpl( - RandomGenerator& generator, - const std::unordered_map& additional_overrides, - const std::string& root_path, const std::string& override_path, RuntimeStats& stats, - Api::OsSysCalls& os_sys_calls) - : SnapshotImpl(generator), os_sys_calls_(os_sys_calls) { - try { - walkDirectory(root_path, ""); - if (Filesystem::directoryExists(override_path)) { - walkDirectory(override_path, ""); - stats.override_dir_exists_.inc(); - } else { - stats.override_dir_not_exists_.inc(); +void AdminLayer::mergeValues(const std::unordered_map& values) { + for (const auto& kv : values) { + values_.erase(kv.first); + if (!kv.second.empty()) { + values_.emplace(kv.first, SnapshotImpl::createEntry(kv.second)); } - mergeValues(additional_overrides); - } catch (EnvoyException& e) { - stats.load_error_.inc(); - ENVOY_LOG(debug, "error creating runtime snapshot: {}", e.what()); } + stats_.admin_overrides_active_.set(values_.empty() ? 0 : 1); +} - stats.num_keys_.set(values_.size()); +DiskLayer::DiskLayer(const std::string& name, const std::string& path, + Api::OsSysCalls& os_sys_calls) + : OverrideLayerImpl{name}, os_sys_calls_(os_sys_calls) { + walkDirectory(path, ""); } -void DiskBackedSnapshotImpl::walkDirectory(const std::string& path, const std::string& prefix) { +void DiskLayer::walkDirectory(const std::string& path, const std::string& prefix) { ENVOY_LOG(debug, "walking directory: {}", path); Directory current_dir(path); while (true) { @@ -275,7 +269,7 @@ void DiskBackedSnapshotImpl::walkDirectory(const std::string& path, const std::s // for small files. Also, as noted elsewhere, none of this is non-blocking which could // theoretically lead to issues. ENVOY_LOG(debug, "reading file: {}", full_path); - Entry entry; + std::string value; // Read the file and remove any comments. A comment is a line starting with a '#' character. // Comments are useful for placeholder files with no value. @@ -287,51 +281,47 @@ void DiskBackedSnapshotImpl::walkDirectory(const std::string& path, const std::s } if (line == lines.back()) { const absl::string_view trimmed = StringUtil::rtrim(line); - entry.string_value_.append(trimmed.data(), trimmed.size()); + value.append(trimmed.data(), trimmed.size()); } else { - entry.string_value_.append(std::string{line} + "\n"); + value.append(std::string{line} + "\n"); } } - tryConvertToInteger(entry); - // Separate erase/insert calls required due to the value type being constant; this prevents // the use of the [] operator. Can leverage insert_or_assign in C++17 in the future. values_.erase(full_prefix); - values_.insert({full_prefix, entry}); + values_.insert({full_prefix, SnapshotImpl::createEntry(value)}); } } } -LoaderImpl::LoaderImpl(RandomGenerator& generator, ThreadLocal::SlotAllocator& tls) - : LoaderImpl(DoNotLoadSnapshot{}, generator, tls) { +LoaderImpl::LoaderImpl(RandomGenerator& generator, Stats::Store& store, + ThreadLocal::SlotAllocator& tls) + : LoaderImpl(DoNotLoadSnapshot{}, generator, store, tls) { loadNewSnapshot(); } LoaderImpl::LoaderImpl(DoNotLoadSnapshot /* unused */, RandomGenerator& generator, - ThreadLocal::SlotAllocator& tls) - : generator_(generator), tls_(tls.allocateSlot()) {} + Stats::Store& store, ThreadLocal::SlotAllocator& tls) + : generator_(generator), stats_(generateStats(store)), tls_(tls.allocateSlot()) { + adminLayer_ = std::make_shared(stats_); +} std::unique_ptr LoaderImpl::createNewSnapshot() { - return std::make_unique(generator_, values_); + return std::make_unique(generator_, stats_, + std::vector{adminLayer_}); } void LoaderImpl::loadNewSnapshot() { ThreadLocal::ThreadLocalObjectSharedPtr ptr = createNewSnapshot(); - tls_->set([ptr_copy = ptr](Event::Dispatcher&)->ThreadLocal::ThreadLocalObjectSharedPtr { - return ptr_copy; + tls_->set([ptr = std::move(ptr)](Event::Dispatcher&)->ThreadLocal::ThreadLocalObjectSharedPtr { + return ptr; }); } Snapshot& LoaderImpl::snapshot() { return tls_->getTyped(); } void LoaderImpl::mergeValues(const std::unordered_map& values) { - for (const auto& kv : values) { - values_.erase(kv.first); - if (!kv.second.empty()) { - values_.insert(kv); - } - } - + adminLayer_->mergeValues(values); loadNewSnapshot(); } @@ -342,9 +332,9 @@ DiskBackedLoaderImpl::DiskBackedLoaderImpl(Event::Dispatcher& dispatcher, const std::string& override_dir, Stats::Store& store, RandomGenerator& generator, Api::OsSysCallsPtr os_sys_calls) - : LoaderImpl(DoNotLoadSnapshot{}, generator, tls), + : LoaderImpl(DoNotLoadSnapshot{}, generator, store, tls), watcher_(dispatcher.createFilesystemWatcher()), root_path_(root_symlink_path + "/" + subdir), - override_path_(root_symlink_path + "/" + override_dir), stats_(generateStats(store)), + override_path_(root_symlink_path + "/" + override_dir), os_sys_calls_(std::move(os_sys_calls)) { watcher_->addWatch(root_symlink_path, Filesystem::Watcher::Events::MovedTo, [this](uint32_t) -> void { loadNewSnapshot(); }); @@ -352,7 +342,7 @@ DiskBackedLoaderImpl::DiskBackedLoaderImpl(Event::Dispatcher& dispatcher, loadNewSnapshot(); } -RuntimeStats DiskBackedLoaderImpl::generateStats(Stats::Store& store) { +RuntimeStats LoaderImpl::generateStats(Stats::Store& store) { std::string prefix = "runtime."; RuntimeStats stats{ ALL_RUNTIME_STATS(POOL_COUNTER_PREFIX(store, prefix), POOL_GAUGE_PREFIX(store, prefix))}; @@ -360,8 +350,21 @@ RuntimeStats DiskBackedLoaderImpl::generateStats(Stats::Store& store) { } std::unique_ptr DiskBackedLoaderImpl::createNewSnapshot() { - return std::make_unique(generator_, values_, root_path_, override_path_, - stats_, *os_sys_calls_); + std::vector layers; + try { + layers.push_back(std::make_unique("root", root_path_, *os_sys_calls_)); + if (Filesystem::directoryExists(override_path_)) { + layers.push_back(std::make_unique("override", override_path_, *os_sys_calls_)); + stats_.override_dir_exists_.inc(); + } else { + stats_.override_dir_not_exists_.inc(); + } + } catch (EnvoyException& e) { + stats_.load_error_.inc(); + ENVOY_LOG(debug, "error creating runtime snapshot: {}", e.what()); + } + layers.push_back(adminLayer_); + return std::make_unique(generator_, stats_, layers); } } // namespace Runtime diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 19e9c2ce4a371..239155d2590d2 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -44,7 +44,8 @@ class RandomGeneratorImpl : public RandomGenerator { COUNTER(override_dir_not_exists) \ COUNTER(override_dir_exists) \ COUNTER(load_success) \ - GAUGE (num_keys) + GAUGE (num_keys) \ + GAUGE (admin_overrides_active) // clang-format on /** @@ -55,13 +56,12 @@ struct RuntimeStats { }; /** - * Implementation of Snapshot whose source is the values map provided to its constructor. - * Used by LoaderImpl. + * Implementation of Snapshot whose source the vector of layers passed to the constructor. */ class SnapshotImpl : public Snapshot, public ThreadLocal::ThreadLocalObject { public: - SnapshotImpl(RandomGenerator& generator, - const std::unordered_map& values); + SnapshotImpl(RandomGenerator& generator, RuntimeStats& stats, + const std::vector& layers); // Runtime::Snapshot bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, @@ -72,29 +72,56 @@ class SnapshotImpl : public Snapshot, public ThreadLocal::ThreadLocalObject { const std::string& get(const std::string& key) const override; uint64_t getInteger(const std::string& key, uint64_t default_value) const override; const std::unordered_map& getAll() const override; + const std::vector& getAllLayers() const override; + + static Entry createEntry(const std::string& value); + +private: + const std::vector layers_; + std::unordered_map values_; + RandomGenerator& generator_; +}; + +/** + * Base implementation of OverrideLayer that by itself provides an empty values map. + */ +class OverrideLayerImpl : public Snapshot::OverrideLayer { +public: + explicit OverrideLayerImpl(const std::string& name) : name_{name} {} + const std::unordered_map& values() const override { + return values_; + } + const std::string& name() const override { return name_; } protected: - explicit SnapshotImpl(RandomGenerator& generator); + std::unordered_map values_; + const std::string name_; +}; - static void tryConvertToInteger(Snapshot::Entry& entry); +/** + * Extension of OverrideLayerImpl that maintains an in-memory set of values. These values can be + * modified programmatically via mergeValues(). AdminLayer is so named because it can be access and + * manipulated by Envoy's admin interface. + */ +class AdminLayer : public OverrideLayerImpl { +public: + explicit AdminLayer(RuntimeStats& stats) : OverrideLayerImpl{"admin"}, stats_(stats) {} + /** + * Merge the provided values into our entry map. An empty value indicates that a key should be + * removed from our map. + */ void mergeValues(const std::unordered_map& values); - std::unordered_map values_; - private: - RandomGenerator& generator_; + RuntimeStats& stats_; }; /** - * Extension of SnapshotImpl that uses the disk as its primary source of values and SnapshotImpl's - * in-memory values as overrides. Used by DiskBackedLoaderImpl. + * Extension of OverrideLayerImpl that loads values from the file system upon construction. */ -class DiskBackedSnapshotImpl : public SnapshotImpl, Logger::Loggable { +class DiskLayer : public OverrideLayerImpl, Logger::Loggable { public: - DiskBackedSnapshotImpl(RandomGenerator& generator, - const std::unordered_map& additional_overrides, - const std::string& root_path, const std::string& override_path, - RuntimeStats& stats, Api::OsSysCalls& os_sys_calls); + DiskLayer(const std::string& name, const std::string& path, Api::OsSysCalls& os_sys_calls); private: struct Directory { @@ -112,6 +139,7 @@ class DiskBackedSnapshotImpl : public SnapshotImpl, Logger::Loggable adminLayer_; RandomGenerator& generator_; - std::unordered_map values_; + RuntimeStats stats_; private: + RuntimeStats generateStats(Stats::Store& store); ThreadLocal::SlotPtr tls_; }; @@ -153,7 +183,7 @@ class LoaderImpl : public Loader { * Extension of LoaderImpl that watches a symlink for swapping and loads a specified subdirectory * from disk. Values added via mergeValues() are secondary to those loaded from disk. */ -class DiskBackedLoaderImpl : public LoaderImpl { +class DiskBackedLoaderImpl : public LoaderImpl, Logger::Loggable { public: DiskBackedLoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls, const std::string& root_symlink_path, const std::string& subdir, @@ -162,12 +192,10 @@ class DiskBackedLoaderImpl : public LoaderImpl { private: std::unique_ptr createNewSnapshot() override; - RuntimeStats generateStats(Stats::Store& store); Filesystem::WatcherPtr watcher_; std::string root_path_; std::string override_path_; - RuntimeStats stats_; Api::OsSysCallsPtr os_sys_calls_; }; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 0ce513d480b54..e692667260843 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -544,19 +544,50 @@ Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& res Buffer::Instance& response) { Http::Code rc = Http::Code::OK; const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); - const auto& entries = server_.runtime().snapshot().getAll(); - const auto pairs = sortedRuntime(entries); + const auto& layers = server_.runtime().snapshot().getAllLayers(); if (params.size() == 0) { - for (const auto& entry : pairs) { - response.add(fmt::format("{}: {}\n", entry.first, entry.second.string_value_)); + std::set keys; + static constexpr absl::string_view key_header = "KEY"; + + // Determine the maximum width for each column. + std::vector column_widths; + size_t key_column_width = key_header.size(); + for (const auto& layer : layers) { + column_widths.push_back(layer->name().size()); + for (const auto& kv : layer->values()) { + key_column_width = std::max(key_column_width, kv.first.size()); + column_widths.back() = std::max(column_widths.back(), kv.second.string_value_.size()); + keys.insert(kv.first); + } + } + + static const std::string cell_format = "{:<{}}\t"; + // Add header row. + response.add(fmt::format(cell_format, key_header, key_column_width)); + for (size_t i = 0; i < layers.size(); i++) { + response.add( + fmt::format(cell_format, StringUtil::toUpper(layers[i]->name()), column_widths[i])); + } + response.add("\n"); + + // Add a row for each key. + for (const auto& key : keys) { + response.add(fmt::format(cell_format, key, key_column_width)); + for (size_t i = 0; i < layers.size(); i++) { + const auto& layer_values = layers[i]->values(); + const auto value_it = layer_values.find(key); + const auto& value = value_it != layer_values.end() ? value_it->second.string_value_ : ""; + response.add(fmt::format(cell_format, value, column_widths[i])); + } + response.add("\n"); } } else { if (params.begin()->first == "format" && params.begin()->second == "json") { + // TODO: Reviewers, do we want to proto-ize this now? Or continue manually constructing JSON? + // Or skip JSON altogether? response_headers.insertContentType().value().setReference( Http::Headers::get().ContentTypeValues.Json); - response.add(runtimeAsJson(pairs)); - response.add("\n"); } else { response.add("usage: /runtime?format=json\n"); rc = Http::Code::BadRequest; @@ -581,20 +612,6 @@ Http::Code AdminImpl::handlerRuntimeModify(const std::string& url, Http::HeaderM return Http::Code::OK; } -const std::vector> AdminImpl::sortedRuntime( - const std::unordered_map& entries) { - std::vector> pairs(entries.begin(), - entries.end()); - - std::sort(pairs.begin(), pairs.end(), - [](const std::pair& a, - const std::pair& b) -> bool { - return a.first < b.first; - }); - - return pairs; -} - ConfigTracker& AdminImpl::getConfigTracker() { return config_tracker_; } std::string AdminImpl::runtimeAsJson( @@ -699,6 +716,7 @@ AdminImpl::AdminImpl(const std::string& access_log_path, const std::string& prof {"/runtime", "print runtime values", MAKE_ADMIN_HANDLER(handlerRuntime), false, false}, {"/runtime_modify", "modify runtime values", MAKE_ADMIN_HANDLER(handlerRuntimeModify), false, true}}, + // TODO(jsedgwick) add /runtime_reset endpoint that removes all admin-set values listener_(*this, std::move(listener_scope)) { if (!address_out_path.empty()) { diff --git a/source/server/server.cc b/source/server/server.cc index 24419af4607c4..95bcec89f2e02 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -294,7 +294,8 @@ Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server, config.runtime()->subdirectory(), override_subdirectory, server.stats(), server.random(), std::move(os_sys_calls))}; } else { - return Runtime::LoaderPtr{new Runtime::LoaderImpl(server.random(), server.threadLocal())}; + return Runtime::LoaderPtr{ + new Runtime::LoaderImpl(server.random(), server.stats(), server.threadLocal())}; } } diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index e91a2e634b124..7da99cf8026ba 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -231,7 +231,8 @@ TEST_F(DiskBackedLoaderImplTest, mergeValues) { TEST(LoaderImplTest, All) { MockRandomGenerator generator; NiceMock tls; - LoaderImpl loader(generator, tls); + Stats::IsolatedStoreImpl store; + LoaderImpl loader(generator, store, tls); EXPECT_EQ("", loader.snapshot().get("foo")); EXPECT_EQ(1UL, loader.snapshot().getInteger("foo", 1)); EXPECT_CALL(generator, random()).WillOnce(Return(49)); diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index 839bb597ab56b..328e20d17a5c9 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -35,6 +35,7 @@ class MockSnapshot : public Snapshot { 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_METHOD0(getAll, const std::unordered_map&()); + MOCK_CONST_METHOD0(getAllLayers, const std::vector&()); }; class MockLoader : public Loader { @@ -48,5 +49,11 @@ class MockLoader : public Loader { testing::NiceMock snapshot_; }; +class MockOverrideLayer : public Snapshot::OverrideLayer { +public: + MOCK_CONST_METHOD0(name, const std::string&()); + MOCK_CONST_METHOD0(values, const std::unordered_map&()); +}; + } // namespace Runtime } // namespace Envoy diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index a5d12825aaa86..acc987ec6f41d 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -234,63 +234,48 @@ TEST_P(AdminInstanceTest, Runtime) { Http::HeaderMapImpl header_map; Buffer::OwnedImpl response; - std::unordered_map entries{ - {"string_key", {"foo", {}}}, {"int_key", {"1", {1}}}, {"other_key", {"bar", {}}}}; Runtime::MockSnapshot snapshot; Runtime::MockLoader loader; + auto layer1 = std::make_shared>(); + auto layer2 = std::make_shared>(); + std::unordered_map entries1{ + {"string_key", {"foo", {}}}, {"int_key", {"1", {1}}}, {"other_key", {"bar", {}}}}; + std::unordered_map entries2{ + {"string_key", {"override", {}}}, {"extra_key", {"bar", {}}}}; - EXPECT_CALL(snapshot, getAll()).WillRepeatedly(testing::ReturnRef(entries)); - EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); - EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); + ON_CALL(*layer1, name()).WillByDefault(testing::ReturnRef("layer1")); + ON_CALL(*layer1, values()).WillByDefault(testing::ReturnRef(entries1)); + ON_CALL(*layer2, name()).WillByDefault(testing::ReturnRef("layer2")); + ON_CALL(*layer2, values()).WillByDefault(testing::ReturnRef(entries2)); - EXPECT_EQ(Http::Code::OK, admin_.runCallback("/runtime", header_map, response)); - EXPECT_EQ("int_key: 1\nother_key: bar\nstring_key: foo\n", TestUtility::bufferToString(response)); -} + std::vector layers{layer1, layer2}; + EXPECT_CALL(snapshot, getAllLayers()).WillRepeatedly(testing::ReturnRef(layers)); -TEST_P(AdminInstanceTest, RuntimeJSON) { - Http::HeaderMapImpl header_map; - Buffer::OwnedImpl response; - - std::unordered_map entries{ - {"string_key", {"foo", {}}}, {"int_key", {"1", {1}}}, {"other_key", {"bar", {}}}}; - Runtime::MockSnapshot snapshot; - Runtime::MockLoader loader; + const std::string expected_table = "KEY \tLAYER1\tLAYER2 \t\n" + "extra_key \t \tbar \t\n" + "int_key \t1 \t \t\n" + "other_key \tbar \t \t\n" + "string_key\tfoo \toverride\t\n"; - EXPECT_CALL(snapshot, getAll()).WillRepeatedly(testing::ReturnRef(entries)); EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); + EXPECT_EQ(Http::Code::OK, admin_.runCallback("/runtime", header_map, response)); + EXPECT_EQ(expected_table, TestUtility::bufferToString(response)); +} - EXPECT_EQ(Http::Code::OK, admin_.runCallback("/runtime?format=json", header_map, response)); - - std::string output = TestUtility::bufferToString(response); - Json::ObjectSharedPtr json = Json::Factory::loadFromString(output); - - EXPECT_TRUE(json->hasObject("runtime")); - std::vector pairs = json->getObjectArray("runtime"); - EXPECT_EQ(3, pairs.size()); - - Json::ObjectSharedPtr pair = pairs[0]; - EXPECT_EQ("int_key", pair->getString("name", "")); - EXPECT_EQ(1, pair->getInteger("value", -1)); - - pair = pairs[1]; - EXPECT_EQ("other_key", pair->getString("name", "")); - EXPECT_EQ("bar", pair->getString("value", "")); - - pair = pairs[2]; - EXPECT_EQ("string_key", pair->getString("name", "")); - EXPECT_EQ("foo", pair->getString("value", "")); +TEST_P(AdminInstanceTest, RuntimeJSON) { + // TODO Reviewers, see comment in admin.cc about JSON output } TEST_P(AdminInstanceTest, RuntimeBadFormat) { Http::HeaderMapImpl header_map; Buffer::OwnedImpl response; - std::unordered_map entries; + std::vector layers; Runtime::MockSnapshot snapshot; Runtime::MockLoader loader; - EXPECT_CALL(snapshot, getAll()).WillRepeatedly(testing::ReturnRef(entries)); + EXPECT_CALL(snapshot, getAllLayers()).WillRepeatedly(testing::ReturnRef(layers)); EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); @@ -303,12 +288,7 @@ TEST_P(AdminInstanceTest, RuntimeModify) { Http::HeaderMapImpl header_map; Buffer::OwnedImpl response; - std::unordered_map entries; - Runtime::MockSnapshot snapshot; Runtime::MockLoader loader; - - EXPECT_CALL(snapshot, getAll()).WillRepeatedly(testing::ReturnRef(entries)); - EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); std::unordered_map overrides; From fdf40dc7c60ceb4984f5833dd87e41c16624588e Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Tue, 27 Mar 2018 18:53:24 -0700 Subject: [PATCH 03/10] merge Signed-off-by: James Sedgwick --- source/server/http/admin.cc | 2 +- source/server/http/admin.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index e692667260843..1e416672a379f 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -597,7 +597,7 @@ Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& res return rc; } -Http::Code AdminImpl::handlerRuntimeModify(const std::string& url, Http::HeaderMap&, +Http::Code AdminImpl::handlerRuntimeModify(absl::string_view url, Http::HeaderMap&, Buffer::Instance& response) { const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); if (params.empty()) { diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 75c910eda0972..c8972651529dd 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -181,7 +181,7 @@ class AdminImpl : public Admin, Http::HeaderMap& response_headers, Buffer::Instance& response); Http::Code handlerRuntime(absl::string_view path_and_query, Http::HeaderMap& response_headers, Buffer::Instance& response); - Http::Code handlerRuntimeModify(const std::string& path_and_query, + Http::Code handlerRuntimeModify(absl::string_view path_and_query, Http::HeaderMap& response_headers, Buffer::Instance& response); class AdminListener : public Network::ListenerConfig { From 9c0c779707f8ef625045c86bff802fc6495a93b1 Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Wed, 28 Mar 2018 08:45:12 -0700 Subject: [PATCH 04/10] fix opt build Signed-off-by: James Sedgwick --- test/server/http/admin_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index acc987ec6f41d..3a76a6854fbe7 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -243,9 +243,9 @@ TEST_P(AdminInstanceTest, Runtime) { std::unordered_map entries2{ {"string_key", {"override", {}}}, {"extra_key", {"bar", {}}}}; - ON_CALL(*layer1, name()).WillByDefault(testing::ReturnRef("layer1")); + ON_CALL(*layer1, name()).WillByDefault(testing::ReturnRefOfCopy(std::string{"layer1"})); ON_CALL(*layer1, values()).WillByDefault(testing::ReturnRef(entries1)); - ON_CALL(*layer2, name()).WillByDefault(testing::ReturnRef("layer2")); + ON_CALL(*layer2, name()).WillByDefault(testing::ReturnRefOfCopy(std::string{"layer2"})); ON_CALL(*layer2, values()).WillByDefault(testing::ReturnRef(entries2)); std::vector layers{layer1, layer2}; From 5952eb12f08a8e9b3a6ea802f27f2c3c1288f767 Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Thu, 29 Mar 2018 18:11:28 -0700 Subject: [PATCH 05/10] json output Signed-off-by: James Sedgwick --- source/server/http/admin.cc | 111 +++++++++++++++------------------ test/server/http/admin_test.cc | 52 ++++++++------- 2 files changed, 77 insertions(+), 86 deletions(-) diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 1e416672a379f..9d74d47bb79aa 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -542,78 +542,54 @@ Http::Code AdminImpl::handlerCerts(absl::string_view, Http::HeaderMap&, Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& response_headers, Buffer::Instance& response) { - Http::Code rc = Http::Code::OK; const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); + response_headers.insertContentType().value().setReference( + Http::Headers::get().ContentTypeValues.Json); + + rapidjson::Document document; + document.SetObject(); + auto& allocator = document.GetAllocator(); const auto& layers = server_.runtime().snapshot().getAllLayers(); - if (params.size() == 0) { - std::set keys; - static constexpr absl::string_view key_header = "KEY"; - - // Determine the maximum width for each column. - std::vector column_widths; - size_t key_column_width = key_header.size(); - for (const auto& layer : layers) { - column_widths.push_back(layer->name().size()); - for (const auto& kv : layer->values()) { - key_column_width = std::max(key_column_width, kv.first.size()); - column_widths.back() = std::max(column_widths.back(), kv.second.string_value_.size()); - keys.insert(kv.first); - } - } + std::map value_arrays; - static const std::string cell_format = "{:<{}}\t"; - // Add header row. - response.add(fmt::format(cell_format, key_header, key_column_width)); - for (size_t i = 0; i < layers.size(); i++) { - response.add( - fmt::format(cell_format, StringUtil::toUpper(layers[i]->name()), column_widths[i])); - } - response.add("\n"); + rapidjson::Value layer_names{rapidjson::kArrayType}; + for (const auto& layer : layers) { + rapidjson::Value layer_name; + layer_name.SetString(layer->name().c_str(), allocator); + layer_names.PushBack(layer_name, allocator); - // Add a row for each key. - for (const auto& key : keys) { - response.add(fmt::format(cell_format, key, key_column_width)); - for (size_t i = 0; i < layers.size(); i++) { - const auto& layer_values = layers[i]->values(); - const auto value_it = layer_values.find(key); - const auto& value = value_it != layer_values.end() ? value_it->second.string_value_ : ""; - response.add(fmt::format(cell_format, value, column_widths[i])); - } - response.add("\n"); - } - } else { - if (params.begin()->first == "format" && params.begin()->second == "json") { - // TODO: Reviewers, do we want to proto-ize this now? Or continue manually constructing JSON? - // Or skip JSON altogether? - response_headers.insertContentType().value().setReference( - Http::Headers::get().ContentTypeValues.Json); - } else { - response.add("usage: /runtime?format=json\n"); - rc = Http::Code::BadRequest; + for (const auto& kv : layer->values()) { + value_arrays.emplace(kv.first, rapidjson::Value{rapidjson::kArrayType}); } } + document.AddMember("layers", layer_names, allocator); - return rc; -} + for (const auto& layer : layers) { + for (auto& kv : value_arrays) { + const auto it = layer->values().find(kv.first); + const auto& value = it == layer->values().end() ? "" : it->second.string_value_; + rapidjson::Value value_obj; + value_obj.SetString(value.c_str(), allocator); + kv.second.PushBack(value_obj, allocator); + } + } -Http::Code AdminImpl::handlerRuntimeModify(absl::string_view url, Http::HeaderMap&, - Buffer::Instance& response) { - const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); - if (params.empty()) { - response.add("usage: /runtime_modify?key1=value1&key2=value2&keyN=valueN\n"); - response.add("use an empty value to remove a previously added override"); - return Http::Code::BadRequest; + rapidjson::Value value_arrays_obj{rapidjson::kObjectType}; + for (auto& kv : value_arrays) { + value_arrays_obj.AddMember(rapidjson::StringRef(kv.first.c_str()), std::move(kv.second), + allocator); } - std::unordered_map overrides; - overrides.insert(params.begin(), params.end()); - server_.runtime().mergeValues(overrides); - response.add("OK\n"); + + document.AddMember("entries", std::move(value_arrays_obj), allocator); + + rapidjson::StringBuffer strbuf; + rapidjson::PrettyWriter writer(strbuf); + document.Accept(writer); + response.add(strbuf.GetString()); return Http::Code::OK; } -ConfigTracker& AdminImpl::getConfigTracker() { return config_tracker_; } - std::string AdminImpl::runtimeAsJson( const std::vector>& entries) { rapidjson::Document document; @@ -643,6 +619,23 @@ std::string AdminImpl::runtimeAsJson( return strbuf.GetString(); } +Http::Code AdminImpl::handlerRuntimeModify(absl::string_view url, Http::HeaderMap&, + Buffer::Instance& response) { + const Http::Utility::QueryParams params = Http::Utility::parseQueryString(url); + if (params.empty()) { + response.add("usage: /runtime_modify?key1=value1&key2=value2&keyN=valueN\n"); + response.add("use an empty value to remove a previously added override"); + return Http::Code::BadRequest; + } + std::unordered_map overrides; + overrides.insert(params.begin(), params.end()); + server_.runtime().mergeValues(overrides); + response.add("OK\n"); + return Http::Code::OK; +} + +ConfigTracker& AdminImpl::getConfigTracker() { return config_tracker_; } + void AdminFilter::onComplete() { absl::string_view path = request_headers_->Path()->value().getStringView(); ENVOY_STREAM_LOG(debug, "request complete: path: {}", *callbacks_, path); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 3a76a6854fbe7..a2a31ea4f7d12 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -251,37 +251,35 @@ TEST_P(AdminInstanceTest, Runtime) { std::vector layers{layer1, layer2}; EXPECT_CALL(snapshot, getAllLayers()).WillRepeatedly(testing::ReturnRef(layers)); - const std::string expected_table = "KEY \tLAYER1\tLAYER2 \t\n" - "extra_key \t \tbar \t\n" - "int_key \t1 \t \t\n" - "other_key \tbar \t \t\n" - "string_key\tfoo \toverride\t\n"; + const std::string expected_json = R"EOF({ + "layers": [ + "layer1", + "layer2" + ], + "entries": { + "extra_key": [ + "", + "bar" + ], + "int_key": [ + "1", + "" + ], + "other_key": [ + "bar", + "" + ], + "string_key": [ + "foo", + "override" + ] + } +})EOF"; EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); EXPECT_EQ(Http::Code::OK, admin_.runCallback("/runtime", header_map, response)); - EXPECT_EQ(expected_table, TestUtility::bufferToString(response)); -} - -TEST_P(AdminInstanceTest, RuntimeJSON) { - // TODO Reviewers, see comment in admin.cc about JSON output -} - -TEST_P(AdminInstanceTest, RuntimeBadFormat) { - Http::HeaderMapImpl header_map; - Buffer::OwnedImpl response; - - std::vector layers; - Runtime::MockSnapshot snapshot; - Runtime::MockLoader loader; - - EXPECT_CALL(snapshot, getAllLayers()).WillRepeatedly(testing::ReturnRef(layers)); - EXPECT_CALL(loader, snapshot()).WillRepeatedly(testing::ReturnPointee(&snapshot)); - EXPECT_CALL(server_, runtime()).WillRepeatedly(testing::ReturnPointee(&loader)); - - EXPECT_EQ(Http::Code::BadRequest, - admin_.runCallback("/runtime?format=foo", header_map, response)); - EXPECT_EQ("usage: /runtime?format=json\n", TestUtility::bufferToString(response)); + EXPECT_EQ(expected_json, TestUtility::bufferToString(response)); } TEST_P(AdminInstanceTest, RuntimeModify) { From c248042f358c831e9eeabe4faaf439f531113206 Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Thu, 29 Mar 2018 18:24:40 -0700 Subject: [PATCH 06/10] address comments Signed-off-by: James Sedgwick --- include/envoy/runtime/runtime.h | 13 +++--------- source/common/runtime/runtime_impl.cc | 25 +++++++++------------- source/common/runtime/runtime_impl.h | 10 ++++----- source/server/http/admin.cc | 11 +++++----- test/common/runtime/runtime_impl_test.cc | 27 +----------------------- test/mocks/runtime/mocks.h | 3 +-- test/server/http/admin_test.cc | 4 ++-- 7 files changed, 27 insertions(+), 66 deletions(-) diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index 51232c244f3de..22ac284750ffe 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -72,7 +72,7 @@ class Snapshot { virtual const std::string& name() const PURE; }; - typedef std::shared_ptr OverrideLayerSharedPtr; + typedef std::shared_ptr OverrideLayerConstSharedPtr; /** * Test if a feature is enabled using the built in random generator. This is done by generating @@ -134,21 +134,14 @@ class Snapshot { */ virtual uint64_t getInteger(const std::string& key, uint64_t default_value) const PURE; - /** - * TODO: Reviewers, shall I delete getAll()? It's no longer used. - * Fetch the raw runtime entries map. The map data is safe only for the lifetime of the Snapshot. - * @return const std::unordered_map& the raw map of loaded values. - */ - virtual const std::unordered_map& getAll() 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. * Any layer can add a key in addition to overriding keys in layers below. The layer vector is * safe only for the lifetime of the Snapshot. - * @return const std::unordered_map& the raw map of loaded values. + * @return const std::vector& the raw map of loaded values. */ - virtual const std::vector& getAllLayers() const PURE; + virtual const std::vector& getLayers() const PURE; }; /** diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 39f57e18e0b0e..bd60a8826eb48 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -186,16 +186,12 @@ uint64_t SnapshotImpl::getInteger(const std::string& key, uint64_t default_value } } -const std::unordered_map& SnapshotImpl::getAll() const { - return values_; -} - -const std::vector& SnapshotImpl::getAllLayers() const { +const std::vector& SnapshotImpl::getLayers() const { return layers_; } SnapshotImpl::SnapshotImpl(RandomGenerator& generator, RuntimeStats& stats, - const std::vector& layers) + std::vector&& layers) : layers_{layers}, generator_{generator} { for (const auto& layer : layers_) { for (const auto& kv : layer->values()) { @@ -302,13 +298,12 @@ LoaderImpl::LoaderImpl(RandomGenerator& generator, Stats::Store& store, LoaderImpl::LoaderImpl(DoNotLoadSnapshot /* unused */, RandomGenerator& generator, Stats::Store& store, ThreadLocal::SlotAllocator& tls) - : generator_(generator), stats_(generateStats(store)), tls_(tls.allocateSlot()) { - adminLayer_ = std::make_shared(stats_); -} + : generator_(generator), stats_(generateStats(store)), + admin_layer_(std::make_shared(stats_)), tls_(tls.allocateSlot()) {} std::unique_ptr LoaderImpl::createNewSnapshot() { - return std::make_unique(generator_, stats_, - std::vector{adminLayer_}); + return std::make_unique( + generator_, stats_, std::vector{admin_layer_}); } void LoaderImpl::loadNewSnapshot() { @@ -321,7 +316,7 @@ void LoaderImpl::loadNewSnapshot() { Snapshot& LoaderImpl::snapshot() { return tls_->getTyped(); } void LoaderImpl::mergeValues(const std::unordered_map& values) { - adminLayer_->mergeValues(values); + admin_layer_->mergeValues(values); loadNewSnapshot(); } @@ -350,7 +345,7 @@ RuntimeStats LoaderImpl::generateStats(Stats::Store& store) { } std::unique_ptr DiskBackedLoaderImpl::createNewSnapshot() { - std::vector layers; + std::vector layers; try { layers.push_back(std::make_unique("root", root_path_, *os_sys_calls_)); if (Filesystem::directoryExists(override_path_)) { @@ -363,8 +358,8 @@ std::unique_ptr DiskBackedLoaderImpl::createNewSnapshot() { stats_.load_error_.inc(); ENVOY_LOG(debug, "error creating runtime snapshot: {}", e.what()); } - layers.push_back(adminLayer_); - return std::make_unique(generator_, stats_, layers); + layers.push_back(admin_layer_); + return std::make_unique(generator_, stats_, std::move(layers)); } } // namespace Runtime diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 239155d2590d2..7fe79b18ab9b4 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -47,6 +47,7 @@ class RandomGeneratorImpl : public RandomGenerator { GAUGE (num_keys) \ GAUGE (admin_overrides_active) // clang-format on +// TODO XXX TESTME num_keys /** * Struct definition for all runtime stats. @see stats_macros.h @@ -61,7 +62,7 @@ struct RuntimeStats { class SnapshotImpl : public Snapshot, public ThreadLocal::ThreadLocalObject { public: SnapshotImpl(RandomGenerator& generator, RuntimeStats& stats, - const std::vector& layers); + std::vector&& layers); // Runtime::Snapshot bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, @@ -71,13 +72,12 @@ class SnapshotImpl : public Snapshot, public ThreadLocal::ThreadLocalObject { 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; - const std::unordered_map& getAll() const override; - const std::vector& getAllLayers() const override; + const std::vector& getLayers() const override; static Entry createEntry(const std::string& value); private: - const std::vector layers_; + const std::vector layers_; std::unordered_map values_; RandomGenerator& generator_; }; @@ -170,9 +170,9 @@ class LoaderImpl : public Loader { // Load a new Snapshot into TLS void loadNewSnapshot(); - std::shared_ptr adminLayer_; RandomGenerator& generator_; RuntimeStats stats_; + std::shared_ptr admin_layer_; private: RuntimeStats generateStats(Stats::Store& store); diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 9d74d47bb79aa..f9458b3ca3ffe 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -549,21 +549,20 @@ Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& res rapidjson::Document document; document.SetObject(); auto& allocator = document.GetAllocator(); - - const auto& layers = server_.runtime().snapshot().getAllLayers(); std::map value_arrays; - rapidjson::Value layer_names{rapidjson::kArrayType}; + const auto& layers = server_.runtime().snapshot().getLayers(); + for (const auto& layer : layers) { rapidjson::Value layer_name; layer_name.SetString(layer->name().c_str(), allocator); - layer_names.PushBack(layer_name, allocator); + layer_names.PushBack(std::move(layer_name), allocator); for (const auto& kv : layer->values()) { value_arrays.emplace(kv.first, rapidjson::Value{rapidjson::kArrayType}); } } - document.AddMember("layers", layer_names, allocator); + document.AddMember("layers", std::move(layer_names), allocator); for (const auto& layer : layers) { for (auto& kv : value_arrays) { @@ -571,7 +570,7 @@ Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& res const auto& value = it == layer->values().end() ? "" : it->second.string_value_; rapidjson::Value value_obj; value_obj.SetString(value.c_str(), allocator); - kv.second.PushBack(value_obj, allocator); + kv.second.PushBack(std::move(value_obj), allocator); } } diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 7da99cf8026ba..707a6be941697 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -135,31 +135,7 @@ TEST_F(DiskBackedLoaderImplTest, All) { EXPECT_EQ("hello override", loader->snapshot().get("file1")); } -TEST_F(DiskBackedLoaderImplTest, GetAll) { - setup(); - run("test/common/runtime/test_data/current", "envoy_override"); - - auto values = loader->snapshot().getAll(); - - auto entry = values.find("file1"); - EXPECT_FALSE(entry == values.end()); - EXPECT_EQ("hello override", entry->second.string_value_); - EXPECT_FALSE(entry->second.uint_value_); - - entry = values.find("file2"); - EXPECT_FALSE(entry == values.end()); - EXPECT_EQ("world", entry->second.string_value_); - EXPECT_FALSE(entry->second.uint_value_); - - entry = values.find("file3"); - EXPECT_FALSE(entry == values.end()); - EXPECT_EQ("2", entry->second.string_value_); - EXPECT_TRUE(entry->second.uint_value_); - EXPECT_EQ(2UL, entry->second.uint_value_.value()); - - entry = values.find("invalid"); - EXPECT_TRUE(entry == values.end()); -} +// TODO XXX getLayers() test TEST_F(DiskBackedLoaderImplTest, BadDirectory) { setup(); @@ -237,7 +213,6 @@ TEST(LoaderImplTest, All) { EXPECT_EQ(1UL, loader.snapshot().getInteger("foo", 1)); EXPECT_CALL(generator, random()).WillOnce(Return(49)); EXPECT_TRUE(loader.snapshot().featureEnabled("foo", 50)); - EXPECT_TRUE(loader.snapshot().getAll().empty()); testNewOverrides(loader); } diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index 328e20d17a5c9..5267f2841859a 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -34,8 +34,7 @@ class MockSnapshot : public Snapshot { uint64_t random_value, uint64_t num_buckets)); 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_METHOD0(getAll, const std::unordered_map&()); - MOCK_CONST_METHOD0(getAllLayers, const std::vector&()); + MOCK_CONST_METHOD0(getLayers, const std::vector&()); }; class MockLoader : public Loader { diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index a2a31ea4f7d12..d528579db17cc 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -248,8 +248,8 @@ TEST_P(AdminInstanceTest, Runtime) { ON_CALL(*layer2, name()).WillByDefault(testing::ReturnRefOfCopy(std::string{"layer2"})); ON_CALL(*layer2, values()).WillByDefault(testing::ReturnRef(entries2)); - std::vector layers{layer1, layer2}; - EXPECT_CALL(snapshot, getAllLayers()).WillRepeatedly(testing::ReturnRef(layers)); + std::vector layers{layer1, layer2}; + EXPECT_CALL(snapshot, getLayers()).WillRepeatedly(testing::ReturnRef(layers)); const std::string expected_json = R"EOF({ "layers": [ From f9635c5f0c4cc88b3e18edec58b354fa00f1adff Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Fri, 30 Mar 2018 16:33:19 -0700 Subject: [PATCH 07/10] update Signed-off-by: James Sedgwick --- include/envoy/runtime/runtime.h | 6 ++-- source/common/runtime/runtime_impl.cc | 24 ++++++++------- source/common/runtime/runtime_impl.h | 29 +++++++++++-------- test/common/runtime/runtime_impl_test.cc | 37 +++++++++++++++++++++--- test/mocks/runtime/mocks.h | 2 +- test/server/http/admin_test.cc | 8 +++-- 6 files changed, 72 insertions(+), 34 deletions(-) diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index 22ac284750ffe..c85d7033a7409 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -72,7 +72,7 @@ class Snapshot { virtual const std::string& name() const PURE; }; - typedef std::shared_ptr OverrideLayerConstSharedPtr; + typedef std::unique_ptr OverrideLayerConstPtr; /** * Test if a feature is enabled using the built in random generator. This is done by generating @@ -139,9 +139,9 @@ class Snapshot { * to top; for instance, the second layer's entries override the first layer's entries, and so on. * Any layer can add a key in addition to overriding keys in layers below. The layer vector is * safe only for the lifetime of the Snapshot. - * @return const std::vector& the raw map of loaded values. + * @return const std::vector& the raw map of loaded values. */ - virtual const std::vector& getLayers() const PURE; + virtual const std::vector& getLayers() const PURE; }; /** diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index bd60a8826eb48..ddd5d470e3c1e 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -186,13 +186,13 @@ uint64_t SnapshotImpl::getInteger(const std::string& key, uint64_t default_value } } -const std::vector& SnapshotImpl::getLayers() const { +const std::vector& SnapshotImpl::getLayers() const { return layers_; } SnapshotImpl::SnapshotImpl(RandomGenerator& generator, RuntimeStats& stats, - std::vector&& layers) - : layers_{layers}, generator_{generator} { + std::vector&& layers) + : layers_{std::move(layers)}, generator_{generator} { for (const auto& layer : layers_) { for (const auto& kv : layer->values()) { values_.erase(kv.first); @@ -298,12 +298,13 @@ LoaderImpl::LoaderImpl(RandomGenerator& generator, Stats::Store& store, LoaderImpl::LoaderImpl(DoNotLoadSnapshot /* unused */, RandomGenerator& generator, Stats::Store& store, ThreadLocal::SlotAllocator& tls) - : generator_(generator), stats_(generateStats(store)), - admin_layer_(std::make_shared(stats_)), tls_(tls.allocateSlot()) {} + : generator_(generator), stats_(generateStats(store)), admin_layer_(stats_), + tls_(tls.allocateSlot()) {} std::unique_ptr LoaderImpl::createNewSnapshot() { - return std::make_unique( - generator_, stats_, std::vector{admin_layer_}); + std::vector layers; + layers.emplace_back(std::make_unique(admin_layer_)); + return std::make_unique(generator_, stats_, std::move(layers)); } void LoaderImpl::loadNewSnapshot() { @@ -316,7 +317,7 @@ void LoaderImpl::loadNewSnapshot() { Snapshot& LoaderImpl::snapshot() { return tls_->getTyped(); } void LoaderImpl::mergeValues(const std::unordered_map& values) { - admin_layer_->mergeValues(values); + admin_layer_.mergeValues(values); loadNewSnapshot(); } @@ -345,7 +346,7 @@ RuntimeStats LoaderImpl::generateStats(Stats::Store& store) { } std::unique_ptr DiskBackedLoaderImpl::createNewSnapshot() { - std::vector layers; + std::vector layers; try { layers.push_back(std::make_unique("root", root_path_, *os_sys_calls_)); if (Filesystem::directoryExists(override_path_)) { @@ -355,10 +356,11 @@ std::unique_ptr DiskBackedLoaderImpl::createNewSnapshot() { stats_.override_dir_not_exists_.inc(); } } catch (EnvoyException& e) { + layers.clear(); stats_.load_error_.inc(); - ENVOY_LOG(debug, "error creating runtime snapshot: {}", e.what()); + ENVOY_LOG(debug, "error loading runtime values from disk: {}", e.what()); } - layers.push_back(admin_layer_); + layers.push_back(std::make_unique(admin_layer_)); return std::make_unique(generator_, stats_, std::move(layers)); } diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 7fe79b18ab9b4..6fae1ae7d619a 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -57,12 +57,12 @@ struct RuntimeStats { }; /** - * Implementation of Snapshot whose source the vector of layers passed to the constructor. + * Implementation of Snapshot whose source is the vector of layers passed to the constructor. */ class SnapshotImpl : public Snapshot, public ThreadLocal::ThreadLocalObject { public: SnapshotImpl(RandomGenerator& generator, RuntimeStats& stats, - std::vector&& layers); + std::vector&& layers); // Runtime::Snapshot bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, @@ -72,12 +72,12 @@ class SnapshotImpl : public Snapshot, public ThreadLocal::ThreadLocalObject { 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; - const std::vector& getLayers() const override; + const std::vector& getLayers() const override; static Entry createEntry(const std::string& value); private: - const std::vector layers_; + const std::vector layers_; std::unordered_map values_; RandomGenerator& generator_; }; @@ -100,12 +100,17 @@ class OverrideLayerImpl : public Snapshot::OverrideLayer { /** * Extension of OverrideLayerImpl that maintains an in-memory set of values. These values can be - * modified programmatically via mergeValues(). AdminLayer is so named because it can be access and - * manipulated by Envoy's admin interface. + * modified programmatically via mergeValues(). AdminLayer is so named because it can be accessed + * and manipulated by Envoy's admin interface. */ class AdminLayer : public OverrideLayerImpl { public: - explicit AdminLayer(RuntimeStats& stats) : OverrideLayerImpl{"admin"}, stats_(stats) {} + explicit AdminLayer(RuntimeStats& stats) : OverrideLayerImpl{"admin"}, stats_{stats} {} + /** + * Copy-constructible so that it can snapshotted. + */ + AdminLayer(const AdminLayer& o) : AdminLayer{o.stats_} { values_ = o.values(); } + /** * Merge the provided values into our entry map. An empty value indicates that a key should be * removed from our map. @@ -172,7 +177,7 @@ class LoaderImpl : public Loader { RandomGenerator& generator_; RuntimeStats stats_; - std::shared_ptr admin_layer_; + AdminLayer admin_layer_; private: RuntimeStats generateStats(Stats::Store& store); @@ -193,10 +198,10 @@ class DiskBackedLoaderImpl : public LoaderImpl, Logger::Loggable createNewSnapshot() override; - Filesystem::WatcherPtr watcher_; - std::string root_path_; - std::string override_path_; - Api::OsSysCallsPtr os_sys_calls_; + const Filesystem::WatcherPtr watcher_; + const std::string root_path_; + const std::string override_path_; + const Api::OsSysCallsPtr os_sys_calls_; }; } // namespace Runtime diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 707a6be941697..c772a86de9bd8 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -135,7 +135,22 @@ TEST_F(DiskBackedLoaderImplTest, All) { EXPECT_EQ("hello override", loader->snapshot().get("file1")); } -// TODO XXX getLayers() test +TEST_F(DiskBackedLoaderImplTest, GetLayers) { + setup(); + run("test/common/runtime/test_data/current", "envoy_override"); + const auto& layers = loader->snapshot().getLayers(); + EXPECT_EQ(3, layers.size()); + EXPECT_EQ("hello", layers[0]->values().find("file1")->second.string_value_); + EXPECT_EQ("hello override", layers[1]->values().find("file1")->second.string_value_); + // Admin should be last + EXPECT_NE(nullptr, dynamic_cast(layers.back().get())); + EXPECT_TRUE(layers[2]->values().empty()); + + loader->mergeValues({{"foo", "bar"}}); + // The old snapshot and its layers should have been invalidated. Refetch. + const auto& new_layers = loader->snapshot().getLayers(); + EXPECT_EQ("bar", new_layers[2]->values().find("foo")->second.string_value_); +} TEST_F(DiskBackedLoaderImplTest, BadDirectory) { setup(); @@ -147,6 +162,10 @@ TEST_F(DiskBackedLoaderImplTest, BadStat) { EXPECT_CALL(*os_sys_calls_, stat(_, _)).WillOnce(Return(-1)); run("test/common/runtime/test_data/current", "envoy_override"); EXPECT_EQ(store.counter("runtime.load_error").value(), 1); + // We should still have the admin layer + const auto& layers = loader->snapshot().getLayers(); + EXPECT_EQ(1, layers.size()); + EXPECT_NE(nullptr, dynamic_cast(layers.back().get())); } TEST_F(DiskBackedLoaderImplTest, OverrideFolderDoesNotExist) { @@ -156,52 +175,62 @@ TEST_F(DiskBackedLoaderImplTest, OverrideFolderDoesNotExist) { EXPECT_EQ("hello", loader->snapshot().get("file1")); } -void testNewOverrides(Loader& loader) { +void testNewOverrides(Loader& loader, Stats::Store& store) { // New string loader.mergeValues({{"foo", "bar"}}); EXPECT_EQ("bar", loader.snapshot().get("foo")); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove new string loader.mergeValues({{"foo", ""}}); EXPECT_EQ("", loader.snapshot().get("foo")); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); // New integer loader.mergeValues({{"baz", "42"}}); EXPECT_EQ(42, loader.snapshot().getInteger("baz", 0)); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove new integer loader.mergeValues({{"baz", ""}}); EXPECT_EQ(0, loader.snapshot().getInteger("baz", 0)); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); } TEST_F(DiskBackedLoaderImplTest, mergeValues) { setup(); run("test/common/runtime/test_data/current", "envoy_override"); - testNewOverrides(*loader); + testNewOverrides(*loader, store); // Override string loader->mergeValues({{"file2", "new world"}}); EXPECT_EQ("new world", loader->snapshot().get("file2")); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove overridden string loader->mergeValues({{"file2", ""}}); EXPECT_EQ("world", loader->snapshot().get("file2")); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); // Override integer loader->mergeValues({{"file3", "42"}}); EXPECT_EQ(42, loader->snapshot().getInteger("file3", 1)); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove overridden integer loader->mergeValues({{"file3", ""}}); EXPECT_EQ(2, loader->snapshot().getInteger("file3", 1)); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); // Override override string loader->mergeValues({{"file1", "hello overridden override"}}); EXPECT_EQ("hello overridden override", loader->snapshot().get("file1")); + EXPECT_EQ(1, store.gauge("runtime.admin_overrides_active").value()); // Remove overridden override string loader->mergeValues({{"file1", ""}}); EXPECT_EQ("hello override", loader->snapshot().get("file1")); + EXPECT_EQ(0, store.gauge("runtime.admin_overrides_active").value()); } TEST(LoaderImplTest, All) { @@ -213,7 +242,7 @@ TEST(LoaderImplTest, All) { EXPECT_EQ(1UL, loader.snapshot().getInteger("foo", 1)); EXPECT_CALL(generator, random()).WillOnce(Return(49)); EXPECT_TRUE(loader.snapshot().featureEnabled("foo", 50)); - testNewOverrides(loader); + testNewOverrides(loader, store); } } // namespace Runtime diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index 5267f2841859a..7b2292da4d7ab 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -34,7 +34,7 @@ class MockSnapshot : public Snapshot { uint64_t random_value, uint64_t num_buckets)); 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_METHOD0(getLayers, const std::vector&()); + MOCK_CONST_METHOD0(getLayers, const std::vector&()); }; class MockLoader : public Loader { diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index d528579db17cc..facd81f253c14 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -236,8 +236,8 @@ TEST_P(AdminInstanceTest, Runtime) { Runtime::MockSnapshot snapshot; Runtime::MockLoader loader; - auto layer1 = std::make_shared>(); - auto layer2 = std::make_shared>(); + auto layer1 = std::make_unique>(); + auto layer2 = std::make_unique>(); std::unordered_map entries1{ {"string_key", {"foo", {}}}, {"int_key", {"1", {1}}}, {"other_key", {"bar", {}}}}; std::unordered_map entries2{ @@ -248,7 +248,9 @@ TEST_P(AdminInstanceTest, Runtime) { ON_CALL(*layer2, name()).WillByDefault(testing::ReturnRefOfCopy(std::string{"layer2"})); ON_CALL(*layer2, values()).WillByDefault(testing::ReturnRef(entries2)); - std::vector layers{layer1, layer2}; + std::vector layers; + layers.push_back(std::move(layer1)); + layers.push_back(std::move(layer2)); EXPECT_CALL(snapshot, getLayers()).WillRepeatedly(testing::ReturnRef(layers)); const std::string expected_json = R"EOF({ From eaf7a66814590fd5bda5bbddbf746061df7b74e3 Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Fri, 30 Mar 2018 16:43:08 -0700 Subject: [PATCH 08/10] remove todo Signed-off-by: James Sedgwick --- source/common/runtime/runtime_impl.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 6fae1ae7d619a..56ca69e88dd0b 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -47,7 +47,6 @@ class RandomGeneratorImpl : public RandomGenerator { GAUGE (num_keys) \ GAUGE (admin_overrides_active) // clang-format on -// TODO XXX TESTME num_keys /** * Struct definition for all runtime stats. @see stats_macros.h From fbb8717690aa5dd225586d3b2d0842bc738fb45a Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Sat, 31 Mar 2018 10:38:51 -0700 Subject: [PATCH 09/10] fix integration test Signed-off-by: James Sedgwick --- test/integration/integration_admin_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index a7b419f5333fa..1e001dc6cfb73 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -238,7 +238,7 @@ TEST_P(IntegrationAdminTest, Admin) { downstreamProtocol(), version_); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - EXPECT_STREQ("text/plain; charset=UTF-8", ContentType(response)); + EXPECT_STREQ("application/json", ContentType(response)); response = IntegrationUtil::makeSingleRequest(lookupPort("admin"), "GET", "/runtime?format=json", "", downstreamProtocol(), version_); From 2c4a1f22457a0341daeaaac67782d5df05b91f19 Mon Sep 17 00:00:00 2001 From: James Sedgwick Date: Mon, 2 Apr 2018 14:09:21 -0700 Subject: [PATCH 10/10] address more comments Signed-off-by: James Sedgwick --- source/common/runtime/runtime_impl.h | 5 +++- source/server/http/admin.cc | 28 ++++++++++++------ source/server/server.cc | 8 ++--- test/server/http/admin_test.cc | 44 ++++++++++++++++++---------- 4 files changed, 55 insertions(+), 30 deletions(-) diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 56ca69e88dd0b..c04e299811151 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -108,7 +108,9 @@ class AdminLayer : public OverrideLayerImpl { /** * Copy-constructible so that it can snapshotted. */ - AdminLayer(const AdminLayer& o) : AdminLayer{o.stats_} { values_ = o.values(); } + AdminLayer(const AdminLayer& admin_layer) : AdminLayer{admin_layer.stats_} { + values_ = admin_layer.values(); + } /** * Merge the provided values into our entry map. An empty value indicates that a key should be @@ -180,6 +182,7 @@ class LoaderImpl : public Loader { private: RuntimeStats generateStats(Stats::Store& store); + ThreadLocal::SlotPtr tls_; }; diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index f9458b3ca3ffe..6d70eee40ab87 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -546,10 +546,11 @@ Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& res response_headers.insertContentType().value().setReference( Http::Headers::get().ContentTypeValues.Json); + // TODO(jsedgwick) Use proto to structure this output instead of arbitrary JSON rapidjson::Document document; document.SetObject(); auto& allocator = document.GetAllocator(); - std::map value_arrays; + std::map entry_objects; rapidjson::Value layer_names{rapidjson::kArrayType}; const auto& layers = server_.runtime().snapshot().getLayers(); @@ -557,25 +558,34 @@ Http::Code AdminImpl::handlerRuntime(absl::string_view url, Http::HeaderMap& res rapidjson::Value layer_name; layer_name.SetString(layer->name().c_str(), allocator); layer_names.PushBack(std::move(layer_name), allocator); - for (const auto& kv : layer->values()) { - value_arrays.emplace(kv.first, rapidjson::Value{rapidjson::kArrayType}); + rapidjson::Value entry_object{rapidjson::kObjectType}; + const auto it = entry_objects.find(kv.first); + if (it == entry_objects.end()) { + rapidjson::Value entry_object{rapidjson::kObjectType}; + entry_object.AddMember("layer_values", rapidjson::Value{kArrayType}, allocator); + entry_object.AddMember("final_value", "", allocator); + entry_objects.emplace(kv.first, std::move(entry_object)); + } } } document.AddMember("layers", std::move(layer_names), allocator); for (const auto& layer : layers) { - for (auto& kv : value_arrays) { + for (auto& kv : entry_objects) { const auto it = layer->values().find(kv.first); - const auto& value = it == layer->values().end() ? "" : it->second.string_value_; - rapidjson::Value value_obj; - value_obj.SetString(value.c_str(), allocator); - kv.second.PushBack(std::move(value_obj), allocator); + const auto& entry_value = it == layer->values().end() ? "" : it->second.string_value_; + rapidjson::Value entry_value_object; + entry_value_object.SetString(entry_value.c_str(), allocator); + if (!entry_value.empty()) { + kv.second["final_value"] = rapidjson::Value{entry_value_object, allocator}; + } + kv.second["layer_values"].PushBack(entry_value_object, allocator); } } rapidjson::Value value_arrays_obj{rapidjson::kObjectType}; - for (auto& kv : value_arrays) { + for (auto& kv : entry_objects) { value_arrays_obj.AddMember(rapidjson::StringRef(kv.first.c_str()), std::move(kv.second), allocator); } diff --git a/source/server/server.cc b/source/server/server.cc index 95bcec89f2e02..3cc6a2a8a22e1 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -289,13 +289,13 @@ Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server, ENVOY_LOG(info, "runtime override subdirectory: {}", override_subdirectory); Api::OsSysCallsPtr os_sys_calls(new Api::OsSysCallsImpl); - return Runtime::LoaderPtr{new Runtime::DiskBackedLoaderImpl( + return std::make_unique( server.dispatcher(), server.threadLocal(), config.runtime()->symlinkRoot(), config.runtime()->subdirectory(), override_subdirectory, server.stats(), server.random(), - std::move(os_sys_calls))}; + std::move(os_sys_calls)); } else { - return Runtime::LoaderPtr{ - new Runtime::LoaderImpl(server.random(), server.stats(), server.threadLocal())}; + return std::make_unique(server.random(), server.stats(), + server.threadLocal()); } } diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 55af130d7482d..09b11bdf4b125 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -261,22 +261,34 @@ TEST_P(AdminInstanceTest, Runtime) { "layer2" ], "entries": { - "extra_key": [ - "", - "bar" - ], - "int_key": [ - "1", - "" - ], - "other_key": [ - "bar", - "" - ], - "string_key": [ - "foo", - "override" - ] + "extra_key": { + "layer_values": [ + "", + "bar" + ], + "final_value": "bar" + }, + "int_key": { + "layer_values": [ + "1", + "" + ], + "final_value": "1" + }, + "other_key": { + "layer_values": [ + "bar", + "" + ], + "final_value": "bar" + }, + "string_key": { + "layer_values": [ + "foo", + "override" + ], + "final_value": "override" + } } })EOF";