-
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 Envoy Quic stream and connection #7721
Changes from 77 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
3e4388b
2397898
0641fba
30d976b
a50fc1e
ed15996
8b297e4
adbdee0
57144ed
d278167
1b8117b
8375028
6361860
4e51861
44e262f
f2fc291
97b7d90
7361ca0
10e85f6
5f55a97
a4050dc
81e5f9a
109e9a9
9d545c5
4e91aae
7c863ef
c6f0763
bca0450
42b5df3
8891a14
9a5468e
1d23053
817c08d
3405bfc
548f8f5
b218f5a
be37cb1
d7c551f
bb1e435
61ecd12
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,23 @@ | ||
#include "extensions/quic_listeners/quiche/codec_impl.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
void QuicHttpConnectionImplBase::goAway() { | ||
alyssawilk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
quic_session_.SendGoAway(quic::QUIC_PEER_GOING_AWAY, "server shutdown imminent"); | ||
} | ||
|
||
bool QuicHttpConnectionImplBase::wantsToWrite() { return quic_session_.HasDataToWrite(); } | ||
|
||
// TODO(danzh): modify QUIC stack to react based on aggregated bytes across all | ||
// the streams. And call StreamCallbackHelper::runHighWatermarkCallbacks() for each stream. | ||
void QuicHttpConnectionImplBase::onUnderlyingConnectionAboveWriteBufferHighWatermark() { | ||
NOT_IMPLEMENTED_GCOVR_EXCL_LINE; | ||
} | ||
|
||
void QuicHttpConnectionImplBase::onUnderlyingConnectionBelowWriteBufferLowWatermark() { | ||
NOT_IMPLEMENTED_GCOVR_EXCL_LINE; | ||
} | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
#include "envoy/http/codec.h" | ||
|
||
#include "common/common/assert.h" | ||
#include "common/common/logger.h" | ||
|
||
#include "extensions/quic_listeners/quiche/envoy_quic_server_session.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
// QuicHttpConnectionImplBase instance is a thin QUIC codec just providing quic interface to HCM. | ||
// Owned by HCM and created during onNewConnection() if the network connection | ||
// is a QUIC connection. | ||
class QuicHttpConnectionImplBase : public virtual Http::Connection, | ||
protected Logger::Loggable<Logger::Id::quic> { | ||
public: | ||
QuicHttpConnectionImplBase(EnvoyQuicServerSession& quic_session) : quic_session_(quic_session) {} | ||
|
||
// Http::Connection | ||
void dispatch(Buffer::Instance& /*data*/) override { | ||
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. hm, so I'd imagined that the HCM onData would be passed to the quic codec Dispatch. Do we pass directly to have the per-packet information persisted? 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.
De-multiplexing is handled by QUIC stack, so HCM onData should never be called, so is dispatch() here.
What do you mean? HCM should only see request header, body and trailer, rather than UDP payload. |
||
// Bypassed. QUIC connection already hands all data to streams. | ||
NOT_REACHED_GCOVR_EXCL_LINE; | ||
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. sadly these don't quite work, and so add to our uncovered lines. Let's instead add quiche to COVERAGE_IGNORE_REGEX in test/run_envoy_bazel_coverage.sh and have a tracking item in one of the QUICHE bugs to fix up coverage before we land. 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 |
||
} | ||
Http::Protocol protocol() override { | ||
// From HCM's view, QUIC should behave the same as Http2, only the stats | ||
// should be different. | ||
// TODO(danzh) add Http3 enum value for 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. Q for my own understanding: Would we actually do this? Is HTTP/3 different from HTTP/2 in any substantial way at the app layer? I'm mainly wondering if it's worth it to make Envoy aware of HTTP/3 above the codec. 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. Distinguish Http3 from Http2 traffic is more for monitoring. These two works the same at the app layer. The distinction should only exists in codec |
||
return Http::Protocol::Http2; | ||
} | ||
void goAway() override; | ||
// Returns true if the session has data to send but queued in connection or | ||
// stream send buffer. | ||
bool wantsToWrite() override; | ||
void onUnderlyingConnectionAboveWriteBufferHighWatermark() override; | ||
void onUnderlyingConnectionBelowWriteBufferLowWatermark() override; | ||
|
||
protected: | ||
EnvoyQuicServerSession& quic_session_; | ||
}; | ||
|
||
class QuicHttpServerConnectionImpl : public QuicHttpConnectionImplBase, | ||
public Http::ServerConnection { | ||
public: | ||
QuicHttpServerConnectionImpl(EnvoyQuicServerSession& quic_session, | ||
Http::ServerConnectionCallbacks& callbacks) | ||
: QuicHttpConnectionImplBase(quic_session) { | ||
quic_session.setHttpConnectionCallbacks(callbacks); | ||
} | ||
|
||
// Http::Connection | ||
void shutdownNotice() override { | ||
// TODO(danzh): Add double-GOAWAY support in QUIC. | ||
ENVOY_CONN_LOG(error, "Shutdown notice is not propagated to QUIC.", quic_session_); | ||
} | ||
}; | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
#include "extensions/quic_listeners/quiche/envoy_quic_connection.h" | ||
|
||
#include "common/network/listen_socket_impl.h" | ||
|
||
#include "extensions/quic_listeners/quiche/envoy_quic_utils.h" | ||
#include "extensions/quic_listeners/quiche/quic_io_handle_wrapper.h" | ||
#include "extensions/transport_sockets/well_known_names.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
EnvoyQuicConnection::EnvoyQuicConnection( | ||
const quic::QuicConnectionId& server_connection_id, | ||
quic::QuicSocketAddress initial_peer_address, quic::QuicConnectionHelperInterface& helper, | ||
quic::QuicAlarmFactory& alarm_factory, quic::QuicPacketWriter& writer, bool owns_writer, | ||
quic::Perspective perspective, const quic::ParsedQuicVersionVector& supported_versions, | ||
Network::ListenerConfig& listener_config, Server::ListenerStats& listener_stats) | ||
: quic::QuicConnection(server_connection_id, initial_peer_address, &helper, &alarm_factory, | ||
&writer, owns_writer, perspective, supported_versions), | ||
listener_config_(listener_config), listener_stats_(listener_stats) {} | ||
|
||
bool EnvoyQuicConnection::OnPacketHeader(const quic::QuicPacketHeader& header) { | ||
if (quic::QuicConnection::OnPacketHeader(header) && connection_socket_ == nullptr) { | ||
ASSERT(self_address().IsInitialized()); | ||
// Self address should be initialized by now. It's time to install filters. | ||
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. unclear why it wasn't before? 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. Because self_address is not initialized till this point. 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. It should be possible to make self_address available to the connection earlier, as a parameter of QuicDispatcher::CreateQuicSession(), that way we don't need to do initialization work at here. Maybe add a TODO for that? 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. Client connection is not possible. I would like to keep both side use same logic. 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. To capture our offline discussion: Client connection in Enovy should also know how to initialize the filter chain even without knowing the self address. It'd be nice if we could initialize the filter chan in a more straightforward place, rather than after a packet is received. A TODO to do it later is totally fine. 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 added a TODO to investigate the possibility of this approach |
||
Network::Address::InstanceConstSharedPtr local_addr = | ||
quicAddressToEnvoyAddressInstance(self_address()); | ||
|
||
Network::Address::InstanceConstSharedPtr remote_addr = | ||
quicAddressToEnvoyAddressInstance(peer_address()); | ||
connection_socket_ = std::make_unique<Network::ConnectionSocketImpl>( | ||
// Wraps the real IoHandle instance so that if this socket gets closed, | ||
// the real IoHandle won't be affected. | ||
std::make_unique<QuicIoHandleWrapper>(listener_config_.socket().ioHandle()), | ||
std::move(local_addr), std::move(remote_addr)); | ||
connection_socket_->setDetectedTransportProtocol( | ||
Extensions::TransportSockets::TransportSocketNames::get().Quic); | ||
|
||
filter_chain_ = listener_config_.filterChainManager().findFilterChain(*connection_socket_); | ||
if (filter_chain_ == nullptr) { | ||
listener_stats_.no_filter_chain_match_.inc(); | ||
CloseConnection(quic::QUIC_CRYPTO_INTERNAL_ERROR, | ||
"closing connection: no matching filter chain found for handshake", | ||
quic::ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); | ||
return false; | ||
} | ||
const bool empty_filter_chain = !listener_config_.filterChainFactory().createNetworkFilterChain( | ||
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. are we validating somewhere that the only valid config is HCM, etc? 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. Not right now, but I think it's easier to validate the config during filter factory creation in addListener() 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: no need to have empty_filter_chain. 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 meaning of the return value is not intuitive from the function name, so I added a temporary variable for readability. 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. Sure, sounds good. |
||
*envoy_connection_, filter_chain_->networkFilterFactories()); | ||
if (empty_filter_chain) { | ||
CloseConnection(quic::QUIC_CRYPTO_INTERNAL_ERROR, "closing connection: filter chain is empty", | ||
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 an example of something we might want to unit test? 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. added in envoy_quic_dispatcher_test.cc |
||
quic::ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET); | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
#pragma once | ||
|
||
#pragma GCC diagnostic push | ||
// QUICHE allows unused parameters. | ||
#pragma GCC diagnostic ignored "-Wunused-parameter" | ||
// QUICHE uses offsetof(). | ||
#pragma GCC diagnostic ignored "-Winvalid-offsetof" | ||
|
||
#include "quiche/quic/core/quic_connection.h" | ||
|
||
#pragma GCC diagnostic pop | ||
|
||
#include <memory> | ||
|
||
#include "common/common/logger.h" | ||
#include "envoy/network/connection.h" | ||
#include "envoy/network/listener.h" | ||
#include "server/connection_handler_impl.h" | ||
|
||
namespace Envoy { | ||
namespace Quic { | ||
|
||
// Derived for network filter chain, stats and QoS. This is used on both client | ||
// and server side. | ||
class EnvoyQuicConnection : public quic::QuicConnection, | ||
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. Could this class work as a client connection? Could you add a comment for 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. Yes. Commented. 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. Thanks! |
||
protected Logger::Loggable<Logger::Id::connection> { | ||
public: | ||
EnvoyQuicConnection(const quic::QuicConnectionId& server_connection_id, | ||
quic::QuicSocketAddress initial_peer_address, | ||
quic::QuicConnectionHelperInterface& helper, | ||
quic::QuicAlarmFactory& alarm_factory, quic::QuicPacketWriter& writer, | ||
bool owns_writer, quic::Perspective perspective, | ||
const quic::ParsedQuicVersionVector& supported_versions, | ||
Network::ListenerConfig& listener_config, | ||
Server::ListenerStats& listener_stats); | ||
|
||
~EnvoyQuicConnection() override = default; | ||
|
||
// Called by EnvoyQuicSession::setConnectionStats(). | ||
void setConnectionStats(const Network::Connection::ConnectionStats& stats) { | ||
connection_stats_ = std::make_unique<Network::Connection::ConnectionStats>(stats); | ||
} | ||
|
||
// quic::QuicConnection | ||
// Overridden to retrieve filter chain with initialized self address. | ||
bool OnPacketHeader(const quic::QuicPacketHeader& header) override; | ||
|
||
// Called in session Initialize(). | ||
void setEnvoyConnection(Network::Connection& connection) { envoy_connection_ = &connection; } | ||
|
||
const Network::ConnectionSocketPtr& connectionSocket() const { return connection_socket_; } | ||
|
||
protected: | ||
Network::Connection::ConnectionStats& connectionStats() const { return *connection_stats_; } | ||
|
||
private: | ||
// TODO(danzh): populate stats. | ||
std::unique_ptr<Network::Connection::ConnectionStats> connection_stats_; | ||
// Only initialized after self address is known. Must not own the underlying | ||
// socket because UDP socket is shared among all connections. | ||
Network::ConnectionSocketPtr connection_socket_; | ||
Network::Connection* envoy_connection_{nullptr}; | ||
Network::ListenerConfig& listener_config_; | ||
Server::ListenerStats& listener_stats_; | ||
// Latched to the corresponding quic FilterChain after connection_socket_ is | ||
// initialized. | ||
const Network::FilterChain* filter_chain_{nullptr}; | ||
}; | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
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.
If we can land this as a quic-only change I'll be way happier. If we opt to get rid of isQuic let's just split this out if it's the only other non-quic code in the PR
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.
Q: Why do we need this again? I remember this in a previous PR and we removed it. Shouldn't this never happen / be an assert somewhere?
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! This was added because a test didn't mock out sys calls and previously there was an ASSERT in the
default:
block. Now the test shouldn't crash since we down-graded the severity.