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

quic: use sds for upstream http/3 #16462

Merged
merged 5 commits into from
May 19, 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
18 changes: 16 additions & 2 deletions source/common/conn_pool/conn_pool_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ ConnPoolImplBase::tryCreateNewConnection(float global_preconnect_ratio) {
(ready_clients_.empty() && busy_clients_.empty() && connecting_clients_.empty())) {
ENVOY_LOG(debug, "creating a new connection");
ActiveClientPtr client = instantiateActiveClient();
if (client.get() == nullptr) {
ENVOY_LOG(trace, "connection creation failed");
return ConnectionResult::FailedToCreateConnection;
}
ASSERT(client->state() == ActiveClient::State::CONNECTING);
ASSERT(std::numeric_limits<uint64_t>::max() - connecting_stream_capacity_ >=
client->effectiveConcurrentStreamLimit());
Expand Down Expand Up @@ -249,9 +253,19 @@ ConnectionPool::Cancellable* ConnPoolImplBase::newStream(AttachContext& context)
// increase capacity is if the connection limits are exceeded.
ENVOY_BUG(pending_streams_.size() <= connecting_stream_capacity_ ||
connecting_stream_capacity_ > old_capacity ||
result == ConnectionResult::NoConnectionRateLimited,
(result == ConnectionResult::NoConnectionRateLimited ||
result == ConnectionResult::FailedToCreateConnection),
fmt::format("Failed to create expected connection: {}", *this));
return pending;
if (result == ConnectionResult::FailedToCreateConnection) {
// This currently only happens for HTTP/3 if secrets aren't yet loaded.
// Trigger connection failure.
pending->cancel(Envoy::ConnectionPool::CancelPolicy::CloseExcess);
onPoolFailure(nullptr, absl::string_view(), ConnectionPool::PoolFailureReason::Overflow,
context);
return nullptr;
} else {
return pending;
}
} else {
ENVOY_LOG(debug, "max pending streams overflow");
onPoolFailure(nullptr, absl::string_view(), ConnectionPool::PoolFailureReason::Overflow,
Expand Down
1 change: 1 addition & 0 deletions source/common/conn_pool/conn_pool_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ class ConnPoolImplBase : protected Logger::Loggable<Logger::Id::pool> {
virtual void onConnected(Envoy::ConnectionPool::ActiveClient&) {}

enum class ConnectionResult {
FailedToCreateConnection,
CreatedNewConnection,
ShouldNotConnect,
NoConnectionRateLimited,
Expand Down
9 changes: 8 additions & 1 deletion source/common/http/http3/conn_pool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#ifdef ENVOY_ENABLE_QUIC
#include "common/quic/client_connection_factory_impl.h"
#include "common/quic/envoy_quic_utils.h"
#include "common/quic/quic_transport_socket_factory.h"
#else
#error "http3 conn pool should not be built with QUIC disabled"
#endif
Expand Down Expand Up @@ -64,7 +65,13 @@ allocateConnPool(Event::Dispatcher& dispatcher, Random::RandomGenerator& random_
Upstream::ClusterConnectivityState& state, TimeSource& time_source) {
return std::make_unique<Http3ConnPoolImpl>(
host, priority, dispatcher, options, transport_socket_options, random_generator, state,
[](HttpConnPoolImplBase* pool) {
[](HttpConnPoolImplBase* pool) -> ::Envoy::ConnectionPool::ActiveClientPtr {
// If there's no ssl context, the secrets are not loaded. Fast-fail by returning null.
auto factory = &pool->host()->transportSocketFactory();
ASSERT(dynamic_cast<Quic::QuicClientTransportSocketFactory*>(factory) != nullptr);
if (static_cast<Quic::QuicClientTransportSocketFactory*>(factory)->sslCtx() == nullptr) {
return nullptr;
}
Http3ConnPoolImpl* h3_pool = reinterpret_cast<Http3ConnPoolImpl*>(pool);
Upstream::Host::CreateConnectionData data{};
data.host_description_ = pool->host();
Expand Down
31 changes: 25 additions & 6 deletions source/common/quic/client_connection_factory_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,34 @@ getContext(Network::TransportSocketFactory& transport_socket_factory) {
auto* quic_socket_factory =
dynamic_cast<QuicClientTransportSocketFactory*>(&transport_socket_factory);
ASSERT(quic_socket_factory != nullptr);
ASSERT(quic_socket_factory->sslCtx() != nullptr);
return quic_socket_factory->sslCtx();
}

std::shared_ptr<quic::QuicCryptoClientConfig> PersistentQuicInfoImpl::cryptoConfig() {
Copy link
Contributor

@danzh2010 danzh2010 May 12, 2021

Choose a reason for hiding this comment

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

This is somewhat laid-back updating. If a client connection is created before the SSL context update, will its crypto config not be updated till another createQuicNetworkConnection() is called?
Could this be a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think once a quic client is created we support reloading, so AFIK it'd only be applied when we created a new client anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK.
Another question is if we need to copy over resumption tickets stored in EnvoyQuicSessionCache before replacing crypto config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would think if we changed client certs we'd not want to keep resumption info? cc @RyanTheOptimist

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that sounds right to me.

auto context = getContext(transport_socket_factory_);
// If the secrets haven't been loaded, there is no crypto config.
if (context == nullptr) {
return nullptr;
}

// If the secret has been updated, update the proof source.
if (context.get() != client_context_.get()) {
client_context_ = context;
client_config_ = std::make_shared<quic::QuicCryptoClientConfig>(
std::make_unique<EnvoyQuicProofVerifier>(getContext(transport_socket_factory_)),
std::make_unique<EnvoyQuicSessionCache>((time_source_)));
}
// Return the latest client config.
return client_config_;
}

PersistentQuicInfoImpl::PersistentQuicInfoImpl(
Event::Dispatcher& dispatcher, Network::TransportSocketFactory& transport_socket_factory,
TimeSource& time_source, Network::Address::InstanceConstSharedPtr server_addr)
: conn_helper_(dispatcher), alarm_factory_(dispatcher, *conn_helper_.GetClock()),
server_id_{getConfig(transport_socket_factory).serverNameIndication(),
static_cast<uint16_t>(server_addr->ip()->port()), false},
crypto_config_(std::make_unique<quic::QuicCryptoClientConfig>(
std::make_unique<EnvoyQuicProofVerifier>(getContext(transport_socket_factory)),
std::make_unique<EnvoyQuicSessionCache>(time_source))) {
transport_socket_factory_(transport_socket_factory), time_source_(time_source) {
quiche::FlagRegistry::getInstance();
}

Expand All @@ -53,6 +68,10 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d
// This flag fix a QUICHE issue which may crash Envoy during connection close.
SetQuicReloadableFlag(quic_single_ack_in_packet2, true);
PersistentQuicInfoImpl* info_impl = reinterpret_cast<PersistentQuicInfoImpl*>(&info);
auto config = info_impl->cryptoConfig();
if (config == nullptr) {
return nullptr; // no secrets available yet.
}

auto connection = std::make_unique<EnvoyQuicClientConnection>(
quic::QuicUtils::CreateRandomConnectionId(), server_addr, info_impl->conn_helper_,
Expand All @@ -66,8 +85,8 @@ createQuicNetworkConnection(Http::PersistentQuicInfo& info, Event::Dispatcher& d
// send_buffer_limit instead of using 0.
auto ret = std::make_unique<EnvoyQuicClientSession>(
info_impl->quic_config_, info_impl->supported_versions_, std::move(connection),
info_impl->server_id_, info_impl->crypto_config_.get(), &static_info.push_promise_index_,
dispatcher, /*send_buffer_limit=*/0);
info_impl->server_id_, std::move(config), &static_info.push_promise_index_, dispatcher,
/*send_buffer_limit=*/0);
return ret;
}

Expand Down
20 changes: 15 additions & 5 deletions source/common/quic/client_connection_factory_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,25 @@ struct PersistentQuicInfoImpl : public Http::PersistentQuicInfo {
TimeSource& time_source,
Network::Address::InstanceConstSharedPtr server_addr);

// Returns the most recent crypto config from transport_socket_factory_;
std::shared_ptr<quic::QuicCryptoClientConfig> cryptoConfig();

EnvoyQuicConnectionHelper conn_helper_;
EnvoyQuicAlarmFactory alarm_factory_;
// server-id and server address can change over the lifetime of Envoy but will be consistent for a
// server-id can change over the lifetime of Envoy but will be consistent for a
// given connection pool.
quic::QuicServerId server_id_;
quic::ParsedQuicVersionVector supported_versions_{quic::CurrentSupportedVersions()};
// TODO(danzh) move this into client transport socket factory so that it can
// be updated with SDS.
std::unique_ptr<quic::QuicCryptoClientConfig> crypto_config_;
// Latch the transport socket factory, to get the latest crypto config and the
// time source to create it.
Network::TransportSocketFactory& transport_socket_factory_;
TimeSource& time_source_;
// Latch the latest crypto config, to determine if it has updated since last
// checked.
Envoy::Ssl::ClientContextSharedPtr client_context_;
// If client context changes, client config will be updated as well.
std::shared_ptr<quic::QuicCryptoClientConfig> client_config_;
const quic::ParsedQuicVersionVector supported_versions_{quic::CurrentSupportedVersions()};
// TODO(alyssawilk) actually set this up properly.
quic::QuicConfig quic_config_;
};

Expand Down
6 changes: 3 additions & 3 deletions source/common/quic/envoy_quic_client_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ namespace Quic {
EnvoyQuicClientSession::EnvoyQuicClientSession(
const quic::QuicConfig& config, const quic::ParsedQuicVersionVector& supported_versions,
std::unique_ptr<EnvoyQuicClientConnection> connection, const quic::QuicServerId& server_id,
quic::QuicCryptoClientConfig* crypto_config,
std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config,
quic::QuicClientPushPromiseIndex* push_promise_index, Event::Dispatcher& dispatcher,
uint32_t send_buffer_limit)
: QuicFilterManagerConnectionImpl(*connection, connection->connection_id(), dispatcher,
send_buffer_limit),
quic::QuicSpdyClientSession(config, supported_versions, connection.release(), server_id,
crypto_config, push_promise_index),
host_name_(server_id.host()) {}
crypto_config.get(), push_promise_index),
host_name_(server_id.host()), crypto_config_(crypto_config) {}

EnvoyQuicClientSession::~EnvoyQuicClientSession() {
ASSERT(!connection()->connected());
Expand Down
3 changes: 2 additions & 1 deletion source/common/quic/envoy_quic_client_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl,
const quic::ParsedQuicVersionVector& supported_versions,
std::unique_ptr<EnvoyQuicClientConnection> connection,
const quic::QuicServerId& server_id,
quic::QuicCryptoClientConfig* crypto_config,
std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config,
quic::QuicClientPushPromiseIndex* push_promise_index,
Event::Dispatcher& dispatcher, uint32_t send_buffer_limit);

Expand Down Expand Up @@ -86,6 +86,7 @@ class EnvoyQuicClientSession : public QuicFilterManagerConnectionImpl,
// them.
Http::ConnectionCallbacks* http_connection_callbacks_{nullptr};
const absl::string_view host_name_;
std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config_;
};

} // namespace Quic
Expand Down
10 changes: 3 additions & 7 deletions source/common/quic/quic_transport_socket_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,7 @@ class QuicClientTransportSocketFactory : public QuicTransportSocketFactoryBase {
Ssl::ClientContextConfigPtr config,
Server::Configuration::TransportSocketFactoryContext& factory_context);

void initialize() override {
// TODO(14829) fallback_factory_ needs to call onSecretUpdated() upon SDS update.
}
void initialize() override {}

// As documented above for QuicTransportSocketFactoryBase, the actual HTTP/3
// code does not create transport sockets.
Expand All @@ -118,10 +116,8 @@ class QuicClientTransportSocketFactory : public QuicTransportSocketFactoryBase {
}

protected:
void onSecretUpdated() override {
// fallback_factory_ will update the stats.
// TODO(14829) Client transport socket factory may also need to update quic crypto.
}
// fallback_factory_ will update the context.
void onSecretUpdated() override {}

private:
// The QUIC client transport socket can create TLS sockets for fallback to TCP.
Expand Down
9 changes: 5 additions & 4 deletions test/common/quic/envoy_quic_client_session_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class TestEnvoyQuicClientSession : public EnvoyQuicClientSession {
const quic::ParsedQuicVersionVector& supported_versions,
std::unique_ptr<EnvoyQuicClientConnection> connection,
const quic::QuicServerId& server_id,
quic::QuicCryptoClientConfig* crypto_config,
std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config,
quic::QuicClientPushPromiseIndex* push_promise_index,
Event::Dispatcher& dispatcher, uint32_t send_buffer_limit)
: EnvoyQuicClientSession(config, supported_versions, std::move(connection), server_id,
Expand Down Expand Up @@ -109,10 +109,11 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<bool> {
quic_connection_(new TestEnvoyQuicClientConnection(
quic::test::TestConnectionId(), connection_helper_, alarm_factory_, writer_,
quic_version_, *dispatcher_, createConnectionSocket(peer_addr_, self_addr_, nullptr))),
crypto_config_(quic::test::crypto_test_utils::ProofVerifierForTesting()),
crypto_config_(std::make_shared<quic::QuicCryptoClientConfig>(
quic::test::crypto_test_utils::ProofVerifierForTesting())),
envoy_quic_session_(quic_config_, quic_version_,
std::unique_ptr<TestEnvoyQuicClientConnection>(quic_connection_),
quic::QuicServerId("example.com", 443, false), &crypto_config_, nullptr,
quic::QuicServerId("example.com", 443, false), crypto_config_, nullptr,
*dispatcher_,
/*send_buffer_limit*/ 1024 * 1024),
stats_({ALL_HTTP3_CODEC_STATS(POOL_COUNTER_PREFIX(scope_, "http3."),
Expand Down Expand Up @@ -173,7 +174,7 @@ class EnvoyQuicClientSessionTest : public testing::TestWithParam<bool> {
Network::Address::InstanceConstSharedPtr self_addr_;
TestEnvoyQuicClientConnection* quic_connection_;
quic::QuicConfig quic_config_;
quic::QuicCryptoClientConfig crypto_config_;
std::shared_ptr<quic::QuicCryptoClientConfig> crypto_config_;
TestEnvoyQuicClientSession envoy_quic_session_;
Network::MockConnectionCallbacks network_connection_callbacks_;
Http::MockServerConnectionCallbacks http_connection_callbacks_;
Expand Down
3 changes: 2 additions & 1 deletion test/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,8 @@ void ConfigHelper::setProtocolOptions(envoy::config::cluster::v3::Cluster& clust
HttpProtocolOptions old_options = MessageUtil::anyConvert<ConfigHelper::HttpProtocolOptions>(
(*cluster.mutable_typed_extension_protocol_options())
["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"]);
protocol_options.MergeFrom(old_options);
old_options.MergeFrom(protocol_options);
protocol_options.CopyFrom(old_options);
}
(*cluster.mutable_typed_extension_protocol_options())
["envoy.extensions.upstreams.http.v3.HttpProtocolOptions"]
Expand Down
2 changes: 1 addition & 1 deletion test/integration/quic_http_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class QuicHttpIntegrationTest : public HttpIntegrationTest, public QuicMultiVers
auto& persistent_info = static_cast<PersistentQuicInfoImpl&>(*quic_connection_persistent_info_);
auto session = std::make_unique<EnvoyQuicClientSession>(
persistent_info.quic_config_, supported_versions_, std::move(connection),
persistent_info.server_id_, persistent_info.crypto_config_.get(), &push_promise_index_,
persistent_info.server_id_, persistent_info.cryptoConfig(), &push_promise_index_,
*dispatcher_,
// Use smaller window than the default one to have test coverage of client codec buffer
// exceeding high watermark.
Expand Down
Loading