From 5a971d26214382737a51c98a9786cec92f833023 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 21 Aug 2024 12:43:20 +0200 Subject: [PATCH 01/15] feat: add DNS interceptor --- index.js | 3 +- lib/interceptor/dns.js | 238 +++++++++++++++++++++++++++++++++++++++ test/interceptors/dns.js | 51 +++++++++ 3 files changed, 291 insertions(+), 1 deletion(-) create mode 100644 lib/interceptor/dns.js create mode 100644 test/interceptors/dns.js diff --git a/index.js b/index.js index 444706560ae..090f6cfc892 100644 --- a/index.js +++ b/index.js @@ -39,7 +39,8 @@ module.exports.RedirectHandler = RedirectHandler module.exports.interceptors = { redirect: require('./lib/interceptor/redirect'), retry: require('./lib/interceptor/retry'), - dump: require('./lib/interceptor/dump') + dump: require('./lib/interceptor/dump'), + dns: require('./lib/interceptor/dns') } module.exports.buildConnector = buildConnector diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js new file mode 100644 index 00000000000..ccbf25da353 --- /dev/null +++ b/lib/interceptor/dns.js @@ -0,0 +1,238 @@ +'use strict' +const { isIP } = require('node:net') +const { lookup } = require('node:dns') +const DecoratorHandler = require('../handler/decorator-handler') +const maxInt = Math.pow(2, 31) - 1 + +class DNSInstance { + #maxTTL = 0 // TODO: support TTL + dualStack = true + record = new Map() + lookup = null + pick = null + lastIpFamily = null + + constructor (opts) { + this.#maxTTL = opts.maxTTL + this.dualStack = opts.dualStack + this.lookup = opts.lookup ?? this.#defaultLookup + this.pick = opts.pick ?? this.#defaultPick + } + + #defaultLookup (origin, opts, callback) { + // TODO: replace for resolve + lookup(origin.hostname, { all: true }, (err, addresses) => { + if (err) { + return callback(err) + } + + const results = addresses.map(addr => { + return { + address: addr.address, + ttl: opts.maxTTL, + family: addr.family + } + }) + + const records = results.reduce( + (acc, record) => { + if (record.family === 4) { + acc[4].ips.push({ address: record.address, ttl: record.ttl }) + } else { + acc[6].ips.push({ address: record.address, ttl: record.ttl }) + } + + return acc + }, + { 4: { ips: [] }, 6: { ips: [] } } + ) + + this.record.set(origin.hostname, { records }) + + callback(null, records) + }) + } + + #defaultPick (origin, hostnameRecords, affinity) { + const { records, offset = 0 } = this.record.get(origin.hostname) + let newOffet = 0 + + if (offset === maxInt) { + newOffet = 0 + } else { + newOffet = offset + 1 + } + + // We balance between the two IP families + const family = + records[affinity] ?? + records[(this.lastIpFamily = (newOffet & 1) === 1 ? 4 : 6)] + family.offset = family.offset ?? 0 + hostnameRecords.offset = newOffet + + if (family.offset === maxInt) { + family.offset = 0 + } else { + family.offset++ + } + + const ip = family.ips[family.offset % family.ips.length] + + if (ip.timestamp != null && Date.now() - ip.timestamp > ip.ttl * 1000) { + return this.pick(origin, hostnameRecords, affinity) + } + + ip.timestamp = Date.now() + + return ip + } + + /** + * TODO: re-evaluate + * + * So far it seems that this can be better offloaded to the handler + * especially if we want to handle situations where the request failed + * and we want to support try again on another IP family. + */ + + getHandler (handler) { + return new DNSDispatchHandler(this, handler) + } +} + +class DNSDispatchHandler extends DecoratorHandler { + #state = null + #opts = null + #dispatch = null + #handler = null + + constructor (state, { handler, dispatch }, opts) { + super(handler) + this.#handler = handler + this.#opts = { ...opts } + this.#state = state + this.#dispatch = dispatch + } + + onError (err) { + switch (err.code) { + case 'ETIMEDOUT': + case 'ECONNREFUSED': { + // Abstract into a method + if (this.#state.dualStack) { + const ips = this.#state.record.get(this.origin.hostname) + // If no IPs we lookup + if (ips == null) { + this.#state.lookup(this.origin, this.#opts, (err, addresses) => { + if (err) { + return this.#handler.onError(err) + } + + const ip = this.#state.pick( + this.origin, + addresses, + this.#state.lastIpFamily === 4 ? 6 : this.#state.lastIpFamily + ) + const opts = { ...this.#opts, origin: ip.address } + this.#dispatch(opts, this) + }) + } + + // If there's IPs we pick + const ip = this.#state.pick( + this.opts.origin, + this.#state.lastIpFamily === '4' ? '6' : this.#state.lastIpFamily + ) + + const opts = { ...this.#opts, origin: ip.address } + this.#dispatch(opts, this) + } + + return + } + case 'ENOTFOUND': + this.#state.record.delete(this.origin.hostname) + // eslint-disable-next-line no-fallthrough + default: + this.#handler.onError(err) + break + } + } +} + +module.exports = interceptorOpts => { + // TODO: verify opts + const opts = { + maxTTL: interceptorOpts?.maxTTL ?? 10, // Expressed in seconds + resolve: interceptorOpts?.resolve ?? null, + pick: interceptorOpts?.pick ?? null, + dualStack: interceptorOpts?.dualStack ?? true + } + const instance = new DNSInstance(opts) + + return dispatch => { + return function dnsInterceptor (origDispatchOpts, handler) { + const origin = + origDispatchOpts.origin.constructor === URL + ? origDispatchOpts.origin + : new URL(origDispatchOpts.origin) + + if (isIP(origin.hostname) !== 0) { + return dispatch(origDispatchOpts, handler) + } + + const ips = instance.record.get(origin.hostname) + + // If no IPs we lookup + if (ips == null) { + instance.lookup(origin, opts, (err, addresses) => { + if (err) { + return handler.onError(err) + } + + const ip = instance.pick( + origin, + addresses, + origDispatchOpts.dns?.affinity + ) + + const dispatchOpts = { + ...origDispatchOpts, + origin: `${origin.protocol}//${ip.address}${ + origin.port === '' ? '' : `:${origin.port}` + }` + } + + dispatch( + dispatchOpts, + instance.getHandler( + { dispatch, handler }, + { ...opts, ...origDispatchOpts } + ) + ) + }) + } else { + // If there's IPs we pick + const ip = instance.pick(origin, origDispatchOpts.dns?.affinity) + + const dispatchOpts = { + ...origDispatchOpts, + origin: `${origin.protocol}//${ip.address}${ + origin.port === '' ? '' : `:${origin.port}` + }` + } + + return dispatch( + dispatchOpts, + instance.getHandler( + { dispatch, handler }, + { ...opts, ...origDispatchOpts } + ) + ) + } + + // TODO: or shall we ask for room to breathe? + return true + } + } +} diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js new file mode 100644 index 00000000000..de870472ebf --- /dev/null +++ b/test/interceptors/dns.js @@ -0,0 +1,51 @@ +'use strict' + +const { tspl } = require('@matteo.collina/tspl') +const { test, after } = require('node:test') +const { createServer } = require('node:http') +const { once } = require('node:events') + +const { interceptors, Agent } = require('../..') +const { dns } = interceptors + +test('Should automatically resolve IPs', async t => { + t = tspl(t, { plan: 2 }) + + const server = createServer() + const requestOptions = { + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + } + + server.on('request', (req, res) => { + // t.equa(req.headers.host, ) + console.log(req.headers.host) + res.writeHead(200, { 'content-type': 'text/plain' }) + res.end('hello world!') + }) + + server.listen(0) + + await once(server, 'listening') + console.log(server.address()) + + const client = new Agent().compose(dns()) + + 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!') +}) From da09d0ee4ca61e1f8e105d928d038ec973f28dc6 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 28 Aug 2024 12:56:32 +0200 Subject: [PATCH 02/15] feat: add affinity support --- lib/interceptor/dns.js | 195 +++++++++++++--------- test/interceptors/dns.js | 342 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 459 insertions(+), 78 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index ccbf25da353..bda0510c8d8 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -6,8 +6,9 @@ const maxInt = Math.pow(2, 31) - 1 class DNSInstance { #maxTTL = 0 // TODO: support TTL + #records = new Map() dualStack = true - record = new Map() + affinity = null lookup = null pick = null lastIpFamily = null @@ -15,60 +16,65 @@ class DNSInstance { constructor (opts) { this.#maxTTL = opts.maxTTL this.dualStack = opts.dualStack + this.affinity = opts.affinity this.lookup = opts.lookup ?? this.#defaultLookup this.pick = opts.pick ?? this.#defaultPick } - #defaultLookup (origin, opts, callback) { - // TODO: replace for resolve - lookup(origin.hostname, { all: true }, (err, addresses) => { - if (err) { - return callback(err) - } - - const results = addresses.map(addr => { - return { - address: addr.address, - ttl: opts.maxTTL, - family: addr.family + #defaultLookup (origin, opts, cb) { + lookup( + origin.hostname, + { all: true, family: this.dualStack === false ? this.affinity : 0 }, + (err, addresses) => { + if (err) { + return cb(err) } - }) - const records = results.reduce( - (acc, record) => { - if (record.family === 4) { - acc[4].ips.push({ address: record.address, ttl: record.ttl }) - } else { - acc[6].ips.push({ address: record.address, ttl: record.ttl }) - } + const results = [] - return acc - }, - { 4: { ips: [] }, 6: { ips: [] } } - ) + for (const addr of addresses) { + const record = { + address: addr.address, + ttl: opts.maxTTL, + family: addr.family + } - this.record.set(origin.hostname, { records }) + results.push(record) + } - callback(null, records) - }) + cb(null, results) + } + ) } #defaultPick (origin, hostnameRecords, affinity) { - const { records, offset = 0 } = this.record.get(origin.hostname) - let newOffet = 0 + const { records, offset = 0 } = hostnameRecords + let newOffset = 0 if (offset === maxInt) { - newOffet = 0 + newOffset = 0 } else { - newOffet = offset + 1 + newOffset = offset + 1 } // We balance between the two IP families + // If dual-stack disabled, we automatically pick the affinity const family = - records[affinity] ?? - records[(this.lastIpFamily = (newOffet & 1) === 1 ? 4 : 6)] + this.dualStack === false + ? records[this.affinity] + : records[affinity] ?? + records[(this.lastIpFamily = (newOffset & 1) === 1 ? 4 : 6)] + + if (family == null) { + return this.pick( + origin, + hostnameRecords, + affinity ?? this.lastIpFamily === 4 ? 6 : 4 + ) + } + family.offset = family.offset ?? 0 - hostnameRecords.offset = newOffet + hostnameRecords.offset = newOffset if (family.offset === maxInt) { family.offset = 0 @@ -78,15 +84,33 @@ class DNSInstance { const ip = family.ips[family.offset % family.ips.length] - if (ip.timestamp != null && Date.now() - ip.timestamp > ip.ttl * 1000) { + const timestamp = Date.now() + if (ip.timestamp != null && timestamp - ip.timestamp > ip.ttl * 1000) { return this.pick(origin, hostnameRecords, affinity) } - ip.timestamp = Date.now() + ip.timestamp = timestamp return ip } + setRecords (origin, addresses) { + const records = { records: { 4: { ips: [] }, 6: { ips: [] } } } + for (const record of addresses) { + records.records[record.family].ips.push(record) + } + + this.#records.set(origin.hostname, records) + } + + getRecords (origin) { + return this.#records.get(origin.hostname) + } + + deleteRecord (origin) { + this.#records.delete(origin.hostname) + } + /** * TODO: re-evaluate * @@ -95,8 +119,8 @@ class DNSInstance { * and we want to support try again on another IP family. */ - getHandler (handler) { - return new DNSDispatchHandler(this, handler) + getHandler (meta, opts) { + return new DNSDispatchHandler(this, meta, opts) } } @@ -105,9 +129,11 @@ class DNSDispatchHandler extends DecoratorHandler { #opts = null #dispatch = null #handler = null + #origin = null - constructor (state, { handler, dispatch }, opts) { + constructor (state, { origin, handler, dispatch }, opts) { super(handler) + this.#origin = origin this.#handler = handler this.#opts = { ...opts } this.#state = state @@ -118,40 +144,57 @@ class DNSDispatchHandler extends DecoratorHandler { switch (err.code) { case 'ETIMEDOUT': case 'ECONNREFUSED': { - // Abstract into a method if (this.#state.dualStack) { - const ips = this.#state.record.get(this.origin.hostname) + const origin = this.#origin + const instance = this.#state + const ips = instance.getRecords(origin) + // If no IPs we lookup if (ips == null) { - this.#state.lookup(this.origin, this.#opts, (err, addresses) => { + instance.lookup(origin, this.#opts, (err, addresses) => { if (err) { return this.#handler.onError(err) } - const ip = this.#state.pick( - this.origin, - addresses, - this.#state.lastIpFamily === 4 ? 6 : this.#state.lastIpFamily + instance.setRecords(origin, addresses) + + const ip = instance.pick( + origin, + instance.getRecords(origin), + instance.lastIpFamily === 4 ? 6 : 4 ) - const opts = { ...this.#opts, origin: ip.address } + const opts = { + ...this.#opts, + origin: `${origin.protocol}//${ + ip.family === 6 ? `[${ip.address}]` : ip.address + }${origin.port === '' ? '' : `:${origin.port}`}` + } + this.#dispatch(opts, this) }) - } + } else { + // If there's IPs we pick + const ip = instance.pick( + origin, + instance.getRecords(origin), + instance.lastIpFamily === 4 ? 6 : 4 + ) - // If there's IPs we pick - const ip = this.#state.pick( - this.opts.origin, - this.#state.lastIpFamily === '4' ? '6' : this.#state.lastIpFamily - ) + const opts = { + ...this.#opts, + origin: `${origin.protocol}//${ + ip.family === 6 ? `[${ip.address}]` : ip.address + }${origin.port === '' ? '' : `:${origin.port}`}` + } - const opts = { ...this.#opts, origin: ip.address } - this.#dispatch(opts, this) + this.#dispatch(opts, this) + } + // if dual-stack disabled, we error out + return } - - return } case 'ENOTFOUND': - this.#state.record.delete(this.origin.hostname) + this.#state.deleteRecord(this.#origin) // eslint-disable-next-line no-fallthrough default: this.#handler.onError(err) @@ -163,10 +206,11 @@ class DNSDispatchHandler extends DecoratorHandler { module.exports = interceptorOpts => { // TODO: verify opts const opts = { - maxTTL: interceptorOpts?.maxTTL ?? 10, // Expressed in seconds + maxTTL: interceptorOpts?.maxTTL ?? 10e3, // Expressed in ms resolve: interceptorOpts?.resolve ?? null, pick: interceptorOpts?.pick ?? null, - dualStack: interceptorOpts?.dualStack ?? true + dualStack: interceptorOpts?.dualStack ?? true, + affinity: interceptorOpts?.affinity ?? 4 } const instance = new DNSInstance(opts) @@ -181,7 +225,7 @@ module.exports = interceptorOpts => { return dispatch(origDispatchOpts, handler) } - const ips = instance.record.get(origin.hostname) + const ips = instance.getRecords(origin) // If no IPs we lookup if (ips == null) { @@ -190,42 +234,49 @@ module.exports = interceptorOpts => { return handler.onError(err) } + instance.setRecords(origin, addresses) + const ip = instance.pick( origin, - addresses, + instance.getRecords(origin), origDispatchOpts.dns?.affinity ) const dispatchOpts = { ...origDispatchOpts, - origin: `${origin.protocol}//${ip.address}${ - origin.port === '' ? '' : `:${origin.port}` - }` + origin: `${origin.protocol}//${ + ip.family === 6 ? `[${ip.address}]` : ip.address + }${origin.port === '' ? '' : `:${origin.port}`}` } dispatch( dispatchOpts, instance.getHandler( - { dispatch, handler }, + { origin, dispatch, handler }, { ...opts, ...origDispatchOpts } ) ) }) } else { // If there's IPs we pick - const ip = instance.pick(origin, origDispatchOpts.dns?.affinity) + const records = instance.getRecords(origin) + const ip = instance.pick( + origin, + records, + origDispatchOpts.dns?.affinity + ) const dispatchOpts = { ...origDispatchOpts, - origin: `${origin.protocol}//${ip.address}${ - origin.port === '' ? '' : `:${origin.port}` - }` + origin: `${origin.protocol}//${ + ip.family === 6 ? `[${ip.address}]` : ip.address + }${origin.port === '' ? '' : `:${origin.port}`}` } return dispatch( dispatchOpts, instance.getHandler( - { dispatch, handler }, + { origin, dispatch, handler }, { ...opts, ...origDispatchOpts } ) ) diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index de870472ebf..b4123222322 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -2,15 +2,17 @@ const { tspl } = require('@matteo.collina/tspl') const { test, after } = require('node:test') +const { isIP } = require('node:net') const { createServer } = require('node:http') const { once } = require('node:events') const { interceptors, Agent } = require('../..') const { dns } = interceptors -test('Should automatically resolve IPs', async t => { - t = tspl(t, { plan: 2 }) +test('Should automatically resolve IPs (dual stack)', async t => { + t = tspl(t, { plan: 6 }) + let counter = 0 const server = createServer() const requestOptions = { method: 'GET', @@ -21,8 +23,6 @@ test('Should automatically resolve IPs', async t => { } server.on('request', (req, res) => { - // t.equa(req.headers.host, ) - console.log(req.headers.host) res.writeHead(200, { 'content-type': 'text/plain' }) res.end('hello world!') }) @@ -30,9 +30,31 @@ test('Should automatically resolve IPs', async t => { server.listen(0) await once(server, 'listening') - console.log(server.address()) - const client = new Agent().compose(dns()) + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + t.equal(isIP(url.hostname), 4) + break + + case 2: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns() + ]) after(async () => { await client.close() @@ -48,4 +70,312 @@ test('Should automatically resolve IPs', async t => { t.equal(response.statusCode, 200) t.equal(await response.body.text(), 'hello world!') + + 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!') +}) + +test('Should recover on network errors (dual stack - 4)', async t => { + t = tspl(t, { plan: 8 }) + + let counter = 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, '::1') + + await once(server, 'listening') + + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + t.equal(isIP(url.hostname), 4) + break + + case 2: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + + case 3: + // [::1] -> ::1 + t.equal(isIP(url.hostname), 4) + break + + case 4: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns() + ]) + + 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!') + + 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!') +}) + +test('Should recover on network errors (dual stack - 6)', async t => { + t = tspl(t, { plan: 7 }) + + let counter = 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([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + t.equal(isIP(url.hostname), 4) + break + + case 2: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + + case 3: + // [::1] -> ::1 + t.equal(isIP(url.hostname), 4) + break + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns() + ]) + + 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!') + + 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!') +}) + +test('Should automatically resolve IPs (dual stack disabled - 4)', async t => { + t = tspl(t, { plan: 6 }) + + let counter = 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) + + await once(server, 'listening') + + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + t.equal(isIP(url.hostname), 4) + break + + case 2: + // [::1] -> ::1 + t.equal(isIP(url.hostname), 4) + break + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns({ dualStack: false }) + ]) + + 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!') + + 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!') +}) + +test('Should automatically resolve IPs (dual stack disabled - 6)', async t => { + t = tspl(t, { plan: 6 }) + + let counter = 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) + + await once(server, 'listening') + + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + + case 2: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns({ dualStack: false, affinity: 6 }) + ]) + + 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!') + + 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!') }) From dd2d0938ba4c583e12f524497dba29413b52c54d Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 29 Aug 2024 08:38:28 +0200 Subject: [PATCH 03/15] fix: lint --- lib/interceptor/dns.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index bda0510c8d8..34bce81f6ce 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -193,6 +193,7 @@ class DNSDispatchHandler extends DecoratorHandler { return } } + // eslint-disable-next-line no-fallthrough case 'ENOTFOUND': this.#state.deleteRecord(this.#origin) // eslint-disable-next-line no-fallthrough From 65ad987560e78601267d2250252ce4851eeba1f8 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 30 Aug 2024 18:50:10 +0200 Subject: [PATCH 04/15] refactor: cleanup --- lib/interceptor/dns.js | 20 ++++----- test/interceptors/dns.js | 87 ++++++++++++++++++++++++++++++++++++++++ types/interceptors.d.ts | 1 + 3 files changed, 96 insertions(+), 12 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index 34bce81f6ce..58416d56d80 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -2,6 +2,7 @@ const { isIP } = require('node:net') const { lookup } = require('node:dns') const DecoratorHandler = require('../handler/decorator-handler') +const { InvalidArgumentError } = require('../core/errors') const maxInt = Math.pow(2, 31) - 1 class DNSInstance { @@ -95,9 +96,12 @@ class DNSInstance { } setRecords (origin, addresses) { - const records = { records: { 4: { ips: [] }, 6: { ips: [] } } } + const records = { records: { 4: null, 6: null } } for (const record of addresses) { - records.records[record.family].ips.push(record) + const familyRecords = records.records[record.family] ?? { ips: [] } + + familyRecords.ips.push(record) + records.records[record.family] = familyRecords } this.#records.set(origin.hostname, records) @@ -111,14 +115,6 @@ class DNSInstance { this.#records.delete(origin.hostname) } - /** - * TODO: re-evaluate - * - * So far it seems that this can be better offloaded to the handler - * especially if we want to handle situations where the request failed - * and we want to support try again on another IP family. - */ - getHandler (meta, opts) { return new DNSDispatchHandler(this, meta, opts) } @@ -208,11 +204,12 @@ module.exports = interceptorOpts => { // TODO: verify opts const opts = { maxTTL: interceptorOpts?.maxTTL ?? 10e3, // Expressed in ms - resolve: interceptorOpts?.resolve ?? null, + lookup: interceptorOpts?.lookup ?? null, pick: interceptorOpts?.pick ?? null, dualStack: interceptorOpts?.dualStack ?? true, affinity: interceptorOpts?.affinity ?? 4 } + const instance = new DNSInstance(opts) return dispatch => { @@ -283,7 +280,6 @@ module.exports = interceptorOpts => { ) } - // TODO: or shall we ask for room to breathe? return true } } diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index b4123222322..521d8366366 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -237,6 +237,93 @@ test('Should recover on network errors (dual stack - 6)', async t => { t.equal(await response2.body.text(), 'hello world!') }) +test('Should throw when on dual-stack disabled (4)', async t => { + t = tspl(t, { plan: 2 }) + + let counter = 0 + const requestOptions = { + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + } + + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + t.equal(isIP(url.hostname), 4) + break + + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns({ dualStack: false, affinity: 4 }) + ]) + + const promise = client.request({ + ...requestOptions, + origin: 'http://localhost' + }) + + await t.rejects(promise, 'ECONNREFUSED') + + await t.complete +}) + +test('Should throw when on dual-stack disabled (6)', async t => { + t = tspl(t, { plan: 2 }) + + let counter = 0 + const requestOptions = { + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + } + + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns({ dualStack: false, affinity: 6 }) + ]) + + const promise = client.request({ + ...requestOptions, + origin: 'http://localhost' + }) + + await t.rejects(promise, 'ECONNREFUSED') + + await t.complete +}) + test('Should automatically resolve IPs (dual stack disabled - 4)', async t => { t = tspl(t, { plan: 6 }) diff --git a/types/interceptors.d.ts b/types/interceptors.d.ts index 53835e01299..12e16c5e8dd 100644 --- a/types/interceptors.d.ts +++ b/types/interceptors.d.ts @@ -1,5 +1,6 @@ import Dispatcher from './dispatcher' import RetryHandler from './retry-handler' +import { LookupOptions } from 'node:dns' export default Interceptors From 77387d7f2ef9af39e540aff69e75b5e95f345693 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 30 Aug 2024 18:50:33 +0200 Subject: [PATCH 05/15] feat: implement validation --- lib/interceptor/dns.js | 39 ++++++++++++++++++++++++++++++++++++++- test/interceptors/dns.js | 14 ++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index 58416d56d80..3ce57dc4498 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -201,7 +201,44 @@ class DNSDispatchHandler extends DecoratorHandler { } module.exports = interceptorOpts => { - // TODO: verify opts + if ( + interceptorOpts?.maxTTL != null && + (typeof interceptorOpts?.maxTTL !== 'number' || + !Number.isFinite(interceptorOpts?.maxTTL) || + interceptorOpts?.maxTTL < 0) + ) { + throw new InvalidArgumentError('Invalid maxTTL. Must be a positive number') + } + + if ( + interceptorOpts?.affinity != null && + interceptorOpts?.affinity !== 4 && + interceptorOpts?.affinity !== 6 + ) { + throw new InvalidArgumentError('Invalid affinity. Must be either 4 or 6') + } + + if ( + interceptorOpts?.dualStack != null && + typeof interceptorOpts?.dualStack !== 'boolean' + ) { + throw new InvalidArgumentError('Invalid dualStack. Must be a boolean') + } + + if ( + interceptorOpts?.lookup != null && + typeof interceptorOpts?.lookup !== 'function' + ) { + throw new InvalidArgumentError('Invalid lookup. Must be a function') + } + + if ( + interceptorOpts?.pick != null && + typeof interceptorOpts?.pick !== 'function' + ) { + throw new InvalidArgumentError('Invalid pick. Must be a function') + } + const opts = { maxTTL: interceptorOpts?.maxTTL ?? 10e3, // Expressed in ms lookup: interceptorOpts?.lookup ?? null, diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index 521d8366366..ba460c1ae26 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -9,6 +9,20 @@ const { once } = require('node:events') const { interceptors, Agent } = require('../..') const { dns } = interceptors +test('Should validate options', t => { + t = tspl(t, { plan: 9 }) + + t.throws(() => dns({ dualStack: 'true' }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ dualStack: 0 }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ affinity: '4' }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ affinity: 7 }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ maxTTL: Infinity }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ maxTTL: -1 }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ maxTTL: '0' }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ lookup: {} }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ pick: [] }), { code: 'UND_ERR_INVALID_ARG' }) +}) + test('Should automatically resolve IPs (dual stack)', async t => { t = tspl(t, { plan: 6 }) From e0ee8db99201d2b1a16004fa03208e39d2301e6b Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 30 Aug 2024 18:50:42 +0200 Subject: [PATCH 06/15] feat: add ts types --- types/interceptors.d.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/types/interceptors.d.ts b/types/interceptors.d.ts index 12e16c5e8dd..08c57bfc089 100644 --- a/types/interceptors.d.ts +++ b/types/interceptors.d.ts @@ -8,11 +8,23 @@ declare namespace Interceptors { export type DumpInterceptorOpts = { maxSize?: number } export type RetryInterceptorOpts = RetryHandler.RetryOptions export type RedirectInterceptorOpts = { maxRedirections?: number } + export type ResponseErrorInterceptorOpts = { throwOnError: boolean } + // DNS interceptor + export type DNSInterceptorRecord = { address: string, ttl: number, family: 4 | 6 } + export type DNSInterceptorRecords = { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null } + export type DNSInterceptorOpts = { + maxTTL?: number + lookup?: (hostname: string, options: LookupOptions, callback: (err: NodeJS.ErrnoException | null, addresses: DNSInterceptorRecord[]) => void) => void + pick?: (origin: URL, records: DNSInterceptorRecords, affinity: 4 | 6) => DNSInterceptorRecord + dualStack?: boolean + affinity?: 4 | 6 + } export function createRedirectInterceptor (opts: RedirectInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function dump (opts?: DumpInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function retry (opts?: RetryInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function redirect (opts?: RedirectInterceptorOpts): Dispatcher.DispatcherComposeInterceptor export function responseError (opts?: ResponseErrorInterceptorOpts): Dispatcher.DispatcherComposeInterceptor + export function dns (opts?: DNSInterceptorOpts): Dispatcher.DispatcherComposeInterceptor } From 52f6c346d450f8277b309b33302fdef4769a042e Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Sun, 8 Sep 2024 14:32:58 +0200 Subject: [PATCH 07/15] feat: add support for TTL --- lib/interceptor/dns.js | 226 +++++++++++++++++++++------------------ test/interceptors/dns.js | 215 ++++++++++++++++++++++++++++++++++++- 2 files changed, 333 insertions(+), 108 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index 3ce57dc4498..80e8bf19332 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -2,11 +2,11 @@ const { isIP } = require('node:net') const { lookup } = require('node:dns') const DecoratorHandler = require('../handler/decorator-handler') -const { InvalidArgumentError } = require('../core/errors') +const { InvalidArgumentError, InformationalError } = require('../core/errors') const maxInt = Math.pow(2, 31) - 1 class DNSInstance { - #maxTTL = 0 // TODO: support TTL + #maxTTL = 0 #records = new Map() dualStack = true affinity = null @@ -22,6 +22,70 @@ class DNSInstance { this.pick = opts.pick ?? this.#defaultPick } + runLookup (origin, opts, cb) { + const ips = this.#records.get(origin.hostname) + const newOpts = { + affinity: this.affinity, + dualStack: this.dualStack, + lookup: this.lookup, + pick: this.pick, + ...opts.dns, + maxTTL: this.#maxTTL + } + + // If no IPs we lookup + if (ips == null) { + this.lookup(origin, newOpts, (err, addresses) => { + if (err || addresses == null || addresses.length === 0) { + cb(err ?? new InformationalError('No DNS entries found')) + return + } + + this.setRecords(origin, addresses) + const records = this.#records.get(origin.hostname) + + const ip = this.pick( + origin, + records, + // Only set affinity if dual stack is disabled + // otherwise let it go through normal flow + !newOpts.dualStack && newOpts.affinity + ) + + return cb( + null, + `${origin.protocol}//${ + ip.family === 6 ? `[${ip.address}]` : ip.address + }${origin.port === '' ? '' : `:${origin.port}`}` + ) + }) + } else { + // If there's IPs we pick + const records = this.#records.get(origin.hostname) + const ip = this.pick( + origin, + records, + // Only set affinity if dual stack is disabled + // otherwise let it go through normal flow + !newOpts.dualStack && newOpts.affinity + ) + + // If no IPs we lookup - deleting old records + if (ip == null) { + this.#records.delete(origin.hostname) + this.runLookup(origin, opts, cb) + return + } + + cb( + null, + `${origin.protocol}//${ + ip.family === 6 ? `[${ip.address}]` : ip.address + }${origin.port === '' ? '' : `:${origin.port}`}` + ) + } + } + #defaultLookup (origin, opts, cb) { lookup( origin.hostname, @@ -36,7 +100,7 @@ class DNSInstance { for (const addr of addresses) { const record = { address: addr.address, - ttl: opts.maxTTL, + ttl: addr.ttl ? addr.ttl * 1000 : this.#maxTTL, family: addr.family } @@ -60,13 +124,18 @@ class DNSInstance { // We balance between the two IP families // If dual-stack disabled, we automatically pick the affinity + const newIpFamily = (newOffset & 1) === 1 ? 4 : 6 const family = this.dualStack === false ? records[this.affinity] - : records[affinity] ?? - records[(this.lastIpFamily = (newOffset & 1) === 1 ? 4 : 6)] - - if (family == null) { + : records[affinity] ?? records[newIpFamily] + + // If no IPs and we already tried both families, we return null + if ( + family == null && + // eslint-disable-next-line eqeqeq + (this.dualStack === false || this.lastIpFamily != newIpFamily) + ) { return this.pick( origin, hostnameRecords, @@ -74,6 +143,15 @@ class DNSInstance { ) } + // If no IPs and we have tried both families, we return null + if ( + family.ips.length === 0 && + // eslint-disable-next-line eqeqeq + (this.dualStack === false || this.lastIpFamily != newIpFamily) + ) { + return null + } + family.offset = family.offset ?? 0 hostnameRecords.offset = newOffset @@ -83,15 +161,25 @@ class DNSInstance { family.offset++ } - const ip = family.ips[family.offset % family.ips.length] + const position = family.offset % family.ips.length + const ip = family.ips[position] + + if (ip == null) { + return null + } const timestamp = Date.now() - if (ip.timestamp != null && timestamp - ip.timestamp > ip.ttl * 1000) { + // Record TTL is already in ms + if (ip.timestamp != null && timestamp - ip.timestamp > ip.ttl) { + // 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 } @@ -107,14 +195,6 @@ class DNSInstance { this.#records.set(origin.hostname, records) } - getRecords (origin) { - return this.#records.get(origin.hostname) - } - - deleteRecord (origin) { - this.#records.delete(origin.hostname) - } - getHandler (meta, opts) { return new DNSDispatchHandler(this, meta, opts) } @@ -141,53 +221,26 @@ class DNSDispatchHandler extends DecoratorHandler { case 'ETIMEDOUT': case 'ECONNREFUSED': { if (this.#state.dualStack) { - const origin = this.#origin - const instance = this.#state - const ips = instance.getRecords(origin) - - // If no IPs we lookup - if (ips == null) { - instance.lookup(origin, this.#opts, (err, addresses) => { - if (err) { - return this.#handler.onError(err) - } - - instance.setRecords(origin, addresses) - - const ip = instance.pick( - origin, - instance.getRecords(origin), - instance.lastIpFamily === 4 ? 6 : 4 - ) - const opts = { - ...this.#opts, - origin: `${origin.protocol}//${ - ip.family === 6 ? `[${ip.address}]` : ip.address - }${origin.port === '' ? '' : `:${origin.port}`}` - } - - this.#dispatch(opts, this) - }) - } else { - // If there's IPs we pick - const ip = instance.pick( - origin, - instance.getRecords(origin), - instance.lastIpFamily === 4 ? 6 : 4 - ) - - const opts = { + // We delete the record and retry + this.#state.runLookup(this.#origin, this.#opts, (err, newOrigin) => { + if (err) { + return this.#handler.onError(err) + } + + const dispatchOpts = { ...this.#opts, - origin: `${origin.protocol}//${ - ip.family === 6 ? `[${ip.address}]` : ip.address - }${origin.port === '' ? '' : `:${origin.port}`}` + origin: newOrigin } - this.#dispatch(opts, this) - } + this.#dispatch(dispatchOpts, this) + }) + // if dual-stack disabled, we error out return } + + this.#handler.onError(err) + return } // eslint-disable-next-line no-fallthrough case 'ENOTFOUND': @@ -260,62 +313,21 @@ module.exports = interceptorOpts => { return dispatch(origDispatchOpts, handler) } - const ips = instance.getRecords(origin) - - // If no IPs we lookup - if (ips == null) { - instance.lookup(origin, opts, (err, addresses) => { - if (err) { - return handler.onError(err) - } - - instance.setRecords(origin, addresses) - - const ip = instance.pick( - origin, - instance.getRecords(origin), - origDispatchOpts.dns?.affinity - ) - - const dispatchOpts = { - ...origDispatchOpts, - origin: `${origin.protocol}//${ - ip.family === 6 ? `[${ip.address}]` : ip.address - }${origin.port === '' ? '' : `:${origin.port}`}` - } - - dispatch( - dispatchOpts, - instance.getHandler( - { origin, dispatch, handler }, - { ...opts, ...origDispatchOpts } - ) - ) - }) - } else { - // If there's IPs we pick - const records = instance.getRecords(origin) - const ip = instance.pick( - origin, - records, - origDispatchOpts.dns?.affinity - ) + instance.runLookup(origin, origDispatchOpts, (err, newOrigin) => { + if (err) { + return handler.onError(err) + } const dispatchOpts = { ...origDispatchOpts, - origin: `${origin.protocol}//${ - ip.family === 6 ? `[${ip.address}]` : ip.address - }${origin.port === '' ? '' : `:${origin.port}`}` + origin: newOrigin } - return dispatch( + dispatch( dispatchOpts, - instance.getHandler( - { origin, dispatch, handler }, - { ...opts, ...origDispatchOpts } - ) + instance.getHandler({ origin, dispatch, handler }, origDispatchOpts) ) - } + }) return true } diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index ba460c1ae26..9cd69c037fb 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -1,10 +1,13 @@ 'use strict' -const { tspl } = require('@matteo.collina/tspl') const { test, after } = require('node:test') const { isIP } = require('node:net') +const { lookup } = require('node:dns') const { createServer } = require('node:http') const { once } = require('node:events') +const { setTimeout: sleep } = require('node:timers/promises') + +const { tspl } = require('@matteo.collina/tspl') const { interceptors, Agent } = require('../..') const { dns } = interceptors @@ -480,3 +483,213 @@ test('Should automatically resolve IPs (dual stack disabled - 6)', async t => { t.equal(response2.statusCode, 200) t.equal(await response2.body.text(), 'hello world!') }) + +test('Should we handle TTL (4)', async t => { + t = tspl(t, { plan: 7 }) + + let counter = 0 + 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([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + t.equal(isIP(url.hostname), 4) + break + + case 2: + t.equal(isIP(url.hostname), 4) + break + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns({ + dualStack: false, + affinity: 4, + maxTTL: 100, + 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) + } + ) + } + }) + ]) + + 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(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) +}) + +test('Should we handle TTL (6)', async t => { + t = tspl(t, { plan: 7 }) + + let counter = 0 + 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, '::1') + + await once(server, 'listening') + + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + + case 2: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + default: + t.fail('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns({ + dualStack: false, + affinity: 6, + maxTTL: 100, + 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) + } + ) + } + }) + ]) + + 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(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) +}) From 710ee5521db48de4303398a4d68d45941ecfa888 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 11 Sep 2024 11:36:12 +0200 Subject: [PATCH 08/15] feat: support maxItems --- lib/interceptor/dns.js | 34 +++++++++++--- test/interceptors/dns.js | 98 +++++++++++++++++++++++++++++++++++++++- types/interceptors.d.ts | 1 + 3 files changed, 125 insertions(+), 8 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index 80e8bf19332..10e7d70da7e 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -7,6 +7,7 @@ const maxInt = Math.pow(2, 31) - 1 class DNSInstance { #maxTTL = 0 + #maxItems = 0 #records = new Map() dualStack = true affinity = null @@ -16,21 +17,34 @@ class DNSInstance { constructor (opts) { this.#maxTTL = opts.maxTTL + this.#maxItems = opts.maxItems this.dualStack = opts.dualStack this.affinity = opts.affinity this.lookup = opts.lookup ?? this.#defaultLookup this.pick = opts.pick ?? this.#defaultPick } + get full () { + return this.#records.size === this.#maxItems + } + runLookup (origin, opts, cb) { const ips = this.#records.get(origin.hostname) + + // If full, we just return the origin + if (ips == null && this.full) { + cb(null, origin.origin) + return + } + const newOpts = { affinity: this.affinity, dualStack: this.dualStack, lookup: this.lookup, pick: this.pick, ...opts.dns, - maxTTL: this.#maxTTL + maxTTL: this.#maxTTL, + maxItems: this.#maxItems } // If no IPs we lookup @@ -242,7 +256,6 @@ class DNSDispatchHandler extends DecoratorHandler { this.#handler.onError(err) return } - // eslint-disable-next-line no-fallthrough case 'ENOTFOUND': this.#state.deleteRecord(this.#origin) // eslint-disable-next-line no-fallthrough @@ -256,13 +269,21 @@ class DNSDispatchHandler extends DecoratorHandler { module.exports = interceptorOpts => { if ( interceptorOpts?.maxTTL != null && - (typeof interceptorOpts?.maxTTL !== 'number' || - !Number.isFinite(interceptorOpts?.maxTTL) || - interceptorOpts?.maxTTL < 0) + (typeof interceptorOpts?.maxTTL !== 'number' || interceptorOpts?.maxTTL < 0) ) { throw new InvalidArgumentError('Invalid maxTTL. Must be a positive number') } + if ( + interceptorOpts?.maxItems != null && + (typeof interceptorOpts?.maxItems !== 'number' || + interceptorOpts?.maxItems < 1) + ) { + throw new InvalidArgumentError( + 'Invalid maxItems. Must be a positive number' + ) + } + if ( interceptorOpts?.affinity != null && interceptorOpts?.affinity !== 4 && @@ -297,7 +318,8 @@ module.exports = interceptorOpts => { lookup: interceptorOpts?.lookup ?? null, pick: interceptorOpts?.pick ?? null, dualStack: interceptorOpts?.dualStack ?? true, - affinity: interceptorOpts?.affinity ?? 4 + affinity: interceptorOpts?.affinity ?? 4, + maxItems: interceptorOpts?.maxItems ?? Infinity } const instance = new DNSInstance(opts) diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index 9cd69c037fb..0b148a29df4 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -13,15 +13,16 @@ const { interceptors, Agent } = require('../..') const { dns } = interceptors test('Should validate options', t => { - t = tspl(t, { plan: 9 }) + t = tspl(t, { plan: 10 }) t.throws(() => dns({ dualStack: 'true' }), { code: 'UND_ERR_INVALID_ARG' }) t.throws(() => dns({ dualStack: 0 }), { code: 'UND_ERR_INVALID_ARG' }) t.throws(() => dns({ affinity: '4' }), { code: 'UND_ERR_INVALID_ARG' }) t.throws(() => dns({ affinity: 7 }), { code: 'UND_ERR_INVALID_ARG' }) - t.throws(() => dns({ maxTTL: Infinity }), { code: 'UND_ERR_INVALID_ARG' }) t.throws(() => dns({ maxTTL: -1 }), { code: 'UND_ERR_INVALID_ARG' }) t.throws(() => dns({ maxTTL: '0' }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ maxItems: '1' }), { code: 'UND_ERR_INVALID_ARG' }) + t.throws(() => dns({ maxItems: -1 }), { code: 'UND_ERR_INVALID_ARG' }) t.throws(() => dns({ lookup: {} }), { code: 'UND_ERR_INVALID_ARG' }) t.throws(() => dns({ pick: [] }), { code: 'UND_ERR_INVALID_ARG' }) }) @@ -693,3 +694,96 @@ test('Should we handle TTL (6)', async t => { t.equal(await response2.body.text(), 'hello world!') t.equal(lookupCounter, 2) }) + +test('Should handle max cached items', async t => { + t = tspl(t, { plan: 9 }) + + let counter = 0 + const server1 = createServer() + const server2 = createServer() + const requestOptions = { + method: 'GET', + path: '/', + headers: { + 'content-type': 'application/json' + } + } + + server1.on('request', (req, res) => { + res.writeHead(200, { 'content-type': 'text/plain' }) + res.end('hello world!') + }) + + server1.listen(0) + + server2.on('request', (req, res) => { + res.writeHead(200, { 'content-type': 'text/plain' }) + res.end('hello world! (x2)') + }) + server2.listen(0) + + await Promise.all([once(server1, 'listening'), once(server2, 'listening')]) + + const client = new Agent().compose([ + dispatch => { + return (opts, handler) => { + ++counter + const url = new URL(opts.origin) + + switch (counter) { + case 1: + t.equal(isIP(url.hostname), 4) + break + + case 2: + // [::1] -> ::1 + t.equal(isIP(url.hostname.slice(1, 4)), 6) + break + + case 3: + t.equal(url.hostname, 'developer.mozilla.org') + // Rewrite origin to avoid reaching internet + opts.origin = `http://127.0.0.1:${server2.address().port}` + break + default: + t.fails('should not reach this point') + } + + return dispatch(opts, handler) + } + }, + dns({ maxItems: 1 }) + ]) + + after(async () => { + await client.close() + server1.close() + server2.close() + + await Promise.all([once(server1, 'close'), once(server2, 'close')]) + }) + + const response = await client.request({ + ...requestOptions, + origin: `http://localhost:${server1.address().port}` + }) + + t.equal(response.statusCode, 200) + t.equal(await response.body.text(), 'hello world!') + + const response2 = await client.request({ + ...requestOptions, + origin: `http://localhost:${server1.address().port}` + }) + + t.equal(response2.statusCode, 200) + t.equal(await response2.body.text(), 'hello world!') + + const response3 = await client.request({ + ...requestOptions, + origin: 'https://developer.mozilla.org' + }) + + t.equal(response3.statusCode, 200) + t.equal(await response3.body.text(), 'hello world! (x2)') +}) diff --git a/types/interceptors.d.ts b/types/interceptors.d.ts index 08c57bfc089..d5968354694 100644 --- a/types/interceptors.d.ts +++ b/types/interceptors.d.ts @@ -15,6 +15,7 @@ declare namespace Interceptors { export type DNSInterceptorRecords = { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null } export type DNSInterceptorOpts = { maxTTL?: number + maxItems?: number lookup?: (hostname: string, options: LookupOptions, callback: (err: NodeJS.ErrnoException | null, addresses: DNSInterceptorRecord[]) => void) => void pick?: (origin: URL, records: DNSInterceptorRecords, affinity: 4 | 6) => DNSInterceptorRecord dualStack?: boolean From 0132ff4d5aae7fe47270305f24b0347bbb7b3107 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 11 Sep 2024 12:15:06 +0200 Subject: [PATCH 09/15] refactor: Update lib/interceptor/dns.js --- lib/interceptor/dns.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index 10e7d70da7e..78ba29d5fae 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -280,7 +280,7 @@ module.exports = interceptorOpts => { interceptorOpts?.maxItems < 1) ) { throw new InvalidArgumentError( - 'Invalid maxItems. Must be a positive number' + 'Invalid maxItems. Must be a positive number and greater than zero' ) } From 9e79c70062b840bd929c03cbda519bc7b3e46fd9 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 11 Sep 2024 12:31:00 +0200 Subject: [PATCH 10/15] refactor: small adjustments --- lib/interceptor/dns.js | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index 78ba29d5fae..f1caeef3001 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -114,7 +114,10 @@ class DNSInstance { for (const addr of addresses) { const record = { address: addr.address, - ttl: addr.ttl ? addr.ttl * 1000 : this.#maxTTL, + ttl: + addr.ttl != null + ? Math.min(addr.ttl * 1000, this.#maxTTL) + : this.#maxTTL, family: addr.family } @@ -127,6 +130,7 @@ class DNSInstance { } #defaultPick (origin, hostnameRecords, affinity) { + let ip = null const { records, offset = 0 } = hostnameRecords let newOffset = 0 @@ -141,29 +145,16 @@ class DNSInstance { const newIpFamily = (newOffset & 1) === 1 ? 4 : 6 const family = this.dualStack === false - ? records[this.affinity] + ? records[this.affinity] // If dual-stack is disabled, we pick the default affiniy : records[affinity] ?? records[newIpFamily] - // If no IPs and we already tried both families, we return null + // If no IPs and we have tried both families or dual stack is disabled, we return null if ( - family == null && + (family == null || family.ips.length === 0) && // eslint-disable-next-line eqeqeq (this.dualStack === false || this.lastIpFamily != newIpFamily) ) { - return this.pick( - origin, - hostnameRecords, - affinity ?? this.lastIpFamily === 4 ? 6 : 4 - ) - } - - // If no IPs and we have tried both families, we return null - if ( - family.ips.length === 0 && - // eslint-disable-next-line eqeqeq - (this.dualStack === false || this.lastIpFamily != newIpFamily) - ) { - return null + return ip } family.offset = family.offset ?? 0 @@ -176,10 +167,10 @@ class DNSInstance { } const position = family.offset % family.ips.length - const ip = family.ips[position] + ip = family.ips[position] ?? null if (ip == null) { - return null + return ip } const timestamp = Date.now() From 9b01a0985ccb8f6fc0d623ff4b2b3f95484c1493 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 11 Sep 2024 12:33:47 +0200 Subject: [PATCH 11/15] fix: tests --- test/interceptors/dns.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index 0b148a29df4..a63cf5f4c13 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -290,7 +290,7 @@ test('Should throw when on dual-stack disabled (4)', async t => { const promise = client.request({ ...requestOptions, - origin: 'http://localhost' + origin: 'http://localhost:1234' }) await t.rejects(promise, 'ECONNREFUSED') @@ -577,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(200) + await sleep(500) const response2 = await client.request({ ...requestOptions, From 763a2830e3cea310d897ffa8eb744cf41bb1c64c Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 11 Sep 2024 12:50:07 +0200 Subject: [PATCH 12/15] docs: add documentation --- docs/docs/api/Dispatcher.md | 51 +++++++++++++++++++++++++++++++++++++ types/interceptors.d.ts | 4 +-- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/docs/docs/api/Dispatcher.md b/docs/docs/api/Dispatcher.md index 933f4a730f8..c44abf5231b 100644 --- a/docs/docs/api/Dispatcher.md +++ b/docs/docs/api/Dispatcher.md @@ -985,6 +985,57 @@ client.dispatch( ); ``` +##### `dns` + +The `dns` interceptor enables you to cache DNS lookups for a given duration, per origin. + +>It is well suited for scenarios where you want to cache DNS lookups to avoid the overhead of resolving the same domain multiple times + +**Options** +- `maxTTL` - The maximum time-to-live (in milliseconds) of the DNS cache. It should be a positive integer. Default: `10000`. + - Set `0` to disable TTL. +- `maxItems` - The maximum number of items to cache. It should be a positive integer. Default: `Infinity`. +- `dualStack` - Whether to resolve both IPv4 and IPv6 addresses. Default: `true`. + - It will also attempt a happy-eyeballs-like approach to connect to the available addresses in case of a connection failure. +- `affinity` - Whether to use IPv4 or IPv6 addresses. Default: `4`. + - It can be either `'4` or `6`. + - It will only take effect if `dualStack` is `false`. +- `lookup: (hostname: string, options: LookupOptions, callback: (err: NodeJS.ErrnoException | null, addresses: DNSInterceptorRecord[]) => void) => void` - Custom lookup function. Default: `dns.lookup`. + - For more info see [dns.lookup](https://nodejs.org/api/dns.html#dns_dns_lookup_hostname_options_callback). +- `pick: (origin: URL, records: DNSInterceptorRecords, affinity: 4 | 6) => DNSInterceptorRecord` - Custom pick function. Default: `RoundRobin`. + - The function should return a single record from the records array. + - By default a simplified version of Round Robin is used. + - The `records` property can be mutated to store the state of the balancing algorithm. + +> The `Dispatcher#options` also gets extended with the options `dns.affinity`, `dns.dualStack`, `dns.lookup` and `dns.pick` which can be used to configure the interceptor at a request-per-request basis. + + +**DNSInterceptorRecord** +It represents a DNS record. +- `family` - (`number`) The IP family of the address. It can be either `4` or `6`. +- `address` - (`string`) The IP address. + +**DNSInterceptorOriginRecords** +It represents a map of DNS IP addresses records for a single origin. +- `4.ips` - (`DNSInterceptorRecord[] | null`) The IPv4 addresses. +- `6.ips` - (`DNSInterceptorRecord[] | null`) The IPv6 addresses. + +**Example - Basic DNS Interceptor** + +```js +const { Client, interceptors } = require("undici"); +const { dns } = interceptors; + +const client = new Agent().compose([ + dns({ ...opts }) +]) + +const response = await client.request({ + origin: `http://localhost:3030`, + ...requestOpts +}) +``` + ##### `Response Error Interceptor` **Introduction** diff --git a/types/interceptors.d.ts b/types/interceptors.d.ts index d5968354694..6fc50fb8dc1 100644 --- a/types/interceptors.d.ts +++ b/types/interceptors.d.ts @@ -12,12 +12,12 @@ declare namespace Interceptors { export type ResponseErrorInterceptorOpts = { throwOnError: boolean } // DNS interceptor export type DNSInterceptorRecord = { address: string, ttl: number, family: 4 | 6 } - export type DNSInterceptorRecords = { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null } + export type DNSInterceptorOriginRecords = { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null } export type DNSInterceptorOpts = { maxTTL?: number maxItems?: number lookup?: (hostname: string, options: LookupOptions, callback: (err: NodeJS.ErrnoException | null, addresses: DNSInterceptorRecord[]) => void) => void - pick?: (origin: URL, records: DNSInterceptorRecords, affinity: 4 | 6) => DNSInterceptorRecord + pick?: (origin: URL, records: DNSInterceptorOriginRecords, affinity: 4 | 6) => DNSInterceptorRecord dualStack?: boolean affinity?: 4 | 6 } From 454c1f0233e46d9f4e4a60d41d9e5f5ebc5ddcda Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 13 Sep 2024 10:55:01 +0200 Subject: [PATCH 13/15] refactor: remove duplicates --- lib/interceptor/dns.js | 18 ++++++++---------- test/interceptors/dns.js | 6 +++--- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/lib/interceptor/dns.js b/lib/interceptor/dns.js index f1caeef3001..e0e1d7fa486 100644 --- a/lib/interceptor/dns.js +++ b/lib/interceptor/dns.js @@ -66,7 +66,7 @@ class DNSInstance { !newOpts.dualStack && newOpts.affinity ) - return cb( + cb( null, `${origin.protocol}//${ ip.family === 6 ? `[${ip.address}]` : ip.address @@ -75,10 +75,9 @@ class DNSInstance { }) } else { // If there's IPs we pick - const records = this.#records.get(origin.hostname) const ip = this.pick( origin, - records, + ips, // Only set affinity if dual stack is disabled // otherwise let it go through normal flow !newOpts.dualStack && newOpts.affinity @@ -109,22 +108,21 @@ class DNSInstance { return cb(err) } - const results = [] + const results = new Map() for (const addr of addresses) { const record = { address: addr.address, - ttl: - addr.ttl != null - ? Math.min(addr.ttl * 1000, this.#maxTTL) - : this.#maxTTL, + ttl: opts.maxTTL, family: addr.family } - results.push(record) + // On linux we found duplicates, we attempt to remove them with + // the latest record + results.set(`${record.address}:${record.family}`, record) } - cb(null, results) + cb(null, results.values()) } ) } diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index a63cf5f4c13..bca94757556 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -543,7 +543,7 @@ test('Should we handle TTL (4)', async t => { return cb(err) } - const results = [] + const results = new Map() for (const addr of addresses) { const record = { @@ -552,10 +552,10 @@ test('Should we handle TTL (4)', async t => { family: addr.family } - results.push(record) + results.set(`${record.address}:${record.family}`, record) } - cb(null, results) + cb(null, results.values()) } ) } From c7218651b252794b316adc8f523ad3d0f57cee58 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 13 Sep 2024 11:00:19 +0200 Subject: [PATCH 14/15] test: windows --- test/interceptors/dns.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index bca94757556..4621034ea1a 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -334,10 +334,19 @@ test('Should throw when on dual-stack disabled (6)', async t => { const promise = client.request({ ...requestOptions, - origin: 'http://localhost' - }) - - await t.rejects(promise, 'ECONNREFUSED') + origin: 'http://localhost', + headersTimeout: 100 + }) + + // TODO: remove + await t.rejects( + promise.catch(err => { + console.log(err) + throw err + }), + 'ECONNREFUSED' + ) + // await t.rejects(promise, 'ECONNREFUSED') await t.complete }) From 34ab6a35b85e2a19a40fee59b1d691dd0ba75d48 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 13 Sep 2024 11:57:20 +0200 Subject: [PATCH 15/15] test: handle windows gotcha --- test/interceptors/dns.js | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/test/interceptors/dns.js b/test/interceptors/dns.js index 4621034ea1a..e58a1d597ba 100644 --- a/test/interceptors/dns.js +++ b/test/interceptors/dns.js @@ -12,6 +12,8 @@ const { tspl } = require('@matteo.collina/tspl') const { interceptors, Agent } = require('../..') const { dns } = interceptors +const isWindows = process.platform === 'win32' + test('Should validate options', t => { t = tspl(t, { plan: 10 }) @@ -332,21 +334,24 @@ test('Should throw when on dual-stack disabled (6)', async t => { dns({ dualStack: false, affinity: 6 }) ]) - const promise = client.request({ - ...requestOptions, - origin: 'http://localhost', - headersTimeout: 100 - }) - - // TODO: remove - await t.rejects( - promise.catch(err => { - console.log(err) - throw err - }), - 'ECONNREFUSED' - ) - // await t.rejects(promise, 'ECONNREFUSED') + // Note: In windows the IPV6 does not results in ECONNREFUSED + // but rather in TIMEOUT + if (isWindows) { + const promise = client.request({ + ...requestOptions, + origin: 'http://localhost', + headersTimeout: 500 + }) + + await t.rejects(promise, 'UND_ERR_HEADERS_TIMEOUT') + } else { + const promise = client.request({ + ...requestOptions, + origin: 'http://localhost' + }) + + await t.rejects(promise, 'ECONNREFUSED') + } await t.complete })