diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index fe3dd1b5e9ea..4664070264d3 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -74,6 +74,7 @@ New Features * dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash. * grpc_json_transcoder: added option :ref:`strict_http_request_validation ` to reject invalid requests early. * grpc_json_transcoder: filter can now be configured on per-route/per-vhost level as well. Leaving empty list of services in the filter configuration disables transcoding on the specific route. +* http: added support for `Envoy::ScopeTrackedObject` for HTTP/1 dispatching. Crashes while inside the dispatching loop should dump debug information. * http: added support for :ref:`preconnecting `. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1. * http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false. * overload: add support for scaling :ref:`transport connection timeouts`. This can be used to reduce the TLS handshake timeout in response to overload. diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h index 9fbb8b43a175..f2821195fd9d 100644 --- a/source/common/common/dump_state_utils.h +++ b/source/common/common/dump_state_utils.h @@ -10,12 +10,13 @@ namespace Envoy { // memory would likely lead to the crash handler itself causing a subsequent OOM. #define DUMP_MEMBER(member) ", " #member ": " << (member) +#define DUMP_MEMBER_AS(member, value) ", " #member ": " << (value) #define DUMP_OPTIONAL_MEMBER(member) \ ", " #member ": " << ((member).has_value() ? absl::StrCat((member).value()) : "null") #define DUMP_NULLABLE_MEMBER(member, value) \ - ", " #member ": " << ((member) != nullptr ? value : "null") + ", " #member ": " << ((member) != nullptr ? (value) : "null") // Macro assumes local member variables // os (ostream) diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc index 2ed40659314c..a325bcc32110 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -457,6 +457,35 @@ std::string StringUtil::escape(const std::string& source) { return ret; } +// TODO(kbaichoo): If needed, add support for escaping chars < 32 and >= 127. +void StringUtil::escapeToOstream(std::ostream& os, absl::string_view view) { + for (const char c : view) { + switch (c) { + case '\r': + os << "\\r"; + break; + case '\n': + os << "\\n"; + break; + case '\t': + os << "\\t"; + break; + case '"': + os << "\\\""; + break; + case '\'': + os << "\\\'"; + break; + case '\\': + os << "\\\\"; + break; + default: + os << c; + break; + } + } +} + const std::string& getDefaultDateFormat() { CONSTRUCT_ON_FIRST_USE(std::string, "%Y-%m-%dT%H:%M:%E3SZ"); } diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 84acc287b0f3..6b695564400f 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -400,6 +400,16 @@ class StringUtil { */ static std::string escape(const std::string& source); + /** + * Outputs the string to the provided ostream, while escaping \n, \r, \t, and " + * (double quote), ' (single quote), and \ (backslash) escaped. + * This may be particularly useful if you cannot allocate memory, and the + * ostream being written to is backed by an entity that won't allocate memory. + * @param os the ostream to output to. + * @param view a string view to output + */ + static void escapeToOstream(std::ostream& os, absl::string_view view); + /** * Provide a default value for a string if empty. * @param s string. diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index 8c864eaca715..699ab767a9d0 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -33,6 +33,7 @@ envoy_cc_library( ":codec_stats_lib", ":header_formatter_lib", "//include/envoy/buffer:buffer_interface", + "//include/envoy/common:scope_tracker_interface", "//include/envoy/http:codec_interface", "//include/envoy/http:header_map_interface", "//include/envoy/network:connection_interface", @@ -40,6 +41,7 @@ envoy_cc_library( "//source/common/buffer:watermark_buffer_lib", "//source/common/common:assert_lib", "//source/common/common:cleanup_lib", + "//source/common/common:dump_state_utils", "//source/common/common:statusor_lib", "//source/common/common:utility_lib", "//source/common/grpc:common_lib", diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index da1925148756..5afb8b5699bb 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -10,7 +10,9 @@ #include "envoy/network/connection.h" #include "common/common/cleanup.h" +#include "common/common/dump_state_utils.h" #include "common/common/enum_to_int.h" +#include "common/common/scope_tracker.h" #include "common/common/statusor.h" #include "common/common/utility.h" #include "common/grpc/common.h" @@ -67,7 +69,6 @@ HeaderKeyFormatterPtr formatter(const Http::Http1Settings& settings) { return nullptr; } - } // namespace const std::string StreamEncoderImpl::CRLF = "\r\n"; @@ -569,6 +570,8 @@ Http::Status ClientConnectionImpl::dispatch(Buffer::Instance& data) { } Http::Status ConnectionImpl::innerDispatch(Buffer::Instance& data) { + // Add self to the Dispatcher's tracked object stack. + ScopeTrackerScopeState scope(this, connection_.dispatcher()); ENVOY_CONN_LOG(trace, "parsing {} bytes", connection_, data.length()); // Make sure that dispatching_ is set to false after dispatching, even when // http_parser exits early with an error code. @@ -863,6 +866,73 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { onResetStream(reason); } +void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const { + const char* spaces = spacesForLevel(indent_level); + os << spaces << "Http1::ConnectionImpl " << this << DUMP_MEMBER(dispatching_) + << DUMP_MEMBER(dispatching_slice_already_drained_) << DUMP_MEMBER(reset_stream_called_) + << DUMP_MEMBER(handling_upgrade_) << DUMP_MEMBER(deferred_end_stream_headers_) + << DUMP_MEMBER(strict_1xx_and_204_headers_) << DUMP_MEMBER(processing_trailers_) + << DUMP_MEMBER(buffered_body_.length()); + + // Dump header parsing state, and any progress on headers. + os << DUMP_MEMBER(header_parsing_state_); + os << DUMP_MEMBER_AS(current_header_field_, current_header_field_.getStringView()); + os << DUMP_MEMBER_AS(current_header_value_, current_header_value_.getStringView()); + + // Dump Child + os << '\n'; + dumpAdditionalState(os, indent_level); + + // Dump the first slice of the dispatching buffer if not drained escaping + // certain characters. We do this last as the slice could be rather large. + if (current_dispatching_buffer_ == nullptr || dispatching_slice_already_drained_) { + // Buffer is either null or already drained (in the body). + // Use the macro for consistent formatting. + os << DUMP_NULLABLE_MEMBER(current_dispatching_buffer_, "drained"); + return; + } else { + absl::string_view front_slice = [](Buffer::RawSlice slice) { + return absl::string_view(static_cast(slice.mem_), slice.len_); + }(current_dispatching_buffer_->frontSlice()); + + // Dump buffer data escaping \r, \n, \t, ", ', and \. + // This is not the most performant implementation, but we're crashing and + // cannot allocate memory. + os << spaces << "current_dispatching_buffer_ front_slice length: " << front_slice.length() + << " contents: \""; + StringUtil::escapeToOstream(os, front_slice); + os << "\"\n"; + } +} + +void ServerConnectionImpl::dumpAdditionalState(std::ostream& os, int indent_level) const { + const char* spaces = spacesForLevel(indent_level); + os << DUMP_MEMBER_AS(active_request_.request_url_, + active_request_.has_value() && + !active_request_.value().request_url_.getStringView().empty() + ? active_request_.value().request_url_.getStringView() + : "null"); + os << '\n'; + + // Dump header map, it may be null if it was moved to the request, and + // request_url. + if (absl::holds_alternative(headers_or_trailers_)) { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } else { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } +} + +void ClientConnectionImpl::dumpAdditionalState(std::ostream& os, int indent_level) const { + const char* spaces = spacesForLevel(indent_level); + // Dump header map, it may be null if it was moved to the request. + if (absl::holds_alternative(headers_or_trailers_)) { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } else { + DUMP_DETAILS(absl::get(headers_or_trailers_)); + } +} + ServerConnectionImpl::ServerConnectionImpl( Network::Connection& connection, CodecStats& stats, ServerConnectionCallbacks& callbacks, const Http1Settings& settings, uint32_t max_request_headers_kb, diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index a4f13cb38c4e..9bc4a9e97a48 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -8,6 +8,7 @@ #include #include +#include "envoy/common/scope_tracker.h" #include "envoy/config/core/v3/protocol.pb.h" #include "envoy/http/codec.h" #include "envoy/network/connection.h" @@ -170,7 +171,9 @@ class RequestEncoderImpl : public StreamEncoderImpl, public RequestEncoder { * 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 { +class ConnectionImpl : public virtual Connection, + protected Logger::Loggable, + public ScopeTrackedObject { public: /** * @return Network::Connection& the backing network connection. @@ -228,6 +231,9 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(RequestTrailerMapImpl::create()); } } + void dumpAdditionalState(std::ostream& os, int indent_level) const override; void releaseOutboundResponse(const Buffer::OwnedBufferFragmentImpl* fragment); void maybeAddSentinelBufferFragment(Buffer::Instance& output_buffer) override; @@ -614,6 +637,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { headers_or_trailers_.emplace(ResponseTrailerMapImpl::create()); } } + void dumpAdditionalState(std::ostream& os, int indent_level) const override; absl::optional pending_response_; // TODO(mattklein123): The following bool tracks whether a pending response is complete before diff --git a/test/common/common/utility_test.cc b/test/common/common/utility_test.cc index 348a0ae75221..5ebde35e63d2 100644 --- a/test/common/common/utility_test.cc +++ b/test/common/common/utility_test.cc @@ -282,6 +282,43 @@ TEST(StringUtil, escape) { EXPECT_EQ(StringUtil::escape("{\"linux\": \"penguin\"}"), "{\\\"linux\\\": \\\"penguin\\\"}"); } +TEST(StringUtil, escapeToOstream) { + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "hello world"); + EXPECT_EQ(ostream.contents(), "hello world"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "hello\nworld\n"); + EXPECT_EQ(ostream.contents(), "hello\\nworld\\n"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "\t\nworld\r\n"); + EXPECT_EQ(ostream.contents(), "\\t\\nworld\\r\\n"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, "{'linux': \"penguin\"}"); + EXPECT_EQ(ostream.contents(), "{\\'linux\\': \\\"penguin\\\"}"); + } + + { + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + StringUtil::escapeToOstream(ostream, R"(\\)"); + EXPECT_EQ(ostream.contents(), R"(\\\\)"); + } +} + TEST(StringUtil, toUpper) { EXPECT_EQ(StringUtil::toUpper(""), ""); EXPECT_EQ(StringUtil::toUpper("a"), "A"); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index e20a89f2d7b0..1466b4c68c97 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1,3 +1,5 @@ +#include +#include #include #include @@ -6,6 +8,7 @@ #include "envoy/http/codec.h" #include "common/buffer/buffer_impl.h" +#include "common/common/utility.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" #include "common/http/http1/codec_impl.h" @@ -24,6 +27,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::HasSubstr; using testing::InSequence; using testing::Invoke; using testing::NiceMock; @@ -1913,6 +1917,81 @@ TEST_F(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength100) testServerAllowChunkedContentLength(100, true); } +TEST_F(Http1ServerConnectionImplTest, + ShouldDumpParsedAndPartialHeadersWithoutAllocatingMemoryIfProcessingHeaders) { + initialize(); + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); + + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + + Buffer::OwnedImpl headers("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Accept-Language: en\r\n" + "Connection: keep-alive\r\n" + "Unfinished-Header: Not-Finished-Value"); + + auto status = codec_->dispatch(headers); + EXPECT_TRUE(status.ok()); + + // Dumps the header map without allocating memory + Stats::TestUtil::MemoryTest memory_test; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + EXPECT_EQ(memory_test.consumedBytes(), 0); + + // Check dump contents for completed headers and partial headers. + EXPECT_THAT( + ostream.contents(), + testing::HasSubstr("absl::get(headers_or_trailers_): \n ':authority', " + "'host'\n 'accept-language', 'en'\n 'connection', 'keep-alive'")); + EXPECT_THAT(ostream.contents(), + testing::HasSubstr("header_parsing_state_: Value, current_header_field_: " + "Unfinished-Header, current_header_value_: Not-Finished-Value")); +} + +TEST_F(Http1ServerConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingMemory) { + initialize(); + + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder&, bool) -> RequestDecoder& { return decoder; })); + + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + + // Dump the body + // Set content length to enable us to dumpState before + // buffers are drained. Only the first slice should be dumped. + Buffer::OwnedImpl request; + request.appendSliceForTest("POST / HTTP/1.1\r\n" + "Content-Length: 5\r\n" + "\r\n" + "Hello"); + request.appendSliceForTest("GarbageDataShouldNotBeDumped"); + EXPECT_CALL(decoder, decodeData(_, _)) + .WillOnce(Invoke([&](Buffer::Instance&, bool) { + // dumpState here before buffers are drained. No memory should be allocated. + Stats::TestUtil::MemoryTest memory_test; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + EXPECT_EQ(memory_test.consumedBytes(), 0); + })) + .WillOnce(Invoke([]() {})); + + auto status = codec_->dispatch(request); + EXPECT_TRUE(status.ok()); + + // Check dump contents + EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 5, header_parsing_state_: " + "Done, current_header_field_: , current_header_value_: " + "\n, active_request_.request_url_: null")); + EXPECT_THAT(ostream.contents(), + HasSubstr("current_dispatching_buffer_ front_slice length: 43 contents: \"POST / " + "HTTP/1.1\\r\\nContent-Length: 5\\r\\n\\r\\nHello\"\n")); +} + class Http1ClientConnectionImplTest : public Http1CodecTestBase { public: void initialize() { @@ -2960,5 +3039,73 @@ TEST_F(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength100) { testClientAllowChunkedContentLength(100, true); } +TEST_F(Http1ClientConnectionImplTest, + ShouldDumpParsedAndPartialHeadersWithoutAllocatingMemoryIfProcessingHeaders) { + initialize(); + + // Send request and dispatch response without headers completed. + MockResponseDecoder response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); + + Buffer::OwnedImpl response("HTTP/1.1 200 OK\r\nserver: foo\r\nContent-Length: 8"); + auto status = codec_->dispatch(response); + EXPECT_EQ(0UL, response.length()); + EXPECT_TRUE(status.ok()); + + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + + // Check for header map and partial headers. + EXPECT_THAT(ostream.contents(), + testing::HasSubstr( + "absl::get(headers_or_trailers_): \n 'server', 'foo'\n")); + EXPECT_THAT(ostream.contents(), + testing::HasSubstr("header_parsing_state_: Value, current_header_field_: " + "Content-Length, current_header_value_: 8")); +} + +TEST_F(Http1ClientConnectionImplTest, ShouldDumpDispatchBufferWithoutAllocatingMemory) { + initialize(); + + // Send request + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + EXPECT_TRUE(request_encoder.encodeHeaders(headers, true).ok()); + + // Send response; dumpState while parsing response. + std::array buffer; + OutputBufferStream ostream{buffer.data(), buffer.size()}; + EXPECT_CALL(response_decoder, decodeData(_, _)) + .WillOnce(Invoke([&](Buffer::Instance&, bool) { + // dumpState here before buffers are drained. No memory should be allocated. + Stats::TestUtil::MemoryTest memory_test; + dynamic_cast(codec_.get())->dumpState(ostream, 0); + EXPECT_EQ(memory_test.consumedBytes(), 0); + })) + .WillOnce(Invoke([]() {})); + + // Dump the body + // Set content length to enable us to dumpState before + // buffers are drained. Only the first slice should be dumped. + Buffer::OwnedImpl response; + response.appendSliceForTest("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\nHello"); + response.appendSliceForTest("GarbageDataShouldNotBeDumped"); + auto status = codec_->dispatch(response); + // Client codec complains about extraneous data. + EXPECT_EQ(response.length(), 28UL); + EXPECT_FALSE(status.ok()); + + // Check for body data. + EXPECT_THAT(ostream.contents(), HasSubstr("buffered_body_.length(): 5, header_parsing_state_: " + "Done")); + EXPECT_THAT(ostream.contents(), + testing::HasSubstr("current_dispatching_buffer_ front_slice length: 43 contents: " + "\"HTTP/1.1 200 OK\\r\\nContent-Length: 5\\r\\n\\r\\nHello\"\n")); +} + } // namespace Http } // namespace Envoy