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

quiche: implement Envoy Quic stream and connection #7721

Merged
merged 81 commits into from
Sep 9, 2019
Merged
Show file tree
Hide file tree
Changes from 57 commits
Commits
Show all changes
81 commits
Select commit Hold shift + click to select a range
3253b11
add fake proof source/verifier
danzh1989 May 8, 2019
9914fb4
move to test
danzh1989 May 9, 2019
2ea58dd
Revert "move to test"
danzh1989 Jun 14, 2019
a8aa262
Merge branch 'master' into fakeproof
danzh1989 Jun 14, 2019
6b5315b
add tests
danzh1989 Jun 17, 2019
02e4fd9
actually add test file
danzh1989 Jun 17, 2019
6f8c2e4
Merge branch 'master' into fakeproof
danzh1989 Jun 19, 2019
58f3842
revert flag change
danzh1989 Jun 19, 2019
8fec99d
more comment
danzh1989 Jun 19, 2019
15e5e33
add words to dict
danzh1989 Jun 19, 2019
72c2f84
fix clang_tidy
danzh1989 Jun 19, 2019
25998ab
s/dummy/fake
danzh1989 Jun 20, 2019
2fd6df0
Merge branch 'master' into fakeproof
danzh1989 Jun 21, 2019
aacc588
update spell_dict
danzh1989 Jun 21, 2019
f68e5dd
add nofips
danzh1989 Jun 25, 2019
12ef3c7
fix build exlucsion
danzh1989 Jun 26, 2019
6029aeb
add more nofips
danzh1989 Jun 27, 2019
14f1e79
add build_tests_only
danzh1989 Jun 27, 2019
2fccff1
remove build_tag_filter for test
danzh1989 Jun 27, 2019
ef7cf28
update tarball
danzh1989 Jun 27, 2019
5419162
add all targets
danzh1989 Jun 28, 2019
117244f
fix http2 file path
danzh1989 Jul 1, 2019
1719fd0
build spdy
danzh1989 Jul 3, 2019
732b3c8
build quic http lib
danzh1989 Jul 3, 2019
f94f424
add server_push_utils
danzh1989 Jul 3, 2019
0d56de4
Merge branch 'master' into quichttp
danzh1989 Jul 3, 2019
bacb5c5
fix quic_platofrm_test
danzh1989 Jul 3, 2019
f04a469
fix typo
danzh1989 Jul 3, 2019
9ba2754
update spell dict
danzh1989 Jul 3, 2019
05daf8d
switch to NOT_IMPLEMENTED_GCOVR_EXCL_LINE
danzh1989 Jul 8, 2019
6eca9c6
envoy quic stream and session
danzh1989 Jul 10, 2019
b8fc520
Merge branch 'master' into quichttp
danzh1989 Jul 17, 2019
f5e0c11
add listener and test
danzh1989 Jul 24, 2019
b881ae8
more comment
danzh1989 Jul 25, 2019
3db6847
fix connection dealloc
danzh1989 Jul 25, 2019
1e55f68
fix format
danzh1989 Jul 25, 2019
ab474d5
fix stream test
danzh1989 Jul 26, 2019
5f7dd41
Merge branch 'master' into quichttp
danzh1989 Jul 26, 2019
fcfe160
remove debug log
danzh1989 Jul 26, 2019
9dc44be
decouple listener and quic dispatcher
danzh1989 Aug 5, 2019
7a2395f
delete listener impl
danzh1989 Aug 5, 2019
3e4388b
test quic dispatcher
danzh1989 Aug 5, 2019
2397898
replace NOT_REACHED
danzh1989 Aug 6, 2019
0641fba
add tests
danzh1989 Aug 7, 2019
30d976b
fix stream test
danzh1989 Aug 7, 2019
a50fc1e
fix asan
danzh1989 Aug 7, 2019
ed15996
remove listener_lib
danzh1989 Aug 9, 2019
8b297e4
fix release failure
danzh1989 Aug 9, 2019
adbdee0
format
danzh1989 Aug 12, 2019
57144ed
remove unused .cc file
danzh1989 Aug 12, 2019
d278167
address comments
danzh1989 Aug 13, 2019
1b8117b
fix compile error
danzh1989 Aug 13, 2019
8375028
Merge branch 'master' into quichttp
danzh1989 Aug 13, 2019
6361860
Merge branch 'master' into quichttp
danzh1989 Aug 14, 2019
4e51861
error flush write
danzh1989 Aug 14, 2019
44e262f
ownership of connection
danzh1989 Aug 15, 2019
f2fc291
fix include
danzh1989 Aug 15, 2019
97b7d90
TODO for self_address
danzh1989 Aug 16, 2019
7361ca0
split utils to cc
danzh1989 Aug 19, 2019
10e85f6
add .cc file
danzh1989 Aug 19, 2019
5f55a97
spell
danzh1989 Aug 19, 2019
a4050dc
Merge branch 'master' into quichttp
danzh1989 Aug 23, 2019
81e5f9a
adjust comment
danzh1989 Aug 28, 2019
109e9a9
Merge branch 'master' into quichttp
danzh1989 Aug 28, 2019
9d545c5
fix typo
danzh1989 Aug 28, 2019
4e91aae
Merge branch 'master' into quichttp
danzh1989 Aug 29, 2019
7c863ef
fix stream test
danzh1989 Aug 29, 2019
c6f0763
fix pcc sender
danzh1989 Aug 29, 2019
bca0450
more test
danzh1989 Sep 3, 2019
42b5df3
add more tests
danzh1989 Sep 4, 2019
8891a14
fix coverage test failure
danzh1989 Sep 4, 2019
9a5468e
fix recvmsg test
danzh1989 Sep 4, 2019
1d23053
format
danzh1989 Sep 4, 2019
817c08d
revert Http3
danzh1989 Sep 5, 2019
3405bfc
more unit tests, remove isQuic
danzh1989 Sep 6, 2019
548f8f5
comments
danzh1989 Sep 6, 2019
b218f5a
fix session test
danzh1989 Sep 6, 2019
be37cb1
skip coverage for quiche
danzh1989 Sep 9, 2019
d7c551f
Merge branch 'master' into quichttp
danzh1989 Sep 9, 2019
bb1e435
update interface change
danzh1989 Sep 9, 2019
61ecd12
remove EBADF
danzh1989 Sep 9, 2019
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
1 change: 1 addition & 0 deletions api/envoy/data/accesslog/v2/accesslog.proto
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ message HTTPAccessLogEntry {
HTTP10 = 1;
HTTP11 = 2;
HTTP2 = 3;
HTTP3 = 4;
}
HTTPVersion protocol_version = 2;

Expand Down
3 changes: 3 additions & 0 deletions bazel/external/quiche.BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ quiche_copt = [
"-Wno-type-limits",
# quic_inlined_frame.h uses offsetof() to optimize memory usage in frames.
"-Wno-invalid-offsetof",
"-Wno-type-limits",
"-Wno-return-type",
]

envoy_cc_test_library(
Expand Down Expand Up @@ -1945,6 +1947,7 @@ envoy_cc_library(
copts = quiche_copt,
repository = "@envoy",
tags = ["nofips"],
visibility = ["//visibility:public"],
deps = [
":quic_core_packets_lib",
":quic_platform_base",
Expand Down
4 changes: 2 additions & 2 deletions include/envoy/http/protocol.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ namespace Http {
* Possible HTTP connection/request protocols. The parallel NumProtocols constant allows defining
* fixed arrays for each protocol, but does not pollute the enum.
*/
enum class Protocol { Http10, Http11, Http2 };
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we want to do this, we should land a separate PR adding http3 and auditing all calls to existing protocols to see if we need to change them.
Most checks for protocol == http2 or protocol != http2 may need to change to >= <

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, are you suggesting reverting this change and use Http2 for quic for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

H2 or QUIC, any addition to this enum (rather than pretending QUIC is H2) means we need to go through the HCM (and other bits of code) to make sure we're handling things right.

Very often code in the HCM differentiates on how we do error handling, how we handle draining and closing the connection etc. based on HTTP version. If we add one in it's unclear that code will work properly for QUIC. Check out HCM usage of the existing enum fields and you should see what I mean?

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 see. How about treating QUIC as H2 for now and add the enum value later when I modify HCM?

const size_t NumProtocols = 3;
enum class Protocol { Http10, Http11, Http2, Http3 };
const size_t NumProtocols = 4;

} // namespace Http
} // namespace Envoy
5 changes: 5 additions & 0 deletions include/envoy/network/connection.h
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,11 @@ class Connection : public Event::DeferredDeletable, public FilterManager {
* occurred an empty string is returned.
*/
virtual absl::string_view transportFailureReason() const PURE;

/**
* @return return true if it speaks QUIC.
*/
virtual bool isQuic() const PURE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I have context for where this will be used?
edit: ah, in the HCM.

I'm not sure how we're going to handle compiling out the QUIC code if the HCM has to create quic-specific constructs. Do you think it would make more sense to have a quic-specific HCM subclass for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only used by HCM onNewConnection() to create QuicHttpServerConnectionImpl which is a Http::ServerConnection subclass and owned by HCM. Good point that in that way, HCM will depends on quic. A way I can think of as a walk around is to add another interface to Network::Connection, e.g. createQuicCodec(), to create a codec for quic. In onNewConnection(), if isQuic() return true, call createQuicCodec(). In this way, only quic's implementation of Network::Connection, which is EnvoyQuicServerSession, will depend on QUIC core, for other implementation, this method shouldn't be called at all, and should return nullptr. Network::Connection will depends on codec.h. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would compile. Matt may have ideas if there's cleaner options than this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternately, could we use streamInfo->protocol() for this? It's a bit weird to use an L7 protocol in an L4 connection but hey, the struct is there.

I think we could remove this from this PR given I think it's not used here, and then do a follow-up with the HTTP/3 change in isolation, and then use streamInfo protocol existing and being http3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a bit weird as ConnectionImpl::stream_info_.protocol is not set. But it doesn't hurt if we utilize this field in connection. Actually isQuic() is used in envoy_quic_server_session_test.cc now. I make it return Http2 for now, so that we can tests it and not forget setting this field when we add Http3 enum value later.

};

using ConnectionPtr = std::unique_ptr<Connection>;
Expand Down
1 change: 1 addition & 0 deletions source/common/http/headers.h
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ class HeaderValues {
const std::string Http10String{"HTTP/1.0"};
const std::string Http11String{"HTTP/1.1"};
const std::string Http2String{"HTTP/2"};
const std::string Http3String{"HTTP/3"};
} ProtocolStrings;

struct {
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,8 @@ const std::string& Utility::getProtocolString(const Protocol protocol) {
return Headers::get().ProtocolStrings.Http11String;
case Protocol::Http2:
return Headers::get().ProtocolStrings.Http2String;
case Protocol::Http3:
return Headers::get().ProtocolStrings.Http3String;
}

NOT_REACHED_GCOVR_EXCL_LINE;
Expand Down
1 change: 1 addition & 0 deletions source/common/network/connection_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ class ConnectionImpl : public FilterManagerConnection,
delayed_close_timeout_ = timeout;
}
std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; }
bool isQuic() const override { return false; }

protected:
void closeSocket(ConnectionEvent close_type);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ void HttpGrpcAccessLog::emitLog(const Http::HeaderMap& request_headers,
case Http::Protocol::Http2:
log_entry.set_protocol_version(envoy::data::accesslog::v2::HTTPAccessLogEntry::HTTP2);
break;
case Http::Protocol::Http3:
log_entry.set_protocol_version(envoy::data::accesslog::v2::HTTPAccessLogEntry::HTTP3);
break;
}
}

Expand Down
116 changes: 115 additions & 1 deletion source/extensions/quic_listeners/quiche/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "envoy_quic_connection_helper_lib",
hdrs = ["envoy_quic_connection_helper.h"],
tags = ["nofips"],
deps = [
"//source/extensions/quic_listeners/quiche/platform:envoy_quic_clock_lib",
"@com_googlesource_quiche//:quic_core_buffer_allocator_lib",
"@com_googlesource_quiche//:quic_core_connection_lib",
"@com_googlesource_quiche//:quic_core_crypto_random_lib",
],
)

envoy_cc_library(
name = "envoy_quic_packet_writer_lib",
srcs = ["envoy_quic_packet_writer.cc"],
Expand Down Expand Up @@ -78,9 +90,111 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "envoy_quic_stream_lib",
hdrs = ["envoy_quic_stream.h"],
tags = ["nofips"],
deps = [
"//include/envoy/http:codec_interface",
"//source/common/http:codec_helper_lib",
],
)

envoy_cc_library(
name = "envoy_quic_server_stream_lib",
srcs = ["envoy_quic_server_stream.cc"],
hdrs = ["envoy_quic_server_stream.h"],
tags = ["nofips"],
deps = [
":envoy_quic_stream_lib",
":envoy_quic_utils_lib",
"//source/common/buffer:buffer_lib",
"//source/common/common:assert_lib",
"//source/common/http:header_map_lib",
"@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
],
)

envoy_cc_library(
name = "codec_impl_lib",
srcs = ["codec_impl.cc"],
hdrs = ["codec_impl.h"],
tags = ["nofips"],
deps = [
":envoy_quic_server_session_lib",
"//include/envoy/http:codec_interface",
"@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
],
)

envoy_cc_library(
name = "envoy_quic_server_session_lib",
srcs = ["envoy_quic_server_session.cc"],
hdrs = ["envoy_quic_server_session.h"],
tags = ["nofips"],
deps = [
":envoy_quic_connection_lib",
":envoy_quic_server_stream_lib",
"//include/envoy/event:dispatcher_interface",
"//include/envoy/network:connection_interface",
"//source/common/common:empty_string",
"//source/common/network:filter_manager_lib",
"//source/common/stream_info:stream_info_lib",
"@com_googlesource_quiche//:quic_core_http_spdy_session_lib",
],
)

envoy_cc_library(
name = "quic_io_handle_wrapper_lib",
hdrs = ["quic_io_handle_wrapper.h"],
deps = [
"//include/envoy/network:io_handle_interface",
],
)

envoy_cc_library(
name = "envoy_quic_connection_lib",
srcs = ["envoy_quic_connection.cc"],
hdrs = ["envoy_quic_connection.h"],
tags = ["nofips"],
deps = [
":quic_io_handle_wrapper_lib",
"//include/envoy/network:connection_interface",
"//source/common/network:listen_socket_lib",
"//source/extensions/quic_listeners/quiche:envoy_quic_utils_lib",
"//source/server:connection_handler_lib",
"@com_googlesource_quiche//:quic_core_connection_lib",
],
)

envoy_cc_library(
name = "envoy_quic_dispatcher_lib",
srcs = [
"envoy_quic_dispatcher.cc",
],
hdrs = [
"envoy_quic_dispatcher.h",
],
tags = ["nofips"],
deps = [
":envoy_quic_connection_lib",
":envoy_quic_proof_source_lib",
":envoy_quic_server_session_lib",
"//include/envoy/network:listener_interface",
"//source/server:connection_handler_lib",
"@com_googlesource_quiche//:quic_core_server_lib",
"@com_googlesource_quiche//:quic_core_utils_lib",
],
)

envoy_cc_library(
name = "envoy_quic_utils_lib",
hdrs = ["envoy_quic_utils.h"],
external_deps = ["quiche_quic_platform"],
deps = ["//source/common/network:address_lib"],
deps = [
"//include/envoy/http:codec_interface",
"//source/common/http:header_map_lib",
"//source/common/network:address_lib",
"@com_googlesource_quiche//:quic_core_http_header_list_lib",
],
)
23 changes: 23 additions & 0 deletions source/extensions/quic_listeners/quiche/codec_impl.cc
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
53 changes: 53 additions & 0 deletions source/extensions/quic_listeners/quiche/codec_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

De-multiplexing is handled by QUIC stack, so HCM onData should never be called, so is dispatch() here.

Do we pass directly to have the per-packet information persisted?

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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
It really bites when we get close to 97% coverage because if you add some NOT_REACHED_GCOVR_EXCL_LINE in a small CL you then have to write unit tests for unrelated code.

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.
I would like you to take a look at coverge results and see if any of the new code not explicitly NOT_REACHED makes sense to unit test, because I suspect much of it does and it's easier to land in line than do it all at once. If you don't know how to dig through the CI -> coverage -> artifacts I can run you through it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
Http::Protocol protocol() override { return Http::Protocol::Http3; }
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
55 changes: 55 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_connection.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#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"

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear why it wasn't before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because self_address is not initialized till this point.

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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 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));

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(
Copy link
Contributor

Choose a reason for hiding this comment

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

are we validating somewhere that the only valid config is HCM, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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()

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need to have empty_filter_chain. if (!listener_config_....) {} should read just fine.

Copy link
Contributor Author

@danzh2010 danzh2010 Aug 14, 2019

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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_NO_ERROR, "closing connection: filter chain is empty",
Copy link
Contributor

Choose a reason for hiding this comment

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

How about use something other than QUIC_NO_ERROR?

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 had a hard time to choose from QuicErrorCode. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you've used QUIC_CRYPTO_INTERNAL_ERROR above(when filter_chain_ == nullptr), maybe use the same code here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

quic::ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
return false;
}
}
return true;
}

} // namespace Quic
} // namespace Envoy
Loading