From 031d723c7602248f9dd02177dfcf49e0a4587d33 Mon Sep 17 00:00:00 2001 From: Mathias Lundell Date: Fri, 25 Oct 2024 06:40:09 +0200 Subject: [PATCH 1/2] fix: dns interceptor ip may never time out This fixes so that the record lives at most ttl time since lookup by not refreshing the timeout when picked. --- lib/interceptor/dns.js | 22 ++++------- test/interceptors/dns.js | 80 +++++++++++++++++++--------------------- 2 files changed, 45 insertions(+), 57 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index e0e1d7fa486..7d38b4e09aa 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -55,7 +55,7 @@ class DNSInstance { return } - this.setRecords(origin, addresses) + this.setRecords(origin, newOpts, addresses) const records = this.#records.get(origin.hostname) const ip = this.pick( @@ -111,15 +111,9 @@ class DNSInstance { const results = new Map() for (const addr of addresses) { - const record = { - address: addr.address, - ttl: opts.maxTTL, - family: addr.family - } - // On linux we found duplicates, we attempt to remove them with // the latest record - results.set(`${record.address}:${record.family}`, record) + results.set(`${addr.address}:${addr.family}`, addr) } cb(null, results.values()) @@ -171,24 +165,24 @@ class DNSInstance { return ip } - const timestamp = Date.now() - // Record TTL is already in ms - if (ip.timestamp != null && timestamp - ip.timestamp > ip.ttl) { + if (Date.now() - ip.timestamp > ip.ttl) { // record TTL is already in ms // We delete expired records // It is possible that they have different TTL, so we manage them individually family.ips.splice(position, 1) return this.pick(origin, hostnameRecords, affinity) } - ip.timestamp = timestamp - this.lastIpFamily = newIpFamily return ip } - setRecords (origin, addresses) { + setRecords (origin, opts, addresses) { + const timestamp = Date.now() const records = { records: { 4: null, 6: null } } for (const record of addresses) { + record.timestamp = timestamp + record.ttl = opts.maxTTL + const familyRecords = records.records[record.family] ?? { ips: [] } familyRecords.ips.push(record) diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index e58a1d597ba..b756bc78519 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -500,7 +500,7 @@ test('Should automatically resolve IPs (dual stack disabled - 6)', async t => { }) test('Should we handle TTL (4)', async t => { - t = tspl(t, { plan: 7 }) + t = tspl(t, { plan: 10 }) let counter = 0 let lookupCounter = 0 @@ -536,6 +536,10 @@ test('Should we handle TTL (4)', async t => { case 2: t.equal(isIP(url.hostname), 4) break + + case 3: + t.equal(isIP(url.hostname), 4) + break default: t.fail('should not reach this point') } @@ -546,31 +550,13 @@ test('Should we handle TTL (4)', async t => { dns({ dualStack: false, affinity: 4, - maxTTL: 100, + maxTTL: 400, lookup: (origin, opts, cb) => { ++lookupCounter lookup( origin.hostname, { all: true, family: opts.affinity }, - (err, addresses) => { - if (err) { - return cb(err) - } - - const results = new Map() - - for (const addr of addresses) { - const record = { - address: addr.address, - ttl: opts.maxTTL, - family: addr.family - } - - results.set(`${record.address}:${record.family}`, record) - } - - cb(null, results.values()) - } + cb ) } }) @@ -591,7 +577,7 @@ test('Should we handle TTL (4)', async t => { t.equal(response.statusCode, 200) t.equal(await response.body.text(), 'hello world!') - await sleep(500) + await sleep(200) const response2 = await client.request({ ...requestOptions, @@ -600,11 +586,22 @@ test('Should we handle TTL (4)', async t => { t.equal(response2.statusCode, 200) t.equal(await response2.body.text(), 'hello world!') + + await sleep(300) + + const response3 = await client.request({ + ...requestOptions, + origin: `http://localhost:${server.address().port}` + }) + + t.equal(response3.statusCode, 200) + t.equal(await response3.body.text(), 'hello world!') + t.equal(lookupCounter, 2) }) test('Should we handle TTL (6)', async t => { - t = tspl(t, { plan: 7 }) + t = tspl(t, { plan: 10 }) let counter = 0 let lookupCounter = 0 @@ -642,6 +639,11 @@ test('Should we handle TTL (6)', async t => { // [::1] -> ::1 t.equal(isIP(url.hostname.slice(1, 4)), 6) break + + case 3: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break default: t.fail('should not reach this point') } @@ -652,31 +654,13 @@ test('Should we handle TTL (6)', async t => { dns({ dualStack: false, affinity: 6, - maxTTL: 100, + maxTTL: 400, lookup: (origin, opts, cb) => { ++lookupCounter lookup( origin.hostname, { all: true, family: opts.affinity }, - (err, addresses) => { - if (err) { - return cb(err) - } - - const results = [] - - for (const addr of addresses) { - const record = { - address: addr.address, - ttl: opts.maxTTL, - family: addr.family - } - - results.push(record) - } - - cb(null, results) - } + cb ) } }) @@ -706,6 +690,16 @@ test('Should we handle TTL (6)', async t => { t.equal(response2.statusCode, 200) t.equal(await response2.body.text(), 'hello world!') + + await sleep(300) + + const response3 = await client.request({ + ...requestOptions, + origin: `http://localhost:${server.address().port}` + }) + + t.equal(response3.statusCode, 200) + t.equal(await response3.body.text(), 'hello world!') t.equal(lookupCounter, 2) }) From 172654bba720ca0a239c9f18c89ffd2f5112a9d6 Mon Sep 17 00:00:00 2001 From: Mathias Lundell Date: Fri, 25 Oct 2024 11:53:18 +0200 Subject: [PATCH 2/2] fix: use instance options and take min TTL Take the minimum value between the record TTL and the maxTTL from options. The record TTL value is expected to be in ms even though the `dns.resolve` methods provide it in seconds. The reasons are that: * it avoids extra manipulation in the `setRecords` method * `dns.lookup` does not provide ttl * custom provided lookup method's using `dns.resolve` must be manipulated anyway by adding a value for the family. --- lib/interceptor/dns.js | 11 +++-- test/interceptors/dns.js | 91 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 3 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index 7d38b4e09aa..a78612331be 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -55,7 +55,7 @@ class DNSInstance { return } - this.setRecords(origin, newOpts, addresses) + this.setRecords(origin, addresses) const records = this.#records.get(origin.hostname) const ip = this.pick( @@ -176,12 +176,17 @@ class DNSInstance { return ip } - setRecords (origin, opts, addresses) { + setRecords (origin, addresses) { const timestamp = Date.now() const records = { records: { 4: null, 6: null } } for (const record of addresses) { record.timestamp = timestamp - record.ttl = opts.maxTTL + if (typeof record.ttl === 'number') { + // The record TTL is expected to be in ms + record.ttl = Math.min(record.ttl, this.#maxTTL) + } else { + record.ttl = this.#maxTTL + } const familyRecords = records.records[record.family] ?? { ips: [] } diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index b756bc78519..cc1841e417b 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -703,6 +703,97 @@ test('Should we handle TTL (6)', async t => { t.equal(lookupCounter, 2) }) +test('Should set lowest TTL between resolved and option maxTTL', async t => { + t = tspl(t, { plan: 9 }) + + let lookupCounter = 0 + const server = createServer() + const requestOptions = { + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + } + + server.on('request', (req, res) => { + res.writeHead(200, { 'content-type': 'text/plain' }) + res.end('hello world!') + }) + + server.listen(0, '127.0.0.1') + + await once(server, 'listening') + + const client = new Agent().compose( + dns({ + dualStack: false, + affinity: 4, + maxTTL: 200, + lookup: (origin, opts, cb) => { + ++lookupCounter + cb(null, [ + { + address: '127.0.0.1', + family: 4, + ttl: lookupCounter === 1 ? 50 : 500 + } + ]) + } + }) + ) + + after(async () => { + await client.close() + server.close() + + await once(server, 'close') + }) + + const response = await client.request({ + ...requestOptions, + origin: `http://localhost:${server.address().port}` + }) + + t.equal(response.statusCode, 200) + t.equal(await response.body.text(), 'hello world!') + + await sleep(100) + + // 100ms: lookup since ttl = Math.min(50, maxTTL: 200) + const response2 = await client.request({ + ...requestOptions, + origin: `http://localhost:${server.address().port}` + }) + + t.equal(response2.statusCode, 200) + t.equal(await response2.body.text(), 'hello world!') + + await sleep(100) + + // 100ms: cached since ttl = Math.min(500, maxTTL: 200) + const response3 = await client.request({ + ...requestOptions, + origin: `http://localhost:${server.address().port}` + }) + + t.equal(response3.statusCode, 200) + t.equal(await response3.body.text(), 'hello world!') + + await sleep(150) + + // 250ms: lookup since ttl = Math.min(500, maxTTL: 200) + const response4 = await client.request({ + ...requestOptions, + origin: `http://localhost:${server.address().port}` + }) + + t.equal(response4.statusCode, 200) + t.equal(await response4.body.text(), 'hello world!') + + t.equal(lookupCounter, 3) +}) + test('Should handle max cached items', async t => { t = tspl(t, { plan: 9 })