-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Reinforce DNS reverse resolution test #3113
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
Reinforce DNS reverse resolution test #3113
Conversation
|
It's not my localhost problem, github CI also suffers. The branch that changes nothing, and the CI job on it that fails |
tests/unit/dns_test.cc
Outdated
| hostent e = co_await d.get_host_by_name(seastar_name, inet_address::family::INET); | ||
| hostent a = co_await d.get_host_by_addr(e.addr_list.front()); | ||
|
|
||
| try_again: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest:
while (!hostnames.empty()) {
try {
auto d = ...;
break;
} catch(...)
...
continue;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, reimplemented with a for-loop
Signed-off-by: Pavel Emelyanov <[email protected]>
In case reverse resolution fails, try again with another host name. At least one of them should succeed. Signed-off-by: Pavel Emelyanov <[email protected]>
da4c487 to
7e0f288
Compare
|
upd:
|
|
It might be a good idea to use these domains: $ dig +short one.one.one.one
1.0.0.1
1.1.1.1
$ dig +short dns.google
8.8.4.4
8.8.8.8
$ dig +short localhost
127.0.0.1 |
| hostent a = co_await d.get_host_by_addr(e.addr_list.front()); | ||
| hostent e2 = co_await d.get_host_by_name(a.names.front(), inet_address::family::INET); | ||
| BOOST_REQUIRE(std::count(e2.addr_list.begin(), e2.addr_list.end(), e.addr_list.front())); | ||
| co_await d.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an accurate translation, we lose d.close() on exception. And since you do expect failures, we shouldn't leak here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failures that are "expected" don't lose close -- the d is created once before loop, and in case loop continues it re-uses that single resolver. Exceptions that are not caught make test fail anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha
|
IMO, a better approach is to set up local dns with dnsmasq as a fixture so we have reliable infrastructure. But I don't known if it's worth the effort. |
The test does three resolutions in a row -- hostname to address, then address to hostname and finally the reverse-resolved name back to address. The last list of IP-s is checked to contain front IP from the first list. The piggy host for the fun is seastar.io.
Sometimes, reverse resolution fails. E.g. 2b51022 changed the name from www.google.com to seastar.io with the explanation that google hostname's address cannot be resolved back. Now it happened with seastar.io (and www.google.com healed back). The proposal is to try several hostnames in a row, and tolerate the reverse lookup failure.