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

dns: support cares_search for resolution #22226

Closed
wants to merge 2 commits into from

Conversation

SirR4T
Copy link
Contributor

@SirR4T SirR4T commented Aug 10, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Please note: this is my first time writing C++ code, so I'm not sure if this is what was required.
Current test cases are passing, though, but haven't added any tests for cares_search resolution.

Fixes: #17850 (based on approach suggested in #19548)

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dns Issues and PRs related to the dns subsystem. labels Aug 10, 2018
@addaleax addaleax added wip Issues and PRs that are still a work in progress. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 10, 2018
@@ -466,7 +466,11 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) {
dest->h_addrtype = src->h_addrtype;
}

class QueryWrap;
template <void (*F)(ares_channel, const char *, int, int, ares_callback, void *)>
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: Left-leaning pointers (e.g. const char*, not const char *, here and below)

If you’re unsure about something, we have a C++ style guide, and, very recently, introduced make format-cpp CLANG_FORMAT_START=upstream/master (you may need to run make format-cpp-build first)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax thanks for the tip! this produces tools/clang-format/node_modules/ though, in my working directory. Good idea to add this to .gitignore in this PR? or should that be a different PR by itself?


class QueryWrap : public AsyncWrap {
template <void (*F)(ares_channel, const char *, int, int, ares_callback, void *)>
class BaseWrap : public AsyncWrap {
Copy link
Member

Choose a reason for hiding this comment

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

I’m wondering if it makes sense to do templating on this level, since it introduces a new parallel class hierarchy … maybe we could store the function as a member of QueryWrap instead, and pass it to the QueryWrap constructor?

That would also allow us to avoid the this->… oddities below…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax not too sure how this works in C++ land. Could you throw some more light? For instance, If I pass in the cares function (say ares_search) to the base class (say BaseWrap), could the result still be a class (instead of what I assume will be an object)?

I think that was the advantage of templating, so I need not touch wherever QueryWrap is being used to create other objects. Maybe some code snippets would help me understand better.

Copy link
Member

Choose a reason for hiding this comment

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

@SirR4T

For instance, If I pass in the cares function (say ares_search) to the base class (say BaseWrap), could the result still be a class (instead of what I assume will be an object)?

I don’t really understand this, sorry … What do you mean by “pass to the base class”?

I think that was the advantage of templating, so I need not touch wherever QueryWrap is being used to create other objects.

Doing this also duplicates all generated code and

Basically, what you have is something like this:

template <int (*F)()>
class Base {
  /* ... */
};

template <typename BaseWrap>
class SubclassWrap1 : BaseWrap {
  /* ... */
};

/* ... */

template <typename Wrap>
void Query(/* ... */) {
  new Wrap(/*...*/);
}

// And then use:
Query<SubclassWrap1<Base<function1>>>();
Query<SubclassWrap1<Base<function2>>>();
Query<SubclassWrap2<Base<function1>>>();
Query<SubclassWrap2<Base<function2>>>();
/* ... */

While this works, it seems very complex in terms of the generated class hiearchy.

What I’m thinking does not modify the existing class hierachy, but rather passes along one more argument:

class Base {
  Base(int (*fn)()) : search_function_(fn) {}
  /* ... */
 private:
  /* ... */
  int (*search_function_)();
};

class SubclassWrap1 : Base {
  SubclassWrap1(..., int (*fn)()) : Base(..., fn) {}
  // Or, alternatively, we could switch to making the constructor forward 
  // all arguments regardless of what they actually are:
  template <typename... Args>
  SubclassWrap(Args&&... args) : Base(std::forward<Args>(args)...) {}
};

template <typename Wrap>
void Query(/* ... */) {
  new Wrap(..., use_function_1 ? function1 : function2);
}

(You could of course use a typedef instead of always spelling out the function type here)

@Trott
Copy link
Member

Trott commented Aug 10, 2018

Why the alteration of existing test cases? (Seems like it's just setting the value to the default, which seems unnecessary and eliminates those test cases as checks that behavior is not changing.)

Can you add new test cases to that file?

doc/api/dns.md Outdated
- `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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: dns -> DNS?

doc/api/dns.md Outdated
- `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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@SirR4T SirR4T force-pushed the fix19548-support-cares-search branch from 2f6a506 to 99034f8 Compare August 16, 2018 06:44
@SirR4T
Copy link
Contributor Author

SirR4T commented Aug 16, 2018

@Trott : fixed and added new test cases in the same file. Am still not sure, though, that these would cover the ares_search functionality fully. If someone could guide me what kind of tests should be required specifically for search functionality, would be happy to add them.

Copy link
Contributor Author

@SirR4T SirR4T left a comment

Choose a reason for hiding this comment

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

ping @addaleax / @bnoordhuis : I'm guessing this is still incomplete, and needs more review.

@@ -637,7 +649,7 @@ class QueryWrap : public AsyncWrap {
static void CaresAsyncCb(uv_async_t* handle) {
auto data = static_cast<struct CaresAsyncData*>(handle->data);

QueryWrap* wrap = data->wrap;
BaseWrap* wrap = data->wrap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @addaleax , I think I was able to follow through on your suggestions from here. I'm iffy about updating the struct void CaresAsyncCb {} though, as it looks like an internal data structure and I'm not sure what I'm rattling here.

When I try to leave this line in as QueryWrap* wrap = data->wrap;, the compiler complains that I'm trying to use an "incomplete" type (QueryWrap). Something else should be done here?

Copy link
Member

Choose a reason for hiding this comment

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

@SirR4T It’s hard to tell until this PR is at the point where the option starts to work.

I’d personally not split BaseWrap into two QueryWrap and SearchWrap, and instead pass the new constructor parameter along from the subclasses, e.g.:

class QueryAnyWrap: public QueryWrap {
 public:
  QueryAnyWrap(ChannelWrap* channel, Local<Object> req_wrap_obj)
    : QueryWrap(channel, req_wrap_obj) {
  }

becomes

class QueryAnyWrap: public QueryWrap {
 public:
  QueryAnyWrap(ChannelWrap* channel, Local<Object> req_wrap_obj, ares_function* fn)
    : QueryWrap(channel, req_wrap_obj, fn) {
  }

and then decide which function to pass in at the call site (the Query function). But yeah, it’s hard to tell in the current state of things…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax should

  env->SetProtoMethod(channel_wrap, "queryAny", Query<QueryAnyWrap>);
  env->SetProtoMethod(channel_wrap, "queryA", Query<QueryAWrap>);
  env->SetProtoMethod(channel_wrap, "queryAaaa", Query<QueryAaaaWrap>);
  env->SetProtoMethod(channel_wrap, "queryCname", Query<QueryCnameWrap>);
  env->SetProtoMethod(channel_wrap, "queryMx", Query<QueryMxWrap>);
  env->SetProtoMethod(channel_wrap, "queryNs", Query<QueryNsWrap>);
  env->SetProtoMethod(channel_wrap, "queryTxt", Query<QueryTxtWrap>);
  env->SetProtoMethod(channel_wrap, "querySrv", Query<QuerySrvWrap>);
  env->SetProtoMethod(channel_wrap, "queryPtr", Query<QueryPtrWrap>);
  env->SetProtoMethod(channel_wrap, "queryNaptr", Query<QueryNaptrWrap>);
  env->SetProtoMethod(channel_wrap, "querySoa", Query<QuerySoaWrap>);
  env->SetProtoMethod(channel_wrap, "getHostByAddr", Query<GetHostByAddrWrap>);

become something like this:

  env->SetProtoMethod(channel_wrap, "queryAny", Query<QueryAnyWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryA", Query<QueryAWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryAaaa", Query<QueryAaaaWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryCname", Query<QueryCnameWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryMx", Query<QueryMxWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryNs", Query<QueryNsWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryTxt", Query<QueryTxtWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "querySrv", Query<QuerySrvWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryPtr", Query<QueryPtrWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "queryNaptr", Query<QueryNaptrWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "querySoa", Query<QuerySoaWrap, ares_query>);
  env->SetProtoMethod(channel_wrap, "getHostByAddr", Query<GetHostByAddrWrap, ares_query>);

  env->SetProtoMethod(channel_wrap, "searchAny", Query<QueryAnyWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchA", Query<QueryAWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchAaaa", Query<QueryAaaaWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchCname", Query<QueryCnameWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchMx", Query<QueryMxWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchNs", Query<QueryNsWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchTxt", Query<QueryTxtWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchSrv", Query<QuerySrvWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchPtr", Query<QueryPtrWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchNaptr", Query<QueryNaptrWrap, ares_search>);
  env->SetProtoMethod(channel_wrap, "searchSoa", Query<QuerySoaWrap, ares_search>);

?

Although that kind of seems to defeat the purpose of passing in a search: true option to C++ land.

At this point, I think I need some pointers on how to pass the search: true option into (and parse for, while in) C++ land.

Copy link
Member

Choose a reason for hiding this comment

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

@SirR4T Yes, that’s what I want to avoid… The suggestion from my comment above would allow you to inspect the search option inside the Query<…> function, and then pass ares_search or ares_query depending on whether it is set or not

Copy link
Contributor Author

@SirR4T SirR4T Aug 28, 2018

Choose a reason for hiding this comment

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

yay 🎉

compiled successfully, as well as js tests (for test-dns-*) passed, and removed SearchWrap as well.

@SirR4T SirR4T force-pushed the fix19548-support-cares-search branch from 3c28a07 to d753793 Compare August 21, 2018 09:30
@silverwind
Copy link
Contributor

Is there a reason why only A and AAAA query types would get this feature?

I think we should add a options object to resolve so all query types can benefit. There was a recent request to have search domain support specifically on SRV queries.

@SirR4T SirR4T force-pushed the fix19548-support-cares-search branch 2 times, most recently from 00175e4 to 46902e3 Compare August 28, 2018 12:02
@SirR4T
Copy link
Contributor Author

SirR4T commented Aug 28, 2018

@silverwind : working on this further, now passing the search option through. Do expect to see more changes, though, as i'm still a noob in C++.

public:
QueryWrap(ChannelWrap* channel, Local<Object> req_wrap_obj, ares_function* fn)
: BaseWrap(channel, req_wrap_obj, fn) {}
};
Copy link
Member

Choose a reason for hiding this comment

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

I think we don’t need the BaseWrap layer of indirection anymore, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

obvious, once you point it out 😁

removed. now how do we provide search domains, so that we can write some meaningful tests for search: true?

Copy link
Member

Choose a reason for hiding this comment

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

@SirR4T I don’t know the feature well enough (yet?) to understand how to write good tests for it, but yes, I think we’re at that point generally. The mock DNS module might help a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to me that we will need to get the ares_channel channel->ndomains setup properly, for ares_search() to be meaningful (not default to using ares_query() instead).

As far as i can gather, there are four ways of doing this. Not sure which would be easiest to mock / replicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strike that, i was going down the wrong rabbit hole.

Here's what's working, now:

Search for "en" fails, before any changes to /etc/resolv.conf:

> dns.resolve4('en', {ttl: true, search: true}, (err, result) => { if (err) console.error(err); console.log(result); })
QueryReqWrap {
  bindingName: 'queryA',
  callback: [Function],
  hostname: 'en',
  oncomplete: [Function: onresolve],
  ttl: true,
  search: true,
  domain:
   Domain {
     domain: null,
     _events:
      { removeListener: [Function: updateExceptionCapture],
        newListener: [Function: updateExceptionCapture],
        error: [Function: debugDomainError] },
     _eventsCount: 3,
     _maxListeners: undefined,
     members: [] },
  channel:
   ChannelWrap {
     domain:
      Domain {
        domain: null,
        _events: [Object],
        _eventsCount: 3,
        _maxListeners: undefined,
        members: [] } } }
> { Error: queryA ESERVFAIL en
    at QueryReqWrap.onresolve [as oncomplete] (dns.js:197:19)
  errno: 'ESERVFAIL',
  code: 'ESERVFAIL',
  syscall: 'queryA',
  hostname: 'en' }

Same with nslookup:

$ nslookup "en"
;; Got SERVFAIL reply from 10.11.1.20, trying next server
Server:		10.11.1.19
Address:	10.11.1.19#53

** server can't find en: SERVFAIL

After adding search wikipedia.org to /etc/resolv.conf:

> dns.resolve4('en', {ttl: true, search: true}, (err, result) => { if (err) console.error(err); console.log(result); })
QueryReqWrap {
  bindingName: 'queryA',
  callback: [Function],
  hostname: 'en',
  oncomplete: [Function: onresolve],
  ttl: true,
  search: true,
  domain:
   Domain {
     domain: null,
     _events:
      { removeListener: [Function: updateExceptionCapture],
        newListener: [Function: updateExceptionCapture],
        error: [Function: debugDomainError] },
     _eventsCount: 3,
     _maxListeners: undefined,
     members: [] },
  channel:
   ChannelWrap {
     domain:
      Domain {
        domain: null,
        _events: [Object],
        _eventsCount: 3,
        _maxListeners: undefined,
        members: [] } } }
> [ { address: '103.102.166.224', ttl: 228 } ]

I can verify this against nslookup:

$ nslookup "en"
Server:		10.11.1.20
Address:	10.11.1.20#53

Non-authoritative answer:
Name:	en.wikipedia.org
Address: 103.102.166.224

Copy link
Contributor Author

@SirR4T SirR4T Aug 30, 2018

Choose a reason for hiding this comment

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

I think I'm coming across a weird behaviour too, not sure if this is a bug though.

Steps to reproduce:

  1. Start ./out/Release/node, the build result of this PR's branch
  2. Require dns module.
  3. launch this query for "en": dns.resolve4('en', {ttl: true, search: true}, (err, result) => { if (err) console.error(err); console.log(result); })
  4. Observe that ESERVFAIL error is thrown (this is expected).
  5. In a different shell, update /etc/resolv.conf to have this line at the end: search wikipedia.org
  6. Switch back to original shell, relaunch the same query
  7. Observe that ESERVFAIL is still thrown.
  8. Shutdown the repl and start again
  9. Relaunch the query, and observe that it now works!

I expect that the dns.resolve4() query should work with the latest changes in the /etc/resolv.conf file, but I suspect it is getting cached somewhere, during startup / require.

EDIT:
Found out that:

  1. c-ares caches /etc/resolv.conf once on startup, and doesn't re-read it,
  2. ares_reinit() has been on c-ares TODO list for a while now, a prerequisite to solve this issue.

So this is pretty much expected behaviour currently. Also, still need to figure out how to mock /etc/resolv.conf for tests:

  • Programmatically adding a search domain to /etc/resolv.conf is a no-no: we do not want to mess with the host system's config, not to mention privilege escalation issues and resetting it back.
  • Spawning a child process and testing for the output, after /etc/resolv.conf is somehow updated / mocked, still works, so yay.

@silverwind
Copy link
Contributor

silverwind commented Aug 28, 2018

In hindsight, it was a bad idea to add the ttl option only to A and AAAA queries, so if you could also port it over to resolve, that'd be much appreciated.

@SirR4T
Copy link
Contributor Author

SirR4T commented Aug 28, 2018

@silverwind : From the docs, it seems as if the ares_addrttl and ares_addr6ttl structs are set in the ares_parse_<type>_reply() parsing functions only when <type> is a or aaaa, respectively. I do not think this is simply portable across other query types?

@silverwind
Copy link
Contributor

silverwind commented Aug 28, 2018

You're right, now that you mention it, I recall this as a c-ares limitation. Maybe make the ttl option only apply when rrtype is A and AAAA, if that's not too much of a hassle.

edit: added it as a known limitation in #14713.

@SirR4T SirR4T force-pushed the fix19548-support-cares-search branch from a3dd868 to f6aa919 Compare August 29, 2018 05:39
@SirR4T
Copy link
Contributor Author

SirR4T commented Aug 29, 2018

we do support the options object only when using dns.resolve4() or dns.resolve6(), really. The docs reflect this too, as the options object or the ttl parameter isn't documented anywhere else...

Do you mean we should throw errors on receiving options object, for other resolveXtype()s? Currently, we silently ignore them as far as I can see:

> dns.resolveMx('nodejs.org', {ttl: true}, (err, result) => { if (err) console.error(err); console.log(result); })
QueryReqWrap {
  bindingName: 'queryMx',
  callback: [Function],
  hostname: 'nodejs.org',
  oncomplete: [Function: onresolve],
  ttl: true,
  domain:
   Domain {
     domain: null,
     _events: { error: [Function: debugDomainError] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] },
  channel: ChannelWrap {} }
> [ { exchange: 'aspmx2.googlemail.com', priority: 30 },
  { exchange: 'aspmx3.googlemail.com', priority: 30 },
  { exchange: 'aspmx.l.google.com', priority: 10 },
  { exchange: 'alt1.aspmx.l.google.com', priority: 20 },
  { exchange: 'alt2.aspmx.l.google.com', priority: 20 } ]

>

And I fear we could be breaking things by throwing, if this was already (erroneously) being used.


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

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.

@SirR4T
Copy link
Contributor Author

SirR4T commented Aug 30, 2018

more details here and here, but I'm currently stuck because:

  1. The search: true functionality kicks in, when the /etc/resolv.conf has a search example.com stanza
  2. The c-ares library caches the /etc/resolv.conf file on node process startup.
  3. So meaningful tests should, i presume:
    • Modify (or mock, within a child process: how?) the resolv.conf file to add a search example.com stanza,
    • Create and call a child_process (so that c-ares would re-read the resolv.conf file/mock),
    • assert that a search query within the child process succeeds. (For instance, with search nodejs.org stanza in the resolv.conf file, a query such as dns.resolve4('coverage', {search: true}, (err, result) => err ? console.error(err) : console.log(result) should result in the correct IP for coverage.nodejs.org (tested on local system manually, this approach works).
    • revert any changes to the resolv.conf file safely (how?)
  4. Optionally, figure out a way to do this without the need for live internet (use common.dns module, maybe).

Any pointers towards these are appreciated! cc: @addaleax @lundibundi @bnoordhuis

Copy link
Contributor Author

@SirR4T SirR4T left a comment

Choose a reason for hiding this comment

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

more of a style question.. which one is preferable?

src/cares_wrap.cc Show resolved Hide resolved
@addaleax
Copy link
Member

@SirR4T I don’t know how to tackle the /etc/resolv.conf issue without upstream changes? Maybe they would be open to accepting something like a CARES_RESOLV_CONF environment variable that we could point to a file we choose…

@SirR4T
Copy link
Contributor Author

SirR4T commented Aug 31, 2018

k, thanks. Raised an issue upstream, let's see.

@addaleax
Copy link
Member

@SirR4T Sounds good! Also, feel free to try their mailing list – I think the maintainers are a tad more active there.

@SirR4T
Copy link
Contributor Author

SirR4T commented Sep 4, 2018

This PR should probably be tagged as blocked, for now.

Blocked on:

  • upstream accepting this fix,
  • depending on timelines, releasing it,
  • patch being backported / version upgraded in to master

@jasnell jasnell added the blocked PRs that are blocked by other issues or PRs. label Sep 4, 2018
@SirR4T
Copy link
Contributor Author

SirR4T commented Sep 17, 2018

Hi @addaleax / @silverwind / @lundibundi , the upstream issue has been fixed and merged into master. Should we wait for an official release, and then upgrade the version being used here? Or can we backport the fixes in upstream here?

@addaleax
Copy link
Member

@SirR4T I’m not sure … I guess you could cherry-pick it if you like? I don’t really know how frequently c-ares does releases, but it doesn’t seem to be too often?

@SirR4T SirR4T force-pushed the fix19548-support-cares-search branch from 62cd4c6 to 0d259c9 Compare September 17, 2018 15:44
dns: copy over non-cpp changes

src: introduce BaseWrap, and refactor QueryWrap
to support SearchWrap, which shares much of the functionality
with QueryWrap.

src: cares_wrap formatted

doc: style nit for dns

test: undo unnecessary changes for test-dns.js

test: add dns tests for cares_search

Revert "src: introduce BaseWrap, and refactor QueryWrap"

This reverts commit f8c96e04d894d4af78feaa254373dcaa36990422.

src: introduce a BaseWrap class

- refactor `QueryWrap` and introduce `SearchWrap` classes, to
  use `BaseWrap` class.

doc: update yaml changes for dns.md

dns: pass whether search option is set

src: parse whether search option is set, in cares

remove class `SearchWrap`, and instead, parse within callsite
`Query` itself, whether `ares_query` or `ares_search` needs
to be used.

src: remove BaseWrap indirection

Since we do not need `SearchWrap` class anymore, we wouldn't need
a `BaseWrap` either. Fix linter issues as well.

dns: do not attach search to req object
@SirR4T SirR4T force-pushed the fix19548-support-cares-search branch from 0d259c9 to b6a828e Compare September 17, 2018 17:21
Backport upstream fix from nodejs#22226
for adding path configurability for resolv.conf file
@SirR4T
Copy link
Contributor Author

SirR4T commented Sep 17, 2018

maybe @rvagg could weigh in? Asking since i see some recent activity from him, in deps/cares.

@SirR4T
Copy link
Contributor Author

SirR4T commented Sep 17, 2018

actually, even after fixing the upstream issue I'm still not very clear how we would be able to inject mock resolv.conf files, while testing in JS land.

@silverwind
Copy link
Contributor

Any update on whether we can support this option on .resolve (specifically Resolver.prototype.resolve)? It should not be restriced to A and AAAA only.

@SirR4T
Copy link
Contributor Author

SirR4T commented Sep 23, 2018

Hi @silverwind , all queries are now supported. Specifically:

  • resolveAny
  • resolve4
  • resolve6
  • resolveCname
  • resolveMx
  • resolveNs
  • resolveTxt
  • resolveSrv
  • resolvePtr
  • resolveNaptr
  • resolveSoa
  • reverse

@apapirovski
Copy link
Member

@SirR4T would you like to follow up on this? I believe the upstream issue has been resolved and an official version update released?

also /cc @addaleax

@SirR4T
Copy link
Contributor Author

SirR4T commented Nov 4, 2018

Hi @apapirovski, I think I'm currently stuck trying to point resolv.conf file in different test cases to different fixtures. Apart from that, if you're asking for updating the cares version, that could be handled as well. Do let me know if that needs a different PR or should be handled here itself

@rvagg
Copy link
Member

rvagg commented Nov 6, 2018

@SirR4T re your commits, have a look at doc/guides/contributing/pull-requests.md for commit message formatting. Also have a look in the git history, do a regex search for deps.*cherry to see how we do upstream cherry-pick commits, you'll need to be more verbose in how you've pulled that in.

FWIW I'm +1 on cherry-picking from c-ares, they release very sporadically and c-ares/c-ares@7e6af8e looks pretty safe.

@@ -757,9 +767,9 @@ class QueryWrap : public AsyncWrap {

private:
const char* trace_name_;
ares_function* function_;
Copy link
Member

Choose a reason for hiding this comment

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

yeah, I'm not sure function_ is the best name for this variable, it's like calling something variable and will likely lead to misunderstanding. ares_function_ would be fine IMO

Copy link
Member

Choose a reason for hiding this comment

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

or ares_fn_ if you want it to stand out from the type I guess?

@addaleax
Copy link
Member

addaleax commented Nov 6, 2018

@rvagg I just landed #23854, no reason to cherry-pick anything :)

@rvagg
Copy link
Member

rvagg commented Nov 6, 2018

OK, so I can't think of a clean way to properly test this either.

Unclean ways include:

  • Adding a magic environment variable that adjusts our ares_options, but then you get into the same discussion you had over with the c-ares folks about introducing magic for no user-facing reason. I can't come up with a good justification for that (in fact, adding more magic env vars opens up minor security concerns).
  • If we could run chroot as non-root then that'd work, but alas we can't. We could add a test case that first checks for the existence of fakechroot on the system and then optionally runs the test if it exists. But then we'd have to remember to put it on as many systems as we can and we'd probably be limited to Linux (which isn't terrible but not ideal).
  • Having a test case that parses /etc/resolv.conf and extracts any search property. If one exists, run a lookup for www and www.${search value} and compare. Unfortunately this depends on both having a search property and also having a www.whatever setup. I've checked a few of our CI hosts and none of our big infra providers give us a search to work with. Some hosts do, but they're minor, like some of the ARM machines. So we'd get some coverage but not a whole lot.
  • Adjusting CI hosts via our Ansible setup scripts to insert something novel into every /etc/resolv.conf, maybe search nodejs.org and then we have full control and could do DNS lookups for whatever we wanted (make up a test value for every query type even!). The catch here is that resolv.conf is usually set by DHCP so they'd get overwritten each time the machine reboots. Setting resolv.conf to read-only is an option but would screw us if we had a necessary DNS change that came in via DHCP (although just using 1.1.1.1 would fix that potential problem I suppose). There's a bunch of other ways to override DHCP too, but every distro is different (see https://www.cyberciti.biz/faq/dhclient-etcresolvconf-hooks/ for the plethora of ways) and I don't even know what's possible on non-Linux!

Out of all of those I think the last option is most reasonable. Unfortunately that's going to require some major work on Ansible scripts which is not really a reasonable ask IMO.

Unless anyone else has any good ideas I think I'm inclined to say that we could just accept this without comprehensive tests. Some basic tests to make sure that passing search doesn't crash might have to be enough.

(also btw @addaleax I suspect that given all of the above that the c-ares update (or the floating patch) isn't really even helpful).

@rvagg
Copy link
Member

rvagg commented Nov 6, 2018

For @nodejs/tsc: there's a question here about whether the impracticality of coming up with workable test cases should dictate whether something can land. If we can't find a way to invoke the code path in question here in a reliable way within our testing infra, does that mean that we can't land this? I don't recall us having a discussion (lately anyway) about the burden of testability. Thoughts? (See my last comment here if you want details about potential paths, which are all sub-optimal).

@Trott
Copy link
Member

Trott commented Nov 6, 2018

@nodejs/build on @rvagg's testing options above.

I don't suppose the fact that we run internet tests daily on CI could somehow be leveraged, could it?

@rvagg
Copy link
Member

rvagg commented Nov 6, 2018

@Trott not really since this is about server configuration rather than workload. But perhaps we could do some basic investment in some Ansible setup to ensure that we have at least some hosts with a search pointing to nodejs.org and implement some tests that way. We could add A, AAAA, CNAME, etc. for dns-search.test_.nodejs.org or something. Maybe that's better than nothing? The tests could print out a skip if it doesn't find search nodejs.org in /etc/resolv.conf so it doesn't get completely lost to memory.

@fhinkel
Copy link
Member

fhinkel commented Oct 28, 2019

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!

@fhinkel fhinkel closed this Oct 28, 2019
@strideynet
Copy link

I think it's a shame to let this PR close off based on an inability to effectively test it. Would there be any potential opposition to @rvagg's suggested solution of using ansible scripts to insert some dummy search domain onto the test hosts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. dns Issues and PRs related to the dns subsystem. semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No DNS Search Domain support in C-Ares calls