http3: make quic/quiche core code by mattklein123 · Pull Request #15720 · envoyproxy/envoy

mattklein123 · 2021-03-26T21:52:53Z

Part of #12829. The transport
extension will stay a built-in extension since it fits well. UDP
listener and UDP writer extension points have been removed. GSO is
still only enabled for QUIC because it currently depends on QUICHE, has
some obvious perf issues, and is failing non-QUIC integration tests.
Futher work is needed to remove codec extension factories.

Risk Level: Low
Testing: Existing/modified tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Part of #12829. The transport extension will stay a built-in extension since it fits well. UDP listener and UDP writer extension points have been removed. GSO is still only enabled for QUIC because it currently depends on QUICHE, has some obvious perf issues, and is failing non-QUIC integration tests. Futher work is needed to remove codec extension factories. Part of #12829 Signed-off-by: Matt Klein <mklein@lyft.com>

repokitteh-read-only · 2021-03-26T21:53:04Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15720 was opened by mattklein123.

see: more, trace.

mattklein123 · 2021-03-26T21:53:57Z

cc @moderation if you want to test this I believe this should allow you to compile out H3. There is some more cleanup work to do but let me know if that's not the case.

mattklein123 · 2021-03-26T22:54:19Z

I might have missed a few spots for fully compiling out. Will continue in the next follow up.

moderation · 2021-03-27T00:48:02Z

@mattklein123 I can confirm that this allowed me to compile on Red Hat 8 with Clang 10. Thanks

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2021-03-29T00:42:29Z

I believe the clang tidy issues are pre-existing. I will poke around a bit but my preference would be to merge this modulo any other PR comments, just to avoid merge conflicts.

Signed-off-by: Matt Klein <mklein@lyft.com>

alyssawilk

Awesome, thanks for tackling this.

api/envoy/config/listener/v3/udp_listener_config.proto

source/common/quic/BUILD

source/server/listener_manager_impl.cc

alyssawilk · 2021-03-29T13:24:17Z

test/common/quic/envoy_quic_proof_verifier_test.cc

 #include <memory>

-#include "extensions/quic_listeners/quiche/envoy_quic_proof_verifier.h"
+#include "common/quic/envoy_quic_proof_verifier.h"


so I'm all for QUIC being built in, but why the big file move? If we have TLS in extensions why not QUIC?
(I'm also happy with everything core being in core, but we have a bunch of other exceptions so I'd like to make sure we have a plan and maybe file a follow up issue for others)

The other things that are "core extensions" fall into 2 buckets today:

TLS which is an extension because technically there is an openssl port out there.

Extensions that actually are true extensions in terms of documentation, proto build, etc. but we force into the build because they are needed for core functionality (like admin server, etc.).

IMO QUIC doesn't fall into either of those categories which is why I moved the code. I just can't see any situation in which we will override the QUICHE code realistically.

test/common/quic/integration/BUILD

alyssawilk · 2021-03-29T14:17:06Z

Hey Matt, can you fold this into your mega PR:
https://github.com/envoyproxy/envoy/compare/main...alyssawilk:quic_upstream?expand=1
I'm also happy to send it out as a standalone but I figure it's easier for you to do it than deal with merge conflicts.

@moderation caught that Envoy currently doesn't do quic upstream because I didn't stick client_connection_factory_lib in the envoy select for the server build, I only included it in the integration tests.

mattklein123 · 2021-03-29T16:01:44Z

@moderation caught that Envoy currently doesn't do quic upstream because I didn't stick client_connection_factory_lib in the envoy select for the server build, I only included it in the integration tests.

Yeah I will merge that in. FYI I'm doing a follow up to this PR that will further clean all of the client/codec stuff up but we can have this in for now.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2021-03-29T16:38:21Z

@alyssawilk updated per comments

alyssawilk

I'm fine merging over clang tidy failures, as IMO there's no need to clean up all the things. I would suggest waiting for at least the other builds to pass as for example I wonder if "is_quic" is going to be 'unused' in opt mode.

cc @DavidSchinazi @danzh2010 as this is going to break everything we have in flight. Whee! =P

alyssawilk · 2021-03-29T16:43:43Z

source/server/BUILD

        "@envoy_api//envoy/extensions/filters/listener/proxy_protocol/v3:pkg_cc_proto",
    ] + envoy_select_enable_http3([
        "//source/common/quic:active_quic_listener_lib",
+        "//source/common/quic:client_connection_factory_lib",


I'd like this removed from quic protocol test too in the hopes of future-proofing but I can do in one of my many quic PRS.

alyssawilk · 2021-03-29T16:45:37Z

source/server/listener_manager_impl.cc

 #else
-  UNREFERENCED_PARAMETER(listener_);
+  // When QUIC is compiled out it should not be possible to configure either the QUIC transport
+  // socket or the QUIC listener and get to this point.


mind filing a tracking issue for upstream/downstream quic to have tests in the compile time options build that we fail gracefully if H3 is configured upstream/downstream while compiled out?

I made a note here: #12829 (comment)

I will work on this in the follow up PR I have ongoing to remove codec/client extension points for QUIC.

mattklein123 · 2021-03-29T16:48:45Z

I would suggest waiting for at least the other builds to pass as for example I wonder if "is_quic" is going to be 'unused' in opt mode.

I think the ASSERT macro handles this correctly but let's see. I will definitely wait for the builds to complete.

cc @DavidSchinazi @danzh2010 as this is going to break everything we have in flight. Whee! =P

Thank you for dealing with the pain!!!

PiotrSikora · 2021-03-30T19:29:00Z

FYI, this broke builds with --define boringssl=fips.

ggreenway · 2021-03-30T19:51:41Z

FYI, this broke builds with --define boringssl=fips.

How did that pass CI? The compile_time_options build is compiled with boringssl=fips, and it looks like it passed.

alyssawilk · 2021-03-30T19:54:24Z

compile time options is also where we check that quic compile out works. I think we need to do 2 build passes to make this work correctly :-/

…

On Tue, Mar 30, 2021 at 3:51 PM Greg Greenway ***@***.***> wrote: FYI, this broke builds with --define boringssl=fips. How did that pass CI? The compile_time_options build is compiled with boringssl=fips, and it looks like it passed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15720 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPP5XWS7IX2M3BJVIMDTGITVZANCNFSM4Z4E2FUA> .

PiotrSikora · 2021-03-30T19:55:45Z

FYI, this broke builds with --define boringssl=fips.

How did that pass CI? The compile_time_options build is compiled with boringssl=fips, and it looks like it passed.

I don't know, yet. I originally suspected compile_time_options passed because this PR added --@envoy//bazel:http3=False, but that doesn't seem to work for me.

mattklein123 · 2021-03-30T20:02:25Z

I will take a look and fix. I suspect also that fips with http3=False works, but not just fips.

mattklein123 · 2021-03-30T20:06:59Z

@PiotrSikora are you also passing --test_tag_filters=-nofips and --build_tag_filters=-nofips? I'm trying to figure clean this up further in #15752 but unfortunately this is not easy.

mattklein123 · 2021-03-30T20:37:03Z

I can confirm that bazel build //source/exe:envoy-static --define boringssl=fips --test_tag_filters=-nofips --build_tag_filters=-nofips is broken. I will try to fix it but as I said above I'm getting into a real bazel mess here.

mattklein123 · 2021-03-30T21:08:41Z

ubuntu@ip-10-1-0-68:~/Source/envoy$ git diff
diff --git a/bazel/BUILD b/bazel/BUILD
index 2cb2c4330..a0e97c5e8 100644
--- a/bazel/BUILD
+++ b/bazel/BUILD
@@ -363,6 +363,7 @@ config_setting(
     constraint_values = [
         "@bazel_tools//platforms:linux",
         "@bazel_tools//platforms:x86_64",
+        "disable_http3",
     ],
     values = {"define": "boringssl=fips"},
 )

At least makes this problem obvious, so I will add that to my other PR while I see if I can figure out something better.

Edit: nevermind that doesn't seem to work. Will keep poking.

mattklein123 requested review from alyssawilk, ggreenway and htuch as code owners March 26, 2021 21:52

repokitteh-read-only bot added the api label Mar 26, 2021

repokitteh-read-only bot assigned markdroth Mar 26, 2021

mattklein123 assigned moderation, ggreenway, alyssawilk and danzh2010 and unassigned markdroth Mar 26, 2021

mattklein123 mentioned this pull request Mar 26, 2021

ci: making sure we test building sans quic #15655

Closed

mattklein123 added 2 commits March 28, 2021 17:30

Merge remote-tracking branch 'origin/main' into remove_quic_as_extension

f7063ef

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

296368e

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

7428546

Signed-off-by: Matt Klein <mklein@lyft.com>

alyssawilk reviewed Mar 29, 2021

View reviewed changes

comments

fb6f911

Signed-off-by: Matt Klein <mklein@lyft.com>

alyssawilk approved these changes Mar 29, 2021

View reviewed changes

mattklein123 mentioned this pull request Mar 29, 2021

quic: remove ability to have pluggable implementation #12829

Closed

mattklein123 merged commit 8e8a21d into main Mar 29, 2021

mattklein123 deleted the remove_quic_as_extension branch March 29, 2021 20:01

Conversation

mattklein123 commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 26, 2021

Uh oh!

mattklein123 commented Mar 26, 2021

Uh oh!

mattklein123 commented Mar 26, 2021

Uh oh!

moderation commented Mar 27, 2021

Uh oh!

mattklein123 commented Mar 29, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alyssawilk Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk commented Mar 29, 2021

Uh oh!

mattklein123 commented Mar 29, 2021

Uh oh!

mattklein123 commented Mar 29, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Mar 29, 2021

Uh oh!

PiotrSikora commented Mar 30, 2021

Uh oh!

ggreenway commented Mar 30, 2021

Uh oh!

alyssawilk commented Mar 30, 2021 via email

Uh oh!

PiotrSikora commented Mar 30, 2021

Uh oh!

mattklein123 commented Mar 30, 2021

Uh oh!

mattklein123 commented Mar 30, 2021

Uh oh!

mattklein123 commented Mar 30, 2021

Uh oh!

mattklein123 commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mattklein123 commented Mar 26, 2021 •

edited

Loading

mattklein123 commented Mar 30, 2021 •

edited

Loading