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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions doc/api/dns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Resolve the DNS using -> Resolve the hostname

list for host-name lookup.
When `true`, the resolve will use the search list which can create extra dns queries.
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Nits:

  1. dns -> DNS.
  2. In docs, we wrap lines after 80 characters at most. Otherwise, there will be doc linting error.

Copy link
Member

Choose a reason for hiding this comment

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

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

- `callback` {Function}
- `err` {Error}
- `addresses` {string[] | Object[]}
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

The same)

- `callback` {Function}
- `err` {Error}
- `addresses` {string[] | Object[]}
Expand Down
1 change: 1 addition & 0 deletions lib/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,7 @@ function resolver(bindingName) {
req.hostname = name;
req.oncomplete = onresolve;
req.ttl = !!(options && options.ttl);
req.search = !!(options && options.search);
var err = this._handle[bindingName](req, name);
if (err) throw dnsException(err, bindingName);
return req;
Expand Down
9 changes: 9 additions & 0 deletions src/base_object-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ inline v8::Local<v8::Object> BaseObject::object() {
}


inline v8::MaybeLocal<v8::Value> BaseObject::GetField(
std::string const& field) {
return object()->Get(env()->context(),
v8::String::NewFromUtf8(env()->isolate(),
field.c_str(),
v8::NewStringType::kNormal).ToLocalChecked());
}
Copy link
Member

Choose a reason for hiding this comment

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

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



inline Environment* BaseObject::env() const {
return env_;
}
Expand Down
2 changes: 2 additions & 0 deletions src/base_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ class BaseObject {
// persistent.IsEmpty() is true.
inline v8::Local<v8::Object> object();

inline v8::MaybeLocal<v8::Value> GetField(std::string const& field);

inline Persistent<v8::Object>& persistent();

inline Environment* env() const;
Expand Down
38 changes: 36 additions & 2 deletions src/cares_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,14 @@ class QueryWrap : public AsyncWrap {
static_cast<void*>(this));
}

void AresSearch(const char* name,
int dnsclass,
int type) {
channel_->EnsureServers();
ares_search(channel_->cares_channel(), name, dnsclass, type, Callback,
static_cast<void*>(this));
}

static void CaresAsyncClose(uv_handle_t* handle) {
uv_async_t* async = reinterpret_cast<uv_async_t*>(handle);
auto data = static_cast<struct CaresAsyncData*>(async->data);
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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


if (!prop_value.IsEmpty()) {
auto search_prop = prop_value.ToLocalChecked()->ToBoolean();
bSearch = search_prop->Value();
}

if (bSearch) {
AresSearch(name, ns_c_in, ns_t_a);
} else {
AresQuery(name, ns_c_in, ns_t_a);
}

return 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

  • How can I Test the feature?

Copy link
Member

Choose a reason for hiding this comment

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

Re: resolveMap, that's correct.

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


Expand Down Expand Up @@ -1404,7 +1425,20 @@ class QueryAaaaWrap: public QueryWrap {
}

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

if (!prop_value.IsEmpty()) {
auto search_prop = prop_value.ToLocalChecked()->ToBoolean();
bSearch = search_prop->Value();
}

if (bSearch) {
AresSearch(name, ns_c_in, ns_t_a);
} else {
AresQuery(name, ns_c_in, ns_t_a);
}

return 0;
}

Expand Down
4 changes: 2 additions & 2 deletions test/internet/test-dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ TEST(function test_reverse_bogus(done) {

TEST(function test_resolve4_ttl(done) {
const req = dns.resolve4(addresses.INET4_HOST, {
ttl: true
ttl: true, search: false
}, function(err, result) {
assert.ifError(err);
assert.ok(result.length > 0);
Expand All @@ -99,7 +99,7 @@ TEST(function test_resolve4_ttl(done) {

TEST(function test_resolve6_ttl(done) {
const req = dns.resolve6(addresses.INET6_HOST, {
ttl: true
ttl: true, search: false
}, function(err, result) {
assert.ifError(err);
assert.ok(result.length > 0);
Expand Down