http3: further cleanup and extension removal

[mattklein123]

Fixes #12829

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

[mattklein123]

Opening as a draft to check CI. I need to add more test coverage of the QUIC disabled case to make sure we do sufficient configuration error checking and can't hit any of the not reached guards.

[repokitteh-read-only]

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

🐱

Caused by: #15752 was synchronize by mattklein123.

see: more, trace.

[mattklein123]

@alyssawilk @danzh2010 I think this should be ready for review.

[alyssawilk]

So. Much. Cleaner.

Honestly I would tend to stick with some of the factories over proliferation of #defines but that's style and I'm good either way.

[mattklein123]

Honestly I would tend to stick with some of the factories over proliferation of #defines but that's style and I'm good either way.

I can take a look at stubs instead, which I may have to do if we want to fully compile everything out even for tests. I will take another look.

[mattklein123]

@PiotrSikora the last commit should fix the fips build. cc @keith thanks for working through that with me.

@alyssawilk this whole situation is honestly a total mess and I think we clean it up with a new feature in bazel 4.0 which supports targets being "compatible with" flags. I would suggest we land this, upgrade to 4.0, and then we can clean up further with actually fully blocking the targets if disabled?cc @lizan

[PiotrSikora]

@PiotrSikora the last commit should fix the fips build. cc @keith thanks for working through that with me.

Thanks! I can confirm it fixes the issue.

[alyssawilk]

Just when we thought it couldn't get any harier :-P

question - do you think we should have the compile options build do one pass with
!quic
and another with
!fips
to futureproof against that sort of regression? I'm inlined to think it's worth it but we can always try with this as-is and revisit if there are future breakages

[alyssawilk]

Hey @lizan can you do a pass of just the bazel magic?

[mattklein123]

cc @keith also

[keith]

As long as this works it's definitely fine, I'm impressed it does though, we should separately change the apple config to use this instead, looking at the skylib impl it's very hard to parse, but I believe it's doing that same hack https://github.com/bazelbuild/bazel-skylib/blob/f80bc733d4b9f83d427ce3442be2e07427b2cc8d/lib/selects.bzl#L145-L186

[mattklein123]

Believe it or not, I found this trick by googling, where I found the bazel issue you opened about the apple hack, where the person commented it could be done with this thing, but it wasn't merged yet. Now it's merged. :)

[keith]

Amazing, I forgot this, here's the update for the other ones that can benefit from this #15797

[bencebeky]

The change to HttpIntegrationTest::makeClientConnectionWithOptions() in test/integration/http_integration.cc breaks compilation on certain embedders with the following error message:
third_party/envoy/src/test/integration/http_integration.cc:239:1: error: non-void function does not return a value in all control paths [-Werror,-Wreturn-type]

This is part of presubmit and thus blocking submission of any CL for our project. Please fix. (Unfortunately I do not have my Envoy checkout any more, otherwise I would fix it myself.)

[alyssawilk]

I'll take a look