dns: fix resolving STRICT_DNS cluster endpoint FQDN with trailing dot '.' by lmarszal · Pull Request #13644 · envoyproxy/envoy

lmarszal · 2020-10-20T08:19:43Z

Commit Message: dns: fix resolving STRICT_DNS cluster endpoint FQDN with trailing dot '.'
Additional Description: bump c-ares dependency to a version having c-ares/c-ares@ba59781 issue resolved
Risk Level: Low
Testing: Manual test - repro steps from #13587 now results in expected behaviour
Docs Changes: feature of adding trailing dot '.' to FQDN was never documented; not sure if applicable
Release Notes: N/A
Platform Specific Features: N/A

Fixes #13587

repokitteh-read-only · 2020-10-20T08:19:48Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #13644 was opened by lmarszal.

see: more, trace.

lmarszal · 2020-10-20T08:20:28Z

CC @htuch

Signed-off-by: Łukasz Marszał <lmarszal@gmail.com>

htuch

Thanks for the fix, couple of short questions.

htuch · 2020-10-20T14:19:25Z

-        "//bazel:windows_x86_64": "cp -L $EXT_BUILD_ROOT/external/com_github_c_ares_c_ares/nameser.h $INSTALLDIR/include/nameser.h",
-        "//conditions:default": "",
+        "//bazel:windows_x86_64": "cp -L $EXT_BUILD_ROOT/external/com_github_c_ares_c_ares/src/lib/nameser.h $INSTALLDIR/include/nameser.h && cp -L $EXT_BUILD_ROOT/external/com_github_c_ares_c_ares/src/lib/ares_dns.h $INSTALLDIR/include/ares_dns.h",
+        "//conditions:default": "cp -L $EXT_BUILD_ROOT/external/com_github_c_ares_c_ares/src/lib/ares_dns.h $INSTALLDIR/include/ares_dns.h",


Why is this now needed in non-Windows?

There was a change in directory structure in c-ares (c-ares/c-ares@0bf721c). We're using macros from ares_dns.h in dns_imp_test.cc - this header file is no longer available after make install, so I had to make a "hack" in order to make it visible for us.

lmarszal · 2020-10-20T16:13:12Z

Run into some concurrency (tsan and asan) problems in upgraded c-ares:

WARNING: ThreadSanitizer: heap-use-after-free (pid=15)
  Read of size 1 at 0x7b04000018cf by main thread:
    #0 as_is_first /b/f/w/external/com_github_c_ares_c_ares/src/lib/ares_getaddrinfo.c:765:7 (dns_impl_test+0x2e658eb)
    #1 next_dns_lookup /b/f/w/external/com_github_c_ares_c_ares/src/lib/ares_getaddrinfo.c:693:11 (dns_impl_test+0x2e648b2)
    #2 next_lookup /b/f/w/external/com_github_c_ares_c_ares/src/lib/ares_getaddrinfo.c:513:15 (dns_impl_test+0x2e64714)
    #3 next_lookup /b/f/w/external/com_github_c_ares_c_ares/src/lib/ares_getaddrinfo.c:527:11 (dns_impl_test+0x2e647df)
    #4 ares_getaddrinfo /b/f/w/external/com_github_c_ares_c_ares/src/lib/ares_getaddrinfo.c:681:3 (dns_impl_test+0x2e63c22)
    #5 Envoy::Network::DnsResolverImpl::PendingResolution::getAddrInfo(int) /proc/self/cwd/source/common/network/dns_impl.cc:283:3 (dns_impl_test+0x2e457e4)
    #6 Envoy::Network::DnsResolverImpl::resolve(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, Envoy::Network::DnsLookupFamily, std::__1::function<void (Envoy::Network::DnsResolver::ResolutionStatus, std::__1::list<Envoy::Network::DnsResponse, std::__1::allocator<Envoy::Network::DnsResponse> >&&)>) /proc/self/cwd/source/common/network/dns_impl.cc:252:25 (dns_impl_test+0x2e46543)
    #7 Envoy::Network::DnsImplTest::resolveWithExpectations(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, Envoy::Network::DnsLookupFamily, Envoy::Network::DnsResolver::ResolutionStatus, std::__1::list<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, std::__1::list<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > > const&, std::__1::optional<std::__1::chrono::duration<long long, std::__1::ratio<1l, 1l> > >) /proc/self/cwd/test/common/network/dns_impl_test.cc:483:23 (dns_impl_test+0x1a7d0b1)
    #8 Envoy::Network::DnsImplTest_DestroyChannelOnRefused_Test::TestBody() /proc/self/cwd/test/common/network/dns_impl_test.cc:681:3 (dns_impl_test+0x1a63b27)
    #9 void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2580:10 (dns_impl_test+0x52da1cc)
    #10 void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2616:14 (dns_impl_test+0x52babbe)
    #11 testing::Test::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2655:5 (dns_impl_test+0x529cbc1)
    #12 testing::TestInfo::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2832:11 (dns_impl_test+0x529da99)
    #13 testing::TestSuite::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2986:28 (dns_impl_test+0x529e70e)
    #14 testing::internal::UnitTestImpl::RunAllTests() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:5697:44 (dns_impl_test+0x52af03c)
    #15 bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2580:10 (dns_impl_test+0x52e174c)
    #16 bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:2616:14 (dns_impl_test+0x52be9ee)
    #17 testing::UnitTest::Run() /proc/self/cwd/external/com_google_googletest/googletest/src/gtest.cc:5280:10 (dns_impl_test+0x52ae8cb)
    #18 RUN_ALL_TESTS() /proc/self/cwd/external/com_google_googletest/googletest/include/gtest/gtest.h:2485:46 (dns_impl_test+0x2d06d27)
    #19 Envoy::TestRunner::RunTests(int, char**) /proc/self/cwd/test/test_runner.cc:155:10 (dns_impl_test+0x2d05d45)
    #20 main /proc/self/cwd/test/main.cc:34:10 (dns_impl_test+0x2d0319d)

I think I know where the problem is - I will update this PR after resolving this problem upstream.

htuch · 2020-10-20T19:58:38Z

Interesting, let us know if you need any pointers on Envoy's DNS implementation. I just merged #13582, which makes the mentioned last_updated --> release_date rename, so you might want to merge master to pick that up. Thanks.

Signed-off-by: Łukasz Marszał <lmarszal@gmail.com>

lmarszal · 2020-10-21T18:18:07Z

/retest

repokitteh-read-only · 2020-10-21T18:18:12Z

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

🐱

Caused by: a #13644 (comment) was created by @lmarszal.

see: more, trace.

lmarszal · 2020-10-21T19:29:07Z

@htuch It's all green.

htuch

@lmarszal wow, thanks for the upstream fix! 👍

htuch · 2020-10-21T23:50:48Z

/lgtm deps

daveey · 2020-10-24T03:26:47Z

Thanks for fixing! We hit this problem when upgrading envoy from 1.14 -> 1.16. Will this change make its way into a release any time soon?

ramaraochavali · 2021-01-26T04:04:10Z

@lizan @mattklein123 @PiotrSikora Is it possible to backport this to 1.16 release?

ramaraochavali · 2021-01-27T06:52:44Z

Discussed offline with @lizan . Since this has dependencies and hence an element of risk involved, backport for this is not recommended.

repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Oct 20, 2020

lmarszal added 3 commits October 20, 2020 10:21

bum c-ares

3953d1b

Signed-off-by: Łukasz Marszał <lmarszal@gmail.com>

fix tests build

bbc67a8

Signed-off-by: Łukasz Marszał <lmarszal@gmail.com>

fix windows tests build

001b39d

Signed-off-by: Łukasz Marszał <lmarszal@gmail.com>

htuch reviewed Oct 20, 2020

View reviewed changes

lmarszal added 3 commits October 21, 2020 17:11

Merge branch 'master' into 13587-fqdn-trailing-dot

feeac82

bump c-ares

b550f4c

Signed-off-by: Łukasz Marszał <lmarszal@gmail.com>

fix release date

94348c4

Signed-off-by: Łukasz Marszał <lmarszal@gmail.com>

htuch approved these changes Oct 21, 2020

View reviewed changes

repokitteh-read-only Bot removed the deps Approval required for changes to Envoy's external dependencies label Oct 21, 2020

htuch merged commit 58f6dfa into envoyproxy:master Oct 21, 2020

lmarszal deleted the 13587-fqdn-trailing-dot branch October 22, 2020 08:30

lmarszal mentioned this pull request Oct 22, 2020

Broken support for FQDN with trailing dot '.' istio/istio#28103

Closed

Conversation

lmarszal commented Oct 20, 2020

Uh oh!

repokitteh-read-only Bot commented Oct 20, 2020

Uh oh!

lmarszal commented Oct 20, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

lmarszal Oct 20, 2020

Choose a reason for hiding this comment

Uh oh!

lmarszal commented Oct 20, 2020

Uh oh!

htuch commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmarszal commented Oct 21, 2020

Uh oh!

repokitteh-read-only Bot commented Oct 21, 2020

Uh oh!

lmarszal commented Oct 21, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Oct 21, 2020

Uh oh!

daveey commented Oct 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramaraochavali commented Jan 26, 2021

Uh oh!

ramaraochavali commented Jan 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htuch commented Oct 20, 2020 •

edited

Loading

daveey commented Oct 24, 2020 •

edited

Loading