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.

Sign up for GitHub

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

Jump to bottom

dns: stop using cares DNS resolver #2618

Merged
merged 15 commits into from
Oct 21, 2022
Merged

dns: stop using cares DNS resolver #2618

merged 15 commits into from
Oct 21, 2022

Conversation

Augustyniak
Copy link
Contributor

@Augustyniak Augustyniak commented Oct 19, 2022 •
edited
Loading

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

@Augustyniak
remove usage of cares
f4b811c 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
lint fix
b3adcad 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
remove the use of removed API
13e8143 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Copy link
Contributor Author

/retest

@Augustyniak
remove not existing variables
3d02925 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
more fixes
f8c1687 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
lint fix
f345226 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
fix cc tests
1096b28 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Copy link
Contributor Author

/retest

1 similar comment
@Augustyniak
Copy link
Contributor Author

/retest

@Augustyniak
fix
fa84d2f 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Copy link
Contributor Author

/retest

@Augustyniak Augustyniak changed the title remove usage of cares dns: stop using cares DNS resolver Oct 19, 2022
@Augustyniak
update version history
cfe6915 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
update
ff47481 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak Augustyniak marked this pull request as ready for review October 19, 2022 16:57
@Augustyniak
update starting_envoy doc
1fc31f2 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Copy link
Contributor Author

I think that I should be able to remove some dependencies on cares from BUILD files too.

@Augustyniak
Copy link
Contributor Author

/retest

@Augustyniak
Merge branch 'main' into remove-support-for-cares
941da0c 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
alyssawilk
alyssawilk reviewed Oct 20, 2022
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sweet.

remove from envoy_build_config/extensions_build_config.bzl as well?

@Augustyniak
remove cares code
c95b854 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Copy link
Contributor Author

remove from envoy_build_config/extensions_build_config.bzl as well?

I do not see cares referenced in there.

I was able to remove a few more lines of code related to cares tho - c95b854

@Augustyniak
remove more
fdc9f78 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
lint fix
5a7d322 
Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
@Augustyniak
Copy link
Contributor Author

/retest

alyssawilk
alyssawilk reviewed Oct 20, 2022
View reviewed changes
library/cc/engine_builder.cc
@@ -202,27 +202,6 @@ EngineBuilder::enablePlatformCertificatesValidation(bool platform_certificates_v
}

std::string EngineBuilder::generateConfigStr() const {
#if defined(__APPLE__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have high confidence of consistently using the apple resolver without this block - I'd think we'd want to replace cares here with getaddrinfo. Do we have any e2e validation for that? Alternately, can you get a secondary Lyft reviewer?
cc @RyanTheOptimist because perhaps it won't take effect until we switch to the c++ builder by defualt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm following the PR and your comment correctly, I think the reason we'll use the Apple resolver without this block regardless of the Builder type (after this PR lands) is because of the change to config.cc:

https://github.com/envoyproxy/envoy-mobile/pull/2618/files#diff-1af53ae37f8c227f37b80bcb6e2335f55b2f830aa658ac77e099f2549f157712

Unless there is code to specifically override dns_resolver_config (which there shouldn't be since @Augustyniak seems to have removed it in this PR), it will use the Apple resolver on iOS and getaddrinfo elsewhere. Does that sound right to you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough. the one other thing i'd ask for is a follow-up tracking issue on removing the cares dependency, which may be an upstream thing

Copy link
Contributor Author

@Augustyniak Augustyniak Oct 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is code to specifically override dns_resolver_config (which there shouldn't be since @Augustyniak seems to have removed it in this PR), it will use the Apple resolver on iOS and getaddrinfo elsewhere. Does that sound right to you?

  1. iOS engine builder always uses apple dns resolver
  2. Kotlin engine builder uses apple dns resolver on mac and getaddrinfo everywhere else (linux/Android). I can modify it so that it always uses getaddtinfo - let me know.
  3. c++ engine builder uses apple dns resolver on mac/OS and getaddrinfo resolver everywhere else.

I would appreciate your thoughts on 2). Will look into @alyssawilk 's comment more and open GH issue if I cannot figure it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #2629

@Augustyniak
Copy link
Contributor Author

/retest

@Augustyniak Augustyniak mentioned this pull request Oct 21, 2022
Closed
@Augustyniak Augustyniak enabled auto-merge (squash) October 21, 2022 20:49
@Augustyniak Augustyniak merged commit c461cda into main Oct 21, 2022
@Augustyniak Augustyniak deleted the remove-support-for-cares branch October 21, 2022 20:50
jpsim added a commit that referenced this pull request Nov 4, 2022
@jpsim
Merge remote-tracking branch 'origin/main' into ios-remove-forceipv6-…
82af348 
…builder-function

* origin/main:
  remove the use of deprecated flag (#2658)
  Cronvoy: Map EM Errors to Cronet API Errors II (#1594) (#2633)
  build: revert boring patch (#2651)
  Bump Lyft Support Rotation (#2654)
  fix one issue blocking bumping Envoy (#2649)
  ci: remove Snow from Lyft EM rotation (#2650)
  ci: increasing timeouts (#2653)
  python: Apply Envoy python-yapf formatting (#2648)
  repo: Shellcheck cleanups (#2646)
  repo: Switch `pip_install` -> `pip_parse` (#2647)
  Use safe_malloc instead of new when creating new_envoy_map_entry (#2632)
  python: Pin requirement hashes (#2643)
  Disable flaky TestConfig.StringAccessors (#2642)
  ci: migrate from set-output to GITHUB_OUTPUT (#2625)
  Add a comment to addPlatformFilter (#2634)
  Allow Cronvoy to build with proguard. (#2635)
  Update Envoy (#2630)
  Add support for Platform and Native filters to C++ EngineBuilder (#2626)
  Register getaddrinfo in extension_registry (#2627)
  dns: stop using cares DNS resolver (#2618)

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
Reviewers

@RyanTheOptimist RyanTheOptimist RyanTheOptimist approved these changes

@jpsim jpsim Awaiting requested review from jpsim

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

Assignees

@jpsim jpsim

@alyssawilk alyssawilk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

proposal: remove support for cares
4 participants
@Augustyniak @alyssawilk @RyanTheOptimist @jpsim