quic: enabling some more tests #15381

alyssawilk · 2021-03-09T15:11:43Z

Enabling (most of the) the downstream tests for quic upstream for more coverage.
Enabling cookie tests given Dan's fix
Fixing a couple of tests with filters assuming headers-come-with-fin
Defaulting header size to Envoy defaults.

Risk Level: low (quic / test code)
Testing: yes
Docs Changes: n/a
Release Notes: n/a
part of #14829

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

danzh2010

Thanks for fixing some of the quic issues!
FYI, I'm working on adding downstream Http3 tests: https://github.com/envoyproxy/envoy/compare/main...danzh2010:addprotointegrationtest?expand=1

danzh2010 · 2021-03-10T20:57:23Z

test/integration/filters/add_body_filter.cc

    }

    return Http::FilterHeadersStatus::Continue;
  }

+  Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override {
+    // For HTTP/3, there's no headers-only streams so the data will be added here.
+    ASSERT(end_stream == false || decoder_callbacks_->connection()->streamInfo().protocol());


What is decoder_callbacks_->connection()->streamInfo().protocol() for? comment?

danzh2010 · 2021-03-10T21:07:29Z

test/extensions/quic_listeners/quiche/integration/quic_protocol_integration_test.cc

+// TODO(danzh) this should run with QUIC downstream instead of HTTP2.
+INSTANTIATE_TEST_SUITE_P(Protocols, DownstreamProtocolIntegrationTest,
+                         testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams(
+                             {Http::CodecClient::Type::HTTP2}, {FakeHttpConnection::Type::HTTP3})),


suggest to test HTTP and HTTP2 for downstream as well. I will add another test suite for downstream HTTP3.

those are tested in test/integration/instantiate_protocol_integration_test.cc so I think we're good?

I find a test for h3 downstream work for H1 but not H2 upstream. So worth to run both I think.

How about I land this as-is, and after yours is, see if adding the extras nets anything new? I'm mildly skeptical it will and either way I think incremental change should be fine.

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

snowp · 2021-03-11T16:31:05Z

test/integration/filters/add_body_filter.cc

+    } else {
+      headers.removeContentLength();


Why would we hit this if this is used with header only requests?

header only request doesn't mean the fin bit arrives with the headers, in the case of QUIC.
will comment.

snowp · 2021-03-11T16:34:01Z

test/integration/filters/add_body_filter.cc

+    // For HTTP/3, the only time the protocol is set at the connection level, there's no
+    // headers-only streams so the data will be added here.
+    ASSERT(end_stream == false || decoder_callbacks_->connection()->streamInfo().protocol());


This still seems a bit confusing to me, when do we expect end_stream=false and when do we expect the protocol to be set?

snowp · 2021-03-11T16:36:27Z

test/integration/filters/continue_headers_only_inject_body_filter.cc

  }

 private:
  constexpr static absl::string_view body_ = "body";
+  // For HTTP/3 upstream, the headers and fin will arrive separately.


so when we say that HTTP/3 doesn't have header only its because we'd at least expect to see a headers(end_stream=false) with an empty data(end_stream=true)? Seems not great that we have all these differences in callback usage

largely these tests make sure you can go from
headers + fin
to
headers + body + fin.
for QUIC, we just insert the data at a different part of the pipeline. I still think it's important to make sure it works.

Makes sense! I think I'm just worried about filter authors having to be aware of all these variations in callback patterns, but this isn't something we need to address in this PR

snowp · 2021-03-11T16:37:48Z

test/integration/protocol_integration_test.cc

@@ -501,6 +500,11 @@ TEST_P(ProtocolIntegrationTest, Retry) {
        TestUtility::findCounter(stats, "cluster.cluster_0.http2.tx_reset");
    ASSERT_NE(nullptr, counter);
    EXPECT_EQ(1L, counter->value());
+  } else if (upstreamProtocol() == FakeHttpConnection::Type::HTTP3) {
+    Stats::CounterSharedPtr counter =
+        TestUtility::findCounter(stats, "cluster.cluster_0.upstream_rq_tx_reset");


do we not want a http3 stats scope to match the other branches here?

I think landing this will help a few tests over in #15424 so would prefer to fix in a follow-up. added TODO!

snowp · 2021-03-11T16:38:23Z

test/integration/protocol_integration_test.cc

@@ -731,6 +736,7 @@ TEST_P(DownstreamProtocolIntegrationTest, RetryAttemptCountHeader) {
 // The retry priority will always target P1, which would otherwise never be hit due to P0 being
 // healthy.
 TEST_P(DownstreamProtocolIntegrationTest, RetryPriority) {
+  EXCLUDE_UPSTREAM_HTTP3;


maybe include some indication of why these are failing under http/3?

there's a TODO to address all of these where I declare the macro so if I can avoid classifying for now that'd be helpful (I have WIP PRs for about 80% but I don't recall offhand which are which =P)

Sure, sgtm!

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

danzh2010

LGTM

quic: enabling some more tests

df50101

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

alyssawilk requested review from ggreenway and mattklein123 as code owners March 9, 2021 15:11

alyssawilk force-pushed the upstream_quic_test_fixes2 branch from 3a4c278 to ebb1798 Compare March 9, 2021 18:43

spelling

aaddfd5

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

alyssawilk force-pushed the upstream_quic_test_fixes2 branch from ebb1798 to aaddfd5 Compare March 9, 2021 20:02

dio assigned danzh2010 Mar 10, 2021

alyssawilk added 3 commits March 10, 2021 08:42

todo

4cedd0f

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

more tests

d5ccacc

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

format

e37e311

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

danzh2010 reviewed Mar 10, 2021

View reviewed changes

alyssawilk added 2 commits March 10, 2021 16:43

comments

55b6e6a

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

format

682b110

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

alyssawilk assigned snowp Mar 11, 2021

snowp suggested changes Mar 11, 2021

View reviewed changes

alyssawilk added 2 commits March 11, 2021 13:10

comments

d202922

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

TODO

5a593c0

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

danzh2010 approved these changes Mar 11, 2021

View reviewed changes

snowp approved these changes Mar 15, 2021

View reviewed changes

alyssawilk merged commit 68b7a64 into envoyproxy:main Mar 15, 2021

alyssawilk deleted the upstream_quic_test_fixes2 branch July 15, 2021 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quic: enabling some more tests #15381

quic: enabling some more tests #15381

alyssawilk commented Mar 9, 2021

danzh2010 left a comment

danzh2010 Mar 10, 2021

danzh2010 Mar 10, 2021

alyssawilk Mar 10, 2021

danzh2010 Mar 11, 2021

alyssawilk Mar 11, 2021

danzh2010 Mar 11, 2021

snowp Mar 11, 2021

alyssawilk Mar 11, 2021

snowp Mar 11, 2021

snowp Mar 11, 2021

alyssawilk Mar 11, 2021

snowp Mar 11, 2021

snowp Mar 11, 2021

alyssawilk Mar 11, 2021

snowp Mar 11, 2021

alyssawilk Mar 11, 2021

snowp Mar 11, 2021

danzh2010 left a comment

quic: enabling some more tests #15381

quic: enabling some more tests #15381

Conversation

alyssawilk commented Mar 9, 2021

danzh2010 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

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 left a comment

Choose a reason for hiding this comment