Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alternate_protocols_cache: Impose a max size limit on the alternate protocols cache #18258

Merged
merged 8 commits into from
Sep 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/common/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ envoy_cc_library(
"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",
Expand Down
10 changes: 8 additions & 2 deletions source/common/http/alternate_protocols_cache_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,9 @@ AlternateProtocolsCacheImpl::protocolsFromString(absl::string_view alt_svc_strin
}

AlternateProtocolsCacheImpl::AlternateProtocolsCacheImpl(
TimeSource& time_source, std::unique_ptr<KeyValueStore>&& key_value_store)
: time_source_(time_source), key_value_store_(std::move(key_value_store)) {}
TimeSource& time_source, std::unique_ptr<KeyValueStore>&& 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) {}

AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default;

Expand All @@ -76,6 +77,11 @@ 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_.size() >= max_entries_) {
auto iter = protocols_.begin();
key_value_store_->remove(originToString(iter->first));
protocols_.erase(iter);
}
protocols_[origin] = protocols;
if (key_value_store_) {
key_value_store_->addOrUpdate(originToString(origin),
Expand Down
21 changes: 17 additions & 4 deletions source/common/http/alternate_protocols_cache_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -19,7 +20,8 @@ namespace Http {
// See: source/docs/http3_upstream.md
class AlternateProtocolsCacheImpl : public AlternateProtocolsCache {
public:
AlternateProtocolsCacheImpl(TimeSource& time_source, std::unique_ptr<KeyValueStore>&& store);
AlternateProtocolsCacheImpl(TimeSource& time_source, std::unique_ptr<KeyValueStore>&& store,
size_t max_entries);
~AlternateProtocolsCacheImpl() override;

// Convert an AlternateProtocol vector to a string to cache to the key value
Expand Down Expand Up @@ -48,12 +50,23 @@ 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<Origin, std::vector<AlternateProtocol>> protocols_;
struct OriginHash {
size_t operator()(const Origin& origin) const {
// Multiply the hashes by the magic number 37 to spread the bits around.
size_t hash = std::hash<std::string>()(origin.scheme_) +
37 * (std::hash<std::string>()(origin.hostname_) +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

37?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I stole this from source/common/upstream/load_balancer_impl.h which seems to be based on a pattern that's referenced in Effective Java (a great book). Good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added a comment.

37 * std::hash<uint32_t>()(origin.port_));
return hash;
}
};

// Map from origin to list of alternate protocols.
quiche::QuicheLinkedHashMap<Origin, std::vector<AlternateProtocol>, OriginHash> protocols_;

// The key value store, if flushing to persistent storage.
std::unique_ptr<KeyValueStore> key_value_store_;

const size_t max_entries_;
};

} // namespace Http
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ AlternateProtocolsCacheSharedPtr AlternateProtocolsCacheManagerImpl::getCache(
}

AlternateProtocolsCacheSharedPtr new_cache = std::make_shared<AlternateProtocolsCacheImpl>(
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;
}
Expand Down
21 changes: 20 additions & 1 deletion test/common/http/alternate_protocols_cache_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,13 @@ class AlternateProtocolsCacheImplTest : public testing::Test, public Event::Test
public:
AlternateProtocolsCacheImplTest()
: store_(new NiceMock<MockKeyValueStore>()),
protocols_(simTime(), std::unique_ptr<KeyValueStore>(store_)) {}
protocols_(simTime(), std::unique_ptr<KeyValueStore>(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;
Expand Down Expand Up @@ -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<AlternateProtocolsCache::AlternateProtocol> 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 {
Expand Down
4 changes: 2 additions & 2 deletions test/common/http/conn_pool_grid_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<AlternateProtocolsCacheImpl>(simTime(), nullptr)),
alternate_protocols_(std::make_shared<AlternateProtocolsCacheImpl>(simTime(), nullptr, 10)),
quic_stat_names_(store_.symbolTable()),
grid_(dispatcher_, random_,
Upstream::makeTestHost(cluster_, "hostname", "tcp://127.0.0.1:9000", simTime()),
Expand All @@ -120,7 +120,7 @@ class ConnectivityGridTest : public Event::TestUsingSimulatedTime, public testin
if (!use_alternate_protocols) {
return nullptr;
}
return std::make_shared<AlternateProtocolsCacheImpl>(simTime(), nullptr);
return std::make_shared<AlternateProtocolsCacheImpl>(simTime(), nullptr, 10);
}

void addHttp3AlternateProtocol() {
Expand Down