dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED #9899

junr03 · 2020-01-31T23:23:34Z

Description: this PR adds logic to the DnsResolverImpl to destroy and re-initialize its c-ares channel under certain circumstances. A better option would require work in c-ares c-ares/c-ares#301.
Risk Level: med changes in low-level DNS resolution.
Testing: unit tests

Fixes #4543

Signed-off-by: Jose Nino jnino@lyft.com

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-01-31T23:24:09Z

opening as draft until I write tests monday.

cc @htuch as the original writer.

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

Thanks for investigating and fixing this issue. Very excited to see this finally understood and resolved.

source/common/network/dns_impl.cc

source/common/network/dns_impl.h

junr03 · 2020-02-01T00:30:35Z

Whoops did not mean for you to review my half baked code. Thanks for the comments though, I agree with all of them @mattklein123. Will update and write tests Monday.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03

@mattklein123 updated with your comments

junr03 · 2020-02-05T00:56:39Z

source/common/network/dns_impl.cc

+    // The channel cannot be destroyed and reinitialized here because that leads to a c-ares
+    // segfault.
+    if (status == ARES_ECONNREFUSED) {
+      parent_.dirty_channel_ = true;


I thought about this for a bit and debated between returning here, vs. allowing execution to continue and eventually call the callback_.

I ended up going for allowing the callback for a couple reasons:

Ease of testing, as having the callback allows for nice event loop triggers and a way to test assertions on the address_list.

It preserves previous behavior where every time resolution completed, whether successfully or not (with the exception of destruction), the callback_ was called. Obviously after fallbacks were exhausted, and if the query was not cancelled.

wdyt?

Yeah I agree we need to call the callback. As you already pointed out this is related to the other issue, so let's just fix the callbacks to expose errors in a future change.

mattklein123

Awesome work investigating this and fixing! Very excited to see this finally understood.

mattklein123 · 2020-02-05T22:35:23Z

source/common/network/dns_impl.cc

+    // The channel cannot be destroyed and reinitialized here because that leads to a c-ares
+    // segfault.
+    if (status == ARES_ECONNREFUSED) {
+      parent_.dirty_channel_ = true;


Yeah I agree we need to call the callback. As you already pointed out this is related to the other issue, so let's just fix the callbacks to expose errors in a future change.

mattklein123 · 2020-02-05T22:37:57Z

test/common/network/dns_impl_test.cc

+  // If that flag needs to be set, or c-ares changes its handling this test will need to be updated
+  // to create another condition where c-ares invokes onAresGetAddrInfoCallback with status ==
+  // ARES_ECONNREFUSED.


Agreed this is a little fragile but I think it's OK for now. Nice work figuring out a way to test this!

Bumping to include: - `dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED`: envoyproxy/envoy#9899. This should resolve the issues we've been seeing with Envoy Mobile hanging on startup and never properly issuing requests if started in the offline state on iOS - `gzip: add force load factory declaration`: envoyproxy/envoy#9942. This will allow us to use the gzip filter with Envoy Mobile Signed-off-by: Michael Rebello <me@michaelrebello.com>

Bumping to include the following fixes: - `dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED`: envoyproxy/envoy#9899. This should resolve the issues we've been seeing with Envoy Mobile hanging on startup and never properly issuing requests if started in the offline state on iOS. Fixes #672, though more improvements to DNS resolution will be investigated in #673 - `gzip: add force load factory declaration`: envoyproxy/envoy#9942. This will allow us to use the gzip filter with Envoy Mobile - `api listener: add shutdown method and call during server termination`: envoyproxy/envoy#9959. Fixes #667. Fixes #674 Includes the following PRs to fix iOS liveliness tests as well: - `metrics service: force link v2 config`: envoyproxy/envoy#9875 - `config: remove ApiTypeOracle assert`: envoyproxy/envoy#9973 Also contains necessary updates to accommodate [this change](envoyproxy/envoy@dea4eb0). Signed-off-by: Michael Rebello <me@michaelrebello.com>

Description:PendingResolutions get destroyed when complete or when c-ares sent ARES_EDESTRUCTION. Prior to #9899 ARES_EDESTRUCTION only happened when the resolver was destroyed. However, after #9899 the channel, and thus PendingResolutions can be destroyed, without the callback target being aware. This leads to potential use after free issues. This PR calls the resolve callback when the status received is ARES_EDESTRUCTION. Risk Level: med Testing: added test that reproduced segfault and passed after fix. Signed-off-by: Jose Nino <jnino@lyft.com>

Bumping to include the following fixes: - `dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED`: #9899. This should resolve the issues we've been seeing with Envoy Mobile hanging on startup and never properly issuing requests if started in the offline state on iOS. Fixes envoyproxy/envoy-mobile#672, though more improvements to DNS resolution will be investigated in envoyproxy/envoy-mobile#673 - `gzip: add force load factory declaration`: #9942. This will allow us to use the gzip filter with Envoy Mobile - `api listener: add shutdown method and call during server termination`: #9959. Fixes envoyproxy/envoy-mobile#667. Fixes envoyproxy/envoy-mobile#674 Includes the following PRs to fix iOS liveliness tests as well: - `metrics service: force link v2 config`: #9875 - `config: remove ApiTypeOracle assert`: #9973 Also contains necessary updates to accommodate [this change](dea4eb0). Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

Jose Nino added 2 commits January 31, 2020 14:01

wip

13f6454

Signed-off-by: Jose Nino <jnino@lyft.com>

comments

f4ed3cd

Signed-off-by: Jose Nino <jnino@lyft.com>

comment

5077569

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 mentioned this pull request Jan 31, 2020

dns: DnsResolverImpl keeps using a "broken" c-ares channel #4543

Closed

fmt

0db2c6b

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 requested changes Jan 31, 2020

View reviewed changes

source/common/network/dns_impl.cc Outdated Show resolved Hide resolved

source/common/network/dns_impl.cc Outdated Show resolved Hide resolved

source/common/network/dns_impl.cc Show resolved Hide resolved

source/common/network/dns_impl.h Outdated Show resolved Hide resolved

mattklein123 added the waiting label Jan 31, 2020

mattklein123 self-assigned this Jan 31, 2020

Jose Nino added 3 commits January 31, 2020 16:51

comments

de58b01

Signed-off-by: Jose Nino <jnino@lyft.com>

update

84c5a3c

Signed-off-by: Jose Nino <jnino@lyft.com>

spelling

49b7c29

Signed-off-by: Jose Nino <jnino@lyft.com>

repokitteh-read-only bot removed the waiting label Feb 5, 2020

reorder test

037f8a4

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 commented Feb 5, 2020

View reviewed changes

junr03 mentioned this pull request Feb 5, 2020

DNS timeouts cause no healthy upstream #9927

Closed

junr03 marked this pull request as ready for review February 5, 2020 18:41

mattklein123 approved these changes Feb 5, 2020

View reviewed changes

junr03 merged commit a8a43cb into envoyproxy:master Feb 6, 2020

rebello95 mentioned this pull request Feb 6, 2020

upstream: update Envoy to include new fixes envoyproxy/envoy-mobile#663

Merged

junr03 mentioned this pull request Feb 10, 2020

dns: stuck if started without connectivity envoyproxy/envoy-mobile#672

Closed

junr03 mentioned this pull request Feb 14, 2020

dns resolver: invoke resolve callback if not cancelled #10060

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED #9899

dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED #9899

junr03 commented Jan 31, 2020 •

edited

Loading

junr03 commented Jan 31, 2020

mattklein123 left a comment

junr03 commented Feb 1, 2020

junr03 left a comment

junr03 Feb 5, 2020

mattklein123 Feb 5, 2020

mattklein123 left a comment

mattklein123 Feb 5, 2020

mattklein123 Feb 5, 2020

dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED #9899

dns: destroy/reinitialize c-ares channel on ARES_ECONNREFUSED #9899

Conversation

junr03 commented Jan 31, 2020 • edited Loading

junr03 commented Jan 31, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

junr03 commented Feb 1, 2020

junr03 left a comment

Choose a reason for hiding this comment

junr03 Feb 5, 2020

Choose a reason for hiding this comment

mattklein123 Feb 5, 2020

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Feb 5, 2020

Choose a reason for hiding this comment

mattklein123 Feb 5, 2020

Choose a reason for hiding this comment

junr03 commented Jan 31, 2020 •

edited

Loading