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 2 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
4 changes: 4 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,9 @@ 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.
bool allow_upgrade_connect = 5;

@RyanTheOptimist RyanTheOptimist Aug 26, 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.

To confirm my interpretation of this config this only allows RFC 8441 style CONNECT which requires both :path and :protocol. It does not enable RFC 7540 style CONNECT (without :path or :protocol). RFC 8441 requires SETTINGS_ENABLE_CONNECT_PROTOCOL be received by the client before sending the extended CONNECT. Presumably Envoy sends this setting? I wonder if we should call out the RFCs here (and possibly in the corresponding HTTP/2 config). What do you think?

All of this being said, RFC 8841 is an extension to HTTP/2 not an extension to HTTP/3. HTTP/2 settings are not automatically valid for HTTP/3.

Settings need to be defined separately for HTTP/2 and HTTP/3. The IDs of settings defined in [HTTP2] have been reserved for simplicity. Note that the settings identifier space in HTTP/3 is substantially larger (62 bits versus 16 bits), so many HTTP/3 settings have no equivalent HTTP/2 code point. See Section 11.2.2.

I think we need to write a doc describing how to do port RFC 8841 to HTTP/2. (Similar docs need to be written for the ORIGIN and ALT_SVC frames, fwiw. https://www.ietf.org/archive/id/draft-bishop-httpbis-altsvc-quic-01.html ) In particular we need to define a value for SETTINGS_ENABLE_CONNECT_PROTOCOL in HTTP/3. I've started that process here:

https://github.com/RyanTheOptimist/httpbis-h3-websockets/blob/main/draft-hamilton-httpbis-h3-websockets.md

and will work with httpbis folks to push this forward.

@alyssawilk alyssawilk Aug 31, 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.

yeah we could just land standard CONNECT support but I think folks are going to want websocket over h3 if they have it for H2.
I'll link to the H2 spec and your draft and tag it alpha. Alternately I can just add support for standard connect (block anything with path, regardless of protocol) and wait on the RFC. thoughts?

@RyanTheOptimist RyanTheOptimist Aug 31, 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.

I think either approach is fine, but I share your intuition that folks will want H3 websockets if they have H2 websockets. I think linking to the draft and tagging it as alpha is fine. BTW, here's the IETF url for the draft https://datatracker.ietf.org/doc/html/draft-hamilton-httpbis-h3-websockets if you want to link to it.

}

// A message to control transformations to the :scheme header
Expand Down
4 changes: 4 additions & 0 deletions api/envoy/config/core/v4alpha/protocol.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,7 @@ message GrpcProtocolOptions {
}

// A message which allows using HTTP/3.
// [#next-free-field: 6]
message Http3ProtocolOptions {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.core.v3.Http3ProtocolOptions";
Expand All @@ -483,6 +484,9 @@ 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.
bool allow_upgrade_connect = 5;
}

// A message to control transformations to the :scheme header
Expand Down
4 changes: 4 additions & 0 deletions generated_api_shadow/envoy/config/core/v3/protocol.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions generated_api_shadow/envoy/config/core/v4alpha/protocol.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 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,12 @@ 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") &&
headers.Path() && !headers.Protocol()) {

@RyanTheOptimist RyanTheOptimist Aug 26, 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.

It looks like scheme is also required, interestingly. Should we check for that too?

o On requests that contain the :protocol pseudo-header field, the
:scheme and :path pseudo-header fields of the target URI (see
Section 5) MUST also be included.

@RyanTheOptimist RyanTheOptimist Aug 26, 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.

From this code it looks like we allow CONNECT here without protocol (and hence without path). Is that right? If so, my earlier assumption that we only supported 8841 is definitely wrong :)

@alyssawilk alyssawilk Aug 31, 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.

scheme is default required for all HTTP/3 so I believe is validated elsewhere.
Path is required in the HCM unless it's CONNECT, but all envoy route matching asserts a path except for the special connect matchers, so Envoy will 404 unless you explicitly try to support CONNECT.

@RyanTheOptimist RyanTheOptimist Aug 31, 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.

The :scheme header is prohibited from vanilla CONNECT requests in HTTP/3:

A CONNECT request MUST be constructed as follows:
* The ":scheme" and ":path" pseudo-header fields are omitted

Is something validating that :scheme is present for CONNECT requests elsewhere? If so, is that perhaps incorrect?

@danzh2010 danzh2010 Sep 8, 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.

Is !headers.Path() && headers.Protocol() also invalid?

@RyanTheOptimist RyanTheOptimist Sep 8, 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.

Protocol and path MUST both be sent for extended CONNECT. So if one is sent but not the other, yes, I think that's invalid,

@alyssawilk alyssawilk Sep 8, 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.

fixed and tested.

// Path header should only be present for CONNECT for upgrade style CONECT.

@danzh2010 danzh2010 Aug 26, 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.

typo, should be CONNECT

@alyssawilk alyssawilk Aug 31, 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.

done

return absl::InvalidArgumentError(
absl::StrCat("missing required header: ", Envoy::Http::Headers::get().Host.get()));

@RyanTheOptimist RyanTheOptimist Aug 26, 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.

Should this be Protocol.get() instead of Host.get()?

@alyssawilk alyssawilk Aug 31, 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.

yep, nice catch.

}
} else {
if (!headers.Path()) {
// :path header must be present for non-CONNECT requests.
Expand Down
4 changes: 3 additions & 1 deletion 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 @@ -155,7 +155,9 @@ void EnvoyQuicServerStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
onStreamError(close_connection_upon_invalid_header_);
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_upgrade_connect())) {

@RyanTheOptimist RyanTheOptimist Aug 26, 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.

I could be reading this wrong, but it looks like if the request method is CONNECT but it doesn't have the :protocol header, we will consider the request valid. Is that right?

@alyssawilk alyssawilk Aug 31, 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.

yeah CONNECT either has to have no path, or path + protocol.
The former is a standard CONNECT request, and will be blocked at the HCM unless there's CONNECT-specific route matchers (so off by default). The latter gets transformed into an HTTP/1.1 style upgrade, and won't look like it's a CONNECT for the duration of the filter chain, will match normal routes so is disabled by default using a separate knob.

https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/upgrades
which now that I think about it I should update.

details_ = Http3ResponseCodeDetailValues::invalid_http_header;
onStreamError(absl::nullopt);
return;
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 @@ -94,6 +94,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",

@mattklein123 mattklein123 Sep 9, 2021

Copy link
Copy Markdown
Member

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(); }
Comment thread
alyssawilk marked this conversation as resolved.

// 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
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 @@ -3032,6 +3032,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_upgrade_connect(true);

@RyanTheOptimist RyanTheOptimist Aug 26, 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.

It looks like the h2 and h3 option are named differently:

set_allow_connect()
set_allow_upgrade_connect()

Would it make sense to use the same name in both places?

@alyssawilk alyssawilk Aug 31, 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.

I think the H2 option is (sadly) misnamed because HTTP/2 allows standard connect without a special knob, and just disallows the upgrade style connect.

}
}

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 @@ -309,7 +309,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
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 @@ -1329,12 +1329,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/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_upgrade_connect(true);
#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
25 changes: 7 additions & 18 deletions test/integration/protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,8 +583,6 @@ TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) {
}

TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) {
// TODO(danzh) re-enable after adding http3 option "allow_connect".
EXCLUDE_DOWNSTREAM_HTTP3;
if (upstreamProtocol() == Http::CodecType::HTTP3) {
// For QUIC, even through the headers are not sent upstream, the stream will
// be created. Use the autonomous upstream and allow incomplete streams.
Expand All @@ -594,15 +592,10 @@ TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) {
// Faulty filter that removed host in a CONNECT request.
config_helper_.addConfigModifier(
[&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
hcm) -> void { ConfigHelper::setConnectConfig(hcm, false, false); });
config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
// Clone the whole listener.
auto static_resources = bootstrap.mutable_static_resources();
auto* old_listener = static_resources->mutable_listeners(0);
auto* cloned_listener = static_resources->add_listeners();
cloned_listener->CopyFrom(*old_listener);
old_listener->set_name("http_forward");
});
hcm) -> void {
ConfigHelper::setConnectConfig(hcm, false, false,
downstreamProtocol() == Http::CodecType::HTTP3);
});
useAccessLog("%RESPONSE_CODE_DETAILS%");
config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": "
"type.googleapis.com/google.protobuf.Empty } }");
Expand All @@ -613,9 +606,7 @@ TEST_P(DownstreamProtocolIntegrationTest, FaultyFilterWithConnect) {
auto headers = Http::TestRequestHeaderMapImpl{
{":method", "CONNECT"}, {":scheme", "http"}, {":authority", "www.host.com:80"}};

auto response = (downstream_protocol_ == Http::CodecType::HTTP1)
? std::move((codec_client_->startRequest(headers)).second)
: codec_client_->makeHeaderOnlyRequest(headers);
auto response = std::move((codec_client_->startRequest(headers)).second);

ASSERT_TRUE(response->waitForEndStream());
EXPECT_TRUE(response->complete());
Expand Down Expand Up @@ -2533,8 +2524,6 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) {
// Make sure that with override_stream_error_on_invalid_http_message true, CONNECT
// results in stream teardown not connection teardown.
TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) {
// TODO(danzh) add "allow_connect" to http3 options.
EXCLUDE_DOWNSTREAM_HTTP3;
if (downstreamProtocol() == Http::CodecType::HTTP1) {
return;
}
Expand All @@ -2551,8 +2540,8 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) {

initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
auto response = codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{
{":method", "CONNECT"}, {":path", "/"}, {":authority", "host"}});
auto response = codec_client_->makeHeaderOnlyRequest(
Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, {":authority", "host"}});

ASSERT_TRUE(response->waitForReset());
EXPECT_FALSE(codec_client_->disconnected());
Expand Down
Loading