Skip to content
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: 5 additions & 0 deletions doc/api/dns.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ The following methods from the `node:dns` module are available:
<!-- YAML
added: v8.3.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/59829
description: The `options` object now accepts a `rotate` option.
- version:
- v16.7.0
- v14.18.0
Expand All @@ -159,6 +162,8 @@ Create a new resolver.
each name server before giving up. **Default:** `4`
* `maxTimeout` {integer} The max retry timeout, in milliseconds.
**Default:** `0`, disabled.
* `rotate` {boolean} Perform round-robin selection of the nameservers for each resolution.
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we go with selectionStrategy: 'round-robin' | undefined rather than a boolean flag?

**Default:** `undefined`, depends on system configuration and Node.js build settings.

### `resolver.cancel()`

Expand Down
29 changes: 23 additions & 6 deletions lib/internal/dns/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const { isIP } = require('internal/net');
const { getOptionValue } = require('internal/options');
const {
validateArray,
validateBoolean,
validateInt32,
validateOneOf,
validateString,
Expand Down Expand Up @@ -62,6 +63,14 @@ function validateTries(options) {
return tries;
}

function validateRotate(options) {
const { rotate } = { ...options };
if (rotate !== undefined) {
validateBoolean(rotate, 'options.rotate');
}
return rotate;
Comment on lines +66 to +71
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
function validateRotate(options) {
const { rotate } = { ...options };
if (rotate !== undefined) {
validateBoolean(rotate, 'options.rotate');
}
return rotate;
function validateRotate({ rotate }) {
if (rotate !== undefined) {
validateBoolean(rotate, 'options.rotate');
}
return rotate;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the parameter of validateRotate maybe undefined which trigger an error.

}

const kSerializeResolver = Symbol('dns:resolver:serialize');
const kDeserializeResolver = Symbol('dns:resolver:deserialize');
const kSnapshotStates = Symbol('dns:resolver:config');
Expand All @@ -75,17 +84,18 @@ class ResolverBase {
const timeout = validateTimeout(options);
const tries = validateTries(options);
const maxTimeout = validateMaxTimeout(options);
const rotate = validateRotate(options);
// If we are building snapshot, save the states of the resolver along
// the way.
if (isBuildingSnapshot()) {
this[kSnapshotStates] = { timeout, tries, maxTimeout };
this[kSnapshotStates] = { timeout, tries, maxTimeout, rotate };
}
this[kInitializeHandle](timeout, tries, maxTimeout);
this[kInitializeHandle](timeout, tries, maxTimeout, rotate);
}

[kInitializeHandle](timeout, tries, maxTimeout) {
[kInitializeHandle](timeout, tries, maxTimeout, rotate) {
const { ChannelWrap } = lazyBinding();
this._handle = new ChannelWrap(timeout, tries, maxTimeout);
this._handle = new ChannelWrap(timeout, tries, maxTimeout, rotate);
}

cancel() {
Expand Down Expand Up @@ -195,8 +205,15 @@ class ResolverBase {
}

[kDeserializeResolver]() {
const { timeout, tries, maxTimeout, localAddress, servers } = this[kSnapshotStates];
this[kInitializeHandle](timeout, tries, maxTimeout);
const {
timeout,
tries,
maxTimeout,
localAddress,
servers,
rotate,
} = this[kSnapshotStates];
this[kInitializeHandle](timeout, tries, maxTimeout, rotate);
if (localAddress) {
const { ipv4, ipv6 } = localAddress;
this._handle.setLocalAddress(ipv4, ipv6);
Expand Down
25 changes: 20 additions & 5 deletions src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ namespace cares_wrap {

using v8::Array;
using v8::ArrayBuffer;
using v8::Boolean;
using v8::Context;
using v8::EscapableHandleScope;
using v8::Exception;
Expand Down Expand Up @@ -791,11 +792,13 @@ ChannelWrap::ChannelWrap(Environment* env,
Local<Object> object,
int timeout,
int tries,
int max_timeout)
int max_timeout,
std::optional<bool> rotate)
: AsyncWrap(env, object, PROVIDER_DNSCHANNEL),
timeout_(timeout),
tries_(tries),
max_timeout_(max_timeout) {
max_timeout_(max_timeout),
rotate_(rotate) {
MakeWeak();

Setup();
Expand All @@ -809,15 +812,21 @@ void ChannelWrap::MemoryInfo(MemoryTracker* tracker) const {

void ChannelWrap::New(const FunctionCallbackInfo<Value>& args) {
CHECK(args.IsConstructCall());
CHECK_EQ(args.Length(), 3);
CHECK_GE(args.Length(), 3);
CHECK(args[0]->IsInt32());
CHECK(args[1]->IsInt32());
CHECK(args[2]->IsInt32());

const int timeout = args[0].As<Int32>()->Value();
const int tries = args[1].As<Int32>()->Value();
const int max_timeout = args[2].As<Int32>()->Value();
std::optional<bool> rotate;
if (!args[3]->IsUndefined()) {
CHECK(args[3]->IsBoolean());
rotate = args[3].As<Boolean>()->Value();
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
rotate = args[3].As<Boolean>()->Value();
rotate = args[3]->IsTrue();

}
Environment* env = Environment::GetCurrent(args);
new ChannelWrap(env, args.This(), timeout, tries, max_timeout);
new ChannelWrap(env, args.This(), timeout, tries, max_timeout, rotate);
}

GetAddrInfoReqWrap::GetAddrInfoReqWrap(Environment* env,
Expand Down Expand Up @@ -889,7 +898,13 @@ void ChannelWrap::Setup() {
options.maxtimeout = max_timeout_;
optmask |= ARES_OPT_MAXTIMEOUTMS;
}

if (rotate_.has_value()) {
if (rotate_.value()) {
optmask |= ARES_OPT_ROTATE;
} else {
optmask |= ARES_OPT_NOROTATE;
}
}
r = ares_init_options(&channel_, &options, optmask);

if (r != ARES_SUCCESS) {
Expand Down
5 changes: 4 additions & 1 deletion src/cares_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "v8.h"
#include "uv.h"

#include <optional>
#include <unordered_set>

#ifdef __POSIX__
Expand Down Expand Up @@ -156,7 +157,8 @@ class ChannelWrap final : public AsyncWrap {
v8::Local<v8::Object> object,
int timeout,
int tries,
int max_timeout);
int max_timeout,
std::optional<bool> rotate);
~ChannelWrap() override;

static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand Down Expand Up @@ -192,6 +194,7 @@ class ChannelWrap final : public AsyncWrap {
int timeout_;
int tries_;
int max_timeout_;
std::optional<bool> rotate_;
int active_query_count_ = 0;
NodeAresTask::List task_list_;
};
Expand Down
108 changes: 108 additions & 0 deletions test/parallel/test-dns-resolver-rotate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
'use strict';
const common = require('../common');
const dnstools = require('../common/dns');
const dns = require('dns');
const assert = require('assert');
const dgram = require('dgram');

function validate() {
[
-1,
1.1,
NaN,
undefined,
{},
[],
null,
function() {},
Symbol(),
Infinity,
].forEach((rotate) => {
try {
new dns.Resolver({ rotate });
} catch (e) {
assert.ok(/ERR_INVALID_ARG_TYPE/i.test(e.code));
}
});
}

const domain = 'example.org';
const answers = [{ type: 'A', address: '1.2.3.4', ttl: 123, domain }];

function createServer() {
return new Promise((resolve) => {
const server = dgram.createSocket('udp4');
server.on('message', (msg, { address, port }) => {
const parsed = dnstools.parseDNSPacket(msg);
server.send(dnstools.writeDNSPacket({
id: parsed.id,
questions: parsed.questions,
answers: answers,
}), port, address);
});
server.bind(0, common.mustCall(() => {
resolve(server);
}));
});
}

function check(result, answers) {
assert.strictEqual(result.length, answers.length);
assert.strictEqual(result[0].type, answers[0].type);
assert.strictEqual(result[0].address, answers[0].address);
assert.strictEqual(result[0].ttl, answers[0].ttl);
}

async function main() {
validate();
{
const resolver = new dns.promises.Resolver({ rotate: false });
const server1 = await createServer();
const server2 = await createServer();
const address1 = server1.address();
const address2 = server2.address();
resolver.setServers([
`127.0.0.1:${address1.port}`,
`127.0.0.1:${address2.port}`,
]);
server2.on('message', common.mustNotCall());
const promises = [];
// All queries should be sent to the server1
for (let i = 0; i < 5; i++) {
promises.push(resolver.resolveAny(domain));
}
const results = await Promise.all(promises);
results.forEach((result) => check(result, answers));
server1.close();
server2.close();
}

{
const resolver = new dns.promises.Resolver({ rotate: true });
const serverPromises = [];
for (let i = 0; i < 10; i++) {
serverPromises.push(createServer());
}
const servers = await Promise.all(serverPromises);
const addresses = [];
let receiver = 0;
servers.forEach((server) => {
addresses.push(`127.0.0.1:${server.address().port}`);
server.once('message', () => {
receiver++;
});
});
resolver.setServers(addresses);
const queryPromises = [];
// All queries should be randomly sent to the server (Unless the same server is chosen every time)
for (let i = 0; i < 30; i++) {
queryPromises.push(resolver.resolveAny(domain));
}
const results = await Promise.all(queryPromises);
results.forEach((result) => check(result, answers));
assert.ok(receiver > 1, `receiver: ${receiver}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you assert that receiver is 30 instead? Since that is the number of requests?

And this is supposed to be round robin, right? So if you dispatch 30 requests simultaneously, there is no way they'd all go to just one server, right? Could you at least assert that not all 30 are on one server to demonstrate that some sort of rotation is happening? I understand we don't have to be uber specific about it.

servers.forEach((server) => server.close());
}
}

main();
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't seen test written like this before. You can remove these function wrappers and just encapsulate the test in { } blocks or even use the Node test runner test, describe/it, apis

Copy link
Member

Choose a reason for hiding this comment

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

There are a lot, run git grep main\( test/parallel/.

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
main();
main().then(common.mustCall());

Loading