http3: applying config options upstream#16532
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Sweet! Looks great!
source/common/http/http3/conn_pool.h
Outdated
| namespace Http3 { | ||
| class Http3ConnPoolImplTest; | ||
|
|
||
| void setQuicConfigFromClusterConfig(const Upstream::ClusterInfo& cluster, |
There was a problem hiding this comment.
Perhaps a comment on this method?
source/common/http/http3/conn_pool.h
Outdated
| Upstream::ClusterConnectivityState& state, CreateClientFn client_fn, | ||
| CreateCodecFn codec_fn, std::vector<Http::Protocol> protocol, | ||
| TimeSource& time_source) | ||
| : FixedHttpConnPoolImpl(host, priority, dispatcher, options, transport_socket_options, |
There was a problem hiding this comment.
Your call, but I'd be inclined to move the body of both the constructor and destructor to the .cc file.
| envoy_cc_test( | ||
| name = "conn_pool_test", | ||
| srcs = envoy_select_enable_http3(["conn_pool_test.cc"]), | ||
| tags = ["nofips"], |
There was a problem hiding this comment.
nofips? As in the compliance standard?
There was a problem hiding this comment.
yeah. they did a fips version of boring a year or two back but we need a more modern version of boringssl for some of the QUIC crypto functions, so our QUIC impl is not compatible with Envoy builds latched to the fips boring.
source/common/http/http3/conn_pool.h
Outdated
| Network::TransportSocketFactory& transport_socket_factory = host->transportSocketFactory(); | ||
| quic_info_ = std::make_unique<Quic::PersistentQuicInfoImpl>( | ||
| dispatcher, transport_socket_factory, time_source, source_address); | ||
| setQuicConfigFromClusterConfig(host_->cluster(), quic_info_->quic_config_); |
There was a problem hiding this comment.
It looks like this method is only called from the constructor. Do we have other places we expect to call it from? If not, perhaps we should just inline it into the constructor?
There was a problem hiding this comment.
I think it's slightly cleaner as a utility, but I could make it a static method in the pool to make usage a bit more clear?
source/common/http/http3/conn_pool.h
Outdated
| setQuicConfigFromClusterConfig(host_->cluster(), quic_info_->quic_config_); | ||
| } | ||
|
|
||
| Quic::PersistentQuicInfoImpl& quicInfo() { return *quic_info_; } |
There was a problem hiding this comment.
Should this be a const ref, or do we intend callers to modify it?
There was a problem hiding this comment.
this is basically a container of things passed to quic client sessions as they're created. since many of the items like the crypto config are non-const I think the into needs to be non-const as well.
| initialize(); | ||
|
|
||
| Quic::PersistentQuicInfoImpl& info = static_cast<Http3ConnPoolImpl*>(pool_.get())->quicInfo(); | ||
| EXPECT_EQ(info.quic_config_.GetMaxUnidirectionalStreamsToSend(), 15); |
There was a problem hiding this comment.
nit: Instead of repeating the numbers, could we do:
options->mutable_max_concurrent_streams()->value()?
|
@lizan PTAL |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
source/common/http/http3/conn_pool.h
Outdated
| namespace Envoy { | ||
| namespace Http { | ||
| namespace Http3 { | ||
| class Http3ConnPoolImplTest; |
There was a problem hiding this comment.
Http3ConnPoolImplTest is not used in this header?
|
LGTM, modulo small cleanup comment from Lizan. /wait |
Risk Level: low (http/3 only) Testing: new unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
4 participants
Risk Level: low (http/3 only)
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a