Skip to content

quiche: client session supports creating bidi stream #17543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alyssawilk merged 8 commits into from
Aug 10, 2021
Merged

quiche: client session supports creating bidi stream #17543

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c275f94
create unidirectional read stream
danzh1989 Jul 29, 2021
531cfbc
Merge branch 'master' into pendingstream
danzh1989 Jul 30, 2021
38774bb
format
danzh1989 Jul 30, 2021
dcf9fef
Merge branch 'master' into pendingstream
danzh1989 Jul 30, 2021
4392432
not create server initiated stream
danzh1989 Jul 30, 2021
fa24b94
return nullptr for pending stream
danzh1989 Aug 5, 2021
25ffac5
Merge branch 'master' into pendingstream
danzh1989 Aug 9, 2021
4b84ac3
close connection for server push stream
danzh1989 Aug 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions source/common/quic/envoy_quic_client_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,14 @@ std::unique_ptr<quic::QuicSpdyClientStream> EnvoyQuicClientSession::CreateClient
}

quic::QuicSpdyStream* EnvoyQuicClientSession::CreateIncomingStream(quic::QuicStreamId /*id*/) {
// Disallow server initiated stream.
NOT_REACHED_GCOVR_EXCL_LINE;
// Envoy doesn't support server initiated stream.
return nullptr;
}

quic::QuicSpdyStream*
EnvoyQuicClientSession::CreateIncomingStream(quic::PendingStream* /*pending*/) {
// Disallow server initiated stream.
NOT_REACHED_GCOVR_EXCL_LINE;
// Envoy doesn't support server push.
return nullptr;
}

bool EnvoyQuicClientSession::hasDataToWrite() { return HasDataToWrite(); }
Expand Down
10 changes: 0 additions & 10 deletions source/common/quic/envoy_quic_client_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,16 +44,6 @@ EnvoyQuicClientStream::EnvoyQuicClientStream(
"Send buffer limit should be larger than 8KB.");
}

EnvoyQuicClientStream::EnvoyQuicClientStream(
quic::PendingStream* pending, quic::QuicSpdyClientSession* client_session,
quic::StreamType type, Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options)
: quic::QuicSpdyClientStream(pending, client_session, type),
EnvoyQuicStream(
static_cast<uint32_t>(GetReceiveWindow().value()), *filterManagerConnection(),
[this]() { runLowWatermarkCallbacks(); }, [this]() { runHighWatermarkCallbacks(); },
stats, http3_options) {}

@antoniovicente antoniovicente Aug 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These changes seem unrelated. Was this code unused?

@danzh2010 danzh2010 Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are only called by server push stream.

Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap& headers,
bool end_stream) {
// Required headers must be present. This can only happen by some erroneous processing after the
Expand Down
3 changes: 0 additions & 3 deletions source/common/quic/envoy_quic_client_stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream,
EnvoyQuicClientStream(quic::QuicStreamId id, quic::QuicSpdyClientSession* client_session,
quic::StreamType type, Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options);
EnvoyQuicClientStream(quic::PendingStream* pending, quic::QuicSpdyClientSession* client_session,
quic::StreamType type, Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options);

void setResponseDecoder(Http::ResponseDecoder& decoder) { response_decoder_ = &decoder; }

Expand Down
2 changes: 1 addition & 1 deletion source/common/quic/envoy_quic_server_session.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::QuicStr
quic::QuicSpdyStream*
EnvoyQuicServerSession::CreateIncomingStream(quic::PendingStream* /*pending*/) {
// Only client side server push stream should trigger this call.

@alyssawilk alyssawilk Aug 3, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I'm a bit confused - if the client side sends a server push stream when it's not negotiated why don't we close the connection for it doing "illegal" things? Should that be a TODO for quiche to fix upstream?

@danzh2010 danzh2010 Aug 3, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not just server push stream actually, instead, any client-initiated uni-directional stream with type byte other than the predesignated ones, QPack encode/decode streams, etc. QUICHE session should accept such stream as its extensions may wants to create such special streams. So it's not illegal for QUICHE.

I can close the connection in Envoy code if we don't have such use case in near future. Or update the comment to be more accurate?

@antoniovicente antoniovicente Aug 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIRC current QUIC support in Envoy is only for HTTP transactions which are always bi-directional. Closing the connections seems appropriate until other use cases are added.

@danzh2010 danzh2010 Aug 6, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#17618 includes a QUICHE change that close the connection in this case. I will continue update this PR after that one is checked in.

NOT_REACHED_GCOVR_EXCL_LINE;
return nullptr;
}

quic::QuicSpdyStream* EnvoyQuicServerSession::CreateOutgoingBidirectionalStream() {
Expand Down
16 changes: 0 additions & 16 deletions source/common/quic/envoy_quic_server_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,22 +49,6 @@ EnvoyQuicServerStream::EnvoyQuicServerStream(
"Send buffer limit should be larger than 8KB.");
}

EnvoyQuicServerStream::EnvoyQuicServerStream(
quic::PendingStream* pending, quic::QuicSpdySession* session, quic::StreamType type,
Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action)
: quic::QuicSpdyServerStreamBase(pending, session, type),
EnvoyQuicStream(
// This should be larger than 8k to fully utilize congestion control
// window. And no larger than the max stream flow control window for
// the stream to buffer all the data.
static_cast<uint32_t>(GetReceiveWindow().value()), *filterManagerConnection(),
[this]() { runLowWatermarkCallbacks(); }, [this]() { runHighWatermarkCallbacks(); },
stats, http3_options),
headers_with_underscores_action_(headers_with_underscores_action) {}

void EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) {
ASSERT(headers.Status()->value() == "100");
encodeHeaders(headers, false);
Expand Down
6 changes: 0 additions & 6 deletions source/common/quic/envoy_quic_server_stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,6 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase,
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action);

EnvoyQuicServerStream(quic::PendingStream* pending, quic::QuicSpdySession* session,
quic::StreamType type, Http::Http3::CodecStats& stats,
const envoy::config::core::v3::Http3ProtocolOptions& http3_options,
envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
headers_with_underscores_action);

void setRequestDecoder(Http::RequestDecoder& decoder) { request_decoder_ = &decoder; }

// Http::StreamEncoder
Expand Down
1 change: 1 addition & 0 deletions test/common/quic/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ envoy_cc_test(
"//test/mocks/stats:stats_mocks",
"//test/test_common:logging_lib",
"//test/test_common:simulated_time_system_lib",
"@com_googlesource_quiche//:quic_test_tools_session_peer_lib",
],
)

Expand Down
17 changes: 17 additions & 0 deletions test/common/quic/envoy_quic_client_session_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "quiche/quic/core/crypto/null_encrypter.h"
#include "quiche/quic/test_tools/crypto_test_utils.h"
#include "quiche/quic/test_tools/quic_session_peer.h"
#include "quiche/quic/test_tools/quic_test_utils.h"

#if defined(__GNUC__)
Expand Down Expand Up @@ -324,5 +325,21 @@ TEST_F(EnvoyQuicClientSessionTest, ConnectionClosePopulatesQuicVersionStats) {
EXPECT_EQ(1U, TestUtility::findCounter(store_, "http3.quic_version_rfc_v1")->value());
}

TEST_F(EnvoyQuicClientSessionTest, IncomingUnidirectionalReadStream) {
// IETF stream 3 is server initiated uni-directional stream.
quic::QuicStreamId stream_id = 3u;
auto payload = std::make_unique<char[]>(8);
quic::QuicDataWriter payload_writer(8, payload.get());
EXPECT_TRUE(payload_writer.WriteVarInt62(1ul));
quic::QuicStreamFrame stream_frame(stream_id, false, 0, absl::string_view(payload.get(), 1));
envoy_quic_session_.OnStreamFrame(stream_frame);
EXPECT_FALSE(quic::test::QuicSessionPeer::IsStreamCreated(&envoy_quic_session_, stream_id));

stream_id = 1u;
quic::QuicStreamFrame stream_frame2(stream_id, false, 0, "aaa");
envoy_quic_session_.OnStreamFrame(stream_frame2);
EXPECT_FALSE(quic::test::QuicSessionPeer::IsStreamCreated(&envoy_quic_session_, stream_id));
}

} // namespace Quic
} // namespace Envoy
14 changes: 14 additions & 0 deletions test/common/quic/envoy_quic_server_session_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,5 +1023,19 @@ TEST_F(EnvoyQuicServerSessionTest, SendBufferWatermark) {
EXPECT_TRUE(stream2->write_side_closed());
}

TEST_F(EnvoyQuicServerSessionTest, IncomingUnidirectionalReadStream) {
installReadFilter();
Http::MockRequestDecoder request_decoder;
Http::MockStreamCallbacks stream_callbacks;
// IETF stream 2 is client initiated uni-directional stream.
quic::QuicStreamId stream_id = 2u;
auto payload = std::make_unique<char[]>(8);
quic::QuicDataWriter payload_writer(8, payload.get());
EXPECT_TRUE(payload_writer.WriteVarInt62(1ul));
EXPECT_CALL(http_connection_callbacks_, newStream(_, false)).Times(0u);
quic::QuicStreamFrame stream_frame(stream_id, false, 0, absl::string_view(payload.get(), 1));
envoy_quic_session_.OnStreamFrame(stream_frame);
}

} // namespace Quic
} // namespace Envoy