Skip to content

grid: Modify the AlternateProtocolsCache to specify expiration per-protocol, not per origin, to match the alt-svc spec. #16624

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 10 commits into from
May 26, 2021
Merged

grid: Modify the AlternateProtocolsCache to specify expiration per-protocol, not per origin, to match the alt-svc spec. #16624

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3490c81
grid: Modify the AlternateProtocolsCache to specify expiration per-pr…
RyanTheOptimist May 21, 2021
a2e5ae5
conn_pool_grid_test
RyanTheOptimist May 21, 2021
36bc81f
format
RyanTheOptimist May 21, 2021
7f054b5
Cleanup
RyanTheOptimist May 21, 2021
a747f35
==
RyanTheOptimist May 24, 2021
669e825
Address comments from Alyssa
RyanTheOptimist May 25, 2021
6dde521
format
RyanTheOptimist May 25, 2021
069ca4f
include
RyanTheOptimist May 25, 2021
a22d247
Truncate
RyanTheOptimist May 25, 2021
ad1e2d3
Better truncate
RyanTheOptimist May 25, 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
17 changes: 9 additions & 8 deletions include/envoy/http/alternate_protocols_cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,23 +64,26 @@ 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_) ==
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); }

std::string alpn_;
std::string hostname_;
uint32_t port_;
MonotonicTime expiration_;
};

virtual ~AlternateProtocolsCache() = default;
Expand All @@ -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<AlternateProtocol>& protocols,
const MonotonicTime& expiration) PURE;
const std::vector<AlternateProtocol>& protocols) PURE;

/**
* Returns the possible alternative protocols which can be used to connect to the
Expand Down
31 changes: 17 additions & 14 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 @@ -9,15 +9,8 @@ AlternateProtocolsCacheImpl::AlternateProtocolsCacheImpl(TimeSource& time_source
AlternateProtocolsCacheImpl::~AlternateProtocolsCacheImpl() = default;

void AlternateProtocolsCacheImpl::setAlternatives(const Origin& origin,
const std::vector<AlternateProtocol>& 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<AlternateProtocol>& protocols) {
protocols_[origin] = protocols;
}

OptRef<const std::vector<AlternateProtocolsCache::AlternateProtocol>>
Expand All @@ -28,15 +21,25 @@ 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.
std::vector<AlternateProtocol>& protocols = entry_it->second;

const MonotonicTime now = time_source_.monotonicTime();
auto it = protocols.begin();
Comment thread
RyanTheOptimist marked this conversation as resolved.
Outdated
while (it != protocols.end()) {
Comment thread
RyanTheOptimist marked this conversation as resolved.
Outdated
if (now > it->expiration_) {
it = protocols.erase(it);
continue;
}
++it;
}

if (protocols.empty()) {
protocols_.erase(entry_it);
return makeOptRefFromPtr<const std::vector<AlternateProtocolsCache::AlternateProtocol>>(
nullptr);
}
return makeOptRef(entry.protocols_);

return makeOptRef(const_cast<const std::vector<AlternateProtocol>&>(protocols));
}

size_t AlternateProtocolsCacheImpl::size() const { return protocols_.size(); }
Expand Down
11 changes: 3 additions & 8 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 @@ -22,23 +22,18 @@ class AlternateProtocolsCacheImpl : public AlternateProtocolsCache {
~AlternateProtocolsCacheImpl() override;

// AlternateProtocolsCache
void setAlternatives(const Origin& origin, const std::vector<AlternateProtocol>& protocols,
const MonotonicTime& expiration) override;
void setAlternatives(const Origin& origin,
const std::vector<AlternateProtocol>& protocols) override;
OptRef<const std::vector<AlternateProtocol>> findAlternatives(const Origin& origin) override;
size_t size() const override;

private:
struct Entry {
std::vector<AlternateProtocol> 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<Origin, Entry> protocols_;
std::map<Origin, std::vector<AlternateProtocol>> protocols_;
};

} // namespace Http
Expand Down
35 changes: 23 additions & 12 deletions 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 @@ -23,38 +23,40 @@ 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<AlternateProtocolsCacheImpl::AlternateProtocol> protocols1_ = {protocol1_};
const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol> 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<const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol>> protocols =
protocols_.findAlternatives(origin1_);
ASSERT_TRUE(protocols.has_value());
EXPECT_EQ(protocols1_, protocols.ref());
}

TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterReplacement) {
protocols_.setAlternatives(origin1_, protocols1_, expiration1_);
protocols_.setAlternatives(origin1_, protocols2_, expiration2_);
protocols_.setAlternatives(origin1_, protocols1_);
protocols_.setAlternatives(origin1_, protocols2_);
OptRef<const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol>> protocols =
protocols_.findAlternatives(origin1_);
ASSERT_TRUE(protocols.has_value());
Expand All @@ -63,8 +65,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<const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol>> protocols =
protocols_.findAlternatives(origin1_);
ASSERT_TRUE(protocols.has_value());
Expand All @@ -75,14 +77,23 @@ TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesForMultipleOrigins) {
}

TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterExpiration) {
protocols_.setAlternatives(origin1_, protocols1_, expiration1_);
protocols_.setAlternatives(origin1_, protocols1_);
simTime().setMonotonicTime(expiration1_ + Seconds(1));
OptRef<const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol>> protocols =
protocols_.findAlternatives(origin1_);
ASSERT_FALSE(protocols.has_value());
EXPECT_EQ(0, protocols_.size());
}

TEST_F(AlternateProtocolsCacheImplTest, FindAlternativesAfterPartialExpiration) {
protocols_.setAlternatives(origin1_, {protocol1_, protocol2_});
simTime().setMonotonicTime(expiration1_ + Seconds(1));
OptRef<const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol>> protocols =
protocols_.findAlternatives(origin1_);
ASSERT_TRUE(protocols.has_value());
EXPECT_EQ(protocols2_, protocols.ref());
}

} // namespace
} // namespace Http
} // namespace Envoy
21 changes: 10 additions & 11 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 @@ -122,9 +122,8 @@ class ConnectivityGridTestBase : public Event::TestUsingSimulatedTime, public te
void addHttp3AlternateProtocol() {
AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000);
const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol> 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_;
Expand Down Expand Up @@ -513,8 +512,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3)
TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithExpiredHttp3) {
AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000);
const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol> 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);
Expand All @@ -536,8 +535,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithExpiredHt
TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingHostname) {
AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000);
const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol> 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);

Expand All @@ -557,8 +556,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3N
TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingPort) {
AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000);
const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol> 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);

Expand All @@ -577,8 +576,8 @@ TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3N
TEST_F(ConnectivityGridWithAlternateProtocolsCacheImplTest, SuccessWithoutHttp3NoMatchingAlpn) {
AlternateProtocolsCacheImpl::Origin origin("https", "hostname", 9000);
const std::vector<AlternateProtocolsCacheImpl::AlternateProtocol> 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);

Expand Down