-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
dns: allow --dns-verbatim to change default dns verbatim #38099
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,4 @@ | ||||||||||||||||
'use strict'; | ||||||||||||||||
|
||||||||||||||||
const { | ||||||||||||||||
ArrayPrototypeMap, | ||||||||||||||||
ObjectCreate, | ||||||||||||||||
|
@@ -14,6 +13,7 @@ const { | |||||||||||||||
validateHints, | ||||||||||||||||
validateTimeout, | ||||||||||||||||
emitInvalidHostnameWarning, | ||||||||||||||||
getDefaultVerbatim, | ||||||||||||||||
} = require('internal/dns/utils'); | ||||||||||||||||
const { codes, dnsException } = require('internal/errors'); | ||||||||||||||||
const { toASCII } = require('internal/idna'); | ||||||||||||||||
|
@@ -103,7 +103,7 @@ function lookup(hostname, options) { | |||||||||||||||
var hints = 0; | ||||||||||||||||
var family = -1; | ||||||||||||||||
var all = false; | ||||||||||||||||
var verbatim = false; | ||||||||||||||||
var verbatim = getDefaultVerbatim(); | ||||||||||||||||
aduh95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||
|
||||||||||||||||
// Parse arguments | ||||||||||||||||
if (hostname && typeof hostname !== 'string') { | ||||||||||||||||
|
@@ -112,7 +112,9 @@ function lookup(hostname, options) { | |||||||||||||||
hints = options.hints >>> 0; | ||||||||||||||||
family = options.family >>> 0; | ||||||||||||||||
all = options.all === true; | ||||||||||||||||
verbatim = options.verbatim === true; | ||||||||||||||||
if (typeof options.verbatim === 'boolean') { | ||||||||||||||||
verbatim = options.verbatim === true; | ||||||||||||||||
} | ||||||||||||||||
Comment on lines
+115
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be more useful to throw if the value is not a boolean rather than ignoring it? It the value is not a boolean, it's probably a mistake that users would want to spot.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's better. But I'm afraid of introducing a breaking change here. Let's reconsider this in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to do it in a possible breaking way as long as we don't land it in v15.x. Once it has landed in v16.0.0, we can backport it to v14.x (and v12.x?) without the breaking part. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mcollina wdyt? Should this land with or without argument validation (possibly breaking change)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to say I prefer the argument validation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still want to keep the possibility to backport this to v12 and v14. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could still backport it without the argument validation. I volunteer to take care of the backport PR if that if that helps. |
||||||||||||||||
|
||||||||||||||||
validateHints(hints); | ||||||||||||||||
} else { | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
// Flags: --expose-internals --dns-result-order=ipv4first | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const { internalBinding } = require('internal/test/binding'); | ||
const cares = internalBinding('cares_wrap'); | ||
const { promisify } = require('util'); | ||
|
||
// Test that --dns-result-order=ipv4first works as expected. | ||
|
||
const originalGetaddrinfo = cares.getaddrinfo; | ||
const calls = []; | ||
cares.getaddrinfo = common.mustCallAtLeast((...args) => { | ||
calls.push(args); | ||
originalGetaddrinfo(...args); | ||
}, 1); | ||
|
||
const dns = require('dns'); | ||
const dnsPromises = dns.promises; | ||
|
||
oyyd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let verbatim; | ||
|
||
// We want to test the parameter of verbatim only so that we | ||
// ignore possible errors here. | ||
function allowFailed(fn) { | ||
return fn.catch((_err) => { | ||
// | ||
}); | ||
} | ||
|
||
(async () => { | ||
let callsLength = 0; | ||
const checkParameter = (expected) => { | ||
assert.strictEqual(calls.length, callsLength + 1); | ||
verbatim = calls[callsLength][4]; | ||
assert.strictEqual(verbatim, expected); | ||
callsLength += 1; | ||
}; | ||
|
||
await allowFailed(promisify(dns.lookup)('example.org')); | ||
checkParameter(false); | ||
|
||
await allowFailed(dnsPromises.lookup('example.org')); | ||
checkParameter(false); | ||
|
||
await allowFailed(promisify(dns.lookup)('example.org', {})); | ||
checkParameter(false); | ||
|
||
await allowFailed(dnsPromises.lookup('example.org', {})); | ||
checkParameter(false); | ||
})().then(common.mustCall()); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
// Flags: --expose-internals --dns-result-order=verbatim | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
const { internalBinding } = require('internal/test/binding'); | ||
const cares = internalBinding('cares_wrap'); | ||
const { promisify } = require('util'); | ||
|
||
// Test that --dns-result-order=verbatim works as expected. | ||
|
||
const originalGetaddrinfo = cares.getaddrinfo; | ||
const calls = []; | ||
cares.getaddrinfo = common.mustCallAtLeast((...args) => { | ||
calls.push(args); | ||
originalGetaddrinfo(...args); | ||
}, 1); | ||
|
||
const dns = require('dns'); | ||
const dnsPromises = dns.promises; | ||
|
||
let verbatim; | ||
|
||
// We want to test the parameter of verbatim only so that we | ||
// ignore possible errors here. | ||
function allowFailed(fn) { | ||
return fn.catch((_err) => { | ||
// | ||
}); | ||
} | ||
|
||
(async () => { | ||
let callsLength = 0; | ||
const checkParameter = (expected) => { | ||
assert.strictEqual(calls.length, callsLength + 1); | ||
verbatim = calls[callsLength][4]; | ||
assert.strictEqual(verbatim, expected); | ||
callsLength += 1; | ||
}; | ||
|
||
await allowFailed(promisify(dns.lookup)('example.org')); | ||
checkParameter(true); | ||
|
||
await allowFailed(dnsPromises.lookup('example.org')); | ||
checkParameter(true); | ||
|
||
await allowFailed(promisify(dns.lookup)('example.org', {})); | ||
checkParameter(true); | ||
|
||
await allowFailed(dnsPromises.lookup('example.org', {})); | ||
checkParameter(true); | ||
})().then(common.mustCall()); |
Uh oh!
There was an error while loading. Please reload this page.