[WIP] use llhttp as default HTTP parser #9033

derekargueta · 2019-11-14T20:20:21Z

Description: Uses llhttp to parse HTTP/1.1 traffic. A compile-time switch is provided to use the legacy Node http-parser. Preprocessor macros were selected due to global naming conflicts between the two libraries. The alternative to make it a runtime switch would require #includeing the libraries within separate namespaces (I can backtrack to this option if it's more desirable).
TODO

resolve lenient_http_headers issue as mentioned on the ticket
fix BadRequestNoStream test to not open a new stream

There are other places that still use the http-parser, notably the http_inspector network filter and the url parsing utilities.

Risk Level: medium
Testing: existing
Docs Changes:
Release Notes:
Fixes #5155
Deprecated: http-parser

Signed-off-by: Derek Argueta <dereka@pinterest.com>

indutny · 2019-11-14T21:02:01Z

Fantastic work, @derekargueta ! I'm curious if this will bring any noticeable performance improvements to envoyproxy!

indutny · 2019-11-14T21:07:32Z

source/common/http/http1/codec_impl.cc

    sendProtocolError();
    throw CodecProtocolException("http/1.1 protocol error: " +
-                                 std::string(http_errno_name(HTTP_PARSER_ERRNO(&parser_))));
+                                 std::string(parser_errno_name(parser_get_errno(&parser_))));


There is a new opportunity for detailed errors here. One could potentially use llhttp_get_error_reason to get the detailed reason for the error.

lizan · 2019-11-14T21:56:21Z

source/common/http/http1/BUILD

@@ -2,6 +2,7 @@ licenses(["notice"])  # Apache 2

 load(
    "//bazel:envoy_build_system.bzl",
+    "envoy_cc_binary",


lizan · 2019-11-14T22:01:38Z

source/common/http/http1/codec_impl.cc

  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.
    ASSERT(!deferred_end_stream_headers_);
    ENVOY_CONN_LOG(trace, "Pausing parser due to upgrade.", connection_);
+#ifdef ENVOY_ENABLE_LEGACY_HTTP_PARSER


can we always return HPE_PAUSED and let the calling site call http_parser_pause? i.e. here: https://github.com/envoyproxy/envoy/pull/9033/files#diff-2ba39613a1b78b994ec31ddd03cf42f5R342

same for other places.

lizan · 2019-11-14T22:04:41Z

source/common/http/http1/parser_adapter.h

+    nread = llhttp_get_error_pos(parser) - slice;
+    if (err == HPE_PAUSED_UPGRADE) {
+      err = HPE_OK;
+      llhttp_resume_after_upgrade(parser);


we don't always handle upgrade today? don't we need to check handling_upgrade_ here before resume?

htuch · 2019-11-14T22:34:53Z

@envoyproxy/security-team for review as well.

mattklein123 · 2019-11-15T01:03:52Z

@derekargueta awesome stuff. I put this comment in the issue, but is it possible to compile both in for a period of time so we can feature flag which parser to use?

/wait-any

alyssawilk · 2019-11-18T18:43:48Z

+1 to what Matt said. I'm super excited for the switch and it'd be even better if we can runtime switch this for a release or two!

indutny · 2019-11-18T18:50:37Z

FWIW, I've created a PR to introduce lenient parsing to llhttp: nodejs/llhttp#33 .

derekargueta · 2019-11-18T19:22:26Z

@mattklein123 @alyssawilk yeah I started working on a runtime switch. The catch is that llhttp.h and http_parser.h can't be in the same compilation unit due to conflicting enum definitions (ODR \o/) so the solution I have working so far uses the pimpl pattern to contain those libraries in source files. I'm a bit concerned about introducing pointer-indirection in the hot path, but am planning on writing a few benchmark tests to justify the change. Doesn't seem we have codec benchmark tests yet, and it could also give perf feedback on the difference between the two libraries. If I'm going overboard or there's a better pattern I'd be happy to hear it 🙂

lizan · 2019-11-18T21:21:48Z

@derekargueta from a quick glance it doesn't seems have conflicting enum between llhttp and http_parser, seems everything in llhttp is prefixed with llhttp_. I don't think pimpl solves the ODR because you still link them together at the end.

derekargueta · 2019-11-18T21:52:49Z

@lizan sorry I misspoke, it's not the enum's themselves (which you are correct, llhttp is prefixed with llhttp_) but the enum values since they are plain C enums and not C++ class-enums.

When the 2 are included into the same context (directly or transitively):

...
In file included from source/common/http/http1/comparison.cc:7:
external/com_github_nodejs_http_parser/http_parser.h:210:19: error: redefinition of enumerator 'HTTP_REBIND'
  HTTP_METHOD_MAP(XX)
                  ^
external/com_github_nodejs_llhttp/include/llhttp.h:124:3: note: previous definition is here
  HTTP_REBIND = 17,
  ^
In file included from source/common/http/http1/comparison.cc:7:
external/com_github_nodejs_http_parser/http_parser.h:210:19: error: redefinition of enumerator 'HTTP_UNBIND'
  HTTP_METHOD_MAP(XX)
                  ^
external/com_github_nodejs_llhttp/include/llhttp.h:125:3: note: previous definition is here
  HTTP_UNBIND = 18,
  ^

jmarantz · 2019-11-18T23:04:05Z

Can we change llhttp to prefix it's enum values? I am not sure that is necessary though.

We should have an abstraction layer with its own enum to enable the switch, right?

lizan · 2019-11-19T17:39:42Z

@derekargueta thanks that makes sense, yeah then you need two classes implement same interface in different compilation unit to make it work.

stale · 2019-11-26T18:15:17Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta · 2019-11-27T06:22:01Z

Still very much a WIP, not yet ready for full review but high-level comments welcome. Wrestling with getting the static-casting right from void* to ParserCallbacks*, then likely debugging a few tests, and cleaning up misc comments & cruft. Also still massaging the abstractions a bit.

This introduces quite a few abstractions with the aim of completely decoupling the HTTP parser from the codec/connectionimpl which is necessary for the 2 libraries to coexist. Main components are

ParserCallbacks, an interface the {Server|Client}ConnectionImpl classes implement to hook into the parser callbacks
ParserFactory for constructing the parser
Parser/LlHttpParserImpl/LegacyHttpParserImpl which is where the PImpl pattern mentioned earlier is being used to encapsulate the libraries in separate compilation units.

Also keeping an eye on #8667 which looks like will have conflicts with this PR

Signed-off-by: Derek Argueta <dereka@pinterest.com>

alyssawilk

Huzzah for progress!
some high level comments while you're doing clean up :-)

alyssawilk · 2019-12-02T15:05:42Z

docs/root/intro/version_history.rst

@@ -12,6 +12,7 @@ Version history
 * health check: gRPC health checker sets the gRPC deadline to the configured timeout duration.
 * 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: support :ref:`auto_host_rewrite_header<envoy_api_field_config.filter.http.dynamic_forward_proxy.v2alpha.PerRouteConfig.auto_host_rewrite_header>` in the dynamic forward proxy.
+* http: use llhttp as default http parser


Drive by comment as you clean things up, I wouldn't default this on until we've had some fuzzing aimed at it.
I'd be inclined to land default off, make sure we have comparable fuzzing coverage, encourage folks to smoke test it on, fuzz for a few weeks, and then switch the default.

Sure, will flip this off-by-default for now

alyssawilk · 2019-12-02T15:07:09Z

source/common/http/http1/codec_impl.cc

@@ -314,47 +314,13 @@ void RequestStreamEncoderImpl::encodeHeaders(const HeaderMap& headers, bool end_
  StreamEncoderImpl::encodeHeaders(headers, end_stream);
 }

-http_parser_settings ConnectionImpl::settings_{


While you're doing your clean-up, WDYD of landing some of the pure refactor as its own PR?
It'd let us land some of this ASAP, make for easier and smaller reviews, etc.

Sure I can break this up a bit (I was admittedly surprised how large the delta was once I got the refactor building)

alyssawilk · 2019-12-02T15:10:52Z

source/common/http/http1/parser_factory.cc

+
+bool ParserFactory::usesLegacyParser() { return use_legacy_parser_; }
+
+void ParserFactory::useLegacy(bool use_legacy_parser) { use_legacy_parser_ = use_legacy_parser; }


As long as we're able to link both, why not make this based on runtime?
I could imagine finding an issue where things don't work as expected, and switching all new connections over is safer than changing command line flags and doing fleet-wide restarts

Are you suggesting just runtime value and not introducing a flag? Or keeping the flag but allow runtime to override? I'm guessing the former since the latter could be confusing to someone seeing the flag enabled but not knowing the runtime value is overriding it, but just want to clarify.

I definitely think it's worth having runtime. Neutral about flags - last I heard from Matt most non-google companies don't use command line flags heavily so if you folks don't need it I'd lean towards sticking with one configuration method for consistency, as you say.

Yeah I would probably just do runtime.

alyssawilk · 2019-12-02T15:12:16Z

source/common/http/http1/codec_impl.cc

  if (strict_header_validation_) {
    if (!Http::HeaderUtility::headerIsValid(header_value)) {
      ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value);
      error_code_ = Http::Code::BadRequest;
      sendProtocolError();
      throw CodecProtocolException("http/1.1 protocol error: header value contains invalid chars");
    }
-  } else if (header_value.find('\0') != absl::string_view::npos) {
+  } else if (ParserFactory::usesLegacyParser() &&


Is this an optimization because we're confident the new parser handles this for us? I'd think we wouldn't want to allow \0 for the new version either.
(calling out because if we make this a runtime switch we can't depend on the previously stable value)

yanavlasov · 2019-12-03T00:16:09Z

source/common/http/http1/codec_impl.cc

    sendProtocolError();
-    throw CodecProtocolException("http/1.1 protocol error: " +
-                                 std::string(http_errno_name(HTTP_PARSER_ERRNO(&parser_))));
+    throw CodecProtocolException("http/1.1 protocol error: " + std::string(parser_->errnoName()));


absl::StrCat would be a good think to use here.

yanavlasov · 2019-12-03T00:22:23Z

source/common/http/http1/parser.h

+  virtual ~Parser() = default;
+  virtual int execute(const char* slice, int len) PURE;
+  virtual void resume() PURE;
+  virtual int pause() PURE;


Would it make sense to return ParserStatus here? Or would it be too much work to translate from implementation specific status to the ParserStatus?

yanavlasov · 2019-12-03T00:24:54Z

source/common/http/http1/codec_impl.cc

@@ -607,12 +569,13 @@ void ServerConnectionImpl::onEncodeComplete() {
 void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int method) {


Would it make sense to make method to be type Method?

yanavlasov · 2019-12-03T00:26:11Z

source/common/http/http1/codec_impl.cc


-    // Currently, CONNECT is not supported, however; http_parser_parse_url needs to know about
+    // Currently, CONNECT is not supported, however; llhttp_parse_url needs to know about


Do both parsers need to know about CONNECT?

yanavlasov · 2019-12-03T00:28:11Z

source/common/http/http1/codec_impl.cc

+  current_header_map_.reset();
+  header_parsing_state_ = HeaderParsingState::Done;
+
+  // Returning 2 informs llhttp to not expect a body or further data on this connection.


Is this code common to both http_parser and llhttp? What would http_parser do with 2?

moderation · 2020-01-10T20:17:11Z

bazel/repository_locations.bzl

+        strip_prefix = "llhttp-release-v2.0.1",
+        urls = ["https://github.com/nodejs/llhttp/archive/release/v2.0.1.tar.gz"],


llhttp 2.0.2 was just released - https://github.com/nodejs/llhttp/releases/tag/release%2Fv2.0.2

llhttp 2.0.3 was just released - https://github.com/nodejs/llhttp/releases/tag/release%2Fv2.0.3

llhttp 2.0.4 was just released - https://github.com/nodejs/llhttp/releases/tag/v2.0.4

llhttp 2.0.5 released - https://github.com/nodejs/llhttp/releases/tag/v2.0.5

As @indutny mentioned at #5155 (comment), llhttp 2.1.1 released - https://github.com/nodejs/llhttp/releases/tag/v2.1.1

llhttp 2.2.0 released - https://github.com/nodejs/llhttp/releases/tag/v2.2.0

llhttp 3.0.0 released - https://github.com/nodejs/llhttp/releases/tag/release%2Fv3.0.0

Signed-off-by: Derek Argueta <deargueta@tesla.com>

stale · 2020-04-29T22:37:54Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Derek Argueta <darguetap@gmail.com>

stale · 2020-05-13T19:24:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-05-20T19:43:37Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

derekargueta added 6 commits November 14, 2019 12:11

begin integrating llhttp

e2e2f37

Signed-off-by: Derek Argueta <dereka@pinterest.com>

fix llhttp finish

0f783e7

Signed-off-by: Derek Argueta <dereka@pinterest.com>

remove extra comparison program

b606eee

Signed-off-by: Derek Argueta <dereka@pinterest.com>

cleanup

0e3aa68

Signed-off-by: Derek Argueta <dereka@pinterest.com>

add a compile-time switch between llhttp and http_parser

bc59371

Signed-off-by: Derek Argueta <dereka@pinterest.com>

add release note

3b54253

Signed-off-by: Derek Argueta <dereka@pinterest.com>

lizan self-requested a review November 14, 2019 20:22

indutny reviewed Nov 14, 2019

View reviewed changes

lizan reviewed Nov 14, 2019

View reviewed changes

htuch assigned lizan, mattklein123 and alyssawilk Nov 14, 2019

repokitteh-read-only bot added the waiting:any label Nov 15, 2019

repokitteh-read-only bot removed the waiting:any label Nov 18, 2019

mattklein123 added the waiting label Nov 21, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Nov 26, 2019

alyssawilk added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Nov 26, 2019

start switching to runtime flag

082f0f7

Signed-off-by: Derek Argueta <dereka@pinterest.com>

derekargueta added 3 commits November 26, 2019 23:58

Merge branch 'master' into dereka/llhttp

b186af1

Signed-off-by: Derek Argueta <dereka@pinterest.com>

remove speed test for now

bb7071e

Signed-off-by: Derek Argueta <dereka@pinterest.com>

format

de604e3

Signed-off-by: Derek Argueta <dereka@pinterest.com>

repokitteh-read-only bot removed the waiting label Nov 27, 2019

derekargueta added 4 commits November 27, 2019 00:28

fix BUILD formatting

1ed643b

Signed-off-by: Derek Argueta <dereka@pinterest.com>

cleanup

c4981fc

Signed-off-by: Derek Argueta <dereka@pinterest.com>

remove outdated comment

6f94c61

Signed-off-by: Derek Argueta <dereka@pinterest.com>

fmt

cdee9ac

Signed-off-by: Derek Argueta <dereka@pinterest.com>

alyssawilk reviewed Dec 2, 2019

View reviewed changes

mattklein123 added the waiting label Dec 2, 2019

yanavlasov requested changes Dec 3, 2019

View reviewed changes

moderation reviewed Jan 10, 2020

View reviewed changes

Merge branch 'master' into dereka/llhttp

ba651d8

Signed-off-by: Derek Argueta <deargueta@tesla.com>

mattklein123 removed the no stalebot Disables stalebot from closing an issue label Apr 22, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 29, 2020

Merge branch 'master' into dereka/llhttp

28257f6

Signed-off-by: Derek Argueta <darguetap@gmail.com>

repokitteh-read-only bot removed the waiting label May 6, 2020

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 6, 2020

fix version_history, repository_locations, bump llhttp to 2.0.5

09551b5

Signed-off-by: Derek Argueta <darguetap@gmail.com>

mattklein123 added the waiting label May 6, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 13, 2020

stale bot closed this May 20, 2020

moderation mentioned this pull request Oct 28, 2020

Switch from http-parser to llhttp #5155

Closed

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 28, 2020

asraa mentioned this pull request Mar 2, 2021

[http1] Refactor H/1 codec with a parser interface #15263

Merged

zhxie mentioned this pull request Aug 23, 2023

Performance issue with Balsa Parser #29203

Closed


		bool ParserFactory::usesLegacyParser() { return use_legacy_parser_; }

		void ParserFactory::useLegacy(bool use_legacy_parser) { use_legacy_parser_ = use_legacy_parser; }

		@@ -607,12 +569,13 @@ void ServerConnectionImpl::onEncodeComplete() {
		void ServerConnectionImpl::handlePath(HeaderMapImpl& headers, unsigned int method) {


		// Currently, CONNECT is not supported, however; http_parser_parse_url needs to know about
		// Currently, CONNECT is not supported, however; llhttp_parse_url needs to know about

		strip_prefix = "llhttp-release-v2.0.1",
		urls = ["https://github.com/nodejs/llhttp/archive/release/v2.0.1.tar.gz"],

[WIP] use llhttp as default HTTP parser #9033

[WIP] use llhttp as default HTTP parser #9033

Conversation

derekargueta commented Nov 14, 2019

indutny commented Nov 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Nov 14, 2019

mattklein123 commented Nov 15, 2019

alyssawilk commented Nov 18, 2019

indutny commented Nov 18, 2019

derekargueta commented Nov 18, 2019 • edited

lizan commented Nov 18, 2019

derekargueta commented Nov 18, 2019

jmarantz commented Nov 18, 2019 • edited

lizan commented Nov 19, 2019

stale bot commented Nov 26, 2019

derekargueta commented Nov 27, 2019

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moderation Jul 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Apr 29, 2020

stale bot commented May 13, 2020

stale bot commented May 20, 2020

derekargueta commented Nov 18, 2019 •

edited

jmarantz commented Nov 18, 2019 •

edited

moderation Jul 20, 2020 •

edited