diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index f3b9a41286e0..61ca6df6f7b5 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -46,6 +46,7 @@ message HttpProtocolOptions { google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; } +// [#next-free-field: 6] message Http1ProtocolOptions { message HeaderKeyFormat { message ProperCaseWords { @@ -83,6 +84,17 @@ message Http1ProtocolOptions { // Describes how the keys for response headers should be formatted. By default, all header keys // are lower cased. HeaderKeyFormat header_key_format = 4; + + // Enables trailers for HTTP/1. By default the HTTP/1 codec drops proxied trailers. + // + // .. attention:: + // + // Note that this only happens when Envoy is chunk encoding which occurs when: + // - The request is HTTP/1.1. + // - Is neither a HEAD only request nor a HTTP Upgrade. + // - Not a response to a HEAD request. + // - The content length header is not present. + bool enable_trailers = 5; } // [#next-free-field: 13] diff --git a/api/envoy/api/v3alpha/core/protocol.proto b/api/envoy/api/v3alpha/core/protocol.proto index 77ad860b237a..02dce6011225 100644 --- a/api/envoy/api/v3alpha/core/protocol.proto +++ b/api/envoy/api/v3alpha/core/protocol.proto @@ -53,6 +53,7 @@ message HttpProtocolOptions { google.protobuf.UInt32Value max_headers_count = 2 [(validate.rules).uint32 = {gte: 1}]; } +// [#next-free-field: 6] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http1ProtocolOptions"; @@ -98,6 +99,17 @@ message Http1ProtocolOptions { // Describes how the keys for response headers should be formatted. By default, all header keys // are lower cased. HeaderKeyFormat header_key_format = 4; + + // Enables trailers for HTTP/1. By default the HTTP/1 codec drops proxied trailers. + // + // .. attention:: + // + // Note that this only happens when Envoy is chunk encoding which occurs when: + // - The request is HTTP/1.1. + // - Is neither a HEAD only request nor a HTTP Upgrade. + // - Not a response to a HEAD request. + // - The content length header is not present. + bool enable_trailers = 5; } // [#next-free-field: 13] diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index e44b84c407d6..2af8168ea0ad 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -12,6 +12,7 @@ Version history * decompressor: remove decompressor hard assert failure and replace with an error flag. * ext_authz: added :ref:`configurable ability` to send the :ref:`certificate` to the `ext_authz` service. * health check: gRPC health checker sets the gRPC deadline to the configured timeout duration. +* http: added support for http1 trailers. To enable use :ref:`enable_trailers `. * http: added the ability to sanitize headers nominated by the Connection header. This new behavior is guarded by envoy.reloadable_features.connection_header_sanitization which defaults to true. * http: blocks unsupported transfer-encodings. Can be reverted temporarily by setting runtime feature `envoy.reloadable_features.reject_unsupported_transfer_encodings` to false. * http: support :ref:`auto_host_rewrite_header` in the dynamic forward proxy. diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 2a6250c5b63f..ca092f037434 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -238,6 +238,12 @@ struct Http1Settings { bool accept_http_10_{false}; // Set a default host if no Host: header is present for HTTP/1.0 requests.` std::string default_host_for_http_10_; + // Encode trailers in Http. By default the HTTP/1 codec drops proxied trailers. + // Note that this only happens when Envoy is chunk encoding which occurs when: + // - The request is HTTP/1.1 + // - Is neither a HEAD only request nor a HTTP Upgrade + // - Not a HEAD request + bool enable_trailers_{false}; enum class HeaderKeyFormat { // By default no formatting is performed, presenting all headers in lowercase (as Envoy diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 01a343d83e2d..eed4f72605e3 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -33,7 +33,13 @@ struct Http1ResponseCodeDetailValues { const absl::string_view InvalidUrl = "http1.invalid_url"; }; +struct Http1HeaderTypesValues { + const absl::string_view Headers = "headers"; + const absl::string_view Trailers = "trailers"; +}; + using Http1ResponseCodeDetails = ConstSingleton; +using Http1HeaderTypes = ConstSingleton; const StringUtil::CaseUnorderedSet& caseUnorderdSetContainingUpgradeAndHttp2Settings() { CONSTRUCT_ON_FIRST_USE(StringUtil::CaseUnorderedSet, @@ -51,7 +57,8 @@ HeaderKeyFormatterPtr formatter(const Http::Http1Settings& settings) { } // namespace const std::string StreamEncoderImpl::CRLF = "\r\n"; -const std::string StreamEncoderImpl::LAST_CHUNK = "0\r\n\r\n"; +// Last chunk as defined here https://tools.ietf.org/html/rfc7230#section-4.1 +const std::string StreamEncoderImpl::LAST_CHUNK = "0\r\n"; StreamEncoderImpl::StreamEncoderImpl(ConnectionImpl& connection, HeaderKeyFormatter* header_key_formatter) @@ -200,7 +207,31 @@ void StreamEncoderImpl::encodeData(Buffer::Instance& data, bool end_stream) { } } -void StreamEncoderImpl::encodeTrailers(const HeaderMap&) { endEncode(); } +void StreamEncoderImpl::encodeTrailers(const HeaderMap& trailers) { + if (!connection_.enableTrailers()) { + return endEncode(); + } + // Trailers only matter if it is a chunk transfer encoding + // https://tools.ietf.org/html/rfc7230#section-4.4 + if (chunk_encoding_) { + // Finalize the body + connection_.buffer().add(LAST_CHUNK); + + trailers.iterate( + [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { + static_cast(context)->encodeFormattedHeader( + header.key().getStringView(), header.value().getStringView()); + return HeaderMap::Iterate::Continue; + }, + this); + + connection_.flushOutput(); + connection_.buffer().add(CRLF); + } + + connection_.flushOutput(); + connection_.onEncodeComplete(); +} void StreamEncoderImpl::encodeMetadata(const MetadataMapVector&) { connection_.stats().metadata_not_supported_error_.inc(); @@ -209,6 +240,7 @@ void StreamEncoderImpl::encodeMetadata(const MetadataMapVector&) { void StreamEncoderImpl::endEncode() { if (chunk_encoding_) { connection_.buffer().add(LAST_CHUNK); + connection_.buffer().add(CRLF); } connection_.flushOutput(); @@ -366,13 +398,15 @@ const ToLowerTable& ConnectionImpl::toLowerTable() { ConnectionImpl::ConnectionImpl(Network::Connection& connection, Stats::Scope& stats, http_parser_type type, uint32_t max_headers_kb, const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter) + HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers) : connection_(connection), stats_{ALL_HTTP1_CODEC_STATS(POOL_COUNTER_PREFIX(stats, "http1."))}, - header_key_formatter_(std::move(header_key_formatter)), handling_upgrade_(false), - reset_stream_called_(false), strict_header_validation_(Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.strict_header_validation")), + header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), + handling_upgrade_(false), reset_stream_called_(false), + strict_header_validation_( + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation")), connection_header_sanitization_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.connection_header_sanitization")), + enable_trailers_(enable_trailers), output_buffer_([&]() -> void { this->onBelowLowWatermark(); }, [&]() -> void { this->onAboveHighWatermark(); }), max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { @@ -384,16 +418,20 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, Stats::Scope& st void ConnectionImpl::completeLastHeader() { ENVOY_CONN_LOG(trace, "completed header: key={} value={}", connection_, current_header_field_.getStringView(), current_header_value_.getStringView()); + if (!current_header_field_.empty()) { toLowerTable().toLowerCase(current_header_field_.buffer(), current_header_field_.size()); current_header_map_->addViaMove(std::move(current_header_field_), std::move(current_header_value_)); } + // Check if the number of headers exceeds the limit. if (current_header_map_->size() > max_headers_count_) { error_code_ = Http::Code::RequestHeaderFieldsTooLarge; sendProtocolError(Http1ResponseCodeDetails::get().TooManyHeaders); - throw CodecProtocolException("headers size exceeds limit"); + const absl::string_view header_type = + processing_trailers_ ? Http1HeaderTypes::get().Trailers : Http1HeaderTypes::get().Headers; + throw CodecProtocolException(absl::StrCat(header_type, " size exceeds limit")); } header_parsing_state_ = HeaderParsingState::Field; @@ -462,11 +500,16 @@ size_t ConnectionImpl::dispatchSlice(const char* slice, size_t len) { } void ConnectionImpl::onHeaderField(const char* data, size_t length) { + // We previously already finished up the headers, these headers are + // now trailers. if (header_parsing_state_ == HeaderParsingState::Done) { - // Ignore trailers. - return; + if (!enable_trailers_) { + // Ignore trailers. + return; + } + processing_trailers_ = true; + header_parsing_state_ = HeaderParsingState::Field; } - if (header_parsing_state_ == HeaderParsingState::Value) { completeLastHeader(); } @@ -475,11 +518,14 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { } void ConnectionImpl::onHeaderValue(const char* data, size_t length) { - if (header_parsing_state_ == HeaderParsingState::Done) { + if (header_parsing_state_ == HeaderParsingState::Done && !enable_trailers_) { // Ignore trailers. return; } + if (!current_header_map_) { + current_header_map_ = std::make_unique(); + } // Work around a bug in http_parser where trailing whitespace is not trimmed // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length)); @@ -505,16 +551,20 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { const uint32_t total = current_header_field_.size() + current_header_value_.size() + current_header_map_->byteSize(); if (total > (max_headers_kb_ * 1024)) { - + const absl::string_view header_type = + processing_trailers_ ? Http1HeaderTypes::get().Trailers : Http1HeaderTypes::get().Headers; error_code_ = Http::Code::RequestHeaderFieldsTooLarge; sendProtocolError(Http1ResponseCodeDetails::get().HeadersTooLarge); - throw CodecProtocolException("headers size exceeds limit"); + throw CodecProtocolException(absl::StrCat(header_type, " size exceeds limit")); } } int ConnectionImpl::onHeadersCompleteBase() { - ENVOY_CONN_LOG(trace, "headers complete", connection_); + const absl::string_view header_type = + processing_trailers_ ? Http1HeaderTypes::get().Trailers : Http1HeaderTypes::get().Headers; + ENVOY_CONN_LOG(trace, "onHeadersCompleteBase complete for {}", connection_, header_type); completeLastHeader(); + if (!(parser_.http_major == 1 && parser_.http_minor == 1)) { // This is not necessarily true, but it's good enough since higher layers only care if this is // HTTP/1.1 or not. @@ -571,6 +621,7 @@ int ConnectionImpl::onHeadersCompleteBase() { int rc = onHeadersComplete(std::move(current_header_map_)); current_header_map_.reset(); + header_parsing_state_ = HeaderParsingState::Done; // Returning 2 informs http_parser to not expect a body or further data on this connection. @@ -579,6 +630,7 @@ int ConnectionImpl::onHeadersCompleteBase() { void ConnectionImpl::onMessageCompleteBase() { ENVOY_CONN_LOG(trace, "message complete", connection_); + if (handling_upgrade_) { // If this is an upgrade request, swallow the onMessageComplete. The // upgrade payload will be treated as stream body. @@ -587,7 +639,14 @@ void ConnectionImpl::onMessageCompleteBase() { http_parser_pause(&parser_, 1); return; } - onMessageComplete(); + + // If true, this indicates we were processing trailers and must + // move the last header into current_header_map_ + if (header_parsing_state_ == HeaderParsingState::Value) { + completeLastHeader(); + } + + onMessageComplete(std::move(current_header_map_)); } void ConnectionImpl::onMessageBeginBase() { @@ -610,10 +669,11 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, Stats::Scope& stats, ServerConnectionCallbacks& callbacks, - Http1Settings settings, uint32_t max_request_headers_kb, + const Http1Settings& settings, + uint32_t max_request_headers_kb, const uint32_t max_request_headers_count) : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings)), + max_request_headers_count, formatter(settings), settings.enable_trailers_), callbacks_(callbacks), codec_settings_(settings) {} void ServerConnectionImpl::onEncodeComplete() { @@ -670,6 +730,8 @@ void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int metho } int ServerConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { + ENVOY_CONN_LOG(trace, "Server: onHeadersComplete size={}", connection_, headers->size()); + // Handle the case where response happens prior to request complete. It's up to upper layer code // to disconnect the connection but we shouldn't fire any more events since it doesn't make // sense. @@ -734,16 +796,17 @@ void ServerConnectionImpl::onBody(const char* data, size_t length) { } } -void ServerConnectionImpl::onMessageComplete() { +void ServerConnectionImpl::onMessageComplete(HeaderMapImplPtr&& trailers) { if (active_request_) { - Buffer::OwnedImpl buffer; active_request_->remote_complete_ = true; - if (deferred_end_stream_headers_) { active_request_->request_decoder_->decodeHeaders(std::move(deferred_end_stream_headers_), true); deferred_end_stream_headers_.reset(); + } else if (processing_trailers_) { + active_request_->request_decoder_->decodeTrailers(std::move(trailers)); } else { + Buffer::OwnedImpl buffer; active_request_->request_decoder_->decodeData(buffer, true); } } @@ -792,7 +855,7 @@ ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, Stat ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings)) {} + max_response_headers_count, formatter(settings), settings.enable_trailers_) {} bool ClientConnectionImpl::cannotHaveBody() { if ((!pending_responses_.empty() && pending_responses_.front().head_request_) || @@ -825,6 +888,7 @@ void ClientConnectionImpl::onEncodeHeaders(const HeaderMap& headers) { } int ClientConnectionImpl::onHeadersComplete(HeaderMapImplPtr&& headers) { + ENVOY_CONN_LOG(trace, "Client: onHeadersComplete size={}", connection_, headers->size()); headers->setStatus(parser_.status_code); // Handle the case where the client is closing a kept alive connection (by sending a 408 @@ -859,7 +923,7 @@ void ClientConnectionImpl::onBody(const char* data, size_t length) { } } -void ClientConnectionImpl::onMessageComplete() { +void ClientConnectionImpl::onMessageComplete(HeaderMapImplPtr&& trailers) { ENVOY_CONN_LOG(trace, "message complete", connection_); if (ignore_message_complete_for_100_continue_) { ignore_message_complete_for_100_continue_ = false; @@ -885,6 +949,8 @@ void ClientConnectionImpl::onMessageComplete() { if (deferred_end_stream_headers_) { response.decoder_->decodeHeaders(std::move(deferred_end_stream_headers_), true); deferred_end_stream_headers_.reset(); + } else if (processing_trailers_) { + response.decoder_->decodeTrailers(std::move(trailers)); } else { Buffer::OwnedImpl buffer; response.decoder_->decodeData(buffer, true); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index eb3e6c732965..302feb05b83c 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -141,6 +141,8 @@ class RequestStreamEncoderImpl : public StreamEncoderImpl { /** * Base class for HTTP/1.1 client and server connections. + * Handles the callbacks of http_parser with its own base routine and then + * virtual dispatches to its subclasses. */ class ConnectionImpl : public virtual Connection, protected Logger::Loggable { public: @@ -195,10 +197,12 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable frame; - Grpc::Encoder().newFrame(Grpc::GRPC_FH_DEFAULT, length, frame); - buffer.prepend(buffer_); - Buffer::OwnedImpl frame_buffer(frame.data(), frame.size()); - buffer.prepend(frame_buffer); + buildGrpcFrameHeader(buffer); } return Http::FilterDataStatus::Continue; @@ -203,6 +190,32 @@ Http::FilterDataStatus Filter::encodeData(Buffer::Instance& buffer, bool end_str } } +Http::FilterTrailersStatus Filter::encodeTrailers(Http::HeaderMap& trailers) { + trailers.setGrpcStatus(grpc_status_); + + if (withhold_grpc_frames_) { + buildGrpcFrameHeader(buffer_); + encoder_callbacks_->addEncodedData(buffer_, false); + } + + return Http::FilterTrailersStatus::Continue; +} + +void Filter::buildGrpcFrameHeader(Buffer::Instance& buffer) { + // Compute the size of the payload and construct the length prefix. + // + // We do this even if the upstream failed: If the response returned non-200, + // we'll respond with a grpc-status with an error, so clients will know that the request + // was unsuccessful. Since we're guaranteed at this point to have a valid response + // (unless upstream lied in content-type) we attempt to return a well-formed gRPC + // response body. + const auto length = buffer.length(); + std::array frame; + Grpc::Encoder().newFrame(Grpc::GRPC_FH_DEFAULT, length, frame); + Buffer::OwnedImpl frame_buffer(frame.data(), frame.size()); + buffer.prepend(frame_buffer); +} + } // namespace GrpcHttp1ReverseBridge } // namespace HttpFilters } // namespace Extensions diff --git a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h index f3e57361b1e1..665c0543ded0 100644 --- a/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h +++ b/source/extensions/filters/http/grpc_http1_reverse_bridge/filter.h @@ -28,8 +28,12 @@ class Filter : public Envoy::Http::PassThroughFilter { // Http::StreamEncoderFilter Http::FilterHeadersStatus encodeHeaders(Http::HeaderMap& headers, bool end_stream) override; Http::FilterDataStatus encodeData(Buffer::Instance& buffer, bool end_stream) override; + Http::FilterTrailersStatus encodeTrailers(Http::HeaderMap& trailers) override; private: + // Prepend the grpc frame into the buffer + void buildGrpcFrameHeader(Buffer::Instance& buffer); + const std::string upstream_content_type_; const bool withhold_grpc_frames_; diff --git a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc index 37c593d7026b..f032f9e250d4 100644 --- a/source/extensions/filters/http/grpc_web/grpc_web_filter.cc +++ b/source/extensions/filters/http/grpc_web/grpc_web_filter.cc @@ -208,6 +208,9 @@ Http::FilterTrailersStatus GrpcWebFilter::encodeTrailers(Http::HeaderMap& traile return Http::HeaderMap::Iterate::Continue; }, &temp); + + // Clear out the trailers so they don't get added since it is now in the body + trailers.clear(); Buffer::OwnedImpl buffer; // Adds the trailers frame head. buffer.add(&GRPC_WEB_TRAILER, 1); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 190464d72e26..c36c2467148f 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -22,11 +22,13 @@ #include "gtest/gtest.h" using testing::_; +using testing::AtLeast; using testing::InSequence; using testing::Invoke; using testing::NiceMock; using testing::Return; using testing::ReturnRef; +using testing::StrictMock; namespace Envoy { namespace Http { @@ -60,7 +62,10 @@ class Http1ServerConnectionImplTest : public testing::Test { void expect400(Protocol p, bool allow_absolute_url, Buffer::OwnedImpl& buffer, absl::string_view details = ""); void testRequestHeadersExceedLimit(std::string header_string, absl::string_view details = ""); + void testTrailersExceedLimit(std::string trailer_string, bool enable_trailers); void testRequestHeadersAccepted(std::string header_string); + // Used to test if trailers are decoded/encoded + void expectTrailersTest(bool enable_trailers); // Send the request, and validate the received request headers. // Then send a response just to clean up. @@ -140,6 +145,76 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs EXPECT_EQ(p, codec_->protocol()); } +void Http1ServerConnectionImplTest::expectTrailersTest(bool enable_trailers) { + initialize(); + + // Make a new 'codec' with the right settings + if (enable_trailers) { + codec_settings_.enable_trailers_ = enable_trailers; + codec_ = + std::make_unique(connection_, store_, callbacks_, codec_settings_, + max_request_headers_kb_, max_request_headers_count_); + } + + StrictMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce( + Invoke([&](Http::StreamEncoder&, bool) -> Http::StreamDecoder& { return decoder; })); + + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + + if (enable_trailers) { + EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeTrailers_).Times(1); + } else { + EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeData(_, true)); + } + + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello " + "World\r\n0\r\nhello: world\r\nsecond: header\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); +} + +void Http1ServerConnectionImplTest::testTrailersExceedLimit(std::string trailer_string, + bool enable_trailers) { + initialize(); + // Make a new 'codec' with the right settings + codec_settings_.enable_trailers_ = enable_trailers; + codec_ = + std::make_unique(connection_, store_, callbacks_, codec_settings_, + max_request_headers_kb_, max_request_headers_count_); + std::string exception_reason; + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce( + Invoke([&](Http::StreamEncoder&, bool) -> Http::StreamDecoder& { return decoder; })); + + if (enable_trailers) { + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + } else { + EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + EXPECT_CALL(decoder, decodeData(_, true)); + } + + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\n" + "body\r\n0\r\n"); + codec_->dispatch(buffer); + buffer = Buffer::OwnedImpl(trailer_string + "\r\n\r\n"); + if (enable_trailers) { + EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), EnvoyException, + "trailers size exceeds limit"); + } else { + // If trailers are not enabled, we expect Envoy to simply skip over the large + // trailers as if nothing has happened! + codec_->dispatch(buffer); + } +} void Http1ServerConnectionImplTest::testRequestHeadersExceedLimit(std::string header_string, absl::string_view details) { initialize(); @@ -398,6 +473,30 @@ TEST_F(Http1ServerConnectionImplTest, Http11InvalidRequest) { expect400(Protocol::Http11, true, buffer, "http1.codec_error"); } +TEST_F(Http1ServerConnectionImplTest, Http11InvalidTrailerPost) { + initialize(); + + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + + StrictMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce( + Invoke([&](Http::StreamEncoder&, bool) -> Http::StreamDecoder& { return decoder; })); + + EXPECT_CALL(decoder, decodeHeaders_(_, false)); + EXPECT_CALL(decoder, decodeData(_, false)).Times(AtLeast(1)); + + Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\n" + "body\r\n0\r\n" + "badtrailer\r\n\r\n"); + EXPECT_THROW(codec_->dispatch(buffer), CodecProtocolException); + EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", output); +} + TEST_F(Http1ServerConnectionImplTest, Http11AbsolutePathNoSlash) { initialize(); @@ -846,7 +945,41 @@ TEST_F(Http1ServerConnectionImplTest, ChunkedResponse) { Buffer::OwnedImpl data("Hello World"); response_encoder->encodeData(data, true); - EXPECT_EQ("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello World\r\n0\r\n\r\n", + + EXPECT_EQ("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello " + "World\r\n0\r\n\r\n", + output); +} + +TEST_F(Http1ServerConnectionImplTest, ChunkedResponseWithTrailers) { + codec_settings_.enable_trailers_ = true; + initialize(); + NiceMock decoder; + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + + Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n\r\n"); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + + std::string output; + ON_CALL(connection_, write(_, _)).WillByDefault(AddBufferToString(&output)); + + TestHeaderMapImpl headers{{":status", "200"}}; + response_encoder->encodeHeaders(headers, false); + + Buffer::OwnedImpl data("Hello World"); + response_encoder->encodeData(data, false); + + TestHeaderMapImpl trailers{{"foo", "bar"}, {"foo", "baz"}}; + response_encoder->encodeTrailers(trailers); + + EXPECT_EQ("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello " + "World\r\n0\r\nfoo: bar\r\nfoo: baz\r\n\r\n", output); } @@ -947,22 +1080,9 @@ TEST_F(Http1ServerConnectionImplTest, DoubleRequest) { EXPECT_EQ(0U, buffer.length()); } -TEST_F(Http1ServerConnectionImplTest, RequestWithTrailers) { - initialize(); - - NiceMock decoder; - Http::StreamEncoder* response_encoder = nullptr; - EXPECT_CALL(callbacks_, newStream(_, _)) - .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { - response_encoder = &encoder; - return decoder; - })); +TEST_F(Http1ServerConnectionImplTest, RequestWithTrailersDropped) { expectTrailersTest(false); } - Buffer::OwnedImpl buffer("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\nb\r\nHello " - "World\r\n0\r\nhello: world\r\nsecond: header\r\n\r\n"); - codec_->dispatch(buffer); - EXPECT_EQ(0U, buffer.length()); -} +TEST_F(Http1ServerConnectionImplTest, RequestWithTrailersKept) { expectTrailersTest(true); } TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2c) { initialize(); @@ -1478,6 +1598,31 @@ TEST_F(Http1ClientConnectionImplTest, HighwatermarkMultipleResponses) { static_cast(codec_.get()) ->onUnderlyingConnectionBelowWriteBufferLowWatermark(); } + +TEST_F(Http1ServerConnectionImplTest, LargeTrailersRejected) { + // Default limit of 60 KiB + std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n"; + testTrailersExceedLimit(long_string, true); +} + +// Tests that the default limit for the number of request headers is 100. +TEST_F(Http1ServerConnectionImplTest, ManyTrailersRejected) { + // Send a request with 101 headers. + testTrailersExceedLimit(createHeaderFragment(101), true); +} + +TEST_F(Http1ServerConnectionImplTest, LargeTrailersRejectedIgnored) { + // Default limit of 60 KiB + std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n"; + testTrailersExceedLimit(long_string, false); +} + +// Tests that the default limit for the number of request headers is 100. +TEST_F(Http1ServerConnectionImplTest, ManyTrailersIgnored) { + // Send a request with 101 headers. + testTrailersExceedLimit(createHeaderFragment(101), false); +} + TEST_F(Http1ServerConnectionImplTest, LargeRequestHeadersRejected) { // Default limit of 60 KiB std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n"; diff --git a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc index 6846fca135a7..4c9e6c2f14ea 100644 --- a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc +++ b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_integration_test.cc @@ -110,6 +110,7 @@ TEST_P(ReverseBridgeIntegrationTest, EnabledRoute) { std::equal(response->body().begin(), response->body().begin() + 4, expected_prefix.begin())); EXPECT_THAT(response->headers(), HeaderValueOf(Http::Headers::get().ContentType, "application/grpc")); + EXPECT_THAT(*response->trailers(), HeaderValueOf(Http::Headers::get().GrpcStatus, "0")); codec_client_->close(); ASSERT_TRUE(fake_upstream_connection_->close()); diff --git a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc index f2bc72abd15a..41d4f01c487a 100644 --- a/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc +++ b/test/extensions/filters/http/grpc_http1_reverse_bridge/reverse_bridge_test.cc @@ -667,6 +667,84 @@ TEST_F(ReverseBridgeTest, FilterConfigPerRouteEnabled) { } } +TEST_F(ReverseBridgeTest, RouteWithTrailers) { + initialize(); + decoder_callbacks_.is_grpc_request_ = true; + + envoy::config::filter::http::grpc_http1_reverse_bridge::v2alpha1::FilterConfigPerRoute + filter_config_per_route; + filter_config_per_route.set_disabled(false); + FilterConfigPerRoute filterConfigPerRoute(filter_config_per_route); + + ON_CALL(*decoder_callbacks_.route_, + perFilterConfig(HttpFilterNames::get().GrpcHttp1ReverseBridge)) + .WillByDefault(testing::Return(&filterConfigPerRoute)); + + { + EXPECT_CALL(decoder_callbacks_, route()).Times(2); + EXPECT_CALL(decoder_callbacks_, clearRouteCache()); + Http::TestHeaderMapImpl headers({{"content-type", "application/grpc"}, + {"content-length", "25"}, + {":path", "/testing.ExampleService/SendData"}}); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->decodeHeaders(headers, false)); + EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().ContentType, "application/x-protobuf")); + EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().ContentLength, "20")); + EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().Accept, "application/x-protobuf")); + } + + { + // We should remove the first five bytes. + Envoy::Buffer::OwnedImpl buffer; + buffer.add("abcdefgh", 8); + EXPECT_EQ(Http::FilterDataStatus::Continue, filter_->decodeData(buffer, false)); + EXPECT_EQ("fgh", buffer.toString()); + } + + { + Http::TestHeaderMapImpl trailers; + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->decodeTrailers(trailers)); + } + + Http::TestHeaderMapImpl headers( + {{":status", "200"}, {"content-length", "30"}, {"content-type", "application/x-protobuf"}}); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_->encodeHeaders(headers, false)); + EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().ContentType, "application/grpc")); + EXPECT_THAT(headers, HeaderValueOf(Http::Headers::get().ContentLength, "35")); + + { + // First few calls should drain the buffer + Envoy::Buffer::OwnedImpl buffer; + buffer.add("abc", 4); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->encodeData(buffer, false)); + EXPECT_EQ(0, buffer.length()); + } + { + // First few calls should drain the buffer + Envoy::Buffer::OwnedImpl buffer; + buffer.add("def", 4); + EXPECT_EQ(Http::FilterDataStatus::StopIterationAndBuffer, filter_->encodeData(buffer, false)); + EXPECT_EQ(0, buffer.length()); + } + + { + // Last call should prefix the buffer with the size and insert the gRPC status into trailers. + Envoy::Buffer::OwnedImpl buffer; + EXPECT_CALL(encoder_callbacks_, addEncodedData(_, false)) + .WillOnce(Invoke([&](Envoy::Buffer::Instance& buf, bool) -> void { buffer.move(buf); })); + Http::TestHeaderMapImpl trailers({{"foo", "bar"}, {"one", "two"}, {"three", "four"}}); + EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_->encodeTrailers(trailers)); + EXPECT_THAT(trailers, HeaderValueOf(Http::Headers::get().GrpcStatus, "0")); + + Grpc::Decoder decoder; + std::vector frames; + decoder.decode(buffer, frames); + + EXPECT_EQ(4, trailers.size()); + EXPECT_EQ(1, frames.size()); + EXPECT_EQ(8, frames[0].length_); + } +} + } // namespace } // namespace GrpcHttp1ReverseBridge } // namespace HttpFilters diff --git a/test/extensions/filters/http/grpc_web/BUILD b/test/extensions/filters/http/grpc_web/BUILD index 482dfbf0b254..5c35c5c6c357 100644 --- a/test/extensions/filters/http/grpc_web/BUILD +++ b/test/extensions/filters/http/grpc_web/BUILD @@ -34,3 +34,17 @@ envoy_extension_cc_test( "//test/mocks/server:server_mocks", ], ) + +envoy_extension_cc_test( + name = "grpc_web_integration_test", + srcs = ["grpc_web_filter_integration_test.cc"], + extension_name = "envoy.filters.http.grpc_web", + deps = [ + "//source/common/buffer:buffer_lib", + "//source/common/http:header_map_lib", + "//source/extensions/filters/http/grpc_web:grpc_web_filter_lib", + "//test/integration:http_integration_lib", + "//test/mocks/upstream:upstream_mocks", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc new file mode 100644 index 000000000000..ae8bfbd47738 --- /dev/null +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_integration_test.cc @@ -0,0 +1,62 @@ +#include + +#include "extensions/filters/http/well_known_names.h" + +#include "test/integration/http_integration.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace { + +class GrpcWebFilterIntegrationTest : public ::testing::TestWithParam, + public HttpIntegrationTest { +public: + GrpcWebFilterIntegrationTest() + : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()) {} + + void SetUp() override { + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + config_helper_.addFilter("name: envoy.grpc_web"); + } +}; + +TEST_P(GrpcWebFilterIntegrationTest, GRPCWebTrailersNotDuplicated) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + setUpstreamProtocol(FakeHttpConnection::Type::HTTP2); + + Http::TestHeaderMapImpl request_trailers{{"request1", "trailer1"}, {"request2", "trailer2"}}; + Http::TestHeaderMapImpl response_trailers{{"response1", "trailer1"}, {"response2", "trailer2"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {"content-type", "application/grpc-web"}, + {":authority", "host"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 1, false); + codec_client_->sendTrailers(*request_encoder_, request_trailers); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, false); + upstream_request_->encodeData(1, false); + upstream_request_->encodeTrailers(response_trailers); + response->waitForEndStream(); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(1, upstream_request_->bodyLength()); + EXPECT_THAT(*upstream_request_->trailers(), HeaderMapEqualRef(&request_trailers)); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_TRUE(absl::StrContains(response->body(), "response1:trailer1")); + EXPECT_TRUE(absl::StrContains(response->body(), "response2:trailer2")); + // Expect that the trailers be in the response-body instead + EXPECT_EQ(response->trailers(), nullptr); +} + +} // namespace +} // namespace Envoy diff --git a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc index b20f517ee5e9..538716f5541e 100644 --- a/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc +++ b/test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc @@ -355,6 +355,7 @@ TEST_P(GrpcWebFilterTest, Unary) { FAIL() << "Unsupported gRPC-Web response content-type: " << response_headers.ContentType()->value().getStringView(); } + EXPECT_EQ(0, response_trailers.size()); } INSTANTIATE_TEST_SUITE_P(Unary, GrpcWebFilterTest, diff --git a/test/integration/autonomous_upstream.cc b/test/integration/autonomous_upstream.cc index dfecdc5f66d6..997f28cad759 100644 --- a/test/integration/autonomous_upstream.cc +++ b/test/integration/autonomous_upstream.cc @@ -21,12 +21,15 @@ const char AutonomousStream::EXPECT_REQUEST_SIZE_BYTES[] = "expect_request_size_ const char AutonomousStream::RESET_AFTER_REQUEST[] = "reset_after_request"; AutonomousStream::AutonomousStream(FakeHttpConnection& parent, Http::StreamEncoder& encoder, - AutonomousUpstream& upstream) - : FakeStream(parent, encoder, upstream.timeSystem()), upstream_(upstream) {} + AutonomousUpstream& upstream, bool allow_incomplete_streams) + : FakeStream(parent, encoder, upstream.timeSystem()), upstream_(upstream), + allow_incomplete_streams_(allow_incomplete_streams) {} -// For now, assert all streams which are started are completed. -// Support for incomplete streams can be added when needed. -AutonomousStream::~AutonomousStream() { RELEASE_ASSERT(complete(), ""); } +AutonomousStream::~AutonomousStream() { + if (!allow_incomplete_streams_) { + RELEASE_ASSERT(complete(), "Found that end_stream is not true"); + } +} // By default, automatically send a response when the request is complete. void AutonomousStream::setEndStream(bool end_stream) { @@ -68,7 +71,8 @@ AutonomousHttpConnection::AutonomousHttpConnection(SharedConnectionWrapper& shar Http::StreamDecoder& AutonomousHttpConnection::newStream(Http::StreamEncoder& response_encoder, bool) { - auto stream = new AutonomousStream(*this, response_encoder, upstream_); + auto stream = + new AutonomousStream(*this, response_encoder, upstream_, upstream_.allow_incomplete_streams_); streams_.push_back(FakeStreamPtr{stream}); return *(stream); } diff --git a/test/integration/autonomous_upstream.h b/test/integration/autonomous_upstream.h index 15110798b8f9..4cda07a7ad2e 100644 --- a/test/integration/autonomous_upstream.h +++ b/test/integration/autonomous_upstream.h @@ -21,7 +21,7 @@ class AutonomousStream : public FakeStream { static const char RESET_AFTER_REQUEST[]; AutonomousStream(FakeHttpConnection& parent, Http::StreamEncoder& encoder, - AutonomousUpstream& upstream); + AutonomousUpstream& upstream, bool allow_incomplete_streams); ~AutonomousStream() override; void setEndStream(bool set) override; @@ -29,6 +29,7 @@ class AutonomousStream : public FakeStream { private: AutonomousUpstream& upstream_; void sendResponse(); + const bool allow_incomplete_streams_{false}; }; // An upstream which creates AutonomousStreams for new incoming streams. @@ -50,15 +51,15 @@ using AutonomousHttpConnectionPtr = std::unique_ptr; class AutonomousUpstream : public FakeUpstream { public: AutonomousUpstream(const Network::Address::InstanceConstSharedPtr& address, - FakeHttpConnection::Type type, Event::TestTimeSystem& time_system) - : FakeUpstream(address, type, time_system) {} - AutonomousUpstream(uint32_t port, FakeHttpConnection::Type type, - Network::Address::IpVersion version, Event::TestTimeSystem& time_system) - : FakeUpstream(port, type, version, time_system) {} + FakeHttpConnection::Type type, Event::TestTimeSystem& time_system, + bool allow_incomplete_streams) + : FakeUpstream(address, type, time_system), + allow_incomplete_streams_(allow_incomplete_streams) {} AutonomousUpstream(Network::TransportSocketFactoryPtr&& transport_socket_factory, uint32_t port, FakeHttpConnection::Type type, Network::Address::IpVersion version, - Event::TestTimeSystem& time_system) - : FakeUpstream(std::move(transport_socket_factory), port, type, version, time_system) {} + Event::TestTimeSystem& time_system, bool allow_incomplete_streams) + : FakeUpstream(std::move(transport_socket_factory), port, type, version, time_system), + allow_incomplete_streams_(allow_incomplete_streams) {} ~AutonomousUpstream() override; bool @@ -70,6 +71,7 @@ class AutonomousUpstream : public FakeUpstream { void setLastRequestHeaders(const Http::HeaderMap& headers); std::unique_ptr lastRequestHeaders(); + const bool allow_incomplete_streams_{false}; private: Thread::MutexBasicLockable headers_lock_; diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 05d1e3732db5..1a2b7c680771 100644 --- a/test/integration/fake_upstream.cc +++ b/test/integration/fake_upstream.cc @@ -222,9 +222,12 @@ FakeHttpConnection::FakeHttpConnection(SharedConnectionWrapper& shared_connectio uint32_t max_request_headers_count) : FakeConnectionBase(shared_connection, time_system) { if (type == Type::HTTP1) { + Http::Http1Settings http1_settings; + // For the purpose of testing, we always have the upstream encode the trailers if any + http1_settings.enable_trailers_ = true; codec_ = std::make_unique( - shared_connection_.connection(), store, *this, Http::Http1Settings(), - max_request_headers_kb, max_request_headers_count); + shared_connection_.connection(), store, *this, http1_settings, max_request_headers_kb, + max_request_headers_count); } else { auto settings = Http::Http2Settings(); settings.allow_connect_ = true; diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 2f6a9a708f29..5c4cde301887 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -840,9 +840,11 @@ TEST_P(Http2IntegrationTest, GoAway) { EXPECT_EQ("200", response->headers().Status()->value().getStringView()); } -TEST_P(Http2IntegrationTest, Trailers) { testTrailers(1024, 2048); } +TEST_P(Http2IntegrationTest, Trailers) { testTrailers(1024, 2048, false, false); } -TEST_P(Http2IntegrationTest, TrailersGiantBody) { testTrailers(1024 * 1024, 1024 * 1024); } +TEST_P(Http2IntegrationTest, TrailersGiantBody) { + testTrailers(1024 * 1024, 1024 * 1024, false, false); +} TEST_P(Http2IntegrationTest, GrpcRequestTimeout) { config_helper_.addConfigModifier( diff --git a/test/integration/http2_upstream_integration_test.cc b/test/integration/http2_upstream_integration_test.cc index 1cbda9125e6c..5323cac6773c 100644 --- a/test/integration/http2_upstream_integration_test.cc +++ b/test/integration/http2_upstream_integration_test.cc @@ -55,7 +55,7 @@ TEST_P(Http2UpstreamIntegrationTest, Retry) { testRetry(); } TEST_P(Http2UpstreamIntegrationTest, GrpcRetry) { testGrpcRetry(); } -TEST_P(Http2UpstreamIntegrationTest, Trailers) { testTrailers(1024, 2048); } +TEST_P(Http2UpstreamIntegrationTest, Trailers) { testTrailers(1024, 2048, true, true); } // Ensure Envoy handles streaming requests and responses simultaneously. void Http2UpstreamIntegrationTest::bidirectionalStreaming(uint32_t bytes) { diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 4ad710919deb..85440f3aae5e 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -205,6 +205,7 @@ HttpIntegrationTest::makeRawHttpConnection(Network::ClientConnectionPtr&& conn) cluster->max_response_headers_count_ = 200; cluster->http2_settings_.allow_connect_ = true; cluster->http2_settings_.allow_metadata_ = true; + cluster->http1_settings_.enable_trailers_ = true; Upstream::HostDescriptionConstSharedPtr host_description{Upstream::makeTestHostDescription( cluster, fmt::format("tcp://{}:80", Network::Test::getLoopbackAddressUrlString(version_)))}; return std::make_unique(*dispatcher_, std::move(conn), host_description, @@ -270,6 +271,23 @@ void HttpIntegrationTest::setDownstreamProtocol(Http::CodecClient::Type downstre config_helper_.setClientCodec(typeToCodecType(downstream_protocol_)); } +ConfigHelper::HttpModifierFunction HttpIntegrationTest::setEnableDownstreamTrailersHttp1() { + return + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) { + hcm.mutable_http_protocol_options()->set_enable_trailers(true); + }; +} + +ConfigHelper::ConfigModifierFunction HttpIntegrationTest::setEnableUpstreamTrailersHttp1() { + return [&](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { + RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() == 1, ""); + if (fake_upstreams_[0]->httpType() == FakeHttpConnection::Type::HTTP1) { + auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0); + cluster->mutable_http_protocol_options()->set_enable_trailers(true); + } + }; +} + IntegrationStreamDecoderPtr HttpIntegrationTest::sendRequestAndWaitForResponse( const Http::TestHeaderMapImpl& request_headers, uint32_t request_body_size, const Http::TestHeaderMapImpl& response_headers, uint32_t response_size, int upstream_index, @@ -965,13 +983,18 @@ void HttpIntegrationTest::testLargeRequestTrailers(uint32_t size, uint32_t max_s codec_client_->sendData(*request_encoder_, 10, false); codec_client_->sendTrailers(*request_encoder_, request_trailers); - if (size >= max_size && downstream_protocol_ == Http::CodecClient::Type::HTTP2) { - // For HTTP/2, expect a stream reset when the size of the trailers is larger than the maximum - // limit. - response->waitForReset(); - codec_client_->close(); - EXPECT_FALSE(response->complete()); - + if (size >= max_size) { + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + codec_client_->waitForDisconnect(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("431", response->headers().Status()->value().getStringView()); + } else { + // Expect a stream reset when the size of the trailers is larger than the maximum + // limit. + response->waitForReset(); + codec_client_->close(); + EXPECT_FALSE(response->complete()); + } } else { waitForNextUpstreamRequest(); upstream_request_->encodeHeaders(default_response_headers_, true); @@ -1055,7 +1078,8 @@ void HttpIntegrationTest::testDownstreamResetBeforeResponseComplete() { EXPECT_EQ(512U, response->body().size()); } -void HttpIntegrationTest::testTrailers(uint64_t request_size, uint64_t response_size) { +void HttpIntegrationTest::testTrailers(uint64_t request_size, uint64_t response_size, + bool check_request, bool check_response) { Http::TestHeaderMapImpl request_trailers{{"request1", "trailer1"}, {"request2", "trailer2"}}; Http::TestHeaderMapImpl response_trailers{{"response1", "trailer1"}, {"response2", "trailer2"}}; @@ -1078,15 +1102,19 @@ void HttpIntegrationTest::testTrailers(uint64_t request_size, uint64_t response_ EXPECT_TRUE(upstream_request_->complete()); EXPECT_EQ(request_size, upstream_request_->bodyLength()); - if (fake_upstreams_[0]->httpType() == FakeHttpConnection::Type::HTTP2) { + if (check_request) { EXPECT_THAT(*upstream_request_->trailers(), HeaderMapEqualRef(&request_trailers)); + } else { + EXPECT_EQ(upstream_request_->trailers(), nullptr); } EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().Status()->value().getStringView()); EXPECT_EQ(response_size, response->body().size()); - if (fake_upstreams_[0]->httpType() == FakeHttpConnection::Type::HTTP2) { + if (check_response) { EXPECT_THAT(*response->trailers(), HeaderMapEqualRef(&response_trailers)); + } else { + EXPECT_EQ(response->trailers(), nullptr); } } diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 9dce110a222b..16fe8192d5f9 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -116,6 +116,12 @@ class HttpIntegrationTest : public BaseIntegrationTest { // config_helper_. void setDownstreamProtocol(Http::CodecClient::Type type); + // Enable the encoding/decoding of Http1 trailers downstream + ConfigHelper::HttpModifierFunction setEnableDownstreamTrailersHttp1(); + + // Enable the encoding/decoding of Http1 trailers upstream + ConfigHelper::ConfigModifierFunction setEnableUpstreamTrailersHttp1(); + // Sends |request_headers| and |request_body_size| bytes of body upstream. // Configured upstream to send |response_headers| and |response_body_size| // bytes of body downstream. @@ -208,7 +214,11 @@ class HttpIntegrationTest : public BaseIntegrationTest { // HTTP/2 client tests. void testDownstreamResetBeforeResponseComplete(); - void testTrailers(uint64_t request_size, uint64_t response_size); + // Test that trailers are sent. request_trailers_present and + // response_trailers_present will check if the trailers are present, otherwise + // makes sure they were dropped. + void testTrailers(uint64_t request_size, uint64_t response_size, bool request_trailers_present, + bool response_trailers_present); Http::CodecClient::Type downstreamProtocol() const { return downstream_protocol_; } // Prefix listener stat with IP:port, including IP version dependent loopback address. diff --git a/test/integration/integration.cc b/test/integration/integration.cc index 10f309dfe3a1..f41111d8c048 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -299,8 +299,8 @@ void BaseIntegrationTest::createUpstreams() { for (uint32_t i = 0; i < fake_upstreams_count_; ++i) { auto endpoint = upstream_address_fn_(i); if (autonomous_upstream_) { - fake_upstreams_.emplace_back( - new AutonomousUpstream(endpoint, upstream_protocol_, *time_system_)); + fake_upstreams_.emplace_back(new AutonomousUpstream( + endpoint, upstream_protocol_, *time_system_, autonomous_allow_incomplete_streams_)); } else { fake_upstreams_.emplace_back(new FakeUpstream(endpoint, upstream_protocol_, *time_system_, enable_half_close_, udp_fake_upstream_)); diff --git a/test/integration/integration.h b/test/integration/integration.h index 1120797dc607..b3839198b3a8 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -371,6 +371,10 @@ class BaseIntegrationTest : protected Logger::Loggable { // If true, use AutonomousUpstream for fake upstreams. bool autonomous_upstream_{false}; + // If true, allow incomplete streams in AutonomousUpstream + // This does nothing if autonomous_upstream_ is false + bool autonomous_allow_incomplete_streams_{false}; + bool enable_half_close_{false}; // Whether the default created fake upstreams are UDP listeners. diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index 7c8b22b0b434..c971a7b77653 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -399,6 +399,40 @@ TEST_P(IntegrationTest, InvalidVersion) { EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", response); } +// Expect that malformed trailers to break the connection +TEST_P(IntegrationTest, BadTrailer) { + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + std::string response; + sendRawHttpAndWaitForResponse(lookupPort("http"), + "POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\n" + "body\r\n0\r\n" + "badtrailer\r\n\r\n", + &response); + + EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", response); +} + +// Expect malformed headers to break the connection +TEST_P(IntegrationTest, BadHeader) { + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + std::string response; + sendRawHttpAndWaitForResponse(lookupPort("http"), + "POST / HTTP/1.1\r\n" + "Host: host\r\n" + "badHeader\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\n" + "body\r\n0\r\n\r\n", + &response); + + EXPECT_EQ("HTTP/1.1 400 Bad Request\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", response); +} + TEST_P(IntegrationTest, Http10Disabled) { initialize(); std::string response; @@ -532,6 +566,60 @@ TEST_P(IntegrationTest, Pipeline) { connection.close(); } +// Checks to ensure that we reject the third request that is pipelined in the +// same request +TEST_P(IntegrationTest, PipelineWithTrailers) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); + autonomous_upstream_ = true; + autonomous_allow_incomplete_streams_ = true; + initialize(); + std::string response; + + std::string good_request("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\n" + "body\r\n0\r\n" + "trailer1:t2\r\n" + "trailer2:t3\r\n" + "\r\n"); + + std::string bad_request("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\n" + "body\r\n0\r\n" + "trailer1\r\n" + "trailer2:t3\r\n" + "\r\n"); + + Buffer::OwnedImpl buffer(absl::StrCat(good_request, good_request, bad_request)); + + RawConnectionDriver connection( + lookupPort("http"), buffer, + [&](Network::ClientConnection&, const Buffer::Instance& data) -> void { + response.append(data.toString()); + }, + version_); + + // First response should be success. + size_t pos; + while ((pos = response.find("200")) == std::string::npos) { + connection.run(Event::Dispatcher::RunType::NonBlock); + } + EXPECT_THAT(response, HasSubstr("HTTP/1.1 200 OK\r\n")); + while (response.find("200", pos + 1) == std::string::npos) { + connection.run(Event::Dispatcher::RunType::NonBlock); + } + while (response.find("400") == std::string::npos) { + connection.run(Event::Dispatcher::RunType::NonBlock); + } + + EXPECT_THAT(response, HasSubstr("HTTP/1.1 400 Bad Request\r\n")); + connection.close(); +} + // Add a pipeline test where complete request headers in the first request merit // an inline sendLocalReply to make sure the "kick" works under the call stack // of dispatch as well as when a response is proxied from upstream. @@ -1017,6 +1105,18 @@ TEST_P(IntegrationTest, ProcessObjectUnealthy) { EXPECT_THAT(response->headers(), HttpStatusIs("500")); } +TEST_P(IntegrationTest, TrailersDroppedDuringEncoding) { testTrailers(10, 10, false, false); } + +TEST_P(IntegrationTest, TrailersDroppedUpstream) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + testTrailers(10, 10, false, false); +} + +TEST_P(IntegrationTest, TrailersDroppedDownstream) { + config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); + testTrailers(10, 10, false, false); +} + INSTANTIATE_TEST_SUITE_P(IpVersions, UpstreamEndpointIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 3070e62c465f..5b4e46271502 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -84,6 +84,13 @@ class DownstreamProtocolIntegrationTest : public HttpProtocolIntegrationTest { // downstream and H1/H2 upstreams. using ProtocolIntegrationTest = HttpProtocolIntegrationTest; +TEST_P(ProtocolIntegrationTest, TrailerSupportHttp1) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); + config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); + + testTrailers(10, 20, true, true); +} + TEST_P(ProtocolIntegrationTest, ShutdownWithActiveConnPoolConnections) { auto response = makeHeaderOnlyRequest(nullptr, 0); // Shut down the server with active connection pool connections. @@ -952,6 +959,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersAccepted) { TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) { // Default header (and trailer) count limit is 100. + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); Http::TestHeaderMapImpl request_trailers; for (int i = 0; i < 150; i++) { request_trailers.addCopy("trailer", std::string(1, 'a')); @@ -965,25 +973,15 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) { codec_client_->sendData(*request_encoder_, 1, false); codec_client_->sendTrailers(*request_encoder_, request_trailers); - // Only relevant to Http2Downstream. - if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { - // Http1 Downstream ignores trailers. - waitForNextUpstreamRequest(); - upstream_request_->encodeHeaders(default_response_headers_, true); - response->waitForEndStream(); - EXPECT_TRUE(response->complete()); - EXPECT_EQ("200", response->headers().Status()->value().getStringView()); - } else { - // Expect rejection. - // TODO(asraa): we shouldn't need this unexpected disconnect, but some tests hit unparented - // connections without it. Likely need to reconsider whether the waits/closes should be on - // client or upstream. - fake_upstreams_[0]->set_allow_unexpected_disconnects(true); - ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); - response->waitForReset(); - ASSERT_TRUE(fake_upstream_connection_->close()); - ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); - } + // Expect rejection. + // TODO(asraa): we shouldn't need this unexpected disconnect, but some tests hit unparented + // connections without it. Likely need to reconsider whether the waits/closes should be on + // client or upstream. + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + response->waitForReset(); + ASSERT_TRUE(fake_upstream_connection_->close()); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); } TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersAccepted) { @@ -995,6 +993,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersAccepted) { hcm.mutable_common_http_protocol_options()->mutable_max_headers_count()->set_value( max_count); }); + config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); max_request_headers_count_ = max_count; Http::TestHeaderMapImpl request_trailers; for (int i = 0; i < 150; i++) { @@ -1023,10 +1022,12 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersTimeout) { } TEST_P(DownstreamProtocolIntegrationTest, LargeRequestTrailersAccepted) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); testLargeRequestTrailers(60, 96); } TEST_P(DownstreamProtocolIntegrationTest, LargeRequestTrailersRejected) { + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); testLargeRequestTrailers(66, 60); } @@ -1036,6 +1037,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyTrailerHeaders) { max_request_headers_kb_ = 96; max_request_headers_count_ = 20005; + config_helper_.addConfigModifier(setEnableDownstreamTrailersHttp1()); config_helper_.addConfigModifier( [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) -> void { diff --git a/test/integration/transport_socket_match_integration_test.cc b/test/integration/transport_socket_match_integration_test.cc index bfe5f0cce4ee..613aa86108ff 100644 --- a/test/integration/transport_socket_match_integration_test.cc +++ b/test/integration/transport_socket_match_integration_test.cc @@ -149,11 +149,11 @@ require_client_certificate: true if (isTLSUpstream(i)) { fake_upstreams_.emplace_back(new AutonomousUpstream( createUpstreamSslContext(), endpoint->ip()->port(), FakeHttpConnection::Type::HTTP1, - endpoint->ip()->version(), timeSystem())); + endpoint->ip()->version(), timeSystem(), false)); } else { fake_upstreams_.emplace_back(new AutonomousUpstream( Network::Test::createRawBufferSocketFactory(), endpoint->ip()->port(), - FakeHttpConnection::Type::HTTP1, endpoint->ip()->version(), timeSystem())); + FakeHttpConnection::Type::HTTP1, endpoint->ip()->version(), timeSystem(), false)); } } }