From cd171d9a95f551628453c83f0b00924e8b310bf0 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Tue, 28 Aug 2018 09:06:48 -0400 Subject: [PATCH] websocket: tunneling websockets (and upgrades in general) over H2 (#4188) This allows tunneling over H2, unfortunately only enabled via nghttp2_option_set_no_http_messaging until nghttp2/nghttp2#1181 is sorted out. See the big warnings about not using (at least without knowing you're going to have a roll-out that may break backwards-compatibility some time in the not too distant future) Risk Level: Medium (changes are contained behind H2-with-Upgrade header which doesn't work today) Testing: unit tests, and turned up the full H1/H2 upstream/downstream in the integration test Docs Changes: for now, though I may take them out. I think they're useful for review. Release Notes: not added since we don't want folks using it (outside of testbeds) yet. #1630 Signed-off-by: Alyssa Wilk --- api/envoy/api/v2/core/protocol.proto | 14 ++ docs/root/intro/arch_overview/websocket.rst | 23 ++++ include/envoy/http/codec.h | 3 + include/envoy/http/header_map.h | 1 + source/common/http/conn_manager_impl.cc | 2 +- source/common/http/conn_manager_utility.cc | 4 +- source/common/http/conn_manager_utility.h | 8 +- source/common/http/headers.h | 2 + source/common/http/http2/codec_impl.cc | 26 +++- source/common/http/http2/codec_impl.h | 26 ++++ source/common/http/utility.cc | 51 ++++++++ source/common/http/utility.h | 33 +++++ test/common/http/conn_manager_utility_test.cc | 16 +-- test/common/http/utility_test.cc | 87 +++++++++++++ test/integration/BUILD | 2 +- test/integration/fake_upstream.cc | 4 +- test/integration/http_integration.cc | 1 + .../integration/websocket_integration_test.cc | 122 +++++++++++++----- test/integration/websocket_integration_test.h | 45 +++++-- test/test_common/BUILD | 9 ++ test/test_common/utility.cc | 28 ++++ test/test_common/utility.h | 14 ++ test/test_common/utility_test.cc | 29 +++++ 23 files changed, 482 insertions(+), 68 deletions(-) create mode 100644 test/test_common/utility_test.cc diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 2e7948787df..44d684841e0 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -76,6 +76,20 @@ message Http2ProtocolOptions { // window. Currently, this has the same minimum/maximum/default as *initial_stream_window_size*. google.protobuf.UInt32Value initial_connection_window_size = 4 [(validate.rules).uint32 = {gte: 65535, lte: 2147483647}]; + + // [#not-implemented-hide:] Hiding until nghttp2 has native support. + // + // Allows proxying Websocket and other upgrades over H2 connect. + // + // THIS IS NOT SAFE TO USE IN PRODUCTION + // + // This currently works via disabling all HTTP sanity checks for H2 traffic + // which is a much larger hammer than we'd like to use. Eventually when + // https://github.com/nghttp2/nghttp2/issues/1181 is resolved, this will work + // with simply enabling CONNECT for H2. This may require some tweaks to the + // headers making pre-CONNECT-support proxying not backwards compatible with + // post-CONNECT-support proxying. + bool allow_connect = 5; } // [#not-implemented-hide:] diff --git a/docs/root/intro/arch_overview/websocket.rst b/docs/root/intro/arch_overview/websocket.rst index 35aa8477fc5..a1b66aadd8a 100644 --- a/docs/root/intro/arch_overview/websocket.rst +++ b/docs/root/intro/arch_overview/websocket.rst @@ -20,6 +20,29 @@ one can set up custom for the given upgrade type, up to and including only using the router filter to send the WebSocket data upstream. +Handling H2 hops (implementation in progress) +--------------------------------------------- + +Envoy currently has an alpha implementation of tunneling websockets over H2 streams for deployments +that prefer a uniform H2 mesh throughout, for example, for a deployment of the form: + +[Client] ---- HTTP/1.1 ---- [Front Envoy] ---- HTTP/2 ---- [Sidecar Envoy ---- H1 ---- App] + +In this case, if a client is for example using WebSocket, we want the Websocket to arive at the +upstream server functionally intact, which means it needs to traverse the HTTP/2 hop. + +TODO(alyssawilk) copy the warnings from the config here, or just land the docs when we unhide. + +This is accomplished via +`extended CONNECT `_ support. 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. +Non-WebSocket upgrades are allowed to use any valid HTTP method (i.e. POST) and the current +upgrade/downgrade mechanism will drop the original method and transform the Upgrade request to +a GET method on the final Envoy-Upstream hop. + Old style WebSocket support =========================== diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 943b9d74bd3..65a9074c66d 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -208,6 +208,7 @@ struct Http2Settings { uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS}; uint32_t initial_stream_window_size_{DEFAULT_INITIAL_STREAM_WINDOW_SIZE}; uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE}; + bool allow_connect_{DEFAULT_ALLOW_CONNECT}; // disable HPACK compression static const uint32_t MIN_HPACK_TABLE_SIZE = 0; @@ -241,6 +242,8 @@ struct Http2Settings { // our default connection-level window also equals to our stream-level static const uint32_t DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE = 256 * 1024 * 1024; static const uint32_t MAX_INITIAL_CONNECTION_WINDOW_SIZE = (1U << 31) - 1; + // By default both nghttp2 and Envoy do not allow CONNECT over H2. + static const bool DEFAULT_ALLOW_CONNECT = false; }; /** diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index fa02952a2b2..d7af629b84c 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -281,6 +281,7 @@ class HeaderEntry { HEADER_FUNC(Origin) \ HEADER_FUNC(OtSpanContext) \ HEADER_FUNC(Path) \ + HEADER_FUNC(Protocol) \ HEADER_FUNC(ProxyConnection) \ HEADER_FUNC(Referer) \ HEADER_FUNC(RequestId) \ diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index c0fb71b5bf6..df80eecb435 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -580,7 +580,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, // Modify the downstream remote address depending on configuration and headers. request_info_.setDownstreamRemoteAddress(ConnectionManagerUtility::mutateRequestHeaders( - *request_headers_, protocol, connection_manager_.read_callbacks_->connection(), + *request_headers_, connection_manager_.read_callbacks_->connection(), connection_manager_.config_, *snapped_route_config_, connection_manager_.random_generator_, connection_manager_.runtime_, connection_manager_.local_info_)); ASSERT(request_info_.downstreamRemoteAddress() != nullptr); diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index ebe58b59d1d..7fc2e023e8e 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -19,13 +19,13 @@ namespace Envoy { namespace Http { Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequestHeaders( - Http::HeaderMap& request_headers, Protocol protocol, Network::Connection& connection, + Http::HeaderMap& request_headers, Network::Connection& connection, ConnectionManagerConfig& config, const Router::Config& route_config, Runtime::RandomGenerator& random, Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info) { // If this is a Upgrade request, do not remove the Connection and Upgrade headers, // as we forward them verbatim to the upstream hosts. - if (protocol == Protocol::Http11 && Utility::isUpgrade(request_headers)) { + if (Utility::isUpgrade(request_headers)) { // The current WebSocket implementation re-uses the HTTP1 codec to send upgrade headers to // the upstream host. This adds the "transfer-encoding: chunked" request header if the stream // has not ended and content-length does not exist. In HTTP1.1, if transfer-encoding and diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index c27886cda99..a17f7cde81a 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -28,10 +28,10 @@ class ConnectionManagerUtility { * existence of the x-forwarded-for header. Again see the method for more details. */ static Network::Address::InstanceConstSharedPtr - mutateRequestHeaders(Http::HeaderMap& request_headers, Protocol protocol, - Network::Connection& connection, ConnectionManagerConfig& config, - const Router::Config& route_config, Runtime::RandomGenerator& random, - Runtime::Loader& runtime, const LocalInfo::LocalInfo& local_info); + mutateRequestHeaders(Http::HeaderMap& request_headers, Network::Connection& connection, + ConnectionManagerConfig& config, const Router::Config& route_config, + Runtime::RandomGenerator& random, Runtime::Loader& runtime, + const LocalInfo::LocalInfo& local_info); static void mutateResponseHeaders(Http::HeaderMap& response_headers, const Http::HeaderMap* request_headers, const std::string& via); diff --git a/source/common/http/headers.h b/source/common/http/headers.h index 09816aea981..abe23615dfa 100644 --- a/source/common/http/headers.h +++ b/source/common/http/headers.h @@ -76,6 +76,7 @@ class HeaderValues { const LowerCaseString Origin{"origin"}; const LowerCaseString OtSpanContext{"x-ot-span-context"}; const LowerCaseString Path{":path"}; + const LowerCaseString Protocol{":protocol"}; const LowerCaseString ProxyConnection{"proxy-connection"}; const LowerCaseString Referer{"referer"}; const LowerCaseString RequestId{"x-request-id"}; @@ -158,6 +159,7 @@ class HeaderValues { } ExpectValues; struct { + const std::string Connect{"CONNECT"}; const std::string Get{"GET"}; const std::string Head{"HEAD"}; const std::string Post{"POST"}; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index c9f623a4344..9451e82e4d3 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -17,7 +17,6 @@ #include "common/http/codes.h" #include "common/http/exception.h" #include "common/http/headers.h" -#include "common/http/utility.h" namespace Envoy { namespace Http { @@ -90,7 +89,15 @@ void ConnectionImpl::StreamImpl::encode100ContinueHeaders(const HeaderMap& heade void ConnectionImpl::StreamImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) { std::vector final_headers; - buildHeaders(final_headers, headers); + + Http::HeaderMapPtr modified_headers; + if (Http::Utility::isUpgrade(headers)) { + modified_headers = std::make_unique(headers); + transformUpgradeFromH1toH2(*modified_headers); + buildHeaders(final_headers, *modified_headers); + } else { + buildHeaders(final_headers, headers); + } nghttp2_data_provider provider; if (!end_stream) { @@ -151,6 +158,11 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { readDisable(false); } +void ConnectionImpl::StreamImpl::decodeHeaders() { + maybeTransformUpgradeFromH2ToH1(); + decoder_->decodeHeaders(std::move(headers_), remote_end_stream_); +} + void ConnectionImpl::StreamImpl::pendingSendBufferHighWatermark() { ENVOY_CONN_LOG(debug, "send buffer over limit ", parent_.connection_); ASSERT(!pending_send_buffer_high_watermark_called_); @@ -366,13 +378,13 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { ASSERT(!stream->remote_end_stream_); stream->decoder_->decode100ContinueHeaders(std::move(stream->headers_)); } else { - stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); + stream->decodeHeaders(); } break; } case NGHTTP2_HCAT_REQUEST: { - stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); + stream->decodeHeaders(); break; } @@ -401,7 +413,7 @@ int ConnectionImpl::onFrameReceived(const nghttp2_frame* frame) { // start out with. In this case, raise as headers. nghttp2 message checking guarantees // proper flow here. ASSERT(!stream->headers_->Status() || stream->headers_->Status()->value() != "100"); - stream->decoder_->decodeHeaders(std::move(stream->headers_), stream->remote_end_stream_); + stream->decodeHeaders(); } } @@ -734,6 +746,10 @@ ConnectionImpl::Http2Options::Http2Options(const Http2Settings& http2_settings) if (http2_settings.hpack_table_size_ != NGHTTP2_DEFAULT_HEADER_TABLE_SIZE) { nghttp2_option_set_max_deflate_dynamic_table_size(options_, http2_settings.hpack_table_size_); } + if (http2_settings.allow_connect_) { + // TODO(alyssawilk) change to ENABLE_CONNECT_PROTOCOL when it's available. + nghttp2_option_set_no_http_messaging(options_, 1); + } } ConnectionImpl::Http2Options::~Http2Options() { nghttp2_option_del(options_); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index d08994a9d51..a55d1864a2a 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -19,6 +19,7 @@ #include "common/common/logger.h" #include "common/http/codec_helper.h" #include "common/http/header_map_impl.h" +#include "common/http/utility.h" #include "absl/types/optional.h" #include "nghttp2/nghttp2.h" @@ -187,6 +188,13 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable 0; } ConnectionImpl& parent_; @@ -224,6 +232,16 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, nghttp2_data_provider* provider) override; + void transformUpgradeFromH1toH2(HeaderMap& headers) override { + upgrade_type_ = headers.Upgrade()->value().c_str(); + Http::Utility::transformUpgradeRequestFromH1toH2(headers); + } + void maybeTransformUpgradeFromH2ToH1() override { + if (!upgrade_type_.empty() && headers_->Status()) { + Http::Utility::transformUpgradeResponseFromH2toH1(*headers_, upgrade_type_); + } + } + std::string upgrade_type_; }; /** @@ -235,6 +253,14 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& final_headers, nghttp2_data_provider* provider) override; + void transformUpgradeFromH1toH2(HeaderMap& headers) override { + Http::Utility::transformUpgradeResponseFromH1toH2(headers); + } + void maybeTransformUpgradeFromH2ToH1() override { + if (Http::Utility::isH2UpgradeRequest(*headers_)) { + Http::Utility::transformUpgradeRequestFromH2toH1(*headers_); + } + } }; ConnectionImpl* base() { return this; } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 0204f0fd32e..5b73058f2da 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -209,6 +209,12 @@ bool Utility::isUpgrade(const HeaderMap& headers) { Http::Headers::get().ConnectionValues.Upgrade.c_str())); } +bool Utility::isH2UpgradeRequest(const HeaderMap& headers) { + return headers.Method() && + headers.Method()->value().c_str() == Http::Headers::get().MethodValues.Connect && + headers.Protocol() && !headers.Protocol()->value().empty(); +} + bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) { return (isUpgrade(headers) && (0 == StringUtil::caseInsensitiveCompare( headers.Upgrade()->value().c_str(), @@ -227,6 +233,7 @@ Utility::parseHttp2Settings(const envoy::api::v2::core::Http2ProtocolOptions& co ret.initial_connection_window_size_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, initial_connection_window_size, Http::Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); + ret.allow_connect_ = config.allow_connect(); return ret; } @@ -398,5 +405,49 @@ std::string Utility::queryParamsToString(const QueryParams& params) { return out; } +void Utility::transformUpgradeRequestFromH1toH2(HeaderMap& headers) { + ASSERT(Utility::isUpgrade(headers)); + + const HeaderString& upgrade = headers.Upgrade()->value(); + headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Connect); + headers.insertProtocol().value().setCopy(upgrade.c_str(), upgrade.size()); + headers.removeUpgrade(); + headers.removeConnection(); +} + +void Utility::transformUpgradeResponseFromH1toH2(HeaderMap& headers) { + if (getResponseStatus(headers) == 101) { + headers.insertStatus().value().setInteger(200); + } + headers.removeUpgrade(); + headers.removeConnection(); +} + +void Utility::transformUpgradeRequestFromH2toH1(HeaderMap& headers) { + ASSERT(Utility::isH2UpgradeRequest(headers)); + + const HeaderString& protocol = headers.Protocol()->value(); + headers.insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); + headers.insertUpgrade().value().setCopy(protocol.c_str(), protocol.size()); + headers.insertConnection().value().setReference(Http::Headers::get().ConnectionValues.Upgrade); + headers.removeProtocol(); + if (headers.ContentLength() == nullptr) { + headers.insertTransferEncoding().value().setReference( + Http::Headers::get().TransferEncodingValues.Chunked); + } +} + +void Utility::transformUpgradeResponseFromH2toH1(HeaderMap& headers, absl::string_view upgrade) { + if (getResponseStatus(headers) == 200) { + headers.insertUpgrade().value().setCopy(upgrade.data(), upgrade.size()); + headers.insertConnection().value().setReference(Http::Headers::get().ConnectionValues.Upgrade); + if (headers.ContentLength() == nullptr) { + headers.insertTransferEncoding().value().setReference( + Http::Headers::get().TransferEncodingValues.Chunked); + } + headers.insertStatus().value().setInteger(101); + } +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 65787cb24e0..f7e3544ae33 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -101,6 +101,11 @@ uint64_t getResponseStatus(const HeaderMap& headers); */ bool isUpgrade(const HeaderMap& headers); +/** + * @return true if this is a CONNECT request with a :protocol header present, false otherwise. + */ +bool isH2UpgradeRequest(const HeaderMap& headers); + /** * Determine whether this is a WebSocket Upgrade request. * This function returns true if the following HTTP headers and values are present: @@ -200,6 +205,34 @@ MessagePtr prepareHeaders(const ::envoy::api::v2::core::HttpUri& http_uri); */ std::string queryParamsToString(const QueryParams& query_params); +/** + * Transforms the supplied headers from an HTTP/1 Upgrade request to an H2 style upgrade. + * Changes the method to connection, moves the Upgrade to a :protocol header, + * @param headers the headers to convert. + */ +void transformUpgradeRequestFromH1toH2(HeaderMap& headers); + +/** + * Transforms the supplied headers from an HTTP/1 Upgrade response to an H2 style upgrade response. + * Changes the 101 upgrade response to a 200 for the CONNECT response. + * @param headers the headers to convert. + */ +void transformUpgradeResponseFromH1toH2(HeaderMap& headers); + +/** + * Transforms the supplied headers from an H2 "CONNECT"-with-:protocol-header to an HTTP/1 style + * Upgrade response. + * @param headers the headers to convert. + */ +void transformUpgradeRequestFromH2toH1(HeaderMap& headers); + +/** + * Transforms the supplied headers from an H2 "CONNECT success" to an HTTP/1 style Upgrade response. + * The caller is responsible for ensuring this only happens on upgraded streams. + * @param headers the headers to convert. + */ +void transformUpgradeResponseFromH2toH1(HeaderMap& headers, absl::string_view upgrade); + } // namespace Utility } // namespace Http } // namespace Envoy diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 00bc9ac8a52..1e987304400 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -88,11 +88,11 @@ class ConnectionManagerUtilityTest : public testing::Test { // This is a convenience method used to call mutateRequestHeaders(). It is done in this // convoluted way to force tests to check both the final downstream address as well as whether // the request is internal/external, given the importance of these two pieces of data. - MutateRequestRet callMutateRequestHeaders(HeaderMap& headers, Protocol protocol) { + MutateRequestRet callMutateRequestHeaders(HeaderMap& headers, Protocol) { MutateRequestRet ret; ret.downstream_address_ = - ConnectionManagerUtility::mutateRequestHeaders( - headers, protocol, connection_, config_, route_config_, random_, runtime_, local_info_) + ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, route_config_, + random_, runtime_, local_info_) ->asString(); ret.internal_ = headers.EnvoyInternalRequest() != nullptr; return ret; @@ -536,16 +536,6 @@ TEST_F(ConnectionManagerUtilityTest, RemoveConnectionUpgradeForNonWebSocketReque EXPECT_FALSE(headers.has("upgrade")); } -// Make sure we remove connections headers for a WS request over h2. -TEST_F(ConnectionManagerUtilityTest, RemoveConnectionUpgradeForHttp2Requests) { - TestHeaderMapImpl headers{{"connection", "upgrade"}, {"upgrade", "websocket"}}; - - EXPECT_EQ((MutateRequestRet{"10.0.0.3:50000", false}), - callMutateRequestHeaders(headers, Protocol::Http2)); - EXPECT_FALSE(headers.has("connection")); - EXPECT_FALSE(headers.has("upgrade")); -} - // Test cleaning response headers. TEST_F(ConnectionManagerUtilityTest, MutateResponseHeaders) { TestHeaderMapImpl response_headers{ diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 93476a8f8a3..0e72e83261a 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -74,6 +74,93 @@ TEST(HttpUtility, isUpgrade) { TestHeaderMapImpl{{"connection", "keep-alive, Upgrade"}, {"upgrade", "FOO"}})); } +// Start with H1 style websocket request headers. Transform to H2 and back. +TEST(HttpUtility, H1H2H1Request) { + TestHeaderMapImpl converted_headers = { + {":method", "GET"}, {"content-length", "0"}, {"Upgrade", "foo"}, {"Connection", "upgrade"}}; + const TestHeaderMapImpl original_headers(converted_headers); + + ASSERT_TRUE(Utility::isUpgrade(converted_headers)); + ASSERT_FALSE(Utility::isH2UpgradeRequest(converted_headers)); + Utility::transformUpgradeRequestFromH1toH2(converted_headers); + + ASSERT_FALSE(Utility::isUpgrade(converted_headers)); + ASSERT_TRUE(Utility::isH2UpgradeRequest(converted_headers)); + Utility::transformUpgradeRequestFromH2toH1(converted_headers); + + ASSERT_TRUE(Utility::isUpgrade(converted_headers)); + ASSERT_FALSE(Utility::isH2UpgradeRequest(converted_headers)); + ASSERT_EQ(converted_headers, original_headers); +} + +// Start with H2 style websocket request headers. Transform to H1 and back. +TEST(HttpUtility, H2H1H2Request) { + TestHeaderMapImpl converted_headers = { + {":method", "CONNECT"}, {"content-length", "0"}, {":protocol", "websocket"}}; + const TestHeaderMapImpl original_headers(converted_headers); + + ASSERT_FALSE(Utility::isUpgrade(converted_headers)); + ASSERT_TRUE(Utility::isH2UpgradeRequest(converted_headers)); + Utility::transformUpgradeRequestFromH2toH1(converted_headers); + + ASSERT_TRUE(Utility::isUpgrade(converted_headers)); + ASSERT_FALSE(Utility::isH2UpgradeRequest(converted_headers)); + Utility::transformUpgradeRequestFromH1toH2(converted_headers); + + ASSERT_FALSE(Utility::isUpgrade(converted_headers)); + ASSERT_TRUE(Utility::isH2UpgradeRequest(converted_headers)); + ASSERT_EQ(converted_headers, original_headers); +} + +// Start with H1 style websocket response headers. Transform to H2 and back. +TEST(HttpUtility, H1H2H1Response) { + TestHeaderMapImpl converted_headers = {{":status", "101"}, + {"content-length", "0"}, + {"upgrade", "websocket"}, + {"connection", "upgrade"}}; + const TestHeaderMapImpl original_headers(converted_headers); + + ASSERT_TRUE(Utility::isUpgrade(converted_headers)); + Utility::transformUpgradeResponseFromH1toH2(converted_headers); + + ASSERT_FALSE(Utility::isUpgrade(converted_headers)); + Utility::transformUpgradeResponseFromH2toH1(converted_headers, "websocket"); + + ASSERT_TRUE(Utility::isUpgrade(converted_headers)); + ASSERT_EQ(converted_headers, original_headers); +} + +// Users of the transformation functions should not expect the results to be +// identical. Because the headers are always added in a set order, the original +// header order may not be preserved. +TEST(HttpUtility, OrderNotPreserved) { + TestHeaderMapImpl expected_headers = { + {":method", "GET"}, {"content-length", "0"}, {"Upgrade", "foo"}, {"Connection", "upgrade"}}; + + TestHeaderMapImpl converted_headers = { + {":method", "GET"}, {"content-length", "0"}, {"Connection", "upgrade"}, {"Upgrade", "foo"}}; + + Utility::transformUpgradeRequestFromH1toH2(converted_headers); + Utility::transformUpgradeRequestFromH2toH1(converted_headers); + EXPECT_EQ(converted_headers, expected_headers); +} + +// A more serious problem with using WebSocket help for general Upgrades, is that method for +// WebSocket is always GET but the method for other upgrades is allowed to be a +// POST. This is a documented weakness in Envoy docs and can be addressed with +// a custom x-envoy-original-method header if it is ever needed. +TEST(HttpUtility, MethodNotPreserved) { + TestHeaderMapImpl expected_headers = { + {":method", "GET"}, {"content-length", "0"}, {"Upgrade", "foo"}, {"Connection", "upgrade"}}; + + TestHeaderMapImpl converted_headers = { + {":method", "POST"}, {"content-length", "0"}, {"Upgrade", "foo"}, {"Connection", "upgrade"}}; + + Utility::transformUpgradeRequestFromH1toH2(converted_headers); + Utility::transformUpgradeRequestFromH2toH1(converted_headers); + EXPECT_EQ(converted_headers, expected_headers); +} + TEST(HttpUtility, appendXff) { { TestHeaderMapImpl headers; diff --git a/test/integration/BUILD b/test/integration/BUILD index 6a7aaaa48e0..7d6409f3277 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -331,7 +331,7 @@ envoy_cc_test( "websocket_integration_test.h", ], deps = [ - ":http_integration_lib", + ":http_protocol_integration_lib", "//source/common/http:header_map_lib", "//source/extensions/access_loggers/file:config", "//source/extensions/filters/http/buffer:config", diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 02677e1aacb..ef3ee25532f 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -204,8 +204,10 @@ FakeHttpConnection::FakeHttpConnection(SharedConnectionWrapper& shared_connectio codec_.reset(new Http::Http1::ServerConnectionImpl(shared_connection_.connection(), *this, Http::Http1Settings())); } else { + auto settings = Http::Http2Settings(); + settings.allow_connect_ = true; codec_.reset(new Http::Http2::ServerConnectionImpl(shared_connection_.connection(), *this, - store, Http::Http2Settings())); + store, settings)); ASSERT(type == Type::HTTP2); } diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 1761bfd0063..12272b0a62e 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -172,6 +172,7 @@ IntegrationCodecClientPtr HttpIntegrationTest::makeHttpConnection(uint32_t port) IntegrationCodecClientPtr HttpIntegrationTest::makeHttpConnection(Network::ClientConnectionPtr&& conn) { std::shared_ptr cluster{new NiceMock()}; + cluster->http2_settings_.allow_connect_ = true; Upstream::HostDescriptionConstSharedPtr host_description{Upstream::makeTestHostDescription( cluster, fmt::format("tcp://{}:80", Network::Test::getLoopbackAddressUrlString(version_)))}; return IntegrationCodecClientPtr{new IntegrationCodecClient( diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index 14de3e9ebbc..574bd920649 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -26,21 +26,47 @@ Http::TestHeaderMapImpl upgradeRequestHeaders(const char* upgrade_type = "websoc return Http::TestHeaderMapImpl{ {":authority", "host"}, {"content-length", fmt::format("{}", content_length)}, {":path", "/websocket/test"}, {":method", "GET"}, - {"upgrade", upgrade_type}, {"connection", "keep-alive, Upgrade"}}; + {"upgrade", upgrade_type}, {"connection", "keep-alive, upgrade"}}; } Http::TestHeaderMapImpl upgradeResponseHeaders(const char* upgrade_type = "websocket") { return Http::TestHeaderMapImpl{{":status", "101"}, - {"connection", "Upgrade"}, + {"connection", "upgrade"}, {"upgrade", upgrade_type}, {"content-length", "0"}}; } -static std::string websocketTestParamsToString( - const testing::TestParamInfo> params) { - return absl::StrCat(std::get<0>(params.param) == Network::Address::IpVersion::v4 ? "IPv4" - : "IPv6", - "_", std::get<1>(params.param) == true ? "OldStyle" : "NewStyle"); +std::string +websocketTestParamsToString(const ::testing::TestParamInfo& params) { + return absl::StrCat( + (params.param.version == Network::Address::IpVersion::v4 ? "IPv4_" : "IPv6_"), + (params.param.downstream_protocol == Http::CodecClient::Type::HTTP2 ? "Http2Downstream_" + : "HttpDownstream_"), + (params.param.upstream_protocol == FakeHttpConnection::Type::HTTP2 ? "Http2Upstream" + : "HttpUpstream"), + params.param.old_style == true ? "_Old" : "_New"); +} + +std::vector getWebsocketTestParams() { + const std::vector downstream_protocols = { + Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2}; + const std::vector upstream_protocols = { + FakeHttpConnection::Type::HTTP1, FakeHttpConnection::Type::HTTP2}; + std::vector ret; + + for (auto ip_version : TestEnvironment::getIpVersionsForTest()) { + for (auto downstream_protocol : downstream_protocols) { + for (auto upstream_protocol : upstream_protocols) { + ret.push_back( + WebsocketProtocolTestParams{ip_version, downstream_protocol, upstream_protocol, false}); + } + } + } + for (auto ip_version : TestEnvironment::getIpVersionsForTest()) { + ret.push_back(WebsocketProtocolTestParams{ip_version, Http::CodecClient::Type::HTTP1, + FakeHttpConnection::Type::HTTP1, true}); + } + return ret; } } // namespace @@ -90,22 +116,29 @@ void WebsocketIntegrationTest::validateUpgradeResponseHeaders( } commonValidate(proxied_response_headers, original_response_headers); - EXPECT_EQ(proxied_response_headers, original_response_headers); + EXPECT_THAT(&proxied_response_headers, HeaderMapEqualIgnoreOrder(&original_response_headers)); } void WebsocketIntegrationTest::commonValidate(Http::HeaderMap& proxied_headers, const Http::HeaderMap& original_headers) { - // If no content length is specified, the HTTP codec should add a chunked encoding header. - if (original_headers.ContentLength() == nullptr) { + // If no content length is specified, the HTTP1 codec will add a chunked encoding header. + if (original_headers.ContentLength() == nullptr && + proxied_headers.TransferEncoding() != nullptr) { ASSERT_STREQ(proxied_headers.TransferEncoding()->value().c_str(), "chunked"); proxied_headers.removeTransferEncoding(); } + if (proxied_headers.Connection() != nullptr && + proxied_headers.Connection()->value() == "upgrade" && + original_headers.Connection() != nullptr && + original_headers.Connection()->value() == "keep-alive, upgrade") { + // The keep-alive is implicit for HTTP/1.1, so Enovy only sets the upgrade + // header when converting from HTTP/1.1 to H2 + proxied_headers.Connection()->value().setCopy("keep-alive, upgrade", 19); + } } -INSTANTIATE_TEST_CASE_P(IpVersions, WebsocketIntegrationTest, - testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - testing::Bool()), - websocketTestParamsToString); +INSTANTIATE_TEST_CASE_P(Protocols, WebsocketIntegrationTest, + testing::ValuesIn(getWebsocketTestParams()), websocketTestParamsToString); ConfigHelper::HttpModifierFunction setRouteUsingWebsocket(const envoy::api::v2::route::RouteAction::WebSocketProxyConfig* ws_config, @@ -133,6 +166,19 @@ void WebsocketIntegrationTest::initialize() { if (old_style_websockets_) { // Set a less permissive default route so it does not pick up the /websocket query. config_helper_.setDefaultHostAndRoute("*", "/asd"); + } else { + if (upstreamProtocol() != FakeHttpConnection::Type::HTTP1) { + config_helper_.addConfigModifier( + [&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) -> void { + auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0); + cluster->mutable_http2_protocol_options()->set_allow_connect(true); + }); + } + if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) { + config_helper_.addConfigModifier( + [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& + hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_connect(true); }); + } } HttpIntegrationTest::initialize(); } @@ -191,7 +237,8 @@ TEST_P(WebsocketIntegrationTest, WebSocketConnectionDownstreamDisconnect) { // Verify the final data was received and that the connection is torn down. ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hellobye!")); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + + ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); } TEST_P(WebsocketIntegrationTest, WebSocketConnectionUpstreamDisconnect) { @@ -212,8 +259,7 @@ TEST_P(WebsocketIntegrationTest, WebSocketConnectionUpstreamDisconnect) { // Verify both the data and the disconnect went through. response_->waitForBodyData(5); EXPECT_EQ("world", response_->body()); - codec_client_->waitForDisconnect(); - ASSERT(!fake_upstream_connection_->connected()); + waitForClientDisconnectOrReset(); } TEST_P(WebsocketIntegrationTest, EarlyData) { @@ -250,8 +296,12 @@ TEST_P(WebsocketIntegrationTest, EarlyData) { response_->waitForHeaders(); auto upgrade_response_headers(upgradeResponseHeaders()); validateUpgradeResponseHeaders(response_->headers(), upgrade_response_headers); - response_->waitForBodyData(5); - codec_client_->waitForDisconnect(); + + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + // For H2, the disconnect may result in the terminal data not being proxied. + response_->waitForBodyData(5); + } + waitForClientDisconnectOrReset(); EXPECT_EQ("world", response_->body()); } @@ -282,8 +332,8 @@ TEST_P(WebsocketIntegrationTest, WebSocketConnectionIdleTimeout) { } else { test_server_->waitForCounterGe("http.config_test.downstream_rq_idle_timeout", 1); } - codec_client_->waitForDisconnect(); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + waitForClientDisconnectOrReset(); + ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); } TEST_P(WebsocketIntegrationTest, WebSocketLogging) { @@ -364,15 +414,19 @@ TEST_P(WebsocketIntegrationTest, NonWebsocketUpgrade) { performUpgrade(upgradeRequestHeaders("foo", 0), upgradeResponseHeaders("foo")); sendBidirectionalData(); - - // downstream disconnect codec_client_->sendData(*request_encoder_, "bye!", false); - codec_client_->close(); + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + codec_client_->close(); + } else { + codec_client_->sendReset(*request_encoder_); + } + ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, "hellobye!")); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); auto upgrade_response_headers(upgradeResponseHeaders("foo")); validateUpgradeResponseHeaders(response_->headers(), upgrade_response_headers); + codec_client_->close(); } TEST_P(WebsocketIntegrationTest, WebsocketCustomFilterChain) { @@ -408,7 +462,8 @@ TEST_P(WebsocketIntegrationTest, WebsocketCustomFilterChain) { codec_client_->sendData(encoder_decoder.first, early_data_req_str, false); response_->waitForEndStream(); EXPECT_STREQ("413", response_->headers().Status()->value().c_str()); - codec_client_->waitForDisconnect(); + waitForClientDisconnectOrReset(); + codec_client_->close(); } // HTTP requests are configured to disallow large bodies. @@ -421,7 +476,8 @@ TEST_P(WebsocketIntegrationTest, WebsocketCustomFilterChain) { codec_client_->sendData(encoder_decoder.first, early_data_req_str, false); response_->waitForEndStream(); EXPECT_STREQ("413", response_->headers().Status()->value().c_str()); - codec_client_->waitForDisconnect(); + waitForClientDisconnectOrReset(); + codec_client_->close(); } // Foo upgrades are configured without the buffer filter, so should explicitly @@ -434,7 +490,7 @@ TEST_P(WebsocketIntegrationTest, WebsocketCustomFilterChain) { // Tear down all the connections cleanly. codec_client_->close(); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); } } @@ -450,8 +506,12 @@ TEST_P(WebsocketIntegrationTest, BidirectionalChunkedData) { // With content-length not present, the HTTP codec will send the request with // transfer-encoding: chunked. - ASSERT_TRUE(upstream_request_->headers().TransferEncoding() != nullptr); - ASSERT_TRUE(response_->headers().TransferEncoding() != nullptr); + if (upstreamProtocol() == FakeHttpConnection::Type::HTTP1) { + ASSERT_TRUE(upstream_request_->headers().TransferEncoding() != nullptr); + } + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + ASSERT_TRUE(response_->headers().TransferEncoding() != nullptr); + } // Send both a chunked request body and "websocket" payload. std::string request_payload = "3\r\n123\r\n0\r\n\r\nSomeWebsocketRequestPayload"; @@ -473,7 +533,7 @@ TEST_P(WebsocketIntegrationTest, BidirectionalChunkedData) { // Clean up. codec_client_->close(); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + ASSERT_TRUE(waitForUpstreamDisconnectOrReset()); } } // namespace Envoy diff --git a/test/integration/websocket_integration_test.h b/test/integration/websocket_integration_test.h index d37476d67b9..69f9c2530ff 100644 --- a/test/integration/websocket_integration_test.h +++ b/test/integration/websocket_integration_test.h @@ -1,18 +1,28 @@ #pragma once -#include "test/integration/http_integration.h" +#include "test/integration/http_protocol_integration.h" #include "gtest/gtest.h" namespace Envoy { -class WebsocketIntegrationTest - : public HttpIntegrationTest, - public testing::TestWithParam> { +struct WebsocketProtocolTestParams { + Network::Address::IpVersion version; + Http::CodecClient::Type downstream_protocol; + FakeHttpConnection::Type upstream_protocol; + bool old_style; +}; + +class WebsocketIntegrationTest : public HttpIntegrationTest, + public testing::TestWithParam { public: - void initialize() override; WebsocketIntegrationTest() - : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, std::get<0>(GetParam())) {} + : HttpIntegrationTest(GetParam().downstream_protocol, GetParam().version) {} + void initialize() override; + void SetUp() override { + setDownstreamProtocol(GetParam().downstream_protocol); + setUpstreamProtocol(GetParam().upstream_protocol); + } protected: void performUpgrade(const Http::TestHeaderMapImpl& upgrade_request_headers, @@ -21,18 +31,33 @@ class WebsocketIntegrationTest void validateUpgradeRequestHeaders(const Http::HeaderMap& proxied_request_headers, const Http::HeaderMap& original_request_headers); - void validateUpgradeResponseHeaders(const Http::HeaderMap& proxied_response_headers, const Http::HeaderMap& original_response_headers); - void commonValidate(Http::HeaderMap& proxied_headers, const Http::HeaderMap& original_headers); + ABSL_MUST_USE_RESULT + testing::AssertionResult waitForUpstreamDisconnectOrReset() { + if (upstreamProtocol() != FakeHttpConnection::Type::HTTP1) { + return upstream_request_->waitForReset(); + } else { + return fake_upstream_connection_->waitForDisconnect(); + } + } + + void waitForClientDisconnectOrReset() { + if (downstreamProtocol() != Http::CodecClient::Type::HTTP1) { + response_->waitForReset(); + } else { + codec_client_->waitForDisconnect(); + } + } + + IntegrationStreamDecoderPtr response_; // True if the test uses "old style" TCP proxy websockets. False to use the // new style "HTTP filter chain" websockets. // See // https://github.com/envoyproxy/envoy/blob/master/docs/root/intro/arch_overview/websocket.rst - bool old_style_websockets_{std::get<1>(GetParam())}; - IntegrationStreamDecoderPtr response_; + bool old_style_websockets_{GetParam().old_style}; }; } // namespace Envoy diff --git a/test/test_common/BUILD b/test/test_common/BUILD index 0d7db72237c..25ecd618efc 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -4,6 +4,7 @@ load( "//bazel:envoy_build_system.bzl", "envoy_basic_cc_library", "envoy_cc_library", + "envoy_cc_test", "envoy_cc_test_library", "envoy_package", ) @@ -92,6 +93,14 @@ envoy_cc_test_library( ], ) +envoy_cc_test( + name = "utility_test", + srcs = ["utility_test.cc"], + deps = [ + ":utility_lib", + ], +) + envoy_cc_test_library( name = "logging_lib", srcs = ["logging.cc"], diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index c2f738de313..e103ce215f1 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -43,6 +43,34 @@ TestRandomGenerator::TestRandomGenerator() uint64_t TestRandomGenerator::random() { return generator_(); } +bool TestUtility::headerMapEqualIgnoreOrder(const Http::HeaderMap& lhs, + const Http::HeaderMap& rhs) { + if (lhs.size() != rhs.size()) { + return false; + } + + struct State { + const Http::HeaderMap& lhs; + bool equal; + }; + + State state{lhs, true}; + rhs.iterate( + [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { + State* state = static_cast(context); + const Http::HeaderEntry* entry = + state->lhs.get(Http::LowerCaseString(std::string(header.key().c_str()))); + if (entry == nullptr || (entry->value() != header.value().c_str())) { + state->equal = false; + return Http::HeaderMap::Iterate::Break; + } + return Http::HeaderMap::Iterate::Continue; + }, + &state); + + return state.equal; +} + bool TestUtility::buffersEqual(const Buffer::Instance& lhs, const Buffer::Instance& rhs) { if (lhs.length() != rhs.length()) { return false; diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 3300c44678c..a732fc36320 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -102,6 +102,15 @@ class TestRandomGenerator { class TestUtility { public: + /** + * Compare 2 HeaderMaps. + * @param lhs supplies HeaderMaps 1. + * @param rhs supplies HeaderMaps 2. + * @return TRUE if the HeaderMapss are equal, ignoring the order of the + * headers, false if not. + */ + static bool headerMapEqualIgnoreOrder(const Http::HeaderMap& lhs, const Http::HeaderMap& rhs); + /** * Compare 2 buffers. * @param lhs supplies buffer 1. @@ -405,6 +414,11 @@ class MockedTestAllocator : public RawStatDataAllocator { } // namespace Stats +MATCHER_P(HeaderMapEqualIgnoreOrder, rhs, "") { + *result_listener << *rhs << " is not equal to " << *arg; + return TestUtility::headerMapEqualIgnoreOrder(*arg, *rhs); +} + MATCHER_P(ProtoEq, rhs, "") { return TestUtility::protoEqual(arg, rhs); } MATCHER_P(RepeatedProtoEq, rhs, "") { return TestUtility::repeatedPtrFieldEqual(arg, rhs); } diff --git a/test/test_common/utility_test.cc b/test/test_common/utility_test.cc new file mode 100644 index 00000000000..40083fe3394 --- /dev/null +++ b/test/test_common/utility_test.cc @@ -0,0 +1,29 @@ +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +using Envoy::Http::HeaderMap; + +namespace Envoy { + +TEST(headerMapEqualIgnoreOrder, ActuallyEqual) { + Http::TestHeaderMapImpl lhs{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + Http::TestHeaderMapImpl rhs{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + EXPECT_TRUE(TestUtility::headerMapEqualIgnoreOrder(lhs, rhs)); + EXPECT_EQ(lhs, rhs); +} + +TEST(headerMapEqualIgnoreOrder, IgnoreOrder) { + Http::TestHeaderMapImpl lhs{{":method", "GET"}, {":authority", "host"}, {":path", "/"}}; + Http::TestHeaderMapImpl rhs{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + EXPECT_TRUE(TestUtility::headerMapEqualIgnoreOrder(lhs, rhs)); + EXPECT_THAT(&lhs, HeaderMapEqualIgnoreOrder(&rhs)); + EXPECT_FALSE(lhs == rhs); +} + +TEST(headerMapEqualIgnoreOrder, NotEqual) { + Http::TestHeaderMapImpl lhs{{":method", "GET"}, {":authority", "host"}, {":authority", "host"}}; + Http::TestHeaderMapImpl rhs{{":method", "GET"}, {":authority", "host"}}; + EXPECT_FALSE(TestUtility::headerMapEqualIgnoreOrder(lhs, rhs)); +} +} // namespace Envoy