Import current snapshot of QUICHE as an external dependency. by mpwarres · Pull Request #5548 · envoyproxy/envoy

mpwarres · 2019-01-09T06:46:43Z

Description:

QUICHE is Google's implementation of
QUIC and related protocols. It is the same code used in Chromium and Google's
servers, but packaged in a form that is intended to be easier to incorporate
into third-party projects.

QUICHE is still a work in progress, with current efforts focusing on
eliminating diffs between Google-internal and Chromium variants of the code, as
well as providing buildfiles and a default implementation of the QUICHE
platform API (the platform-dependent porting layer that sits underneath the
main body of QUICHE, which is platform-independent).

In the interim, the Google QUIC team has created a branch of QUICHE,
envoy-integration,
to provide access to the full set of QUICHE source files, while the work above
continues. Since Envoy can overlay its own build rules on top of external
dependencies, this should allow Envoy/QUICHE integration work to move forwards,
without being blocked by completion of "full" QUICHE. When QUICHE is fully
complete, we can just update the dependency to point to the official QUICHE
release, making whatever buildfile adjustments are necessary.

This PR is limited to setting up a basic import of QUICHE with a bare skeleton
of build rules to build and test one element of the platform API. The plan is
to add more build rules and platform implementation classes incrementally, in a
series of follow-on CLs. The end milestone of the platform implementation phase
will be when we can run the QUIC end-to-end tests included in QUICHE on top of
the Envoy platform implementation.

Signed-off-by: Michael Warres mpw@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Risk Level: low--code not used yet
Testing: unit test to exercise build rules
Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]

QUICHE (https://quiche.googlesource.com/quiche/) is Google's implementation of QUIC and related protocols. It is the same code used in Chromium and Google's servers, but packaged in a form that is intended to be easier to incorporate into third-party projects. QUICHE is still a work in progress, with current efforts focusing on eliminating diffs between Google-internal and Chromium variants of the code, as well as providing buildfiles and a default implementation of the QUICHE platform API (the platform-dependent porting layer that sits underneath the main body of QUICHE, which is platform-independent). In the interim, the Google QUIC team has created a branch of QUICHE, envoy-integration (https://quiche.googlesource.com/quiche/+/envoy-integration), to provide access to the full set of QUICHE source files, while the work above continues. Since Envoy can overlay its own build rules on top of external dependencies, this should allow Envoy/QUICHE integration work to move forwards, without being blocked by completion of "full" QUICHE. When QUICHE is fully complete, we can just update the dependency to point to the official QUICHE release, making whatever buildfile adjustments are necessary. This PR is limited to setting up a basic import of QUICHE with a bare skeleton of build rules to build and test one element of the platform API. The plan is to add more build rules and platform implementation classes incrementally, in a series of follow-on CLs. The end milestone of the platform implementation phase will be when we can run the QUIC end-to-end tests included in QUICHE on top of the Envoy platform implementation. Signed-off-by: Michael Warres <mpw@google.com>

mpwarres · 2019-01-09T07:05:40Z

FYI @mattklein123 @alyssawilk @conqerAtapple

alyssawilk · 2019-01-09T14:30:54Z

I think on this one I'd like @htuch to take a pass since it's mostly bazel hackery and I want to make sure all QUIC code is easy to compile out.
Also you'll want to check the builds - looks like format and clang_tidy need a pass.

alyssawilk · 2019-01-09T14:36:09Z

Also builds or not, this PR is super exciting. Hurrah for QUIC integration!

htuch

Awesome to do see this happening! I have some comment on repo structure, happy to chat IRL as well.

bazel/repository_locations.bzl

source/extensions/quic_listeners/quiche/BUILD

source/extensions/quic_listeners/quiche/platform/BUILD

bazel/external/quiche.BUILD

source/extensions/quic_listeners/quiche/platform/http2_reconstruct_object_impl.h

Signed-off-by: Michael Warres <mpw@google.com>

htuch · 2019-01-11T00:30:09Z

@mpwarres as discussed IRL, what you have here looks pragmatic. Can you add the TODOs and fix the CI build?

Signed-off-by: Michael Warres <mpw@google.com>

For the impl files that are trivially implemented, add the implementation. For the impl files that require more work, add TODO comments. Signed-off-by: Michael Warres <mpw@google.com>

mpwarres · 2019-01-14T23:10:07Z

Update: was almost ready to send this back out for review, but now I've hit a snag in the form of linker errors, that might indicate a problem with this approach for importing. I've pushed my latest commits even though they will cause pre-integration checks to fail, to help provide concrete illustration of the issue.

Details: after resolving other CI issues, I figured I might as well fill in some of the more trivial platform impl definitions. In doing so, I discovered that when the newly-added QUICHE platform impl classes in //source/extensions/quic_listeners/quiche/platform attempt to depend on Envoy classes, it causes linker errors, since (IIUC) the build target chain of "@com_googlesource_quiche//:http2_platform" -> "@envoy//source/extensions/quic_listeners/quiche/platform:http2_platform_impl_lib" -> "//source/common/common:assert_lib" generates .o files for various Envoy source files under bazel-out/k8-fastbuild/bin/external/envoy/source/, while the same Envoy classes compiled through normal build target chains (which do not got through "@com_googlesource_quiche//:http2_platform") end up in bazel-out/k8-fastbuild/bin/source/. Then, when the linker tries to link all .o files, it sees multiple definitions.

For example:

$ bazel build //test/extensions/quic_listeners/...
[...]
/usr/bin/ld.gold: error: bazel-out/k8-fastbuild/bin/source/common/common/_objs/assert_lib/assert.pic.o: multiple definition of 'Envoy::Assert::ActionRegistrationImpl::debug_assertion_failure_record_action_'
/usr/bin/ld.gold: bazel-out/k8-fastbuild/bin/external/envoy/source/common/common/_objs/assert_lib/assert.pic.o: previous definition here
/usr/bin/ld.gold: error: bazel-out/k8-fastbuild/bin/source/common/common/_objs/assert_lib/assert.pic.o: multiple definition of 'Envoy::Assert::setDebugAssertionFailureRecordAction(std::function<void ()>)'
/usr/bin/ld.gold: bazel-out/k8-fastbuild/bin/external/envoy/source/common/common/_objs/assert_lib/assert.pic.o: previous definition here
/usr/bin/ld.gold: error: bazel-out/k8-fastbuild/bin/source/common/common/_objs/assert_lib/assert.pic.o: multiple definition of 'Envoy::Assert::invokeDebugAssertionFailureRecordAction_ForAssertMacroUseOnly()'
/usr/bin/ld.gold: bazel-out/k8-fastbuild/bin/external/envoy/source/common/common/_objs/assert_lib/assert.pic.o: previous definition here
[...]

This (unfortunately) kind of makes sense, given the funky way in which Envoy is depending on QUICHE, which (for platform impl) is depending back on Envoy code.

I need to think more about if there's a way to resolve this. If anyone else has ideas/suggestions, please let me know. Argh...

mpwarres · 2019-01-14T23:24:24Z

Hmm, one solution might be to restrict the QUICHE platform impl from depending on any Envoy libraries. Most if not all of it should be implementable on top of Abseil. Though that would be unfortunate in the case of things like e.g. logging, where it would be nice for QUICHE logging to go through the same logging pipeline as Envoy code.

If we go this route, it might argue for placing the platform impl somewhere outside of the Envoy source tree, i.e. as a separate repo that is brought in as an external dependency, itself (on which QUICHE would depend). I think htuch@ floated this as a possibility in earlier comments, even before this issue arose.

Signed-off-by: Michael Warres <mpw@google.com>

…ild targets. Signed-off-by: Michael Warres <mpw@google.com>

mpwarres · 2019-01-15T21:24:15Z

I was able to find a solution to the link issue that allows QUICHE platform impl to depend on Envoy build targets--see bazel-discuss@ mail thread. @htuch, I think this is ready for another look.

mpwarres · 2019-01-15T22:02:03Z

...and it looks like I spoke too soon. While build_image and clang_tidy now succeed, asan and tsan are still broken, due to some difference in the way they are set up. Looking at that now.

htuch

Yep, looks good now modulo remaining few comments. I think the TSAN/ASAN might be a Clang vs. gcc thing, but the Mac build is also Clang, so not sure. What is unfortunate is that looking at the TSAN build log, it seems to be the same issue that @ was supposed to fix..

htuch · 2019-01-16T07:35:03Z

bazel/external/quiche.BUILD

+# '@envoy' as the repository identifier. Otherwise, Bazel generates duplicate
+# object files for the same build target (one under
+# bazel-out/.../bin/external/, and one under bazel-out/.../bin/), eventually
+# resulting in link-time errors.


See latest top-level comment for an explanation of what's up with TSAN/ASAN. Basically it's because they build Envoy as an external dependency of the filter example.

htuch · 2019-01-16T07:36:51Z

source/extensions/quic_listeners/quiche/platform/http2_bug_tracker_impl.h

@@ -0,0 +1,9 @@
+#pragma once
+
+// NOLINT(namespace-envoy)


Could you add comments to all the platform impl files just after the NOLINE explaining these are part of the QUICHE platform implementation, these are not consumed directly in Envoy and should not be referenced directly by any Envoy code, etc. This would make the style differences below more palatable. Thanks :)

htuch · 2019-01-16T07:37:54Z

source/extensions/quic_listeners/quiche/platform/http2_test_helpers_impl.h

+
+// TODO: implement
+
+#define VERIFY_AND_RETURN_SUCCESS(expression) 0


FWIW, I would love these in OSS gtest..

…eased. Signed-off-by: Michael Warres <mpw@google.com>

…y Envoy code. Signed-off-by: Michael Warres <mpw@google.com>

mpwarres · 2019-01-17T16:39:33Z

tl;dr: the linker issues are due to a Bazel bug that has already been fixed, but is not yet in an official Bazel release. I've confirmed that (with the QUICHE platform -> Envoy dependency) both regular and asan/tsan builds succeed with Bazel built at HEAD and the requisite feature flag flipped. Since the fix is not yet in the Bazel version used by CI, though, I have backed out the QUICHE platform -> Envoy dependency in this PR (as we do not need it immediately), and will add it back later on once the Bazel fix is present in the version we use.

I've also added the QUICHE platform comments requested, so I think this is ready for another pass.

Details: for the full story, read this bazel-discuss@ mail thread. It turns out we hit a known Bazel issue, bazelbuild/bazel#6848, which (in our case) caused references made by an external dependency (QUICHE) to "@envoy//foo:bar" to be considered external, even though "@envoy" designates the main workspace.

My earlier fix of changing the dependency to use "@//foo:bar" instead of "@envoy//foo:bar" worked for the default build since it forced the dependency to refer to the main workspace (i.e. "@"). However, this caused failures in the CI ASAN and TSAN builds because those build Envoy as an external dependency of envoy-filter-example, so in those setups, "@" resolved to the main workspace of envoy-filter-example, and not @envoy. (The context-dependent nature of "@" and its implications are explained most clearly in this message from the bazel-discuss@ thread.)

Eventually, once the fix for bazelbuild/bazel#6848 is in a release and made default behavior, dependency references to "@envoy//foo:bar" will resolve to the right place in both situations.

Signed-off-by: Michael Warres <mpw@google.com>

htuch

LGTM, thanks. Sorry about all the Bazel pain. If there are any unresolved issues please reach out to me offline, I can connect you directly with folks in Bazel team who can assist.

mpwarres · 2019-01-18T18:28:38Z

SG, will do if we run into other Bazel issues. Thanks for the review!

Start populating QUICHE platform implementation. Follows on #5548, fleshing out the framework for adding the Envoy implementation of the QUICHE platform (i.e. porting layer). Details: * Add QUICHE spdy_platform and quic_platform build rules. * Prune not yet implemented platform implementation files, to reduce noise. * For partially complete platform implementation files, add "TODO: implement" comments to individual items that still need to be implemented. * Add unit tests for platform APIs that have already been implemented. Risk Level: minimal: code not used yet Testing: unit tests Signed-off-by: Michael Warres <mpw@google.com>

Start populating QUICHE platform implementation. Follows on envoyproxy#5548, fleshing out the framework for adding the Envoy implementation of the QUICHE platform (i.e. porting layer). Details: * Add QUICHE spdy_platform and quic_platform build rules. * Prune not yet implemented platform implementation files, to reduce noise. * For partially complete platform implementation files, add "TODO: implement" comments to individual items that still need to be implemented. * Add unit tests for platform APIs that have already been implemented. Risk Level: minimal: code not used yet Testing: unit tests Signed-off-by: Michael Warres <mpw@google.com> Signed-off-by: Dan Zhang <danzh@google.com>

mpwarres · 2019-01-28T21:30:15Z

Tagging associated issue #2557

Start populating QUICHE platform implementation. Follows on envoyproxy#5548, fleshing out the framework for adding the Envoy implementation of the QUICHE platform (i.e. porting layer). Details: * Add QUICHE spdy_platform and quic_platform build rules. * Prune not yet implemented platform implementation files, to reduce noise. * For partially complete platform implementation files, add "TODO: implement" comments to individual items that still need to be implemented. * Add unit tests for platform APIs that have already been implemented. Risk Level: minimal: code not used yet Testing: unit tests Signed-off-by: Michael Warres <mpw@google.com>

…oxy#5548) QUICHE is Google's implementation of QUIC and related protocols. It is the same code used in Chromium and Google's servers, but packaged in a form that is intended to be easier to incorporate into third-party projects. QUICHE is still a work in progress, with current efforts focusing on eliminating diffs between Google-internal and Chromium variants of the code, as well as providing buildfiles and a default implementation of the QUICHE platform API (the platform-dependent porting layer that sits underneath the main body of QUICHE, which is platform-independent). In the interim, the Google QUIC team has created a branch of QUICHE, envoy-integration, to provide access to the full set of QUICHE source files, while the work above continues. Since Envoy can overlay its own build rules on top of external dependencies, this should allow Envoy/QUICHE integration work to move forwards, without being blocked by completion of "full" QUICHE. When QUICHE is fully complete, we can just update the dependency to point to the official QUICHE release, making whatever buildfile adjustments are necessary. This PR is limited to setting up a basic import of QUICHE with a bare skeleton of build rules to build and test one element of the platform API. The plan is to add more build rules and platform implementation classes incrementally, in a series of follow-on CLs. The end milestone of the platform implementation phase will be when we can run the QUIC end-to-end tests included in QUICHE on top of the Envoy platform implementation. Risk Level: low--code not used yet Testing: unit test to exercise build rules Signed-off-by: Michael Warres <mpw@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

Start populating QUICHE platform implementation. Follows on envoyproxy#5548, fleshing out the framework for adding the Envoy implementation of the QUICHE platform (i.e. porting layer). Details: * Add QUICHE spdy_platform and quic_platform build rules. * Prune not yet implemented platform implementation files, to reduce noise. * For partially complete platform implementation files, add "TODO: implement" comments to individual items that still need to be implemented. * Add unit tests for platform APIs that have already been implemented. Risk Level: minimal: code not used yet Testing: unit tests Signed-off-by: Michael Warres <mpw@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

This was referenced Jan 9, 2019

WIP: POC import of current (incomplete) QUICHE into Envoy. #5431

Closed

Bootstrap and configuration of monolithic QuicListener. #5549

Closed

htuch self-assigned this Jan 9, 2019

htuch suggested changes Jan 9, 2019

View reviewed changes

Address htuch@ review comments.

1d45255

Signed-off-by: Michael Warres <mpw@google.com>

mpwarres added 7 commits January 12, 2019 22:15

Merge branch 'master' into import_quiche

fcdd491

Apply fix_format changes.

8587991

Signed-off-by: Michael Warres <mpw@google.com>

Fix clang-tidy errors.

afde556

Signed-off-by: Michael Warres <mpw@google.com>

Fix mac ci failure caused by differences in OS X version of sed.

02219ba

Signed-off-by: Michael Warres <mpw@google.com>

Merge branch 'master' into import_quiche

11522d6

Add explanatory comments to quiche.BUILD, along with misc TODOs.

90a5c01

Signed-off-by: Michael Warres <mpw@google.com>

Flesh out QUICHE http2 platform impls.

09a72b8

For the impl files that are trivially implemented, add the implementation. For the impl files that require more work, add TODO comments. Signed-off-by: Michael Warres <mpw@google.com>

mpwarres added 3 commits January 14, 2019 21:59

Merge branch 'master' into import_quiche

736e15c

Resolve ODR violations by removing dependency on Envoy assert_lib.

d011bcb

Signed-off-by: Michael Warres <mpw@google.com>

Tweak quiche.BUILD deps clause to allow QUICHE dependency on Envoy bu…

ef361ef

…ild targets. Signed-off-by: Michael Warres <mpw@google.com>

htuch reviewed Jan 16, 2019

View reviewed changes

mpwarres added 2 commits January 17, 2019 09:13

Remove QUICHE dependency on Envoy build target until Bazel fix is rel…

b27d83a

…eased. Signed-off-by: Michael Warres <mpw@google.com>

Add comments that QUICHE platform files should not be used directly b…

713eb9f

…y Envoy code. Signed-off-by: Michael Warres <mpw@google.com>

Merge branch 'master' into import_quiche

bc88212

Signed-off-by: Michael Warres <mpw@google.com>

htuch approved these changes Jan 18, 2019

View reviewed changes

htuch merged commit 020c720 into envoyproxy:master Jan 18, 2019

mpwarres deleted the import_quiche branch January 18, 2019 18:28

mpwarres mentioned this pull request Jan 22, 2019

Start populating QUICHE platform implementation. #5684

Merged

This was referenced Jan 29, 2019

For QUICHE integration, add --experimental_remap_main_repo to bazelrc. #5759

Closed

For QUICHE integration, add --experimental_remap_main_repo to bazelrc. #5767

Closed


		// TODO: implement

		#define VERIFY_AND_RETURN_SUCCESS(expression) 0

Conversation

mpwarres commented Jan 9, 2019

Uh oh!

mpwarres commented Jan 9, 2019

Uh oh!

alyssawilk commented Jan 9, 2019

Uh oh!

alyssawilk commented Jan 9, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

htuch commented Jan 11, 2019

Uh oh!

mpwarres commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpwarres commented Jan 14, 2019

Uh oh!

mpwarres commented Jan 15, 2019

Uh oh!

mpwarres commented Jan 15, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

mpwarres Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

mpwarres Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

mpwarres commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mpwarres commented Jan 18, 2019

Uh oh!

mpwarres commented Jan 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mpwarres commented Jan 14, 2019 •

edited

Loading

mpwarres commented Jan 17, 2019 •

edited

Loading