feat: add dns cache #2440

[antoinegomez]

This relates to #2440

Rationale

Implement a dns cache lookup feature as requested.

Copy code from https://github.com/szmarczak/cacheable-lookup/blob/master/source/index.mjs

Changes

• add lookup in core/connect.js

Features

• dns cache lookup

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

• [X] I have read and agreed to the Developer's Certificate of Origin
• [x] Tested
• [ ] Benchmarked (optional)
• [x] Documented
• [ ] Review ready
• [ ] In review
• [ ] Merge ready

[antoinegomez]

Note that this is just an initial draft, anyone can jump in and use this as a base or if in wrong direction simply discard it.

• if ipv6 is available on host and returned by lookup use this
• this is a quick copy of the mentioned cacheable-lookup module and it's clearly not finished
• the difficulty is to know how to set the options for this module and pass them down to the core/connect method

[antoinegomez]

Benchmarks:

Linux KDE Neon
AMD Ryzen 5 5600X 6-Core
4 x 8GB RAM

main branch 9b9ca602bcc33803200507280c2462b60e21b642

│ Tests               │ Samples │          Result │ Tolerance │ Difference with slowest │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - no keepalive │       1 │ 1962.85 req/sec │  ± 0.00 % │                       - │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - keepalive    │       1 │ 2387.90 req/sec │  ± 0.00 % │               + 21.65 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - pipeline   │       1 │ 3035.14 req/sec │  ± 0.00 % │               + 54.63 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - request    │       1 │ 3160.77 req/sec │  ± 0.00 % │               + 61.03 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - stream     │       1 │ 3807.23 req/sec │  ± 0.00 % │               + 93.96 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - dispatch   │       1 │ 4833.26 req/sec │  ± 0.00 % │              + 146.24 % │

Feature branch:

│ Tests               │ Samples │          Result │ Tolerance │ Difference with slowest │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - no keepalive │       1 │ 1824.48 req/sec │  ± 0.00 % │                       - │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ http - keepalive    │       1 │ 2483.32 req/sec │  ± 0.00 % │               + 36.11 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - request    │       1 │ 3087.63 req/sec │  ± 0.00 % │               + 69.23 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - pipeline   │       1 │ 3289.09 req/sec │  ± 0.00 % │               + 80.28 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - stream     │       1 │ 3935.02 req/sec │  ± 0.00 % │              + 115.68 % │
|─────────────────────|─────────|─────────────────|───────────|─────────────────────────|
│ undici - dispatch   │       1 │ 5294.26 req/sec │  ± 0.00 % │              + 190.18 % │

[antoinegomez]

Update:

• As mentioned above, happy to refactor a bit more and to remove some functionalities like the custom caching if we want to slim down the code and responsibilities. in the end people could easily use cacheable-lookup module in Undici if they want
• Added a file global-dns-resolver to get/set the global instance of the resolver, as my guess is that the default behavior we want is to cache across all requests
• There is no option/method to disable the DNS cache globally easily
  • for now you have to create and set a new global Agent with the option dnsResolver.disable: true
• By default and for the first release should we not enable it by default to test? This kind of feature could break people production if not tested more carefully. DNS sometimes can behaves in unexpected ways.

[mcollina]

I don't think there should be a global dns resolver. We already have a global agent, which is enough to handle this use case.

[codecov-commenter]

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (8ee39e8) 85.56%. Report is 311 commits behind head on main.

Files	Patch %	Lines
lib/dns-resolver.js	96.71%	7 Missing :warning:
lib/agent.js	85.71%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2552      +/-   ##
==========================================
+ Coverage   85.54%   85.56%   +0.02%     
==========================================
  Files          76       85       +9     
  Lines        6858     7810     +952     
==========================================
+ Hits         5867     6683     +816     
- Misses        991     1127     +136     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[antoinegomez]

Can we add tests that covers the usage within the Agent/Pool?

I did add two tests, one that will check that the cache is incrementing when using the DNS resolver and one that check that no cache is being used if disabled.

Since Agent is not exposing the DNSResolver it is not possible to get it and check the cache, but I will have a look for a solution to see if I can intercept/mock it.

[mcollina]

linting is failing

[antoinegomez]

I forgot with my other review but this does need types as well.

first time I add types in non typescript project, is that ok?

https://github.com/nodejs/undici/pull/2552/files#diff-6704654b8ba0b45cd2cc752d380e7c9f70c6c8aac678524044da44446db6c976

[mcollina]

cc @ShogunPanda

[ShogunPanda]

In regard of Happy Eyeballs, let me clarify our options here.

From what I see now, with this new feature undici will attempt the records in the order node (and thus the DNS server) resolves them). This is a good thing, at least as default.

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

If we do this, we have two options:

1. Implement Happy Eyeballs on the Dispatcher class.
2. Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

[mcollina]

I think we would need happy eyeballs to enable this by default.

[metcoder95]

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

We should also not forget to add an option to disable it if required

1. Implement Happy Eyeballs on the Dispatcher class.

I'd like to see if we can keep the Dispatcher class lean, and leverage all this logic to the DNSResolver class, so we can also enable users to customize the behaviour as they need.

2. Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

Like this pretty much, do you think this can enable features such as #2515?

[ShogunPanda]

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

We should also not forget to add an option to disable it if required

Absolutely yes!

1. Implement Happy Eyeballs on the Dispatcher class.

I'd like to see if we can keep the Dispatcher class lean, and leverage all this logic to the DNSResolver class, so we can also enable users to customize the behaviour as they need.

I'm fine with it. Maybe a separate class used by the the DNSResolver.

2. Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

Like this pretty much, do you think this can enable features such as #2515?

Probably yes, and I would love it!

[antoinegomez]

The next step would be to implement Happy Eyeballs. I would add a specific implementation in undici since we can be explicit and control the sockets we open and thus we can implement the full Happy Eyeballs specification. In other words, in Node I was forced to implement a serialized version of the algorithm (not to break too many things), here we don't have to.

We should also not forget to add an option to disable it if required

Absolutely yes!

1. Implement Happy Eyeballs on the Dispatcher class.

I'd like to see if we can keep the Dispatcher class lean, and leverage all this logic to the DNSResolver class, so we can also enable users to customize the behaviour as they need.

I'm fine with it. Maybe a separate class used by the the DNSResolver.

2. Implement Happy Eyeballs on the DNSResolver class. Maybe this is too much for it, but it would open the possibility to return to the caller an IP which we already verified via Happy Eyeballs. And/or, potentially, to return the socket we were already able to open.

Like this pretty much, do you think this can enable features such as #2515?

Probably yes, and I would love it!

I added some tests for types, tried to resolve your comments.

For the Happy Eyeballs I would be glad to do it too if that's ok. Any source or link to existing material?

Since I changed the behavior for this to be disabled by default it can be done in another PR.

[metcoder95]

Yeah, another PR will be good. So far I believe this is the implementation: https://github.com/ShogunPanda/node/blob/ffb326c583c6a23bc94ff4989fe8a06edb92b011/lib/net.js#L1401

and this is the standard that is based on: https://datatracker.ietf.org/doc/html/rfc8305

Are those resources all right @ShogunPanda?

As pointed out, we should seek to stick to the spec as much as we can as we have better ergonomics here to do so.

[ShogunPanda]

@metcoder95 Yes, all good links sir. :)

[antoinegomez]

Hello @ShogunPanda, I have read the RFC and also the code linked.

Some thoughts.

For now this PR is creating a custom lookup function to cache dns queries. So in my understanding if happy eye balls is already implemented in node/net module then it seems redundant to implement it again here.

By default the options for lookup are { family: undefined, hints: 32 } And in node/net the happy eye balls happens when: if (dnsopts.family !== 4 && dnsopts.family !== 6 && !localAddress && autoSelectFamily). I can see that autoSelectFamily is undefined by default in undici.

In the RFC it suggest that we should not wait for all DNS queries to resolve to start connecting. Starting to query ipv6 then ipv4. Would be a bit more complex to make it work.

I also understand that we could implement the happy eye balls in undici using a similar approach than node/net module but since we are relying on it I am wondering if it is worth it.

[ShogunPanda]

In Node.js it is going to use Happy Eyeballs only if using a hostname when doing net.connect, which is not the case in undici since we're using a DNS cache. We don't have to implement Happy Eyeballs exactly as the RFC mandates, for instance not even Node is doing it.

[osuka]

Great to see work on this, we are very interested in the possibility of DNS caching as in kubernetes in particular the DNS lookups become a big issue at scale (even with local caching)

[DarkGL]

@antoinegomez could I help with that pr in any way ?

[ronag]

Closing due to staleness.