From b49f7e184d6c3d1aa86ba6ff2f55496ac0d8411f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 16 Aug 2018 15:38:45 -0400 Subject: [PATCH 1/5] tunneling websockets (and upgrades in general) over H2 Signed-off-by: Alyssa Wilk --- api/envoy/api/v2/core/protocol.proto | 14 ++ docs/root/intro/arch_overview/websocket.rst | 25 ++++ 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 | 33 ++++- source/common/http/http2/codec_impl.h | 5 + source/common/http/utility.cc | 53 ++++++++ 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 | 124 +++++++++++++----- 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, 475 insertions(+), 67 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 2e7948787dfe..41bd70d5555f 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. + google.protobuf.BoolValue 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 35aa8477fc53..2e192e31d59a 100644 --- a/docs/root/intro/arch_overview/websocket.rst +++ b/docs/root/intro/arch_overview/websocket.rst @@ -20,6 +20,31 @@ 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) +--------------------------------------------- + +One oft requested feature for Envoy was to allow WebSocket to traverse HTTP/2 hops, where there +was a set-up such as + +Client ---- HTTP/1.1 ---- Frontline Envoy ---- HTTP/2 ---- Second tier Envoy ---- H1 ---- Upstream + +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 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 ebe58b59d1db..7fc2e023e8ee 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 c27886cda993..a17f7cde81a8 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 09816aea9812..abe23615dfa2 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 200c33441995..03d5b7c610ca 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -90,7 +90,19 @@ 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); + + if (Http::Utility::isUpgrade(headers)) { + HeaderMapImpl modified_headers(headers); + upgrade_type_ = headers.Upgrade()->value().c_str(); + if (headers.Status()) { + Http::Utility::transformUpgradeResponseFromH1toH2(modified_headers); + } else { + Http::Utility::transformUpgradeRequestFromH1toH2(modified_headers); + } + buildHeaders(final_headers, modified_headers); + } else { + buildHeaders(final_headers, headers); + } nghttp2_data_provider provider; if (!end_stream) { @@ -151,6 +163,15 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { readDisable(false); } +void ConnectionImpl::StreamImpl::decodeHeaders() { + if (Http::Utility::isH2UpgradeRequest(*headers_)) { + Http::Utility::transformUpgradeRequestFromH2toH1(*headers_); + } else if (!upgrade_type_.empty() && headers_->Status()) { + Http::Utility::transformUpgradeResponseFromH2toH1(*headers_, upgrade_type_); + } + 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 +387,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 +422,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(); } } @@ -735,6 +756,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 d08994a9d512..2b34af28f883 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -187,11 +187,16 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable 0; } ConnectionImpl& parent_; HeaderMapImplPtr headers_; StreamDecoder* decoder_{}; + std::string upgrade_type_; int32_t stream_id_{-1}; uint32_t unconsumed_bytes_{0}; uint32_t read_disable_count_{0}; diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index e3ff89ad8594..f296f8998b63 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,8 @@ 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_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, allow_connect, + Http::Http2Settings::DEFAULT_ALLOW_CONNECT); return ret; } @@ -392,5 +400,50 @@ 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(); + if (headers.ContentLength() == nullptr) { + headers.insertTransferEncoding().value().setReference( + Http::Headers::get().TransferEncodingValues.Chunked); + } +} + +void Utility::transformUpgradeResponseFromH1toH2(HeaderMap& headers) { + if (getResponseStatus(headers) == 101) { + headers.insertStatus().value().setCopy("200", 3); + } + headers.removeUpgrade(); + headers.removeConnection(); + if (headers.ContentLength() == nullptr) { + headers.insertTransferEncoding().value().setReference( + Http::Headers::get().TransferEncodingValues.Chunked); + } +} + +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(); +} + +void Utility::transformUpgradeResponseFromH2toH1(HeaderMap& headers, absl::string_view upgrade) { + if (getResponseStatus(headers) == 200) { + headers.insertStatus().value().setCopy("101", 3); + } + // TODO(alyssawilk) what should we do for websocket responses on the failure path? + headers.insertUpgrade().value().setCopy(upgrade.data(), upgrade.size()); + headers.insertConnection().value().setReference(Http::Headers::get().ConnectionValues.Upgrade); +} + } // namespace Http } // namespace Envoy diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 8a11ae5129f2..3f82b293c954 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: @@ -198,6 +203,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 411d0f483a3b..fc74f106e6ee 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 c50be78887ab..b323b35662db 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 25534d61eec2..fc2e6ae246a7 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -330,7 +330,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 29f2289a5ce9..f20715ddbc8b 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -203,8 +203,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 e1bfbab9e51c..6d0155b21abd 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -171,6 +171,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 14de3e9ebbc6..1a8396c42213 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,21 @@ 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()->mutable_allow_connect()->set_value(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()->mutable_allow_connect()->set_value(true); + }); + } } HttpIntegrationTest::initialize(); } @@ -191,7 +239,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 +261,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 +298,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 +334,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 +416,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 +464,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 +478,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 +492,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 +508,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 +535,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 d37476d67b95..69f9c2530ff1 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 d861e0b99ec7..cb48ffbf9be0 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 c2f738de3132..e103ce215f13 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 c27d4be9319f..a5ff381c1814 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 000000000000..40083fe33946 --- /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 From d8befc31370bb33eddaed6e12bfeb5987152b060 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 20 Aug 2018 12:29:50 -0400 Subject: [PATCH 2/5] reviwer comments -websocket-only Signed-off-by: Alyssa Wilk --- docs/root/intro/arch_overview/websocket.rst | 8 +++---- include/envoy/http/codec.h | 2 +- source/common/http/http2/codec_impl.cc | 19 +++++----------- source/common/http/http2/codec_impl.h | 23 ++++++++++++++++++- source/common/http/utility.cc | 25 ++++++++++----------- 5 files changed, 44 insertions(+), 33 deletions(-) diff --git a/docs/root/intro/arch_overview/websocket.rst b/docs/root/intro/arch_overview/websocket.rst index 2e192e31d59a..b9c4c98b8b49 100644 --- a/docs/root/intro/arch_overview/websocket.rst +++ b/docs/root/intro/arch_overview/websocket.rst @@ -23,10 +23,10 @@ data upstream. Handling H2 hops (implementation in progress) --------------------------------------------- -One oft requested feature for Envoy was to allow WebSocket to traverse HTTP/2 hops, where there -was a set-up such as +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 ---- Frontline Envoy ---- HTTP/2 ---- Second tier Envoy ---- H1 ---- Upstream +[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. @@ -34,7 +34,7 @@ upstream server functionally intact, which means it needs to traverse the HTTP/2 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 diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index f0afefc9e902..65a9074c66d2 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -242,7 +242,7 @@ 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 now allow CONNECT over H2. + // By default both nghttp2 and Envoy do not allow CONNECT over H2. static const bool DEFAULT_ALLOW_CONNECT = false; }; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 76c6c29ea416..9451e82e4d39 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 { @@ -91,15 +90,11 @@ void ConnectionImpl::StreamImpl::encode100ContinueHeaders(const HeaderMap& heade void ConnectionImpl::StreamImpl::encodeHeaders(const HeaderMap& headers, bool end_stream) { std::vector final_headers; + Http::HeaderMapPtr modified_headers; if (Http::Utility::isUpgrade(headers)) { - HeaderMapImpl modified_headers(headers); - upgrade_type_ = headers.Upgrade()->value().c_str(); - if (headers.Status()) { - Http::Utility::transformUpgradeResponseFromH1toH2(modified_headers); - } else { - Http::Utility::transformUpgradeRequestFromH1toH2(modified_headers); - } - buildHeaders(final_headers, modified_headers); + modified_headers = std::make_unique(headers); + transformUpgradeFromH1toH2(*modified_headers); + buildHeaders(final_headers, *modified_headers); } else { buildHeaders(final_headers, headers); } @@ -164,11 +159,7 @@ void ConnectionImpl::StreamImpl::pendingRecvBufferLowWatermark() { } void ConnectionImpl::StreamImpl::decodeHeaders() { - if (Http::Utility::isH2UpgradeRequest(*headers_)) { - Http::Utility::transformUpgradeRequestFromH2toH1(*headers_); - } else if (!upgrade_type_.empty() && headers_->Status()) { - Http::Utility::transformUpgradeResponseFromH2toH1(*headers_, upgrade_type_); - } + maybeTransformUpgradeFromH2ToH1(); decoder_->decodeHeaders(std::move(headers_), remote_end_stream_); } diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 2b34af28f883..a55d1864a2a3 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" @@ -191,12 +192,14 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable 0; } ConnectionImpl& parent_; HeaderMapImplPtr headers_; StreamDecoder* decoder_{}; - std::string upgrade_type_; int32_t stream_id_{-1}; uint32_t unconsumed_bytes_{0}; uint32_t read_disable_count_{0}; @@ -229,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_; }; /** @@ -240,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 f296f8998b63..f433ab21bda4 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -408,22 +408,14 @@ void Utility::transformUpgradeRequestFromH1toH2(HeaderMap& headers) { headers.insertProtocol().value().setCopy(upgrade.c_str(), upgrade.size()); headers.removeUpgrade(); headers.removeConnection(); - if (headers.ContentLength() == nullptr) { - headers.insertTransferEncoding().value().setReference( - Http::Headers::get().TransferEncodingValues.Chunked); - } } void Utility::transformUpgradeResponseFromH1toH2(HeaderMap& headers) { if (getResponseStatus(headers) == 101) { - headers.insertStatus().value().setCopy("200", 3); + headers.insertStatus().value().setInteger(200); } headers.removeUpgrade(); headers.removeConnection(); - if (headers.ContentLength() == nullptr) { - headers.insertTransferEncoding().value().setReference( - Http::Headers::get().TransferEncodingValues.Chunked); - } } void Utility::transformUpgradeRequestFromH2toH1(HeaderMap& headers) { @@ -434,15 +426,22 @@ void Utility::transformUpgradeRequestFromH2toH1(HeaderMap& headers) { 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.insertStatus().value().setCopy("101", 3); + 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); } - // TODO(alyssawilk) what should we do for websocket responses on the failure path? - headers.insertUpgrade().value().setCopy(upgrade.data(), upgrade.size()); - headers.insertConnection().value().setReference(Http::Headers::get().ConnectionValues.Upgrade); } } // namespace Http From de45c148a54a4200d626b11e494128e53c33d9b4 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 20 Aug 2018 12:51:35 -0400 Subject: [PATCH 3/5] Removed TODO: can't link to allow_connect until we unhide it Signed-off-by: Alyssa Wilk --- docs/root/intro/arch_overview/websocket.rst | 2 -- 1 file changed, 2 deletions(-) diff --git a/docs/root/intro/arch_overview/websocket.rst b/docs/root/intro/arch_overview/websocket.rst index b9c4c98b8b49..a1b66aadd8a1 100644 --- a/docs/root/intro/arch_overview/websocket.rst +++ b/docs/root/intro/arch_overview/websocket.rst @@ -43,8 +43,6 @@ Non-WebSocket upgrades are allowed to use any valid HTTP method (i.e. POST) and upgrade/downgrade mechanism will drop the original method and transform the Upgrade request to a GET method on the final Envoy-Upstream hop. -TODO(alyssawilk) link to the config changes required to enable upgrade upstream/downstream - Old style WebSocket support =========================== From a515a1e5321d711afd6d79da1b17450248610cf8 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 27 Aug 2018 13:20:07 -0400 Subject: [PATCH 4/5] BoolValue->bool Signed-off-by: Alyssa Wilk --- api/envoy/api/v2/core/protocol.proto | 2 +- source/common/http/utility.cc | 3 +-- test/integration/websocket_integration_test.cc | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index 41bd70d5555f..44d684841e0c 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -89,7 +89,7 @@ message Http2ProtocolOptions { // 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. - google.protobuf.BoolValue allow_connect = 5; + bool allow_connect = 5; } // [#not-implemented-hide:] diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index f433ab21bda4..3c968bd4e039 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -233,8 +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_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, allow_connect, - Http::Http2Settings::DEFAULT_ALLOW_CONNECT); + ret.allow_connect_ = config.allow_connect(); return ret; } diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index 1a8396c42213..c161b970aada 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -171,14 +171,14 @@ void WebsocketIntegrationTest::initialize() { config_helper_.addConfigModifier( [&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) -> void { auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0); - cluster->mutable_http2_protocol_options()->mutable_allow_connect()->set_value(true); + 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()->mutable_allow_connect()->set_value(true); + hcm.mutable_http2_protocol_options()->set_allow_connect(true); }); } } From 96ea33fb6ad929fbae03107a2d3e42555238ea36 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 27 Aug 2018 15:21:58 -0400 Subject: [PATCH 5/5] Fixing format :-( Signed-off-by: Alyssa Wilk --- test/integration/websocket_integration_test.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index c161b970aada..574bd9206491 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -177,9 +177,7 @@ void WebsocketIntegrationTest::initialize() { 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); - }); + hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_connect(true); }); } } HttpIntegrationTest::initialize();