quiche: use max header size configured in HCM #15912

danzh2010 · 2021-04-10T18:18:39Z

Commit Message: use max header size in HCM config to initialize quic session.
Additional Description:
change QuicSession::Initialize() and filter chain creation order to set max header list size for each session before Initialize(). Move Initialize() of Http3 connection from CodecClient to CodecClientProd.
Added CodecClient::connect() interface.
Change EnvoyQuicConnection not to directly inherit from QuicConnection. And renamed it.
Fix a use-after-free bug in FakeUpstream which causes ASAN failure.

Risk Level: low
Testing: more protocol H3 tests

Part of #2557 #12930 #14829

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-04-26T14:25:28Z

/retest

repokitteh-read-only · 2021-04-26T14:25:33Z

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

🐱

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

see: more, trace.

alyssawilk · 2021-04-26T15:46:28Z

@snowp want me to take this one back?

snowp · 2021-04-26T15:58:35Z

@alyssawilk Sure, go for it :)

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk

Cool, looking good.
I do think we should up the risk level to medium given the client connect() change.

source/common/http/codec_client.cc

alyssawilk · 2021-04-26T18:57:05Z

source/common/quic/quic_filter_manager_connection_impl.h

@@ -5,11 +5,14 @@
 #include "envoy/event/dispatcher.h"
 #include "envoy/network/connection.h"

+#define QUICHE_INCLUDE_1 "quiche/quic/core/quic_connection.h"


What's this all about? Can we have macro comments?

This is to avoid having duplicated #pragma GCC diagnostic... block in files includes QUICHE files. The block is moved to source/common/quic/quic_includes_ignores.h which only needs to be included at the end of the #include section. #define QUICHE_INCLUDE_1/2/...N is used in place of #include before each QUICHE header files.

Commented about its usage.

Do we have a plan to get rid of these? cc @DavidSchinazi yak shaver extrordinare

What's the goal of this macro? What's wrong with having duplicated pragmas?

Do we have a plan to get rid of these? cc @DavidSchinazi yak shaver extrordinare

AFAIK, -Winvalid-offsetof will persist because it is caused by a QUIC frame memory optimization.

What's the goal of this macro? What's wrong with having duplicated pragmas?

These pragmas are repeated in many files. And adding/removing a compiler option requires changing all the places. The goal of quic_includes_ignores.h is to avoid the duplication to improve both readability and maintainability. Though the macros are written in a very native way due to my limited preprocessor skill.

2 things:

QUICHE_INCLUDE_1 is the only one of these that is used. We probably don't need 2 to 5

The header include that needs the pragma seems to be only quiche/quic/core/quic_connection.h; can we add a wrapper header with appropriate pragmas and have that include be the one that is included through the rest of Envoy code? Or have these pragmas added to quiche/quic/core/quic_connection.h via a quiche patch that Envoy adds while fetching the quiche dependency via bazel?

The best course of action would be to fix QUICHE. Until then, I guess duplicate pragmas are fine.

QUICHE_INCLUDE_1 is the only one of these that is used. We probably don't need 2 to 5

The plan is to use QUICHE_INCLUDE_X in other files as well, they includes more QUICHE files which also needs the pragma diagnose ignore.

The header include that needs the pragma seems to be only quiche/quic/core/quic_connection.h; can we add a wrapper header with appropriate pragmas and have that include be the one that is included through the rest of Envoy code? Or have these pragmas added to quiche/quic/core/quic_connection.h via a quiche patch that Envoy adds while fetching the quiche dependency via bazel?

quic_connection.h is not the only one that needs the pragma, but most of them.

I see what you're trying to do. I'd say just use the pragmas directly in places that need them for now (and remove quic_includes_ignores.h), and I can take an action item to fix QUICHE. The correct fix here is to make QUICHE build without warnings, not to disable them in Envoy.

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk

Sorry looks like this needs a main merge as well?

alyssawilk · 2021-04-26T20:52:31Z

source/common/quic/quic_filter_manager_connection_impl.h

@@ -5,11 +5,14 @@
 #include "envoy/event/dispatcher.h"
 #include "envoy/network/connection.h"

+#define QUICHE_INCLUDE_1 "quiche/quic/core/quic_connection.h"


Do we have a plan to get rid of these? cc @DavidSchinazi yak shaver extrordinare

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-04-27T19:01:39Z

/retest

repokitteh-read-only · 2021-04-27T19:01:43Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk

Looks great - almost there!

alyssawilk · 2021-04-27T21:13:56Z

source/common/quic/quic_filter_manager_connection_impl.cc

    // Already detached from quic connection.
    return;
  }
+  if (!initialized_) {


Can we get here? I thought quicConnection would return null above if not initialized.

Good point! We need to distinguish between already closed and not initialized. In latter case, we need to retain close type and close in OnCanWrite().

test/integration/quic_protocol_integration_test.cc

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk

LGTM, assuming we're going to debug the re-excluded tests in follow-ups :-)

Commit Message: use max header size in HCM config to initialize quic session. Additional Description: change QuicSession::Initialize() and filter chain creation order to set max header list size for each session before Initialize(). Move Initialize() of Http3 connection from CodecClient to CodecClientProd. Added CodecClient::connect() interface. Change EnvoyQuicConnection not to directly inherit from QuicConnection. And renamed it. Fix a use-after-free bug in FakeUpstream which causes ASAN failure. Risk Level: low Testing: more protocol H3 tests Part of envoyproxy#2557 envoyproxy#12930 envoyproxy#14829 Signed-off-by: Dan Zhang <[email protected]> Signed-off-by: Gokul Nair <[email protected]>

danzh1989 added 30 commits March 9, 2021 14:49

enable protocol integration test

bb02ae6

Signed-off-by: Dan Zhang <[email protected]>

visibility

09fc660

Signed-off-by: Dan Zhang <[email protected]>

make protocol test pass

6ed3c94

Signed-off-by: Dan Zhang <[email protected]>

add more test

186a69d

Signed-off-by: Dan Zhang <[email protected]>

quic_pause_filter

66dbde0

Signed-off-by: Dan Zhang <[email protected]>

address comments

d5cbd00

Signed-off-by: Dan Zhang <[email protected]>

fix const reference

cf08311

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into addprotointegrationtest

97a3033

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into addprotointegrationtest

e00fd2b

Signed-off-by: Dan Zhang <[email protected]>

fix in filters

5682791

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into addprotointegrationtest

96c587e

Signed-off-by: Dan Zhang <[email protected]>

stop using udp_listener_name

725d93e

Signed-off-by: Dan Zhang <[email protected]>

fix IPv6 test failure

354a322

Signed-off-by: Dan Zhang <[email protected]>

fix setSan

2b94626

Signed-off-by: Dan Zhang <[email protected]>

fix asan and gcc

77cb937

Signed-off-by: Dan Zhang <[email protected]>

test size to large

4e9e56a

Signed-off-by: Dan Zhang <[email protected]>

test size to large

88398b8

Signed-off-by: Dan Zhang <[email protected]>

move config setup to config helper

5c11723

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into addprotointegrationtest

39e3f9a

Signed-off-by: Dan Zhang <[email protected]>

fix setSan

4c2b490

Signed-off-by: Dan Zhang <[email protected]>

change send single request

97292b2

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into addprotointegrationtest

fdc6189

Signed-off-by: Dan Zhang <[email protected]>

address comments

280fc25

Signed-off-by: Dan Zhang <[email protected]>

fix comment

3c02978

Signed-off-by: Dan Zhang <[email protected]>

comment about \0

c31225b

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into addprotointegrationtest

e1fd5d7

Signed-off-by: Dan Zhang <[email protected]>

fix typo

2dd1ae5

Signed-off-by: Dan Zhang <[email protected]>

plumb header size and underscore action

60015bf

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into headersize

55da362

Signed-off-by: Dan Zhang <[email protected]>

fail at QUICHE_CHECK

71eb14c

Signed-off-by: Dan Zhang <[email protected]>

danzh1989 added 4 commits April 24, 2021 00:51

Merge branch 'master' into headersize

f929456

Signed-off-by: Dan Zhang <[email protected]>

NOLINT(namespace-envoy)

6c38879

Signed-off-by: Dan Zhang <[email protected]>

typo

7b29d70

Signed-off-by: Dan Zhang <[email protected]>

connected_called_ NDEBUG

b6e3ace

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into headersize

09aca06

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk reviewed Apr 26, 2021

View reviewed changes

alyssawilk added the waiting label Apr 26, 2021

remove DDEBUG

5d77b16

Signed-off-by: Dan Zhang <[email protected]>

repokitteh-read-only bot removed the waiting label Apr 26, 2021

alyssawilk reviewed Apr 26, 2021

View reviewed changes

danzh1989 added 2 commits April 26, 2021 19:21

Merge branch 'master' into headersize

5d4469a

Signed-off-by: Dan Zhang <[email protected]>

Merge branch 'master' into headersize

42c08af

Signed-off-by: Dan Zhang <[email protected]>

remove quic_includes_ignores

1677acc

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk reviewed Apr 27, 2021

View reviewed changes

danzh1989 added 3 commits April 28, 2021 00:48

delay close

0651906

Signed-off-by: Dan Zhang <[email protected]>

disable ManyLargeRequestHeadersAccepted

55fd7b3

Signed-off-by: Dan Zhang <[email protected]>

spell dict

874a4b0

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk approved these changes Apr 28, 2021

View reviewed changes

antoniovicente approved these changes Apr 28, 2021

View reviewed changes

alyssawilk merged commit 26a60c2 into envoyproxy:main Apr 29, 2021

moderation mentioned this pull request Apr 29, 2021

regression test QUIC crash caused by the import #16234

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quiche: use max header size configured in HCM #15912

quiche: use max header size configured in HCM #15912

danzh2010 commented Apr 10, 2021 •

edited

Loading

danzh2010 commented Apr 26, 2021

repokitteh-read-only bot commented Apr 26, 2021

alyssawilk commented Apr 26, 2021

snowp commented Apr 26, 2021

alyssawilk left a comment

alyssawilk Apr 26, 2021

danzh2010 Apr 26, 2021

alyssawilk Apr 26, 2021

DavidSchinazi Apr 26, 2021

danzh2010 Apr 26, 2021

danzh2010 Apr 26, 2021

antoniovicente Apr 26, 2021 •

edited

Loading

danzh2010 Apr 27, 2021 •

edited

Loading

DavidSchinazi Apr 27, 2021

danzh2010 Apr 27, 2021

alyssawilk left a comment

alyssawilk Apr 26, 2021

danzh2010 commented Apr 27, 2021

repokitteh-read-only bot commented Apr 27, 2021

alyssawilk left a comment

alyssawilk Apr 27, 2021

danzh2010 Apr 28, 2021

alyssawilk left a comment

quiche: use max header size configured in HCM #15912

quiche: use max header size configured in HCM #15912

Conversation

danzh2010 commented Apr 10, 2021 • edited Loading

danzh2010 commented Apr 26, 2021

repokitteh-read-only bot commented Apr 26, 2021

alyssawilk commented Apr 26, 2021

snowp commented Apr 26, 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

antoniovicente Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

danzh2010 Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 commented Apr 27, 2021

repokitteh-read-only bot commented Apr 27, 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

alyssawilk left a comment

Choose a reason for hiding this comment

danzh2010 commented Apr 10, 2021 •

edited

Loading

antoniovicente Apr 26, 2021 •

edited

Loading

danzh2010 Apr 27, 2021 •

edited

Loading