-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
quiche: implement ActiveQuicListener #7896
Changes from 93 commits
3253b11
9914fb4
2ea58dd
a8aa262
6b5315b
02e4fd9
6f8c2e4
58f3842
8fec99d
15e5e33
72c2f84
25998ab
2fd6df0
aacc588
f68e5dd
12ef3c7
6029aeb
14f1e79
2fccff1
ef7cf28
5419162
117244f
1719fd0
732b3c8
f94f424
0d56de4
bacb5c5
f04a469
9ba2754
05daf8d
6eca9c6
b8fc520
f5e0c11
b881ae8
3db6847
1e55f68
ab474d5
5f7dd41
fcfe160
9dc44be
7a2395f
e9d66fa
3e4388b
2397898
0641fba
a5f8ba3
30d976b
a50fc1e
dbda7ef
ed15996
8b297e4
adbdee0
3dc7365
57144ed
c9abaa5
d278167
1b8117b
8375028
6361860
4e51861
44e262f
f2fc291
97b7d90
7361ca0
10e85f6
5f55a97
a4050dc
81e5f9a
109e9a9
9d545c5
4e91aae
7c863ef
c6f0763
bca0450
42b5df3
8891a14
9a5468e
1d23053
817c08d
5987586
3405bfc
548f8f5
b218f5a
3deb19e
be37cb1
d7c551f
bb1e435
799c5fa
c617a40
7618f8b
6a58153
c8428df
8e4d2a6
4b3d7be
11b653f
dcbb7d0
6ecaff1
fe5a5fa
dfc26fd
c7e416d
5b317be
32d0e78
2b0273f
2a6efb5
94dc9a3
ce75217
e31e933
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
syntax = "proto3"; | ||
|
||
package envoy.api.v2.listener; | ||
|
||
option java_outer_classname = "ListenerProto"; | ||
option java_multiple_files = true; | ||
option java_package = "io.envoyproxy.envoy.api.v2.listener"; | ||
option csharp_namespace = "Envoy.Api.V2.ListenerNS"; | ||
option ruby_package = "Envoy::Api::V2::ListenerNS"; | ||
|
||
// Configuration specific to the QUIC protocol. | ||
// Next id: 4 | ||
message QuicConfigProto { | ||
// Maximum number of streams that the client can negotiate per connection. 100 | ||
// if not specified. | ||
int32 max_streams_per_connection = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also let's model naming off of existing config where we can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I include envoy::api::v2::core::Http2ProtocolOptions here instead of duplicating the same fields? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 please reuse messages where we can, though per the comment above if there are HPACK options in Http2ProtocolOptions I'm not sure if we will be able to do it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about defining stand alone fields here for now and consolidate with H2/TCP Proto options later? |
||
|
||
// Maximum number of milliseconds that connection will be alive when there is | ||
// no network activity. 300000ms if not specified. | ||
int32 idle_network_timeout_ms = 2; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is comparable to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reuse TcpProxy message or define our own here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TcpProxy message is for raw tcp proxying, (including things like upstream (backend) configuration) so I don't think we want to reuse the proto, just have similar names. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
// Connection timeout before the crypto handshake is finished. 20s if not | ||
// specified. | ||
int32 max_time_before_crypto_handshake_ms = 3; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was assuming there was an equivalent for TLS but I'm not seeing it. @PiotrSikora am I missing this or do we not configure handshake specific timeouts? Either way should probably be a Duration There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe we have one today, it's just covered under the general idle timeout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, if this is the first of its kind we can't totally copy naming, but I'd lean towards having timeout in there somewhere. handshake_timeout? crypto_handshake_timeout? I think we've gone for "idle timeout" for keepalive style timeouts, and just "timeout" for overall timeouts, like http connection manager's request_timeout for overall request duration There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed it to crypto_handshake_timeout |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
#include "extensions/quic_listeners/quiche/active_quic_listener.h" | ||
|
||
#include "extensions/quic_listeners/quiche/envoy_quic_alarm_factory.h" | ||
#include "extensions/quic_listeners/quiche/envoy_quic_connection_helper.h" | ||
#include "extensions/quic_listeners/quiche/envoy_quic_dispatcher.h" | ||
#include "extensions/quic_listeners/quiche/envoy_quic_fake_proof_source.h" | ||
#include "extensions/quic_listeners/quiche/envoy_quic_packet_writer.h" | ||
#include "extensions/quic_listeners/quiche/envoy_quic_utils.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
ActiveQuicListener::ActiveQuicListener(Event::Dispatcher& dispatcher, | ||
Network::ConnectionHandler& parent, spdlog::logger& logger, | ||
Network::ListenerConfig& listener_config, | ||
const quic::QuicConfig& quic_config) | ||
: ActiveQuicListener(dispatcher, parent, | ||
dispatcher.createUdpListener(listener_config.socket(), *this), logger, | ||
listener_config, quic_config) {} | ||
|
||
ActiveQuicListener::ActiveQuicListener(Event::Dispatcher& dispatcher, | ||
Network::ConnectionHandler& parent, | ||
Network::ListenerPtr&& listener, spdlog::logger& logger, | ||
Network::ListenerConfig& listener_config, | ||
const quic::QuicConfig& quic_config) | ||
: Server::ConnectionHandlerImpl::ActiveListenerImplBase(std::move(listener), listener_config), | ||
logger_(logger), dispatcher_(dispatcher), version_manager_(quic::CurrentSupportedVersions()) { | ||
quic::QuicRandom* const random = quic::QuicRandom::GetInstance(); | ||
random->RandBytes(random_seed_, sizeof(random_seed_)); | ||
crypto_config_ = std::make_unique<quic::QuicCryptoServerConfig>( | ||
quic::QuicStringPiece(reinterpret_cast<char*>(random_seed_), sizeof(random_seed_)), | ||
quic::QuicRandom::GetInstance(), std::make_unique<EnvoyQuicFakeProofSource>(), | ||
quic::KeyExchangeSource::Default()); | ||
auto connection_helper = std::make_unique<EnvoyQuicConnectionHelper>(dispatcher_); | ||
auto alarm_factory = | ||
std::make_unique<EnvoyQuicAlarmFactory>(dispatcher_, *connection_helper->GetClock()); | ||
quic_dispatcher_ = std::make_unique<EnvoyQuicDispatcher>( | ||
crypto_config_.get(), quic_config, &version_manager_, std::move(connection_helper), | ||
std::move(alarm_factory), quic::kQuicDefaultConnectionIdLength, parent, config_, stats_, | ||
dispatcher); | ||
quic_dispatcher_->InitializeWithWriter( | ||
new EnvoyQuicPacketWriter(*dynamic_cast<Network::UdpListener*>(listener_.get()))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a TODO here to try to get rid of the dynamic cast? I will try to look at this myself and provide some guidance instead of just complaining. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is easy to remove actually. I modified Dispatcher interface and ActiveQuicListener constructor a bit. Done |
||
} | ||
|
||
void ActiveQuicListener::onListenerShutdown() { | ||
ENVOY_LOG_TO_LOGGER(logger_, info, "Listener shutdown."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can we make this log message more useful? Especially if it stays info level? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a once per lifetime message, so I think it worth to be at info level. I added the listener tag in the log. Would it be more useful then? |
||
quic_dispatcher_->Shutdown(); | ||
} | ||
|
||
void ActiveQuicListener::onData(Network::UdpRecvData& data) { | ||
quic::QuicSocketAddress peer_address(envoyAddressInstanceToQuicSocketAddress(data.peer_address_)); | ||
quic::QuicSocketAddress self_address( | ||
envoyAddressInstanceToQuicSocketAddress(data.local_address_)); | ||
quic::QuicTime timestamp = | ||
quic::QuicTime::Zero() + | ||
quic::QuicTime::Delta::FromMilliseconds(std::chrono::duration_cast<std::chrono::milliseconds>( | ||
data.receive_time_.time_since_epoch()) | ||
.count()); | ||
uint64_t num_slice = data.buffer_->getRawSlices(nullptr, 0); | ||
ASSERT(num_slice == 1); | ||
Buffer::RawSlice slice; | ||
data.buffer_->getRawSlices(&slice, 1); | ||
// TODO(danzh): pass in TTL and UDP header. | ||
quic::QuicReceivedPacket packet(reinterpret_cast<char*>(slice.mem_), slice.len_, timestamp, | ||
/*owns_buffer=*/false, /*ttl=*/0, /*ttl_valid=*/true, | ||
/*packet_headers=*/nullptr, /*headers_length=*/0, | ||
/*owns_header_buffer*/ false); | ||
quic_dispatcher_->ProcessPacket(self_address, peer_address, packet); | ||
} | ||
|
||
void ActiveQuicListener::onWriteReady(const Network::Socket& /*socket*/) { | ||
quic_dispatcher_->OnCanWrite(); | ||
} | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
#pragma once | ||
|
||
#include "envoy/api/v2/listener/quic_config.pb.h" | ||
#include "envoy/network/connection_handler.h" | ||
#include "envoy/network/listener.h" | ||
|
||
#include "server/connection_handler_impl.h" | ||
|
||
#include "extensions/quic_listeners/quiche/envoy_quic_dispatcher.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
// QUIC specific UdpListenerCallbacks implemention which delegates incoming | ||
// packets, write signals and listener errors to QuicDispatcher. | ||
class ActiveQuicListener : public Network::UdpListenerCallbacks, | ||
public Server::ConnectionHandlerImpl::ActiveListenerImplBase, | ||
// Inherits below two interfaces just to have common | ||
mattklein123 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// interfaces. Not expected to support listener | ||
// filter. | ||
public Network::UdpListenerFilterManager, | ||
public Network::UdpReadFilterCallbacks { | ||
public: | ||
ActiveQuicListener(Event::Dispatcher& dispatcher, Network::ConnectionHandler& parent, | ||
spdlog::logger& logger, Network::ListenerConfig& listener_config, | ||
const quic::QuicConfig& quic_config); | ||
|
||
ActiveQuicListener(Event::Dispatcher& dispatcher, Network::ConnectionHandler& parent, | ||
Network::ListenerPtr&& listener, spdlog::logger& logger, | ||
Network::ListenerConfig& listener_config, const quic::QuicConfig& quic_config); | ||
// TODO(#7465): Make this a callback. | ||
void onListenerShutdown(); | ||
|
||
// Network::UdpListenerCallbacks | ||
void onData(Network::UdpRecvData& data) override; | ||
void onWriteReady(const Network::Socket& socket) override; | ||
void onReceiveError(const Network::UdpListenerCallbacks::ErrorCode& /*error_code*/, | ||
Api::IoError::IoErrorCode /*err*/) override { | ||
// No-op. Quic can't do anything upon listener error. | ||
} | ||
|
||
// Network::UdpListenerFilterManager | ||
void addReadFilter(Network::UdpListenerReadFilterPtr&& /*filter*/) override { | ||
// QUIC doesn't support listener filter. | ||
NOT_REACHED_GCOVR_EXCL_LINE; | ||
} | ||
|
||
// Network::UdpReadFilterCallbacks | ||
Network::UdpListener& udpListener() override { NOT_REACHED_GCOVR_EXCL_LINE; } | ||
|
||
private: | ||
friend class ActiveQuicListenerPeer; | ||
|
||
uint8_t random_seed_[16]; | ||
std::unique_ptr<quic::QuicCryptoServerConfig> crypto_config_; | ||
spdlog::logger& logger_; | ||
Event::Dispatcher& dispatcher_; | ||
quic::QuicVersionManager version_manager_; | ||
std::unique_ptr<EnvoyQuicDispatcher> quic_dispatcher_; | ||
}; | ||
|
||
using ActiveQuicListenerPtr = std::unique_ptr<ActiveQuicListener>; | ||
|
||
// A factory to create ActiveQuicListener based on given config. | ||
class ActiveQuicListenerFactory : public Network::ActiveUdpListenerFactory { | ||
public: | ||
ActiveQuicListenerFactory(const envoy::api::v2::listener::QuicConfigProto& config) { | ||
int32_t idle_network_timeout_ms = | ||
config.idle_network_timeout_ms() == 0 ? 300000 : config.idle_network_timeout_ms(); | ||
quic_config_.SetIdleNetworkTimeout( | ||
quic::QuicTime::Delta::FromMilliseconds(idle_network_timeout_ms), | ||
quic::QuicTime::Delta::FromMilliseconds(idle_network_timeout_ms)); | ||
int32_t max_time_before_crypto_handshake_ms = | ||
config.max_time_before_crypto_handshake_ms() == 0 | ||
? 20000 | ||
: config.max_time_before_crypto_handshake_ms(); | ||
quic_config_.set_max_time_before_crypto_handshake( | ||
quic::QuicTime::Delta::FromMilliseconds(max_time_before_crypto_handshake_ms)); | ||
int32_t max_streams = | ||
config.max_streams_per_connection() == 0 ? 100 : config.max_streams_per_connection(); | ||
quic_config_.SetMaxIncomingBidirectionalStreamsToSend(max_streams); | ||
quic_config_.SetMaxIncomingUnidirectionalStreamsToSend(max_streams); | ||
} | ||
|
||
Network::ConnectionHandler::ActiveListenerPtr | ||
createActiveUdpListener(Network::ConnectionHandler& parent, Event::Dispatcher& disptacher, | ||
spdlog::logger& logger, Network::ListenerConfig& config) const override { | ||
return std::make_unique<ActiveQuicListener>(disptacher, parent, logger, config, quic_config_); | ||
} | ||
|
||
private: | ||
friend class ActiveQuicListenerFactoryPeer; | ||
|
||
quic::QuicConfig quic_config_; | ||
}; | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
#include "extensions/quic_listeners/quiche/active_quic_listener_config.h" | ||
|
||
#include "envoy/api/v2/listener/quic_config.pb.h" | ||
|
||
#include "server/well_known_names.h" | ||
|
||
#include "extensions/quic_listeners/quiche/active_quic_listener.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
ProtobufTypes::MessagePtr ActiveQuicListenerConfigFactory::createEmptyConfigProto() { | ||
return std::make_unique<envoy::api::v2::listener::QuicConfigProto>(); | ||
} | ||
|
||
Network::ActiveUdpListenerFactoryPtr | ||
ActiveQuicListenerConfigFactory::createActiveUdpListenerFactory(const Protobuf::Message& message) { | ||
auto& config = dynamic_cast<const envoy::api::v2::listener::QuicConfigProto&>(message); | ||
return std::make_unique<ActiveQuicListenerFactory>(config); | ||
} | ||
|
||
std::string ActiveQuicListenerConfigFactory::name() { return Server::UdpListenerNames::get().Quic; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name can now be moved out of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what inline means here. The string is also used in tests. So I added a statistic const in the .h file. |
||
|
||
REGISTER_FACTORY(ActiveQuicListenerConfigFactory, Server::ActiveUdpListenerConfigFactory); | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
#pragma once | ||
|
||
#include "envoy/registry/registry.h" | ||
#include "envoy/server/active_udp_listener_config.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
// A factory to create ActiveQuicListenerFactory based on given protobuf. | ||
class ActiveQuicListenerConfigFactory : public Server::ActiveUdpListenerConfigFactory { | ||
public: | ||
ProtobufTypes::MessagePtr createEmptyConfigProto() override; | ||
|
||
Network::ActiveUdpListenerFactoryPtr | ||
createActiveUdpListenerFactory(const Protobuf::Message&) override; | ||
|
||
std::string name() override; | ||
}; | ||
|
||
DECLARE_FACTORY(ActiveQuicListenerConfigFactory); | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,16 +6,18 @@ namespace Envoy { | |
namespace Quic { | ||
|
||
EnvoyQuicDispatcher::EnvoyQuicDispatcher( | ||
const quic::QuicCryptoServerConfig* crypto_config, quic::QuicVersionManager* version_manager, | ||
const quic::QuicCryptoServerConfig* crypto_config, const quic::QuicConfig& quic_config, | ||
quic::QuicVersionManager* version_manager, | ||
std::unique_ptr<quic::QuicConnectionHelperInterface> helper, | ||
std::unique_ptr<quic::QuicAlarmFactory> alarm_factory, | ||
uint8_t expected_server_connection_id_length, Server::ConnectionHandlerImpl& connection_handler, | ||
Network::ListenerConfig& listener_config, Server::ListenerStats& listener_stats) | ||
: quic::QuicDispatcher(&quic_config_, crypto_config, version_manager, std::move(helper), | ||
uint8_t expected_server_connection_id_length, Network::ConnectionHandler& connection_handler, | ||
Network::ListenerConfig& listener_config, Server::ListenerStats& listener_stats, | ||
Event::Dispatcher& dispatcher) | ||
: quic::QuicDispatcher(&quic_config, crypto_config, version_manager, std::move(helper), | ||
std::make_unique<EnvoyQuicCryptoServerStreamHelper>(), | ||
std::move(alarm_factory), expected_server_connection_id_length), | ||
connection_handler_(connection_handler), listener_config_(listener_config), | ||
listener_stats_(listener_stats) { | ||
listener_stats_(listener_stats), dispatcher_(dispatcher) { | ||
// Turn off chlo buffering in QuicDispatcher because per event loop clean | ||
// up is not implemented. | ||
// TODO(danzh): Add a per event loop callback to | ||
|
@@ -29,8 +31,8 @@ void EnvoyQuicDispatcher::OnConnectionClosed(quic::QuicConnectionId connection_i | |
const std::string& error_details, | ||
quic::ConnectionCloseSource source) { | ||
quic::QuicDispatcher::OnConnectionClosed(connection_id, error, error_details, source); | ||
ASSERT(connection_handler_.num_connections_ > 0); | ||
--connection_handler_.num_connections_; | ||
ASSERT(connection_handler_.numConnections() > 0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you move this assert into the dec call itself? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
connection_handler_.decNumConnections(); | ||
} | ||
|
||
quic::QuicSession* EnvoyQuicDispatcher::CreateQuicSession( | ||
|
@@ -42,15 +44,15 @@ quic::QuicSession* EnvoyQuicDispatcher::CreateQuicSession( | |
listener_config_, listener_stats_); | ||
auto quic_session = new EnvoyQuicServerSession( | ||
config(), quic::ParsedQuicVersionVector{version}, std::move(quic_connection), this, | ||
session_helper(), crypto_config(), compressed_certs_cache(), connection_handler_.dispatcher_); | ||
session_helper(), crypto_config(), compressed_certs_cache(), dispatcher_); | ||
quic_session->Initialize(); | ||
// Filter chain can't be retrieved here as self address is unknown at this | ||
// point. | ||
// TODO(danzh): change QUIC interface to pass in self address as it is already | ||
// known. In this way, filter chain can be retrieved at this point. But one | ||
// thing to pay attention is that if the retrival fails, connection needs to | ||
// be closed, and it should be added to time wait list instead of session map. | ||
++connection_handler_.num_connections_; | ||
connection_handler_.incNumConnections(); | ||
return quic_session; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QuicProtocolOptions to mirror Http2ProtocolOptions and TcpProtocolOptions
Though do we want to say QUIC or H3? Itooks to me like this is all QUIC specific, but if we have configuration options which are H2 specific (say HPACK related) what do we want the structure to be? I'd think we'd want Http3ProtocolOptions containing QuicProtocolOptions so we can reuse QuicProtocolOptions for non-HTTP3 things like QuicWebRTC
@ianswett @envoyproxy/api-shepherds for thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that QUIC is a transport and H3 is an application, separate configs make sense, though it's not how the code is currently structured.
If we create separate configs, I'm not sure whether nesting makes more sense or putting them side-by-side? If TcpProtocolOptions is inside Http2ProtocolOptions, then following that pattern is the most obvious approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http2ProtocolOptions seems to already have all the config we want. But it's a network filter (HCM) proto, which is not accessed before connection is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http2ProtocolOptions is in protocol.proto so we can easily reuse it, but don't we have issues with HPACK settings? Should we make QuicProtocolOptions and Http3ProtocolOptions as needed and clean this up in v3 if there are shared message possibilities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point that Http2ProtocolOptions has unwanted HPACK setting config except for the stream multiplexing config which we wanted in QuicProtocolOptions. And Http3ProtocolOptions will have its own config for things like QPACK table size. I think it's better not to reuse Http2ProtocolOptions, instead, adding all the fields needed in our own protobuf messages, basically, as it is currently defined now. WDYT?
Since I didn't know protocol.proto before, would it make more sense to define QuicProtocolOptions and Http3ProtocolOptions there instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be cool with adding them there, but given we want QUIC to be optional, I don't know if we want to be purists about having the protos compiled out.
I'm inclined to say it's useful to have all transport things in the same place, even the optional ones, given that the protos will only add negligable overhead to non-QUIC builds.
@htuch would be the person to ask but he's out this week. I'm good with landing it in either place and checking in with him when he gets back, since they'll be hidden and "Ok" to move without the deprecation dance.