<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/4437/">https://reviewboard.asterisk.org/r/4437/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 24th, 2015, 5:48 p.m. CST, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">First off, good page! It's pretty easy to follow the expected flow for DNS queries based on the API you've provided. From a low-level, there isn't much that's missing: you have ways of performing synchronous and asynchronous DNS lookups, and you have methods for examining the results. I have a couple of critiques of the presented API:
* ast_dns_resolve_async_recurring() is not very well defined and there are no examples illustrating its use.
* The function ast_dns_query_set_get() creates a leaky abstraction for the query set. I suggest one of the following:
* Have ast_dns_query_set_add() return an integer "token" that can be used to retrieve that record from the query set. (enthusiasm level: meh)
* Have an ast_dns_query_set iterator API to iterate over the queries in the set. (enthusiasm level: YES)
* While there are ways of querying if a result is secure, there is no current way of requesting only secure results.
>From a low-level, the one big thing that I feel is not talked about is the threading model. I know that's getting really low level, but there are some questions that came to mind, such as: if I perform multiple asynchronous DNS lookups (without using a query set) can I guarantee the results will be presented to me serially, or can the results be presented in separate threads at the same time? This can have an impact on how I write my async callbacks, especially if I pass a reference to the same user data to both async queries.
The low-level API looks fine, and it provides a lot of areas where some higher-level functions could be created. For instance:
* NAPTR has a bunch of interesting possibilities:
* Have a function to automatically perform the regex replacement and present the result to you as a string.
* Or if you want to be even lazier, have a NAPTR function that will take a NAPTR result and convert that into an ast_dns_query that you can then resolve yourself.
* Or if you want to be even lazier, have an async NAPTR function not return results until it boils down to an A or AAAA record.
* NAPTR and SRV have the potential for functions that allow for you to iterate over the different priority records that are returned.
* NAPTR and SRV could have fallback functions built into them, too.
As far as the examples are concerned, they're good, but I feel like an example that uses NAPTR (which subsequently results in an SRV lookup) could be useful. You may find that when you write the example, manual construction of further queries based on the data that you are retrieving from NAPTR records leads you to want to implement some of the suggested high-level functions for it.
Regarding your question about DNS being core or module-based, it kind of depends on a few factors:
* What code in Asterisk is going to be updated to use the new DNS API? If older code is not going to be updated to use the new DNS API, then I think that resolvers should live in loadable modules. If someone upgrades to Asterisk 15 and plans to continue using chan_sip and hasn't had issues with DNS, then they probably aren't going to be too thrilled about having to download c-ares or libunbound, only to not actually use it for anything. If old code is going to be updated to use the new DNS API, then...
* Are we planning on keeping any of the old DNS code from Asterisk around as a resolver implementation choice? If so, then again, I suggest having resolvers be separate modules. This way we still provide an upgrade path that does not have new dependencies or new underlying implementations.
My personal opinion is that all old code should be updated to use the new DNS API (without actually changing behavior, even if the old behavior is incorrect) and that the old implementation should be thrown away. If we go that route, then resolvers should be part of the core since DNS resolution is a fundamental thing in most Asterisk installations.
There is one further thing I can think of here, and that is the current dnsmgr code in Asterisk. Is that simply going to be updated to use the new DNS API, or is dnsmgr going to get some sort of overhaul to provide new functionality that the underlying DNS API makes it capable of?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">{quote}
Regarding your question about DNS being core or module-based, it kind of depends on a few factors:
* What code in Asterisk is going to be updated to use the new DNS API? If older code is not going to be updated to use the new DNS API, then I think that resolvers should live in loadable modules. If someone upgrades to Asterisk 15 and plans to continue using chan_sip and hasn't had issues with DNS, then they probably aren't going to be too thrilled about having to download c-ares or libunbound, only to not actually use it for anything. If old code is going to be updated to use the new DNS API, then...
{code}
I think we'd scope the work to only support synchronous queries and a single SRV result. Going beyond that would entail a substantial amount of work that is nice to have, but not directly relating to the DNS work itself.
Josh pointed out that we could take the existing core API and have it wrap around the new API as well. That requires no changes to consumers, and ends up providing a single core API in the end. chan_sip tends to use the SRV/DNSManager API in quite a few places (with quite a few inconsistencies) - there's quite a bit of risk in pushing it directly over to the core API.
{quote}
* Are we planning on keeping any of the old DNS code from Asterisk around as a resolver implementation choice? If so, then again, I suggest having resolvers be separate modules. This way we still provide an upgrade path that does not have new dependencies or new underlying implementations.
{quote}
DNS is so integral to so many parts of Asterisk, that I think it should simply be in the core.
That being said, Asterisk should not have to require a particular DNS library. The DNS API can act as a wrapper around a particular library choice, much like the JSON code does with libjansson. Unlike libjansson, however, we can probably operate even if the library has not been installed - in which case, we could default to getaddrinfo and return what we can.
{quote}
My personal opinion is that all old code should be updated to use the new DNS API (without actually changing behavior, even if the old behavior is incorrect) and that the old implementation should be thrown away. If we go that route, then resolvers should be part of the core since DNS resolution is a fundamental thing in most Asterisk installations.
{quote}
Agreed, although I am partial to the idea of keeping the old API and simply using it as a wrapper around the new one.
{quote}
There is one further thing I can think of here, and that is the current dnsmgr code in Asterisk. Is that simply going to be updated to use the new DNS API, or is dnsmgr going to get some sort of overhaul to provide new functionality that the underlying DNS API makes it capable of?
{quote}
Right now, I would plan on keeping it, even if its usage is not terribly useful. It's relatively pervasive in several modules, and removing it could result in a loss of functionality if the module doesn't take full advantage of the new API.</pre>
<br />
<p>- Matt</p>
<br />
<p>On February 22nd, 2015, 6:25 p.m. CST, Joshua Colp wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Joshua Colp.</div>
<p style="color: grey;"><i>Updated Feb. 22, 2015, 6:25 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A wiki page is present at:
https://wiki.asterisk.org/wiki/display/~jcolp/Asterisk+DNS+API
Which details a new DNS API for Asterisk. This exists as a thin wrapper over other resolver libraries. The core part provides a common interface and some additional features, such as NAPTR/SRV parsing and recurring lookups. Examples are provided which cover the common use cases for the API.
Some stuff to think about:
1. Does this encompass everything we think a low level API should?
2. Are there any higher level APIs that would be useful to have?
3. Is the usage intuitive and easy?
4. Are there other examples which would help?
5. Do we want resolvers to be actual modules or keep them in-core?
6. Anything else you think of
Have at it!</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've logically run through the API and examples to ensure they provide what is needed for the future, to make them as easy as possible to use, and to ensure higher level APIs can be created.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
</ul>
<p><a href="https://reviewboard.asterisk.org/r/4437/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>