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.

Sign up for GitHub

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

dns: support cares_search for resolution #22226

Closed
wants to merge 2 commits into from
Closed

dns: support cares_search for resolution #22226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6a828e
Squashing multiple commits to ease rebase
SirR4T Aug 10, 2018
a29a17c
backport upstream fix
SirR4T Sep 17, 2018
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
2 changes: 2 additions & 0 deletions deps/cares/include/ares.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ extern "C" {
#define ARES_OPT_ROTATE (1 << 14)
#define ARES_OPT_EDNSPSZ (1 << 15)
#define ARES_OPT_NOROTATE (1 << 16)
#define ARES_OPT_RESOLVCONF (1 << 17)

/* Nameinfo flag values */
#define ARES_NI_NOFQDN (1 << 0)
Expand Down Expand Up @@ -270,6 +271,7 @@ struct ares_options {
struct apattern *sortlist;
int nsort;
int ednspsz;
char *resolvconf_path;
};

struct hostent;
Expand Down
5 changes: 5 additions & 0 deletions deps/cares/src/ares_destroy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ void ares_destroy_options(struct ares_options *options)
ares_free(options->sortlist);
if(options->lookups)
ares_free(options->lookups);
if(options->resolvconf_path)
ares_free(options->resolvconf_path);
}

void ares_destroy(ares_channel channel)
Expand Down Expand Up @@ -85,6 +87,9 @@ void ares_destroy(ares_channel channel)
if (channel->lookups)
ares_free(channel->lookups);

if (channel->resolvconf_path)
ares_free(channel->resolvconf_path);

ares_free(channel);
}

Expand Down
35 changes: 33 additions & 2 deletions deps/cares/src/ares_init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ int ares_init_options(ares_channel *channelptr, struct ares_options *options,
channel->sock_config_cb_data = NULL;
channel->sock_funcs = NULL;
channel->sock_func_cb_data = NULL;
channel->resolvconf_path = NULL;

channel->last_server = 0;
channel->last_timeout_processed = (time_t)now.tv_sec;
Expand Down Expand Up @@ -246,6 +247,8 @@ int ares_init_options(ares_channel *channelptr, struct ares_options *options,
ares_free(channel->sortlist);
if(channel->lookups)
ares_free(channel->lookups);
if(channel->resolvconf_path)
ares_free(channel->resolvconf_path);
ares_free(channel);
return status;
}
Expand Down Expand Up @@ -351,7 +354,7 @@ int ares_save_options(ares_channel channel, struct ares_options *options,
(*optmask) = (ARES_OPT_FLAGS|ARES_OPT_TRIES|ARES_OPT_NDOTS|
ARES_OPT_UDP_PORT|ARES_OPT_TCP_PORT|ARES_OPT_SOCK_STATE_CB|
ARES_OPT_SERVERS|ARES_OPT_DOMAINS|ARES_OPT_LOOKUPS|
ARES_OPT_SORTLIST|ARES_OPT_TIMEOUTMS);
ARES_OPT_SORTLIST|ARES_OPT_TIMEOUTMS|ARES_OPT_RESOLVCONF);
(*optmask) |= (channel->rotate ? ARES_OPT_ROTATE : ARES_OPT_NOROTATE);

/* Copy easy stuff */
Expand Down Expand Up @@ -426,6 +429,13 @@ int ares_save_options(ares_channel channel, struct ares_options *options,
}
options->nsort = channel->nsort;

/* copy path for resolv.conf file */
if (channel->resolvconf_path) {
options->resolvconf_path = ares_strdup(channel->resolvconf_path);
if (!options->resolvconf_path)
return ARES_ENOMEM;
}

return ARES_SUCCESS;
}

Expand Down Expand Up @@ -534,6 +544,14 @@ static int init_by_options(ares_channel channel,
channel->nsort = options->nsort;
}

/* Set path for resolv.conf file, if given. */
if ((optmask & ARES_OPT_RESOLVCONF) && !channel->resolvconf_path)
{
channel->resolvconf_path = ares_strdup(options->resolvconf_path);
if (!channel->resolvconf_path && options->resolvconf_path)
return ARES_ENOMEM;
}

channel->optmask = optmask;

return ARES_SUCCESS;
Expand Down Expand Up @@ -1740,6 +1758,7 @@ static int init_by_resolv_conf(ares_channel channel)
size_t linesize;
int error;
int update_domains;
const char *resolvconf_path;

/* Don't read resolv.conf and friends if we don't have to */
if (ARES_CONFIG_CHECK(channel))
Expand All @@ -1748,7 +1767,14 @@ static int init_by_resolv_conf(ares_channel channel)
/* Only update search domains if they're not already specified */
update_domains = (channel->ndomains == -1);

fp = fopen(PATH_RESOLV_CONF, "r");
/* Support path for resolvconf filename set by ares_init_options */
if(channel->resolvconf_path) {
resolvconf_path = channel->resolvconf_path;
} else {
resolvconf_path = PATH_RESOLV_CONF;
}

fp = fopen(resolvconf_path, "r");
if (fp) {
while ((status = ares__read_line(fp, &line, &linesize)) == ARES_SUCCESS)
{
Expand Down Expand Up @@ -2050,6 +2076,11 @@ static int init_by_defaults(ares_channel channel)
ares_free(channel->lookups);
channel->lookups = NULL;
}

if(channel->resolvconf_path) {
ares_free(channel->resolvconf_path);
channel->resolvconf_path = NULL;
}
}

if(hostname)
Expand Down
3 changes: 3 additions & 0 deletions deps/cares/src/ares_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,9 @@ struct ares_channeldata {

const struct ares_socket_functions * sock_funcs;
void *sock_func_cb_data;

/* Path for resolv.conf file, configurable via ares_options */
char *resolvconf_path;
};

/* Memory management functions */
Expand Down
14 changes: 14 additions & 0 deletions doc/api/dns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,9 @@ On error, `err` is an [`Error`][] object, where `err.code` is one of the
<!-- YAML
added: v0.1.16
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/22226
description: The `options.search` parameter is now supported.
- version: v7.2.0
pr-url: https://github.com/nodejs/node/pull/9296
description: This method now supports passing `options`,
Expand All @@ -287,6 +290,10 @@ changes:
When `true`, the callback receives an array of
`{ address: '1.2.3.4', ttl: 60 }` objects rather than an array of strings,
with the TTL expressed in seconds.
- `search` {boolean} Resolve the hostname using the local domain name or the
search list for host-name lookup.
When `true`, the resolver will use the search list, which can create extra
DNS queries. **Default:** `false`.
* `callback` {Function}
- `err` {Error}
- `addresses` {string[] | Object[]}
Expand All @@ -300,6 +307,9 @@ will contain an array of IPv4 addresses (e.g.
<!-- YAML
added: v0.1.16
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/22226
description: The `options.search` parameter is now supported.
- version: v7.2.0
pr-url: https://github.com/nodejs/node/pull/9296
description: This method now supports passing `options`,
Expand All @@ -311,6 +321,10 @@ changes:
When `true`, the callback receives an array of
`{ address: '0:1:2:3:4:5:6:7', ttl: 60 }` objects rather than an array of
strings, with the TTL expressed in seconds.
- `search` {boolean} Resolve the hostname using the local domain name or the
search list for host-name lookup.
When `true`, the resolver will use the search list, which can create extra
DNS queries. **Default:** `false`.
* `callback` {Function}
- `err` {Error}
- `addresses` {string[] | Object[]}
Expand Down
3 changes: 2 additions & 1 deletion lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ function resolver(bindingName) {
req.hostname = name;
req.oncomplete = onresolve;
req.ttl = !!(options && options.ttl);
var err = this._handle[bindingName](req, name);
const search = !!(options && options.search);
var err = this._handle[bindingName](req, name, search);
if (err) throw dnsException(err, bindingName, name);
return req;
}
Expand Down
8 changes: 5 additions & 3 deletions lib/internal/dns/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ function onresolve(err, result, ttls) {
this.resolve(result);
}

function createResolverPromise(resolver, bindingName, hostname, ttl) {
function createResolverPromise(resolver, bindingName, hostname, ttl, isSearch) {
return new Promise((resolve, reject) => {
const req = new QueryReqWrap();

Expand All @@ -181,8 +181,9 @@ function createResolverPromise(resolver, bindingName, hostname, ttl) {
req.resolve = resolve;
req.reject = reject;
req.ttl = ttl;
req.search = isSearch;

const err = resolver._handle[bindingName](req, hostname);
const err = resolver._handle[bindingName](req, hostname, isSearch);
Copy link
Member

Choose a reason for hiding this comment

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

I may be misunderstanding something, but is it needed to pass an argument isSearch while it is already in req object and it can be extracted from there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure either, how JSland calls into C++ land, but looking at this block of code, I figured this is how I'd have to add an additional parameter, and it worked.

If you can show me an example of unwrapping properties on an object, in C++ land, I'm willing to test this approach out and update if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides, what would be the better approach? Unwrapping properties would implicitly assume that the property names are part of the API contract, whereas passing in parameters would make the signature explicit.

Copy link
Member

Choose a reason for hiding this comment

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

Since on the c++ you already have an Local<Object> I think you can just

Local<Value> search = req_wrap_obj->Get(env->isolate(), FIXED_ONE_BYTE_STRING(env->isolate(), "search"));
CHECK(search->IsBoolean());
bool is_search = (search.As<Boolean>)->Value();

Though, yeah, I'm not sure if this is a correct way of doing things (no need to change this yet). I just thought I'd mention it since it is both in the object and as an argument. Is it needed to set it in the req object at all (maybe just an argument will be enough)?

Copy link
Contributor Author

@SirR4T SirR4T Aug 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

not really needed anywhere else, I guess. Fixed this.

Copy link
Member

Choose a reason for hiding this comment

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

You need to be consistent, it's not done on the non-promises version. I think the only reason ttl is set on req is so that we can tack ttl on to the response object in onresolve(), i.e. to connect the input with the output processing rather than interacting with c-ares at all. So I think it'd be appropriate to not attach anything new to req since we don't need it after the query is sent.


if (err)
reject(dnsException(err, bindingName, hostname));
Expand All @@ -196,7 +197,8 @@ function resolver(bindingName) {
}

const ttl = !!(options && options.ttl);
return createResolverPromise(this, bindingName, name, ttl);
const isSearch = !!(options && options.search);
return createResolverPromise(this, bindingName, name, ttl, isSearch);
}

Object.defineProperty(query, 'name', { value: bindingName });
Expand Down
Loading