http3: turning up more tests by alyssawilk · Pull Request #15853 · envoyproxy/envoy

alyssawilk · 2021-04-06T13:43:30Z

Risk Level: n/a
Testing: yes
Docs Changes: no
Release Notes: no

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

danzh2010

Nice! Thanks a lot for enabling H3 in this test!

danzh2010 · 2021-04-06T17:58:20Z

source/common/quic/envoy_quic_server_stream.cc


 void EnvoyQuicServerStream::maybeDecodeTrailers() {
  if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
-    ASSERT(!received_trailers().empty());


some h2 protocol test sends empty trailer?

yeah, the autonomous upstream sends empty trailers for H2/H3 by default

danzh2010 · 2021-04-06T18:26:11Z

test/integration/multiplexed_integration_test.cc

+
  initialize();
  std::string response;
  auto connection = createConnectionDriver(


RawConnectionDriver doesn't create a QUIC "connection".

agreed, but I think we need a similar facility to send invalid packets for a quic connection to make sure it's handled gracefully.

danzh2010 · 2021-04-06T18:29:28Z

test/integration/multiplexed_integration_test.cc

 // Send client headers, a GoAway and then a body and ensure the full request and
 // response are received.
 TEST_P(Http2IntegrationTest, GoAway) {
+  EXCLUDE_DOWNSTREAM_HTTP3; // QuicHttpClientConnectionImpl::goAway NOT_REACHED_GCOVR_EXCL_LINE


QUICHE doesn't support client sending GOAWAY as there is no need. AFAIK Envoy doesn't support server push either, why Envoy needs this feature?

That's fine. I think we should test that if the API is called we don't do anything harmful (it should be a no-op like it is for HTTP/1.1, rather than crashing) and we should also test to make sure if a client sends a goaway it is a no-op or treated as a protocol error and again doesn't crash.

It also tests that GOAWAY does not cause existing streams to terminate.

For HTTP/2. For HTTP/3 given it's a protocol error to send goaway client -> server, I think it probably would cause connection termination.

It's not a protocol error, but QUICHE client doesn't support it because QUICHE doesn't do server push. I can change NOT_REACHED_GCOVR_EXCL_LINE to log error and return. It won't affect out-going request.

isn't it a protocol error if an H3 server gets a goaway? cc @DavidSchinazi so I don't make things up =P

@alyssawilk that's no longer true in IETF QUIC. Client-to-server GOAWAY tells the server to stop using server push. https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-goaway

Ahh, thanks, I was looking at the wrong version of the spec, and Dan is of course just more up to date than me :-P

danzh2010

LGTM

danzh2010 · 2021-04-07T21:12:50Z

test/integration/multiplexed_integration_test.cc

 // Send client headers, a GoAway and then a body and ensure the full request and
 // response are received.
 TEST_P(Http2IntegrationTest, GoAway) {
+  EXCLUDE_DOWNSTREAM_HTTP3; // QuicHttpClientConnectionImpl::goAway NOT_REACHED_GCOVR_EXCL_LINE


danzh2010 · 2021-04-07T21:14:07Z

test/integration/multiplexed_integration_test.cc

+
  initialize();
  std::string response;
  auto connection = createConnectionDriver(


danzh2010 · 2021-04-07T21:20:21Z

test/integration/multiplexed_integration_test.cc

 TEST_P(Http2IntegrationTest, BadMagic) {
+  if (downstreamProtocol() == Http::CodecClient::Type::HTTP3) {
+    return; // protocol-specific framing.
+  }


EXCLUDE_DOWNSTREAM_HTTP3 means need to fix. This one doesn't?

correct. I don't think H3 has the equivalent of the initial H2 magic, so we don't have to replicate this test for H3.
I'll add a better comment next I'm in that file.

Or just rename the test BadHttp2Magic

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

yanavlasov · 2021-04-08T14:02:05Z

test/integration/multiplexed_integration_test.cc

 TEST_P(Http2IntegrationTest, BadMagic) {
+  if (downstreamProtocol() == Http::CodecClient::Type::HTTP3) {
+    return; // protocol-specific framing.
+  }


Or just rename the test BadHttp2Magic

yanavlasov · 2021-04-08T14:06:38Z

test/integration/multiplexed_integration_test.cc

 // Send client headers, a GoAway and then a body and ensure the full request and
 // response are received.
 TEST_P(Http2IntegrationTest, GoAway) {
+  EXCLUDE_DOWNSTREAM_HTTP3; // QuicHttpClientConnectionImpl::goAway NOT_REACHED_GCOVR_EXCL_LINE


It also tests that GOAWAY does not cause existing streams to terminate.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-04-12T18:09:46Z

ok, re-excluded the tests which were failing TSAN. @yanavlasov PTAL?

danzh2010 · 2021-04-12T21:36:09Z

test/integration/multiplexed_integration_test.cc

+  EXCLUDE_DOWNSTREAM_HTTP3;  // sort out timeouts
  testRouterRequestAndResponseWithBody(10 * 1024 * 1024, 10 * 1024 * 1024, false, false, nullptr,
-                                       10 * TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);
+                                       TSAN_TIMEOUT_FACTOR * TestUtility::DefaultTimeout);


Do they simply timed out?
Mind adding TODO for me to re-enable it after #15865?

Unsure. There's already a TODO to look into the excludes so I'd prefer to not do another push-and-ci cycle to update TODO if that's Ok?

Fair enough. I'll keep in mind of these tests.

Risk Level: n/a Testing: yes Docs Changes: no Release Notes: no Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk added 3 commits April 5, 2021 18:22

WIP

53cd6cf

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

http3: turning up the multiplexed integration test

4022687

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

clang

df4e7b6

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 assigned danzh2010 Apr 6, 2021

alyssawilk marked this pull request as ready for review April 6, 2021 17:42

danzh2010 reviewed Apr 6, 2021

View reviewed changes

alyssawilk assigned yanavlasov Apr 7, 2021

danzh2010 previously approved these changes Apr 7, 2021

View reviewed changes

alyssawilk added 2 commits April 8, 2021 08:20

Merge branch 'main' into mux2

16b749b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into mux2

c90dbe3

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

yanavlasov requested changes Apr 8, 2021

View reviewed changes

alyssawilk added 3 commits April 8, 2021 10:53

Merge branch 'main' into mux2

9eccbae

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

comment

91ca778

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

comments + merge fixes

ec4b8ec

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed danzh2010’s stale review via ec4b8ec April 8, 2021 15:04

alyssawilk added 2 commits April 8, 2021 13:08

more time?

923657f

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

re-disabling giant tests

a90445f

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the mux2 branch from 94c0526 to 4e2e549 Compare April 12, 2021 14:27

ffs

5a5f3f8

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the mux2 branch from 4e2e549 to 5a5f3f8 Compare April 12, 2021 15:10

danzh2010 reviewed Apr 12, 2021

View reviewed changes

danzh2010 approved these changes Apr 13, 2021

View reviewed changes

yanavlasov approved these changes Apr 13, 2021

View reviewed changes

alyssawilk merged commit a527216 into envoyproxy:main Apr 13, 2021

Monkeyanator pushed a commit to Monkeyanator/envoy that referenced this pull request Apr 20, 2021

http3: turning up more tests (envoyproxy#15853)

e49d80b

Risk Level: n/a Testing: yes Docs Changes: no Release Notes: no Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk deleted the mux2 branch February 28, 2022 21:25

Conversation

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

Uh oh!

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!

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!

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 commented Apr 12, 2021

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alyssawilk commented Apr 6, 2021 •

edited

Loading