diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 9ae9bae8c67fe..21d36b1e9e5c8 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..86917af88028d 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,13 @@ 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. 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) { onComplete(); } @@ -220,6 +226,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 +1056,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 8770cd24b7d2d..2a2e27ed07c42 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..c2f9930214d24 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -16,9 +16,11 @@ namespace Envoy { -INSTANTIATE_TEST_CASE_P(IpVersions, IntegrationAdminTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - TestUtility::ipTestParamsToString); +INSTANTIATE_TEST_CASE_P(Protocols, IntegrationAdminTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams( + {Http::CodecClient::Type::HTTP1, Http::CodecClient::Type::HTTP2}, + {FakeHttpConnection::Type::HTTP1})), + 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 {