-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
API changes for refactoring Envoy DNS resolution as first class extension #17272
Changes from 8 commits
716be2a
e37c0ba
25bd8da
f967a8b
68042f5
528e53a
e88529b
47e5f1e
4a9b4cc
9284f74
4eca309
71a6407
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,7 +40,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |
// <config_overview_bootstrap>` for more detail. | ||
|
||
// Bootstrap :ref:`configuration overview <config_overview_bootstrap>`. | ||
// [#next-free-field: 31] | ||
// [#next-free-field: 32] | ||
message Bootstrap { | ||
option (udpa.annotations.versioning).previous_message_type = | ||
"envoy.config.bootstrap.v2.Bootstrap"; | ||
|
@@ -260,8 +260,24 @@ message Bootstrap { | |
// This may be overridden on a per-cluster basis in cds_config, when | ||
// :ref:`dns_resolution_config <envoy_v3_api_field_config.cluster.v3.Cluster.dns_resolution_config>` | ||
// is specified. | ||
// *dns_resolution_config* will be deprecated once | ||
// :ref:'typed_dns_resolver_config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.typed_dns_resolver_config>' | ||
// is fully supported. | ||
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>' | ||
// will be enclosed in this *typed_dns_resolver_config*. | ||
// This configuration is to replace the *dns_resolution_config* configuration eventually. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/is to/will/ Express this as a TODO and/or an issue to track it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ACK |
||
// 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. | ||
htuch marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// [#not-implemented-hide:] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
core.v3.TypedExtensionConfig typed_dns_resolver_config = 31; | ||
|
||
// Specifies optional bootstrap extensions to be instantiated at startup time. | ||
// Each item contains extension specific configuration. | ||
// [#extension-category: envoy.bootstrap] | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.