diff --git a/bazel/BUILD b/bazel/BUILD index 3d65037c2792..c78ff9fb1307 100644 --- a/bazel/BUILD +++ b/bazel/BUILD @@ -537,6 +537,11 @@ config_setting( define_values = {"FUZZING_ENGINE": "oss-fuzz"}, ) +config_setting( + name = "enable_new_http1_parser_in_integration_tests", + values = {"define": "use_new_http1_parser_in_integration_tests=true"}, +) + alias( name = "fuzzing_engine", actual = select({ diff --git a/bazel/envoy_build_system.bzl b/bazel/envoy_build_system.bzl index a0463ab770d0..a728e0230276 100644 --- a/bazel/envoy_build_system.bzl +++ b/bazel/envoy_build_system.bzl @@ -21,6 +21,7 @@ load( _envoy_select_enable_http3 = "envoy_select_enable_http3", _envoy_select_google_grpc = "envoy_select_google_grpc", _envoy_select_hot_restart = "envoy_select_hot_restart", + _envoy_select_new_http1_parser_in_integration_tests = "envoy_select_new_http1_parser_in_integration_tests", _envoy_select_wasm_cpp_tests = "envoy_select_wasm_cpp_tests", _envoy_select_wasm_rust_tests = "envoy_select_wasm_rust_tests", _envoy_select_wasm_v8 = "envoy_select_wasm_v8", @@ -206,6 +207,7 @@ def envoy_google_grpc_external_deps(): envoy_select_boringssl = _envoy_select_boringssl envoy_select_google_grpc = _envoy_select_google_grpc envoy_select_enable_http3 = _envoy_select_enable_http3 +envoy_select_new_http1_parser_in_integration_tests = _envoy_select_new_http1_parser_in_integration_tests envoy_select_hot_restart = _envoy_select_hot_restart envoy_select_wasm_cpp_tests = _envoy_select_wasm_cpp_tests envoy_select_wasm_rust_tests = _envoy_select_wasm_rust_tests diff --git a/bazel/envoy_select.bzl b/bazel/envoy_select.bzl index c1a16005bba5..0d144face654 100644 --- a/bazel/envoy_select.bzl +++ b/bazel/envoy_select.bzl @@ -87,3 +87,9 @@ def envoy_select_wasm_wasmtime(xs): "@envoy//bazel:wasm_wasmtime": xs, "//conditions:default": [], }) + +def envoy_select_new_http1_parser_in_integration_tests(xs): + return select({ + "@envoy//bazel:enable_new_http1_parser_in_integration_tests": xs, + "//conditions:default": [], + }) diff --git a/bazel/external/llhttp.BUILD b/bazel/external/llhttp.BUILD new file mode 100644 index 000000000000..e15a683a8f3d --- /dev/null +++ b/bazel/external/llhttp.BUILD @@ -0,0 +1,16 @@ +load("@rules_cc//cc:defs.bzl", "cc_library") + +licenses(["notice"]) # Apache 2 + +cc_library( + name = "llhttp", + srcs = [ + "src/api.c", + "src/http.c", + "src/llhttp.c", + ], + hdrs = ["include/llhttp.h"], + defines = ["LLHTTP_STRICT_MODE"], + includes = ["include"], + visibility = ["//visibility:public"], +) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 7d6734c90df8..656bf6480ae2 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -144,6 +144,7 @@ def envoy_dependencies(skip_targets = []): _com_github_nghttp2_nghttp2() _com_github_skyapm_cpp2sky() _com_github_nodejs_http_parser() + _com_github_nodejs_llhttp() _com_github_alibaba_hessian2_codec() _com_github_tencent_rapidjson() _com_github_nlohmann_json() @@ -483,6 +484,16 @@ def _com_github_nodejs_http_parser(): actual = "@com_github_nodejs_http_parser//:http_parser", ) +def _com_github_nodejs_llhttp(): + external_http_archive( + name = "com_github_nodejs_llhttp", + build_file = "@envoy//bazel/external:llhttp.BUILD", + ) + native.bind( + name = "llhttp", + actual = "@com_github_nodejs_llhttp//:llhttp", + ) + def _com_github_alibaba_hessian2_codec(): external_http_archive("com_github_alibaba_hessian2_codec") native.bind( diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 84bbecc32303..64f9faa1e655 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -490,6 +490,18 @@ REPOSITORY_LOCATIONS_SPEC = dict( release_date = "2020-07-10", cpe = "cpe:2.3:a:nodejs:node.js:*", ), + com_github_nodejs_llhttp = dict( + project_name = "llhttp", + project_desc = "Parser for HTTP messages written in C", + project_url = "https://github.com/nodejs/llhttp", + version = "6.0.3", + sha256 = "ef50b15176f417287bca34e5f16c76f4e751e6e842738ddb339b9765be835132", + strip_prefix = "llhttp-release-v{version}", + urls = ["https://github.com/nodejs/llhttp/archive/release/v{version}.tar.gz"], + use_category = ["dataplane_core"], + release_date = "2021-06-13", + cpe = "cpe:2.3:a:nodejs:node.js:*", + ), com_github_alibaba_hessian2_codec = dict( project_name = "hessian2-codec", project_desc = "hessian2-codec is a C++ library for hessian2 codec", diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 690dbd511dd8..417404c4b19d 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -309,6 +309,7 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then "--define" "deprecated_features=disabled" "--define" "tcmalloc=gperftools" "--define" "zlib=ng" + "--define" "use_new_http1_parser_in_integration_tests=true" "--@envoy//bazel:http3=False" "--@envoy//source/extensions/filters/http/kill_request:enabled" "--test_env=ENVOY_HAS_EXTRA_EXTENSIONS=true") diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2b4e64afe0fc..a353dc6999cb 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -93,6 +93,7 @@ New Features value is `true`, the unsupported http filter will be ignored by envoy. This is also same with unsupported http filter in the typed per filter config. For more information, please reference :ref:`HttpFilter `. +* http: added new HTTP/1.1 parser llhttp. This is off by default and can be enabled by setting the ``envoy.reloadable_features.enable_new_http1_parser`` runtime feature to true. * http: added :ref:`stripping trailing host dot from host header` support. * http: added support for :ref:`original IP detection extensions`. Two initial extensions were added, the :ref:`custom header ` extension and the diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index e801968ec533..2f52322f673b 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -36,6 +36,7 @@ envoy_cc_library( ":header_formatter_lib", ":legacy_parser_lib", ":parser_interface", + ":parser_lib", "//envoy/buffer:buffer_interface", "//envoy/common:scope_tracker_interface", "//envoy/http:codec_interface", @@ -116,3 +117,14 @@ envoy_cc_library( "//source/common/common:assert_lib", ], ) + +envoy_cc_library( + name = "parser_lib", + srcs = ["parser_impl.cc"], + hdrs = ["parser_impl.h"], + external_deps = ["llhttp"], + deps = [ + ":parser_interface", + "//source/common/common:assert_lib", + ], +) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 01ff8b69f439..523b23664d07 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -21,6 +21,7 @@ #include "source/common/http/headers.h" #include "source/common/http/http1/header_formatter.h" #include "source/common/http/http1/legacy_parser_impl.h" +#include "source/common/http/http1/parser_impl.h" #include "source/common/http/utility.h" #include "source/common/runtime/runtime_features.h" @@ -475,7 +476,11 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat []() -> void { /* TODO(adisuissa): Handle overflow watermark */ })), max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { output_buffer_->setWatermarks(connection.bufferLimit()); - parser_ = std::make_unique(type, this); + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.enable_new_http1_parser")) { + parser_ = std::make_unique(type, this); + } else { + parser_ = std::make_unique(type, this); + } } Status ConnectionImpl::completeLastHeader() { @@ -593,10 +598,10 @@ Http::Status ConnectionImpl::dispatch(Buffer::Instance& data) { } total_parsed += statusor_parsed.value(); - if (parser_->getStatus() != ParserStatus::Success) { + if (!parser_->isOk()) { // Parse errors trigger an exception in dispatchSlice so we are guaranteed to be paused at // this point. - ASSERT(parser_->getStatus() == ParserStatus::Paused); + ASSERT(parser_->isPaused()); break; } } @@ -668,6 +673,8 @@ Status ConnectionImpl::onHeaderValue(const char* data, size_t length) { } absl::string_view header_value{data, length}; + // TODO(5155): This can be removed when http_parser is removed. llhttp enables strict validation + // of HTTP header values. if (!Http::HeaderUtility::headerValueIsValid(header_value)) { ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value); error_code_ = Http::Code::BadRequest; @@ -776,6 +783,7 @@ StatusOr ConnectionImpl::onHeadersComplete() { return codecProtocolError("http/1.1 protocol error: unsupported transfer encoding"); } } + parser_->setHasContentLength(request_or_response_headers.ContentLength() != nullptr); auto statusor = onHeadersCompleteBase(); if (!statusor.ok()) { @@ -800,8 +808,7 @@ void ConnectionImpl::bufferBody(const char* data, size_t length) { } void ConnectionImpl::dispatchBufferedBody() { - ASSERT(parser_->getStatus() == ParserStatus::Success || - parser_->getStatus() == ParserStatus::Paused); + ASSERT(parser_->isOk() || parser_->isPaused()); ASSERT(codec_status_.ok()); if (buffered_body_.length() > 0) { onBody(buffered_body_); diff --git a/source/common/http/http1/legacy_parser_impl.cc b/source/common/http/http1/legacy_parser_impl.cc index 840ba419099f..b2c988018ce5 100644 --- a/source/common/http/http1/legacy_parser_impl.cc +++ b/source/common/http/http1/legacy_parser_impl.cc @@ -10,26 +10,6 @@ namespace Envoy { namespace Http { namespace Http1 { -namespace { -ParserStatus intToStatus(int rc) { - // See - // https://github.com/nodejs/http-parser/blob/5c5b3ac62662736de9e71640a8dc16da45b32503/http_parser.h#L72. - switch (rc) { - case -1: - return ParserStatus::Error; - case 0: - return ParserStatus::Success; - case 1: - return ParserStatus::NoBody; - case 2: - return ParserStatus::NoBodyData; - case 31: - return ParserStatus::Paused; - default: - return ParserStatus::Unknown; - } -} -} // namespace class LegacyHttpParserImpl::Impl { public: @@ -160,7 +140,9 @@ void LegacyHttpParserImpl::resume() { impl_->resume(); } ParserStatus LegacyHttpParserImpl::pause() { return impl_->pause(); } -ParserStatus LegacyHttpParserImpl::getStatus() { return intToStatus(impl_->getErrno()); } +bool LegacyHttpParserImpl::isOk() { return impl_->getErrno() == HPE_OK; } + +bool LegacyHttpParserImpl::isPaused() { return impl_->getErrno() == HPE_PAUSED; } uint16_t LegacyHttpParserImpl::statusCode() const { return impl_->statusCode(); } diff --git a/source/common/http/http1/legacy_parser_impl.h b/source/common/http/http1/legacy_parser_impl.h index 2099f815824b..e643a713fb00 100644 --- a/source/common/http/http1/legacy_parser_impl.h +++ b/source/common/http/http1/legacy_parser_impl.h @@ -17,11 +17,13 @@ class LegacyHttpParserImpl : public Parser { RcVal execute(const char* data, int len) override; void resume() override; ParserStatus pause() override; - ParserStatus getStatus() override; + bool isOk() override; + bool isPaused() override; uint16_t statusCode() const override; int httpMajor() const override; int httpMinor() const override; absl::optional contentLength() const override; + void setHasContentLength(bool) override{}; bool isChunked() const override; absl::string_view methodName() const override; absl::string_view errnoName(int rc) const override; diff --git a/source/common/http/http1/parser.h b/source/common/http/http1/parser.h index 23f6795a1f75..1842c7e298f7 100644 --- a/source/common/http/http1/parser.h +++ b/source/common/http/http1/parser.h @@ -123,8 +123,11 @@ class Parser { // Pauses the parser and returns a status indicating pause. virtual ParserStatus pause() PURE; - // Returns a parser status representing the errno value from the parser. - virtual ParserStatus getStatus() PURE; + // Checks if the parser status is OK. + virtual bool isOk() PURE; + + // Check is the parser status is paused. + virtual bool isPaused() PURE; // Returns an integer representing the status code stored in the parser structure. For responses // only. @@ -140,6 +143,10 @@ class Parser { // Returns the number of bytes in the body. absl::nullopt if no Content-Length header virtual absl::optional contentLength() const PURE; + // Indicated that Content-Length header is present. This is used to differentiate between an unset + // Content-Length and a 0 value. + virtual void setHasContentLength(bool val) PURE; + // Returns whether headers are chunked. virtual bool isChunked() const PURE; diff --git a/source/common/http/http1/parser_impl.cc b/source/common/http/http1/parser_impl.cc new file mode 100644 index 000000000000..b5689ab34ab2 --- /dev/null +++ b/source/common/http/http1/parser_impl.cc @@ -0,0 +1,207 @@ +#include "source/common/http/http1/parser_impl.h" + +#include + +#include + +#include "source/common/common/assert.h" +#include "source/common/http/http1/parser.h" + +namespace Envoy { +namespace Http { +namespace Http1 { + +class HttpParserImpl::Impl { +public: + Impl(llhttp_type_t type) { + llhttp_init(&parser_, type, &settings_); + llhttp_set_lenient_chunked_length(&parser_, 1); + } + + Impl(llhttp_type_t type, void* data) : Impl(type) { + parser_.data = data; + settings_ = { + [](llhttp_t* parser) -> int { + auto* conn_impl = static_cast(parser->data); + auto status = conn_impl->onMessageBegin(); + return conn_impl->setAndCheckCallbackStatus(std::move(status)); + }, + [](llhttp_t* parser, const char* at, size_t length) -> int { + auto* conn_impl = static_cast(parser->data); + auto status = conn_impl->onUrl(at, length); + return conn_impl->setAndCheckCallbackStatus(std::move(status)); + }, + nullptr, // on_status + [](llhttp_t* parser, const char* at, size_t length) -> int { + auto* conn_impl = static_cast(parser->data); + auto status = conn_impl->onHeaderField(at, length); + return conn_impl->setAndCheckCallbackStatus(std::move(status)); + }, + [](llhttp_t* parser, const char* at, size_t length) -> int { + auto* conn_impl = static_cast(parser->data); + auto status = conn_impl->onHeaderValue(at, length); + return conn_impl->setAndCheckCallbackStatus(std::move(status)); + }, + [](llhttp_t* parser) -> int { + auto* conn_impl = static_cast(parser->data); + auto statusor = conn_impl->onHeadersComplete(); + return conn_impl->setAndCheckCallbackStatusOr(std::move(statusor)); + }, + [](llhttp_t* parser, const char* at, size_t length) -> int { + static_cast(parser->data)->bufferBody(at, length); + return 0; + }, + [](llhttp_t* parser) -> int { + auto* conn_impl = static_cast(parser->data); + auto status = conn_impl->onMessageComplete(); + return conn_impl->setAndCheckCallbackStatusOr(std::move(status)); + }, + [](llhttp_t* parser) -> int { + // A 0-byte chunk header is used to signal the end of the chunked body. + // When this function is called, http-parser holds the size of the chunk in + // parser->content_length. See + // https://github.com/nodejs/http-parser/blob/v2.9.3/http_parser.h#L336 + const bool is_final_chunk = (parser->content_length == 0); + static_cast(parser->data)->onChunkHeader(is_final_chunk); + return 0; + }, + nullptr, // on_chunk_complete + nullptr, // on_url_complete + nullptr, // on_status_complete + nullptr, // on_header_field_complete + nullptr // on_header_value_complete + }; + } + + RcVal execute(const char* slice, int len) { + llhttp_errno_t error; + if (slice == nullptr || len == 0) { + error = llhttp_finish(&parser_); + } else { + error = llhttp_execute(&parser_, slice, len); + } + size_t nread = len; + // Adjust number of bytes read in case of error. + if (error != HPE_OK) { + nread = llhttp_get_error_pos(&parser_) - slice; + // Resume after upgrade. + if (error == HPE_PAUSED_UPGRADE) { + error = HPE_OK; + llhttp_resume_after_upgrade(&parser_); + } + } + return {nread, error}; + } + + void resume() { llhttp_resume(&parser_); } + + ParserStatus pause() { + // llhttp can only pause by returning a paused status in user callbacks. + return ParserStatus::Paused; + } + + int getErrno() { return llhttp_get_errno(&parser_); } + + uint16_t statusCode() const { return parser_.status_code; } + + int httpMajor() const { return parser_.http_major; } + + int httpMinor() const { return parser_.http_minor; } + + absl::optional contentLength() const { + if (has_content_length_) { + return parser_.content_length; + } + return absl::nullopt; + } + + void setHasContentLength(bool val) { has_content_length_ = val; } + + bool isChunked() const { return parser_.flags & F_CHUNKED; } + + absl::string_view methodName() const { + return llhttp_method_name(static_cast(parser_.method)); + } + + int hasTransferEncoding() const { return parser_.flags & F_TRANSFER_ENCODING; } + +private: + llhttp_t parser_; + llhttp_settings_s settings_; + bool has_content_length_{true}; +}; + +HttpParserImpl::HttpParserImpl(MessageType type, ParserCallbacks* data) { + llhttp_type_t parser_type; + switch (type) { + case MessageType::Request: + parser_type = HTTP_REQUEST; + break; + case MessageType::Response: + parser_type = HTTP_RESPONSE; + break; + default: + NOT_IMPLEMENTED_GCOVR_EXCL_LINE; + } + + impl_ = std::make_unique(parser_type, data); +} + +// Because we have a pointer-to-impl using std::unique_ptr, we must place the destructor in the +// same compilation unit so that the destructor has a complete definition of Impl. +HttpParserImpl::~HttpParserImpl() = default; + +HttpParserImpl::RcVal HttpParserImpl::execute(const char* slice, int len) { + return impl_->execute(slice, len); +} + +void HttpParserImpl::resume() { impl_->resume(); } + +ParserStatus HttpParserImpl::pause() { return impl_->pause(); } + +bool HttpParserImpl::isOk() { return impl_->getErrno() == HPE_OK; } + +bool HttpParserImpl::isPaused() { return impl_->getErrno() == HPE_PAUSED; } + +uint16_t HttpParserImpl::statusCode() const { return impl_->statusCode(); } + +int HttpParserImpl::httpMajor() const { return impl_->httpMajor(); } + +int HttpParserImpl::httpMinor() const { return impl_->httpMinor(); } + +absl::optional HttpParserImpl::contentLength() const { return impl_->contentLength(); } + +void HttpParserImpl::setHasContentLength(bool val) { return impl_->setHasContentLength(val); } + +bool HttpParserImpl::isChunked() const { return impl_->isChunked(); } + +absl::string_view HttpParserImpl::methodName() const { return impl_->methodName(); } + +absl::string_view HttpParserImpl::errnoName(int rc) const { + return llhttp_errno_name(static_cast(rc)); +} + +int HttpParserImpl::hasTransferEncoding() const { return impl_->hasTransferEncoding(); } + +int HttpParserImpl::statusToInt(const ParserStatus code) const { + // See + // https://github.com/nodejs/llhttp/blob/a620012f3fd1b64ace16d31c52cd57b97ee7174c/src/native/api.h#L29-L36 + switch (code) { + case ParserStatus::Error: + return HPE_USER; + case ParserStatus::Success: + return HPE_OK; + case ParserStatus::NoBody: + return 1; + case ParserStatus::NoBodyData: + return 2; + case ParserStatus::Paused: + return HPE_PAUSED; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +} // namespace Http1 +} // namespace Http +} // namespace Envoy diff --git a/source/common/http/http1/parser_impl.h b/source/common/http/http1/parser_impl.h new file mode 100644 index 000000000000..3c1d9fc37d53 --- /dev/null +++ b/source/common/http/http1/parser_impl.h @@ -0,0 +1,43 @@ +#pragma once + +#include + +#include "source/common/http/http1/parser.h" + +namespace Envoy { +namespace Http { +namespace Http1 { + +class HttpParserImpl : public Parser { +public: + HttpParserImpl(MessageType type, ParserCallbacks* data); + ~HttpParserImpl() override; + + // Http1::Parser + RcVal execute(const char* data, int len) override; + void resume() override; + ParserStatus pause() override; + bool isOk() override; + bool isPaused() override; + uint16_t statusCode() const override; + int httpMajor() const override; + int httpMinor() const override; + absl::optional contentLength() const override; + void setHasContentLength(bool val) override; + bool isChunked() const override; + absl::string_view methodName() const override; + absl::string_view errnoName(int rc) const override; + int hasTransferEncoding() const override; + int statusToInt(const ParserStatus code) const override; + +private: + // TODO(5155): This secondary layer with a private class can be removed after http-parser is + // removed. This layer avoids colliding symbols between the two libraries by isolating the + // libraries in separate compilation units. + class Impl; + std::unique_ptr impl_; +}; + +} // namespace Http1 +} // namespace Http +} // namespace Envoy diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 546fc4e8eda9..14c4ad7cb3f8 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -110,6 +110,8 @@ constexpr const char* runtime_features[] = { constexpr const char* disabled_runtime_features[] = { // v2 is fatal-by-default. "envoy.test_only.broken_in_production.enable_deprecated_v2_api", + // TODO(asraa): Flip to true after staging. + "envoy.reloadable_features.enable_new_http1_parser", // TODO(asraa) flip to true in a separate PR to enable the new JSON by default. "envoy.reloadable_features.remove_legacy_json", // Sentinel and test flag. diff --git a/test/common/http/BUILD b/test/common/http/BUILD index df4b5763d249..7b4d81ec208d 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -95,6 +95,7 @@ envoy_cc_fuzz_test( "//test/fuzz:utility_lib", "//test/mocks/http:http_mocks", "//test/mocks/network:network_mocks", + "//test/test_common:test_runtime_lib", ], ) @@ -402,22 +403,31 @@ envoy_cc_test( ], ) +MIXED_CONN_POOL_TEST = [ + ":common_lib", + "//source/common/http:mixed_conn_pool", + "//test/common/upstream:utility_lib", + "//test/mocks:common_lib", + "//test/mocks/buffer:buffer_mocks", + "//test/mocks/http:http_mocks", + "//test/mocks/local_info:local_info_mocks", + "//test/mocks/network:connection_mocks", + "//test/mocks/router:router_mocks", + "//test/mocks/runtime:runtime_mocks", + "//test/mocks/stats:stats_mocks", +] + envoy_cc_test( name = "mixed_conn_pool_test", srcs = ["mixed_conn_pool_test.cc"], - deps = [ - ":common_lib", - "//source/common/http:mixed_conn_pool", - "//test/common/upstream:utility_lib", - "//test/mocks:common_lib", - "//test/mocks/buffer:buffer_mocks", - "//test/mocks/http:http_mocks", - "//test/mocks/local_info:local_info_mocks", - "//test/mocks/network:connection_mocks", - "//test/mocks/router:router_mocks", - "//test/mocks/runtime:runtime_mocks", - "//test/mocks/stats:stats_mocks", - ], + deps = MIXED_CONN_POOL_TEST, +) + +envoy_cc_test( + name = "legacy_mixed_conn_pool_test", + srcs = ["mixed_conn_pool_test.cc"], + args = ["--runtime-feature-override-for-tests=envoy.reloadable_features.enable_new_http1_parser"], + deps = MIXED_CONN_POOL_TEST, ) envoy_cc_test( diff --git a/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5689430000795648 b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5689430000795648 new file mode 100644 index 000000000000..e1115defee29 --- /dev/null +++ b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5689430000795648 @@ -0,0 +1,62 @@ +actions { + new_stream { + request_headers { + headers { + key: ":method" + value: "GET" + } + headers { + key: ":path" + value: "/" + } + } + } +} +actions { + quiesce_drain { + } +} +actions { + stream_action { + request { + data: 54 + } + dispatching_action { + data: 2686976 + } + } +} +actions { + stream_action { + response { + headers { + } + } + } +} +actions { + stream_action { + request { + trailers { + } + } + } +} +actions { + stream_action { + response { + trailers { + } + } + } +} +actions { + new_stream { + request_headers { + headers { + key: ":method" + value: "GET" + } + } + } +} \ No newline at end of file diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index 61e8bbd736ce..b60d11525e4c 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -16,7 +16,9 @@ #include "source/common/http/http1/codec_impl.h" #include "source/common/http/http2/codec_impl.h" #include "source/common/http/conn_manager_utility.h" +#include "source/common/runtime/runtime_impl.h" +#include "test/test_common/test_runtime.h" #include "test/common/http/codec_impl_fuzz.pb.validate.h" #include "test/common/http/http2/codec_impl_test_util.h" #include "test/fuzz/fuzz_runner.h" @@ -61,6 +63,7 @@ Http1Settings fromHttp1Settings(const test::common::http::Http1ServerSettings& s h1_settings.allow_absolute_url_ = settings.allow_absolute_url(); h1_settings.accept_http_10_ = settings.accept_http_10(); h1_settings.default_host_for_http_10_ = settings.default_host_for_http_10(); + h1_settings.enable_trailers_ = true; return h1_settings; } @@ -183,11 +186,20 @@ class HttpStream : public LinkedObject { request_.closeRemote(); })); ON_CALL(response_.response_decoder_, decodeHeaders_(_, true)) - .WillByDefault(InvokeWithoutArgs([this] { response_.closeLocalAndRemote(); })); + .WillByDefault(InvokeWithoutArgs([this] { + response_.closeLocalAndRemote(); + response_.stream_callbacks_.onResetStream(StreamResetReason::RemoteReset, "stream reset"); + })); ON_CALL(response_.response_decoder_, decodeData(_, true)) - .WillByDefault(InvokeWithoutArgs([this] { response_.closeLocalAndRemote(); })); + .WillByDefault(InvokeWithoutArgs([this] { + response_.closeLocalAndRemote(); + response_.stream_callbacks_.onResetStream(StreamResetReason::RemoteReset, "stream reset"); + })); ON_CALL(response_.response_decoder_, decodeTrailers_(_)) - .WillByDefault(InvokeWithoutArgs([this] { response_.closeLocalAndRemote(); })); + .WillByDefault(InvokeWithoutArgs([this] { + response_.closeLocalAndRemote(); + response_.stream_callbacks_.onResetStream(StreamResetReason::RemoteReset, "stream reset"); + })); if (!end_stream) { request_.request_encoder_->getStream().addCallbacks(request_.stream_callbacks_); } @@ -692,8 +704,15 @@ DEFINE_PROTO_FUZZER(const test::common::http::CodecImplFuzzTestCase& input) { try { // Validate input early. TestUtility::validate(input); - codecFuzz(input, HttpVersion::Http1); + // Test new and legacy HTTP/1.1 parsers. + for (const auto& value : {"false", "true"}) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.enable_new_http1_parser", value}}); + codecFuzz(input, HttpVersion::Http1); + } codecFuzz(input, HttpVersion::Http2); + } catch (const EnvoyException& e) { ENVOY_LOG_MISC(debug, "EnvoyException: {}", e.what()); } diff --git a/test/common/http/http1/BUILD b/test/common/http/http1/BUILD index 200909ceeb1e..5e23db953195 100644 --- a/test/common/http/http1/BUILD +++ b/test/common/http/http1/BUILD @@ -16,28 +16,45 @@ envoy_cc_test( ], ) +CODEC_DEPS = [ + "//envoy/buffer:buffer_interface", + "//envoy/event:dispatcher_interface", + "//source/common/buffer:buffer_lib", + "//source/common/event:dispatcher_lib", + "//source/common/http:exception_lib", + "//source/common/http:header_map_lib", + "//source/common/http/http1:codec_lib", + "//test/common/stats:stat_test_utility_lib", + "//test/mocks/buffer:buffer_mocks", + "//test/mocks/http:http_mocks", + "//test/mocks/init:init_mocks", + "//test/mocks/local_info:local_info_mocks", + "//test/mocks/network:network_mocks", + "//test/mocks/protobuf:protobuf_mocks", + "//test/mocks/thread_local:thread_local_mocks", + "//test/mocks/upstream:upstream_mocks", + "//test/test_common:logging_lib", + "//test/test_common:test_runtime_lib", +] + +envoy_cc_test( + name = "legacy_codec_impl_test", + srcs = ["codec_impl_test.cc"], + deps = CODEC_DEPS, +) + envoy_cc_test( name = "codec_impl_test", srcs = ["codec_impl_test.cc"], + args = ["--runtime-feature-override-for-tests=envoy.reloadable_features.enable_new_http1_parser"], + deps = CODEC_DEPS, +) + +envoy_cc_test( + name = "parser_impl_test", + srcs = ["parser_impl_test.cc"], deps = [ - "//envoy/buffer:buffer_interface", - "//envoy/event:dispatcher_interface", - "//source/common/buffer:buffer_lib", - "//source/common/event:dispatcher_lib", - "//source/common/http:exception_lib", - "//source/common/http:header_map_lib", - "//source/common/http/http1:codec_lib", - "//test/common/stats:stat_test_utility_lib", - "//test/mocks/buffer:buffer_mocks", - "//test/mocks/http:http_mocks", - "//test/mocks/init:init_mocks", - "//test/mocks/local_info:local_info_mocks", - "//test/mocks/network:network_mocks", - "//test/mocks/protobuf:protobuf_mocks", - "//test/mocks/thread_local:thread_local_mocks", - "//test/mocks/upstream:upstream_mocks", - "//test/test_common:logging_lib", - "//test/test_common:test_runtime_lib", + "//source/common/http/http1:parser_lib", ], ) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 59f8e1dc0dd1..88290471da50 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1109,12 +1109,9 @@ TEST_F(Http1ServerConnectionImplTest, HostHeaderTranslation) { EXPECT_EQ(0U, buffer.length()); } -// Ensures that requests with invalid HTTP header values are properly rejected -// when the runtime guard is enabled for the feature. +// Ensures that requests with invalid HTTP header values are properly rejected. TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRejection) { TestScopedRuntime scoped_runtime; - // When the runtime-guarded feature is enabled, invalid header values - // should result in a rejection. initialize(); @@ -1130,8 +1127,14 @@ TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRejection) { EXPECT_CALL(decoder, sendLocalReply(_, _, _, _, _)); auto status = codec_->dispatch(buffer); EXPECT_TRUE(isCodecProtocolError(status)); - EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars"); - EXPECT_EQ("http1.invalid_characters", response_encoder->getStream().responseDetails()); + // Invalid header characters are handled by llhttp directly. + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.enable_new_http1_parser")) { + EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_HEADER_TOKEN"); + EXPECT_EQ("http1.codec_error", response_encoder->getStream().responseDetails()); + } else { + EXPECT_EQ(status.message(), "http/1.1 protocol error: header value contains invalid chars"); + EXPECT_EQ("http1.invalid_characters", response_encoder->getStream().responseDetails()); + } } // Ensures that request headers with names containing the underscore character are allowed diff --git a/test/common/http/http1/parser_impl_test.cc b/test/common/http/http1/parser_impl_test.cc new file mode 100644 index 000000000000..5cc918f76192 --- /dev/null +++ b/test/common/http/http1/parser_impl_test.cc @@ -0,0 +1,93 @@ +#include "source/common/http/http1/parser_impl.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Http { +namespace Http1 { +namespace { + +// No-op parser callbacks. +class TestParserCallbacks : public ParserCallbacks { + Status onMessageBegin() override { return okStatus(); }; + Status onUrl(const char*, size_t) override { return okStatus(); }; + Status onHeaderField(const char*, size_t) override { return okStatus(); }; + Status onHeaderValue(const char*, size_t) override { return okStatus(); }; + Envoy::StatusOr onHeadersComplete() override { return ParserStatus::Success; }; + void bufferBody(const char*, size_t) override{}; + StatusOr onMessageComplete() override { return ParserStatus::Success; }; + void onChunkHeader(bool) override{}; + + int setAndCheckCallbackStatus(Status&&) override { return 0; }; + int setAndCheckCallbackStatusOr(Envoy::StatusOr&&) override { return 0; }; +}; + +class RequestParserImplTest : public testing::Test { +public: + RequestParserImplTest() { + parser_ = std::make_unique(MessageType::Request, &callbacks_); + } + TestParserCallbacks callbacks_; + std::unique_ptr parser_; +}; + +TEST_F(RequestParserImplTest, TestExecute) { + const char* request = "GET / HTTP/1.1\r\n\r\n"; + int request_len = strlen(request); + + auto [nread, rc] = parser_->execute(request, request_len); + EXPECT_EQ(rc, 0); + EXPECT_EQ(parser_->methodName(), "GET"); + EXPECT_EQ(parser_->httpMajor(), 1); + EXPECT_EQ(parser_->httpMinor(), 1); +} + +TEST_F(RequestParserImplTest, TestContentLength) { + const char* request = "POST / HTTP/1.1\r\nContent-Length: 003\r\n\r\n"; + int request_len = strlen(request); + + auto [nread, rc] = parser_->execute(request, request_len); + EXPECT_EQ(rc, 0); + EXPECT_TRUE(parser_->contentLength().has_value()); + EXPECT_EQ(parser_->contentLength().value(), 3); +} + +TEST_F(RequestParserImplTest, TestErrorName) { + // Duplicate Content-Length causes error. + const char* request = "POST / HTTP/1.1\r\nContent-Length: 003\r\nContent-Length: 001\r\n"; + int request_len = strlen(request); + auto [nread, rc] = parser_->execute(request, request_len); + EXPECT_NE(rc, 0); + EXPECT_EQ(parser_->errnoName(rc), "HPE_UNEXPECTED_CONTENT_LENGTH"); +} + +TEST_F(RequestParserImplTest, TestChunked) { + const char* request = "POST / HTTP/1.1\r\nTransfer-Encoding: chunked\r\n\r\na\r\n0\r\n\r\n"; + int request_len = strlen(request); + auto [nread, rc] = parser_->execute(request, request_len); + EXPECT_EQ(rc, 0); + EXPECT_TRUE(parser_->hasTransferEncoding()); + EXPECT_TRUE(parser_->isChunked()); +} + +class ResponseParserImplTest : public testing::Test { +public: + ResponseParserImplTest() { + parser_ = std::make_unique(MessageType::Response, &callbacks_); + } + TestParserCallbacks callbacks_; + std::unique_ptr parser_; +}; + +TEST_F(ResponseParserImplTest, TestStatus) { + const char* response = "HTTP/1.1 200 OK\r\n\r\n"; + int response_len = strlen(response); + auto [nread, rc] = parser_->execute(response, response_len); + EXPECT_EQ(rc, 0); + EXPECT_EQ(parser_->statusCode(), 200); +} + +} // namespace +} // namespace Http1 +} // namespace Http +} // namespace Envoy diff --git a/test/integration/BUILD b/test/integration/BUILD index 5d4083e23c05..c8ade16ae000 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -9,6 +9,7 @@ load( "envoy_proto_library", "envoy_select_enable_http3", "envoy_select_hot_restart", + "envoy_select_new_http1_parser_in_integration_tests", "envoy_sh_test", ) @@ -727,6 +728,9 @@ envoy_cc_test_library( hdrs = [ "base_integration_test.h", ], + copts = envoy_select_new_http1_parser_in_integration_tests( + ["-DENVOY_USE_NEW_HTTP1_PARSER_IN_INTEGRATION_TESTS"], + ), deps = [ ":autonomous_upstream_lib", ":fake_upstream_lib", diff --git a/test/integration/base_integration_test.cc b/test/integration/base_integration_test.cc index d74b71962a03..62bcfa046c4a 100644 --- a/test/integration/base_integration_test.cc +++ b/test/integration/base_integration_test.cc @@ -53,6 +53,9 @@ BaseIntegrationTest::BaseIntegrationTest(const InstanceConstSharedPtrFn& upstrea version_(version), upstream_address_fn_(upstream_address_fn), config_helper_(version, *api_, config), default_log_level_(TestEnvironment::getOptions().logLevel()) { +#ifdef ENVOY_USE_NEW_HTTP1_PARSER_IN_INTEGRATION_TESTS + config_helper_.addRuntimeOverride("envoy.reloadable_features.enable_new_http1_parser", "true"); +#endif // This is a hack, but there are situations where we disconnect fake upstream connections and // then we expect the server connection pool to get the disconnect before the next test starts. // This does not always happen. This pause should allow the server to pick up the disconnect diff --git a/tools/dependency/utils.py b/tools/dependency/utils.py index e47107fe1463..1f4424140229 100644 --- a/tools/dependency/utils.py +++ b/tools/dependency/utils.py @@ -39,11 +39,11 @@ def get_github_release_from_urls(urls): if components[5] == 'archive': # Only support .tar.gz, .zip today. Figure out the release tag from this # filename. - if components[6].endswith('.tar.gz'): - github_version = components[6][:-len('.tar.gz')] + if components[-1].endswith('.tar.gz'): + github_version = components[-1][:-len('.tar.gz')] else: - assert (components[6].endswith('.zip')) - github_version = components[6][:-len('.zip')] + assert (components[-1].endswith('.zip')) + github_version = components[-1][:-len('.zip')] else: # Release tag is a path component. assert (components[5] == 'releases') diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 6291ed0b42e9..60430620ca76 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -769,6 +769,7 @@ linearize linearized linux livelock +llhttp llvm loc localhost