sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.28k stars 935 forks source link

Got's DNS cache in Kubernetes makes some queries 5s slower #1238

Closed fungiboletus closed 4 years ago

fungiboletus commented 4 years ago

Describe the bug

The DNS Cache feature may not be well compatible with some Kubernetes clusters. cacheable-lookup uses dns.resolve and not dns.lookup. As explained in the NodeJS documentation, and in cacheable-lookup's readme, dns.lookup use the standard getaddrinfo from the standard library while dns.resolve use a NodeJS implementation.

In practices, some DNS phases takes a while. 2s or 5s more in my environment.

Environment

Actual behavior

About 10% of got queries are taking up to 2 to 5 seconds during the DNS phase. It's because got uses dns.resolve and not dns.lookup. I don't know why dns.resolve has this issue. If you Google DNS, kubernetes and 5s, you will never consider to use Kubernetes in production again. Anyway, dns.lookup and getaddrinfo are always fast.

Expected behavior

All requests should have a fast DNS phase without random delays.

Code to reproduce

First you need a shitty Kubernetes cluster. Azure AKS offers great shitty Kubernetes cluster.

kubectl run got-reproduce-bug-node-14 --rm -i --tty --image node:14-buster-slim -- bash
...
npm install got
node
const got = require('got');
setInterval(() => {
    got('http://another_service_from_your_cluster:4242').then(response =>{
        console.log(`${response.timings.phases.dns},${response.timings.phases.total}`);
    });
}, 3000);

You may see randomly some queries taking 2s or 5s, while most of them are very fast.

If you disable dnsCache, all the queries are fast:

const got = require('got');
setInterval(() => {
    got('http://another_service_from_your_cluster:4242', {
        dnsCache: false,
    }).then(response =>{
        console.log(`${response.timings.phases.dns},${response.timings.phases.total}`);
    });
}, 3000);

Suggestion

Perhaps the dnsCache option should be false by default, as it introduces major changes in behaviour, as explained in the NodeJS documentation. Another possibility is to add a section in the README about the dnsCache and the considerations to have for some systems such as Kubernetes. Similarly to the retry option.

Checklist

szmarczak commented 4 years ago

We're going to turn it off due to the differences. Will make a release in 10 mins.

szmarczak commented 4 years ago

Once the Travis tests pass, I'll release 11.1.2.

szmarczak commented 4 years ago

Released 11.1.2, please let me know if it's fixed.

fungiboletus commented 4 years ago

It's fixed using the default got options ! 🎉

Thanks a lot for the super quick fix and release.