Skip to content
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

quiche: enable downstream HTTP3 in quic_protocol_integration_test #15424

Merged
merged 27 commits into from
Mar 24, 2021
Merged

quiche: enable downstream HTTP3 in quic_protocol_integration_test #15424

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
bb02ae6
enable protocol integration test
danzh1989 Mar 9, 2021
09fc660
visibility
danzh1989 Mar 9, 2021
6ed3c94
make protocol test pass
danzh1989 Mar 10, 2021
186a69d
add more test
danzh1989 Mar 11, 2021
66dbde0
quic_pause_filter
danzh1989 Mar 11, 2021
d5cbd00
address comments
danzh1989 Mar 13, 2021
cf08311
fix const reference
danzh1989 Mar 15, 2021
97a3033
Merge branch 'master' into addprotointegrationtest
danzh1989 Mar 15, 2021
e00fd2b
Merge branch 'master' into addprotointegrationtest
danzh1989 Mar 16, 2021
5682791
fix in filters
danzh1989 Mar 16, 2021
96c587e
Merge branch 'master' into addprotointegrationtest
danzh1989 Mar 16, 2021
725d93e
stop using udp_listener_name
danzh1989 Mar 16, 2021
354a322
fix IPv6 test failure
danzh1989 Mar 17, 2021
2b94626
fix setSan
danzh1989 Mar 17, 2021
77cb937
fix asan and gcc
danzh1989 Mar 17, 2021
4e9e56a
test size to large
danzh1989 Mar 17, 2021
88398b8
test size to large
danzh1989 Mar 18, 2021
5c11723
move config setup to config helper
danzh1989 Mar 18, 2021
39e3f9a
Merge branch 'master' into addprotointegrationtest
danzh1989 Mar 18, 2021
4c2b490
fix setSan
danzh1989 Mar 19, 2021
97292b2
change send single request
danzh1989 Mar 22, 2021
fdc6189
Merge branch 'master' into addprotointegrationtest
danzh1989 Mar 22, 2021
280fc25
address comments
danzh1989 Mar 22, 2021
3c02978
fix comment
danzh1989 Mar 22, 2021
c31225b
comment about \0
danzh1989 Mar 23, 2021
e1fd5d7
Merge branch 'master' into addprotointegrationtest
danzh1989 Mar 23, 2021
2dd1ae5
fix typo
danzh1989 Mar 23, 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
1 change: 1 addition & 0 deletions source/common/http/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class AlpnNameValues {
const std::string Http11 = "http/1.1";
const std::string Http2 = "h2";
const std::string Http2c = "h2c";
const std::string Http3 = "h3";
};

using AlpnNames = ConstSingleton<AlpnNameValues>;
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/quic_listeners/quiche/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ envoy_cc_library(
srcs = ["quic_filter_manager_connection_impl.cc"],
hdrs = ["quic_filter_manager_connection_impl.h"],
tags = ["nofips"],
visibility = ["//test:__subpackages__"],
deps = [
":envoy_quic_connection_lib",
":envoy_quic_simulated_watermark_buffer_lib",
Expand Down Expand Up @@ -385,6 +386,7 @@ envoy_cc_extension(
# Needed to verify that a quic specific configuration is used for quic transport socket.
extra_visibility = [
"//source/server:__subpackages__",
"//test:__subpackages__",
],
security_posture = "unknown",
tags = ["nofips"],
Expand Down
3 changes: 3 additions & 0 deletions source/extensions/quic_listeners/quiche/active_quic_listener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ ActiveQuicListener::ActiveQuicListener(
&listener_config),
dispatcher_(dispatcher), version_manager_(quic::CurrentSupportedVersions()),
kernel_worker_routing_(kernel_worker_routing) {
// This flag fix a QUICHE issue which may crash Envoy during connection close.
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a TODO and/or link to a follow up issue? Do we plan on removing this later?

Copy link
Contributor

Choose a reason for hiding this comment

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

quiche flags are like our runtime guards only they start false - it'll move to true by default and get removed over some number of import cycles.

SetQuicReloadableFlag(quic_single_ack_in_packet2, true);
alyssawilk marked this conversation as resolved.
Show resolved Hide resolved

if (Runtime::LoaderSingleton::getExisting()) {
enabled_.emplace(Runtime::FeatureFlag(enabled, Runtime::LoaderSingleton::get()));
}
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/quic_listeners/quiche/client_connection_factory_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ QuicClientConnectionFactoryImpl::createQuicNetworkConnection(
Http::PersistentQuicInfo& info, Event::Dispatcher& dispatcher,
Network::Address::InstanceConstSharedPtr server_addr,
Network::Address::InstanceConstSharedPtr local_addr) {
// This flag fix a QUICHE issue which may crash Envoy during connection close.
Copy link
Member

Choose a reason for hiding this comment

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

TODO/link?

SetQuicReloadableFlag(quic_single_ack_in_packet2, true);
PersistentQuicInfoImpl* info_impl = reinterpret_cast<PersistentQuicInfoImpl*>(&info);

auto connection = std::make_unique<EnvoyQuicClientConnection>(
Expand Down
14 changes: 13 additions & 1 deletion source/extensions/quic_listeners/quiche/envoy_quic_client_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,19 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
? static_cast<quic::QuicStream*>(this)
: (dynamic_cast<quic::QuicSpdySession*>(session())->headers_stream());
const uint64_t bytes_to_send_old = writing_stream->BufferedDataBytes();
WriteHeaders(envoyHeadersToSpdyHeaderBlock(headers), end_stream, nullptr);
auto spdy_headers = envoyHeadersToSpdyHeaderBlock(headers);
if (headers.Method() && headers.Method()->value() == "CONNECT") {
// It is a bytestream connect and should have :path and :protocol set accordingly
// As HTTP/1.1 does not require a path for CONNECT, we may have to add one
// if shifting codecs. For now, default to "/" - this can be made
// configurable if necessary.
// https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream;
if (!headers.Path()) {
spdy_headers[":path"] = "/";
}
}
WriteHeaders(std::move(spdy_headers), end_stream, nullptr);
local_end_stream_ = end_stream;
const uint64_t bytes_to_send_new = writing_stream->BufferedDataBytes();
ASSERT(bytes_to_send_old <= bytes_to_send_new);
Expand Down
12 changes: 9 additions & 3 deletions source/extensions/quic_listeners/quiche/envoy_quic_server_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "common/buffer/buffer_impl.h"
#include "common/http/header_map_impl.h"
#include "common/common/assert.h"
#include "common/http/header_utility.h"

namespace Envoy {
namespace Quic {
Expand Down Expand Up @@ -160,9 +161,14 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
if (fin) {
end_stream_decoded_ = true;
}
request_decoder_->decodeHeaders(
quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(header_list),
/*end_stream=*/fin);
std::unique_ptr<Http::RequestHeaderMapImpl> headers =
quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(header_list);
if (!Http::HeaderUtility::authorityIsValid(headers->Host()->value().getStringView())) {
stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers");
return;
}
request_decoder_->decodeHeaders(std::move(headers),
/*end_stream=*/fin);
ConsumeHeaderList();
}

Expand Down
4 changes: 4 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "envoy/config/core/v3/base.pb.h"

#include "common/network/socket_option_factory.h"
#include "common/network/utility.h"

namespace Envoy {
namespace Quic {
Expand Down Expand Up @@ -118,6 +119,9 @@ Network::ConnectionSocketPtr
createConnectionSocket(Network::Address::InstanceConstSharedPtr& peer_addr,
Network::Address::InstanceConstSharedPtr& local_addr,
const Network::ConnectionSocket::OptionsSharedPtr& options) {
if (local_addr == nullptr) {
alyssawilk marked this conversation as resolved.
Show resolved Hide resolved
local_addr = Network::Utility::getLocalAddress(peer_addr->ip()->version());
}
auto connection_socket = std::make_unique<Network::ConnectionSocketImpl>(
Network::Socket::Type::Datagram, local_addr, peer_addr);
connection_socket->addOptions(Network::SocketOptionFactory::buildIpPacketInfoOptions());
Expand Down
5 changes: 4 additions & 1 deletion source/extensions/quic_listeners/quiche/envoy_quic_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ std::unique_ptr<T> spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& he
for (auto entry : header_block) {
// TODO(danzh): Avoid temporary strings and addCopy() with string_view.
std::string key(entry.first);
headers->addCopy(Http::LowerCaseString(key), entry.second);
std::vector<absl::string_view> values = absl::StrSplit(entry.second, '\0');
Copy link
Contributor

Choose a reason for hiding this comment

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

I was surprised to see splitting on \0 and so I went looking around and found the definition in spdy/core/spdy_header_block.h which confirms it, but not all the time:

  // If a header with the key is already present, then append the value to the
  // existing header value, NUL ("\0") separated unless the key is cookie, in
  // which case the separator is "; ".
  // If there is no such key, a new header with the key and value is added.
  void AppendValueOrAddHeader(const absl::string_view key,
                              const absl::string_view value);

I've never worked with that library before but I wonder if it supplies some sort of split API we should be using rather than calling StrSplit directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Josh, is your concern that the header may be split by \0 or ;? I think the base Enovy classes can handle ; (e.g. parseCookieValue) but would basically fail to recognize quic-multi-valued-headers with \0 because HTTP/2 and below don't support that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was mostly just thinking that the \0 delim looks like an implementation detail of spdy_header_block.h and the detail should ideally be abstracted away at that level.

And yes I was also wondering specifically about what the behavior would be with cookies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, SpdyHeaderBlock doesn't have such API to split the \0 delimited header values. Even quic qpack code has to write its own logic to handle this special case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments here might help at least clear up the mysticism, if it's really impossible to create an abstraction boundary around this.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for more comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

for (const absl::string_view& value : values) {
headers->addCopy(Http::LowerCaseString(key), value);
alyssawilk marked this conversation as resolved.
Show resolved Hide resolved
}
}
return headers;
}
Expand Down
5 changes: 5 additions & 0 deletions source/extensions/quic_listeners/quiche/quic_filter_manager_connection_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
#include "extensions/quic_listeners/quiche/envoy_quic_simulated_watermark_buffer.h"

namespace Envoy {

class TestPauseFilterForQuic;

namespace Quic {

// Act as a Network::Connection to HCM and a FilterManager to FilterFactoryCb.
Expand Down Expand Up @@ -123,6 +126,8 @@ class QuicFilterManagerConnectionImpl : public Network::ConnectionImplBase {
EnvoyQuicConnection* quic_connection_{nullptr};

private:
friend class Envoy::TestPauseFilterForQuic;

// Called when aggregated buffered bytes across all the streams exceeds high watermark.
void onSendBufferHighWatermark();
// Called when aggregated buffered bytes across all the streams declines to low watermark.
Expand Down
22 changes: 21 additions & 1 deletion test/config/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,10 @@ name: "envoy.filters.listener.tls_inspector"
)EOF";
}

std::string ConfigHelper::httpProxyConfig() {
std::string ConfigHelper::httpProxyConfig(bool downstream_use_quic) {
if (downstream_use_quic) {
return quicHttpProxyConfig();
}
return absl::StrCat(baseConfig(), fmt::format(R"EOF(
filter_chains:
filters:
Expand Down Expand Up @@ -1050,6 +1053,23 @@ void ConfigHelper::addSslConfig(const ServerSslOptions& options) {
filter_chain->mutable_transport_socket()->mutable_typed_config()->PackFrom(tls_context);
}

void ConfigHelper::addQuicDownstreamTransportSocketConfig(bool reuse_port) {
envoy::extensions::transport_sockets::quic::v3::QuicDownstreamTransport
quic_transport_socket_config;
auto tls_context = quic_transport_socket_config.mutable_downstream_tls_context();
ConfigHelper::initializeTls(ConfigHelper::ServerSslOptions().setRsaCert(true).setTlsV13(true),
*tls_context->mutable_common_tls_context());
for (auto& listener : *bootstrap_.mutable_static_resources()->mutable_listeners()) {
if (listener.udp_listener_config().listener_config().typed_config().type_url() ==
"type.googleapis.com/envoy.config.listener.v3.QuicProtocolOptions") {
auto* filter_chain = listener.mutable_filter_chains(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we be checking that filter_chains is non-empty?

Copy link
Contributor Author

@danzh2010 danzh2010 Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is just a test util function whose input is the config template defined in the same file. But I added ASSERT to help future debugging.

auto* transport_socket = filter_chain->mutable_transport_socket();
transport_socket->mutable_typed_config()->PackFrom(quic_transport_socket_config);
listener.set_reuse_port(reuse_port);
}
}
}

bool ConfigHelper::setAccessLog(
const std::string& filename, absl::string_view format,
std::vector<envoy::config::core::v3::TypedExtensionConfig> formatters) {
Expand Down
7 changes: 5 additions & 2 deletions test/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class ConfigHelper {
// By default, this runs with an L7 proxy config, but config can be set to TCP_PROXY_CONFIG
// to test L4 proxying.
ConfigHelper(const Network::Address::IpVersion version, Api::Api& api,
const std::string& config = httpProxyConfig());
const std::string& config = httpProxyConfig(false));

static void
initializeTls(const ServerSslOptions& options,
Expand All @@ -111,7 +111,7 @@ class ConfigHelper {
// A basic configuration for L4 proxying.
static std::string tcpProxyConfig();
// A basic configuration for L7 proxying.
static std::string httpProxyConfig();
static std::string httpProxyConfig(bool downstream_use_quic = false);
// A basic configuration for L7 proxying with QUIC transport.
static std::string quicHttpProxyConfig();
// A string for a basic buffer filter, which can be used with addFilter()
Expand Down Expand Up @@ -217,6 +217,9 @@ class ConfigHelper {
void addSslConfig(const ServerSslOptions& options);
void addSslConfig() { addSslConfig({}); }

// Add the default SSL configuration for QUIC downstream.
void addQuicDownstreamTransportSocketConfig(bool resuse_port);

// Set the HTTP access log for the first HCM (if present) to a given file. The default is
// the platform's null device.
bool setAccessLog(const std::string& filename, absl::string_view format = "",
Expand Down
15 changes: 13 additions & 2 deletions test/extensions/quic_listeners/quiche/envoy_quic_utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,24 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) {
headers_block[":authority"] = "www.google.com";
headers_block[":path"] = "/index.hml";
headers_block[":scheme"] = "https";
headers_block.AppendValueOrAddHeader("key", "value1");
headers_block.AppendValueOrAddHeader("key", "value2");
auto envoy_headers = spdyHeaderBlockToEnvoyHeaders<Http::RequestHeaderMapImpl>(headers_block);
EXPECT_EQ(headers_block.size(), envoy_headers->size());
EXPECT_EQ(headers_block.size() + 1u, envoy_headers->size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment why the +1 please, since it's not obvious about the \0 split

EXPECT_EQ("www.google.com", envoy_headers->getHostValue());
EXPECT_EQ("/index.hml", envoy_headers->getPathValue());
EXPECT_EQ("https", envoy_headers->getSchemeValue());
EXPECT_EQ("value1", envoy_headers->get(Http::LowerCaseString("key"))[0]->value().getStringView());
EXPECT_EQ("value2", envoy_headers->get(Http::LowerCaseString("key"))[1]->value().getStringView());

quic::QuicHeaderList quic_headers = quic::test::AsHeaderList(headers_block);
quic::QuicHeaderList quic_headers;
quic_headers.OnHeaderBlockStart();
quic_headers.OnHeader(":authority", "www.google.com");
quic_headers.OnHeader(":path", "/index.hml");
quic_headers.OnHeader(":scheme", "https");
quic_headers.OnHeader("key", "value1");
quic_headers.OnHeader("key", "value2");
quic_headers.OnHeaderBlockEnd(0, 0);
auto envoy_headers2 = quicHeadersToEnvoyHeaders<Http::RequestHeaderMapImpl>(quic_headers);
EXPECT_EQ(*envoy_headers, *envoy_headers2);
}
Expand Down
6 changes: 4 additions & 2 deletions test/extensions/quic_listeners/quiche/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ envoy_package()

envoy_cc_test(
name = "quic_protocol_integration_test",
size = "medium",
size = "large",
srcs = [
"quic_protocol_integration_test.cc",
],
data = ["//test/config/integration/certs"],
shard_count = 6,
shard_count = 8,
tags = [
"fails_on_clang_cl",
"fails_on_windows",
Expand All @@ -27,6 +27,7 @@ envoy_cc_test(
"//source/extensions/quic_listeners/quiche:quic_factory_lib",
"//source/extensions/quic_listeners/quiche:quic_transport_socket_factory_lib",
"//test/integration:protocol_integration_test_lib",
"//test/integration/filters:pause_filter_for_quic_lib",
],
)

Expand All @@ -51,6 +52,7 @@ envoy_cc_test(
deps = [
"//source/extensions/filters/http/dynamo:config",
"//source/extensions/quic_listeners/quiche:active_quic_listener_config_lib",
"//source/extensions/quic_listeners/quiche:client_connection_factory_lib",
"//source/extensions/quic_listeners/quiche:codec_lib",
"//source/extensions/quic_listeners/quiche:envoy_quic_client_connection_lib",
"//source/extensions/quic_listeners/quiche:envoy_quic_client_session_lib",
Expand Down
Loading