fix(resty.dns.client): fix the UDP sockets leak #10691

liverpool8056 · 2023-04-18T12:04:26Z

Summary

In resty.dns.client module when resolver:new() is called, a UDP socket is created. In 2.x, the UDP socket is released quickly, while in 3.x, it has to take several seconds to release. If the ttl of dns responses is shorter than the time it waits, the leak will happen. In this PR, a forcible release is added.

FTI-4962

Checklist

The Pull Request has tests
There's an entry in the CHANGELOG
There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Full changelog

[Implement ...]

Issue reference

FTI-4962

Tieske · 2023-04-18T12:15:26Z

kong/resty/dns/client.lua

@@ -702,6 +702,14 @@ local function parseAnswer(qname, qtype, answers, try_list)
  return true
 end

+local function close_socks(resolver)


do we have an upstream PR for this? if not can we send one (for lua-resty-dns)

no upstream PR committed yet, actually I think it might not be lua-resty-dnss business, the sockets could be destroyed in 2.8, and I don't know why not being destroyed in 3.x

hmmm, then maybe we should look for the root-cause first, before fixing it.

are we keeping them cached somewhere? such that they stay alive instead of getting GC'ed?

also; an upstream PR for always closing the sockets would be a sanity item imo. So would still make sesne to me.

since we're now fiddling with that libs internals.

seems not being cached somewhere, the resolver is created in ngx.timer, I'm not sure if the timer matters. However the logic of dns queries is similar between in 2.x and 3.x, so I have no idea why it just happens in 3.x.

@ADD-SP and I did further investigations and found that this is caused by timer-ng lib. After replacing the timer-ng with the native timer to schedule the dns query, this issue never happens. I'm not sure but it seems like there are some differences between timer-ng and native time over garbage collection.

Even if the issue is caused by timer-ng, it still makes sense to me that the sockets should be released explicitly.

This is very strange, we try to make timer-ng run full GC after each handler and this issue does not appear.

So there is no leak, but it looks like the GC has slowed down.

@Tieske I committed an upstream PR: openresty/lua-resty-dns#64
Can we just take this PR as a workaround so that this issue could be fixed quickly? If the PR for resty-dns is released, we can revert this PR and make a better one.

Based on OpenResty's claim at here, there are three ways to release socket objects:

Call the close() method explicitly.

Released by the Lua GC.

Current client HTTP request finishes processing.

Since lua-resty-dns does not explicitly close socket objects or expose the close() method to users, the relevant socket objects can only be released through methods 2 and 3.

The reason why the native timer is not problematic is that once the timer ends, the fake request is considered finished (effectively fulfilling the condition of method 3). However, timer_ng reuses the existing ngx_timer to execute tasks, so the expected condition of method 3 did not occur and it was only dependent on method 2. I believe this is why those UDP socket objects were leaked.

Can we just take this PR as a workaround so that this issue could be fixed quickly? If the PR for resty-dns is released, we can revert this PR and make a better one.

Yes, that'd work. Please create an item to track this. eg a PR based on the proposed changes in resty-dns, or an internal Jira ticket, whatever works for you.

kong/resty/dns/client.lua

spec/02-integration/05-proxy/05-dns_spec.lua

kong/resty/dns/client.lua

ms2008

As Thijs mentioned it seems more appropriate to add a new close() method to the upstream project lua-resty-dns, and then we call it here.

But overall this looks great; nice test BTW.

kong/resty/dns/client.lua

vm-001

LGTM.

I saw an upstream PR already out there, and this is just a temporary solution as it will switch to calling the convenient close function after the upstream one merged and released.

ADD-SP

I think the better way is write a patch to lua-resty-dns until they merge our fix and release a new version.

I don't want to do something in the core that should be done by an external dependency, which would reduce maintainability.

ADD-SP · 2023-04-24T02:34:32Z

spec/02-integration/05-proxy/05-dns_spec.lua

@@ -145,5 +145,60 @@ for _, strategy in helpers.each_strategy() do
        assert.response(r).has.status(503)
      end)
    end)
+    describe("udp sockets", function()


Of course, we should write tests for all fixes, but this issue is caused by third-party dependencies and I'm not sure how we should do it.

I think it's too heavy for introducing an integration test for this purpose, I'd prefer to use unit tests to cover it.

The perfect thing to do would be to have lua-resty-dns cover the issue, but until then I think it would be better to use unit tests instead.

I suspect that even if we change this pr to a patch, the test case is still coverable.

Yes, the issue can be covered, but does it make more sense to move the tests to unit tests?

I'm not sure that makes more sense, just a suggestion.

This comment is NOT A BLOCKER for this PR.

ADD-SP · 2023-04-24T02:42:50Z

@Tieske FYI, we plan to write a patch to the lua-resty-dns instead of changing the core code.

liverpool8056 · 2023-04-24T02:45:08Z

I think the better way is write a patch to lua-resty-dns until they merge our fix and release a new version.

makes sense to me

ADD-SP

spec/02-integration/05-proxy/05-dns_spec.lua

If someone who has no context for this issue read this test, it is hard to understand why we write this test.

So could you please add some comments to explain more?

done

build/openresty/patches/lua-resty-dns-0.22_01-destory_resolver.patch

Tieske · 2023-04-24T13:10:23Z

FYI, we plan to write a patch to the lua-resty-dns instead of changing the core code.

My 2cts: the upstream is moving slowly, so the patch is going to exist for quite a while probably. I personally find patches cumbersome to maintain, and the code is less clear, because the code is not in-line. You already need to know the patch exists to be able to reason about what you actually read in the code.
So imho maintainability is better if we apply the temporary fix to the core.

That said; it's not my call, so feel free to ignore.

spec/02-integration/05-proxy/05-dns_spec.lua

In `resty.dns.client` module a resolver is created for domain query. When `resolver:new()` is called, a UDP socket is created. In 2.x, the UDP socket is released quickly, while in 3.x, it has to take several seconds to release. If the ttl of dns reponses is shorter than the time it waits, the leak will happen. In this PR, a forcible release is added. FTI-4962

Co-authored-by: Thijs Schreijer <thijs@thijsschreijer.nl>

Co-authored-by: Zijing Zhang <50045289+pluveto@users.noreply.github.com>

Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>

…cessary For those sockets created inside the schedule of timer-ng, the sockets must be closed explicitly. More details, please refer to: [#10691](#10691)

…cessary (#10783) * fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when necessary For those sockets created inside the schedule of timer-ng, the sockets must be closed explicitly. For more details, please refer to: [#10691](#10691) Co-authored-by: Chrono <chrono_cpp@me.com> Co-authored-by: Jun Ouyang <ouyangjun1999@gmail.com>

kong/resty/dns/client.lua

#10691) * fix(resty.dns.client): fix the leak of UDP sockets in resty.dns.client In `resty.dns.client` module a resolver is created for domain query. When `resolver:new()` is called, a UDP socket is created. In 2.x, the UDP socket is released quickly, while in 3.x, it has to take several seconds to release. If the ttl of dns reponses is shorter than the time it waits, the leak will happen. In this PR, a forcible release is added. FTI-4962 FTI-5803 --------- Co-authored-by: Thijs Schreijer <thijs@thijsschreijer.nl> Co-authored-by: Zijing Zhang <50045289+pluveto@users.noreply.github.com> Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>

KAG-5729, KAG-5736, KAG-5755, KAG-5770, KAG-5757 (cherry picked from commit #13853) Co-authored-by: Chrono <chrono_cpp@me.com>

pull-request-size bot added the size/M label Apr 18, 2023

github-actions bot assigned liverpool8056 Apr 18, 2023

github-actions bot added the changelog label Apr 18, 2023

liverpool8056 force-pushed the fix/udp-socket-leak-in-dns-client branch 2 times, most recently from 82f6e1c to 6e1abe8 Compare April 18, 2023 12:16

Tieske reviewed Apr 18, 2023

View reviewed changes

kong/resty/dns/client.lua Outdated Show resolved Hide resolved

pluveto reviewed Apr 18, 2023

View reviewed changes

spec/02-integration/05-proxy/05-dns_spec.lua Outdated Show resolved Hide resolved

ms2008 reviewed Apr 18, 2023

View reviewed changes

kong/resty/dns/client.lua Outdated Show resolved Hide resolved

liverpool8056 force-pushed the fix/udp-socket-leak-in-dns-client branch from f711900 to f238569 Compare April 19, 2023 05:02

liverpool8056 requested review from Tieske and ms2008 April 20, 2023 03:26

ms2008 reviewed Apr 20, 2023

View reviewed changes

vm-001 reviewed Apr 23, 2023

View reviewed changes

kong/resty/dns/client.lua Outdated Show resolved Hide resolved

vm-001 approved these changes Apr 23, 2023

View reviewed changes

ADD-SP reviewed Apr 24, 2023

View reviewed changes

ADD-SP previously requested changes Apr 24, 2023

View reviewed changes

pull-request-size bot added size/L and removed size/M labels Apr 24, 2023

github-actions bot added the build/bazel label Apr 24, 2023

liverpool8056 force-pushed the fix/udp-socket-leak-in-dns-client branch from df98961 to 524a86e Compare April 24, 2023 06:44

liverpool8056 requested a review from ADD-SP April 24, 2023 07:04

liverpool8056 added the pr/please review label Apr 24, 2023

ADD-SP reviewed Apr 24, 2023

View reviewed changes

build/openresty/patches/lua-resty-dns-0.22_01-destory_resolver.patch Outdated Show resolved Hide resolved

locao reviewed Apr 24, 2023

View reviewed changes

spec/02-integration/05-proxy/05-dns_spec.lua Outdated Show resolved Hide resolved

liverpool8056 and others added 13 commits April 25, 2023 11:13

CHANGELOG

036105c

a missed destory

986f72b

Update kong/resty/dns/client.lua

1a6855c

Co-authored-by: Thijs Schreijer <thijs@thijsschreijer.nl>

make spec compatible for dbless

e115bc6

Update spec/02-integration/05-proxy/05-dns_spec.lua

b1da143

Co-authored-by: Zijing Zhang <50045289+pluveto@users.noreply.github.com>

empty trick

b591603

truncate routes

59885a6

lua-resty-dns patch

3a9d79d

add comment for the test case

7ae0d25

remove uselessness

72ad0a9

Update spec/02-integration/05-proxy/05-dns_spec.lua

84d1145

Co-authored-by: Vinicius Mignot <vinicius.mignot@gmail.com>

comment for destroy

5cf355a

ms2008 force-pushed the fix/udp-socket-leak-in-dns-client branch from e759f88 to 5cf355a Compare April 25, 2023 03:13

windmgc approved these changes Apr 25, 2023

View reviewed changes

windmgc merged commit f46078c into master Apr 25, 2023

windmgc deleted the fix/udp-socket-leak-in-dns-client branch April 25, 2023 04:17

liverpool8056 mentioned this pull request May 4, 2023

fix(loggly&tcp-log&udp-log&reports): close sockets explicitly when necessary #10783

Merged

3 tasks

chobits reviewed Nov 16, 2023

View reviewed changes

kong/resty/dns/client.lua Show resolved Hide resolved

liverpool8056 mentioned this pull request Mar 6, 2024

[backport-2.8.x] fix(dns): fix the UDP socket leaking issue #12698

Merged

3 tasks

lhanjian pushed a commit that referenced this pull request Dec 23, 2024

tests(clustering/rpc): add test cases for inc sync (#10691)

f11e0b8

KAG-5729, KAG-5736, KAG-5755, KAG-5770, KAG-5757 (cherry picked from commit #13853) Co-authored-by: Chrono <chrono_cpp@me.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(resty.dns.client): fix the UDP sockets leak #10691

fix(resty.dns.client): fix the UDP sockets leak #10691

liverpool8056 commented Apr 18, 2023

Tieske Apr 18, 2023

liverpool8056 Apr 18, 2023

Tieske Apr 18, 2023

Tieske Apr 18, 2023 •

edited

Loading

liverpool8056 Apr 18, 2023

liverpool8056 Apr 20, 2023

ADD-SP Apr 20, 2023

liverpool8056 Apr 21, 2023

ms2008 Apr 23, 2023

Tieske Apr 24, 2023

ms2008 left a comment

vm-001 left a comment

ADD-SP left a comment

ADD-SP Apr 24, 2023

liverpool8056 Apr 24, 2023

ADD-SP Apr 24, 2023

ADD-SP Apr 24, 2023

ADD-SP Apr 24, 2023 •

edited

Loading

ADD-SP commented Apr 24, 2023 •

edited

Loading

liverpool8056 commented Apr 24, 2023

ADD-SP left a comment

Tieske commented Apr 24, 2023

fix(resty.dns.client): fix the UDP sockets leak #10691

fix(resty.dns.client): fix the UDP sockets leak #10691

Conversation

liverpool8056 commented Apr 18, 2023

Summary

Checklist

Full changelog

Issue reference

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tieske Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms2008 left a comment

Choose a reason for hiding this comment

vm-001 left a comment

Choose a reason for hiding this comment

ADD-SP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ADD-SP Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

ADD-SP commented Apr 24, 2023 • edited Loading

liverpool8056 commented Apr 24, 2023

ADD-SP left a comment

Choose a reason for hiding this comment

Tieske commented Apr 24, 2023

Tieske Apr 18, 2023 •

edited

Loading

ADD-SP Apr 24, 2023 •

edited

Loading

ADD-SP commented Apr 24, 2023 •

edited

Loading