Skip to content

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

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.

Sign up for GitHub

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

Merged
alyssawilk merged 8 commits into from
Sep 28, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
38d3171
alternate_protocols_cache: Impose a max size limit on the alternate p…
RyanTheOptimist Sep 24, 2021
2ae6924
Fix comments from Renjie
RyanTheOptimist Sep 25, 2021
133ef91
Format
RyanTheOptimist Sep 25, 2021
6ae877a
fix asan
RyanTheOptimist Sep 25, 2021
becd093
linked_hash_map
RyanTheOptimist Sep 27, 2021
b450d42
format
RyanTheOptimist Sep 27, 2021
227d286
Comment
RyanTheOptimist Sep 27, 2021
581687d
spelling
RyanTheOptimist Sep 27, 2021
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
31 changes: 23 additions & 8 deletions source/common/http/alternate_protocols_cache_impl.cc
View file
Open in desktop
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,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->origin_));
protocols_map_.erase(protocols_map_.find(iter->origin_));
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<Origin, std::vector<AlternateProtocol>> 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_));
Expand All @@ -85,12 +99,12 @@ void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin,

OptRef<const std::vector<AlternateProtocolsCache::AlternateProtocol>>
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<const std::vector<AlternateProtocol>>(nullptr);
}

std::vector<AlternateProtocol>& protocols = entry_it->second;
std::vector<AlternateProtocol>& protocols = entry_it->second->protocols_;

auto original_size = protocols.size();
const MonotonicTime now = time_source_.monotonicTime();
Expand All @@ -101,7 +115,8 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) {
protocols.end());

if (protocols.empty()) {
protocols_.erase(entry_it);
protocols_list_.erase(entry_it->second);
protocols_map_.erase(entry_it);
if (key_value_store_) {
key_value_store_->remove(originToString(origin));
}
Expand All @@ -115,7 +130,7 @@ AlternateProtocolsCacheImpl::findAlternatives(const Origin& origin) {
return makeOptRef(const_cast<const std::vector<AlternateProtocol>&>(protocols));
}

size_t AlternateProtocolsCacheImpl::size() const { return protocols_.size(); }
size_t AlternateProtocolsCacheImpl::size() const { return protocols_list_.size(); }

} // namespace Http
} // namespace Envoy
19 changes: 15 additions & 4 deletions source/common/http/alternate_protocols_cache_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,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 +49,22 @@ 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 OriginProtocols {
OriginProtocols(const Origin& origin, const std::vector<AlternateProtocol>& protocols)
: origin_(origin), protocols_(protocols) {}

Origin origin_;
std::vector<AlternateProtocol> protocols_;
};
// List of origin, alternate protocol pairs, in insertion order.
std::list<OriginProtocols> protocols_list_;
// Map from hostname to iterator into protocols_list_.
std::map<Origin, std::list<OriginProtocols>::iterator> protocols_map_;

@alyssawilk alyssawilk Sep 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

since we already depend on quiche for the spdy wire parsing, what do you think of also pulling in quiche's linked hash map?

@RyanTheOptimist RyanTheOptimist Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, good call! I didn't even consider that. It's a much simpler change. Thanks! Done.


// 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
2 changes: 1 addition & 1 deletion source/common/http/alternate_protocols_cache_manager_impl.cc
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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