-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
android: enable forceIPv6 by default #2510
Conversation
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
/retest |
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
docs/root/intro/version_history.rst
Outdated
@@ -47,6 +47,7 @@ Features: | |||
- api: improved C++ APIs compatibility with Java / Kotlin / Swift (:issue `#2362 <2362>`) | |||
- api: add option to use the a ``getaddrinfo``-based system DNS resolver instead of c-ares (:issue: `#2419 <2419>`) | |||
- iOS: add ``KeyValueStore`` protocol conformance to ``UserDefaults`` (:issue: `#2452 <2452>`) | |||
- android: enable forcing of IPv6 socket addresses by default and remove ``forceIPv6`` method from the engine builder. (:issue: `#2510 <2510>`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should adjust this line to reflect this:
- api: add experimental option to force all connections to use IPv6 (:issue: `#2379 <2379>`, :issue: `#2396 <2396>`)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, great point - done. I still use 'ios' / 'android' to stay consistent for this release but planning to move to swift etc in the next release.
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
{"dns_preresolve_hostnames", this->dns_preresolve_hostnames_}, | ||
{"dns_refresh_rate", fmt::format("{}s", this->dns_refresh_seconds_)}, | ||
{"dns_query_timeout", fmt::format("{}s", this->dns_query_timeout_seconds_)}, | ||
{"dns_resolver_name", dns_resolver_name}, {"dns_resolver_config", dns_resolver_config}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep these on two lines?
{"dns_resolver_name", dns_resolver_name}, {"dns_resolver_config", dns_resolver_config}, | |
{"dns_resolver_name", dns_resolver_name}, | |
{"dns_resolver_config", dns_resolver_config}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a linter tool that made a change in here. It does not allow me to make a change from your suggestion.
For reference, I ran ./tools/code_format/check_format.py fix ../library/
from within envoy
directory locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, interesting that it was previously on two lines.
…e-the-system-dns-resolver * origin/main: android: enable forceIPv6 by default (#2510) 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 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>
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