dns: disable the use of search namespaces when using the c-ares resolver by suniltheta · Pull Request #16237 · envoyproxy/envoy

suniltheta · 2021-04-29T17:46:59Z

Signed-off-by: Sunil Narasimhamurthy sunnrs@amazon.com

Commit Message: Modify API to support configuration of ARES flags to disable the use of search namespaces when using the c-ares resolver and reconcile DNS options in a single message
Additional Description:
Risk Level: low
Testing: unit test
Docs Changes: yes
Release Notes: updated new feature and deprecated list
Platform Specific Features:
Issues: #12432
Deprecated: field 'use_tcp_for_dns_lookups' in bootstrap, cluster & dynamic_forward_proxy xDS APIs

…of search namespaces when using the c-ares resolver and reconcile DNS options in a single message Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only · 2021-04-29T17:47:06Z

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: #16237 was opened by suniltheta.

see: more, trace.

suniltheta · 2021-04-29T17:53:08Z

Hi, this is a draft PR to address the issue #12432 .
I want to get the opinion and suggestions about the DnsLookupOptions fields. Can I get the tips on how I can propagate this message throughout the code where the DNS resolvers are instantiated.

Also need consensus on this change where use_tcp_for_dns_lookups option will be deprecated and will be moved inside new protobuf message DnsLookupOptions.

~~Note: I have not included the files from running the tools/proto_format/proto_format.sh fix, to keep the change set small. So CI might be failing.~~

Also @rshriram please let me know if you have any suggestion on this PR.

Thank you.

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

htuch · 2021-04-30T02:08:43Z

api/envoy/config/core/v3/base.proto

 }
+
+// Configuration of DNS Lookup option flags which control the behavior of the DNS resolver.
+message DnsLookupOptions {


FWIW I think we are heading towards a model of pluggable DNS resolvers in the not too distant future. I wonder if the DNS lookup options should reflect that?

Yes, good idea. I never thought of that. Thank you for your suggestion. I will keep this in mind and take care of it in upcoming commits.

adisuissa

Thanks for working on this. I've added a few comments.

adisuissa · 2021-04-30T03:21:38Z

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto

  // ``envoy.restart_features.use_apple_api_for_dns_lookups`` runtime value is true during
  // server startup. Apple' API only uses UDP for DNS resolution.
-  bool use_tcp_for_dns_lookups = 8;
+  bool use_tcp_for_dns_lookups = 8


Can you add to the comment a reference to the configuration that should be used (instead of the deprecated one)?

Added the comment.

adisuissa · 2021-04-30T03:25:28Z

api/envoy/config/core/v3/base.proto

+  google.protobuf.BoolValue use_tcp_for_dns_lookups = 1;
+
+  // Do not use the default search domains; only query hostnames as-is or as aliases.
+  google.protobuf.BoolValue no_defalt_search_domain = 2;


Suggested change

google.protobuf.BoolValue no_defalt_search_domain = 2;

google.protobuf.BoolValue no_default_search_domain = 2;

Took care of this manually. thanks for noticing.

adisuissa · 2021-04-30T03:31:42Z

api/envoy/config/core/v3/base.proto

+  // specified.
+  // Setting this value causes failure if the
+  // ``envoy.restart_features.use_apple_api_for_dns_lookups`` runtime value is true during
+  // server startup. Apple' API only uses UDP for DNS resolution.


Suggested change

// server startup. Apple' API only uses UDP for DNS resolution.

// server startup. Apple's API only uses UDP for DNS resolution.

Took care of this manually. thanks for noticing.

…dressed few review comments Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

adisuissa

Looks good.
Please fix link issues, as it blocks CI.

adisuissa · 2021-05-04T01:44:21Z

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

  // server startup. Apple' API only uses UDP for DNS resolution.
-  bool use_tcp_for_dns_lookups = 20;
+  // This field is deprecated. Instead use
+  // :ref:`dns_lookup_options <envoy_api_field_config.config.bootstrap.v3.Bootstrap.dns_lookup_options>`


I think there's a wrong link here. Should probably be:
envoy_api_field_config.bootstrap.v3.Bootstrap.dns_lookup_options.
Please fix here and in other places.

Thanks for taking a look. I am working on these changes on the side. I will try to push rest of the changes by tomorrow. I will fix this :ref: link as well.

adisuissa · 2021-05-04T01:51:46Z

api/envoy/config/core/v3/base.proto

+  // server startup. Apple's API only uses UDP for DNS resolution.
+  google.protobuf.BoolValue use_tcp_for_dns_lookups = 1;
+
+  // Do not use the default search domains; only query hostnames as-is or as aliases.


Also I think this should be tagged with [#not-implemented-hide:], because IIUC it is not implemented yet.

All the changes are committed.

adisuissa · 2021-05-04T01:54:17Z

api/envoy/config/core/v3/base.proto

+  // specified.
+  // Setting this value causes failure if the
+  // ``envoy.restart_features.use_apple_api_for_dns_lookups`` runtime value is true during
+  // server startup. Apple's API only uses UDP for DNS resolution.


@htuch this new message replaces a deprecated field (use_tcp_for_dns_lookups). However, it also modifies the field type from bool to google.protobuf.BoolValue.
I'm assuming it is ok, as the other field is deprecated, just want your opinion on this.

I think this is fine. What is the default value for this field? BoolValue is mostly useful when we anticipate wanting to change the default (in particular for security related fields).

The default value is protobuf's bool default value (false).
Actually, the name of the field use_tcp_for_dns_lookups implies it is meant to be "enabled". I'm not certain what's the added value of BoolValue in this case.

So here is my approach to keep supporting the deprecated field "bool use_tcp_for_dns_lookups". I would love to get your opinions on them.

Even though the field "bool use_tcp_for_dns_lookups" is marked deprecated, we expect previous implementations of xDSv3 control plane to still set this value. So the logic will check if the new field "google.protobuf.BoolValue use_tcp_for_dns_lookups" is being set via dns_lookup_options or not. The code will rely on has_use_tcp_for_dns_lookups() to check if this value is being programmed by the server. If this dns_lookup_options.has_use_tcp_for_dns_lookups() is false (which means value is not set) then the code will check if the deprecated field bool use_tcp_for_dns_lookups has the value true and uses it. This makes sure of the backward compatibility of the changes.

Correct me if I am wrong here, by just using bool field type we will not be able to know if the config (true or false) is applied or not.

One might ask can we just do OR of bool use_tcp_for_dns_lookups and dns_lookup_options.use_tcp_for_dns_lookups().value(), like -> config.dns_lookup_options.use_tcp_for_dns_lookups().value() || config.use_tcp_for_dns_lookups. But as per the logic which is being worked upon now, the new field
google.protobuf.BoolValue dns_lookup_options.use_tcp_for_dns_lookups will get preference and override any value set by bare bool use_tcp_for_dns_lookups value.

I think you can use the presence of the dns_lookup_options message alone to detect that we aren't dealing with legacy configuration..

Thanks for the suggestions. Not using BoolValue.

…of search namespaces when using the c-ares resolver and reconcile DNS options in a single message. Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

adisuissa

API looks good.
Left some drive-by comments on the non-api code.

adisuissa · 2021-05-07T13:24:39Z

include/envoy/event/dispatcher.h

   * @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups.
   * Otherwise, udp is used.


Should be removed.

Removed. thanks for noticing

adisuissa · 2021-05-07T13:29:07Z

source/common/event/dispatcher_impl.cc

                   "using TCP for DNS lookups is not possible when using Apple APIs for DNS "
                   "resolution. Apple' API only uses UDP for DNS resolution. Use UDP or disable "
                   "the envoy.restart_features.use_apple_api_for_dns_lookups runtime feature.");
    return std::make_shared<Network::AppleDnsResolverImpl>(*this, api_.randomGenerator(),


Note that in this case the new no_default_search_domain flag won't be used to initialize the AppleDnsResolver.

I need some clarity here. Originally AppleDnsResolver has problem using TCP for DNS resolution. Now as we introduce no_default_search_domain as DNS lookup/resolver options, nowhere in this PR we are making this options available for AppleDnsResolver.

Can you please suggest what we can do here?

Should we find a way to pass this no_default_search_domain into the DNS resolver library that AppleDnsResolver uses, or

Highlight that setting any options via dns_resolver_options is not allowed for AppleDnsResolver? Before this PR there was already a RELEASE_ASSERT which make sure the user is not setting use_tcp_for_dns_lookups . Should that be the norm for any options set via dns_resolver_options going forward ?

Please let me know. Thanks in advance.

adisuissa · 2021-05-07T13:44:36Z

source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

      max_hosts_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, max_hosts, 1024)) {
+  envoy::config::core::v3::DnsResolverOptions dns_resolver_options;
+  if (config.has_dns_resolver_options()) {
+    const auto& config_dns_options = config.dns_resolver_options();


Same as above.
Also not sure if the two pieces of code can be refactored, as they are essentially the same.

Took care of this change. Need your thoughts here. In the updated changes the code looks like

envoy::config::core::v3::DnsResolverOptions dns_resolver_options; if (cluster.has_dns_resolver_options()) { dns_resolver_options.CopyFrom(cluster.dns_resolver_options()); } else { // <code comment> dns_resolver_options.set_use_tcp_for_dns_lookups(cluster.use_tcp_for_dns_lookups()); }

technically we don't need both the if & else condition. We could just do with

envoy::config::core::v3::DnsResolverOptions dns_resolver_options; dns_resolver_options.CopyFrom(cluster.dns_resolver_options()); if (!cluster.has_dns_resolver_options()) { dns_resolver_options.set_use_tcp_for_dns_lookups(cluster.use_tcp_for_dns_lookups()); }

But I thought it is better if we make it explicit that if legacy control plane has not configured dns_resolver_options then we stick to just setting dns_resolver_options.use_tcp... based on deprecated field use_tcp_for_dns_lookups.

adisuissa · 2021-05-07T13:47:15Z

source/server/server.cc

-  const bool use_tcp_for_dns_lookups = bootstrap_.use_tcp_for_dns_lookups();
-  dns_resolver_ = dispatcher_->createDnsResolver({}, use_tcp_for_dns_lookups);
+  envoy::config::core::v3::DnsResolverOptions dns_resolver_options;
+  if (bootstrap_.has_dns_resolver_options()) {


Same as above.
I think this can be refactored to avoid code dup.

adisuissa · 2021-05-07T13:48:08Z

source/common/upstream/cluster_factory_impl.cc

+      const auto& cluster_dns_options = cluster.dns_resolver_options();
+      dns_resolver_options.set_use_tcp_for_dns_lookups(
+          cluster_dns_options.use_tcp_for_dns_lookups());
+      dns_resolver_options.set_no_default_search_domain(
+          cluster_dns_options.no_default_search_domain());


Can you use something like CopyFrom here?
Otherwise, more fields will require manual copying.

done. thanks.

…ssue in a Test Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

suniltheta · 2021-05-08T00:27:38Z

Waiting on #16294 to be merged before merging this.

suniltheta · 2021-05-08T04:34:39Z

/retest

repokitteh-read-only · 2021-05-08T04:34:43Z

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

🐱

Caused by: a #16237 (comment) was created by @suniltheta.

see: more, trace.

suniltheta · 2021-05-28T15:40:55Z

The job running on agent Azure Pipelines 17 ran longer than the maximum time of 180 minutes.

So restarting the tests.

suniltheta · 2021-05-28T15:41:04Z

/retest

repokitteh-read-only · 2021-05-28T15:41:08Z

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

🐱

Caused by: a #16237 (comment) was created by @suniltheta.

see: more, trace.

suniltheta · 2021-05-28T18:10:01Z

Hi @htuch & @adisuissa, this PR is ready for review.
The CI is failing with the following reason below. I thought let me just get the review comments and address those comments so that subsequent commit can trigger a new CI run. Thank you for your time.

##[error]The agent did not connect within the alloted time of 45 minute(s).
,##[error]The request: 395683 was abandoned due to an infrastructure failure. Notification of assignment to an agent was never received.

htuch · 2021-05-28T21:53:30Z

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

+  // 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.
+  core.v3.DnsResolutionConfig dns_resolution_config = 30;


@yanjunxiang-google this is the proto message that DNS resolver extension should live in.

htuch

/lgtm api

htuch · 2021-05-28T21:54:34Z

api/envoy/config/core/v3/resolver.proto

+  bool use_tcp_for_dns_lookups = 1;
+
+  // Do not use the default search domains; only query hostnames as-is or as aliases.
+  bool no_default_search_domain = 2;


I think you should add a version history entry for this change.

Done, thanks.

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

adisuissa · 2021-06-01T02:40:47Z

/lgtm api

suniltheta · 2021-06-01T23:07:43Z

Hi Maintainers, this PR is ready for merge?
@htuch & @adisuissa have given first pass approval. This commit should fix the issue #12432. Thank you for your time.
Tagging for attention @mattklein123, @htuch @dio, @lizan

mattklein123 · 2021-06-02T02:09:46Z

cc @alyssawilk this is a case for the bot of a PR not getting a non-API reviewer. I will review.

mattklein123

Thank you!

alyssawilk · 2021-06-02T16:45:24Z

Ugh, this was definitely missed with old and new versions of the script, though I also see cases in maintainer-oncall where the API bug is definitely caught. Can't debug this easily since it's merged but please let me know if you see other cases :-(

alyssawilk · 2021-06-02T16:52:46Z

on no wait, this was caught, I just searched for the wrong string. So should be fixed forward as long as on-call goes through the list :-)

YaseenAlk · 2021-08-12T17:29:15Z

Hi, I am running buf on some previous commits to the envoy api to see if there were any commits that break api compatibility and it detected this change as being non-backwards-compatible:

api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto:110:3:Field "9" on message "DnsCacheConfig" changed type from "envoy.config.core.v3.DnsResolver" to "envoy.config.core.v3.DnsResolutionConfig".
api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto:110:38:Field "9" on message "DnsCacheConfig" changed name from "dns_resolver" to "dns_resolution_config".
api/envoy/extensions/common/dynamic_forward_proxy/v3/dns_cache.proto:110:3:Field "9" with name "dns_resolution_config" on message "DnsCacheConfig" changed option "json_name" from "dnsResolver" to "dnsResolutionConfig".

Just wanted to flag it in case it was unintentional! cc @adisuissa

htuch · 2021-08-13T02:37:51Z

@YaseenAlk what is the SHA or PR this happened at? I seem to remember OKing something like that because it was within a short window of the original commit, where we allow these changes, but I should verify.

suniltheta · 2021-08-13T02:44:53Z

The incompatibility is between this PR and the PR at #16294
And anything that is incompatible is not across a release. So we should be good here?

htuch · 2021-08-13T02:47:25Z

Yep, we allow breaking API changes within a 14 day window, see https://github.com/envoyproxy/envoy/blob/main/api/API_VERSIONING.md.

Good catch though!

jpeach · 2021-08-30T23:51:04Z

@htuch The upstream_resolvers field in DnsFilterConfig was present in Envoy 1.18, but absent in Envoy 1.19. AFAICT that makes this an incompatible change, and it's also difficult for a control plane to support both since the field number was re-used.

suniltheta · 2021-08-31T00:00:42Z

https://github.com/envoyproxy/envoy/blob/main/api/API_VERSIONING.md

API versions tagged vNalpha. Within an alpha major version, arbitrary breaking changes are allowed.

@jpeach That is true, but the API is still v3alpha. Hope that is allowed.

jpeach · 2021-08-31T00:25:13Z

https://github.com/envoyproxy/envoy/blob/main/api/API_VERSIONING.md

API versions tagged vNalpha. Within an alpha major version, arbitrary breaking changes are allowed.

@jpeach That is true, but the API is still v3alpha. Hope that is allowed.

Is it possible to upgrade across the breaking change? i.e. I have to serve 1.18 and 1.19 clients concurrently.

…ver (envoyproxy#16237) Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

Modify API to support configuration of ARES flags to disable the use …

49e3a98

…of search namespaces when using the c-ares resolver and reconcile DNS options in a single message Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only bot added the api label Apr 29, 2021

repokitteh-read-only bot assigned adisuissa Apr 29, 2021

auto generated proto format files for api proto file change

5862a19

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

htuch reviewed Apr 30, 2021

View reviewed changes

adisuissa reviewed Apr 30, 2021

View reviewed changes

suniltheta force-pushed the area_flag_nosearch_option branch 2 times, most recently from 009f522 to 5862a19 Compare April 30, 2021 06:28

add reason why the field use_tcp_for_dns_lookups is deprecated and ad…

dc3d735

…dressed few review comments Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

adisuissa requested changes May 4, 2021

View reviewed changes

adisuissa reviewed May 4, 2021

View reviewed changes

htuch mentioned this pull request May 6, 2021

dynamic_forward_proxy: adding dns_resolvers to dns_cache used by the proxy filter #16294

Merged

Sunil Narasimhamurthy added 2 commits May 6, 2021 04:39

Modify API to support configuration of ARES flags to disable the use …

0804c30

…of search namespaces when using the c-ares resolver and reconcile DNS options in a single message. Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

Merge branch 'main' into area_flag_nosearch_option

690a228

suniltheta changed the title ~~[WIP] dns filter: disable the use of search namespaces when using the c-ares resolver~~ dns: disable the use of search namespaces when using the c-ares resolver May 6, 2021

suniltheta marked this pull request as ready for review May 6, 2021 04:48

suniltheta requested review from PiotrSikora, alyssawilk, lizan and mattklein123 as code owners May 6, 2021 04:48

Sunil Narasimhamurthy added 2 commits May 6, 2021 16:36

make logic more readable and remove unused imports

285cdad

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

fix clang tidy issue

782c926

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

adisuissa reviewed May 7, 2021

View reviewed changes

Sunil Narasimhamurthy added 2 commits May 8, 2021 00:04

Address PR comments by using CopyFrom to simplify logic also fix CI i…

f909844

…ssue in a Test Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

Merge branch 'main' into area_flag_nosearch_option

ab58afe

htuch reviewed May 28, 2021

View reviewed changes

add version history entry for DnsResolutionConfig & DnsResolverOptions

307d80e

Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

repokitteh-read-only bot removed the api label Jun 1, 2021

mattklein123 self-assigned this Jun 2, 2021

mattklein123 approved these changes Jun 2, 2021

View reviewed changes

mattklein123 merged commit 2c0e1f2 into envoyproxy:main Jun 2, 2021

This was referenced Jun 2, 2021

dns: do not use default search domain for dns querying suniltheta/envoy#3

Closed

DNS filter: O(n2) domain search #12432

Closed

chaoqin-li1123 mentioned this pull request Jun 16, 2021

remove invalid corpus that contains use_tcp_for_dns_lookup in bootstrap proto #17011

Closed

jpeach mentioned this pull request Aug 31, 2021

chore(*) upgrade Envoy to v1.19.1 kumahq/kuma#2701

Closed

5 tasks

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

dns: disable the use of search namespaces when using the c-ares resol…

87abf8e

…ver (envoyproxy#16237) Signed-off-by: Sunil Narasimhamurthy <sunnrs@amazon.com>

	google.protobuf.BoolValue no_defalt_search_domain = 2;
	google.protobuf.BoolValue no_default_search_domain = 2;

	// server startup. Apple' API only uses UDP for DNS resolution.
	// server startup. Apple's API only uses UDP for DNS resolution.

		* @param use_tcp_for_dns_lookups if set to true, tcp will be used to perform dns lookups.
		* Otherwise, udp is used.

Conversation

suniltheta commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 29, 2021

Uh oh!

suniltheta commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suniltheta commented May 8, 2021

Uh oh!

suniltheta commented May 8, 2021

Uh oh!

repokitteh-read-only bot commented May 8, 2021

Uh oh!

suniltheta commented Apr 29, 2021 •

edited

Loading

suniltheta commented Apr 29, 2021 •

edited

Loading

suniltheta commented May 28, 2021 •

edited

Loading

suniltheta commented Jun 1, 2021 •

edited

Loading