mobile: allow using a light listener manager by alyssawilk · Pull Request #24363 · envoyproxy/envoy

alyssawilk · 2022-12-05T18:04:36Z

This enables using Envoy Mobile from the real downstream listener manger to a lightweight listener manager, allowing compiling out all the downstream listener code.

The real listener is still by default. This should be largely a no-op for everything but the c++ CI.

Risk Level: low
Testing: c++ ci regression tests new code paths.
Docs Changes: n/a
Release Notes: n/a

Part of envoyproxy/envoy-mobile#2711

repokitteh-read-only · 2022-12-05T18:04:42Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24363 was opened by alyssawilk.

see: more, trace.

repokitteh-read-only · 2022-12-05T18:04:53Z

CC @envoyproxy/mobile-maintainers: FYI only for changes made to (mobile/).
envoyproxy/mobile-maintainers assignee is @Augustyniak

🐱

Caused by: #24363 was opened by alyssawilk.

see: more, trace.

alyssawilk · 2022-12-06T16:36:54Z

/retest

repokitteh-read-only · 2022-12-06T16:37:04Z

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

🐱

Caused by: a #24363 (comment) was created by @alyssawilk.

see: more, trace.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

jpsim · 2022-12-08T21:43:48Z

.github/workflows/android_tests.yml

            --build_tests_only \
            $([ -z $GITHUB_TOKEN ] || echo "--config=remote-ci-linux-clang") \
-            --remote_header="Authorization=Bearer $GITHUB_TOKEN" \
+             --copt=-DALLOW_TCP_LISTENERS --remote_header="Authorization=Bearer $GITHUB_TOKEN" \


Can this be moved to a .bazelrc config?

The goal here is to have E-M build without TCP listeners, but we exempt it for this CI due to the proxying tests. will comment inline.

But then these tests will fail when run locally?

Why not add build test --copt=-DALLOW_TCP_LISTENERS to the .bazelrc so it works both on CI and locally?

because then all tests will always run with TCP listeners, and we won't have regression testing for, say, the C++ tests running without TCP listeners.

What I'm hearing is you'd prefer I add it to the bazelrc, and explicitly not allow TCP listeners on all the non-legacy CI instead?

Hmm, yeah I was thinking a config that we could pass when running these tests specifically, but then it doesn't make much difference if you have to pass a config or a --copt because either way ./bazelw test ... won't be sufficient.

I'm ok keeping this as-is then.

envoy/server/options.h

jpsim · 2022-12-08T21:46:16Z

The real listener is still used if Envoy Mobile is compiled with --copt=-DALLOW_TCP_LISTENERS as evidenced by the kotlin build.

Why does Kotlin need to build with the real listener?

jpsim · 2022-12-08T21:46:53Z

Release Notes: maybe should have for E-M?

We have release notes in https://github.com/envoyproxy/envoy/blob/main/mobile/docs/root/intro/version_history.rst

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2022-12-09T13:16:34Z

Also adding in Snow for the changes to Envoy. Mind taking a look? Very minimal PR.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Augustyniak · 2022-12-12T14:37:39Z

A discussion related to this PR happened in Slack. The current hypothesis is that #24294 broke objc hello world app CI job which was disabled in #24363 to 'unblock' main (make it green again).

We think that your PR may fix objc hello world app issue but to test it we would need to re-enable the aforementioned job. Could we verify whether your PR fixes the objc hello world app CI job?

…xy#24478)" This reverts commit 0a8043c.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2022-12-12T19:29:38Z

Ok CI reinstated and passing (needed a different force-register)

Commit Message: Additional Description: Register default listener. Envoy Mobile crashes in optimized builds without this change. The crash was introduced in #24294. The objc hello world app CI job was disabled in #24363 to make main green again. Risk Level: Low Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Augustyniak · 2022-12-12T21:15:01Z

mobile/envoy_build_config/BUILD

    deps = [
        "extension_registry_platform_additions",
        "@envoy//source/common/network:socket_lib",
+        "@envoy//source/common/quic:quic_transport_socket_factory_lib",


This may be missing envoy_select_enable_http3 usage https://github.com/envoyproxy/envoy/pull/24496/files#diff-4517a4a52636c1c8e390c1d9653aa000a67fc1070d977ebc114ca9eacac2ec49R48

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mobile/envoy_build_config/BUILD

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Augustyniak · 2022-12-14T17:51:29Z

Why does Kotlin need to build with the real listener?

It's needed by Android proxy tests which spam 2 instances of Envoy Mobile, one of which acts as a "fake" proxy and uses the default/complex listener to handle incoming requests.

Augustyniak

Left a comment with regard to including the new api listener based on the value of the introduced flag.

It's not a hard blocker but it's worth noting that the size of the binary will actually go up by a tiny bit until we do not remove the old listener and if we always include the new listener as part of the build. Based on JP's numbers for the size increase I am not worried about the size increase tho (as it appears to be tiny).

Augustyniak · 2022-12-14T17:54:51Z

mobile/envoy_build_config/extension_registry.cc

  Envoy::Network::forceRegisterGetAddrInfoDnsResolverFactory();
  Envoy::Extensions::RequestId::forceRegisterUUIDRequestIDExtensionFactory();
  Envoy::Server::forceRegisterDefaultListenerManagerFactoryImpl();
+  Envoy::Server::forceRegisterApiListenerManagerFactoryImpl();


should we force register in here only if DUSE_API_LISTENER is set?

I was going to do that but I think it requires a bazel flag and additional plumbing to be useful given the library include

Augustyniak · 2022-12-14T17:55:34Z

mobile/envoy_build_config/BUILD

        "@envoy_mobile//library/common/extensions/filters/http/platform_bridge:config",
        "@envoy_mobile//library/common/extensions/filters/http/route_cache_reset:config",
        "@envoy_mobile//library/common/extensions/filters/http/socket_tag:config",
+        "@envoy_mobile//library/common/extensions/listener_managers/api_listener_manager:api_listener_manager_lib",


similar to my other comment - should we include this dep only if the new compiler/build flag is set?

I think it depends. if the long term plan is to have a bazel flag to have it optional, we can add it now. if long term we want to say E-M is a client library not a proxy, I'm inclined to not do the busy-work which I'd then have to unwind, and accept the additional include in the interim (we can remove it in-housE)

actually having thought a bit more I think it is sensible to have a flag but I would like to add it when we flip the default, so have one for "enable TCP listener" where right now we have compile support for "enable API listener"

repokitteh-read-only bot added the mobile label Dec 5, 2022

repokitteh-read-only bot assigned Augustyniak Dec 5, 2022

alyssawilk mentioned this pull request Dec 5, 2022

listeners: moving listeners to extension directory #24248

Merged

alyssawilk force-pushed the api_listener_manager branch from f0d0fb7 to fdc3eaa Compare December 6, 2022 15:14

alyssawilk force-pushed the api_listener_manager branch 2 times, most recently from 1dc4db7 to fdc3eaa Compare December 6, 2022 18:48

mobile: switch to a light listener manager

c4a121c

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk mentioned this pull request Dec 8, 2022

swift: stop using loopback for direct responses #24402

Merged

Attempting to fix kotlin tests

c19cdd7

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the api_listener_manager branch from fdc3eaa to c19cdd7 Compare December 8, 2022 20:38

alyssawilk added 2 commits December 8, 2022 15:42

Merge branch 'main' into api_listener_manager

e140c68

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

fixes

5fa97f2

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk marked this pull request as ready for review December 8, 2022 20:51

alyssawilk requested review from Augustyniak, RyanTheOptimist, abeyad and jpsim as code owners December 8, 2022 20:51

alyssawilk assigned jpsim Dec 8, 2022

jpsim reviewed Dec 8, 2022

View reviewed changes

envoy/server/options.h Show resolved Hide resolved

alyssawilk added 2 commits December 9, 2022 08:00

Merge branch 'main' into api_listener_manager

ade2883

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

comments and merge fixes

8883326

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk assigned snowp Dec 9, 2022

review comment

ba6d083

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Revert "ci: disable running the Objective-C integration app (envoypro…

8bed534

…xy#24478)" This reverts commit 0a8043c.

Augustyniak mentioned this pull request Dec 12, 2022

ci: testing re-enabling of objc hello world app #24489

Closed

Merge branch 'main' into api_listener_manager

b1ba5d5

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Augustyniak mentioned this pull request Dec 12, 2022

ci/mobile: register listener #24490

Merged

quic factory

3e2d82e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'main' into api_listener_manager

eaa6084

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Augustyniak reviewed Dec 12, 2022

View reviewed changes

select

663a784

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Augustyniak reviewed Dec 12, 2022

View reviewed changes

mobile/envoy_build_config/BUILD Outdated Show resolved Hide resolved

alyssawilk added 3 commits December 13, 2022 09:25

Merge branch 'main' into api_listener_manager

9da477c

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

merge fix

72c1c4c

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

off by default

feeb2dc

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk changed the title ~~mobile: switch to a light listener manager~~ mobile: allow using a light listener manager Dec 13, 2022

alyssawilk added 2 commits December 13, 2022 16:51

yaml fix

fca0861

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

reverse polarity

8dc5e5e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Augustyniak approved these changes Dec 14, 2022

View reviewed changes

alyssawilk enabled auto-merge (squash) December 14, 2022 18:12

jpsim approved these changes Dec 14, 2022

View reviewed changes

alyssawilk merged commit f15c4a1 into envoyproxy:main Dec 14, 2022

alyssawilk deleted the api_listener_manager branch April 5, 2023 16:39

Bfarkiani mentioned this pull request Dec 6, 2023

Envoy mobile old TCP listeners #31206

Closed

Conversation

alyssawilk commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Dec 5, 2022

Uh oh!

repokitteh-read-only bot commented Dec 5, 2022

Uh oh!

alyssawilk commented Dec 6, 2022

Uh oh!

repokitteh-read-only bot commented Dec 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpsim commented Dec 8, 2022

Uh oh!

jpsim commented Dec 8, 2022

Uh oh!

alyssawilk commented Dec 9, 2022

Uh oh!

Augustyniak commented Dec 12, 2022

Uh oh!

alyssawilk commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Augustyniak commented Dec 14, 2022

Uh oh!

Augustyniak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alyssawilk commented Dec 5, 2022 •

edited

Loading