Skip to content

quiche: add 100 continue support#14246

Merged
alyssawilk merged 5 commits intoenvoyproxy:mainfrom
danzh2010:100continue
Jan 27, 2021
Merged

quiche: add 100 continue support#14246
alyssawilk merged 5 commits intoenvoyproxy:mainfrom
danzh2010:100continue

Conversation

@danzh2010
Copy link
Contributor

@danzh2010 danzh2010 commented Dec 2, 2020 •
edited
Loading

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

Commit Message: Add 100 continue support to envoy_quic_client_stream. Fix some quic stream and stream decoder interaction issues.
Additional Description: issues fixed:
Following StreamDecoder callbacks shouldn't be called if decodeHeaders|Data|Traiers() causes stream is reset or connection close.
decodeHeaders|Data|Traiers() shouldn't be called if quic header/trailers are too large.
StreamCallback::onResetStream() shouldn't be called if end_stream is decoded.

Risk Level: low, not in use
Testing: Added more stream unit tests and an e2e test

Part of #2557

@danzh1989
add 100 continue
00c5734 
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh1989
add tests
5bafa4a 
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Contributor Author

danzh2010 commented Dec 2, 2020

/assign @alyssawilk

alyssawilk
alyssawilk reviewed Dec 3, 2020
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome - so exciting to see the corner cases in QUIC getting handled!

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc Outdated
response_decoder_->decodeHeaders(
quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list),
/*end_stream=*/fin);
ENVOY_STREAM_LOG(debug, "Receive headers: {}.", *this, header_list.DebugString());
Copy link
Contributor

@alyssawilk alyssawilk Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Receive -> Received, here and below

Copy link
Contributor Author

@danzh2010 danzh2010 Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc Outdated
const uint64_t status = Http::Utility::getResponseStatus(*headers);
if (status >= 100 && status < 200) {
// These are Informational 1xx headers, not the actual response headers.
ENVOY_STREAM_LOG(debug, "Received informational response code: {}", *this, status);
Copy link
Contributor

@alyssawilk alyssawilk Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this redundant given full headers logged above?

Copy link
Contributor Author

@danzh2010 danzh2010 Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc Outdated
std::unique_ptr<Http::ResponseHeaderMapImpl> headers =
quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list);
const uint64_t status = Http::Utility::getResponseStatus(*headers);
if (status >= 100 && status < 200) {
Copy link
Contributor

@alyssawilk alyssawilk Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeUtility::is1xx ?

Copy link
Contributor Author

@danzh2010 danzh2010 Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@danzh1989
debug logging
85465ff 
Signed-off-by: Dan Zhang <danzh@google.com>
alyssawilk
alyssawilk reviewed Dec 7, 2020
View reviewed changes
source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc
std::unique_ptr<Http::ResponseHeaderMapImpl> headers =
quicHeadersToEnvoyHeaders<Http::ResponseHeaderMapImpl>(header_list);
const uint64_t status = Http::Utility::getResponseStatus(*headers);
if (Http::CodeUtility::is1xx(status)) {
Copy link
Contributor

@alyssawilk alyssawilk Dec 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what happens with 1xx headers which are not 100-continue?

For non-QUIC we send them up via decodeHeaders.

Copy link
Contributor Author

@danzh2010 danzh2010 Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know that. Currently we drop these headers on the floor. Will fix it.
Question please: QUIC doens't support 101 response code: https://tools.ietf.org/html/draft-ietf-quic-http-32#section-4.3. What shall we do in that case?

Copy link
Contributor

@alyssawilk alyssawilk Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh fair point. maybe treat it as invalid and fast-fail the response then?

Copy link
Contributor Author

@danzh2010 danzh2010 Jan 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-wrote 1xx header handling logic here. PTAL!

@github-actions
Copy link

github-actions bot commented Jan 14, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 14, 2021
Base automatically changed from master to main January 15, 2021 23:02
@github-actions
Copy link

github-actions bot commented Jan 23, 2021

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Jan 23, 2021
@danzh1989
pass non-100 headers
48597cd 
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Contributor Author

danzh2010 commented Jan 25, 2021

Can someone reopen this PR?

@ggreenway ggreenway reopened this Jan 25, 2021
@danzh2010 danzh2010 mentioned this pull request Jan 25, 2021
Merged
@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 25, 2021
@danzh2010
Copy link
Contributor Author

danzh2010 commented Jan 26, 2021

/retest

@repokitteh-read-only
Copy link

repokitteh-read-only bot commented Jan 26, 2021

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.
Check envoy-presubmit didn't fail.

🐱

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

see: more, trace.

alyssawilk
alyssawilk reviewed Jan 26, 2021
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo one test/TODO

test/extensions/quic_listeners/quiche/integration/quic_http_integration_test.cc
}

// Multiple 1xx before the request completes.
TEST_P(QuicHttpIntegrationTest, EnvoyProxyingEarlyMultiple1xx) {
Copy link
Contributor

@alyssawilk alyssawilk Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think it's worth a test of the reset for 101 here, or want to TODO it for later?

Copy link
Contributor Author

@danzh2010 danzh2010 Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is already unit tested and will be covered by protocol integration test once HTTP3 is added into the test.

Copy link
Contributor Author

@danzh2010 danzh2010 Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misunderstood the test for non-100 case. 101 response status is indeed a HTTP3 specific case. Added an integration test here.

alyssawilk
alyssawilk previously approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me :-)

@danzh1989
added e2e test for 101 response code
b7c9a2d 
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Contributor Author

danzh2010 commented Jan 26, 2021

Works for me :-)

Sorry for reverting my previous response, I added an e2e test intead. Mind taking another look?

alyssawilk
alyssawilk approved these changes Jan 26, 2021
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better, thanks!

@danzh2010
Copy link
Contributor Author

danzh2010 commented Jan 27, 2021

Can this PR be merged now?

@alyssawilk alyssawilk merged commit 53124e8 into envoyproxy:main Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggreenway ggreenway Awaiting requested review from ggreenway

@mattklein123 mattklein123 Awaiting requested review from mattklein123

1 more reviewer

@alyssawilk alyssawilk alyssawilk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@alyssawilk alyssawilk

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@danzh2010 @alyssawilk @ggreenway @danzh1989