From 3490c81b20e0d424a05eae6ab72dbb661b7f3bef Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 21 May 2021 20:39:31 +0000 Subject: [PATCH 01/10] grid: Modify the AlternateProtocolsCache to specify expiration per-protocol not per origin, to match the alt-svc spec. Signed-off-by: Ryan Hamilton --- .../envoy/http/alternate_protocols_cache.h | 13 ++++---- .../http/alternate_protocols_cache_impl.cc | 32 ++++++++++-------- .../http/alternate_protocols_cache_impl.h | 10 ++---- .../alternate_protocols_cache_impl_test.cc | 33 ++++++++++++------- 4 files changed, 48 insertions(+), 40 deletions(-) diff --git a/include/envoy/http/alternate_protocols_cache.h b/include/envoy/http/alternate_protocols_cache.h index 6d94f32eafcb6..0f83c718d6f21 100644 --- a/include/envoy/http/alternate_protocols_cache.h +++ b/include/envoy/http/alternate_protocols_cache.h @@ -64,12 +64,14 @@ class AlternateProtocolsCache { }; /** - * Represents an alternative protocol that can be used to connect to an origin. + * Represents an alternative protocol that can be used to connect to an origin + * with a specified expiration time. */ struct AlternateProtocol { public: - AlternateProtocol(absl::string_view alpn, absl::string_view hostname, uint32_t port) - : alpn_(alpn), hostname_(hostname), port_(port) {} + AlternateProtocol(absl::string_view alpn, absl::string_view hostname, uint32_t port, + MonotonicTime expiration) + : alpn_(alpn), hostname_(hostname), port_(port), expiration_(expiration) {} bool operator==(const AlternateProtocol& other) const { return std::tie(alpn_, hostname_, port_) == @@ -81,6 +83,7 @@ class AlternateProtocolsCache { std::string alpn_; std::string hostname_; uint32_t port_; + MonotonicTime expiration_; }; virtual ~AlternateProtocolsCache() = default; @@ -90,11 +93,9 @@ class AlternateProtocolsCache { * specified origin. Expires after the specified expiration time. * @param origin The origin to set alternate protocols for. * @param protocols A list of alternate protocols. - * @param expiration The time after which the alternatives are no longer valid. */ virtual void setAlternatives(const Origin& origin, - const std::vector& protocols, - const MonotonicTime& expiration) PURE; + const std::vector& protocols) PURE; /** * Returns the possible alternative protocols which can be used to connect to the diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index 89d31df615aaf..8d732312dad90 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -9,15 +9,8 @@ AlternateProtocolsCacheImpl::AlternateProtocolsCacheImpl(TimeSource& time_source AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default; void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, - const std::vector& protocols, - const MonotonicTime& expiration) { - Entry& entry = protocols_[origin]; - if (entry.protocols_ != protocols) { - entry.protocols_ = protocols; - } - if (entry.expiration_ != expiration) { - entry.expiration_ = expiration; - } + const std::vector& protocols) { + protocols_[origin] = protocols; } OptRef> @@ -28,15 +21,26 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { nullptr); } - const Entry& entry = entry_it->second; - if (time_source_.monotonicTime() > entry.expiration_) { - // Expire the entry. - // TODO(RyanTheOptimist): expire entries based on a timer. + auto& protocols = entry_it->second; + + const MonotonicTime now = time_source_.monotonicTime(); + auto it = protocols.begin(); + while (it != protocols.end()) { + if (now > it->expiration_) { + it = protocols.erase(it); + continue; + } + ++it; + } + + if (protocols.empty()) { protocols_.erase(entry_it); return makeOptRefFromPtr>( nullptr); } - return makeOptRef(entry.protocols_); + + const auto& p = entry_it->second; + return makeOptRef(p); } size_t AlternateProtocolsCacheImpl::size() const { return protocols_.size(); } diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index f72ec506dc2d4..342c0bedabf05 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -22,23 +22,17 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { ~AlternateProtocolsCacheImpl() override; // AlternateProtocolsCache - void setAlternatives(const Origin& origin, const std::vector& protocols, - const MonotonicTime& expiration) override; + void setAlternatives(const Origin& origin, const std::vector& protocols) override; OptRef> findAlternatives(const Origin& origin) override; size_t size() const override; private: - struct Entry { - std::vector protocols_; - MonotonicTime expiration_; - }; - // 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_; + std::map> protocols_; }; } // namespace Http diff --git a/test/common/http/alternate_protocols_cache_impl_test.cc b/test/common/http/alternate_protocols_cache_impl_test.cc index dc0f95848d2b3..19172ddd22c2d 100644 --- a/test/common/http/alternate_protocols_cache_impl_test.cc +++ b/test/common/http/alternate_protocols_cache_impl_test.cc @@ -23,29 +23,29 @@ class AlternateProtocolsCacheImplTest : public testing::Test, public Event::Test const std::string alpn1_ = "alpn1"; const std::string alpn2_ = "alpn2"; + const MonotonicTime expiration1_ = simTime().monotonicTime() + Seconds(5); + const MonotonicTime expiration2_ = simTime().monotonicTime() + Seconds(10); + const AlternateProtocolsCacheImpl::Origin origin1_ = {https_, hostname1_, port1_}; const AlternateProtocolsCacheImpl::Origin origin2_ = {https_, hostname2_, port2_}; - const AlternateProtocolsCacheImpl::AlternateProtocol protocol1_ = {alpn1_, hostname1_, port1_}; - const AlternateProtocolsCacheImpl::AlternateProtocol protocol2_ = {alpn2_, hostname2_, port2_}; + const AlternateProtocolsCacheImpl::AlternateProtocol protocol1_ = {alpn1_, hostname1_, port1_, expiration1_}; + const AlternateProtocolsCacheImpl::AlternateProtocol protocol2_ = {alpn2_, hostname2_, port2_, expiration2_}; const std::vector protocols1_ = {protocol1_}; const std::vector protocols2_ = {protocol2_}; - - const MonotonicTime expiration1_ = simTime().monotonicTime() + Seconds(5); - const MonotonicTime expiration2_ = simTime().monotonicTime() + Seconds(10); }; TEST_F(AlternateProtocolsCacheImplTest, Init) { EXPECT_EQ(0, protocols_.size()); } TEST_F(AlternateProtocolsCacheImplTest, SetAlternatives) { EXPECT_EQ(0, protocols_.size()); - protocols_.setAlternatives(origin1_, protocols1_, expiration1_); + protocols_.setAlternatives(origin1_, protocols1_); EXPECT_EQ(1, protocols_.size()); } TEST_F(AlternateProtocolsCacheImplTest, FindAlternatives) { - protocols_.setAlternatives(origin1_, protocols1_, expiration1_); + protocols_.setAlternatives(origin1_, protocols1_); OptRef> protocols = protocols_.findAlternatives(origin1_); ASSERT_TRUE(protocols.has_value()); @@ -53,8 +53,8 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternatives) { } TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterReplacement) { - protocols_.setAlternatives(origin1_, protocols1_, expiration1_); - protocols_.setAlternatives(origin1_, protocols2_, expiration2_); + protocols_.setAlternatives(origin1_, protocols1_); + protocols_.setAlternatives(origin1_, protocols2_); OptRef> protocols = protocols_.findAlternatives(origin1_); ASSERT_TRUE(protocols.has_value()); @@ -63,8 +63,8 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterReplacement) { } TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesForMultipleOrigins) { - protocols_.setAlternatives(origin1_, protocols1_, expiration1_); - protocols_.setAlternatives(origin2_, protocols2_, expiration2_); + protocols_.setAlternatives(origin1_, protocols1_); + protocols_.setAlternatives(origin2_, protocols2_); OptRef> protocols = protocols_.findAlternatives(origin1_); ASSERT_TRUE(protocols.has_value()); @@ -75,7 +75,7 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesForMultipleOrigins) { } TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterExpiration) { - protocols_.setAlternatives(origin1_, protocols1_, expiration1_); + protocols_.setAlternatives(origin1_, protocols1_); simTime().setMonotonicTime(expiration1_ + Seconds(1)); OptRef> protocols = protocols_.findAlternatives(origin1_); @@ -83,6 +83,15 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterExpiration) { EXPECT_EQ(0, protocols_.size()); } +TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterPartialExpiration) { + protocols_.setAlternatives(origin1_, { protocol1_, protocol2_}); + simTime().setMonotonicTime(expiration1_ + Seconds(1)); + OptRef> protocols = + protocols_.findAlternatives(origin1_); + ASSERT_TRUE(protocols.has_value()); + EXPECT_EQ(protocols2_, protocols.ref()); +} + } // namespace } // namespace Http } // namespace Envoy From a2e5ae56ba52befd01e5ccf7f09d42fc2f6f5445 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 21 May 2021 20:45:41 +0000 Subject: [PATCH 02/10] conn_pool_grid_test Signed-off-by: Ryan Hamilton --- test/common/http/conn_pool_grid_test.cc | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index e25551aaaf5dc..a66acff24102b 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -122,9 +122,8 @@ class ConnectivityGridTestBase : public Event::TestUsingSimulatedTime, public te void addHttp3AlternateProtocol() { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "", origin.port_}}; - alternate_protocols_->setAlternatives(origin, protocols, - simTime().monotonicTime() + Seconds(5)); + {"h3-29", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + alternate_protocols_->setAlternatives(origin, protocols); } const Network::ConnectionSocket::OptionsSharedPtr socket_options_; @@ -513,8 +512,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3) TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithExpiredHttp3) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "", origin.port_}}; - alternate_protocols_->setAlternatives(origin, protocols, simTime().monotonicTime() + Seconds(5)); + {"h3-29", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + alternate_protocols_->setAlternatives(origin, protocols); simTime().setMonotonicTime(simTime().monotonicTime() + Seconds(10)); EXPECT_EQ(grid_.first(), nullptr); @@ -536,8 +535,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithExpiredHt TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingHostname) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "otherhostname", origin.port_}}; - alternate_protocols_->setAlternatives(origin, protocols, simTime().monotonicTime() + Seconds(5)); + {"h3-29", "otherhostname", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + alternate_protocols_->setAlternatives(origin, protocols); EXPECT_EQ(grid_.first(), nullptr); @@ -557,8 +556,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3N TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingPort) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "", origin.port_ + 1}}; - alternate_protocols_->setAlternatives(origin, protocols, simTime().monotonicTime() + Seconds(5)); + {"h3-29", "", origin.port_ + 1, simTime().monotonicTime() + Seconds(5)}}; + alternate_protocols_->setAlternatives(origin, protocols); EXPECT_EQ(grid_.first(), nullptr); @@ -577,8 +576,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3N TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingAlpn) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"http/2", "", origin.port_}}; - alternate_protocols_->setAlternatives(origin, protocols, simTime().monotonicTime() + Seconds(5)); + {"http/2", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + alternate_protocols_->setAlternatives(origin, protocols); EXPECT_EQ(grid_.first(), nullptr); From 36bc81f2871cbf13b4f715e5ed84e3fa75dc46e0 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 21 May 2021 20:52:07 +0000 Subject: [PATCH 03/10] format Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.h | 3 ++- .../common/http/alternate_protocols_cache_impl_test.cc | 8 +++++--- test/common/http/conn_pool_grid_test.cc | 10 +++++----- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/source/common/http/alternate_protocols_cache_impl.h b/source/common/http/alternate_protocols_cache_impl.h index 342c0bedabf05..a029a970c763f 100644 --- a/source/common/http/alternate_protocols_cache_impl.h +++ b/source/common/http/alternate_protocols_cache_impl.h @@ -22,7 +22,8 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache { ~AlternateProtocolsCacheImpl() override; // AlternateProtocolsCache - void setAlternatives(const Origin& origin, const std::vector& protocols) override; + void setAlternatives(const Origin& origin, + const std::vector& protocols) override; OptRef> findAlternatives(const Origin& origin) override; size_t size() const override; diff --git a/test/common/http/alternate_protocols_cache_impl_test.cc b/test/common/http/alternate_protocols_cache_impl_test.cc index 19172ddd22c2d..5db7b80533aa9 100644 --- a/test/common/http/alternate_protocols_cache_impl_test.cc +++ b/test/common/http/alternate_protocols_cache_impl_test.cc @@ -29,8 +29,10 @@ class AlternateProtocolsCacheImplTest : public testing::Test, public Event::Test const AlternateProtocolsCacheImpl::Origin origin1_ = {https_, hostname1_, port1_}; const AlternateProtocolsCacheImpl::Origin origin2_ = {https_, hostname2_, port2_}; - const AlternateProtocolsCacheImpl::AlternateProtocol protocol1_ = {alpn1_, hostname1_, port1_, expiration1_}; - const AlternateProtocolsCacheImpl::AlternateProtocol protocol2_ = {alpn2_, hostname2_, port2_, expiration2_}; + const AlternateProtocolsCacheImpl::AlternateProtocol protocol1_ = {alpn1_, hostname1_, port1_, + expiration1_}; + const AlternateProtocolsCacheImpl::AlternateProtocol protocol2_ = {alpn2_, hostname2_, port2_, + expiration2_}; const std::vector protocols1_ = {protocol1_}; const std::vector protocols2_ = {protocol2_}; @@ -84,7 +86,7 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterExpiration) { } TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterPartialExpiration) { - protocols_.setAlternatives(origin1_, { protocol1_, protocol2_}); + protocols_.setAlternatives(origin1_, {protocol1_, protocol2_}); simTime().setMonotonicTime(expiration1_ + Seconds(1)); OptRef> protocols = protocols_.findAlternatives(origin1_); diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index a66acff24102b..d52fd391620af 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -122,7 +122,7 @@ class ConnectivityGridTestBase : public Event::TestUsingSimulatedTime, public te void addHttp3AlternateProtocol() { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + {"h3-29", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; alternate_protocols_->setAlternatives(origin, protocols); } @@ -512,7 +512,7 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3) TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithExpiredHttp3) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + {"h3-29", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; alternate_protocols_->setAlternatives(origin, protocols); simTime().setMonotonicTime(simTime().monotonicTime() + Seconds(10)); @@ -535,7 +535,7 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithExpiredHt TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingHostname) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "otherhostname", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + {"h3-29", "otherhostname", origin.port_, simTime().monotonicTime() + Seconds(5)}}; alternate_protocols_->setAlternatives(origin, protocols); EXPECT_EQ(grid_.first(), nullptr); @@ -556,7 +556,7 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3N TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingPort) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"h3-29", "", origin.port_ + 1, simTime().monotonicTime() + Seconds(5)}}; + {"h3-29", "", origin.port_ + 1, simTime().monotonicTime() + Seconds(5)}}; alternate_protocols_->setAlternatives(origin, protocols); EXPECT_EQ(grid_.first(), nullptr); @@ -576,7 +576,7 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3N TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingAlpn) { AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000); const std::vector protocols = { - {"http/2", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; + {"http/2", "", origin.port_, simTime().monotonicTime() + Seconds(5)}}; alternate_protocols_->setAlternatives(origin, protocols); EXPECT_EQ(grid_.first(), nullptr); From 7f054b538836e013451aa7a0d0e1c62a610105ec Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 21 May 2021 21:03:51 +0000 Subject: [PATCH 04/10] Cleanup Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.cc | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index 8d732312dad90..861101b1c58f3 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -21,7 +21,7 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { nullptr); } - auto& protocols = entry_it->second; + std::vector& protocols = entry_it->second; const MonotonicTime now = time_source_.monotonicTime(); auto it = protocols.begin(); @@ -39,8 +39,7 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { nullptr); } - const auto& p = entry_it->second; - return makeOptRef(p); + return makeOptRef(const_cast&>(protocols)); } size_t AlternateProtocolsCacheImpl::size() const { return protocols_.size(); } From a747f35508dd9b0e19de80f718ad4e5266a46962 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Mon, 24 May 2021 22:46:59 +0000 Subject: [PATCH 05/10] == Signed-off-by: Ryan Hamilton --- include/envoy/http/alternate_protocols_cache.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/envoy/http/alternate_protocols_cache.h b/include/envoy/http/alternate_protocols_cache.h index 0f83c718d6f21..2d02cda9bd48e 100644 --- a/include/envoy/http/alternate_protocols_cache.h +++ b/include/envoy/http/alternate_protocols_cache.h @@ -74,8 +74,8 @@ class AlternateProtocolsCache { : alpn_(alpn), hostname_(hostname), port_(port), expiration_(expiration) {} bool operator==(const AlternateProtocol& other) const { - return std::tie(alpn_, hostname_, port_) == - std::tie(other.alpn_, other.hostname_, other.port_); + return std::tie(alpn_, hostname_, port_, expiration_) == + std::tie(other.alpn_, other.hostname_, other.port_, other.expiration_); } bool operator!=(const AlternateProtocol& other) const { return !this->operator==(other); } From 669e82554bd9792b61f7369cb74552cfff45afec Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Tue, 25 May 2021 17:19:10 +0000 Subject: [PATCH 06/10] Address comments from Alyssa Signed-off-by: Ryan Hamilton --- .../http/alternate_protocols_cache_impl.cc | 21 ++++++++----------- .../alternate_protocols_cache_impl_test.cc | 1 + 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index 861101b1c58f3..c032c95e619cf 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -10,6 +10,8 @@ AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default; void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, const std::vector& protocols) { + // TODO(RyanTheOptimist): Figure out the best place to enforce this limit. + ASSERT(protocols.size() < 10); protocols_[origin] = protocols; } @@ -17,26 +19,21 @@ OptRef> AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { auto entry_it = protocols_.find(origin); if (entry_it == protocols_.end()) { - return makeOptRefFromPtr>( - nullptr); + return makeOptRefFromPtr>(nullptr); } std::vector& protocols = entry_it->second; const MonotonicTime now = time_source_.monotonicTime(); - auto it = protocols.begin(); - while (it != protocols.end()) { - if (now > it->expiration_) { - it = protocols.erase(it); - continue; - } - ++it; - } + protocols.erase(std::remove_if(protocols.begin(), protocols.end(), + [now](const AlternateProtocol& protocol) { + return (now > protocol.expiration_); + }), + protocols.end()); if (protocols.empty()) { protocols_.erase(entry_it); - return makeOptRefFromPtr>( - nullptr); + return makeOptRefFromPtr>(nullptr); } return makeOptRef(const_cast&>(protocols)); diff --git a/test/common/http/alternate_protocols_cache_impl_test.cc b/test/common/http/alternate_protocols_cache_impl_test.cc index 5db7b80533aa9..569c4aebd560f 100644 --- a/test/common/http/alternate_protocols_cache_impl_test.cc +++ b/test/common/http/alternate_protocols_cache_impl_test.cc @@ -91,6 +91,7 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterPartialExpiration) OptRef> protocols = protocols_.findAlternatives(origin1_); ASSERT_TRUE(protocols.has_value()); + EXPECT_EQ(protocols2_.size(), protocols->size()); EXPECT_EQ(protocols2_, protocols.ref()); } From 6dde521daa4cc9500ac58fbc3146e67e9118ac31 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Tue, 25 May 2021 17:21:13 +0000 Subject: [PATCH 07/10] format Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index c032c95e619cf..fbcfcd4b74398 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -26,9 +26,9 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) { const MonotonicTime now = time_source_.monotonicTime(); protocols.erase(std::remove_if(protocols.begin(), protocols.end(), - [now](const AlternateProtocol& protocol) { - return (now > protocol.expiration_); - }), + [now](const AlternateProtocol& protocol) { + return (now > protocol.expiration_); + }), protocols.end()); if (protocols.empty()) { From 069ca4f60d0cc3a68d1fcdb08e9b6e94a0ebe679 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Tue, 25 May 2021 17:49:16 +0000 Subject: [PATCH 08/10] include Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index fbcfcd4b74398..52b0d2257c82a 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -1,5 +1,7 @@ #include "common/http/alternate_protocols_cache_impl.h" +#include "common/common/assert.h" + namespace Envoy { namespace Http { From a22d247eec2c48de328641ea6a8668c0df99d90b Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Tue, 25 May 2021 23:19:06 +0000 Subject: [PATCH 09/10] Truncate Signed-off-by: Ryan Hamilton --- source/common/http/BUILD | 1 + .../http/alternate_protocols_cache_impl.cc | 9 ++++++--- .../alternate_protocols_cache_impl_test.cc | 20 +++++++++++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index bc2b727a1b3fb..a581bc5d8d54e 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -176,6 +176,7 @@ envoy_cc_library( "//include/envoy/singleton:manager_interface", "//include/envoy/thread_local:thread_local_interface", "//include/envoy/upstream:resource_manager_interface", + "//source/common/common:logger_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index 52b0d2257c82a..c2e2f1ce2130f 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -1,6 +1,6 @@ #include "common/http/alternate_protocols_cache_impl.h" -#include "common/common/assert.h" +#include "common/common/logger.h" namespace Envoy { namespace Http { @@ -12,9 +12,12 @@ AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default; void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, const std::vector& protocols) { - // TODO(RyanTheOptimist): Figure out the best place to enforce this limit. - ASSERT(protocols.size() < 10); protocols_[origin] = protocols; + static const size_t max_protocols = 10; + if (protocols.size() > max_protocols) { + ENVOY_LOG_MISC(trace, "Too many alternate protocols: {}, truncating", protocols.size()); + protocols_[origin].erase(protocols_[origin].begin() + max_protocols); + } } OptRef> diff --git a/test/common/http/alternate_protocols_cache_impl_test.cc b/test/common/http/alternate_protocols_cache_impl_test.cc index 569c4aebd560f..c4aa1c753b769 100644 --- a/test/common/http/alternate_protocols_cache_impl_test.cc +++ b/test/common/http/alternate_protocols_cache_impl_test.cc @@ -95,6 +95,26 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterPartialExpiration) EXPECT_EQ(protocols2_, protocols.ref()); } +TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterTruncation) { + AlternateProtocolsCacheImpl::AlternateProtocol protocol = protocol1_; + + std::vector expected_protocols; + for (size_t i = 0; i < 10; ++i) { + protocol.port_++; + expected_protocols.push_back(protocol); + } + std::vector full_protocols = expected_protocols; + protocol.port_++; + full_protocols.push_back(protocol); + + protocols_.setAlternatives(origin1_, full_protocols); + OptRef> protocols = + protocols_.findAlternatives(origin1_); + ASSERT_TRUE(protocols.has_value()); + EXPECT_EQ(10, protocols->size()); + EXPECT_EQ(expected_protocols, protocols.ref()); +} + } // namespace } // namespace Http } // namespace Envoy From ad1e2d3cc63595c3267c54374f3a4aacec98f4f1 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Tue, 25 May 2021 23:59:40 +0000 Subject: [PATCH 10/10] Better truncate Signed-off-by: Ryan Hamilton --- source/common/http/alternate_protocols_cache_impl.cc | 3 ++- test/common/http/alternate_protocols_cache_impl_test.cc | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/http/alternate_protocols_cache_impl.cc b/source/common/http/alternate_protocols_cache_impl.cc index c2e2f1ce2130f..4368e9a778599 100644 --- a/source/common/http/alternate_protocols_cache_impl.cc +++ b/source/common/http/alternate_protocols_cache_impl.cc @@ -16,7 +16,8 @@ void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin, static const size_t max_protocols = 10; if (protocols.size() > max_protocols) { ENVOY_LOG_MISC(trace, "Too many alternate protocols: {}, truncating", protocols.size()); - protocols_[origin].erase(protocols_[origin].begin() + max_protocols); + std::vector& p = protocols_[origin]; + p.erase(p.begin() + max_protocols, p.end()); } } diff --git a/test/common/http/alternate_protocols_cache_impl_test.cc b/test/common/http/alternate_protocols_cache_impl_test.cc index c4aa1c753b769..88347e98bac3e 100644 --- a/test/common/http/alternate_protocols_cache_impl_test.cc +++ b/test/common/http/alternate_protocols_cache_impl_test.cc @@ -106,6 +106,7 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterTruncation) { std::vector full_protocols = expected_protocols; protocol.port_++; full_protocols.push_back(protocol); + full_protocols.push_back(protocol); protocols_.setAlternatives(origin1_, full_protocols); OptRef> protocols =