quiche: modify some envoy quic implementation for code sharing by danzh2010 · Pull Request #8349 · envoyproxy/envoy

danzh2010 · 2019-09-24T16:24:02Z

Refactor EnvoyQuicServerSession to inherit from QuicFilterManagerConnectionImpl which implements most of the Network::FilterManagerConnection interface. This base class can be shared with client side implementation. Also split out some server side logic from EnvoyQuicConnection into EnvoyQuicServerConnection. So that EnvoyQuicConnection can be shared with client connection implementation

Modify EnvoyQuicPacketWriter to wrap a Socket& rather than Listener& because client side doesn't have listener. Extract socket write code from UdpListenerImpl::send() to Network::Utility so that writer can use same code for writing.

Alike wise, move some socket read logic in UdpListenerImpl::handleReadCallback() into Utility for code sharing with client connection later.

Made a slight change in EnvoyQuicServerSession to raiseEvent with type Connected when handshake is done.

Convert some ASSERT of not implemented feature to NOT_IMPLEMENT or NOT_REACHED.

Risk Level: low, refactor to unused code
Testing: existing tests
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-09-24T16:36:03Z

/assign @alyssawilk

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Thanks, flushing out a few more comments.

/wait

source/extensions/quic_listeners/quiche/envoy_quic_server_connection.cc

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

mattklein123 · 2019-09-27T17:31:34Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+
+void QuicFilterManagerConnectionImpl::addBytesSentCallback(
+    Network::Connection::BytesSentCb /*cb*/) {
+  // TODO(danzh): implement to support proxy. This interface is only called from


I would prefer that we either crash hard if things should not happen, or actually implement stuff. Since QUIC is not production ready yet, can we just make everything that shouldn't happen NOT_IMPLEMENTED for now? Same elsewhere?

mattklein123 · 2019-09-27T17:31:47Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+}
+
+std::chrono::milliseconds QuicFilterManagerConnectionImpl::delayedCloseTimeout() const {
+  // Not called outside of Network::ConnectionImpl. Maybe remove this interface


That's fine, please make this an actual TODO.

mattklein123 · 2019-09-27T17:32:47Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+const Network::ConnectionSocket::OptionsSharedPtr&
+QuicFilterManagerConnectionImpl::socketOptions() const {
+  ENVOY_CONN_LOG(
+      error,


Per elsewhere, let's please not log at error, etc. Let's either implement or NOT_IMPLEMENTED everywhere in this file.

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Sorry to keep going back and forth on this but I would still like to tighten up the interfaces a bit more in terms of the things we need to implement, the things that can't happen, etc. Thank you!

/wait

mattklein123 · 2019-09-30T16:53:45Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+}
+
+void QuicFilterManagerConnectionImpl::enableHalfClose(bool enabled) {
+  ASSERT(!enabled, "Quic connection doesn't support half close.");


Does this get called? Can this be NOT_IMPLEMENTED?

This should only be called with enable == true. This is the implementation desired.

I would make this a RELEASE_ASSERT then just to check our assumption.

mattklein123 · 2019-09-30T16:54:26Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+  // TODO(danzh): add interface to quic for connection level buffer throttling.
+  // Currently read buffer is capped by connection level flow control. And
+  // write buffer is not capped.
+  ENVOY_CONN_LOG(error, "Quic manages its own buffer currently.", *this);


Does this get called? Can this be NOT_IMPLEMENTED? If it does get called can we just have this be an empty function with the TODO?

According to my next PR, this method shouldn't be called. I changed it to NOT_REACHED here.

mattklein123 · 2019-09-30T16:54:47Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+void QuicFilterManagerConnectionImpl::close(Network::ConnectionCloseType type) {
+  if (type != Network::ConnectionCloseType::NoFlush) {
+    // TODO(danzh): Implement FlushWrite and FlushWriteAndDelay mode.
+    ENVOY_CONN_LOG(error, "Flush write is not implemented for QUIC.", *this);


Same here? Basically can we remove the error logs and just crash or have a TODO?

We don't want to crash, as it's called by HCM with other CloseType. Removed the error log. The empty if block looks weird though...

mattklein123 · 2019-09-30T16:56:19Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+}
+
+Ssl::ConnectionInfoConstSharedPtr QuicFilterManagerConnectionImpl::ssl() const {
+  // TODO(danzh): construct Ssl::ConnectionInfo from crypto stream


Per the comments above can we just remove the error logs? If something will be implemented later I think it's fine to just leave a TODO?

Essentially what I would like to see is:

No error logs in this type of code

NOT_IMPLEMENTED if it can't happen with a comment why.

Or a default implementation with a TODO to implement the real thing.

mattklein123 · 2019-09-30T16:57:05Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

+  void setDelayedCloseTimeout(std::chrono::milliseconds timeout) override;
+  std::chrono::milliseconds delayedCloseTimeout() const override;
+  void readDisable(bool disable) override {
+    ASSERT(!disable, "Quic connection should be able to read through out its life time.");


For these normal asserts, do these functions get called? Or should they not get called at all? Same elsewhere.

It shouldn't be called for multiplexing case according to my reading of the HCM implementation. I might be wrong, but NOT_REACHED for now.

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010

I changed error log and ASSERT to NOT_REACHED in most of the places according to my current understanding. But I'm not sure how meaningful it would be to tighten the interface at this point, considering they might get reverted in integration tests if it turns out those places are reachable.

danzh2010 · 2019-09-30T23:10:40Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+}
+
+void QuicFilterManagerConnectionImpl::enableHalfClose(bool enabled) {
+  ASSERT(!enabled, "Quic connection doesn't support half close.");


This should only be called with enable == true. This is the implementation desired.

danzh2010 · 2019-09-30T23:18:26Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+void QuicFilterManagerConnectionImpl::close(Network::ConnectionCloseType type) {
+  if (type != Network::ConnectionCloseType::NoFlush) {
+    // TODO(danzh): Implement FlushWrite and FlushWriteAndDelay mode.
+    ENVOY_CONN_LOG(error, "Flush write is not implemented for QUIC.", *this);


We don't want to crash, as it's called by HCM with other CloseType. Removed the error log. The empty if block looks weird though...

danzh2010 · 2019-09-30T23:19:45Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+  // TODO(danzh): add interface to quic for connection level buffer throttling.
+  // Currently read buffer is capped by connection level flow control. And
+  // write buffer is not capped.
+  ENVOY_CONN_LOG(error, "Quic manages its own buffer currently.", *this);


According to my next PR, this method shouldn't be called. I changed it to NOT_REACHED here.

danzh2010 · 2019-09-30T23:28:10Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

+  void setDelayedCloseTimeout(std::chrono::milliseconds timeout) override;
+  std::chrono::milliseconds delayedCloseTimeout() const override;
+  void readDisable(bool disable) override {
+    ASSERT(!disable, "Quic connection should be able to read through out its life time.");


It shouldn't be called for multiplexing case according to my reading of the HCM implementation. I might be wrong, but NOT_REACHED for now.

danzh2010 · 2019-10-01T14:56:53Z

/retest

repokitteh-read-only · 2019-10-01T14:56:56Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #8349 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-10-01T15:09:35Z

/azp envoy-linux(bazel release)

azure-pipelines · 2019-10-01T15:09:40Z

Command 'envoy-linux(bazel' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run. Example: "run" or "run pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

mattklein123

Thanks, almost there. Just a few more small comments. Also, can you please make it consistent between NOT_REACHED/NOT_IMPLEMENTED, or better, use NOT_REACHED when it for real should never happen and NOT_IMPLEMENTED/RELEASE_ASSERT when we need to implement it later as we add tests? Thank you!

/wait

mattklein123 · 2019-10-01T18:05:12Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+}
+
+void QuicFilterManagerConnectionImpl::enableHalfClose(bool enabled) {
+  ASSERT(!enabled, "Quic connection doesn't support half close.");


I would make this a RELEASE_ASSERT then just to check our assumption.

mattklein123 · 2019-10-01T18:08:04Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.cc

+}
+
+Ssl::ConnectionInfoConstSharedPtr QuicFilterManagerConnectionImpl::ssl() const {
+  // TODO(danzh): construct Ssl::ConnectionInfo from crypto stream


Sorry I'm not following what is going on here. Presumably this does need to be implemented eventually but if you NOT_IMPLEMENTED this now it crashes? I would remove the error log and just track with a TODO.

mattklein123 · 2019-10-01T18:08:49Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

+  const Network::Address::InstanceConstSharedPtr& localAddress() const override;
+  absl::optional<Network::Connection::UnixDomainSocketPeerCredentials>
+  unixSocketPeerCredentials() const override {
+    ASSERT(false, "Unix domain socket is not supported.");


Swap with NOT_IMPLEMENTED

mattklein123 · 2019-10-01T18:09:57Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

+    NOT_REACHED_GCOVR_EXCL_LINE;
+  }
+  bool localAddressRestored() const override {
+    // SO_ORIGINAL_DST not supported by QUIC.


I'm pretty sure this will never get called. NOT_IMPLEMENTED?

mattklein123 · 2019-10-01T18:10:18Z

source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h

+  // them.
+  std::list<Network::ConnectionCallbacks*> network_connection_callbacks_;
+  std::string transport_failure_reason_;
+  uint64_t id_;


Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Nice, let's ship and iterate!

…proxy#8349) Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 25 commits August 21, 2019 17:25

build FilterChain

33a71d8

Signed-off-by: Dan Zhang <danzh@google.com>

add test

24692ce

Signed-off-by: Dan Zhang <danzh@google.com>

format

bfeff99

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filterchain

bd688c2

Signed-off-by: Dan Zhang <danzh@google.com>

test quic

2a53122

Signed-off-by: Dan Zhang <danzh@google.com>

remove debug log

31ad1f8

Signed-off-by: Dan Zhang <danzh@google.com>

fix compile error

81aa31d

Signed-off-by: Dan Zhang <danzh@google.com>

fix listener manager

8cce689

Signed-off-by: Dan Zhang <danzh@google.com>

fix linux test error

e37f233

Signed-off-by: Dan Zhang <danzh@google.com>

more logging

e01047d

Signed-off-by: Dan Zhang <danzh@google.com>

fix socket close error

8049fd1

Signed-off-by: Dan Zhang <danzh@google.com>

more logging details

79c93d6

Signed-off-by: Dan Zhang <danzh@google.com>

return const reference of ContextConfig

7ba4cf5

Signed-off-by: Dan Zhang <danzh@google.com>

fix build error

3644632

Signed-off-by: Dan Zhang <danzh@google.com>

remove not null check in test

4daa91b

Signed-off-by: Dan Zhang <danzh@google.com>

add transport socket factory for quic

877ae42

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filterchain

05bb8c2

Signed-off-by: Dan Zhang <danzh@google.com>

fix test

2f6788a

Signed-off-by: Dan Zhang <danzh@google.com>

revert tls_context factory

8a7337e

Signed-off-by: Dan Zhang <danzh@google.com>

more comment

2ae5458

Signed-off-by: Dan Zhang <danzh@google.com>

comment

f3d426f

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into filterchain

f0ac1f1

Signed-off-by: Dan Zhang <danzh@google.com>

add quic client impl

6a5ea77

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into quicclient

a464256

Signed-off-by: Dan Zhang <danzh@google.com>

remove client side impl

6fe087a

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners September 24, 2019 16:24

typo

eee356d

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned alyssawilk Sep 24, 2019

mattklein123 self-assigned this Sep 26, 2019

connection id

eae93e4

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed alyssawilk’s stale review via eae93e4 September 27, 2019 15:48

repokitteh-read-only bot removed the waiting label Sep 27, 2019

typo

e8ac251

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 requested changes Sep 27, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 27, 2019

add TODO

2aac4fa

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 27, 2019

danzh1989 added 4 commits September 27, 2019 14:58

remove outdated log

6d4766d

Signed-off-by: Dan Zhang <danzh@google.com>

ASSERT to NOT_IMPLEMENT/REACHED

a1312d7

Signed-off-by: Dan Zhang <danzh@google.com>

fix server session test

0ecb10c

Signed-off-by: Dan Zhang <danzh@google.com>

fix coverage ci

7656fff

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 requested changes Sep 30, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 30, 2019

more NOT_REACHED

99b06ea

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Sep 30, 2019

danzh2010 commented Sep 30, 2019

View reviewed changes

mattklein123 requested changes Oct 1, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 1, 2019

more NOT_REACHED

3fb8246

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Oct 1, 2019

mattklein123 approved these changes Oct 2, 2019

View reviewed changes

mattklein123 merged commit 57ac960 into envoyproxy:master Oct 2, 2019

danzh2010 added a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

quiche: modify some envoy quic implementation for code sharing (envoy…

31200c5

…proxy#8349) Signed-off-by: Dan Zhang <danzh@google.com>

Conversation

danzh2010 commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Sep 24, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Oct 1, 2019

Uh oh!

repokitteh-read-only bot commented Oct 1, 2019

Uh oh!

danzh2010 commented Oct 1, 2019

Uh oh!

azure-pipelines bot commented Oct 1, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

danzh2010 commented Sep 24, 2019 •

edited

Loading