From 76a70b40f57bd9a75b50d4783d28dec0e0aa29ae Mon Sep 17 00:00:00 2001 From: Max Kuznetsov Date: Tue, 30 Nov 2021 11:06:01 -0500 Subject: [PATCH] Extend StatefulHeaderFormatter to allow forwarding HTTP1 reason phrase (#18997) Signed-off-by: Max Kuznetsov --- .../preserve_case/v3/preserve_case.proto | 3 + docs/root/version_history/current.rst | 1 + envoy/http/header_formatter.h | 10 ++ source/common/http/http1/codec_impl.cc | 22 ++++- source/common/http/http1/codec_impl.h | 2 + .../common/http/http1/legacy_parser_impl.cc | 6 +- source/common/http/http1/parser.h | 8 ++ .../preserve_case/preserve_case_formatter.cc | 34 ++++++- .../preserve_case/preserve_case_formatter.h | 7 ++ .../header_formatters/preserve_case/BUILD | 14 +++ ...ormatter_reason_phrase_integration_test.cc | 93 +++++++++++++++++++ .../preserve_case_formatter_test.cc | 18 +++- 12 files changed, 210 insertions(+), 8 deletions(-) create mode 100644 test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_reason_phrase_integration_test.cc diff --git a/api/envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.proto b/api/envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.proto index 64bdd497ecab..5bd369141637 100644 --- a/api/envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.proto +++ b/api/envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.proto @@ -16,4 +16,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // See the :ref:`header casing ` configuration guide for more // information. message PreserveCaseFormatterConfig { + // Allows forwarding reason phrase text. + // This is off by default, and a standard reason phrase is used for a corresponding HTTP response code. + bool forward_reason_phrase = 1; } diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 126a79c1bb97..ddaed36ae2dc 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -70,6 +70,7 @@ New Features * http: added support for %REQUESTED_SERVER_NAME% to extract SNI as a custom header. * http: added support for :ref:`retriable health check status codes `. * http: added timing information about upstream connection and encryption establishment to stream info. These can currently be accessed via custom access loggers. +* http: added support for :ref:`forwarding HTTP1 reason phrase `. * listener: added API for extensions to access :ref:`typed_filter_metadata ` configured in the listener's :ref:`metadata ` field. * listener: added support for :ref:`MPTCP ` (multipath TCP). * listener: added support for opting out listeners from the globally set downstream connection limit via :ref:`ignore_global_conn_limit `. diff --git a/envoy/http/header_formatter.h b/envoy/http/header_formatter.h index 8615cd7a7a5d..6a7c244e5e0a 100644 --- a/envoy/http/header_formatter.h +++ b/envoy/http/header_formatter.h @@ -33,6 +33,16 @@ class StatefulHeaderKeyFormatter : public HeaderKeyFormatter { * Called for each header key received by the codec. */ virtual void processKey(absl::string_view key) PURE; + + /** + * Called to save received reason phrase + */ + virtual void setReasonPhrase(absl::string_view reason_phrase) PURE; + + /** + * Called to get saved reason phrase + */ + virtual absl::string_view getReasonPhrase() const PURE; }; using StatefulHeaderKeyFormatterPtr = std::unique_ptr; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index fdd317babbbf..0ddc9c8a69aa 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -414,9 +414,15 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e connection_.addIntToBuffer(numeric_status); connection_.addCharToBuffer(' '); - const char* status_string = CodeUtility::toString(static_cast(numeric_status)); - uint32_t status_string_len = strlen(status_string); - connection_.copyToBuffer(status_string, status_string_len); + StatefulHeaderKeyFormatterOptConstRef formatter(headers.formatter()); + + if (formatter.has_value() && !formatter->getReasonPhrase().empty()) { + connection_.addToBuffer(formatter->getReasonPhrase()); + } else { + const char* status_string = CodeUtility::toString(static_cast(numeric_status)); + uint32_t status_string_len = strlen(status_string); + connection_.copyToBuffer(status_string, status_string_len); + } connection_.addCharToBuffer('\r'); connection_.addCharToBuffer('\n'); @@ -1301,6 +1307,16 @@ RequestEncoder& ClientConnectionImpl::newStream(ResponseDecoder& response_decode return pending_response_.value().encoder_; } +Status ClientConnectionImpl::onStatus(const char* data, size_t length) { + auto& headers = absl::get(headers_or_trailers_); + StatefulHeaderKeyFormatterOptRef formatter(headers->formatter()); + if (formatter.has_value()) { + formatter->setReasonPhrase(absl::string_view(data, length)); + } + + return okStatus(); +} + Envoy::StatusOr ClientConnectionImpl::onHeadersCompleteBase() { ENVOY_CONN_LOG(trace, "status_code {}", connection_, parser_->statusCode()); diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index c0904991cf3b..94864dff09f0 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -499,6 +499,7 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { // ParserCallbacks. Status onUrl(const char* data, size_t length) override; + Status onStatus(const char*, size_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } // ConnectionImpl void onEncodeComplete() override; StreamInfo::BytesMeter& getBytesMeter() override { @@ -593,6 +594,7 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { // ParserCallbacks. Status onUrl(const char*, size_t) override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + Status onStatus(const char* data, size_t length) override; // ConnectionImpl Http::Status dispatch(Buffer::Instance& data) override; void onEncodeComplete() override {} diff --git a/source/common/http/http1/legacy_parser_impl.cc b/source/common/http/http1/legacy_parser_impl.cc index 840ba419099f..2da1d3bc177c 100644 --- a/source/common/http/http1/legacy_parser_impl.cc +++ b/source/common/http/http1/legacy_parser_impl.cc @@ -51,7 +51,11 @@ class LegacyHttpParserImpl::Impl { auto status = conn_impl->onUrl(at, length); return conn_impl->setAndCheckCallbackStatus(std::move(status)); }, - nullptr, // on_status + [](http_parser* parser, const char* at, size_t length) -> int { + auto* conn_impl = static_cast(parser->data); + auto status = conn_impl->onStatus(at, length); + return conn_impl->setAndCheckCallbackStatus(std::move(status)); + }, [](http_parser* parser, const char* at, size_t length) -> int { auto* conn_impl = static_cast(parser->data); auto status = conn_impl->onHeaderField(at, length); diff --git a/source/common/http/http1/parser.h b/source/common/http/http1/parser.h index 23f6795a1f75..324ae41cc414 100644 --- a/source/common/http/http1/parser.h +++ b/source/common/http/http1/parser.h @@ -72,6 +72,14 @@ class ParserCallbacks { */ virtual Status onHeaderValue(const char* data, size_t length) PURE; + /** + * Called when response status data is received. + * @param data supplies the start address. + * @param length supplies the length. + * @return Status representing success or failure. + */ + virtual Status onStatus(const char* data, size_t length) PURE; + /** * Called when headers are complete. A base routine happens first then a virtual dispatch is * invoked. Note that this only applies to headers and NOT trailers. End of diff --git a/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.cc b/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.cc index a27b35478f1e..5e1ee269286d 100644 --- a/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.cc +++ b/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.cc @@ -4,12 +4,17 @@ #include "envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.pb.validate.h" #include "envoy/registry/registry.h" +#include "source/common/protobuf/message_validator_impl.h" + namespace Envoy { namespace Extensions { namespace Http { namespace HeaderFormatters { namespace PreserveCase { +PreserveCaseHeaderFormatter::PreserveCaseHeaderFormatter(const bool forward_reason_phrase) + : forward_reason_phrase_(forward_reason_phrase) {} + std::string PreserveCaseHeaderFormatter::format(absl::string_view key) const { const auto remembered_key_itr = original_header_keys_.find(key); // TODO(mattklein123): We can avoid string copies here if the formatter interface allowed us @@ -34,12 +39,28 @@ void PreserveCaseHeaderFormatter::processKey(absl::string_view key) { original_header_keys_.emplace(key); } +void PreserveCaseHeaderFormatter::setReasonPhrase(absl::string_view reason_phrase) { + if (forward_reason_phrase_) { + reason_phrase_ = std::string(reason_phrase); + } +}; + +absl::string_view PreserveCaseHeaderFormatter::getReasonPhrase() const { + return absl::string_view(reason_phrase_); +}; + class PreserveCaseFormatterFactory : public Envoy::Http::StatefulHeaderKeyFormatterFactory { public: + PreserveCaseFormatterFactory(const bool forward_reason_phrase) + : forward_reason_phrase_(forward_reason_phrase) {} + // Envoy::Http::StatefulHeaderKeyFormatterFactory Envoy::Http::StatefulHeaderKeyFormatterPtr create() override { - return std::make_unique(); + return std::make_unique(forward_reason_phrase_); } + +private: + const bool forward_reason_phrase_; }; class PreserveCaseFormatterFactoryConfig @@ -47,10 +68,17 @@ class PreserveCaseFormatterFactoryConfig public: // Envoy::Http::StatefulHeaderKeyFormatterFactoryConfig std::string name() const override { return "preserve_case"; } + Envoy::Http::StatefulHeaderKeyFormatterFactorySharedPtr - createFromProto(const Protobuf::Message&) override { - return std::make_shared(); + createFromProto(const Protobuf::Message& message) override { + auto config = + MessageUtil::downcastAndValidate( + message, ProtobufMessage::getStrictValidationVisitor()); + + return std::make_shared(config.forward_reason_phrase()); } + ProtobufTypes::MessagePtr createEmptyConfigProto() override { return std::make_unique(); diff --git a/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.h b/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.h index d2e3f7da00ff..07ef7747d039 100644 --- a/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.h +++ b/source/extensions/http/header_formatters/preserve_case/preserve_case_formatter.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.pb.h" #include "envoy/http/header_formatter.h" #include "source/common/common/utility.h" @@ -13,11 +14,17 @@ namespace PreserveCase { class PreserveCaseHeaderFormatter : public Envoy::Http::StatefulHeaderKeyFormatter { public: // Envoy::Http::StatefulHeaderKeyFormatter + PreserveCaseHeaderFormatter(const bool forward_reason_phrase); + std::string format(absl::string_view key) const override; void processKey(absl::string_view key) override; + void setReasonPhrase(absl::string_view reason_phrase) override; + absl::string_view getReasonPhrase() const override; private: StringUtil::CaseUnorderedSet original_header_keys_; + bool forward_reason_phrase_{false}; + std::string reason_phrase_; }; } // namespace PreserveCase diff --git a/test/extensions/http/header_formatters/preserve_case/BUILD b/test/extensions/http/header_formatters/preserve_case/BUILD index f3998938cc5e..d8e102c206a1 100644 --- a/test/extensions/http/header_formatters/preserve_case/BUILD +++ b/test/extensions/http/header_formatters/preserve_case/BUILD @@ -35,3 +35,17 @@ envoy_extension_cc_test( "//test/integration:http_integration_lib", ], ) + +envoy_extension_cc_test( + name = "preserve_case_formatter_reason_phrase_integration_test", + srcs = [ + "preserve_case_formatter_reason_phrase_integration_test.cc", + ], + extension_names = ["envoy.http.stateful_header_formatters.preserve_case"], + deps = [ + "//source/extensions/http/header_formatters/preserve_case:preserve_case_formatter", + "//test/integration:http_integration_lib", + "//test/integration:http_protocol_integration_lib", + "@envoy_api//envoy/extensions/http/header_formatters/preserve_case/v3:pkg_cc_proto", + ], +) diff --git a/test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_reason_phrase_integration_test.cc b/test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_reason_phrase_integration_test.cc new file mode 100644 index 000000000000..d98a04a95e4e --- /dev/null +++ b/test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_reason_phrase_integration_test.cc @@ -0,0 +1,93 @@ +#include "envoy/extensions/http/header_formatters/preserve_case/v3/preserve_case.pb.h" + +#include "test/integration/filters/common.h" +#include "test/integration/http_integration.h" +#include "test/test_common/registry.h" + +namespace Envoy { +namespace { + +struct TestParams { + Network::Address::IpVersion ip_version; + bool forward_reason_phrase; +}; + +std::string testParamsToString(const ::testing::TestParamInfo& p) { + return fmt::format("{}_{}", + p.param.ip_version == Network::Address::IpVersion::v4 ? "IPv4" : "IPv6", + p.param.forward_reason_phrase ? "enabled" : "disabled"); +} + +std::vector getTestsParams() { + std::vector ret; + + for (auto ip_version : TestEnvironment::getIpVersionsForTest()) { + ret.push_back(TestParams{ip_version, true}); + ret.push_back(TestParams{ip_version, false}); + } + + return ret; +} + +class PreserveCaseFormatterReasonPhraseIntegrationTest : public testing::TestWithParam, + public HttpIntegrationTest { +public: + PreserveCaseFormatterReasonPhraseIntegrationTest() + : HttpIntegrationTest(Http::CodecType::HTTP1, GetParam().ip_version) {} + + void SetUp() override { + setDownstreamProtocol(Http::CodecType::HTTP1); + setUpstreamProtocol(Http::CodecType::HTTP1); + } + + void initialize() override { + if (upstreamProtocol() == Http::CodecType::HTTP1) { + config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) { + ConfigHelper::HttpProtocolOptions protocol_options; + auto typed_extension_config = protocol_options.mutable_explicit_http_config() + ->mutable_http_protocol_options() + ->mutable_header_key_format() + ->mutable_stateful_formatter(); + typed_extension_config->set_name("preserve_case"); + + auto config = + TestUtility::parseYaml(fmt::format( + "forward_reason_phrase: {}", GetParam().forward_reason_phrase ? "true" : "false")); + typed_extension_config->mutable_typed_config()->PackFrom(config); + + ConfigHelper::setProtocolOptions(*bootstrap.mutable_static_resources()->mutable_clusters(0), + protocol_options); + }); + } + + HttpIntegrationTest::initialize(); + } +}; + +INSTANTIATE_TEST_SUITE_P(CaseFormatter, PreserveCaseFormatterReasonPhraseIntegrationTest, + testing::ValuesIn(getTestsParams()), testParamsToString); + +TEST_P(PreserveCaseFormatterReasonPhraseIntegrationTest, VerifyReasonPhraseEnabled) { + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("http")); + auto request = "GET / HTTP/1.1\r\nhost: host\r\n\r\n"; + ASSERT_TRUE(tcp_client->write(request, false)); + + Envoy::FakeRawConnectionPtr upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(upstream_connection)); + + auto response = "HTTP/1.1 503 Slow Down\r\ncontent-length: 0\r\n\r\n"; + ASSERT_TRUE(upstream_connection->write(response)); + + auto expected_reason_phrase = + GetParam().forward_reason_phrase ? "Slow Down" : "Service Unavailable"; + // Verify that the downstream response has proper reason phrase + tcp_client->waitForData(fmt::format("HTTP/1.1 503 {}", expected_reason_phrase), false); + + tcp_client->close(); +} + +} // namespace +} // namespace Envoy diff --git a/test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_test.cc b/test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_test.cc index 053425ec8835..ab24ed9de9ce 100644 --- a/test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_test.cc +++ b/test/extensions/http/header_formatters/preserve_case/preserve_case_formatter_test.cc @@ -9,7 +9,7 @@ namespace HeaderFormatters { namespace PreserveCase { TEST(PreserveCaseFormatterTest, All) { - PreserveCaseHeaderFormatter formatter; + PreserveCaseHeaderFormatter formatter(false); formatter.processKey("Foo"); formatter.processKey("Bar"); formatter.processKey("BAR"); @@ -22,6 +22,22 @@ TEST(PreserveCaseFormatterTest, All) { EXPECT_EQ("baz", formatter.format("baz")); } +TEST(PreserveCaseFormatterTest, ReasonPhraseEnabled) { + PreserveCaseHeaderFormatter formatter(true); + + formatter.setReasonPhrase(absl::string_view("Slow Down")); + + EXPECT_EQ("Slow Down", formatter.getReasonPhrase()); +} + +TEST(PreserveCaseFormatterTest, ReasonPhraseDisabled) { + PreserveCaseHeaderFormatter formatter(false); + + formatter.setReasonPhrase(absl::string_view("Slow Down")); + + EXPECT_TRUE(formatter.getReasonPhrase().empty()); +} + } // namespace PreserveCase } // namespace HeaderFormatters } // namespace Http