Add Support for resolving DNS using search list for host-name loopkup. #19548

Manicqin · 2018-03-23T06:49:10Z

lib: dns.js - passing the search field form the options object to
QueryReqWrap object.

src:
base_object.h - added GetField function to read fields from the
persistent object (persistent QueryReqWrap object).

cares_wrap.cc - added a AresSearch function.
- QueryAWrap (resolve4) and QueryAaaaWrap (resolve6)
utilizes the AresSearch when options.search is true.

doc/api: added search option to the resolve4 and resolve6 documentation

Fixes: #17850

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

lib: dns.js - passing the search field form the options object to QueryReqWrap object. src: base_object.h - added GetField function to read fields from the persistent object (persistent QueryReqWrap object). cares_wrap.cc - added a AresSearch function. - QueryAWrap (resolve4) and QueryAaaaWrap (resolve6) utilizes the AresSearch when options.search is true. doc/api: added search option to the resolve4 and resolve6 documentation Fixes: nodejs#17850

Manicqin · 2018-03-23T06:51:08Z

Hi, How I can I write tests for the added feature.

bnoordhuis · 2018-03-23T08:14:12Z

src/base_object-inl.h

+                    v8::String::NewFromUtf8(env()->isolate(),
+                    field.c_str(),
+                    v8::NewStringType::kNormal).ToLocalChecked());
+}


Can you drop this method? There is no reason for it to live on BaseObject.

bnoordhuis · 2018-03-23T08:15:23Z

src/cares_wrap.cc

@@ -1360,7 +1368,20 @@ class QueryAWrap: public QueryWrap {
  }

  int Send(const char* name) override {
-    AresQuery(name, ns_c_in, ns_t_a);
+    auto prop_value = GetField("search");
+    bool bSearch = false;


Style: no hungarian, just (e.g.) search.

bnoordhuis · 2018-03-23T08:45:44Z

src/cares_wrap.cc

+    } else {
+      AresQuery(name, ns_c_in, ns_t_a);
+    }
+
    return 0;
  }


Rather than looking up properties this is arguably better handled by making QueryWrap a template that takes a function pointer:

template <void (*F)(const char*, int, int)> class BaseWrap { // ... void AresQuery(const char* name, int dnsclass, int type) { channel_->EnsureServers(); F(channel_->cares_channel(), name, dnsclass, type, Callback, static_cast<void*>(this)); } // ... }; using QueryWrap = BaseWrap<ares_query>; using SearchWrap = BaseWrap<ares_search>;

And then do the same to QueryAWrap and QueryAaaaWrap:

template <typename BaseT> class BaseAWrap: public BaseT { // ... - no changes needed to body }; using QueryAWrap = BaseAWrap<QueryWrap>; using SearchAWrap = BaseAWrap<SearchWrap>; // Repeat for QueryAaaaWrap

No problem I'll make the changes.
A couple of Qs.

What is the guideline when handling code on both C++ and JS, should I try to make most of the changes on the JS side?

Where will I dispatch the correct object according to the search option?
If it's in the lib side (Javascript) then I'll need to direct the resolve4 and resolve6 mapping in the resolveMap (dns.js:240) to a new js function that will call the correct object, Is that correct?

How can I Test the feature?

Re: resolveMap, that's correct.

Re: testing: I don't know. It depends too much on the configuration of the system, I think.

vsemozhetbyt · 2018-03-23T10:24:36Z

doc/api/dns.md

@@ -285,6 +285,9 @@ 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 DNS using the local domain name or the search 
+    list for host-name lookup.
+    When `true`, the resolve will use the search list which can create extra dns queries.


Nits:

dns -> DNS.

In docs, we wrap lines after 80 characters at most. Otherwise, there will be doc linting error.

the resolve will use -> the resolver will use? Or the resolve will use -> the hostname will be resolved with

vsemozhetbyt · 2018-03-23T10:25:00Z

doc/api/dns.md

@@ -310,6 +313,9 @@ 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 DNS using the local domain name or the search 
+    list for host-name lookup.
+    When `true`, the resolve will use the search list which can create extra dns queries.


Trott · 2018-03-23T20:32:44Z

doc/api/dns.md

@@ -285,6 +285,9 @@ 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 DNS using the local domain name or the search 


Resolve the DNS using -> Resolve the hostname

Trott · 2018-03-23T20:34:06Z

Hi, @Manicqin! Welcome and thanks for the PR!

BridgeAR · 2018-04-28T14:54:06Z

@Manicqin would you be so kind and try to address the comments? If you need any help / advice, please let me know.

Manicqin · 2018-05-01T07:06:29Z

@BridgeAR Hi sorry I had some work overload, I'll will try to resubmit by the weekend.

I have a couple of questions but currently the main one is: will it be ok if I split the cares_wrap.cc file into multiple files, where the cares_wrap.cc will handle the module interface and add cares_query_wrap cpp and header files that will handle the classes that are being used by the module (BaseWrap, QueryWrap implementors, etc).

bnoordhuis · 2018-05-01T10:58:53Z

I don't quite follow why that is necessary. The change I suggested isn't so big that breaking up cares_wrap.cc is needed, it's just some minor refactoring to make behavior pluggable.

(And it's just one way of doing it, a template + traits is another. But I digress.)

Manicqin · 2018-05-10T22:10:49Z

Hi.

I'm terribly sorry but I wont be able to address this issue.

bnoordhuis · 2018-06-11T19:29:12Z

I'm afraid this PR has bitrotted. I'll close it out but let me know if you want to pick it up again.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 23, 2018

bnoordhuis reviewed Mar 23, 2018

View reviewed changes

vsemozhetbyt reviewed Mar 23, 2018

View reviewed changes

vsemozhetbyt added 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. labels Mar 23, 2018

Trott reviewed Mar 23, 2018

View reviewed changes

bnoordhuis closed this Jun 11, 2018

This was referenced Aug 10, 2018

dns: support cares_search for resolution #22226

Closed

No DNS Search Domain support in C-Ares calls #17850

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for resolving DNS using search list for host-name loopkup. #19548

Add Support for resolving DNS using search list for host-name loopkup. #19548

Manicqin commented Mar 23, 2018 •

edited

Loading

Manicqin commented Mar 23, 2018 •

edited

Loading

bnoordhuis Mar 23, 2018

bnoordhuis Mar 23, 2018

bnoordhuis Mar 23, 2018

Manicqin Mar 23, 2018

Manicqin Mar 23, 2018

bnoordhuis Mar 23, 2018

vsemozhetbyt Mar 23, 2018 •

edited

Loading

Trott Mar 23, 2018

vsemozhetbyt Mar 23, 2018

Trott Mar 23, 2018

Trott commented Mar 23, 2018

BridgeAR commented Apr 28, 2018

Manicqin commented May 1, 2018

bnoordhuis commented May 1, 2018

Manicqin commented May 10, 2018

bnoordhuis commented Jun 11, 2018

Add Support for resolving DNS using search list for host-name loopkup. #19548

Add Support for resolving DNS using search list for host-name loopkup. #19548

Conversation

Manicqin commented Mar 23, 2018 • edited Loading

Checklist

Manicqin commented Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Mar 23, 2018

BridgeAR commented Apr 28, 2018

Manicqin commented May 1, 2018

bnoordhuis commented May 1, 2018

Manicqin commented May 10, 2018

bnoordhuis commented Jun 11, 2018

Manicqin commented Mar 23, 2018 •

edited

Loading

Manicqin commented Mar 23, 2018 •

edited

Loading

vsemozhetbyt Mar 23, 2018 •

edited

Loading