Refactoring Envoy DNS resolution as extension by yanjunxiang-google · Pull Request #17479 · envoyproxy/envoy

yanjunxiang-google · 2021-07-25T20:27:21Z

Problem Description:
This PR is the code changes for refactoring Envoy DNS resolution as first class extension for issue: #14310. This is a follow up PR of #17272.

Solution:
1 Move c-ares and Apple DNS resolvers to their own extensions.
2) Create a DnsResolver factory interface, and derive two sub factory classes from it: CaresDnsResolver and AppleDnsResolver, each implement the method to create the corresponding dns_resolver object.
3) Refactor the boostrap_, cluster, dns_cache and dns_filter configuration parsing code, removing the dns_resolution_config parsing logic from them, call a standalone template function to parse the configuration, get a typed configuration. In this, 2.1) if theTypedExtensionConfig is in the config, using it to get the corresponding factory.
2.2) if TypedExtensionConfig is missing, then : if this is MacOS, and the run time flag: envoy.restart_features.use_apple_api_for_dns_lookups is enabled, then have the Envoy config factory synthesize a apple DNS resolver TypedExtensionConfig, and using it to get the apple DNS resolver factory. In all other cases, have the Envoy Synthesize a pseudo-configuration for the c-ares TypedExtensionConfig, and using it to get the cares DNS resolver factory
4) The DNS resolver factory retrieved in 3) will create a corresponding DNS resolver object (cares or apple).

Build:
passed

Testing:
passed

Release Notes:
N/A

Issues: DNS resolver as an extension point #14310

Fix #14310

Signed-off-by: Yanjun Xiang [email protected]

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

… class extension for issue: envoyproxy#14310. Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google · 2021-07-25T20:42:35Z

This PR is a work in progress. Sending it to start get comments about whether the direction wise look good.

There are pending issues need to be done:

Build in Apple system.
get "blaze test ..." working
There are temporary code change in wasm.cc, dns_filter_resolver.h, and some config validation files. Need to address them.
4)...

yanjunxiang-google · 2021-07-25T20:59:29Z

/assign @htuch @yanavlasov @jmarantz @adisuissa

This PR is still working in progress as mentioned above. However, I would like to send out this preliminary version out to get comments.
This way, in case there is direction wise issue in the approach, we can correct it earlier.

Thanks!

adisuissa

Thanks for working on this!
I've left a few comments/questions.

envoy/event/dispatcher.h

source/common/event/dispatcher_impl.cc

source/extensions/filters/udp/dns_filter/dns_filter_resolver.h

source/extensions/common/wasm/wasm.cc

source/extensions/network/dns_resolver/cares/dns_impl.h

Shikugawa · 2021-07-26T12:35:23Z

I think this PR also closes #17335.

yanjunxiang-google · 2021-07-26T13:45:43Z

/assign krajshiva

repokitteh-read-only · 2021-07-26T13:45:46Z

krajshiva cannot be assigned to this issue.

🐱

Caused by: a #17479 (comment) was created by @yanjunxiang-google.

see: more, trace.

htuch

The shape of this solution looks good, thanks! The main thing to sort out is to give each extension a unique proto type
/wait

source/common/event/dispatcher_impl.cc

source/common/upstream/cluster_factory_impl.cc

source/extensions/network/dns_resolver/apple/apple_dns_impl.h

envoy/event/dispatcher.h

source/common/event/dispatcher_impl.cc

yanavlasov · 2021-07-27T14:25:57Z

/wait

repokitteh-read-only · 2021-07-30T18:14:32Z

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

🐱

Caused by: #17479 was synchronize by yanjunxiang-google.

see: more, trace.

jmarantz · 2021-08-02T12:21:59Z

Ping when it's time again to take another look

/wait

yanjunxiang-google · 2021-08-03T19:38:15Z

I think this PR also closes #17335.

thanks!

source/extensions/extensions_build_config.bzl

source/common/event/dispatcher_impl.cc

alyssawilk · 2021-10-14T15:03:28Z

I think we do? If you hard-code them into the build as we do with TLS it works. I had to do that when I made upstreams pluggable (https://github.com/envoyproxy/envoy/pull/13548/files#) but all the things have changed since then so not convinced a reference PR will be useful.

mattklein123 · 2021-10-14T15:45:52Z

I guess when we do this:

https://github.com/envoyproxy/envoy/pull/17479/files#diff-33a9f1c9c5215b2ad3cfad885d23057b73805a1e2f5ce27fc3349e7079341016R10-R11

It doesn't work unless it's in the custom build config. I guess that's OK, and we can fix this on the EM side.

mattklein123 · 2021-10-14T15:48:04Z

@yanjunxiang-google I think we should update this also? https://github.com/envoyproxy/envoy/blob/main/ci/osx-build-config/extensions_build_config.bzl

yanavlasov · 2021-10-14T15:50:08Z

I think we do? If you hard-code them into the build as we do with TLS it works. I had to do that when I made upstreams pluggable (https://github.com/envoyproxy/envoy/pull/13548/files#) but all the things have changed since then so not convinced a reference PR will be useful.

We use the same mechanism to make DNS resolvers required as for TLS, by adding them to the _required_extensions here https://github.com/envoyproxy/envoy/pull/17479/files#diff-33a9f1c9c5215b2ad3cfad885d23057b73805a1e2f5ce27fc3349e7079341016R10-R11

so EM must be not using this mechanism.

yanavlasov · 2021-10-14T15:51:11Z

@yanjunxiang-google I think we should update this also? https://github.com/envoyproxy/envoy/blob/main/ci/osx-build-config/extensions_build_config.bzl

I think this one should work fine, since it will add everything in _required_extensions.

mattklein123 · 2021-10-14T15:55:03Z

so EM must be not using this mechanism.

It's using the same build system, so there must be something else going on? cc @keith

yanjunxiang-google · 2021-10-14T16:22:38Z

@yanjunxiang-google I think we should update this also? https://github.com/envoyproxy/envoy/blob/main/ci/osx-build-config/extensions_build_config.bzl

I think this one should work fine, since it will add everything in _required_extensions.

Ok, let me add this.

mattklein123 · 2021-10-14T16:24:25Z

Ok, let me add this.

Per @yanavlasov maybe there is something else going on? I guess we need to get to the bottom of it. Either we should add it or we need to figure out a different build issue.

Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google · 2021-10-14T18:15:30Z

Ok, let me add this.

Per @yanavlasov maybe there is something else going on? I guess we need to get to the bottom of it. Either we should add it or we need to figure out a different build issue.

okay, sounds good.

Earlier stage of this PR, I had been hard-coding c-ares and apple DNS resolver extension into Envoy binary as below. This way, they are always compiled in (based on OS), and not gated by the extension_build_config.bzl (or any other .bzl) file. This should get the EM build pass. Is this the right direction to go?

diff --git a/source/common/event/BUILD b/source/common/event/BUILD
index f847b9dd6..08a979d1e 100644
--- a/source/common/event/BUILD
+++ b/source/common/event/BUILD
@@ -44,11 +44,11 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:thread_lib",
"//source/common/filesystem:watcher_lib",
'-' "//source/common/network:dns_lib",
"//source/common/network:connection_lib",
"//source/common/network:listener_lib",
'+' "//source/extensions/network/dns_resolver/cares:config",
] + select({
'-' "//bazel:apple": ["//source/common/network:apple_dns_lib"],
'+' "//bazel:apple": ["//source/extensions/network/dns_resolver/apple:config"],
"//conditions:default": [],
}),
)

…s-extension Signed-off-by: Yanjun Xiang <[email protected]>

yanavlasov · 2021-10-14T18:36:57Z

Per @yanavlasov maybe there is something else going on? I guess we need to get to the bottom of it. Either we should add it or we need to figure out a different build issue.

I've just built linux envoy-static using ci/osx-build-config and did include cares DNS resolver. So i'm fairly confident that DNS resolvers are added by default and there is something in EM build that prevents it.

mattklein123 · 2021-10-14T18:39:47Z

I just looked through the code, and I think the problem must have to do with the extension registry h/cc files that exist in EM to force registration. I think this is missing for apple.

IMO what you have is OK, and EM can add the new registry for the apple stuff, but can we make it fail in a more obvious way? For example, here:

https://github.com/envoyproxy/envoy/pull/17479/files#diff-45b2fec2b06ec0ddec48315ea6b40a3c76edd07b46df14233ccbbd498818adc9R37-R38

Instead of checking for null when you try to do a default setup, can you actually gate this on #ifdef __APPLE__ and just release assert if the extension is not available? This should work fine for OSX CI per the above discussion, then EM will fail in an obvious way and we can fix that.

wdyt? Should be a very small change.

cc @goaway

yanjunxiang-google · 2021-10-14T21:31:31Z

HI, @mattklein123 ,

If adding a check something like below is purely to flag the extension build file issue, then is it really worthwhile?

'#ifdef APPLE'
if envoy.restart_features.use_apple_api_for_dns_lookups == true &&
Config::Utility::getAndCheckFactoryByNameNetwork::DnsResolverFactory(std::string(AppleDnsResolver), true) == nullptr
RELEASE_ASSERT()

#endif

We have other means to easily spot this extension build file issue, e.g., check the bootstrapping logs:

[2021-10-14 18:06:35.672][1889399][info][main] [source/server/server.cc:371] statically linked extensions:
...
[2021-10-14 18:06:35.672][1889399][info][main] [source/server/server.cc:373] envoy.network.dns_resolver: envoy.network.dns_resolver.cares <<------ Is this dns_resolver extension (apple for macOS) show up in the log? If it's missing, then it's a problem.

and:
"Didn't find a registered implementation for name: 'envoy.network.dns_resolver.cares'"
<<-------This is very good indication of the extensions are not included correctly.

ccing @htuch for his opinion on adding back the "#ifdef APPLE".

thanks,
Yanjun

mattklein123 · 2021-10-14T21:50:49Z

I don't want to actually check the logs, because it's going to fall back to c-ares in a non-obvious way.

All I'm saying to do is in the checkApple function do:

If not #ifdef apple just do nothing/return.
Otherwise pull the apple config and let it fail/crash if not there to indicate a build issue.

…er extension is not included Envoy MacOS build file Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google · 2021-10-14T23:17:09Z

HI, @mattklein123 ,

Thanks for the comments. I modified checkUseAppleApiForDnsLookups() function to add the #ifdef APPLE, RELEASE_ASSERT() logic you mentioned.

PTAL.

Signed-off-by: Yanjun Xiang <[email protected]>

mattklein123

Thanks!

This is the code change for refactoring Envoy DNS resolution as first…

7046a2c

… class extension for issue: envoyproxy#14310. Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google requested review from PiotrSikora, alyssawilk, lizan, mathetake and mattklein123 as code owners July 25, 2021 20:27

repokitteh-read-only bot assigned adisuissa, htuch, jmarantz and yanavlasov Jul 25, 2021

adisuissa reviewed Jul 26, 2021

View reviewed changes

yanjunxiang-google changed the title ~~This is the code change for refactoring Envoy DNS resolution as first…~~ Refactoring Envoy DNS resolution as extension Jul 26, 2021

htuch suggested changes Jul 27, 2021

View reviewed changes

source/common/event/dispatcher_impl.cc Outdated Show resolved Hide resolved

source/common/upstream/cluster_factory_impl.cc Outdated Show resolved Hide resolved

source/extensions/network/dns_resolver/apple/apple_dns_impl.h Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 27, 2021

yanavlasov requested changes Jul 27, 2021

View reviewed changes

envoy/event/dispatcher.h Outdated Show resolved Hide resolved

source/common/event/dispatcher_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added api and removed waiting labels Jul 30, 2021

repokitteh-read-only bot added waiting and removed waiting labels Aug 2, 2021

yanavlasov requested changes Aug 5, 2021

View reviewed changes

source/extensions/extensions_build_config.bzl Outdated Show resolved Hide resolved

yanavlasov requested changes Aug 5, 2021

View reviewed changes

source/common/event/dispatcher_impl.cc Outdated Show resolved Hide resolved

dns-as-extension

b4470d1

Signed-off-by: Yanjun Xiang <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dns-a…

8fbc51a

…s-extension Signed-off-by: Yanjun Xiang <[email protected]>

Adding #ifdef __APPLE__ to flag build file issue if apple DNS resolv…

d7a9faa

…er extension is not included Envoy MacOS build file Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google added 2 commits October 15, 2021 03:43

fixing macOS integration test failure

b49db1b

Signed-off-by: Yanjun Xiang <[email protected]>

bump up dns_resolver test coverage number

8757f5f

Signed-off-by: Yanjun Xiang <[email protected]>

yanavlasov approved these changes Oct 15, 2021

View reviewed changes

jpsim mentioned this pull request Oct 15, 2021

envoy: bump upstream to c687308 envoyproxy/envoy-mobile#1886

Merged

mattklein123 approved these changes Oct 15, 2021

View reviewed changes

jpsim mentioned this pull request Oct 15, 2021

[dns] Declare AppleDnsResolverFactory #18646

Merged

lkpdn mentioned this pull request Oct 16, 2021

quic: refactor QUICHE flag implementation #18485

Closed

This was referenced Oct 16, 2021

Put dns_resolution_config field in dns_filter.proto as deprecated #18649

Merged

move DnsResolverFactory definition to envoy/network #18651

Merged

This was referenced Oct 19, 2021

Update Envoy to 592868a (Oct 18, 2021) envoyproxy/nighthawk#753

Closed

Update Envoy to 5d4a457 (Oct 19, 2021) envoyproxy/nighthawk#754

Merged

yanjunxiang-google mentioned this pull request Oct 21, 2021

Cleanup ares_library_init from Envoy ProcessWide::ProcessWide() #18713

Closed

botengyao mentioned this pull request Dec 11, 2024

dns_filter: change dns filter to stable #37618

Merged

Conversation

yanjunxiang-google commented Jul 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Jul 25, 2021

Uh oh!

yanjunxiang-google commented Jul 25, 2021

Uh oh!

adisuissa 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!

Shikugawa commented Jul 26, 2021

Uh oh!

yanjunxiang-google commented Jul 26, 2021

Uh oh!

repokitteh-read-only bot commented Jul 26, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yanavlasov commented Jul 27, 2021

Uh oh!

repokitteh-read-only bot commented Jul 30, 2021

Uh oh!

jmarantz commented Aug 2, 2021

Uh oh!

yanjunxiang-google commented Aug 3, 2021

Uh oh!

Uh oh!

Uh oh!

alyssawilk commented Oct 14, 2021

Uh oh!

mattklein123 commented Oct 14, 2021

Uh oh!

mattklein123 commented Oct 14, 2021

Uh oh!

yanavlasov commented Oct 14, 2021

Uh oh!

yanavlasov commented Oct 14, 2021

Uh oh!

mattklein123 commented Oct 14, 2021

Uh oh!

yanjunxiang-google commented Oct 14, 2021

Uh oh!

mattklein123 commented Oct 14, 2021

Uh oh!

yanjunxiang-google commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanavlasov commented Oct 14, 2021

Uh oh!

mattklein123 commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanjunxiang-google commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

yanjunxiang-google commented Jul 25, 2021 •

edited

Loading

yanjunxiang-google commented Oct 14, 2021 •

edited

Loading

mattklein123 commented Oct 14, 2021 •

edited

Loading

yanjunxiang-google commented Oct 14, 2021 •

edited

Loading

mattklein123 commented Oct 14, 2021 •

edited

Loading

yanjunxiang-google commented Oct 14, 2021 •

edited

Loading