common/http: Remove chromium_url by dio · Pull Request #14583 · envoyproxy/envoy

dio · 2021-01-06T21:14:00Z

Commit Message: This patch adds runtime feature flag to switch from chromium_url to url lib from googleurl. The new one is enabled by default.
Additional Description: This is provided as an alternative of #13808.
Risk Level: Medium
Testing: Existing, plus added unit tests for testing different runtime flags.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

repokitteh-read-only · 2021-01-06T21:14:05Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #14583 was opened by dio.

see: more, trace.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch

LGTM, if this can nuke source/common/chromium_url then this is a fairly clean approach (relatively, but the shim could still work..).

dio · 2021-01-07T01:30:11Z

/retest

repokitteh-read-only · 2021-01-07T01:30:14Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14583 (comment) was created by @dio.

see: more, trace.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2021-01-07T13:15:55Z

Nuking chromium_url in this PR. But I'm trying to check if we can have both libs linked and switch based on flag.

dio · 2021-01-07T13:17:17Z

bazel/external/googleurl.patch

+
+ // Canonicalizes a host that requires IDN conversion. Returns true on success
+ bool DoIDNHost(const gurl_base::char16* src, int src_len, CanonOutput* output) {
+-  int original_output_len = output->length();  // So we can rewind below.


Coverage and fuzz_coverage resolves more symbols from url lib, hence it is needed to include url_canon_host.cc.

I feel the shimmed one is safer.

@yanavlasov @dio thinking about this vs. the shim PR, I think ultimately, if we can upstream Chromium to give us an IDN-free optional build, that would be the ideal, since then we don't need to own a patch permanently. Shimming reduces the size of the patch, but in all cases with a patch we have a permanent patch ownership issue. LGTM modulo comments.

I recall this patch is needed to make fuzzing test happy. I was going to make another pass to see if tests can be fixed to get rid of the patch.

Yes, the last bit of this patch is related to fuzzing test. I agree we should take a deeper look on tests and modify it.

This reverts commit be0f0b8. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2021-01-08T05:51:39Z

/retest

repokitteh-read-only · 2021-01-08T05:51:43Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14583 (comment) was created by @dio.

see: more, trace.

dio · 2021-01-08T09:08:25Z

/retest

repokitteh-read-only · 2021-01-08T09:08:30Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14583 (comment) was created by @dio.

see: more, trace.

yanavlasov

Can you add a release note please?
/wait

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2021-01-20T01:58:20Z

/retest

repokitteh-read-only · 2021-01-20T01:58:23Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #14583 (comment) was created by @dio.

see: more, trace.

dio · 2021-01-20T15:26:09Z

/retest

repokitteh-read-only · 2021-01-20T15:26:14Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14583 (comment) was created by @dio.

see: more, trace.

htuch

LGTM; I'd like @alyssawilk to provide a final go/no-go before we merge, as another input on the runtime default direction, but good from my end. Thanks so much @dio for working through an area of large ambiguity and integration complexity, eliminating our fork here is a huge win.

alyssawilk · 2021-01-21T16:45:06Z

Generally we encourage new code paths on by default unless we think it's very high risk. If this is roughly the same code, just a different source, I think on by default totally make sense :-)

dio · 2021-01-25T21:05:43Z

@htuch @yanavlasov Sorry, fixing the conflicted current.rst dismissed the reviews.

htuch

LGTM, thanks!

yanavlasov · 2021-01-27T03:06:23Z

/azp run

azure-pipelines · 2021-01-27T03:06:33Z

Azure Pipelines successfully started running 1 pipeline(s).

dio · 2021-01-27T09:22:51Z

@htuch or @moderation we need lgtm deps? :)

moderation · 2021-01-27T14:54:59Z

/lgtm deps
Looks OK. Patch is large with only a categorization change for existing dep

dio added 4 commits January 6, 2021 21:06

WIP: common/http: Remove chromium_url

cb049e1

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove unused files

fdf1930

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix format

b7c9780

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove unused entry

e87f793

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Jan 6, 2021

dio mentioned this pull request Jan 6, 2021

build: Introduce GURL with shimmed ICU #13808

Closed

mattklein123 assigned yanavlasov Jan 6, 2021

dio added 2 commits January 6, 2021 22:44

Fix format

3b7fa2a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix deps

2775392

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

htuch reviewed Jan 7, 2021

View reviewed changes

dio added 3 commits January 7, 2021 05:20

Update patch, need to trim more

2f750ee

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Resolve more symbols

3e5c57f

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove

be0f0b8

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio commented Jan 7, 2021

View reviewed changes

dio changed the title ~~WIP: common/http: Remove chromium_url~~ common/http: Remove chromium_url Jan 7, 2021

dio marked this pull request as ready for review January 7, 2021 14:51

dio added 3 commits January 7, 2021 21:40

Revert "Remove"

6675cc6

This reverts commit be0f0b8. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Add a runtime feature flag

f7d0931

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Newline

d612982

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

yanavlasov approved these changes Jan 13, 2021

View reviewed changes

yanavlasov requested changes Jan 13, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 13, 2021

dio added 3 commits January 19, 2021 20:23

Leftover

b49d107

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Duplicated

a2d9125

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Format

5b09a00

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio requested a review from yanavlasov January 19, 2021 20:45

Merge remote-tracking branch 'upstream/main'

079ab7e

htuch previously approved these changes Jan 21, 2021

View reviewed changes

yanavlasov previously approved these changes Jan 25, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main'

4271d7a

dio dismissed stale reviews from yanavlasov and htuch via 4271d7a January 25, 2021 21:03

Merge remote-tracking branch 'upstream/main'

c4a7125

yanavlasov approved these changes Jan 26, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main'

d245fb7

htuch approved these changes Jan 27, 2021

View reviewed changes

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Jan 27, 2021

yanavlasov merged commit 18212b5 into envoyproxy:main Jan 27, 2021

alyssawilk mentioned this pull request Oct 5, 2021

envoy.reloadable_features.remove_forked_chromium_url deprecation #18442

Closed

Conversation

dio commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jan 6, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

dio commented Jan 7, 2021

Uh oh!

repokitteh-read-only bot commented Jan 7, 2021

Uh oh!

dio commented Jan 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dio Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

dio Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

htuch Jan 18, 2021

Choose a reason for hiding this comment

Uh oh!

yanavlasov Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

dio Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

dio commented Jan 8, 2021

Uh oh!

repokitteh-read-only bot commented Jan 8, 2021

Uh oh!

dio commented Jan 8, 2021

Uh oh!

repokitteh-read-only bot commented Jan 8, 2021

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

Uh oh!

dio commented Jan 20, 2021

Uh oh!

repokitteh-read-only bot commented Jan 20, 2021

Uh oh!

dio commented Jan 20, 2021

Uh oh!

repokitteh-read-only bot commented Jan 20, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jan 21, 2021

Uh oh!

dio commented Jan 25, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented Jan 27, 2021

Uh oh!

azure-pipelines bot commented Jan 27, 2021

Uh oh!

dio commented Jan 27, 2021

Uh oh!

moderation commented Jan 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dio commented Jan 6, 2021 •

edited

Loading

dio commented Jan 7, 2021 •

edited

Loading