quiche: use max header size configured in HCM by danzh2010 · Pull Request #15912 · envoyproxy/envoy

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 <danzh@google.com>

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 <danzh@google.com>

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

 #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 <danzh@google.com>

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

 #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 <danzh@google.com>

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 <danzh@google.com>

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 <danzh@google.com>

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 <danzh@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

danzh1989 added 30 commits March 9, 2021 14:49

enable protocol integration test

bb02ae6

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

visibility

09fc660

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

make protocol test pass

6ed3c94

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

add more test

186a69d

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

quic_pause_filter

66dbde0

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

address comments

d5cbd00

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

fix const reference

cf08311

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

Merge branch 'master' into addprotointegrationtest

97a3033

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

Merge branch 'master' into addprotointegrationtest

e00fd2b

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

fix in filters

5682791

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

Merge branch 'master' into addprotointegrationtest

96c587e

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

stop using udp_listener_name

725d93e

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

fix IPv6 test failure

354a322

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

fix setSan

2b94626

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

fix asan and gcc

77cb937

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

test size to large

4e9e56a

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

test size to large

88398b8

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

move config setup to config helper

5c11723

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

Merge branch 'master' into addprotointegrationtest

39e3f9a

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

fix setSan

4c2b490

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

change send single request

97292b2

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

Merge branch 'master' into addprotointegrationtest

fdc6189

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

address comments

280fc25

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

fix comment

3c02978

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

comment about \0

c31225b

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

Merge branch 'master' into addprotointegrationtest

e1fd5d7

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

fix typo

2dd1ae5

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

plumb header size and underscore action

60015bf

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

Merge branch 'master' into headersize

55da362

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

fail at QUICHE_CHECK

71eb14c

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

danzh1989 added 4 commits April 24, 2021 00:51

Merge branch 'master' into headersize

f929456

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

NOLINT(namespace-envoy)

6c38879

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

typo

7b29d70

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

connected_called_ NDEBUG

b6e3ace

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

Merge branch 'master' into headersize

09aca06

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

alyssawilk reviewed Apr 26, 2021

View reviewed changes

alyssawilk added the waiting label Apr 26, 2021

remove DDEBUG

5d77b16

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

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 <danzh@google.com>

Merge branch 'master' into headersize

42c08af

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

remove quic_includes_ignores

1677acc

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

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 <danzh@google.com>

disable ManyLargeRequestHeadersAccepted

55fd7b3

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

spell dict

874a4b0

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

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

Conversation

danzh2010 commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danzh2010 commented Apr 26, 2021

Uh oh!

repokitteh-read-only bot commented Apr 26, 2021

Uh oh!

alyssawilk commented Apr 26, 2021

Uh oh!

snowp commented Apr 26, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

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!

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!

antoniovicente Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Apr 27, 2021

Uh oh!

repokitteh-read-only bot commented Apr 27, 2021

Uh oh!

alyssawilk 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!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

danzh2010 commented Apr 10, 2021 •

edited

Loading

antoniovicente Apr 26, 2021 •

edited

Loading

danzh2010 Apr 27, 2021 •

edited

Loading