Skip to content

http3: adding connect support #17877

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 16 commits into from
Sep 14, 2021
Merged

http3: adding connect support #17877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
64cb5a6
david's first pass
alyssawilk Aug 25, 2021
3924a76
http3: adding CONNECT support
alyssawilk Aug 25, 2021
3679898
Merge branch 'main' into connect
alyssawilk Aug 26, 2021
fa2c3a8
comments
alyssawilk Aug 31, 2021
e40d6b4
comments and tidy
alyssawilk Aug 31, 2021
65e8a4f
coverage
alyssawilk Aug 31, 2021
9297851
regex fail
alyssawilk Sep 1, 2021
5988a7e
tidy, coverage
alyssawilk Sep 1, 2021
ec4bfa4
test fix
alyssawilk Sep 2, 2021
8c06cdd
typo
alyssawilk Sep 8, 2021
49ad7fa
Merge branch 'main' into connect
alyssawilk Sep 8, 2021
3f4ee6c
comments
alyssawilk Sep 8, 2021
6b3bb58
missed one
alyssawilk Sep 8, 2021
ed3cbff
relnote
alyssawilk Sep 9, 2021
fe54c45
link
alyssawilk Sep 13, 2021
ba1234c
Merge branch 'main' into connect
alyssawilk Sep 13, 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
9 changes: 9 additions & 0 deletions api/envoy/config/core/v3/protocol.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ message GrpcProtocolOptions {
}

// A message which allows using HTTP/3.
// [#next-free-field: 6]
message Http3ProtocolOptions {
QuicProtocolOptions quic_protocol_options = 1;

Expand All @@ -483,6 +484,14 @@ message Http3ProtocolOptions {
// If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging
// <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>`.
google.protobuf.BoolValue override_stream_error_on_invalid_http_message = 2;

// Allows proxying Websocket and other upgrades over HTTP/3 CONNECT using
// the header mechanisms from the `HTTP/2 extended connect RFC
// <https://datatracker.ietf.org/doc/html/rfc8441>`_
// and settings `proposed for HTTP/3
// <https://datatracker.ietf.org/doc/draft-ietf-httpbis-h3-websockets/>`_
// [#alpha:] as HTTP/3 CONNECT is not yet an RFC.
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Sep 9, 2021

Choose a reason for hiding this comment

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

I'm going to eventually convert this into an annotation. I assume that for a primitive type, if it's set to something other than default, I can determine that the annotation is set in that case but not if it's left to the default?

Basically I'm wondering if we need to make this a WKT vs. a primitive or not for fields. cc @envoyproxy/api-shepherds who know more about proto.

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Sep 9, 2021

Choose a reason for hiding this comment

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

I'd think it'd be fine if for no other reason than setting it false is the same as not-present and doesn't need an alpha tag. it's really only the behavior when this is explicitly set true that may change depending on the RFC

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Sep 9, 2021

Choose a reason for hiding this comment

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

OK sounds good. I will see if this case works OK when I implement the feature.

Copy link
Copy Markdown
Member

@htuch htuch Sep 10, 2021

Choose a reason for hiding this comment

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

Yeah, I think we do need in general to make WIP annotated things WKT/optional/messages when the default value is material, but if their absence is a no-op then we can skip.

bool allow_extended_connect = 5;
Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Sep 10, 2021

Choose a reason for hiding this comment

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

When we had discussed this last, I was under the impression that we wanted an HTTP/3 option to enable CONNECT altogether. Is that no longer the case? Are we allowing non-extended CONNECT by default?

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Sep 10, 2021

Choose a reason for hiding this comment

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

Alyssa can correct me, but if I'm remembering our conversation correctly, I believe vanilla CONNECT is controlled by an HCM knob and we did not need a per-protocol vanilla CONNECT knob. (The HTTP/2 option, while named "allow_connect" actually enables extended connect so this HTTP/3 is parallel to the HTTP/3 option but with a more accurate name)

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Sep 13, 2021

Choose a reason for hiding this comment

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

Yep, we've gone over that a couple of times now :-) Old-style CONNECT gets rejected in the HCM if there's not connect matchers, and so we only need to control extended connect in the protoco-specific options. I tried to also make sure the documentation pretty clear on this point (https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/upgrades#connect-support) but please let me know if you have ideas of how I could improve it!

Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Sep 13, 2021

Choose a reason for hiding this comment

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

Thanks for clarifying!

}

// A message to control transformations to the :scheme header
Expand Down
9 changes: 6 additions & 3 deletions docs/root/intro/arch_overview/http/upgrades.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,13 @@ a deployment of the form:
In this case, if a client is for example using WebSocket, we want the Websocket to arrive at the
upstream server functionally intact, which means it needs to traverse the HTTP/2+ hop.

This is accomplished via `Extended CONNECT (RFC8441) <https://tools.ietf.org/html/rfc8441>`_ support,
This is accomplished for HTTP/2 via `Extended CONNECT (RFC8441) <https://tools.ietf.org/html/rfc8441>`_ support,
turned on by setting :ref:`allow_connect <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.allow_connect>`
true at the second layer Envoy. The
WebSocket request will be transformed into an HTTP/2+ CONNECT stream, with :protocol header
true at the second layer Envoy. For HTTP/3 there is parallel support configured by the alpha option
:ref:`allow_extended_connect <envoy_v3_api_field_config.core.v3.Http3ProtocolOptions.allow_extended_connect>` as
there is no formal RFC yet.

The WebSocket request will be transformed into an HTTP/2+ CONNECT stream, with :protocol header
indicating the original upgrade, traverse the HTTP/2+ hop, and be downgraded back into an HTTP/1
WebSocket Upgrade. This same Upgrade-CONNECT-Upgrade transformation will be performed on any
HTTP/2+ hop, with the documented flaw that the HTTP/1.1 method is always assumed to be GET.
Expand Down
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ New Features
* http: added :ref:`x-envoy-upstream-stream-duration-ms <config_http_filters_router_x-envoy-upstream-stream-duration-ms>` that allows configuring the max stream duration via a request header.
* http: added support for :ref:`max_requests_per_connection <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.max_requests_per_connection>` for both upstream and downstream connections.
* http: sanitizing the referer header as documented :ref:`here <config_http_conn_man_headers_referer>`. This feature can be temporarily turned off by setting runtime guard ``envoy.reloadable_features.sanitize_http_header_referer`` to false.
* http: validating outgoing HTTP/2 CONNECT requests to ensure that if ``:path`` is set that ``:protocol`` is present. This behavior can be temporarily turned off by setting runtime guard ``envoy.reloadable_features.validate_connect`` to false.
* jwt_authn: added support for :ref:`Jwt Cache <envoy_v3_api_field_extensions.filters.http.jwt_authn.v3.JwtProvider.jwt_cache_config>` and its size can be specified by :ref:`jwt_cache_size <envoy_v3_api_field_extensions.filters.http.jwt_authn.v3.JwtCacheConfig.jwt_cache_size>`.
* jwt_authn: added support for extracting JWTs from request cookies using :ref:`from_cookies <envoy_v3_api_field_extensions.filters.http.jwt_authn.v3.JwtProvider.from_cookies>`.
* listener: new listener metric ``downstream_cx_transport_socket_connect_timeout`` to track transport socket timeouts.
Expand Down
12 changes: 12 additions & 0 deletions source/common/http/header_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,18 @@ Http::Status HeaderUtility::checkRequiredRequestHeaders(const Http::RequestHeade
return absl::InvalidArgumentError(
absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get()));
}
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.validate_connect")) {
if (headers.Path() && !headers.Protocol()) {
Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Sep 10, 2021

Choose a reason for hiding this comment

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

As per the spec, when :protocol is included, both :path and :scheme are required. Should we validate both?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Sep 13, 2021

Choose a reason for hiding this comment

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

:scheme is required for all non-CONNECT requests and is supposed to be done prior to this point as we expect core protocol invariant to be enforced by the codec (this is pre-RFC and extension so less so for this). If QUICHE isn't doing that enforcement, I'd argue it probably should so thanks for volunteering to pick it up below.

// Path and Protocol header should only be present for CONNECT for upgrade style CONNECT.
return absl::InvalidArgumentError(
absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Protocol.get()));
}
if (!headers.Path() && headers.Protocol()) {
// Path and Protocol header should only be present for CONNECT for upgrade style CONNECT.
return absl::InvalidArgumentError(
absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Path.get()));
}
}
} else {
if (!headers.Path()) {
// :path header must be present for non-CONNECT requests.
Expand Down
8 changes: 6 additions & 2 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 @@ -46,6 +46,8 @@ EnvoyQuicServerStream::EnvoyQuicServerStream(
headers_with_underscores_action_(headers_with_underscores_action) {
ASSERT(static_cast<uint32_t>(GetReceiveWindow().value()) > 8 * 1024,
"Send buffer limit should be larger than 8KB.");
// TODO(alyssawilk, danzh) if http3_options_.allow_extended_connect() is true,
Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Sep 10, 2021

Choose a reason for hiding this comment

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

FWIW I'm planning on adding support for this SETTING in QUICHE for Privacy Proxy, I plan on always sending it and validating the presence of pseudo-headers in QUICHE

Copy link
Copy Markdown
Contributor

@RyanTheOptimist RyanTheOptimist Sep 10, 2021

Choose a reason for hiding this comment

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

Thanks for taking this on. We should perhaps discuss this on the QUICHE PR/CL, but I'm not convinced that always sending the setting is the right choice. But let's discuss there.

Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Sep 13, 2021

Choose a reason for hiding this comment

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

@DavidSchinazi Actually I'm already working on a QUICHE change to add this setting per previous discussion. I can take this feature if you don't mind?

Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Sep 13, 2021

Choose a reason for hiding this comment

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

@danzh2010 that's fine by me! From Alyssa's other comment I think QUICHE should validate that the correct headers are present. Please add me as a reviewer on the change

// send the correct SETTINGS.
}

void EnvoyQuicServerStream::encode100ContinueHeaders(const Http::ResponseHeaderMap& headers) {
Expand Down Expand Up @@ -167,7 +169,9 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
onStreamError(close_connection_upon_invalid_header_, rst);
return;
}
if (Http::HeaderUtility::requestHeadersValid(*headers) != absl::nullopt) {
if (Http::HeaderUtility::requestHeadersValid(*headers) != absl::nullopt ||
Http::HeaderUtility::checkRequiredRequestHeaders(*headers) != Http::okStatus() ||
(headers->Protocol() && !http3_options_.allow_extended_connect())) {
details_ = Http3ResponseCodeDetailValues::invalid_http_header;
onStreamError(absl::nullopt);
return;
Expand Down Expand Up @@ -392,7 +396,7 @@ void EnvoyQuicServerStream::onStreamError(absl::optional<bool> should_close_conn
!http3_options_.override_stream_error_on_invalid_http_message().value();
}
if (close_connection_upon_invalid_header) {
stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers");
stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, std::string(details_));
} else {
Reset(rst);
}
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ constexpr const char* runtime_features[] = {
"envoy.reloadable_features.unquote_log_string_values",
"envoy.reloadable_features.upstream_host_weight_change_causes_rebuild",
"envoy.reloadable_features.use_observable_cluster_name",
"envoy.reloadable_features.validate_connect",
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Sep 9, 2021

Choose a reason for hiding this comment

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

Should this be documented somewhere?

"envoy.reloadable_features.vhds_heartbeats",
"envoy.reloadable_features.wasm_cluster_name_envoy_grpc",
"envoy.reloadable_features.upstream_http2_flood_checks",
Expand Down
10 changes: 8 additions & 2 deletions source/common/tcp_proxy/upstream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,14 @@ HttpConnPool::HttpConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks,
Http::CodecType type)
: config_(config), type_(type), upstream_callbacks_(upstream_callbacks) {
conn_pool_data_ = thread_local_cluster.httpConnPool(Upstream::ResourcePriority::Default,
absl::nullopt, context);
absl::optional<Http::Protocol> protocol;
if (type_ == Http::CodecType::HTTP3) {
protocol = Http::Protocol::Http3;
} else if (type_ == Http::CodecType::HTTP2) {
protocol = Http::Protocol::Http2;
}
conn_pool_data_ =
thread_local_cluster.httpConnPool(Upstream::ResourcePriority::Default, protocol, context);
}

HttpConnPool::~HttpConnPool() {
Expand Down
3 changes: 1 addition & 2 deletions source/common/tcp_proxy/upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callba
Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks, Http::CodecType type);
~HttpConnPool() override;

// HTTP/3 upstreams are not supported at the moment.
bool valid() const { return conn_pool_data_.has_value() && type_ <= Http::CodecType::HTTP2; }
bool valid() const { return conn_pool_data_.has_value(); }

// GenericConnPool
void newStream(GenericConnectionPoolCallbacks& callbacks) override;
Expand Down
13 changes: 9 additions & 4 deletions source/extensions/upstreams/tcp/generic/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,15 @@ TcpProxy::GenericConnPoolPtr GenericConnPoolFactory::createGenericConnPool(
const absl::optional<TunnelingConfig>& config, Upstream::LoadBalancerContext* context,
Envoy::Tcp::ConnectionPool::UpstreamCallbacks& upstream_callbacks) const {
if (config.has_value()) {
auto pool_type =
((thread_local_cluster.info()->features() & Upstream::ClusterInfo::Features::HTTP2) != 0)
? Http::CodecType::HTTP2
: Http::CodecType::HTTP1;
Http::CodecType pool_type;
if ((thread_local_cluster.info()->features() & Upstream::ClusterInfo::Features::HTTP2) != 0) {
pool_type = Http::CodecType::HTTP2;
} else if ((thread_local_cluster.info()->features() & Upstream::ClusterInfo::Features::HTTP3) !=
0) {
pool_type = Http::CodecType::HTTP3;
} else {
pool_type = Http::CodecType::HTTP1;
}
auto ret = std::make_unique<TcpProxy::HttpConnPool>(
thread_local_cluster, context, config.value(), upstream_callbacks, pool_type);
return (ret->valid() ? std::move(ret) : nullptr);
Expand Down
28 changes: 28 additions & 0 deletions test/common/http/header_utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,34 @@ TEST(ValidateHeaders, HeaderNameWithUnderscores) {
rejected));
}

TEST(ValidateHeaders, Connect) {
{
// Basic connect.
TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":authority", "foo.com:80"}};
EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(headers));
}
{
// Extended connect.
TestRequestHeaderMapImpl headers{{":method", "CONNECT"},
{":authority", "foo.com:80"},
{":path", "/"},
{":protocol", "websocket"}};
EXPECT_EQ(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(headers));
}
{
// Missing path.
TestRequestHeaderMapImpl headers{
{":method", "CONNECT"}, {":authority", "foo.com:80"}, {":protocol", "websocket"}};
EXPECT_NE(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(headers));
}
{
// Missing protocol.
TestRequestHeaderMapImpl headers{
{":method", "CONNECT"}, {":authority", "foo.com:80"}, {":path", "/"}};
EXPECT_NE(Http::okStatus(), HeaderUtility::checkRequiredRequestHeaders(headers));
}
}

TEST(ValidateHeaders, ContentLength) {
bool should_close_connection;
EXPECT_EQ(HeaderUtility::HeaderValidationResult::ACCEPT,
Expand Down
6 changes: 3 additions & 3 deletions test/common/http/http1/codec_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2757,7 +2757,7 @@ TEST_F(Http1ClientConnectionImplTest, ConnectResponse) {

NiceMock<MockResponseDecoder> response_decoder;
Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}};
TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":authority", "host"}};
EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok());

// Send response headers
Expand Down Expand Up @@ -2788,7 +2788,7 @@ TEST_F(Http1ClientConnectionImplTest, ConnectResponseWithEarlyData) {

NiceMock<MockResponseDecoder> response_decoder;
Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}};
TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":authority", "host"}};
EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok());

// Send response headers and payload
Expand All @@ -2807,7 +2807,7 @@ TEST_F(Http1ClientConnectionImplTest, ConnectRejected) {

NiceMock<MockResponseDecoder> response_decoder;
Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder);
TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}};
TestRequestHeaderMapImpl headers{{":method", "CONNECT"}, {":authority", "host"}};
EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok());

EXPECT_CALL(response_decoder, decodeHeaders_(_, false));
Expand Down
1 change: 1 addition & 0 deletions test/common/http/http2/codec_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3037,6 +3037,7 @@ TEST_P(Http2CodecImplTest, ConnectTest) {
TestRequestHeaderMapImpl request_headers;
HttpTestUtility::addDefaultHeaders(request_headers);
request_headers.setReferenceKey(Headers::get().Method, Http::Headers::get().MethodValues.Connect);
request_headers.setReferenceKey(Headers::get().Protocol, "bytestream");
TestRequestHeaderMapImpl expected_headers;
HttpTestUtility::addDefaultHeaders(expected_headers);
expected_headers.setReferenceKey(Headers::get().Method,
Expand Down
3 changes: 3 additions & 0 deletions test/common/router/router_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5968,6 +5968,7 @@ TEST_F(RouterTest, ConnectPauseAndResume) {
Http::TestRequestHeaderMapImpl headers;
HttpTestUtility::addDefaultHeaders(headers);
headers.setMethod("CONNECT");
headers.removePath();
router_.decodeHeaders(headers, false);

// Make sure any early data does not go upstream.
Expand Down Expand Up @@ -6040,6 +6041,7 @@ TEST_F(RouterTest, ConnectPauseNoResume) {
Http::TestRequestHeaderMapImpl headers;
HttpTestUtility::addDefaultHeaders(headers);
headers.setMethod("CONNECT");
headers.removePath();
router_.decodeHeaders(headers, false);

// Make sure any early data does not go upstream.
Expand Down Expand Up @@ -6070,6 +6072,7 @@ TEST_F(RouterTest, ConnectExplicitTcpUpstream) {
Http::TestRequestHeaderMapImpl headers;
HttpTestUtility::addDefaultHeaders(headers);
headers.setMethod("CONNECT");
headers.removePath();
router_.decodeHeaders(headers, false);

router_.onDestroy();
Expand Down
5 changes: 4 additions & 1 deletion test/config/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ void ConfigHelper::addClusterFilterMetadata(absl::string_view metadata_yaml,

void ConfigHelper::setConnectConfig(
envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm,
bool terminate_connect, bool allow_post) {
bool terminate_connect, bool allow_post, bool http3) {
auto* route_config = hcm.mutable_route_config();
ASSERT_EQ(1, route_config->virtual_hosts_size());
auto* route = route_config->mutable_virtual_hosts(0)->mutable_routes(0);
Expand Down Expand Up @@ -671,6 +671,9 @@ void ConfigHelper::setConnectConfig(

hcm.add_upgrade_configs()->set_upgrade_type("CONNECT");
hcm.mutable_http2_protocol_options()->set_allow_connect(true);
if (http3) {
hcm.mutable_http3_protocol_options()->set_allow_extended_connect(true);
}
}

void ConfigHelper::applyConfigModifiers() {
Expand Down
3 changes: 2 additions & 1 deletion test/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,8 @@ class ConfigHelper {

// Given an HCM with the default config, set the matcher to be a connect matcher and enable
// CONNECT requests.
static void setConnectConfig(HttpConnectionManager& hcm, bool terminate_connect, bool allow_post);
static void setConnectConfig(HttpConnectionManager& hcm, bool terminate_connect, bool allow_post,
bool http3 = false);

void setLocalReply(
const envoy::extensions::filters::network::http_connection_manager::v3::LocalReplyConfig&
Expand Down
25 changes: 25 additions & 0 deletions test/extensions/upstreams/tcp/generic/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "gtest/gtest.h"

using testing::_;
using testing::AnyNumber;
using testing::NiceMock;
using testing::Return;

Expand All @@ -31,6 +32,30 @@ TEST_F(TcpConnPoolTest, TestNoConnPool) {
factory_.createGenericConnPool(thread_local_cluster_, config, nullptr, callbacks_));
}

TEST_F(TcpConnPoolTest, Http2Config) {
auto info = std::make_shared<Upstream::MockClusterInfo>();
EXPECT_CALL(*info, features()).WillOnce(Return(Upstream::ClusterInfo::Features::HTTP2));
EXPECT_CALL(thread_local_cluster_, info).WillOnce(Return(info));
envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig config;
config.set_hostname("host");
EXPECT_CALL(thread_local_cluster_, httpConnPool(_, _, _)).WillOnce(Return(absl::nullopt));
EXPECT_EQ(nullptr,
factory_.createGenericConnPool(thread_local_cluster_, config, nullptr, callbacks_));
}

TEST_F(TcpConnPoolTest, Http3Config) {
auto info = std::make_shared<Upstream::MockClusterInfo>();
EXPECT_CALL(*info, features())
.Times(AnyNumber())
.WillRepeatedly(Return(Upstream::ClusterInfo::Features::HTTP3));
EXPECT_CALL(thread_local_cluster_, info).Times(AnyNumber()).WillRepeatedly(Return(info));
envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy_TunnelingConfig config;
config.set_hostname("host");
EXPECT_CALL(thread_local_cluster_, httpConnPool(_, _, _)).WillOnce(Return(absl::nullopt));
EXPECT_EQ(nullptr,
factory_.createGenericConnPool(thread_local_cluster_, config, nullptr, callbacks_));
}

} // namespace Generic
} // namespace Tcp
} // namespace Upstreams
Expand Down
2 changes: 2 additions & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,12 +1330,14 @@ envoy_cc_test(

envoy_cc_test(
name = "tcp_tunneling_integration_test",
size = "large",
srcs = [
"tcp_tunneling_integration_test.cc",
],
data = [
"//test/config/integration/certs",
],
shard_count = 3,
deps = [
":http_integration_lib",
":http_protocol_integration_lib",
Expand Down
1 change: 1 addition & 0 deletions test/integration/fake_upstream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ struct FakeUpstreamConfig {
// Legacy options which are always set.
http2_options_.set_allow_connect(true);
http2_options_.set_allow_metadata(true);
http3_options_.set_allow_extended_connect(true);
}

Event::TestTimeSystem& time_system_;
Expand Down
1 change: 1 addition & 0 deletions test/integration/http_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ IntegrationCodecClientPtr HttpIntegrationTest::makeRawHttpConnection(
} else {
cluster->http3_options_ = ConfigHelper::http2ToHttp3ProtocolOptions(
http2_options.value(), quic::kStreamReceiveWindowLimit);
cluster->http3_options_.set_allow_extended_connect(true);
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Sep 8, 2021

Choose a reason for hiding this comment

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

Does this config on client stream has any effect?

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Sep 8, 2021

Choose a reason for hiding this comment

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

I assume it'll result in sending the SETTINGS when we have it.

Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Sep 8, 2021

Choose a reason for hiding this comment

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

How about this one?

Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 Sep 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought the ENABLE_CONNECT_PROTOCOL setting was supposed to be sent by server. So the the knob should only be used by server code. No?

Copy link
Copy Markdown
Contributor

@DavidSchinazi DavidSchinazi Sep 10, 2021

Choose a reason for hiding this comment

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

I'd imagine that on the client we'd use this option to fail the extended connect before it's sent? Or is it a no-op on the client?

#endif
}
cluster->http2_options_ = http2_options.value();
Expand Down
17 changes: 14 additions & 3 deletions test/integration/http_integration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,24 @@ using IntegrationCodecClientPtr = std::unique_ptr<IntegrationCodecClient>;
*/
class HttpIntegrationTest : public BaseIntegrationTest {
public:
HttpIntegrationTest(Http::CodecType downstream_protocol, Network::Address::IpVersion version)
: HttpIntegrationTest(
downstream_protocol, version,
ConfigHelper::httpProxyConfig(/*downstream_use_quic=*/downstream_protocol ==
Http::CodecType::HTTP3)) {}
HttpIntegrationTest(Http::CodecType downstream_protocol, Network::Address::IpVersion version,
const std::string& config = ConfigHelper::httpProxyConfig());
const std::string& config);

HttpIntegrationTest(Http::CodecType downstream_protocol,
const InstanceConstSharedPtrFn& upstream_address_fn,
Network::Address::IpVersion version,
const std::string& config = ConfigHelper::httpProxyConfig());
Network::Address::IpVersion version)
: HttpIntegrationTest(
downstream_protocol, upstream_address_fn, version,
ConfigHelper::httpProxyConfig(/*downstream_use_quic=*/downstream_protocol ==
Http::CodecType::HTTP3)) {}
HttpIntegrationTest(Http::CodecType downstream_protocol,
const InstanceConstSharedPtrFn& upstream_address_fn,
Network::Address::IpVersion version, const std::string& config);
~HttpIntegrationTest() override;

void initialize() override;
Expand Down
Loading