From 548de9224e59cff38009d533bd66cd2f17244a01 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Wed, 9 Jan 2019 09:04:47 -0700 Subject: [PATCH 1/2] misc changes in support of HTTP tap These are various changes split out from https://github.com/envoyproxy/envoy/pull/5515. They include: 1) Allow the admin server to be accessed over HTTP/2 with prior knowledge. This is required to get the tap integration tests to pass on OSX. 2) Change admin to buffer the complete request so that admin handlers can support POST bodies. 3) Small integration test fix in waitForBodyData() to avoid a race condition in which the body data arrives before the call. 4) Small comment and clang-tidy fixes. Signed-off-by: Matt Klein --- docs/root/intro/version_history.rst | 1 + include/envoy/server/admin.h | 5 ++ include/envoy/singleton/BUILD | 1 + include/envoy/singleton/manager.h | 1 + source/common/http/conn_manager_utility.cc | 70 ++++++++++++++----- source/common/http/conn_manager_utility.h | 33 +++++++-- source/common/http/header_utility.cc | 2 +- source/common/http/header_utility.h | 2 +- source/common/http/http2/codec_impl.h | 4 +- .../network/http_connection_manager/config.cc | 29 +------- .../network/http_connection_manager/config.h | 14 ---- source/server/http/BUILD | 1 - source/server/http/admin.cc | 16 +++-- source/server/http/admin.h | 1 + source/server/worker_impl.cc | 3 +- test/common/http/conn_manager_utility_test.cc | 45 ++++++++++++ .../http_connection_manager/config_test.cc | 44 ------------ .../stats_sinks/hystrix/hystrix_test.cc | 2 +- test/integration/BUILD | 2 +- test/integration/http_integration.cc | 8 ++- test/integration/http_integration.h | 2 + test/integration/integration.cc | 13 ++-- test/integration/integration.h | 4 ++ test/integration/integration_admin_test.cc | 6 +- test/integration/integration_admin_test.h | 8 +-- .../integration/websocket_integration_test.cc | 2 +- test/mocks/server/mocks.cc | 51 +++++++------- test/mocks/server/mocks.h | 25 +++---- test/server/http/admin_test.cc | 23 ++++-- 29 files changed, 232 insertions(+), 186 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 17a47a8c772f3..b6156d66f785a 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -4,6 +4,7 @@ Version history 1.10.0 (pending) ================ * access log: added a new flag for upstream retry count exceeded. +* admin: the admin server can now be accessed via HTTP/2 (prior knowledge). * buffer: fix vulnerabilities when allocation fails * config: removed deprecated_v1 sds_config from :ref:`Bootstrap config `. * config: removed REST_LEGACY as a valid :ref:`ApiType `. diff --git a/include/envoy/server/admin.h b/include/envoy/server/admin.h index af14490f12169..7249e65cc2bdd 100644 --- a/include/envoy/server/admin.h +++ b/include/envoy/server/admin.h @@ -39,6 +39,11 @@ class AdminStream { */ virtual Http::StreamDecoderFilterCallbacks& getDecoderFilterCallbacks() const PURE; + /** + * @return const Buffer::Instance* the fully buffered admin request if applicable. + */ + virtual const Buffer::Instance* getRequestBody() const PURE; + /** * @return Http::HeaderMap& to be used by handler to parse header information sent with the * request. diff --git a/include/envoy/singleton/BUILD b/include/envoy/singleton/BUILD index 330dc3f8905c5..23fd60851570f 100644 --- a/include/envoy/singleton/BUILD +++ b/include/envoy/singleton/BUILD @@ -18,5 +18,6 @@ envoy_cc_library( hdrs = ["manager.h"], deps = [ ":instance_interface", + "//include/envoy/registry", ], ) diff --git a/include/envoy/singleton/manager.h b/include/envoy/singleton/manager.h index 089c8cca2eb8a..9721d9318ff82 100644 --- a/include/envoy/singleton/manager.h +++ b/include/envoy/singleton/manager.h @@ -5,6 +5,7 @@ #include #include "envoy/common/pure.h" +#include "envoy/registry/registry.h" #include "envoy/singleton/instance.h" namespace Envoy { diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index 737b0eea54bd8..5cdecd35ea43b 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -8,6 +8,8 @@ #include "common/common/empty_string.h" #include "common/common/utility.h" #include "common/http/headers.h" +#include "common/http/http1/codec_impl.h" +#include "common/http/http2/codec_impl.h" #include "common/http/utility.h" #include "common/network/utility.h" #include "common/runtime/uuid_util.h" @@ -18,10 +20,40 @@ namespace Envoy { namespace Http { +std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection& connection, + const Buffer::Instance& data) { + if (!connection.nextProtocol().empty()) { + return connection.nextProtocol(); + } + + // See if the data we have so far shows the HTTP/2 prefix. We ignore the case where someone sends + // us the first few bytes of the HTTP/2 prefix since in all public cases we use SSL/ALPN. For + // internal cases this should practically never happen. + if (-1 != data.search(Http2::CLIENT_MAGIC_PREFIX.c_str(), Http2::CLIENT_MAGIC_PREFIX.size(), 0)) { + return Http2::ALPN_STRING; + } + + return ""; +} + +ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec(Network::Connection& connection, + const Buffer::Instance& data, + ServerConnectionCallbacks& callbacks, + Stats::Scope& scope, + const Http1Settings& http1_settings, + const Http2Settings& http2_settings) { + if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) { + return ServerConnectionPtr{ + new Http2::ServerConnectionImpl(connection, callbacks, scope, http2_settings)}; + } else { + return ServerConnectionPtr{ + new Http1::ServerConnectionImpl(connection, callbacks, http1_settings)}; + } +} + Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequestHeaders( - Http::HeaderMap& request_headers, Network::Connection& connection, - ConnectionManagerConfig& config, const Router::Config& route_config, - Runtime::RandomGenerator& random, Runtime::Loader& runtime, + 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. @@ -139,7 +171,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest request_headers.removeEnvoyForceTrace(); request_headers.removeEnvoyIpTags(); - for (const Http::LowerCaseString& header : route_config.internalOnlyHeaders()) { + for (const LowerCaseString& header : route_config.internalOnlyHeaders()) { request_headers.remove(header); } } @@ -184,7 +216,7 @@ Network::Address::InstanceConstSharedPtr ConnectionManagerUtility::mutateRequest return final_remote_address; } -void ConnectionManagerUtility::mutateTracingRequestHeader(Http::HeaderMap& request_headers, +void ConnectionManagerUtility::mutateTracingRequestHeader(HeaderMap& request_headers, Runtime::Loader& runtime, ConnectionManagerConfig& config) { if (!config.tracingConfig() || !request_headers.RequestId()) { @@ -221,22 +253,22 @@ void ConnectionManagerUtility::mutateTracingRequestHeader(Http::HeaderMap& reque request_headers.RequestId()->value(x_request_id); } -void ConnectionManagerUtility::mutateXfccRequestHeader(Http::HeaderMap& request_headers, +void ConnectionManagerUtility::mutateXfccRequestHeader(HeaderMap& request_headers, Network::Connection& connection, ConnectionManagerConfig& config) { // When AlwaysForwardOnly is set, always forward the XFCC header without modification. - if (config.forwardClientCert() == Http::ForwardClientCertType::AlwaysForwardOnly) { + if (config.forwardClientCert() == ForwardClientCertType::AlwaysForwardOnly) { return; } // When Sanitize is set, or the connection is not mutual TLS, remove the XFCC header. - if (config.forwardClientCert() == Http::ForwardClientCertType::Sanitize || + if (config.forwardClientCert() == ForwardClientCertType::Sanitize || !(connection.ssl() && connection.ssl()->peerCertificatePresented())) { request_headers.removeForwardedClientCert(); return; } // When ForwardOnly is set, always forward the XFCC header without modification. - if (config.forwardClientCert() == Http::ForwardClientCertType::ForwardOnly) { + if (config.forwardClientCert() == ForwardClientCertType::ForwardOnly) { return; } @@ -246,8 +278,8 @@ void ConnectionManagerUtility::mutateXfccRequestHeader(Http::HeaderMap& request_ std::vector client_cert_details; // When AppendForward or SanitizeSet is set, the client certificate information should be set into // the XFCC header. - if (config.forwardClientCert() == Http::ForwardClientCertType::AppendForward || - config.forwardClientCert() == Http::ForwardClientCertType::SanitizeSet) { + if (config.forwardClientCert() == ForwardClientCertType::AppendForward || + config.forwardClientCert() == ForwardClientCertType::SanitizeSet) { const std::string uri_san_local_cert = connection.ssl()->uriSanLocalCertificate(); if (!uri_san_local_cert.empty()) { client_cert_details.push_back("By=" + uri_san_local_cert); @@ -258,23 +290,23 @@ void ConnectionManagerUtility::mutateXfccRequestHeader(Http::HeaderMap& request_ } for (const auto& detail : config.setCurrentClientCertDetails()) { switch (detail) { - case Http::ClientCertDetailsType::Cert: { + case ClientCertDetailsType::Cert: { const std::string peer_cert = connection.ssl()->urlEncodedPemEncodedPeerCertificate(); if (!peer_cert.empty()) { client_cert_details.push_back("Cert=\"" + peer_cert + "\""); } break; } - case Http::ClientCertDetailsType::Subject: + case ClientCertDetailsType::Subject: // The "Subject" key still exists even if the subject is empty. client_cert_details.push_back("Subject=\"" + connection.ssl()->subjectPeerCertificate() + "\""); break; - case Http::ClientCertDetailsType::URI: + case ClientCertDetailsType::URI: // The "URI" key still exists even if the URI is empty. client_cert_details.push_back("URI=" + connection.ssl()->uriSanPeerCertificate()); break; - case Http::ClientCertDetailsType::DNS: { + case ClientCertDetailsType::DNS: { const std::vector dns_sans = connection.ssl()->dnsSansPeerCertificate(); if (!dns_sans.empty()) { for (const std::string& dns : dns_sans) { @@ -288,18 +320,18 @@ void ConnectionManagerUtility::mutateXfccRequestHeader(Http::HeaderMap& request_ } const std::string client_cert_details_str = absl::StrJoin(client_cert_details, ";"); - if (config.forwardClientCert() == Http::ForwardClientCertType::AppendForward) { + if (config.forwardClientCert() == ForwardClientCertType::AppendForward) { HeaderMapImpl::appendToHeader(request_headers.insertForwardedClientCert().value(), client_cert_details_str); - } else if (config.forwardClientCert() == Http::ForwardClientCertType::SanitizeSet) { + } else if (config.forwardClientCert() == ForwardClientCertType::SanitizeSet) { request_headers.insertForwardedClientCert().value(client_cert_details_str); } else { NOT_REACHED_GCOVR_EXCL_LINE; } } -void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_headers, - const Http::HeaderMap* request_headers, +void ConnectionManagerUtility::mutateResponseHeaders(HeaderMap& response_headers, + const HeaderMap* request_headers, const std::string& via) { if (request_headers != nullptr && Utility::isUpgrade(*request_headers) && Utility::isUpgrade(response_headers)) { diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index a17f7cde81a84..7ba81d8eeb52e 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -16,6 +16,28 @@ namespace Http { */ class ConnectionManagerUtility { public: + /** + * Determine the next protocol to used based both on ALPN as well as protocol inspection. + * @param connection supplies the connection to determine a protocol for. + * @param data supplies the currently available read data on the connection. + */ + static std::string determineNextProtocol(Network::Connection& connection, + const Buffer::Instance& data); + + /** + * Create an HTTP codec given the connection and the beginning of the incoming data. + * @param connection supplies the connection. + * @param data supplies the initial data supplied by the client. + * @param callbacks supplies the codec callbacks. + * @param scope supplies the stats scope for codec stats. + * @param http1_settings supplies the HTTP/1 settings to use if HTTP/1 is chosen. + * @param http2_settings supplies the HTTP/2 settings to use if HTTP/2 is chosen. + */ + static ServerConnectionPtr + autoCreateCodec(Network::Connection& connection, const Buffer::Instance& data, + ServerConnectionCallbacks& callbacks, Stats::Scope& scope, + const Http1Settings& http1_settings, const Http2Settings& http2_settings); + /** * Mutates request headers in various ways. This functionality is broken out because of its * complexity for ease of testing. See the method itself for detailed comments on what @@ -28,23 +50,22 @@ 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, Network::Connection& connection, + mutateRequestHeaders(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); + static void mutateResponseHeaders(HeaderMap& response_headers, const HeaderMap* request_headers, + const std::string& via); private: /** * Mutate request headers if request needs to be traced. */ - static void mutateTracingRequestHeader(Http::HeaderMap& request_headers, Runtime::Loader& runtime, + static void mutateTracingRequestHeader(HeaderMap& request_headers, Runtime::Loader& runtime, ConnectionManagerConfig& config); - static void mutateXfccRequestHeader(Http::HeaderMap& request_headers, - Network::Connection& connection, + static void mutateXfccRequestHeader(HeaderMap& request_headers, Network::Connection& connection, ConnectionManagerConfig& config); }; diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index b579ba10b2bd9..1bb3d4d77c4b1 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -66,7 +66,7 @@ HeaderUtility::HeaderData::HeaderData(const Json::Object& config) bool HeaderUtility::matchHeaders(const Http::HeaderMap& request_headers, const std::vector& config_headers) { - // TODO (rodaine): Should this really allow empty headers to always match? + // No headers to match is considered a match. if (!config_headers.empty()) { for (const HeaderData& cfg_header_data : config_headers) { if (!matchHeaders(request_headers, cfg_header_data)) { diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index ec061ee4fa76f..a2af114bcd84a 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -38,7 +38,7 @@ class HeaderUtility { * @param request_headers supplies the headers from the request. * @param config_headers supplies the list of configured header conditions on which to match. * @return bool true if all the headers (and values) in the config_headers are found in the - * request_headers + * request_headers. If no config_headers are specified, returns true. */ static bool matchHeaders(const Http::HeaderMap& request_headers, const std::vector& config_headers); diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 16d7139c30cad..663f8f55965a2 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -11,17 +11,15 @@ #include "envoy/network/connection.h" #include "envoy/stats/scope.h" -//#include "envoy/stats/stats_macros.h" - #include "common/buffer/buffer_impl.h" #include "common/buffer/watermark_buffer.h" #include "common/common/linked_object.h" #include "common/common/logger.h" #include "common/http/codec_helper.h" #include "common/http/header_map_impl.h" -#include "common/http/utility.h" #include "common/http/http2/metadata_decoder.h" #include "common/http/http2/metadata_encoder.h" +#include "common/http/utility.h" #include "absl/types/optional.h" #include "nghttp2/nghttp2.h" diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 6dd5ef35b8788..f9faf22d2ccfd 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -14,6 +14,7 @@ #include "common/common/fmt.h" #include "common/config/filter_json.h" #include "common/config/utility.h" +#include "common/http/conn_manager_utility.h" #include "common/http/date_provider_impl.h" #include "common/http/default_server_string.h" #include "common/http/http1/codec_impl.h" @@ -117,24 +118,6 @@ static Registry::RegisterFactory registered_; -std::string -HttpConnectionManagerConfigUtility::determineNextProtocol(Network::Connection& connection, - const Buffer::Instance& data) { - if (!connection.nextProtocol().empty()) { - return connection.nextProtocol(); - } - - // See if the data we have so far shows the HTTP/2 prefix. We ignore the case where someone sends - // us the first few bytes of the HTTP/2 prefix since in all public cases we use SSL/ALPN. For - // internal cases this should practically never happen. - if (-1 != data.search(Http::Http2::CLIENT_MAGIC_PREFIX.c_str(), - Http::Http2::CLIENT_MAGIC_PREFIX.size(), 0)) { - return Http::Http2::ALPN_STRING; - } - - return ""; -} - InternalAddressConfig::InternalAddressConfig( const envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager:: InternalAddressConfig& config) @@ -344,14 +327,8 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl( connection, callbacks, context_.scope(), http2_settings_)}; case CodecType::AUTO: - if (HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data) == - Http::Http2::ALPN_STRING) { - return Http::ServerConnectionPtr{new Http::Http2::ServerConnectionImpl( - connection, callbacks, context_.scope(), http2_settings_)}; - } else { - return Http::ServerConnectionPtr{ - new Http::Http1::ServerConnectionImpl(connection, callbacks, http1_settings_)}; - } + return Http::ConnectionManagerUtility::autoCreateCodec( + connection, data, callbacks, context_.scope(), http1_settings_, http2_settings_); } NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 917b2ee3b99a5..43172d7802e3a 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -46,20 +46,6 @@ class HttpConnectionManagerFilterConfigFactory Server::Configuration::FactoryContext& context) override; }; -/** - * Utilities for the HTTP connection manager that facilitate testing. - */ -class HttpConnectionManagerConfigUtility { -public: - /** - * Determine the next protocol to used based both on ALPN as well as protocol inspection. - * @param connection supplies the connection to determine a protocol for. - * @param data supplies the currently available read data on the connection. - */ - static std::string determineNextProtocol(Network::Connection& connection, - const Buffer::Instance& data); -}; - /** * Determines if an address is internal based on user provided config. */ diff --git a/source/server/http/BUILD b/source/server/http/BUILD index 1ccaa0bf106d0..b6761f2865857 100644 --- a/source/server/http/BUILD +++ b/source/server/http/BUILD @@ -47,7 +47,6 @@ envoy_cc_library( "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/http:utility_lib", - "//source/common/http/http1:codec_lib", "//source/common/memory:stats_lib", "//source/common/network:listen_socket_lib", "//source/common/network:raw_buffer_socket_lib", diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 4eba0592cf020..759c5d826ffc3 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -37,9 +37,9 @@ #include "common/common/version.h" #include "common/html/utility.h" #include "common/http/codes.h" +#include "common/http/conn_manager_utility.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" -#include "common/http/http1/codec_impl.h" #include "common/json/json_loader.h" #include "common/memory/stats.h" #include "common/network/listen_socket_impl.h" @@ -192,7 +192,11 @@ Http::FilterHeadersStatus AdminFilter::decodeHeaders(Http::HeaderMap& headers, b return Http::FilterHeadersStatus::StopIteration; } -Http::FilterDataStatus AdminFilter::decodeData(Buffer::Instance&, bool end_stream) { +Http::FilterDataStatus AdminFilter::decodeData(Buffer::Instance& data, bool end_stream) { + // Currently we generically buffer all admin request data in case a handler wants to use it. + // If we ever support streaming admin requests we may need to revisit this. + callbacks_->addDecodedData(data, false); + if (end_stream) { onComplete(); } @@ -220,6 +224,8 @@ Http::StreamDecoderFilterCallbacks& AdminFilter::getDecoderFilterCallbacks() con return *callbacks_; } +const Buffer::Instance* AdminFilter::getRequestBody() const { return callbacks_->decodingBuffer(); } + const Http::HeaderMap& AdminFilter::getRequestHeaders() const { ASSERT(request_headers_ != nullptr); return *request_headers_; @@ -1048,10 +1054,10 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server) admin_filter_chain_(std::make_shared()) {} Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection, - const Buffer::Instance&, + const Buffer::Instance& data, Http::ServerConnectionCallbacks& callbacks) { - return Http::ServerConnectionPtr{ - new Http::Http1::ServerConnectionImpl(connection, callbacks, Http::Http1Settings())}; + return Http::ConnectionManagerUtility::autoCreateCodec( + connection, data, callbacks, server_.stats(), Http::Http1Settings(), Http::Http2Settings()); } bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 979be5a9c718e..ed567c31c7693 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -340,6 +340,7 @@ class AdminFilter : public Http::StreamDecoderFilter, void setEndStreamOnComplete(bool end_stream) override { end_stream_on_complete_ = end_stream; } void addOnDestroyCallback(std::function cb) override; Http::StreamDecoderFilterCallbacks& getDecoderFilterCallbacks() const override; + const Buffer::Instance* getRequestBody() const override; const Http::HeaderMap& getRequestHeaders() const override; private: diff --git a/source/server/worker_impl.cc b/source/server/worker_impl.cc index a7ec175454055..3f4c78aa751dd 100644 --- a/source/server/worker_impl.cc +++ b/source/server/worker_impl.cc @@ -105,8 +105,7 @@ void WorkerImpl::threadRoutine(GuardDog& guard_dog) { // We must close all active connections before we actually exit the thread. This prevents any // destructors from running on the main thread which might reference thread locals. Destroying - // the handler does this as well as destroying the dispatcher which purges the delayed deletion - // list. + // the handler does this which additionally purges the dispatcher delayed deletion list. handler_.reset(); tls_.shutdownThread(); watchdog.reset(); diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 06983d561b2f8..df63b2d661849 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -127,6 +127,51 @@ class ConnectionManagerUtilityTest : public testing::Test { std::string via_; }; +// Tests for ConnectionManagerUtility::determineNextProtocol. +TEST_F(ConnectionManagerUtilityTest, DetermineNextProtocol) { + { + Network::MockConnection connection; + EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("hello")); + Buffer::OwnedImpl data(""); + EXPECT_EQ("hello", ConnectionManagerUtility::determineNextProtocol(connection, data)); + } + + { + Network::MockConnection connection; + EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); + Buffer::OwnedImpl data(""); + EXPECT_EQ("", ConnectionManagerUtility::determineNextProtocol(connection, data)); + } + + { + Network::MockConnection connection; + EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); + Buffer::OwnedImpl data("GET / HTTP/1.1"); + EXPECT_EQ("", ConnectionManagerUtility::determineNextProtocol(connection, data)); + } + + { + Network::MockConnection connection; + EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); + Buffer::OwnedImpl data("PRI * HTTP/2.0\r\n"); + EXPECT_EQ("h2", ConnectionManagerUtility::determineNextProtocol(connection, data)); + } + + { + Network::MockConnection connection; + EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); + Buffer::OwnedImpl data("PRI * HTTP/2"); + EXPECT_EQ("h2", ConnectionManagerUtility::determineNextProtocol(connection, data)); + } + + { + Network::MockConnection connection; + EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); + Buffer::OwnedImpl data("PRI * HTTP/"); + EXPECT_EQ("", ConnectionManagerUtility::determineNextProtocol(connection, data)); + } +} + // Verify external request and XFF is set when we are using remote address and the address is // external. TEST_F(ConnectionManagerUtilityTest, UseRemoteAddressWhenNotLocalHostRemoteAddress) { diff --git a/test/extensions/filters/network/http_connection_manager/config_test.cc b/test/extensions/filters/network/http_connection_manager/config_test.cc index 9a6cc70d30da0..aec4be3bf1941 100644 --- a/test/extensions/filters/network/http_connection_manager/config_test.cc +++ b/test/extensions/filters/network/http_connection_manager/config_test.cc @@ -246,50 +246,6 @@ TEST_F(HttpConnectionManagerConfigTest, SingleDateProvider) { Network::FilterFactoryCb cb2 = factory.createFilterFactory(*json_config, context_); } -TEST(HttpConnectionManagerConfigUtilityTest, DetermineNextProtocol) { - { - Network::MockConnection connection; - EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("hello")); - Buffer::OwnedImpl data(""); - EXPECT_EQ("hello", HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data)); - } - - { - Network::MockConnection connection; - EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); - Buffer::OwnedImpl data(""); - EXPECT_EQ("", HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data)); - } - - { - Network::MockConnection connection; - EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); - Buffer::OwnedImpl data("GET / HTTP/1.1"); - EXPECT_EQ("", HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data)); - } - - { - Network::MockConnection connection; - EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); - Buffer::OwnedImpl data("PRI * HTTP/2.0\r\n"); - EXPECT_EQ("h2", HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data)); - } - - { - Network::MockConnection connection; - EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); - Buffer::OwnedImpl data("PRI * HTTP/2"); - EXPECT_EQ("h2", HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data)); - } - - { - Network::MockConnection connection; - EXPECT_CALL(connection, nextProtocol()).WillRepeatedly(Return("")); - Buffer::OwnedImpl data("PRI * HTTP/"); - EXPECT_EQ("", HttpConnectionManagerConfigUtility::determineNextProtocol(connection, data)); - } -} - TEST_F(HttpConnectionManagerConfigTest, BadHttpConnectionMangerConfig) { std::string json_string = R"EOF( { diff --git a/test/extensions/stats_sinks/hystrix/hystrix_test.cc b/test/extensions/stats_sinks/hystrix/hystrix_test.cc index acf9754dd5ab7..baf24727a0b31 100644 --- a/test/extensions/stats_sinks/hystrix/hystrix_test.cc +++ b/test/extensions/stats_sinks/hystrix/hystrix_test.cc @@ -519,7 +519,7 @@ TEST_F(HystrixSinkTest, HystrixEventStreamHandler) { Http::HeaderMapImpl response_headers; - NiceMock admin_stream_mock; + NiceMock admin_stream_mock; NiceMock connection_mock; auto addr_instance_ = Envoy::Network::Utility::parseInternetAddress("2.3.4.5", 123, false); diff --git a/test/integration/BUILD b/test/integration/BUILD index 2e793df5535b0..5957b9d3c7e71 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -160,7 +160,7 @@ envoy_cc_test( "integration_admin_test.h", ], deps = [ - ":http_integration_lib", + ":http_protocol_integration_lib", "//include/envoy/http:header_map_interface", "//source/common/stats:stats_matcher_lib", "//source/extensions/filters/http/buffer:config", diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 5d2017c02f5ab..5c87937619e32 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -103,11 +103,17 @@ IntegrationCodecClient::makeHeaderOnlyRequest(const Http::HeaderMap& headers) { IntegrationStreamDecoderPtr IntegrationCodecClient::makeRequestWithBody(const Http::HeaderMap& headers, uint64_t body_size) { + return makeRequestWithBody(headers, std::string(body_size, 'a')); +} + +IntegrationStreamDecoderPtr +IntegrationCodecClient::makeRequestWithBody(const Http::HeaderMap& headers, + const std::string& body) { auto response = std::make_unique(dispatcher_); Http::StreamEncoder& encoder = newStream(*response); encoder.getStream().addCallbacks(*response); encoder.encodeHeaders(headers, false); - Buffer::OwnedImpl data(std::string(body_size, 'a')); + Buffer::OwnedImpl data(body); encoder.encodeData(data, true); flushWrite(); return response; diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 1d5357bbd8289..0c06732832fbd 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -25,6 +25,8 @@ class IntegrationCodecClient : public Http::CodecClientProd { IntegrationStreamDecoderPtr makeHeaderOnlyRequest(const Http::HeaderMap& headers); IntegrationStreamDecoderPtr makeRequestWithBody(const Http::HeaderMap& headers, uint64_t body_size); + IntegrationStreamDecoderPtr makeRequestWithBody(const Http::HeaderMap& headers, + const std::string& body); bool sawGoAway() const { return saw_goaway_; } bool connected() const { return connected_; } void sendData(Http::StreamEncoder& encoder, absl::string_view data, bool end_stream); diff --git a/test/integration/integration.cc b/test/integration/integration.cc index 94bea9dcd7b8c..9488b3f8af5a1 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -57,7 +57,11 @@ void IntegrationStreamDecoder::waitForHeaders() { void IntegrationStreamDecoder::waitForBodyData(uint64_t size) { ASSERT(body_data_waiting_length_ == 0); body_data_waiting_length_ = size; - dispatcher_.run(Event::Dispatcher::RunType::Block); + body_data_waiting_length_ -= + std::min(body_data_waiting_length_, static_cast(body_.size())); + if (body_data_waiting_length_ > 0) { + dispatcher_.run(Event::Dispatcher::RunType::Block); + } } void IntegrationStreamDecoder::waitForEndStream() { @@ -91,12 +95,7 @@ void IntegrationStreamDecoder::decodeHeaders(Http::HeaderMapPtr&& headers, bool void IntegrationStreamDecoder::decodeData(Buffer::Instance& data, bool end_stream) { saw_end_stream_ = end_stream; - uint64_t num_slices = data.getRawSlices(nullptr, 0); - STACK_ARRAY(slices, Buffer::RawSlice, num_slices); - data.getRawSlices(slices.begin(), num_slices); - for (const Buffer::RawSlice& slice : slices) { - body_.append(static_cast(slice.mem_), slice.len_); - } + body_ += data.toString(); if (end_stream && waiting_for_end_stream_) { dispatcher_.exit(); diff --git a/test/integration/integration.h b/test/integration/integration.h index 85d7c93d39acb..7150d2e74d37f 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -37,9 +37,13 @@ class IntegrationStreamDecoder : public Http::StreamDecoder, public Http::Stream const Http::MetadataMap& metadata_map() { return *metadata_map_; } void waitForContinueHeaders(); void waitForHeaders(); + // This function waits until body_ has at least size bytes in it (it might have more). clearBody() + // can be used if the previous body data is not relevant and the test wants to wait for a specific + // amount of new data without considering the existing body size. void waitForBodyData(uint64_t size); void waitForEndStream(); void waitForReset(); + void clearBody() { body_.clear(); } // Http::StreamDecoder void decode100ContinueHeaders(Http::HeaderMapPtr&& headers) override; diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 969b65d5dfca1..a44394e30e8c5 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -16,9 +16,9 @@ namespace Envoy { -INSTANTIATE_TEST_CASE_P(IpVersions, IntegrationAdminTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - TestUtility::ipTestParamsToString); +INSTANTIATE_TEST_CASE_P(Protocols, IntegrationAdminTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); TEST_P(IntegrationAdminTest, HealthCheck) { initialize(); diff --git a/test/integration/integration_admin_test.h b/test/integration/integration_admin_test.h index 3a0487d7d60e2..0bc34fbe72d46 100644 --- a/test/integration/integration_admin_test.h +++ b/test/integration/integration_admin_test.h @@ -2,18 +2,14 @@ #include "common/json/json_loader.h" -#include "test/integration/http_integration.h" +#include "test/integration/http_protocol_integration.h" #include "gtest/gtest.h" namespace Envoy { -class IntegrationAdminTest : public HttpIntegrationTest, - public testing::TestWithParam { +class IntegrationAdminTest : public HttpProtocolIntegrationTest { public: - IntegrationAdminTest() - : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam(), realTime()) {} - void initialize() override { config_helper_.addFilter(ConfigHelper::DEFAULT_HEALTH_CHECK_FILTER); HttpIntegrationTest::initialize(); diff --git a/test/integration/websocket_integration_test.cc b/test/integration/websocket_integration_test.cc index 1ce08d0376d19..2be39ceb339e0 100644 --- a/test/integration/websocket_integration_test.cc +++ b/test/integration/websocket_integration_test.cc @@ -444,7 +444,7 @@ TEST_P(WebsocketIntegrationTest, BidirectionalChunkedData) { codec_client_->sendData(*request_encoder_, "FinalClientPayload", false); ASSERT_TRUE(upstream_request_->waitForData(*dispatcher_, request_payload + "FinalClientPayload")); upstream_request_->encodeData("FinalServerPayload", false); - response_->waitForBodyData(5); + response_->waitForBodyData(response_->body().size() + 5); EXPECT_EQ(response_payload + "FinalServerPayload", response_->body()); // Clean up. diff --git a/test/mocks/server/mocks.cc b/test/mocks/server/mocks.cc index 39f365d471fe6..617080c63d79a 100644 --- a/test/mocks/server/mocks.cc +++ b/test/mocks/server/mocks.cc @@ -39,7 +39,7 @@ MockOptions::MockOptions(const std::string& config_path) : config_path_(config_p return std::make_unique(); })); } -MockOptions::~MockOptions() {} +MockOptions::~MockOptions() = default; MockConfigTracker::MockConfigTracker() { ON_CALL(*this, add_(_, _)) @@ -49,37 +49,40 @@ MockConfigTracker::MockConfigTracker() { return new MockEntryOwner(); })); } -MockConfigTracker::~MockConfigTracker() {} +MockConfigTracker::~MockConfigTracker() = default; MockAdmin::MockAdmin() { ON_CALL(*this, getConfigTracker()).WillByDefault(testing::ReturnRef(config_tracker_)); } -MockAdmin::~MockAdmin() {} +MockAdmin::~MockAdmin() = default; + +MockAdminStream::MockAdminStream() = default; +MockAdminStream::~MockAdminStream() = default; MockDrainManager::MockDrainManager() { ON_CALL(*this, startDrainSequence(_)).WillByDefault(SaveArg<0>(&drain_sequence_completion_)); } -MockDrainManager::~MockDrainManager() {} +MockDrainManager::~MockDrainManager() = default; -MockWatchDog::MockWatchDog() {} -MockWatchDog::~MockWatchDog() {} +MockWatchDog::MockWatchDog() = default; +MockWatchDog::~MockWatchDog() = default; MockGuardDog::MockGuardDog() : watch_dog_(new NiceMock()) { ON_CALL(*this, createWatchDog(_)).WillByDefault(Return(watch_dog_)); } -MockGuardDog::~MockGuardDog() {} +MockGuardDog::~MockGuardDog() = default; MockHotRestart::MockHotRestart() { ON_CALL(*this, logLock()).WillByDefault(ReturnRef(log_lock_)); ON_CALL(*this, accessLogLock()).WillByDefault(ReturnRef(access_log_lock_)); ON_CALL(*this, statsAllocator()).WillByDefault(ReturnRef(stats_allocator_)); } -MockHotRestart::~MockHotRestart() {} +MockHotRestart::~MockHotRestart() = default; MockOverloadManager::MockOverloadManager() { ON_CALL(*this, getThreadLocalOverloadState()).WillByDefault(ReturnRef(overload_state_)); } -MockOverloadManager::~MockOverloadManager() {} +MockOverloadManager::~MockOverloadManager() = default; MockListenerComponentFactory::MockListenerComponentFactory() : socket_(std::make_shared>()) { @@ -94,13 +97,13 @@ MockListenerComponentFactory::MockListenerComponentFactory() return socket_; })); } -MockListenerComponentFactory::~MockListenerComponentFactory() {} +MockListenerComponentFactory::~MockListenerComponentFactory() = default; -MockListenerManager::MockListenerManager() {} -MockListenerManager::~MockListenerManager() {} +MockListenerManager::MockListenerManager() = default; +MockListenerManager::~MockListenerManager() = default; -MockWorkerFactory::MockWorkerFactory() {} -MockWorkerFactory::~MockWorkerFactory() {} +MockWorkerFactory::MockWorkerFactory() = default; +MockWorkerFactory::~MockWorkerFactory() = default; MockWorker::MockWorker() { ON_CALL(*this, addListener(_, _)) @@ -117,7 +120,7 @@ MockWorker::MockWorker() { remove_listener_completion_ = completion; })); } -MockWorker::~MockWorker() {} +MockWorker::~MockWorker() = default; MockInstance::MockInstance() : secret_manager_(new Secret::SecretManagerImpl()), cluster_manager_(timeSystem()), @@ -147,7 +150,7 @@ MockInstance::MockInstance() // ON_CALL(*this, timeSystem()).WillByDefault(ReturnRef(test_time_.timeSystem()));; } -MockInstance::~MockInstance() {} +MockInstance::~MockInstance() = default; namespace Configuration { @@ -159,7 +162,7 @@ MockMain::MockMain(int wd_miss, int wd_megamiss, int wd_kill, int wd_multikill) ON_CALL(*this, wdMultiKillTimeout()).WillByDefault(Return(wd_multikill_)); } -MockMain::~MockMain() {} +MockMain::~MockMain() = default; MockFactoryContext::MockFactoryContext() : singleton_manager_( @@ -181,16 +184,15 @@ MockFactoryContext::MockFactoryContext() ON_CALL(*this, overloadManager()).WillByDefault(ReturnRef(overload_manager_)); } -MockFactoryContext::~MockFactoryContext() {} +MockFactoryContext::~MockFactoryContext() = default; MockTransportSocketFactoryContext::MockTransportSocketFactoryContext() : secret_manager_(new Secret::SecretManagerImpl()) {} -MockTransportSocketFactoryContext::~MockTransportSocketFactoryContext() {} - -MockListenerFactoryContext::MockListenerFactoryContext() {} +MockTransportSocketFactoryContext::~MockTransportSocketFactoryContext() = default; -MockListenerFactoryContext::~MockListenerFactoryContext() {} +MockListenerFactoryContext::MockListenerFactoryContext() = default; +MockListenerFactoryContext::~MockListenerFactoryContext() = default; MockHealthCheckerFactoryContext::MockHealthCheckerFactoryContext() { event_logger_ = new NiceMock(); @@ -201,10 +203,7 @@ MockHealthCheckerFactoryContext::MockHealthCheckerFactoryContext() { ON_CALL(*this, eventLogger_()).WillByDefault(Return(event_logger_)); } -MockHealthCheckerFactoryContext::~MockHealthCheckerFactoryContext() {} - -MockAdminStream::MockAdminStream() {} -MockAdminStream::~MockAdminStream() {} +MockHealthCheckerFactoryContext::~MockHealthCheckerFactoryContext() = default; } // namespace Configuration } // namespace Server diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index b2f8cdf4c8564..9b62a9e9f5fb5 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -138,6 +138,19 @@ class MockAdmin : public Admin { NiceMock config_tracker_; }; +class MockAdminStream : public AdminStream { +public: + MockAdminStream(); + ~MockAdminStream(); + + MOCK_METHOD1(setEndStreamOnComplete, void(bool)); + MOCK_METHOD1(addOnDestroyCallback, void(std::function)); + MOCK_CONST_METHOD0(getRequestBody, const Buffer::Instance*()); + MOCK_CONST_METHOD0(getRequestHeaders, Http::HeaderMap&()); + MOCK_CONST_METHOD0(getDecoderFilterCallbacks, + NiceMock&()); +}; + class MockDrainManager : public DrainManager { public: MockDrainManager(); @@ -500,18 +513,6 @@ class MockHealthCheckerFactoryContext : public virtual HealthCheckerFactoryConte testing::NiceMock* event_logger_{}; }; -class MockAdminStream : public AdminStream { -public: - MockAdminStream(); - ~MockAdminStream(); - - MOCK_METHOD1(setEndStreamOnComplete, void(bool)); - MOCK_METHOD1(addOnDestroyCallback, void(std::function)); - MOCK_CONST_METHOD0(getRequestHeaders, Http::HeaderMap&()); - MOCK_CONST_METHOD0(getDecoderFilterCallbacks, - NiceMock&()); -}; - } // namespace Configuration } // namespace Server } // namespace Envoy diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 631c610a697dd..8545bf691e8e4 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -558,22 +558,33 @@ INSTANTIATE_TEST_CASE_P(IpVersions, AdminFilterTest, TEST_P(AdminFilterTest, HeaderOnly) { EXPECT_CALL(callbacks_, encodeHeaders_(_, false)); - filter_.decodeHeaders(request_headers_, true); + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.decodeHeaders(request_headers_, true)); } TEST_P(AdminFilterTest, Body) { - filter_.decodeHeaders(request_headers_, false); + InSequence s; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.decodeHeaders(request_headers_, false)); Buffer::OwnedImpl data("hello"); + EXPECT_CALL(callbacks_, addDecodedData(_, false)); EXPECT_CALL(callbacks_, encodeHeaders_(_, false)); - filter_.decodeData(data, true); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.decodeData(data, true)); } TEST_P(AdminFilterTest, Trailers) { - filter_.decodeHeaders(request_headers_, false); + InSequence s; + + EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, + filter_.decodeHeaders(request_headers_, false)); Buffer::OwnedImpl data("hello"); - filter_.decodeData(data, false); + EXPECT_CALL(callbacks_, addDecodedData(_, false)); + EXPECT_EQ(Http::FilterDataStatus::StopIterationNoBuffer, filter_.decodeData(data, false)); + EXPECT_CALL(callbacks_, decodingBuffer()); + filter_.getRequestBody(); EXPECT_CALL(callbacks_, encodeHeaders_(_, false)); - filter_.decodeTrailers(request_headers_); + EXPECT_EQ(Http::FilterTrailersStatus::StopIteration, filter_.decodeTrailers(request_headers_)); } class AdminInstanceTest : public testing::TestWithParam { From ead9cc33770c8225baaf8d61c886b35e3ee78001 Mon Sep 17 00:00:00 2001 From: Matt Klein Date: Wed, 9 Jan 2019 13:29:05 -0700 Subject: [PATCH 2/2] comments Signed-off-by: Matt Klein --- source/server/http/admin.cc | 4 +++- test/integration/integration_admin_test.cc | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 759c5d826ffc3..86917af88028d 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -194,7 +194,9 @@ Http::FilterHeadersStatus AdminFilter::decodeHeaders(Http::HeaderMap& headers, b Http::FilterDataStatus AdminFilter::decodeData(Buffer::Instance& data, bool end_stream) { // Currently we generically buffer all admin request data in case a handler wants to use it. - // If we ever support streaming admin requests we may need to revisit this. + // If we ever support streaming admin requests we may need to revisit this. Note, we must use + // addDecodedData() here since we might need to perform onComplete() processing if end_stream is + // true. callbacks_->addDecodedData(data, false); if (end_stream) { diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index a44394e30e8c5..c2f9930214d24 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -17,7 +17,9 @@ namespace Envoy { INSTANTIATE_TEST_CASE_P(Protocols, IntegrationAdminTest, - testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams( + {Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2}, + {FakeHttpConnection::Type::HTTP1})), HttpProtocolIntegrationTest::protocolTestParamsToString); TEST_P(IntegrationAdminTest, HealthCheck) {