dns: Add `AppleDnsResolverConfig.include_unroutable_families` config by jpsim · Pull Request #20527 · envoyproxy/envoy

jpsim · 2022-03-25T12:07:12Z

Commit Message: dns: Add AppleDnsResolverConfig.include_unroutable_families config
Additional Description: If set, the resolver will avoid the system's heuristics to only return IPv4 or IPv6 addresses that it considers to be "routable", instead returning all possible IPv4 or IPv6 addresses. This setting is ignored if the DNS lookup family is set to v4-only or v6-only. This may be a useful setting to specify if the addresses considered unroutable by the system's heuristics may in practice be routable.
Risk Level: Low, off by default, only impacts Apple platforms.
Testing: Added unit tests. I've confirmed both branches of the code paths are hit when the flag is on and off via Envoy Mobile on a physical iPhone.
Docs Changes: Proto docs added
Release Notes: Added
Platform Specific Features: Apple-platforms only

If set, the resolver will avoid the system's heuristics to only return IPv4 or IPv6 addresses that it considers to be "routable", instead returning all possible IPv4 or IPv6 addresses. This setting is ignored if the DNS lookup family is set to v4-only or v6-only. This may be a useful setting to specify if the addresses considered unroutable by the system's heuristics may in practice be routable. Signed-off-by: JP Simard <jp@jpsim.com>

repokitteh-read-only · 2022-03-25T12:07:15Z

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: #20527 was opened by jpsim.

see: more, trace.

repokitteh-read-only · 2022-03-25T12:07:20Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #20527 was opened by jpsim.

see: more, trace.

Will add more to test the new functionality soon. Signed-off-by: JP Simard <jp@jpsim.com>

jpsim · 2022-03-25T18:38:08Z

This mostly exposing a way to configure the behavior we had prior to #18418.

htuch · 2022-03-25T21:56:54Z

if the addresses considered unroutable by the system's heuristics may in practice be routable.

Can you elaborate on this? Would be curious to understand if the issue is the heuristics or that we probably should fundamentally have the escape hatch because heuristics are heuristics. CC @mattklein123 @yanavlasov

jpsim · 2022-03-28T15:55:47Z

Can you elaborate on this? Would be curious to understand if the issue is the heuristics or that we probably should fundamentally have the escape hatch because heuristics are heuristics.

Sure, for starters the heuristics are described here:

envoy/source/extensions/network/dns_resolver/apple/apple_dns_impl.cc

Lines 267 to 273 in d2df902

    
                * If neither flag is set, the system will apply an intelligent heuristic, which 
        
                * is (currently) that it will attempt to look up both, except: 
        
                * If "hostname" is a wide-area unicast DNS hostname (i.e. not a ".local." name) but 
        
                * this host has no routable IPv6 address, then the call will not try to look up IPv6 
        
                * addresses for "hostname", since any addresses it found would be unlikely to be of 
        
                * any use anyway. Similarly, if this host has no routable IPv4 address, the call will 
        
                * not try to look up IPv4 addresses for "hostname".

It's probably better to rely on the heuristic most of the time, vs listing all available interfaces, but specifically in the case of Happy Eyeballs, we want to take the process of determining the best interface into our own hands, in which case we think we might do a better job than the system heuristic.

We're not sure though, which is why this is configurable and disabled by default. We intend to experiment with this in concert with Happy Eyeballs in Envoy Mobile in the near future.

cc @goaway

mattklein123

Thanks makes sense to me at a high level.

/wait

mattklein123 · 2022-03-28T16:40:46Z

api/envoy/extensions/network/dns_resolver/apple/v3/apple_dns_resolver.proto

+  // The resolver will avoid the system's heuristics to only return
+  // IPv4 or IPv6 addresses that it considers to be "routable", instead
+  // returning all possible IPv4 or IPv6 addresses. This setting is
+  // ignored if the DNS lookup family is set to v4-only or v6-only.
+  bool include_unroutable_families = 1;


This makes sense to me, but potentially mention that is useful with happy eyeballs and for parity with the c-ares implementation? I think it will make it more clear why someone would want to do this and answer @htuch concerns.

Yes, definitely, this totally makes sense with the mention of Happy Eyeballs.

* main: (150 commits) examples: Cleanups for docker compose (#20777) adaptive concurrency / admission control: mark non-alpha (#20775) listeners: add unified matcher for filter chains (#20110) xds: don't allow multiple instances of a locality in the same priority (#20574) Update QUICHE from bac04054b to 8864d08f6 (#20747) http_connection_manager: support multiple SAN URIs for XFCC (#20724) contrib-sipproxy: Fix the coredump when teardown (#20580) bazel: Update to 5.1.1 thrift: Explicitly express no cluster header for weighted clusters (#20679) conn_pool_grid: try TCP immediately with h3 state FailedRecently (#20722) build: Pin distroless base image with sha (#20759) ci: Pin macos-latest VM version (#20758) compression: add zstd compressor and decompressor (#20434) ratelimit add support for custom http response code (#20520) delta-xds: fix wildcard resource versions (#20726) ci: remove no-op bazel option ci: add contrib target (#20732) Disable test for failing filters in filter_fuzz_test (#20742) build(deps): bump github/codeql-action from 2.1.7 to 2.1.8 (#20739) deps: Bump `com_github_bufbuild_buf` -> v1.3.1 (#20692) ... Signed-off-by: JP Simard <jp@jpsim.com>

* main: (52 commits) stats: change Stats::ScopePtr references to Stats::ScopeSharedPtr (#20871) stats: Repro & fix admin stats crash (#20855) Add async_files library to support filesystem buffering and filesystem caching (#20332) api: Add validate to required member. (#20881) docs: fix release notes (#20879) ext_proc: Support per-route GrpcService configuration. (#20757) matchers: extend network inputs to HTTP requests (#20796) listener: remove the peek from the listener filters (#20728) docs: fix doc link for composite filter (#20865) bazel: move shared action_env above enable_platform_specific_config build(deps): bump pyparsing from 3.0.7 to 3.0.8 in /tools/dependency (#20769) build(deps): bump setuptools from 62.0.0 to 62.1.0 in /tools/base (#20771) build(deps): bump actions/setup-python from 3.1.0 to 3.1.2 (#20770) dist: Add debian packaging (#17979) Add bazel option for enabling Universal Header Validator (#20845) tls: remove SHA-1 cipher suites from the defaults on the server-side (#20643) build(deps): bump actions/checkout from 3.0.0 to 3.0.1 (#20848) test: allow dispatcher test cases running on ipv6 only env (#20854) release: kick off 1.23 (#20849) release: 1.22 (#20847) ... Signed-off-by: JP Simard <jp@jpsim.com>

Signed-off-by: JP Simard <jp@jpsim.com>

goaway · 2022-04-19T18:53:43Z

Just to drop a clarifying comment on here - we generally believe that having this heuristic in place on iOS should be an improvement, at least based on its description, but nonetheless, we have observed cases in practice where the Apple DNS API returns a single non-functional IP when we know that we've advertised both versions and the other version is the one that actually works.

Being able to quantify its impact would be useful, and also being able to run an experiment where we let Happy Eyeballs take precedence in selecting the preferred IP seems worth trying. And since Apple left the value configurable, it also seems reasonable to allow it to be configured through EM's interface as well.

Signed-off-by: JP Simard <jp@jpsim.com>

htuch

/lgtm api

htuch

LGTM, will defer to @mattklein123 to merge.

mattklein123

Thanks!

jpsim · 2022-04-20T16:44:27Z

Thanks for the reviews and the discussion. I'll try to include more context up front in the future.

Diff: envoyproxy/envoy@c96f711...60a13f3 Mostly pulling in for envoyproxy/envoy#20527 Signed-off-by: JP Simard <jp@jpsim.com>

…nvoyproxy#20527) If set, the resolver will avoid the system's heuristics to only return IPv4 or IPv6 addresses that it considers to be "routable", instead returning all possible IPv4 or IPv6 addresses. This setting is ignored if the DNS lookup family is set to v4-only or v6-only. This may be a useful setting to specify if the addresses considered unroutable by the system's heuristics may in practice be routable. Signed-off-by: JP Simard <jp@jpsim.com>

Diff: c96f711...60a13f3 Mostly pulling in for #20527 Signed-off-by: JP Simard <jp@jpsim.com>

repokitteh-read-only bot added the api label Mar 25, 2022

repokitteh-read-only bot assigned htuch Mar 25, 2022

Fix tests

d2df902

Will add more to test the new functionality soon. Signed-off-by: JP Simard <jp@jpsim.com>

mattklein123 self-assigned this Mar 28, 2022

mattklein123 requested changes Mar 28, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 28, 2022

jpsim mentioned this pull request Mar 30, 2022

iOS: Add includeUnroutableDNSResults to engine builder envoyproxy/envoy-mobile#2128

Closed

4 tasks

repokitteh-read-only bot removed the waiting label Apr 12, 2022

mattklein123 added the waiting label Apr 13, 2022

repokitteh-read-only bot removed the waiting label Apr 19, 2022

jpsim added 2 commits April 19, 2022 14:40

Mention Happy Eyeballs in the proto comment.

76a968c

Signed-off-by: JP Simard <jp@jpsim.com>

Add release notes

ac8594b

Signed-off-by: JP Simard <jp@jpsim.com>

jpsim added 2 commits April 19, 2022 15:16

fixup! Add release notes

73fb54d

Signed-off-by: JP Simard <jp@jpsim.com>

fixup! fixup! Add release notes

1604240

Signed-off-by: JP Simard <jp@jpsim.com>

mattklein123 added the waiting label Apr 19, 2022

Add tests

a4589fe

Signed-off-by: JP Simard <jp@jpsim.com>

repokitteh-read-only bot removed the waiting label Apr 19, 2022

jpsim marked this pull request as ready for review April 19, 2022 21:16

jpsim requested a review from yanavlasov as a code owner April 19, 2022 21:16

jpsim requested a review from mattklein123 April 19, 2022 21:16

htuch reviewed Apr 20, 2022

View reviewed changes

repokitteh-read-only bot removed the api label Apr 20, 2022

htuch approved these changes Apr 20, 2022

View reviewed changes

mattklein123 approved these changes Apr 20, 2022

View reviewed changes

mattklein123 merged commit 60a13f3 into envoyproxy:main Apr 20, 2022

jpsim deleted the jp-apple-dns-include-unroutable-families branch April 20, 2022 16:43

jpsim added a commit to envoyproxy/envoy-mobile that referenced this pull request Apr 20, 2022

Update Envoy to 60a13f3

4a1b613

Diff: envoyproxy/envoy@c96f711...60a13f3 Mostly pulling in for envoyproxy/envoy#20527 Signed-off-by: JP Simard <jp@jpsim.com>

jpsim mentioned this pull request Apr 20, 2022

Update Envoy to 60a13f3 envoyproxy/envoy-mobile#2169

Merged

jpsim added a commit to envoyproxy/envoy-mobile that referenced this pull request Apr 21, 2022

Update Envoy to 60a13f3 (#2169)

464cd31

Diff: envoyproxy/envoy@c96f711...60a13f3 Mostly pulling in for envoyproxy/envoy#20527 Signed-off-by: JP Simard <jp@jpsim.com>

jpsim added a commit that referenced this pull request Nov 29, 2022

Update Envoy to 60a13f3 (#2169)

949a267

Diff: c96f711...60a13f3 Mostly pulling in for #20527 Signed-off-by: JP Simard <jp@jpsim.com>

Conversation

jpsim commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 25, 2022

Uh oh!

repokitteh-read-only bot commented Mar 25, 2022

Uh oh!

jpsim commented Mar 25, 2022

Uh oh!

htuch commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpsim commented Mar 28, 2022

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Mar 28, 2022

Choose a reason for hiding this comment

Uh oh!

htuch Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

goaway commented Apr 19, 2022

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jpsim commented Apr 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jpsim commented Mar 25, 2022 •

edited

Loading

htuch commented Mar 25, 2022 •

edited

Loading