Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 129 additions & 47 deletions lib/interceptor/dns.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,120 @@ const DecoratorHandler = require('../handler/decorator-handler')
const { InvalidArgumentError, InformationalError } = require('../core/errors')
const maxInt = Math.pow(2, 31) - 1

class DNSInstance {
export class DNSCache {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export class DNSCache {
export class DNSStore {

As @Uzlopak suggestion 😛

#maxTTL = 0
#maxItems = 0
#records = new Map()

constructor (opts) {
this.#maxTTL = opts.maxTTL
this.#maxItems = opts.maxItems
}

get size () {
return this.#records.size
}

// TODO: it will require to write adapter for different caches, look for a better ideas
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we remove the TODO?

Copy link
Contributor Author

@SuperOleg39 SuperOleg39 Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before changes, can you please help me to choose between DNSStorage implementations:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple implementation, only storage logic extracted - #4589 (files)

I'd prefer this one, is simpler and with the example you shared can be enough for implementers to adapt it to their needs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thans, #4589 is ready to review!

get full () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incompatible with js Map interface

return this.size === this.#maxItems
}

get (hostname, opts = {}) {
if (!this.#records.has(hostname)) {
return null
}

const records = this.#records.get(hostname)

if (records == null) {
return null
}

if (records.records[4] != null) {
const ips = records.records[4].ips

// We delete expired records before returning cached records
records.records[4].ips = ips.filter(ip => {
return Date.now() - ip.timestamp <= ip.ttl
})

if (records.records[4].ips.length === 0) {
records.records[4] = null
}
}

// TODO: deduplicate logic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

if (records.records[6] != null) {
const ips = records.records[6].ips

// We delete expired records before returning cached records
records.records[6].ips = ips.filter(ip => {
return Date.now() - ip.timestamp <= ip.ttl
})

if (records.records[6].ips.length === 0) {
records.records[6] = null
}
}

return records
}

set (hostname, records, opts = {}) {
const timestamp = Date.now()

if (records == null) {
return
}

if (records.records[4] != null) {
const ips = records.records[4].ips

for (const ip of ips) {
ip.timestamp = timestamp

if (typeof ip.ttl === 'number') {
// The record TTL is expected to be in ms
ip.ttl = Math.min(ip.ttl, this.#maxTTL)
} else {
ip.ttl = this.#maxTTL
}
}
}

// TODO: deduplicate logic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we abstract it then?

if (records.records[6] != null) {
const ips = records.records[6].ips

for (const ip of ips) {
ip.timestamp = timestamp

if (typeof ip.ttl === 'number') {
// The record TTL is expected to be in ms
ip.ttl = Math.min(ip.ttl, this.#maxTTL)
} else {
ip.ttl = this.#maxTTL
}
}
}

this.#records.set(hostname, records)
}

delete (hostname) {
this.#records.delete(hostname)
}
}

class DNSInstance {
#maxTTL = 0
#maxItems = 0
dualStack = true
affinity = null
lookup = null
pick = null
cache = null
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache property should not be initialized to null since it's expected to be a Map-like object. Consider initializing it to new Map() or removing the initialization entirely since it's set in the constructor.

Suggested change
cache = null

Copilot uses AI. Check for mistakes.

constructor (opts) {
this.#maxTTL = opts.maxTTL
Expand All @@ -21,17 +127,14 @@ class DNSInstance {
this.affinity = opts.affinity
this.lookup = opts.lookup ?? this.#defaultLookup
this.pick = opts.pick ?? this.#defaultPick
}

get full () {
return this.#records.size === this.#maxItems
this.cache = opts.cache ?? new DNSCache(opts)
}

runLookup (origin, opts, cb) {
const ips = this.#records.get(origin.hostname)
const ips = this.cache.get(origin.hostname)

// If full, we just return the origin
if (ips == null && this.full) {
if (ips == null && this.cache.full) {
cb(null, origin)
return
}
Expand All @@ -43,7 +146,8 @@ class DNSInstance {
pick: this.pick,
...opts.dns,
maxTTL: this.#maxTTL,
maxItems: this.#maxItems
maxItems: this.#maxItems,
cache: this.cache
}

// If no IPs we lookup
Expand All @@ -54,8 +158,9 @@ class DNSInstance {
return
}

this.setRecords(origin, addresses)
const records = this.#records.get(origin.hostname)
this.cache.set(origin.hostname, this.addressesToRecords(addresses))
// We get the records again to remove stale entries and mutate the same object
const records = this.cache.get(origin.hostname)

const ip = this.pick(
origin,
Expand Down Expand Up @@ -89,7 +194,7 @@ class DNSInstance {

// If no IPs we lookup - deleting old records
if (ip == null) {
this.#records.delete(origin.hostname)
this.cache.delete(origin.hostname)
this.runLookup(origin, opts, cb)
return
}
Expand Down Expand Up @@ -178,22 +283,11 @@ class DNSInstance {
const position = family.offset % family.ips.length
ip = family.ips[position] ?? null

if (ip == null) {
return ip
}

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)
}

return ip
}

pickFamily (origin, ipFamily) {
const records = this.#records.get(origin.hostname)?.records
const records = this.cache.get(origin.hostname)?.records
if (!records) {
return null
}
Expand All @@ -211,42 +305,30 @@ class DNSInstance {

const position = family.offset % family.ips.length
const ip = family.ips[position] ?? null
if (ip == null) {
return ip
}

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 ip
}

setRecords (origin, addresses) {
const timestamp = Date.now()
// Converts addresses from `dns.lookup` to a records object
addressesToRecords (addresses) {
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: [] }
if (addresses == null) {
return records
}

familyRecords.ips.push(record)
records.records[record.family] = familyRecords
for (const record of addresses) {
if (records.records[record.family] == null) {
records.records[record.family] = { ips: [] }
}
records.records[record.family].ips.push(record)
}

this.#records.set(origin.hostname, records)
return records
}

deleteRecords (origin) {
this.#records.delete(origin.hostname)
this.cache.delete(origin.hostname)
}

getHandler (meta, opts) {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
"test:infra": "borp -p \"test/infra/*.js\"",
"test:interceptors": "borp -p \"test/interceptors/*.js\"",
"test:jest": "cross-env NODE_V8_COVERAGE= jest",
"test:unit": "borp --expose-gc -p \"test/*.js\"",
"test:unit": "borp --reporter spec",
"test:node-fetch": "borp -p \"test/node-fetch/**/*.js\"",
"test:node-test": "borp -p \"test/node-test/**/*.js\"",
"test:tdd": "borp --expose-gc -p \"test/*.js\"",
Expand Down
8 changes: 7 additions & 1 deletion types/interceptors.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,20 @@ declare namespace Interceptors {

// DNS interceptor
export type DNSInterceptorRecord = { address: string, ttl: number, family: 4 | 6 }
export type DNSInterceptorOriginRecords = { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null }
export type DNSInterceptorOriginRecords = { records: { 4: { ips: DNSInterceptorRecord[] } | null, 6: { ips: DNSInterceptorRecord[] } | null } }
export type DNSInterceptorCache = {
size: number
get(origin: string, options?: Record<string, any>): DNSInterceptorOriginRecords | null
set(origin: string, records: DNSInterceptorOriginRecords | null, options?: Record<string, any>): void
}
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: DNSInterceptorOriginRecords, affinity: 4 | 6) => DNSInterceptorRecord
dualStack?: boolean
affinity?: 4 | 6
cache?: DNSInterceptorCache
}

export function dump (opts?: DumpInterceptorOpts): Dispatcher.DispatcherComposeInterceptor
Expand Down