dns: add a getaddrinfo based system resolver by mattklein123 · Pull Request #22080 · envoyproxy/envoy

mattklein123 · 2022-07-08T20:21:28Z

This can be used when using the system resolver is desired. For
example, on Android.

Risk Level: None, new extension
Testing: New tests
Docs Changes: Added
Release Notes: Added
Platform Specific Features: TBD

repokitteh-read-only · 2022-07-08T20:21:31Z

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: #22080 was opened by mattklein123.

see: more, trace.

repokitteh-read-only · 2022-07-08T20:21:36Z

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

🐱

Caused by: #22080 was opened by mattklein123.

see: more, trace.

mattklein123 · 2022-07-08T20:21:44Z

Not quite finished but I want to see what problems CI gives me.

mattklein123 · 2022-07-08T20:22:30Z

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

This is copied from the Android OS implementation.

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

This can be used when using the system resolver is desired. For example, on Android. Signed-off-by: Matt Klein <mklein@lyft.com>

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

mattklein123 · 2022-07-11T22:23:09Z

OK I think this is ready.

alyssawilk

Looks like coverage isn't happy but here's some first ass comments while you sort that :-)
/wait

source/extensions/extensions_build_config.bzl

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

test/extensions/network/dns_resolver/getaddrinfo/getaddrinfo_test.cc

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

mattklein123 · 2022-07-12T14:45:07Z

Unfortunately the coverage tool is missing braces so there isn't much I can do about this. I will drop the limit:
https://storage.googleapis.com/envoy-pr/17b2cb1/coverage/source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc.gcov.html

adisuissa

/lgtm api

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

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

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

RyanTheOptimist

Other than a few nits, this looks great (not surprisingly).

source/common/api/posix/os_sys_calls_impl.cc

source/common/api/win32/os_sys_calls_impl.cc

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

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

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc

mattklein123 · 2022-07-13T02:04:24Z

/retest

repokitteh-read-only · 2022-07-13T02:04:28Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #22080 (comment) was created by @mattklein123.

see: more, trace.

mattklein123 · 2022-07-13T02:15:23Z

/retest

repokitteh-read-only · 2022-07-13T02:15:30Z

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

🐱

Caused by: a #22080 (comment) was created by @mattklein123.

see: more, trace.

soulxu

LGTM, thanks

api/envoy/extensions/network/dns_resolver/getaddrinfo/v3/getaddrinfo_dns_resolver.proto

alyssawilk

Just the one testing nit and you're good to go. Thanks!

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc

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

For non-Apple platforms like Linux and Android. Updated the Kotlin, Java & C++ builders. Added here: envoyproxy/envoy#22080 Signed-off-by: JP Simard <jp@jpsim.com>

For non-Apple platforms like Linux and Android. Updated the Kotlin, Java & C++ builders. Added here: envoyproxy/envoy#22080 Risk Level: Low to moderate, off by default but I had to refactor some stuff. Testing: Added. Docs Changes: Documentation comments. Release Notes: Added. Signed-off-by: JP Simard <jp@jpsim.com>

For non-Apple platforms like Linux and Android. Updated the Kotlin, Java & C++ builders. Added here: #22080 Risk Level: Low to moderate, off by default but I had to refactor some stuff. Testing: Added. Docs Changes: Documentation comments. Release Notes: Added. Signed-off-by: JP Simard <jp@jpsim.com>

repokitteh-read-only bot added the api label Jul 8, 2022

repokitteh-read-only bot assigned adisuissa Jul 8, 2022

mattklein123 commented Jul 8, 2022

View reviewed changes

RyanTheOptimist reviewed Jul 8, 2022

View reviewed changes

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc Outdated Show resolved Hide resolved

mattklein123 force-pushed the getaddrinfo-resolver branch 3 times, most recently from 1450040 to e093217 Compare July 8, 2022 22:31

dns: add a getaddrinfo based system resolver

69eb66d

This can be used when using the system resolver is desired. For example, on Android. Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 force-pushed the getaddrinfo-resolver branch from e093217 to 69eb66d Compare July 8, 2022 22:44

mattklein123 added 8 commits July 11, 2022 02:41

Merge remote-tracking branch 'origin/main' into getaddrinfo-resolver

c9fbb0f

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

fix test

959a912

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

windows?

43be1a8

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

Merge remote-tracking branch 'origin/main' into getaddrinfo-resolver

3d9c74d

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

fix

7d80cd2

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

fix

370555a

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

Merge remote-tracking branch 'origin/main' into getaddrinfo-resolver

830b980

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

fix

9c10771

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

mattklein123 marked this pull request as ready for review July 11, 2022 22:22

mattklein123 assigned Augustyniak, alyssawilk and RyanTheOptimist Jul 11, 2022

alyssawilk reviewed Jul 12, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 12, 2022

soulxu reviewed Jul 12, 2022

View reviewed changes

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc Show resolved Hide resolved

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc Show resolved Hide resolved

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc Outdated Show resolved Hide resolved

adisuissa reviewed Jul 12, 2022

View reviewed changes

mattklein123 added 2 commits July 12, 2022 16:28

fix

2d299b1

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

add integration test

d04ef91

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

Augustyniak reviewed Jul 12, 2022

View reviewed changes

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc Show resolved Hide resolved

fix

0d44c72

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

RyanTheOptimist reviewed Jul 12, 2022

View reviewed changes

mattklein123 added 2 commits July 12, 2022 18:54

Merge remote-tracking branch 'origin/main' into getaddrinfo-resolver

dadfc06

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

comments

e8464ec

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

soulxu suggested changes Jul 13, 2022

View reviewed changes

source/extensions/network/dns_resolver/getaddrinfo/getaddrinfo.cc Outdated Show resolved Hide resolved

soulxu previously approved these changes Jul 13, 2022

View reviewed changes

ramaraochavali reviewed Jul 13, 2022

View reviewed changes

api/envoy/extensions/network/dns_resolver/getaddrinfo/v3/getaddrinfo_dns_resolver.proto Outdated Show resolved Hide resolved

alyssawilk reviewed Jul 13, 2022

View reviewed changes

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc Outdated Show resolved Hide resolved

test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc Show resolved Hide resolved

mattklein123 added 2 commits July 13, 2022 14:43

Merge remote-tracking branch 'origin/main' into getaddrinfo-resolver

4f7405f

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

comments

0db567e

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

mattklein123 dismissed soulxu’s stale review via 0db567e July 13, 2022 15:00

alyssawilk approved these changes Jul 13, 2022

View reviewed changes

mattklein123 merged commit 516b3f3 into main Jul 14, 2022

mattklein123 deleted the getaddrinfo-resolver branch July 14, 2022 04:07

jpsim mentioned this pull request Jul 14, 2022

dns: allow using the getaddrinfo-based system DNS resolver envoyproxy/envoy-mobile#2419

Merged

Augustyniak mentioned this pull request Sep 8, 2022

proposal: remove support for cares envoyproxy/envoy-mobile#2534

Closed

1 task

Conversation

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

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jul 8, 2022

Uh oh!

repokitteh-read-only bot commented Jul 8, 2022

Uh oh!

mattklein123 commented Jul 8, 2022

Uh oh!

mattklein123 Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 commented Jul 11, 2022

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Jul 12, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Jul 13, 2022

Uh oh!

repokitteh-read-only bot commented Jul 13, 2022

Uh oh!

mattklein123 commented Jul 13, 2022

Uh oh!

repokitteh-read-only bot commented Jul 13, 2022

Uh oh!

soulxu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mattklein123 commented Jul 8, 2022 •

edited

Loading