API changes for refactoring Envoy DNS resolution as first class extension #17272

yanjunxiang-google · 2021-07-08T20:03:51Z

Problem description:
This is the API changes for refactoring Envoy DNS resolution as first class extension for issue: #14310.

Solution:
This PR added a new DNS resolver type configuration extension in a couple of proto files.
This extension can be used to configure c-ares, apple, or any other DNS resolver types and the related parameters.
When c-ares DNS is configured, :ref:'dns_resolution_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.dns_resolution_config>' will be enclosed in this typed_dns_resolver_config. This configuration is to replace the dns_resolution_config configuration eventually.
During the transition period when both dns_resolution_config and typed_dns_resolver_config exists, this configuration is optional. When typed_dns_resolver_config is in place, Envoy will use it and ignore dns_resolution_config. when typed_dns_resolver_config is missing, the default behavior is in place. Envoy will either use c-ares DNS library or apple DNS library based on the compiling flag.

This is the first step towards the fixing of the issue #14310: DNS resolver as an extension point. The follow up work is planned.

Testing:
The code change passed bazel test.

Release Notes:
N/A

Issues: DNS resolver as an extension point #14310

Fix #14310

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

…NS resolver in API definition. This configuration is to support DNS resolution as a first class Envoy extension. This API change is the first step for that task. This configuration is to replace above dns_resolution_config. By default envoy supports c-ares DNS or apple DNS resolvers. This extension can be used to configure other DNS resolver types. This configuration is optional. In case it is missing, the default behavior is in place, which means Envoy will either use c-ares DNS library or apple DNS library based on the compiling flag. When this configuration is in place, Envoy will use the configured DNS resolver carried in the extension typed_config field. In case both typed_dns_resolver_config and dns_resolution_config are configured, typed_dns_resolver_config take precedence. Testing: The code change passed bazel test. Release Notes: N/A Issues: envoyproxy#14310 Fix : N/A Signed-off-by: Yanjun Xiang <[email protected]>

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

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

repokitteh-read-only · 2021-07-08T20:03:58Z

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

🐱

Caused by: #17272 was opened by yanjunxiang-google.

see: more, trace.

yanjunxiang-google · 2021-07-08T20:05:44Z

/assign @htuch @yanavlasov @jmarantz @adisuissa

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

htuch

LGTM, makes sense to me.
/wait

api/envoy/config/bootstrap/v3/bootstrap.proto

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

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

yanjunxiang-google · 2021-07-10T02:18:44Z

/retest

repokitteh-read-only · 2021-07-10T02:18:48Z

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

🐱

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

see: more, trace.

yanjunxiang-google · 2021-07-10T12:21:32Z

@envoyproxy/api-shepherds PTAL

adisuissa · 2021-07-11T18:53:10Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+  // During the transition period when both *dns_resolution_config* and *typed_dns_resolver_config* exists,
+  // this configuration is optional.
+  // When *typed_dns_resolver_config* is in place, Envoy will use it and ignore *dns_resolution_config*.
+  // when *typed_dns_resolver_config* is missing, the default behavior is in place. Envoy will either use


nit: s/when/When here and in the other places.
BTW: do you mean the default behavior will be in effect when both typed_dns_resolver_config and dns_resolution_config are missing?

ACK. That's right.

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

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

jmarantz

@htuch I may not be understanding how this is to be configured.

Is the idea that there will be ifdef-controlled selection of apple DNS vs C-Ares, and then on to top of that there would be a configuration-controlled DNS option also?

I had this really naive notion that we could plumb in a dns resolver library in a fashion parallel to the way we plumb in a ThreadFactory, as a pure interface passed into MainCommon. I guess the limitation of that is that there it captures only platform-based differences in resolver type, and would not allow different resolvers to be used per cluster or per-listener, and so the proto-based config allows for that flexibility.

But I don't have as clear a picture as I would like, looking at this proto change, of how this all fits together.

Should we have a doc describing the bigger picture of how this all works?

One thing, for example, I'd like to be able to do is depending on platform, not have C-Ares linked in at all into the codebase. Is that possible with the strategy you are driving?

jmarantz · 2021-07-12T01:33:48Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+  // or any other DNS resolver types and the related parameters. When c-ares DNS is configured,
+  // :ref:'dns_resolution_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.dns_resolution_config>'
+  // will be enclosed in this *typed_dns_resolver_config*.
+  // This configuration is to replace the *dns_resolution_config* configuration eventually.


s/is to/will/

Express this as a TODO and/or an issue to track it?

jmarantz · 2021-07-12T01:36:19Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+  // When *typed_dns_resolver_config* is in place, Envoy will use it and ignore *dns_resolution_config*.
+  // When *typed_dns_resolver_config* is missing, the default behavior is in place. Envoy will either use
+  // c-ares DNS library or apple DNS library based on the compiling flag.
+  // [#not-implemented-hide:]


can we also track the addition of a null DNS configuration mode, where no DNS library is compiled in, and DNS resolutions all fail gracefully?

Yes, we can support the NULL DNS configuration mode if the new typed_dns_resolver_config configuration is in place, and check it and have DNS resolutions fail gracefully.

However, in the case if this configuration is missing, IIUC there is no information for us to identify NULL DNS configuration mode, then, the existing logic (#ifdef) will be executed.

BTW, to make this change backward compatible, this new typed configuration is an optional field for now.

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

yanjunxiang-google · 2021-07-13T01:17:33Z

/retest

repokitteh-read-only · 2021-07-13T01:17:37Z

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

🐱

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

see: more, trace.

htuch · 2021-07-13T03:14:26Z

@jmarantz why should DNS be any different than any other extension point? I was envisaging just using static linking/registration, the same way we do with network/HTTP filters, cluster types, transport sockets etc. This is likely flexible enough for everything we need and matches Envoys extensibility model. You can, at build time, decide to use c-ares or Apple DNS resolvers and link or don't link.

We should eliminate all #ifdef as part of this refactor, that would be a litmus test of a successful refactor.

htuch

/lgtm api

yanavlasov

/wait

yanavlasov · 2021-07-13T19:41:18Z

api/envoy/config/bootstrap/v3/bootstrap.proto

  core.v3.DnsResolutionConfig dns_resolution_config = 30;

+  // DNS resolver type configuration extension. This extension can be used to configure c-ares, apple,
+  // or any other DNS resolver types and the related parameters. When c-ares DNS is configured,
+  // :ref:'dns_resolution_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.dns_resolution_config>'


This should be <envoy_api_field_core.DnsResultionConfig> instead of <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.dns_resolution_config>

The same issue is in the other proto files.

Yeah, that's right. Thanks!

I did some research and found the complete syntax is: :ref:DnsResolutionConfig <envoy_v3_api_msg_config.core.v3.DnsResolutionConfig> and made the change.

Please let me know whether this look good.

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

…sion (envoyproxy#17272) Adding a core.v3.TypedExtensionConfig typed_dns_resolver_config for DNS resolver in API definition. This configuration is to support DNS resolution as a first class Envoy extension. This API change is the first step for that task. This configuration is to replace above dns_resolution_config. By default envoy supports c-ares DNS or apple DNS resolvers. This extension can be used to configure other DNS resolver types. This configuration is optional. In case it is missing, the default behavior is in place, which means Envoy will either use c-ares DNS library or apple DNS library based on the compiling flag. When this configuration is in place, Envoy will use the configured DNS resolver carried in the extension typed_config field. In case both typed_dns_resolver_config and dns_resolution_config are configured, typed_dns_resolver_config take precedence. Signed-off-by: Yanjun Xiang <[email protected]>

yanjunxiang-google added 3 commits July 8, 2021 16:04

API changes for support Envoy DNS resolution as first class extension.

e37c0ba

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

API changes for refactoring Envoy DNS resolution as first class exten…

25bd8da

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

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

repokitteh-read-only bot assigned lizan Jul 8, 2021

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

Merge branch 'main' of https://github.com/envoyproxy/envoy into dns_r…

f967a8b

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

htuch suggested changes Jul 9, 2021

View reviewed changes

api/envoy/config/bootstrap/v3/bootstrap.proto Outdated Show resolved Hide resolved

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

Put the proto fields as #not-implemented-hide: before the actual chan…

68042f5

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

repokitteh-read-only bot removed the waiting label Jul 9, 2021

Commit the v4alpha version and generated proto files

528e53a

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

adisuissa reviewed Jul 11, 2021

View reviewed changes

yanjunxiang-google added 2 commits July 12, 2021 01:21

address comments

e88529b

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

Merge branch 'main' of https://github.com/envoyproxy/envoy into dns_r…

47e5f1e

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

jmarantz reviewed Jul 12, 2021

View reviewed changes

yanjunxiang-google added 2 commits July 12, 2021 15:25

address comments

4a9b4cc

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

address comments

9284f74

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

htuch reviewed Jul 13, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 13, 2021

htuch previously approved these changes Jul 13, 2021

View reviewed changes

yanavlasov reviewed Jul 13, 2021

View reviewed changes

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

addressing comments

4eca309

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

yanjunxiang-google dismissed htuch’s stale review via 4eca309 July 14, 2021 22:57

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

address comments

71a6407

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

lizan approved these changes Jul 14, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 14, 2021

mattklein123 merged commit b65b397 into envoyproxy:main Jul 15, 2021

htuch mentioned this pull request Jul 19, 2021

DNS resolver as an extension point #14310

Closed

yanjunxiang-google deleted the dns_refactorying branch July 19, 2021 21:15

yanjunxiang-google mentioned this pull request Jul 25, 2021

Refactoring Envoy DNS resolution as extension #17479

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API changes for refactoring Envoy DNS resolution as first class extension #17272

API changes for refactoring Envoy DNS resolution as first class extension #17272

yanjunxiang-google commented Jul 8, 2021 •

edited

Loading

repokitteh-read-only bot commented Jul 8, 2021

yanjunxiang-google commented Jul 8, 2021

htuch left a comment

yanjunxiang-google commented Jul 10, 2021

repokitteh-read-only bot commented Jul 10, 2021

yanjunxiang-google commented Jul 10, 2021

adisuissa Jul 11, 2021

yanjunxiang-google Jul 12, 2021 •

edited

Loading

jmarantz left a comment

jmarantz Jul 12, 2021

yanjunxiang-google Jul 12, 2021

jmarantz Jul 12, 2021

yanjunxiang-google Jul 12, 2021 •

edited

Loading

yanjunxiang-google commented Jul 13, 2021

repokitteh-read-only bot commented Jul 13, 2021

htuch commented Jul 13, 2021

htuch left a comment

yanavlasov left a comment

yanavlasov Jul 13, 2021

yanjunxiang-google Jul 14, 2021

API changes for refactoring Envoy DNS resolution as first class extension #17272

API changes for refactoring Envoy DNS resolution as first class extension #17272

Conversation

yanjunxiang-google commented Jul 8, 2021 • edited Loading

repokitteh-read-only bot commented Jul 8, 2021

yanjunxiang-google commented Jul 8, 2021

htuch left a comment

Choose a reason for hiding this comment

yanjunxiang-google commented Jul 10, 2021

repokitteh-read-only bot commented Jul 10, 2021

yanjunxiang-google commented Jul 10, 2021

adisuissa Jul 11, 2021

Choose a reason for hiding this comment

yanjunxiang-google Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz Jul 12, 2021

Choose a reason for hiding this comment

yanjunxiang-google Jul 12, 2021

Choose a reason for hiding this comment

jmarantz Jul 12, 2021

Choose a reason for hiding this comment

yanjunxiang-google Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

yanjunxiang-google commented Jul 13, 2021

repokitteh-read-only bot commented Jul 13, 2021

htuch commented Jul 13, 2021

htuch left a comment

Choose a reason for hiding this comment

yanavlasov left a comment

Choose a reason for hiding this comment

yanavlasov Jul 13, 2021

Choose a reason for hiding this comment

yanjunxiang-google Jul 14, 2021

Choose a reason for hiding this comment

yanjunxiang-google commented Jul 8, 2021 •

edited

Loading

yanjunxiang-google Jul 12, 2021 •

edited

Loading

yanjunxiang-google Jul 12, 2021 •

edited

Loading