Always use IPv6 sockets on Android by mattklein123 · Pull Request #21803 · envoyproxy/envoy

mattklein123 · 2022-06-21T16:02:40Z

Investigation shows that Android always uses IPv6 dual stack sockets. There is a chance that some long tail connectivity issues are related to this. This change adds a runtime guard (applicable to Android only) which will force use of IPv6 sockets and translate IPv4 addresses to mapped IPv6 addresses upon connection. This is the same as what is done for Java platform sockets.

Risk Level: None, runtime guarded to false on Android.
Testing: No possible tests here. EM will provide testing.
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: Android only
Runtime guard: envoy.reloadable_features.android_always_use_v6 (default false)

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

mattklein123 · 2022-06-21T16:03:19Z

cc @Reflejo @alyssawilk @Augustyniak

alyssawilk

yeesh, good find. Any idea if this is true for the versions of android E-M supports?

source/common/network/io_socket_handle_impl.cc

alyssawilk · 2022-06-21T16:21:48Z

source/common/runtime/runtime_features.cc

 // Used to track if runtime is initialized.
 FALSE_RUNTIME_GUARD(envoy_reloadable_features_runtime_initialized);
+// TODO(mattklein123): Flip this to true and/or remove completely once verified by Envoy Mobile.
+FALSE_RUNTIME_GUARD(envoy_reloadable_features_android_always_use_v6);


Also please unit test, or add unit tests to the TODO here if the timing is too tight.

Do you have a suggestion of how to unit test this in this repo? This is all behind an android only ifdef.

random drive by comment (ignore me if this doesnt make sense) can you make this:

#if defined(__ANDROID_API__) || defined(ENVOY_FORCE_ANDROID)

and add a bazel defines to test it?

if you want to resuse this you can wrap it with a constexpr bool function and reuse it in the codebase.

I think I would prefer to not have a build flag for it since that will be harder to test. I think Alyssa's suggestion is good to just refactor it and add a unit test. I can do that but will do it as a follow up.

mattklein123 · 2022-06-21T16:41:21Z

Any idea if this is true for the versions of android E-M supports?

I haven't looked back through the Android history. It's true on main branch. I can take a look through previous versions of someone can tell me which versions to look back to.

mattklein123 · 2022-06-21T16:57:25Z

I haven't looked back through the Android history. It's true on main branch. I can take a look through previous versions of someone can tell me which versions to look back to.

I looked and this applies all the way back to Android 6: https://cs.android.com/android/platform/superproject/+/android-6.0.1_r81:libcore/luni/src/main/native/libcore_io_Posix.cpp;l=91-128

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

alyssawilk · 2022-06-21T17:25:04Z

Give connect method a boolean argument which defaults to forceV6ForAndroid() and explicitly set the boolean in test? Otherwise test upstream for E-M on android only CI? Again I'm fine with TODO but I think it's possible to test with a little refactor work.

mattklein123 · 2022-06-21T18:04:30Z

Otherwise test upstream for E-M on android only CI?

Yes my assumption is this will be implicitly tested in upstream EM CI, but yeah, let me add a TODO for unit testing this if this solution holds.

alyssawilk

I'm not super confident of the change but as it's off by default and y'all are testing it I'm totally fine letting you folks figure out if there are corner cases missed :-) Good luck!

@mattklein123

Description: Bumping upstream Envoy sha so that it includes recent changes that allow us to force IPv6 use on Android. The Envoy sha that we use in this PR is NOT from Envoy main. The idea is to pick up only @mattklein123's change from envoyproxy/envoy#21803 without any other changes. We are going to move back to Envoy main later this week. Risk Level: Low, disabled by default Testing: Manual, passed Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

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

alyssawilk · 2022-06-22T13:09:07Z

/retest

repokitteh-read-only · 2022-06-22T13:09:11Z

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

🐱

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

see: more, trace.

Signed-off-by: Matt Klein <mklein@lyft.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>

@mattklein123

Description: Bumping upstream Envoy sha so that it includes recent changes that allow us to force IPv6 use on Android. The Envoy sha that we use in this PR is NOT from Envoy main. The idea is to pick up only @mattklein123's change from envoyproxy/envoy#21803 without any other changes. We are going to move back to Envoy main later this week. Risk Level: Low, disabled by default Testing: Manual, passed Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Description: Forcing the use of IPv6 socket addresses is required to make Envoy Mobile work with some carriers when running on an Android device. The option was introduced upstream in envoyproxy/envoy#21803. Risk Level: Low, enabled a well tested option. Testing: Manual, launched the example app Docs Changes: Updated Release Notes: Updated Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>

Description: Forcing the use of IPv6 socket addresses is required to make Envoy Mobile work with some carriers when running on an Android device. The option was introduced upstream in envoyproxy/envoy#21803. Risk Level: Low, enabled a well tested option. Testing: Manual, launched the example app Docs Changes: Updated Release Notes: Updated Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: Chidera Olibie <colibie@google.com>

@mattklein123

Description: Bumping upstream Envoy sha so that it includes recent changes that allow us to force IPv6 use on Android. The Envoy sha that we use in this PR is NOT from Envoy main. The idea is to pick up only @mattklein123's change from #21803 without any other changes. We are going to move back to Envoy main later this week. Risk Level: Low, disabled by default Testing: Manual, passed Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Description: Forcing the use of IPv6 socket addresses is required to make Envoy Mobile work with some carriers when running on an Android device. The option was introduced upstream in #21803. Risk Level: Low, enabled a well tested option. Testing: Manual, launched the example app Docs Changes: Updated Release Notes: Updated Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

mattklein123 added 5 commits June 16, 2022 20:54

tmp: always use v6 on android

e2afd07

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

Merge remote-tracking branch 'origin/main' into android-v6

253fd88

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

fix

5d32db8

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

Merge remote-tracking branch 'origin/main' into android-v6

834822d

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

add runtime

6f2721f

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

mattklein123 assigned Augustyniak and alyssawilk Jun 21, 2022

Augustyniak previously approved these changes Jun 21, 2022

View reviewed changes

alyssawilk reviewed Jun 21, 2022

View reviewed changes

comments

1f1588c

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

mattklein123 dismissed Augustyniak’s stale review via 1f1588c June 21, 2022 17:06

alyssawilk previously approved these changes Jun 21, 2022

View reviewed changes

Augustyniak mentioned this pull request Jun 21, 2022

Bump Envoy sha to f49d4f2 envoyproxy/envoy-mobile#2380

Merged

mattklein123 added 2 commits June 21, 2022 21:32

Merge remote-tracking branch 'origin/main' into android-v6

eff6d25

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

comment

10ff948

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

mattklein123 dismissed alyssawilk’s stale review via 10ff948 June 21, 2022 21:33

alyssawilk approved these changes Jun 22, 2022

View reviewed changes

mattklein123 enabled auto-merge (squash) June 22, 2022 14:44

mattklein123 merged commit 665b4d5 into main Jun 22, 2022

mattklein123 deleted the android-v6 branch June 22, 2022 15:17

Augustyniak mentioned this pull request Jun 24, 2022

Update Envoy to 665b4d5 envoyproxy/envoy-mobile#2373

Merged

Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022

Always use IPv6 sockets on Android (envoyproxy#21803)

b760711

Signed-off-by: Matt Klein <mklein@lyft.com> Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>

Augustyniak mentioned this pull request Sep 1, 2022

android: enable forceIPv6 by default envoyproxy/envoy-mobile#2510

Merged

Augustyniak mentioned this pull request Apr 12, 2023

mobile connectivity: degraded performance for specific domains-carriers pairs #26707

Open

Conversation

mattklein123 commented Jun 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Jun 21, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alyssawilk Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

davinci26 Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

mattklein123 Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jun 21, 2022

Uh oh!

mattklein123 commented Jun 21, 2022

Uh oh!

alyssawilk commented Jun 21, 2022

Uh oh!

mattklein123 commented Jun 21, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jun 22, 2022

Uh oh!

repokitteh-read-only bot commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattklein123 commented Jun 21, 2022 •

edited

Loading