fix: dns interceptor ip ttl (#3770)

Co-authored-by: Carlos Fuentes <[email protected]>
nodejs · Oct 27, 2024 · 2de0f34 · 2de0f34
1 parent 2e79b62
commit 2de0f34
Show file tree

Hide file tree

Showing 2 changed files with 138 additions and 54 deletions.
diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js
@@ -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,29 @@ 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) {
+    const timestamp = Date.now()
     const records = { records: { 4: null, 6: null } }
     for (const record of addresses) {
+      record.timestamp = timestamp
+      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: [] }
 
       familyRecords.ips.push(record)

diff --git 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,35 +654,94 @@ 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)
-            }
+          cb
+        )
+      }
+    })
+  ])
+
+  after(async () => {
+    await client.close()
+    server.close()
 
-            const results = []
+    await once(server, 'close')
+  })
 
-            for (const addr of addresses) {
-              const record = {
-                address: addr.address,
-                ttl: opts.maxTTL,
-                family: addr.family
-              }
+  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(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!')
 
-              results.push(record)
-            }
+  await sleep(300)
 
-            cb(null, results)
+  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 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()
@@ -697,16 +758,40 @@ test('Should we handle TTL (6)', async t => {
   t.equal(response.statusCode, 200)
   t.equal(await response.body.text(), 'hello world!')
 
-  await sleep(200)
+  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!')
-  t.equal(lookupCounter, 2)
+
+  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 => {