[asterisk-dev] [Code Review] 4437: dns: Define a core DNS API with examples.

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4437/#review14539
-----------------------------------------------------------


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?

- Mark Michelson


On Feb. 23, 2015, 12:25 a.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4437/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2015, 12:25 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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!
> 
> 
> Diffs
> -----
> 
> 
> Diff: https://reviewboard.asterisk.org/r/4437/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150224/426c5a38/attachment-0001.html>