http: Use GURL as HTTP URL parser utility #11670

dio · 2020-06-20T06:40:08Z

Commit Message: http: Use GURL as HTTP URL parser utility
Additional Description: This replaces http_parser's URL parser with GURL.
Risk Level: Medium?
Testing: Unit
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Dhi Aurrahman [email protected]

Signed-off-by: Dhi Aurrahman <[email protected]>

bazel/repository_locations.bzl

dio · 2020-06-21T13:07:36Z

bazel/external/googleurl.patch

+-#if defined(__clang__) && __has_attribute(uninitialized)
+#if defined(__clang__)
+#if defined(__has_attribute)
+#if __has_attribute(uninitialized)


Since we put /Za when compiling on Windows, MSVC's C4067 warning is treated as an error.

I find it interesting given that googletest itself supports msvc, but does not support clang-cl.

dio · 2020-06-21T13:07:50Z

bazel/external/googleurl.patch

+ cc_library(
+     name = "strings",
+     srcs = [
+-        "string16.cc",


string16 doesn't make sense to use on 2-byte wchar_t systems (e.g. Windows).

I trust this patch is operating entirely on ISO-8859-1 with utf-8 enablement? Should be no wide char anything, this isn't java.

dio · 2020-06-21T13:08:07Z

bazel/external/googleurl.patch

+         "string_piece.h",
+         "string_piece_forward.h",
+         "string_util.h",
+-        "string_util_posix.h",


@htuch: What's the rationale for this patch to grow larger?
@dio: This is for accommodating windows with msvc compiler that we have.

So, I'm still not super happy with the size of this patch. @wrowe @sunjayBhatia do you have any thoughts on what we can do to avoid having to hand craft this in Envoy? If @dio merges, would you folks be able to followup with upstream to eliminate this patch?

Can we put this review on full pause until we have 11107 merged, and rebased? That will pick up //test/... and we'll be in a better position to give this our full attention, since regressions are taking up most of our free cycles for the past days.

SGTM. I leave it to @htuch to decide.

Sure, we can wait for #11107.

So today we should see 11107 merged. We did go ahead this week and build dio's branch locally with //test/... on Windows, and didn't observe any significant or outright failures, although there are several compilation emits that should be addressed before we merge this to master. This will become apparent to all when it is rebuilt after 11107.

Just to make things very clear, we are patching the deliberate exclusion of one particular architecture, which served no purpose and discouraged development by any interested contributor. I don't know how a patch to revert such a silly commit would be received at the upstream community, so we can make no promises how long a patch will have to live at envoy, @htuch

#11107 is merged and this PR has picked it up.

Great. I'm wondering if we can get some agreement on the long term ownership of this patch. From my perspective, a patch this size is not sustainable in Envoy and that can't be the long term strategy; this will impact our velocity and add friction to upgrading to new versions of GURL in response to security issues. OTOH, I'm eager to move forward and we can accept a patch in the interim.

@dio would it make sense to just try upstream this and see what happens?
@wrowe would you folks be able to take over this patch and long term efforts to shrink it down via tactical upstreaming if this can't be easily done?

bazel/external/googleurl.patch

dio · 2020-06-21T13:08:17Z

bazel/external/icuuc.BUILD

+    tags = ["requires-rtti"],
+    visibility = ["//visibility:private"],
+    deps = [":headers"],
+)


@htuch: This is an autoconf dependency, can we just use foreign_cc to avoid having to hand-maintain a bespoke BUILD file for the project?
@dio: I'm testing it, but it seems the build time is significantly increasing.
@htuch: Interesting, I wonder if it's doing something like building tests or additional libraries. Might be possible to control with some autoconf flags...
@dio: I believe I have turned off everything (that makes sense, like examples, tests, even tools). And it is tricky to force using msvc (when using configure it tends to use mingw and it is failed at configure stage, and I don't think we want to mix mingw and msvc).

Ref: #10599 (comment)

I'm confused, what is an autoconf dependency? If one is introduced by this patch, that will break the contribution of all four engineers working on the Windows port by breaking master. The one I am familiar with is the opentracing dependency which we have excluded for now, but that only breaks tracing extensions. Breaking http/1 seems unwise. Otherwise the basic idea of this bazel BUILD hack looks entirely reasonable, and there is no expectation it would be fixed upstream, because we are cherry-picking a project for subcomponents that don't have any independent project management and don't appear to be supported stand-alone.

I'm OK with this BUILD (as it isn't a huge patch). In terms of being an autoconf dependency, I'm basically saying it should build under rules_foreign_cc, which knows how to build autoconf.

gperftools is an autoconf dep today that we build this way, see https://github.com/envoyproxy/envoy/blob/master/bazel/foreign_cc/BUILD#L13.

Are you saying in the future, we can't take any external dependency that is autoconf based?

Correct, autoconf is gnu tooling and while it runs in tandem the pseudo-unix cygwin toolchain there is no Windows port, and it's likely to be a thorn in the side of other odd SDKs on mobile and such.

Most every project building on autoconf offers an alternative mechanism for some windows build, however well thought out and supported, or not. external/org_unicode_icuuc/icu4c/source/allinone/ is the Windows build logic by the icu4c maintainers.

Most projects are moving in the direction of supporting CMake for a much simpler cross-platform experience, even as they retain autoconf. This makes the build agnostic of the flavor of Visual Studio and even msvc vs llvm clang-cl.

For Envoy, maintaining a bazel BUILD file for this project makes perfect sense, since the lib objects all match the compiler and link choices of this project. We need to evaluate what the autoconf results produce in terms of source file toggles and changes, even if we avoid these long-term.

For upstream, propose a CMakeFiles.txt to cover their existing autoconf determinations. Our rules_foreign_cc support in bazel is much better with CMake than less-predictable autoconf output. This probably results in the most flexibility to the maintainers and would be our simplest solution short of this short-term BUILD maintenance.

@wrowe can you propose some amendment to https://github.com/envoyproxy/envoy/blob/master/bazel/EXTERNAL_DEPS.md based on this?

I don't think it sounds unreasonable, but we should take this through a review process and ensure folks are aligned, as it's a major project restriction.

How come gperftools isn't impacting you? Don't you use tcmalloc?

tcmalloc is disabled on Windows if you look at .bazelrc

gperftools as of 2015 was being ported to Windows using the tcmalloc_minimal implementation. We have have documented this exclusion for Windows both in .bazelrc and in bazel/foreign_cc/BUILD.

Until bazel test //test/... is largely building clean with all currently tagged "fails_on_windows", gperftools hasn't been and won't be a particularly high priority. The same is true for opentracing and all tracing extensions on Windows.

THE major difference between all that and this proposed enhancement is that the envoyproxy is really nothing without a url parser.

This is fine, but I would like to get this down in writing, so that we can ensure everyone is on-board and don't relitigate this each time we add a new dependency. Can you do the PR to https://github.com/envoyproxy/envoy/blob/master/bazel/EXTERNAL_DEPS.md arguing to remove autoconf in rules_foreign_cc for Windows reasons for all deps that are core across platforms?

source/common/http/url_utility.cc

dio · 2020-06-21T13:09:50Z

source/common/http/url_utility.cc

+bool Url::initializeForConnect(GURL&& url) {
+  // CONNECT requests can only contain "hostname:port"
+  // https://github.com/nodejs/http-parser/blob/d9275da4650fd1133ddc96480df32a9efe4b059b/http_parser.c#L2503-L2506.
+  if (!url.is_valid() || url.IsStandard()) {
+    return false;
+  }
+
+  const auto& parsed = url.parsed_for_possibly_invalid_spec();
+  // The parsed.scheme contains the URL's hostname (stored by GURL). While host and port have -1
+  // as its length.
+  if (parsed.scheme.len <= 0 || parsed.host.len > 0 || parsed.port.len > 0) {
+    return false;
+  }
+
+  host_and_port_ = url.possibly_invalid_spec();
+  const auto& parts = StringUtil::splitToken(host_and_port_, ":", /*keep_empty_string=*/true,
+                                             /*trim_whitespace=*/false);
+  if (parts.size() != 2 || static_cast<size_t>(parsed.scheme.len) != parts.at(0).size() ||
+      !validPortForConnect(parts.at(1))) {
+    return false;
+  }
+
+  return true;
+}


@alyssawilk I'm not sure if this is the right thing to do for making sure CONNECT works.

I guess I'm a bit confused why EXPECT_FALSE(url.initialize("foo.com:65536", is_connect)); fails. Could be I'm insufficiently caffeinated though :-P

Also as long as I'm looking, do we think this should be runtime guarded, even with a shorter duration? I could imagine this having behavioral changes in weird corner cases though ideally it should be good

65536 is greater than std::numeric_limits<uint16_t>::max(). Not in 1-65535 valid port range.

re: Runtime guard. SGTM.

Ah, gotcha. maybe comments there but yeah, otherwise this looks fine.
If folks prefer we could also split out connect validation into its own class since it's a weird one-off but I suspect it'll be useful to have a consistent API and this how the old class worked so LGTM!

Signed-off-by: Dhi Aurrahman <[email protected]>

htuch

Looks good to me, thanks for reviving. My main concern is just around the size of the manual patch for GURL and ICUUC. If there is any way or plan to reduce it, it would be really awesome. Otherwise I'm happy to ship and iterate.

htuch · 2020-06-23T20:17:08Z

bazel/external/googleurl.patch

+         "string_piece.h",
+         "string_piece_forward.h",
+         "string_util.h",
+-        "string_util_posix.h",


So, I'm still not super happy with the size of this patch. @wrowe @sunjayBhatia do you have any thoughts on what we can do to avoid having to hand craft this in Envoy? If @dio merges, would you folks be able to followup with upstream to eliminate this patch?

source/common/http/url_utility.cc

dio · 2020-06-23T21:04:23Z

@htuch after this is merged, I'll try to play around with the autoconf flags once again. Since the time taken by the build using the resulted Makefile takes a longer time, it probably requires more patience 🙂.

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2020-06-24T03:53:30Z

/azp run envoy-presubmit

azure-pipelines · 2020-06-24T03:53:39Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-06-29T00:00:47Z

Merging master to pick up #11107.

htuch

LGTM modulo the patch/ICUUC threads. Excited to see this ready to land..

htuch · 2020-06-29T19:04:14Z

bazel/external/googleurl.patch

+         "string_piece.h",
+         "string_piece_forward.h",
+         "string_util.h",
+-        "string_util_posix.h",


Great. I'm wondering if we can get some agreement on the long term ownership of this patch. From my perspective, a patch this size is not sustainable in Envoy and that can't be the long term strategy; this will impact our velocity and add friction to upgrading to new versions of GURL in response to security issues. OTOH, I'm eager to move forward and we can accept a patch in the interim.

@dio would it make sense to just try upstream this and see what happens?
@wrowe would you folks be able to take over this patch and long term efforts to shrink it down via tactical upstreaming if this can't be easily done?

htuch · 2020-06-29T19:06:00Z

bazel/external/icuuc.BUILD

+    tags = ["requires-rtti"],
+    visibility = ["//visibility:private"],
+    deps = [":headers"],
+)


This is fine, but I would like to get this down in writing, so that we can ensure everyone is on-board and don't relitigate this each time we add a new dependency. Can you do the PR to https://github.com/envoyproxy/envoy/blob/master/bazel/EXTERNAL_DEPS.md arguing to remove autoconf in rules_foreign_cc for Windows reasons for all deps that are core across platforms?

dio · 2020-06-29T19:13:48Z

would it make sense to just try upstream this and see what happens?

Do you mean to submit it to the Chromium project? I can try. Probably, we need @alyssawilk's perspective and guidance on how to do that properly.

htuch · 2020-06-29T19:15:21Z

@dio yep. I think @alyssawilk and @danzh2010 probably could both give some advice on how to approach. Doesn't Chromium build for Windows anyway? I'm pretty sure I remember running Chrome on Windows last time I used it :-P

dio · 2020-06-29T19:29:58Z

@htuch haha, yeah. However only clang-cl is supported on Windows, see https://crbug.com/988071. We're doing MSVC. I'm a bit scared. 😶

htuch · 2020-06-29T23:29:05Z

@dio maybe just start by opening a Chromium issue and see if it's not too controversial?

stale · 2020-07-06T23:39:26Z

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!

dio · 2020-07-06T23:50:53Z

I'm trying to join the discussion here: https://bugs.chromium.org/p/chromium/issues/detail?id=1053958.

htuch · 2020-07-09T14:53:03Z

@dio I think if you can just tag the patch with a TODO and #11974 for now, we can move forward with this PR. Ideally we can undo the patch once we move to clang-cl.exe.

Signed-off-by: Dhi Aurrahman <[email protected]>

dio · 2020-07-09T15:08:29Z

Thanks, @htuch! Updated the patch with TODO.

htuch

LGTM, thanks! Epic.

moderation · 2020-07-09T20:44:59Z

bazel/repository_locations.bzl

+    org_unicode_icuuc = dict(
+        strip_prefix = "icu-release-64-2",
+        sha256 = "524960ac99d086cdb6988d2a92fc163436fd3c6ec0a84c475c6382fbf989be05",
+        urls = ["https://github.com/unicode-org/icu/archive/release-64-2.tar.gz"],


@dio is there any reason you picked this April 2019 rather than the latest https://github.com/unicode-org/icu/releases/tag/release-67-1 ?

The BUILD file is inspired from Tensorflow project as known to be stable. Will try to update along the way. Thanks for the reminder! https://github.com/tensorflow/tensorflow/blob/master/third_party/icu/workspace.bzl. Tracked in here. #12015

After envoyproxy#11670, the CSRF filter started failing for us. This change fixes 3 issues that were uncovered after moving to gURL for parsing URLs: 1) the hostAndPort() utility method, in the CSRF filter, was returning a string view of a stack variable. 2) the Origin header always includes the scheme, so let's ensure this is illustrated in tests (which were missing this and passing due to relaxed checks). 3) the Url::initialize method expects an absolute URL, something that the CSRF filter wasn't complying with. Signed-off-by: Raul Gutierrez Segales <[email protected]>

After #11670, the CSRF filter started failing for us. This change fixes 3 issues that were uncovered after moving to gURL for parsing URLs: 1) the hostAndPort() utility method, in the CSRF filter, was returning a string view of a stack variable. 2) the Origin header always includes the scheme, so let's ensure this is illustrated in tests (which were missing this and passing due to relaxed checks). 3) the Url::initialize method expects an absolute URL, something that the CSRF filter wasn't complying with. Signed-off-by: Raul Gutierrez Segales <[email protected]>

After envoyproxy#11670, the CSRF filter started failing for us. This change fixes 3 issues that were uncovered after moving to gURL for parsing URLs: 1) the hostAndPort() utility method, in the CSRF filter, was returning a string view of a stack variable. 2) the Origin header always includes the scheme, so let's ensure this is illustrated in tests (which were missing this and passing due to relaxed checks). 3) the Url::initialize method expects an absolute URL, something that the CSRF filter wasn't complying with. Signed-off-by: Raul Gutierrez Segales <[email protected]> Signed-off-by: Kevin Baichoo <[email protected]>

This replaces http_parser's URL parser with GURL. Risk Level: Medium Testing: Unit Docs Changes: N/A Release Notes: N/A Signed-off-by: Dhi Aurrahman <[email protected]> Signed-off-by: scheler <[email protected]>

After envoyproxy#11670, the CSRF filter started failing for us. This change fixes 3 issues that were uncovered after moving to gURL for parsing URLs: 1) the hostAndPort() utility method, in the CSRF filter, was returning a string view of a stack variable. 2) the Origin header always includes the scheme, so let's ensure this is illustrated in tests (which were missing this and passing due to relaxed checks). 3) the Url::initialize method expects an absolute URL, something that the CSRF filter wasn't complying with. Signed-off-by: Raul Gutierrez Segales <[email protected]> Signed-off-by: scheler <[email protected]>

Add icu-release-64-2

ab13d65

Signed-off-by: Dhi Aurrahman <[email protected]>

dio mentioned this pull request Jun 20, 2020

deps: Move com_googlesource_googleurl top-level BUILD file to a sub-dir #11671

Closed

dio added 5 commits June 21, 2020 05:53

Implement URL utility

d1eface

Signed-off-by: Dhi Aurrahman <[email protected]>

Add WSS to the spelling dictionary

9f24a3d

Signed-off-by: Dhi Aurrahman <[email protected]>

Use workaround for fetching google URL

e665abf

Signed-off-by: Dhi Aurrahman <[email protected]>

Add docs to url_utility

74f39e1

Signed-off-by: Dhi Aurrahman <[email protected]>

Remove http_parser as dep for utility_lib

9faa65e

Signed-off-by: Dhi Aurrahman <[email protected]>

dio commented Jun 21, 2020

View reviewed changes

bazel/repository_locations.bzl Outdated Show resolved Hide resolved

dio commented Jun 21, 2020

View reviewed changes

bazel/external/googleurl.patch Show resolved Hide resolved

dio commented Jun 21, 2020

View reviewed changes

source/common/http/url_utility.cc Show resolved Hide resolved

dio commented Jun 21, 2020

View reviewed changes

dio changed the title ~~WIP: http: Use GURL as HTTP URL parser utility~~ http: Use GURL as HTTP URL parser utility Jun 21, 2020

dio mentioned this pull request Jun 21, 2020

http: Use GURL as HTTP URL parser utility #10599

Closed

dio marked this pull request as ready for review June 21, 2020 13:13

dio requested review from alyssawilk and mattklein123 as code owners June 21, 2020 13:13

asraa assigned htuch Jun 22, 2020

Update googleurl

69f8761

Signed-off-by: Dhi Aurrahman <[email protected]>

htuch reviewed Jun 23, 2020

View reviewed changes

dio added 3 commits June 24, 2020 04:33

Only accept SchemeIsHTTPOrHTTPS

4d375a1

Signed-off-by: Dhi Aurrahman <[email protected]>

Fix

d3b56ca

Signed-off-by: Dhi Aurrahman <[email protected]>

Fix spelling check

10fd07c

Signed-off-by: Dhi Aurrahman <[email protected]>

Merge remote-tracking branch 'upstream/master'

b922a45

htuch reviewed Jun 29, 2020

View reviewed changes

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

Merge remote-tracking branch 'upstream/master'

88fc88c

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

dio mentioned this pull request Jul 7, 2020

http, utility: Allow to optionally decode the parsed param value when parsing query string #11795

Merged

dio added 2 commits July 9, 2020 22:05

Add TODO on moving to clang-cl

e287563

Signed-off-by: Dhi Aurrahman <[email protected]>

Merge remote-tracking branch 'upstream/master'

15f008c

htuch approved these changes Jul 9, 2020

View reviewed changes

htuch merged commit 2d69e30 into envoyproxy:master Jul 9, 2020

moderation reviewed Jul 9, 2020

View reviewed changes

rgs1 mentioned this pull request Jul 16, 2020

csrf: fix issues with host/origin header parsing #12133

Merged

asraa mentioned this pull request Mar 3, 2021

Unify URL parsing with GoogleURL #15290

Open

http: Use GURL as HTTP URL parser utility #11670

http: Use GURL as HTTP URL parser utility #11670

Conversation

dio commented Jun 20, 2020 • edited Loading

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

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

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Jun 23, 2020

dio commented Jun 24, 2020

azure-pipelines bot commented Jun 24, 2020

dio commented Jun 29, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Jun 29, 2020

htuch commented Jun 29, 2020

dio commented Jun 29, 2020 • edited Loading

htuch commented Jun 29, 2020

stale bot commented Jul 6, 2020

dio commented Jul 6, 2020

htuch commented Jul 9, 2020

dio commented Jul 9, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Jun 20, 2020 •

edited

Loading

dio commented Jun 29, 2020 •

edited

Loading