DNS: allow propagating DNS responses with no records back to caller (#20890) by wanlill · Pull Request #21027 · envoyproxy/envoy

wanlill · 2022-04-26T22:04:44Z

Currently ares resolver treats ARES_ENODATA and ARES_ENOTFOUND as failures and won't propagate the results back to the caller, this makes callers like StrictDnsClusterImpl not able to get a chance to detect the change when all records for a name are gone.

Signed-off-by: Wanli Li wanlil@netflix.com

Commit Message: DNS: allow propagating DNS responses with no records back to caller (#20890)
Additional Description: Currently ares resolver treats ARES_ENODATA and ARES_ENOTFOUND as failures and won't propagate the results back to the caller, this makes callers like strict_dns cluster not able to get a chance to detect the change when all records for a name are gone.
Risk Level: High, I did realized that some buggy resolver/recursor might return nxdomain or nodata on failure and this may result in callers like StrictDnsClusterImpl removing all the instances. LogicalDnsCluster and RedisCluster already have checks against success response with no records but we'll change the DNS behavior that can be observed by callers which don't have this check (e.g. StrictDnsClusterImpl, in a good way, this is exactly what initiated this change), some of these callers might don't want this behavior change.
Testing: New unit tests added to dns_impl_test.cc
Docs Changes: current.rst changed, added to minor changes section.
Release Notes:
Platform Specific Features:
Runtime guard: envoy.reloadable_features.cares_accept_nodata controls individual dns resolution request
Fixes: #20890

…nvoyproxy#20890) Currently ares resolver treats ARES_ENODATA and ARES_ENOTFOUND as failures and won't propagate the results back to the caller, this makes callers like strict_dns cluster not able to get a chance to detect the change when all records for a name are gone. Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only · 2022-04-26T22:04:52Z

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

🐱

Caused by: #21027 was opened by wanlill.

see: more, trace.

Signed-off-by: Wanli Li <wanlil@netflix.com>

mattklein123

Thanks for working on this. An API comment to get started.

/wait

mattklein123 · 2022-04-26T23:09:54Z

+  // Treat ARES_ENODATA and ARES_ENOTFOUND as valid responses and propagate the "no records" result back to the caller.
+  bool accept_nodata = 5;


IMO this is the way it should work by default and this is a bug, however, this is a high risk change, and I think we should runtime guard it (feature flag). WDYT?

+1, this is a high risk change.

I wanted to add a runtime guard but then decided to make it a configurable thing by changing the proto and make it default off because:

If this breaks someone then it probably implies their resolver/recursive might not behave correctly and it is probably a long term thing to fix the dns infra, meanwhile they will need to have a way to turn it off.

IMO runtime guard is a temp thing and we'll flip it on and remove the guard at some time.

That said, I'm totally fine to add a guard and make it effective only if both the guard and the config are turned on, do we want that or just a runtime guard w/o the config option (i.e. change the default behavior but guard it with a flag)?

I think I would change the default behavior, clearly document it, and add the runtime flag for this behavior change. We can add a config option later if someone complains about it because they have to flip the flag. APIs are forever and I would rather not add a new API unless we really need to. Thank you!

/wait

SGTM, thanks for the clarification! Can you tell me a little bit more about the "config option layer" you mentioned above? I'm afraid this is something I'm not familiar with (unless it means the proto change we've already been doing here 😀 )

Sorry s/layer/later :)

Signed-off-by: Wanli Li <wanlil@netflix.com>

…ard flag on and off cases Signed-off-by: Wanli Li <wanlil@netflix.com>

yanavlasov

/wait

yanavlasov · 2022-04-28T13:41:04Z

    dual_resolution_ = true;
  }

+  accept_nodata_ = Runtime::runtimeFeatureEnabled("envoy.reloadable_features.cares_accept_nodata");


Please move this into the constructor initializer list.

yanavlasov · 2022-04-28T13:41:37Z

    const DnsLookupFamily dns_lookup_family_;
    // Queried for at construction time.
    const AvailableInterfaces available_interfaces_;
+    bool accept_nodata_;


Please mark it const and provide default initializer.

Signed-off-by: Wanli Li <wanlil@netflix.com>

mattklein123

Thanks generally LGTM with small comments.

/wait

mattklein123 · 2022-04-28T21:18:43Z

    // that the first lookup failed to return any addresses. Note that DnsLookupFamily::All issues
    // both lookups concurrently so there is no need to fire a second lookup here.
    if (dns_lookup_family_ == DnsLookupFamily::Auto) {
+      family_ = AF_INET;


Why is this change and the one below needed?

They are not needed, I just found it's a bit confusing while debugging it since the lookup family_ has changed for the second request, happy to revert this.

OK got it, makes sense. Fine to keep.

mattklein123 · 2022-04-28T21:21:15Z

  if (status != ARES_SUCCESS) {
-    ENVOY_LOG_EVENT(debug, "cares_resolution_failure",
-                    "dns resolution for {} failed with c-ares status {}", dns_name_, status);
+    if (!accept_nodata_ || !isResponseWithNoRecords(status)) {


nit: you can merge this if into the one above.

mattklein123 · 2022-04-28T21:21:39Z

    const DnsLookupFamily dns_lookup_family_;
    // Queried for at construction time.
    const AvailableInterfaces available_interfaces_;
+    const bool accept_nodata_{false};


initializer not needed as you initialize in the constructor.

mattklein123 · 2022-04-28T21:23:08Z

 }

-class DnsImplTest : public testing::TestWithParam<Address::IpVersion> {
+std::vector<std::tuple<Address::IpVersion, bool>> paramGenerator() {


I don't think we need to parameterize every test. Can you just fix all the tests for the new behavior and then add a specific test with a reverted scoped runtime to make sure the revert behavior works also?

repokitteh-read-only · 2022-05-02T23:00:51Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #21027 (comment) was created by @wanlill.

see: more, trace.

yanjunxiang-google · 2022-05-03T01:12:45Z

+    // Treat `ARES_ENODATA` or `ARES_ENOTFOUND` here as success to populate back the
+    // "empty records" response.
+    pending_response_.status_ = ResolutionStatus::Success;
+    ASSERT(addrinfo == nullptr);


Are we sure the caller always set addrinfo to be nullptr when the status is ARES_ENODATA or ARES_ENOTFOUND?

Others LGTM.

I think so, otherwise the current DnsImpl should have been leaking memory in the non-success callback branch, c-ares doc and code for reference.

yanavlasov · 2022-05-03T15:28:58Z

/retest

repokitteh-read-only · 2022-05-03T15:29:02Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #21027 (comment) was created by @yanavlasov.

see: more, trace.

yanavlasov · 2022-05-03T15:30:48Z

I think something got stuck on AZP. Can you add an empty commit or merge main to restart CI please? I will approve and merge after that.

yanavlasov · 2022-05-03T15:30:59Z

/wait

Signed-off-by: Wanli Li <wanlil@netflix.com>

yanjunxiang-google · 2022-05-03T21:08:15Z

LGTM. Defer to @yanavlasov for approval and merge.

Signed-off-by: Wanli Li <wanlil@netflix.com>

wanlill · 2022-05-05T18:34:17Z

@yanavlasov friendly ping, test is green, can you help to take another look?

yanavlasov · 2022-05-05T19:05:38Z

Ok sorry for the delay. One file has merge conflict. Can yo resolve please? You can ping me on Slack and I will enable automerge.

yanavlasov · 2022-05-05T19:05:47Z

/wait

wanlill · 2022-05-05T21:55:57Z

/retest

repokitteh-read-only · 2022-05-05T21:56:01Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21027 (comment) was created by @wanlill.

see: more, trace.

wanlill · 2022-05-05T22:02:26Z

/retest

repokitteh-read-only · 2022-05-05T22:02:30Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21027 (comment) was created by @wanlill.

see: more, trace.

wanlill · 2022-05-05T23:10:09Z

/retest

repokitteh-read-only · 2022-05-05T23:10:13Z

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

🐱

Caused by: a #21027 (comment) was created by @wanlill.

see: more, trace.

…nvoyproxy#20890) (envoyproxy#21027) DNS: allow propagating DNS responses with no records back to caller (envoyproxy#20890) Currently ares resolver treats ARES_ENODATA and ARES_ENOTFOUND as failures and won't propagate the results back to the caller, this makes callers like strict_dns cluster not able to get a chance to detect the change when all records for a name are gone. Signed-off-by: Wanli Li <wanlil@netflix.com>

wanlill requested review from mattklein123 and yanavlasov as code owners April 26, 2022 22:04

repokitteh-read-only Bot added the api label Apr 26, 2022

repokitteh-read-only Bot assigned lizan Apr 26, 2022

wanlill mentioned this pull request Apr 26, 2022

STRICT_DNS clusters are incorrectly handling DNS responses with no records #20890

Closed

mattklein123 assigned mattklein123 and unassigned lizan Apr 26, 2022

fix log level typo

9c4b476

Signed-off-by: Wanli Li <wanlil@netflix.com>

mattklein123 requested changes Apr 26, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 26, 2022

fix format precheck

96b218e

Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only Bot removed the waiting label Apr 27, 2022

add a runtime guard

2d7b1bd

Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only Bot added the waiting label Apr 27, 2022

remove the cares proto config and make cares unit tests cover both gu…

7114213

…ard flag on and off cases Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only Bot removed the waiting label Apr 28, 2022

yanavlasov assigned yanavlasov and yanjunxiang-google Apr 28, 2022

yanavlasov requested changes Apr 28, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 28, 2022

change accept_nodata_ to const

d20c7af

Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only Bot removed the waiting label Apr 28, 2022

Merge branch 'main' into enodata-pr

5657f26

Signed-off-by: Wanli Li <wanlil@netflix.com>

mattklein123 added the waiting label Apr 28, 2022

fix doc format

8f692a3

Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only Bot removed the waiting label Apr 28, 2022

mattklein123 requested changes Apr 28, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 28, 2022

wanlill requested a review from yanavlasov May 2, 2022 23:00

yanjunxiang-google reviewed May 3, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label May 3, 2022

Kick CI

f4a2e2f

Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only Bot removed the waiting label May 3, 2022

Merge branch 'main' into enodata-pr

059b660

Signed-off-by: Wanli Li <wanlil@netflix.com>

Merge branch 'main' into enodata-pr

9168d76

Signed-off-by: Wanli Li <wanlil@netflix.com>

repokitteh-read-only Bot added the waiting label May 5, 2022

Merge remote-tracking branch 'wanlill/main' into enodata-pr

86d51af

repokitteh-read-only Bot removed the waiting label May 5, 2022

yanavlasov approved these changes May 6, 2022

View reviewed changes

yanavlasov merged commit 9160479 into envoyproxy:main May 6, 2022

KBaichoo mentioned this pull request Jan 18, 2023

envoy_reloadable_features_cares_accept_nodata deprecation #25009

Closed

Restless-ET mentioned this pull request May 5, 2023

Add a debug log entry when dns resolution status is ARES_ENODATA or ARES_ENOTFOUND #27214

Closed

		// Treat ARES_ENODATA and ARES_ENOTFOUND as valid responses and propagate the "no records" result back to the caller.
		bool accept_nodata = 5;

Conversation

wanlill commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Apr 26, 2022

Uh oh!

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

mattklein123 Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

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

repokitteh-read-only Bot commented May 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanavlasov commented May 3, 2022

Uh oh!

repokitteh-read-only Bot commented May 3, 2022

Uh oh!

yanavlasov commented May 3, 2022

Uh oh!

yanavlasov commented May 3, 2022

Uh oh!

yanjunxiang-google commented May 3, 2022

Uh oh!

wanlill commented May 5, 2022

Uh oh!

yanavlasov commented May 5, 2022

Uh oh!

yanavlasov commented May 5, 2022

Uh oh!

wanlill commented May 5, 2022

Uh oh!

repokitteh-read-only Bot commented May 5, 2022

Uh oh!

wanlill commented May 5, 2022

Uh oh!

repokitteh-read-only Bot commented May 5, 2022

Uh oh!

wanlill commented May 5, 2022

Uh oh!

repokitteh-read-only Bot commented May 5, 2022

Uh oh!

Reviewers

Assignees

Labels

wanlill commented Apr 26, 2022 •

edited

Loading

mattklein123 Apr 27, 2022 •

edited

Loading