http: Use GURL as HTTP URL parser utility #10599

dio · 2020-04-01T00:49:46Z

This replaces http_parser's URL parser with GURL.

Risk Level: Medium
Testing: Unit
Docs Changes: N/A
Release Notes: N/A

Use GURL as HTTP URL parser utility. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-04-01T23:19:39Z

gurl on On windows:

Only clang-cl is supported on Windows, see https://crbug.com/988071

external/com_googlesource_googleurl\base/compiler_specific.h(11): fatal error C1189: #error:  "Only clang-cl is supported on Windows, see https://crbug.com/988071"

Will do #ifdef-ing.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-04-02T22:44:09Z

bazel/repository_locations.bzl

- urls = ["https://storage.googleapis.com/quiche-envoy-integration/googleurl_dbf5ad147f60afc125e99db7549402af49a5eae8.tar.gz"],
+ sha256 = "5bdbecce22d522f03ac86c6a694d5ff76780a16d6d682910e87ac156b0a25b21",
+ strip_prefix = "googleurl-dbf5ad1",
+ urls = ["https://github.com/dio/googleurl/archive/dbf5ad1.tar.gz"],


A temporary fix, since it has a top-level BUILD file and a build directory (which is problematic when extracting on mac and windows). I renamed the BUILD file to BUILD.bazel, I think removing it is OK.

I assume can just patch?

Seems like we can't, since it is failing when http_archive extracting the tarball, it is stopped when trying to extract BUILD when it saw build dir is already there. I'm not sure how we can have a patch on the downloaded tarball before extracting.

Probably @danzh2010 could shed some light here.

A temporary fix, since it has a top-level BUILD file and a build directory (which is problematic when extracting on mac and windows). I renamed the BUILD file to BUILD.bazel, I think removing it is OK.

The top level BUILD file is a dummy but needed for the build_config.bzl to setup default copts. But they probably can be moved to a subdirectory instead.

As to build directory, it is from chromium, so nothing I can do there. What's the exact issue of having it under root directory?

@danzh2010 with the original archive, http_archive tries to extract BUILD file while the build directory is already extracted (the build directory is extracted before the BUILD file from the tarball) and http_archive throws error. It happens only for systems with case-insensitive filesystem like Windows and macOS.

If I move the top level BUILD file and build_config.bzl to some sub-directory, would it help your case?

Yes! Thanks @danzh2010!

@danzh2010 sorry. Can we still pick this up? (moving the top-level BUILD file sub-directory)

htuch

Totally rad. This gets us most of the way to resolving #6588 (once we fold the path normalization into the Url class).

htuch · 2020-04-03T21:09:55Z

bazel/external/googleurl.patch

+ "url_util.h",
+ ],
+ copts = build_config.default_copts,
+- linkopts = ["-licuuc"],


Why remove this if we now have icuuc?

Since we depend on "@org_unicode_icuuc//:icuuc" directly.

htuch · 2020-04-03T21:10:50Z

source/common/http/url_utility.cc

+constexpr char HTTPS[] = "https";
+#endif
+
+bool Url::initialize(absl::string_view absolute_url) {


At some point, can we get rid of https://github.com/envoyproxy/envoy/tree/master/source/common/chromium_url by doing the normalization in this class via GURL?

I think so yeah.

htuch · 2020-04-03T21:12:08Z

bazel/external/icuuc.BUILD

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


This is an autoconf dependency, can we just use foreign_cc to avoid having to hand-maintain a bespoke BUILD file for the project?

I'm testing it, but it seems the build time is significantly increasing.

Interesting, I wonder if it's doing something like building tests or additional libraries. Might be possible to control with some autoconf flags...

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).

htuch · 2020-04-03T21:12:33Z

bazel/repository_locations.bzl

- urls = ["https://storage.googleapis.com/quiche-envoy-integration/googleurl_dbf5ad147f60afc125e99db7549402af49a5eae8.tar.gz"],
+ sha256 = "5bdbecce22d522f03ac86c6a694d5ff76780a16d6d682910e87ac156b0a25b21",
+ strip_prefix = "googleurl-dbf5ad1",
+ urls = ["https://github.com/dio/googleurl/archive/dbf5ad1.tar.gz"],


I assume can just patch?

source/common/http/url_utility.cc

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-04-06T14:51:17Z

/azp run envoy-presubmit

azure-pipelines · 2020-04-06T14:51:25Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

htuch · 2020-04-06T18:56:03Z

bazel/external/googleurl.patch

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


What's the rationale for this patch to grow larger?

This is for accommodating windows with msvc compiler that we have.

htuch · 2020-04-06T18:56:42Z

bazel/external/icuuc.BUILD

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


Interesting, I wonder if it's doing something like building tests or additional libraries. Might be possible to control with some autoconf flags...

dio · 2020-04-06T22:05:33Z

/azp run envoy-presubmit

azure-pipelines · 2020-04-06T22:05:41Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-04-06T22:12:00Z

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.

dio · 2020-04-06T22:13:26Z

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).

dio · 2020-04-06T22:16:07Z

source/common/router/router.cc

- downstream_headers.setScheme(absolute_url.scheme());
- downstream_headers.setHost(absolute_url.host_and_port());
- downstream_headers.setPath(absolute_url.path_and_query_params());


Caught by clang-tidy.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-04-07T00:59:03Z

Hum, the coverage is changed:

Code coverage 96.52794344082757 is lower than limit of 97.0

Will check.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio · 2020-04-07T21:58:57Z

/azp run envoy-presubmit

azure-pipelines · 2020-04-07T21:59:05Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2020-04-08T05:22:51Z

😔 The macOS job was canceled.

dio · 2020-04-16T06:38:35Z

@lizan this is me having a bad luck or it is a fact that the macOS build is too long and often cancelled or failed to call RBE?

e.g.

Caused by: io.grpc.StatusRuntimeException: UNAVAILABLE: Unable to resolve host remotebuildexecution.googleapis.com

stale · 2020-04-23T09:07:11Z

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-04-30T09:43:44Z

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!

htuch · 2020-06-19T17:20:02Z

@dio looks like this one went stale, do you recall what the major blockers here were? I'm interested in URI parsing for the URI encoding in https://docs.google.com/document/d/1zZav-IYxMO0mP2A7y5XHBa9v0eXyirw1CxrLLSUqWR8/edit.

dio · 2020-06-21T13:13:04Z

@htuch I have moved this PR to #11670. I think the decision of doing bespoke "BUILD" (for building google URL and Unicode libs) is the main blocker. While I think, since I have accommodated this from tensorflow, I hope it is fine.

dio added 2 commits April 1, 2020 02:01

http, utility: Use GURL

ee612de

Use GURL as HTTP URL parser utility. Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix build vs BUILD

003bc93

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio changed the title ~~http: Use GURL as HTTP URL parser utility.~~ http: Use GURL as HTTP URL parser utility Apr 1, 2020

dio added 3 commits April 1, 2020 10:32

Revert name

553d357

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix

499e330

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Make it compiles on osx

2b89e1a

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio added 4 commits April 2, 2020 07:10

clang-tidy

2dfff67

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Introduce url_utility

f7f35f0

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix quic

2214afe

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix

76c618f

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio marked this pull request as ready for review April 2, 2020 22:12

dio requested review from alyssawilk and mattklein123 as code owners April 2, 2020 22:12

dio commented Apr 2, 2020

View reviewed changes

htuch self-assigned this Apr 3, 2020

htuch reviewed Apr 3, 2020

View reviewed changes

dio added 5 commits April 6, 2020 04:47

gurl on windows please

380c24e

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

win: Try again

73e399d

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

string16

b27f6ea

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Don't include string16.cc on windows

815063c

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix MSVC error C4067

2c1c201

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio added 6 commits April 6, 2020 21:52

Merge remote-tracking branch 'upstream/master' into use-gurl

fca16d6

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Use foreign_cc

c3b01da

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Fix osx

d802953

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

mingw

0154933

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Faster build

87f112f

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

CLeanup

6e190bc

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

htuch mentioned this pull request Apr 6, 2020

tidy: fix master #10661

Merged

htuch reviewed Apr 6, 2020

View reviewed changes

dio commented Apr 6, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master'

5875ef9

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

dio added 2 commits April 7, 2020 10:02

Update

747bcc8

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>

Merge remote-tracking branch 'upstream/master'

8f21a57

dio added 2 commits April 9, 2020 00:07

Merge remote-tracking branch 'upstream/master'

00faae9

Merge remote-tracking branch 'upstream/master'

082d95a

alyssawilk mentioned this pull request Apr 15, 2020

http: downstream connect support #10720

Merged

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

stale bot closed this Apr 30, 2020

htuch mentioned this pull request Jun 19, 2020

Maintainable path normalization library #6588

Closed

This was referenced Jun 20, 2020

http: Use GURL as HTTP URL parser utility #11670

Merged

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

Closed

http: Use GURL as HTTP URL parser utility #10599

http: Use GURL as HTTP URL parser utility #10599

Conversation

dio commented Apr 1, 2020

dio commented Apr 1, 2020 • edited Loading

dio Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Apr 6, 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

dio Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio Jun 20, 2020 • edited Loading

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

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

dio Apr 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Apr 6, 2020

azure-pipelines bot commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Apr 6, 2020

azure-pipelines bot commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Apr 7, 2020

dio commented Apr 7, 2020

azure-pipelines bot commented Apr 7, 2020

dio commented Apr 8, 2020

dio commented Apr 16, 2020 • edited Loading

stale bot commented Apr 23, 2020

stale bot commented Apr 30, 2020

htuch commented Jun 19, 2020

dio commented Jun 21, 2020

dio commented Apr 1, 2020 •

edited

Loading

dio Apr 2, 2020 •

edited

Loading

dio Apr 6, 2020 •

edited

Loading

dio Apr 15, 2020 •

edited

Loading

dio Jun 20, 2020 •

edited

Loading

dio Apr 6, 2020 •

edited

Loading

dio commented Apr 16, 2020 •

edited

Loading