Skip to content
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

proposal: remove support for cares #2534

Closed
1 task
Augustyniak opened this issue Sep 8, 2022 · 0 comments · Fixed by #2618
Closed
1 task

proposal: remove support for cares #2534

Augustyniak opened this issue Sep 8, 2022 · 0 comments · Fixed by #2618

Comments

@Augustyniak
Copy link
Contributor

Augustyniak commented Sep 8, 2022

cares has been historically used on non Apple platforms for DNS resolution. Since in #2511 we moved to using getaddrinfo-based DNS resolver by default instead the proposal it to remove the alternative codepath that uses cares.

We've experimented with cares a lot, while we were doing that we introduced a functionality that was supposed to make cares a viable DNS resolution mechanism.

When we noticed that cares didn't work well with some VPNs that were doing DNS tunnelling (i.e. add blockers) we added a addDNSFallbackNameservers method envoyproxy/envoy#19010 to the Kotlin builder. We used that method specify 8.8.8.8 as DNS fallback for cases where a raw cares was not working due to aforementioned VPNs.

Later on, we noticied that even with addDNSFallbackNameservers(listOf('8.8.8.8')) call in our apps there were still some VPNs that caused cares DNS resolution to fail. That's when we finally decided to move to using a DNS resolution mechanism that would depend on getaddrinfo method. It was added in envoyproxy/envoy#22080.

Since we introduced getaddrinfo based DNS resolution we did not see any issues with its performance for when VPN is enabled on user's device. On top of that our A/B experiment between cares and getaddrinfo DNS resolutions showed no difference in any of our top level business metrics. The one thing that we noticed was an increase in p99 in the duration of the first request upon the start of the Envoy Engine (at the same time we did not see any difference in p95).

Discussion of pros and cons of Envoy Mobile moving away from supporting cares:
Pros:

  • the removal of addDNSFallbackNameservers method call from the engine builder. This method does not exist on iOS builder. The removal would help us to make Apple and non Apple engine builder API's more 1 to 1.
  • the removal of enableDNSUseSystemResolver method call from the engine builder. This method does not exist on iOS builder. The removal would help us to make Apple and non Apple engine builder API's more 1 to 1.
  • Simpler setup of the engine builder as addDNSFallbackNameservers call wouldn't be needed to make cares work with some (not all) VPNs anymore.
  • Less code is simpler code.
  • When we remove cares we can focus on adding the missing (pointed out in Cons section) features to getaddrinfo DNS resolution. There is no point in maintaining cares implementation if we have no clear plan to make it work with VPNs.

Cons:

  • cares supports DNS TTL

Things to check:

  • Is getaddrinfoavailable on all non Apple platforms that Envoy Mobile supports?
@Augustyniak Augustyniak changed the title proposal: stop using cares proposal: remove support for cares Sep 8, 2022
Augustyniak added a commit that referenced this issue Oct 21, 2022
Description: Remove the usage of cares DNS resolver, use `getaddrinfo` based DNS resolver instead. Fixes #2534.
Risk Level: Low, Envoy Mobile default was not to use cares.
Testing: Existing unit/integration tests.
Docs Changes: N/A
Release Notes: Done

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
jpsim pushed a commit to envoyproxy/envoy that referenced this issue Nov 29, 2022
Description: Remove the usage of cares DNS resolver, use `getaddrinfo` based DNS resolver instead. Fixes envoyproxy/envoy-mobile#2534.
Risk Level: Low, Envoy Mobile default was not to use cares.
Testing: Existing unit/integration tests.
Docs Changes: N/A
Release Notes: Done

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant