Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: dns interceptor #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
5 changes: 4 additions & 1 deletion lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const interceptors = {
proxy: (await import('./interceptor/proxy.js')).default,
cache: (await import('./interceptor/cache.js')).default,
requestId: (await import('./interceptor/request-id.js')).default,
dns: (await import('./interceptor/dns.js')).default,
lookup: (await import('./interceptor/lookup.js')).default,
}

Expand All @@ -30,6 +31,7 @@ function wrapDispatcher(dispatcher) {
interceptors.responseError(),
interceptors.requestBodyFactory(),
interceptors.log(),
interceptors.dns(),
interceptors.lookup(),
interceptors.requestId(),
interceptors.responseRetry(),
Expand Down Expand Up @@ -58,7 +60,7 @@ function wrapDispatcher(dispatcher) {
headers,
signal: opts.signal,
reset: opts.reset ?? false,
blocking: opts.blocking ?? null,
blocking: opts.blocking ?? opts.method !== 'HEAD',
headersTimeout: opts.headersTimeout,
bodyTimeout: opts.bodyTimeout,
idempotent: opts.idempotent,
Expand All @@ -70,6 +72,7 @@ function wrapDispatcher(dispatcher) {
error: opts.error ?? true,
verify: opts.verify ?? true,
logger: opts.logger ?? null,
dns: opts.dns ?? true,
connect: opts.connect,
lookup: opts.lookup ?? defaultLookup,
maxRedirections: 0, // TODO (fix): Ugly hack to disable undici redirections.
Expand Down
125 changes: 125 additions & 0 deletions lib/interceptor/dns.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
import { DecoratorHandler } from '../utils.js'
import net from 'net'
import { resolve4 } from 'node:dns/promises'

let fastNow = Date.now()
setInterval(() => {
fastNow = Date.now()
}, 500)

class Record {
address = ''
expires = 0
errored = 0

constructor({ address, ttl }) {
this.address = address
this.expires = fastNow + (ttl ?? 60) * 1e3
}
}

class Handler extends DecoratorHandler {
#handler
#record

constructor({ record }, { handler }) {
super(handler)

this.#handler = handler
this.#record = record
}

onError(err) {
if (
[
'ECONNRESET',
'ECONNREFUSED',
'ENOTFOUND',
'ENETDOWN',
'ENETUNREACH',
'EHOSTDOWN',
'EHOSTUNREACH',
'EHOSTNOTFOUND',
'ENODATA',
'EPIPE',
'UND_ERR_CONNECT_TIMEOUT',
].includes(err.code) ||
[503].includes(err.statusCode)
) {
this.#record.errored = fastNow

// TODO (fix): For how long do we "blacklist" the record?

if (err.code === 'UND_ERR_CONNECT_TIMEOUT') {
// We don't expect this address to ever work again...
this.#record.expires = Infinity
}
}

return super.onError(err)
}
}

export default (interceptorOpts) => (dispatch) => {
/** @type {Map<string, Array<Record>>} */
const dnsCache = new Map()

return async (opts, handler) => {
if (!opts.dns) {
return dispatch(opts, handler)
}

const { protocol, port, hostname, host } = new URL(opts.origin)

if (net.isIP(hostname) || opts.headers?.host || !port || !protocol) {

Choose a reason for hiding this comment

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

why skip when headers.host or !port?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should always have port and protocol so I guess I should change that to an asser ot default to http:80.

If a host header is set I'm not sure what kind of weird issues it can cause.

return dispatch(opts, handler)
}

const now = Date.now()
try {
/** @type {Array|undefined} */
let records = dnsCache.get(hostname)

if (!records?.some((record) => record.expires > now && !record.errored)) {
// TODO (fix): Re-use old records while fetching new ones or if fetching fails?
// TODO (fix): Background refresh + health checks?
// TODO (fix): What about old "blacklisted" records?
// TODO (fix): What about ipv6?

records = await resolve4(hostname, { ttl: true })
Copy link

@deadbeef84 deadbeef84 Oct 16, 2024

Choose a reason for hiding this comment

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

if there's no entry in the dnsCache, and multiple requests are made, it will create many concurrent records - possibly an issue?

also perhaps use stale-while-revalidate?

Copy link
Member Author

Choose a reason for hiding this comment

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

can avsolulety be more optimized. I'll have a look

records = records.map((record) => new Record(record))

if (records.length > 0) {
// TODO (fix): Clear old hostnames?
dnsCache.set(hostname, records)
}
}

if (records.length === 0) {
return dispatch(opts, handler)
}

// TODO (perf): sort + Math.random is a bit naive...
records.sort((a, b) =>
a.errored !== b.errored ? a.errored - b.errored : Math.random() - 0.5,
)

const record = records.find((record) => record.expires > now)

if (!record) {
return dispatch(opts, handler)
}
Comment on lines +107 to +111

Choose a reason for hiding this comment

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

not sure I follow this, it was already checked at top?

Copy link
Member Author

Choose a reason for hiding this comment

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

checked where?


return dispatch(
{
...opts,
origin: `${protocol}//${record.address}:${port}`,
headers: { ...opts.headers, host },

Choose a reason for hiding this comment

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

why set host header?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because otherwise the host header will have the ip instead of the dns name which will cause issues.

},
new Handler({ record }, { handler }),
)
} catch (err) {
handler.onError(err)
}
}
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@nxtedition/nxt-undici",
"version": "4.2.20",
"version": "4.2.21",
"license": "MIT",
"author": "Robert Nagy <[email protected]>",
"main": "lib/index.js",
Expand Down