From 38d3171b3a6f6210a14c8032d833d04f307a847c Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 24 Sep 2021 17:33:20 +0000 Subject: [PATCH 1/8] alternate_protocols_cache: Impose a max size limit on the alternate protocols cache Implement a very basic MRU map in AlternateProtocolsCacheImpl. Signed-off-by: Ryan Hamilton --- .../http/alternate_protocols_cache_impl.cc | 31 ++++++++++++++----- .../http/alternate_protocols_cache_impl.h | 13 +++++--- .../alternate_protocols_cache_manager_impl.cc | 2 +- .../alternate_protocols_cache_impl_test.cc | 21 ++++++++++++- 4 files changed, 53 insertions(+), 14 deletions(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index 851209796c9c..cdde58a4673d 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -64,8 +64,9 @@ AlternateProtocolsCacheImpl::protocolsFromString(absl::string_view alt_svc_strin } AlternateProtocolsCacheImpl::AlternateProtocolsCacheImpl( - TimeSource& time_source, std::unique_ptr&& key_value_store) - : time_source_(time_source), key_value_store_(std::move(key_value_store)) {} + TimeSource& time_source, std::unique_ptr&& key_value_store, size_t max_entries) + : time_source_(time_source), key_value_store_(std::move(key_value_store)), + max_entries_(max_entries) {} AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default; @@ -76,7 +77,20 @@ void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, ENVOY_LOG_MISC(trace, "Too many alternate protocols: {}, truncating", protocols.size()); protocols.erase(protocols.begin() + max_protocols, protocols.end()); } - protocols_[origin] = protocols; + while (protocols_list_.size() >= max_entries_) { + auto iter = protocols_list_.rbegin(); + key_value_store_->remove(originToString(iter->first)); + protocols_map_.erase(protocols_map_.find(iter->first)); + protocols_list_.erase((++iter).base()); + } + auto iter = protocols_map_.find(origin); + if (iter != protocols_map_.end()) { + protocols_list_.erase(iter->second); + protocols_map_.erase(iter); + } + std::pair> value = {origin, protocols}; + protocols_list_.emplace_front(origin, protocols); + protocols_map_[origin] = protocols_list_.begin(); if (key_value_store_) { key_value_store_->addOrUpdate(originToString(origin), protocolsToStringForCache(protocols, time_source_)); @@ -85,12 +99,12 @@ void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, OptRef> AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { - auto entry_it = protocols_.find(origin); - if (entry_it == protocols_.end()) { + auto entry_it = protocols_map_.find(origin); + if (entry_it == protocols_map_.end()) { return makeOptRefFromPtr>(nullptr); } - std::vector& protocols = entry_it->second; + std::vector& protocols = entry_it->second->second; auto original_size = protocols.size(); const MonotonicTime now = time_source_.monotonicTime(); @@ -101,7 +115,8 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { protocols.end()); if (protocols.empty()) { - protocols_.erase(entry_it); + protocols_map_.erase(entry_it); + protocols_list_.erase(entry_it->second); if (key_value_store_) { key_value_store_->remove(originToString(origin)); } @@ -115,7 +130,7 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { return makeOptRef(const_cast&>(protocols)); } -size_t AlternateProtocolsCacheImpl::size() const { return protocols_.size(); } +size_t AlternateProtocolsCacheImpl::size() const { return protocols_list_.size(); } } // namespace Http } // namespace Envoy diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index df216bf0407c..f173c21ae50a 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -19,7 +19,8 @@ namespace Http { // See: source/docs/http3_upstream.md class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { public: - AlternateProtocolsCacheImpl(TimeSource& time_source, std::unique_ptr&& store); + AlternateProtocolsCacheImpl(TimeSource& time_source, std::unique_ptr&& store, + size_t max_entries); ~AlternateProtocolsCacheImpl() override; // Convert an AlternateProtocol vector to a string to cache to the key value @@ -48,12 +49,16 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { // Time source used to check expiration of entries. TimeSource& time_source_; - // Map from hostname to list of alternate protocols. - // TODO(RyanTheOptimist): Add a limit to the size of this map and evict based on usage. - std::map> protocols_; + using ListType = std::list>>; + // List of origin, alternate protocol pairs, in insertion order. + ListType protocols_list_; + // Map from hostname to iterator into protocols_list_. + std::map protocols_map_; // The key value store, if flushing to persistent storage. std::unique_ptr key_value_store_; + + const size_t max_entries_; }; } // namespace Http diff --git a/source/common/http/alternate_protocols_cache_manager_impl.cc b/source/common/http/alternate_protocols_cache_manager_impl.cc index 9496a5ea1c8b..c0bf2016258c 100644 --- a/source/common/http/alternate_protocols_cache_manager_impl.cc +++ b/source/common/http/alternate_protocols_cache_manager_impl.cc @@ -51,7 +51,7 @@ AlternateProtocolsCacheSharedPtr AlternateProtocolsCacheManagerImpl::getCache( } AlternateProtocolsCacheSharedPtr new_cache = std::make_shared( - data_.dispatcher_.timeSource(), std::move(store)); + data_.dispatcher_.timeSource(), std::move(store), options.max_entries().value()); (*slot_).caches_.emplace(options.name(), CacheWithOptions{options, new_cache}); return new_cache; } diff --git a/test/common/http/alternate_protocols_cache_impl_test.cc b/test/common/http/alternate_protocols_cache_impl_test.cc index 9aa0dc5f0016..651e371bb89f 100644 --- a/test/common/http/alternate_protocols_cache_impl_test.cc +++ b/test/common/http/alternate_protocols_cache_impl_test.cc @@ -15,10 +15,13 @@ class AlternateProtocolsCacheImplTest : public testing::Test, public Event::Test public: AlternateProtocolsCacheImplTest() : store_(new NiceMock()), - protocols_(simTime(), std::unique_ptr(store_)) {} + protocols_(simTime(), std::unique_ptr(store_), max_entries_) {} + + const size_t max_entries_ = 10; MockKeyValueStore* store_; AlternateProtocolsCacheImpl protocols_; + const std::string hostname1_ = "hostname1"; const std::string hostname2_ = "hostname2"; const uint32_t port1_ = 1; @@ -132,6 +135,22 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterTruncation) { EXPECT_EQ(expected_protocols, protocols.ref()); } +TEST_F(AlternateProtocolsCacheImplTest, MaxEntries) { + EXPECT_EQ(0, protocols_.size()); + const std::string hostname = "hostname"; + for (uint32_t i = 0; i <= max_entries_; ++i) { + const AlternateProtocolsCache::Origin origin = {https_, hostname, i}; + AlternateProtocolsCache::AlternateProtocol protocol = {alpn1_, hostname, i, expiration1_}; + std::vector protocols = {protocol}; + EXPECT_CALL(*store_, addOrUpdate(absl::StrCat("https://hostname:", i), + absl::StrCat("alpn1=\"hostname:", i, "\"; ma=5"))); + if (i == max_entries_) { + EXPECT_CALL(*store_, remove("https://hostname:0")); + } + protocols_.setAlternatives(origin, protocols); + } +} + TEST_F(AlternateProtocolsCacheImplTest, ToAndFromString) { auto testAltSvc = [&](const std::string& original_alt_svc, const std::string& expected_alt_svc) -> void { From 2ae6924624b7d6f6c96b2e2bc44765e52e8d4400 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Sat, 25 Sep 2021 14:59:50 +0000 Subject: [PATCH 2/8] Fix comments from Renjie Signed-off-by: Ryan Hamilton Signed-off-by: Ryan Hamilton --- .../common/http/alternate_protocols_cache_impl.cc | 9 +++++---- source/common/http/alternate_protocols_cache_impl.h | 13 ++++++++++--- test/common/http/conn_pool_grid_test.cc | 4 ++-- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index cdde58a4673d..aad8fc4cf9bb 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -66,7 +66,8 @@ AlternateProtocolsCacheImpl::protocolsFromString(absl::string_view alt_svc_strin AlternateProtocolsCacheImpl::AlternateProtocolsCacheImpl( TimeSource& time_source, std::unique_ptr&& key_value_store, size_t max_entries) : time_source_(time_source), key_value_store_(std::move(key_value_store)), - max_entries_(max_entries) {} + max_entries_(max_entries > 0 ? max_entries : 1024) { +} AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default; @@ -79,8 +80,8 @@ void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, } while (protocols_list_.size() >= max_entries_) { auto iter = protocols_list_.rbegin(); - key_value_store_->remove(originToString(iter->first)); - protocols_map_.erase(protocols_map_.find(iter->first)); + key_value_store_->remove(originToString(iter->origin_)); + protocols_map_.erase(protocols_map_.find(iter->origin_)); protocols_list_.erase((++iter).base()); } auto iter = protocols_map_.find(origin); @@ -104,7 +105,7 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { return makeOptRefFromPtr>(nullptr); } - std::vector& protocols = entry_it->second->second; + std::vector& protocols = entry_it->second->protocols_; auto original_size = protocols.size(); const MonotonicTime now = time_source_.monotonicTime(); diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index f173c21ae50a..25a62cbb0cf9 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -49,11 +49,18 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { // Time source used to check expiration of entries. TimeSource& time_source_; - using ListType = std::list>>; + struct OriginProtocols { + OriginProtocols(const Origin& origin, const std::vector& protocols) + //OriginProtocols(Origin origin, std::vector protocols) + : origin_(origin), protocols_(protocols) {} + + Origin origin_; + std::vector protocols_; + }; // List of origin, alternate protocol pairs, in insertion order. - ListType protocols_list_; + std::list protocols_list_; // Map from hostname to iterator into protocols_list_. - std::map protocols_map_; + std::map::iterator> protocols_map_; // The key value store, if flushing to persistent storage. std::unique_ptr key_value_store_; diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index ebe3accff177..69528416002d 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -102,7 +102,7 @@ class ConnectivityGridTest : public Event::TestUsingSimulatedTime, public testin public: ConnectivityGridTest() : options_({Http::Protocol::Http11, Http::Protocol::Http2, Http::Protocol::Http3}), - alternate_protocols_(std::make_shared(simTime(), nullptr)), + alternate_protocols_(std::make_shared(simTime(), nullptr, 10)), quic_stat_names_(store_.symbolTable()), grid_(dispatcher_, random_, Upstream::makeTestHost(cluster_, "hostname", "tcp://127.0.0.1:9000", simTime()), @@ -120,7 +120,7 @@ class ConnectivityGridTest : public Event::TestUsingSimulatedTime, public testin if (!use_alternate_protocols) { return nullptr; } - return std::make_shared(simTime(), nullptr); + return std::make_shared(simTime(), nullptr, 10); } void addHttp3AlternateProtocol() { From 133ef9163303db47aa3151206ed8d7f4f6b69983 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Sat, 25 Sep 2021 16:09:18 +0000 Subject: [PATCH 3/8] Format Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.cc | 3 +-- source/common/http/alternate_protocols_cache_impl.h | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index aad8fc4cf9bb..514b6a6f913e 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -66,8 +66,7 @@ AlternateProtocolsCacheImpl::protocolsFromString(absl::string_view alt_svc_strin AlternateProtocolsCacheImpl::AlternateProtocolsCacheImpl( TimeSource& time_source, std::unique_ptr&& key_value_store, size_t max_entries) : time_source_(time_source), key_value_store_(std::move(key_value_store)), - max_entries_(max_entries > 0 ? max_entries : 1024) { -} + max_entries_(max_entries > 0 ? max_entries : 1024) {} AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default; diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index 25a62cbb0cf9..05ca0a5f7b03 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -51,7 +51,6 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { struct OriginProtocols { OriginProtocols(const Origin& origin, const std::vector& protocols) - //OriginProtocols(Origin origin, std::vector protocols) : origin_(origin), protocols_(protocols) {} Origin origin_; From 6ae877ab46d7c9d5bf989cfd86da14e8277465b1 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Sat, 25 Sep 2021 19:27:35 +0000 Subject: [PATCH 4/8] fix asan Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index 514b6a6f913e..e96bdf241652 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -115,8 +115,8 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { protocols.end()); if (protocols.empty()) { - protocols_map_.erase(entry_it); protocols_list_.erase(entry_it->second); + protocols_map_.erase(entry_it); if (key_value_store_) { key_value_store_->remove(originToString(origin)); } From becd093bce63256b67c77c02675e50d069e73b8e Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Mon, 27 Sep 2021 19:57:49 +0000 Subject: [PATCH 5/8] linked_hash_map Signed-off-by: Ryan Hamilton --- source/common/http/BUILD | 1 + .../http/alternate_protocols_cache_impl.cc | 29 +++++++------------ .../http/alternate_protocols_cache_impl.h | 21 +++++++------- 3 files changed, 22 insertions(+), 29 deletions(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 60d3cb0cf17e..db5e702ff57e 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -165,6 +165,7 @@ envoy_cc_library( "alternate_protocols_cache_impl.cc", "alternate_protocols_cache_manager_impl.cc", ], + external_deps = ["quiche_quic_platform"], hdrs = [ "alternate_protocols_cache_impl.h", "alternate_protocols_cache_manager_impl.h", diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index e96bdf241652..8f2122049ecd 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -77,20 +77,12 @@ void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, ENVOY_LOG_MISC(trace, "Too many alternate protocols: {}, truncating", protocols.size()); protocols.erase(protocols.begin() + max_protocols, protocols.end()); } - while (protocols_list_.size() >= max_entries_) { - auto iter = protocols_list_.rbegin(); - key_value_store_->remove(originToString(iter->origin_)); - protocols_map_.erase(protocols_map_.find(iter->origin_)); - protocols_list_.erase((++iter).base()); + while (protocols_.size() >= max_entries_) { + auto iter = protocols_.begin(); + key_value_store_->remove(originToString(iter->first)); + protocols_.erase(iter); } - auto iter = protocols_map_.find(origin); - if (iter != protocols_map_.end()) { - protocols_list_.erase(iter->second); - protocols_map_.erase(iter); - } - std::pair> value = {origin, protocols}; - protocols_list_.emplace_front(origin, protocols); - protocols_map_[origin] = protocols_list_.begin(); + protocols_[origin] = protocols; if (key_value_store_) { key_value_store_->addOrUpdate(originToString(origin), protocolsToStringForCache(protocols, time_source_)); @@ -99,12 +91,12 @@ void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, OptRef> AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { - auto entry_it = protocols_map_.find(origin); - if (entry_it == protocols_map_.end()) { + auto entry_it = protocols_.find(origin); + if (entry_it == protocols_.end()) { return makeOptRefFromPtr>(nullptr); } - std::vector& protocols = entry_it->second->protocols_; + std::vector& protocols = entry_it->second; auto original_size = protocols.size(); const MonotonicTime now = time_source_.monotonicTime(); @@ -115,8 +107,7 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { protocols.end()); if (protocols.empty()) { - protocols_list_.erase(entry_it->second); - protocols_map_.erase(entry_it); + protocols_.erase(entry_it); if (key_value_store_) { key_value_store_->remove(originToString(origin)); } @@ -130,7 +121,7 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { return makeOptRef(const_cast&>(protocols)); } -size_t AlternateProtocolsCacheImpl::size() const { return protocols_list_.size(); } +size_t AlternateProtocolsCacheImpl::size() const { return protocols_.size(); } } // namespace Http } // namespace Envoy diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index 05ca0a5f7b03..dbeb163b3897 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -11,6 +11,7 @@ #include "envoy/http/alternate_protocols_cache.h" #include "absl/strings/string_view.h" +#include "quiche/common/quiche_linked_hash_map.h" namespace Envoy { namespace Http { @@ -49,17 +50,17 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { // Time source used to check expiration of entries. TimeSource& time_source_; - struct OriginProtocols { - OriginProtocols(const Origin& origin, const std::vector& protocols) - : origin_(origin), protocols_(protocols) {} - - Origin origin_; - std::vector protocols_; + struct OriginHash { + size_t operator()(const Origin& origin) const { + size_t hash = std::hash()(origin.scheme_) + + 37 * (std::hash()(origin.hostname_) + + 37 * std::hash()(origin.port_)); + return hash; + } }; - // List of origin, alternate protocol pairs, in insertion order. - std::list protocols_list_; - // Map from hostname to iterator into protocols_list_. - std::map::iterator> protocols_map_; + + // Map from origin to list of alternate protocols. + quiche::QuicheLinkedHashMap, OriginHash> protocols_; // The key value store, if flushing to persistent storage. std::unique_ptr key_value_store_; From b450d422f3b041782a7cb4c436aa605f55f7804f Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Mon, 27 Sep 2021 20:42:18 +0000 Subject: [PATCH 6/8] format Signed-off-by: Ryan Hamilton --- source/common/http/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index db5e702ff57e..1a0f09e2c2b3 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -165,11 +165,11 @@ envoy_cc_library( "alternate_protocols_cache_impl.cc", "alternate_protocols_cache_manager_impl.cc", ], - external_deps = ["quiche_quic_platform"], hdrs = [ "alternate_protocols_cache_impl.h", "alternate_protocols_cache_manager_impl.h", ], + external_deps = ["quiche_quic_platform"], deps = [ "//envoy/common:time_interface", "//envoy/event:dispatcher_interface", From 227d2862d296e3cb1e06b234f26ec9c7aa18cd1b Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Mon, 27 Sep 2021 20:51:59 +0000 Subject: [PATCH 7/8] Comment Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index dbeb163b3897..76fff8dc8671 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -52,6 +52,7 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { struct OriginHash { size_t operator()(const Origin& origin) const { + // Mutiply the hashes by the magic number 37 to spread the bits around. size_t hash = std::hash()(origin.scheme_) + 37 * (std::hash()(origin.hostname_) + 37 * std::hash()(origin.port_)); From 581687d94aefd625611791347588921e4d2ae728 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Mon, 27 Sep 2021 20:53:27 +0000 Subject: [PATCH 8/8] spelling Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index 76fff8dc8671..4d3d2ddb10c0 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -52,7 +52,7 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { struct OriginHash { size_t operator()(const Origin& origin) const { - // Mutiply the hashes by the magic number 37 to spread the bits around. + // Multiply the hashes by the magic number 37 to spread the bits around. size_t hash = std::hash()(origin.scheme_) + 37 * (std::hash()(origin.hostname_) + 37 * std::hash()(origin.port_));