From 2b917e3abf33f3c02eefccccf789269094546192 Mon Sep 17 00:00:00 2001 From: Tim Perry Date: Thu, 16 Sep 2021 23:11:10 +0200 Subject: [PATCH 1/2] Fallback to dns.lookup separately for IPv4 & IPv6 This fixes an issue where no results would be incorrectly returned because using family:0 (the default) with dns.lookup would return only results from /etc/hosts if any results were found there. That meant if you had only an IPv4 match in /etc/hosts, but the OS was capable of resolving IPv6 as well, family:0 would only return IPv4 even though family:6 could return an IPv6 address successfully. This is fixed by querying for both separately in parallel, and combining them to get the result that we were expecting the default family:0 option to give us in the first place. --- source/index.js | 37 ++++++++++++++++++++----------------- tests/test.js | 19 ++++++++++++++----- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/source/index.js b/source/index.js index 642060b..963f32f 100644 --- a/source/index.js +++ b/source/index.js @@ -63,6 +63,17 @@ const isIterable = map => { return Symbol.iterator in map; }; +// Wrap a promise that returns an array, but might throw errors if there's no +// result available, and map those errors to an empty array result. +const ignoreNoResultErrors = dnsPromise => + dnsPromise.catch(error => { + if (error.code === 'ENODATA' || error.code === 'ENOTFOUND') { + return []; + } + + throw error; + }); + const ttl = {ttl: true}; const all = {all: true}; @@ -222,23 +233,11 @@ class CacheableLookup { } async _resolve(hostname) { - const wrap = async promise => { - try { - return await promise; - } catch (error) { - if (error.code === 'ENODATA' || error.code === 'ENOTFOUND') { - return []; - } - - throw error; - } - }; - // ANY is unsafe as it doesn't trigger new queries in the underlying server. const [A, AAAA] = await Promise.all([ this._resolve4(hostname, ttl), this._resolve6(hostname, ttl) - ].map(promise => wrap(promise))); + ].map(promise => ignoreNoResultErrors(promise))); let aTtl = 0; let aaaaTtl = 0; @@ -281,12 +280,16 @@ class CacheableLookup { async _lookup(hostname) { try { - const entries = await this._dnsLookup(hostname, { - all: true - }); + const [ipV4Entries, ipV6Entries] = await Promise.all([ + // Testing families individually is required, as otherwise a match in a Hosts + // file can omit family results available elsewhere unexpectedly. See + // https://github.com/szmarczak/cacheable-lookup/issues/42 for more details. + ignoreNoResultErrors(this._dnsLookup(hostname, {family: 4, all: true})), + ignoreNoResultErrors(this._dnsLookup(hostname, {family: 6, all: true})) + ]); return { - entries, + entries: ipV4Entries.concat(ipV6Entries), cacheTtl: 0 }; } catch (_) { diff --git a/tests/test.js b/tests/test.js index de49005..e9ab1bd 100644 --- a/tests/test.js +++ b/tests/test.js @@ -788,7 +788,7 @@ test.serial('fallback works', async t => { t.deepEqual(resolver.counter, { 6: 1, 4: 1, - lookup: 2 + lookup: 3 }); await sleep(100); @@ -1015,12 +1015,21 @@ test('slow dns.lookup', async t => { resolver, lookup: (hostname, options, callback) => { t.is(hostname, 'osHostname'); - t.deepEqual(options, {all: true}); + t.is(options.all, true); + t.true(options.family === 4 || options.family === 6); setTimeout(() => { - callback(null, [ - {address: '127.0.0.1', family: 4} - ]); + if (options.family === 4) { + callback(null, [ + {address: '127.0.0.1', family: 4} + ]); + } + + if (options.family === 6) { + callback(null, [ + {address: '::1', family: 6} + ]); + } }, 10); } }); From c5d06c5b8eb26ce8eabfda5ee05518554808295b Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Tue, 5 Oct 2021 13:06:36 +0200 Subject: [PATCH 2/2] nitpicks --- source/index.js | 48 ++++++++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/source/index.js b/source/index.js index 963f32f..6c3bf42 100644 --- a/source/index.js +++ b/source/index.js @@ -63,19 +63,20 @@ const isIterable = map => { return Symbol.iterator in map; }; -// Wrap a promise that returns an array, but might throw errors if there's no -// result available, and map those errors to an empty array result. -const ignoreNoResultErrors = dnsPromise => - dnsPromise.catch(error => { +const ignoreNoResultErrors = dnsPromise => { + return dnsPromise.catch(error => { if (error.code === 'ENODATA' || error.code === 'ENOTFOUND') { return []; } throw error; }); +}; const ttl = {ttl: true}; const all = {all: true}; +const all4 = {all: true, family: 4}; +const all6 = {all: true, family: 6}; class CacheableLookup { constructor({ @@ -235,9 +236,9 @@ class CacheableLookup { async _resolve(hostname) { // ANY is unsafe as it doesn't trigger new queries in the underlying server. const [A, AAAA] = await Promise.all([ - this._resolve4(hostname, ttl), - this._resolve6(hostname, ttl) - ].map(promise => ignoreNoResultErrors(promise))); + ignoreNoResultErrors(this._resolve4(hostname, ttl)), + ignoreNoResultErrors(this._resolve6(hostname, ttl)) + ]); let aTtl = 0; let aaaaTtl = 0; @@ -279,25 +280,20 @@ class CacheableLookup { } async _lookup(hostname) { - try { - const [ipV4Entries, ipV6Entries] = await Promise.all([ - // Testing families individually is required, as otherwise a match in a Hosts - // file can omit family results available elsewhere unexpectedly. See - // https://github.com/szmarczak/cacheable-lookup/issues/42 for more details. - ignoreNoResultErrors(this._dnsLookup(hostname, {family: 4, all: true})), - ignoreNoResultErrors(this._dnsLookup(hostname, {family: 6, all: true})) - ]); - - return { - entries: ipV4Entries.concat(ipV6Entries), - cacheTtl: 0 - }; - } catch (_) { - return { - entries: [], - cacheTtl: 0 - }; - } + const [A, AAAA] = await Promise.all([ + // Passing {all: true} doesn't return all IPv4 and IPv6 entries. + // See https://github.com/szmarczak/cacheable-lookup/issues/42 + ignoreNoResultErrors(this._dnsLookup(hostname, all4)), + ignoreNoResultErrors(this._dnsLookup(hostname, all6)) + ]); + + return { + entries: [ + ...A, + ...AAAA + ], + cacheTtl: 0 + }; } async _set(hostname, data, cacheTtl) {