quiche: client session supports creating bidi stream #17543

danzh2010 · 2021-07-30T00:05:19Z

Though QUICHE won't negotiate to support server push, it doesn't prevent a problematic peer to open bi-directional streams. In such case, Envoy shouldn't hit NOT_REACHED_GCOVR_EXCL_LINE.

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

danzh2010 · 2021-07-30T17:01:57Z

/retest

repokitteh-read-only · 2021-07-30T17:02:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

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

alyssawilk

LGTM overall but I have some questions I want to sort out

alyssawilk · 2021-08-03T13:14:28Z

source/common/quic/envoy_quic_server_session.cc

 }

-quic::QuicSpdyStream*
-EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* /*pending*/) {
+quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* pending) {
  // Only client side server push stream should trigger this call.


Sorry I'm a bit confused - if the client side sends a server push stream when it's not negotiated why don't we close the connection for it doing "illegal" things? Should that be a TODO for quiche to fix upstream?

Not just server push stream actually, instead, any client-initiated uni-directional stream with type byte other than the predesignated ones, QPack encode/decode streams, etc. QUICHE session should accept such stream as its extensions may wants to create such special streams. So it's not illegal for QUICHE.

I can close the connection in Envoy code if we don't have such use case in near future. Or update the comment to be more accurate?

IIRC current QUIC support in Envoy is only for HTTP transactions which are always bi-directional. Closing the connections seems appropriate until other use cases are added.

#17618 includes a QUICHE change that close the connection in this case. I will continue update this PR after that one is checked in.

alyssawilk · 2021-08-03T13:15:02Z

source/common/quic/envoy_quic_server_session.cc

+              fmt::format(
+                  "Quic session {} attempts to create stream {} before HCM filter is initialized.",
+                  this->id(), pending->id()));
+    return nullptr;


Why not always return nullptr if we don't support?

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

antoniovicente · 2021-08-05T19:46:34Z

source/common/quic/envoy_quic_client_stream.cc

-          static_cast<uint32_t>(GetReceiveWindow().value()), *filterManagerConnection(),
-          [this]() { runLowWatermarkCallbacks(); }, [this]() { runHighWatermarkCallbacks(); },
-          stats, http3_options) {}
-


These changes seem unrelated. Was this code unused?

These are only called by server push stream.

antoniovicente · 2021-08-05T19:52:35Z

source/common/quic/envoy_quic_server_session.cc

 }

-quic::QuicSpdyStream*
-EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* /*pending*/) {
+quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* pending) {
  // Only client side server push stream should trigger this call.


IIRC current QUIC support in Envoy is only for HTTP transactions which are always bi-directional. Closing the connections seems appropriate until other use cases are added.

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

danzh2010 · 2021-08-09T23:20:19Z

Updated this PR using the newly sync'ed QUICHE flag to close connection upon receiving server push stream. PTAL!

alyssawilk

nice clean up!

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ...

* main: (687 commits) ci: set build debug information from env (envoyproxy#17635) ext_authz: do the authentication even the direct response is set (envoyproxy#17546) upstream: various cleanups in connection pool code (envoyproxy#17644) owners: promote Dmitry to maintainer (envoyproxy#17642) quiche: client session supports creating bidi stream (envoyproxy#17543) Update HTTP/2 METADATA documentation. (envoyproxy#17637) ext_proc: Check validity of the :status header (envoyproxy#17596) test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614) ensure that the inline cookie header will be folded correctly (envoyproxy#17560) cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616) owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641) quiche: update QUICHE dependency (envoyproxy#17618) Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623) REPO_LAYOUT.md: fix outdated link (envoyproxy#17626) hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558) thrift proxy: add request shadowing support (envoyproxy#17544) ext_proc: Ensure that timer is always cancelled (envoyproxy#17569) Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362) proto: fix verify to point at v3 only (envoyproxy#17622) api: move generic matcher proto to its own package (envoyproxy#17096) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Though QUICHE won't negotiate to support server push, it doesn't prevent a problematic peer to open bi-directional streams. Risk: low Tests: new unit tess Docs: n/a release notes: n/a Signed-off-by: Dan Zhang <danzh@google.com>

danzh1989 added 3 commits July 29, 2021 18:14

create unidirectional read stream

c275f94

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

Merge branch 'master' into pendingstream

531cfbc

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

format

38774bb

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

danzh1989 added 2 commits July 30, 2021 14:21

Merge branch 'master' into pendingstream

dcf9fef

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

not create server initiated stream

4392432

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

alyssawilk reviewed Aug 3, 2021

View reviewed changes

alyssawilk assigned alyssawilk and antoniovicente Aug 3, 2021

alyssawilk added the waiting label Aug 5, 2021

return nullptr for pending stream

fa24b94

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

repokitteh-read-only bot removed the waiting label Aug 5, 2021

antoniovicente reviewed Aug 5, 2021

View reviewed changes

danzh1989 added 2 commits August 9, 2021 18:48

Merge branch 'master' into pendingstream

25ffac5

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

close connection for server push stream

4b84ac3

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

alyssawilk approved these changes Aug 10, 2021

View reviewed changes

danzh2010 changed the title ~~quiche: support create stream from pending stream~~ quiche: client session supports creating bidi stream Aug 10, 2021

alyssawilk merged commit dbb21f3 into envoyproxy:main Aug 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quiche: client session supports creating bidi stream #17543

quiche: client session supports creating bidi stream #17543

danzh2010 commented Jul 30, 2021 •

edited

Loading

danzh2010 commented Jul 30, 2021

repokitteh-read-only bot commented Jul 30, 2021

alyssawilk left a comment

alyssawilk Aug 3, 2021

danzh2010 Aug 3, 2021

antoniovicente Aug 5, 2021

danzh2010 Aug 6, 2021

alyssawilk Aug 3, 2021

antoniovicente Aug 5, 2021

danzh2010 Aug 6, 2021

antoniovicente Aug 5, 2021

danzh2010 commented Aug 9, 2021

alyssawilk left a comment

quiche: client session supports creating bidi stream #17543

quiche: client session supports creating bidi stream #17543

Conversation

danzh2010 commented Jul 30, 2021 • edited Loading

danzh2010 commented Jul 30, 2021

repokitteh-read-only bot commented Jul 30, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 commented Aug 9, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

danzh2010 commented Jul 30, 2021 •

edited

Loading