Refactoring Envoy DNS resolution as extension

[yanjunxiang-google]

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 yanjunxiang@google.com

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:]

[yanjunxiang-google]

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:

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

[yanjunxiang-google]

/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.

[Shikugawa]

I think this PR also closes #17335.

[yanjunxiang-google]

/assign krajshiva

[repokitteh-read-only]

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

[yanavlasov]

/wait

[repokitteh-read-only]

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]

Ping when it's time again to take another look

/wait

[yanjunxiang-google]

I think this PR also closes #17335.

thanks!

[yanavlasov]

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]

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]

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]

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:

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

[yanjunxiang-google]

HI, @mattklein123 ,

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

PTAL.

[mattklein123]

Thanks!