[asterisk-dev] [Code Review] 4603: pjsip: Add external PJSIP resolver implementation using core DNS API

Thu Apr 9 15:32:03 CDT 2015

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


This review can be summed up in 2 words: "NAPTR ARGH"


/trunk/include/asterisk/dns_query_set.h
<https://reviewboard.asterisk.org/r/4603/#comment25817>

    Seeing the implementation, I'd add some sort of note to this doxygen that states that the returned query is not safe to use after you decrement your reference to query_set.


/trunk/main/dns_query_set.c
<https://reviewboard.asterisk.org/r/4603/#comment25815>

    I don't think it would hurt to increase this to 4 or 5, tbh.


/trunk/main/dns_query_set.c
<https://reviewboard.asterisk.org/r/4603/#comment25816>

    A state variable should be added so that if someone attempts to add to the query set after they have called ast_dns_query_set_resolve_async(), this immediately returns an error.
    
    By doing this, you can have a guarantee that the AST_VECTOR of queries on the query set cannot change size once the resolution has begun. This is important because otherwise, when dns_query_set_callback() is called, it may never call the query_set's callback method since the vector has grown larger than the number of in-flight queries.


/trunk/main/dns_recurring.c
<https://reviewboard.asterisk.org/r/4603/#comment25818>

    I don't understand why these #includes were added. Same goes for dns_srv.c and dns_naptr.c


/trunk/res/res_pjsip/pjsip_resolver.c
<https://reviewboard.asterisk.org/r/4603/#comment25820>

    It's probably okay for it to be this way, but because of the way that PJSIP defines its transport constants, this array is larger than one might expect it to be. The indices of this array are 1, 2, 3, 129, 130, and 131.
    
    If it's not too troublesome, you may want to make this array more compact and have a function that retrieves the appropriate index in the array given a PJSIP transport type.


/trunk/res/res_pjsip/pjsip_resolver.c
<https://reviewboard.asterisk.org/r/4603/#comment25819>

    Why + 10?


/trunk/res/res_pjsip/pjsip_resolver.c
<https://reviewboard.asterisk.org/r/4603/#comment25825>

    NAPTR has a few restrictions that SRV does not have. With SRV, you sort the records and you can continue to fail over to the next one in the list, no matter the priority or weight of each individual record.
    
    With NAPTR, RFC 3403 section 4.1 places the following restriction:
    
    "The important difference between Order and Preference is that once a match is found the client MUST NOT consider records with a different Order but they MAY process records with the same Order but different Preferences"
    
    This means you need to examine the Order field of the records more closely.
    
    Defining "match" is a bit tricky but I'm going to use the NAPTR example in RFC 3264 Section 4.1 as a reference. In that example, it shows three NAPTR records for the three different SIP services provided, each with a different order. It then states:
    
    "This indicates that the server supports TLS over TCP, TCP, and UDP, in that order of preference.  Since the client supports TCP and UDP, TCP will be used, targeted to a host determined by an SRV lookup of _sip._tcp.example.com."
    
    By my reading, a "match" means that you recognize the service in the NAPTR record as being SIP-related AND that you support the transport protocol that the record pertains to.
    
    In the case of this loop, then, if you find a NAPTR record with a SIP+D2X service that you recognize and whose transport you support, you should not do anything with NAPTR records that have a higher order value than this first match. You can process records with the same order but higher preference number, though. That's kosher.


/trunk/res/res_pjsip/pjsip_resolver.c
<https://reviewboard.asterisk.org/r/4603/#comment25823>

    There is a lot of repeated code in these if statements. The variants are
    
    PJSIP_TRANSPORT_X transport
    SIP+D2X NAPTR flag
    
    This could be factored into a function of its own.


/trunk/res/res_pjsip/pjsip_resolver.c
<https://reviewboard.asterisk.org/r/4603/#comment25822>

    For all of these, you may want to double-check that ast_dns_naptr_get_replacement() is a non-zero length string.


/trunk/res/res_pjsip/pjsip_resolver.c
<https://reviewboard.asterisk.org/r/4603/#comment25821>

    RFC 3263 says:
    
    "These NAPTR records provide a mapping from a domain to the SRV record for contacting a server with the specific transport protocol in the NAPTR services field. The resource record will contain an empty regular expression and a replacement value, which is the SRV record for that particular transport protocol."
    
    Unfortunately, this is the only language in the RFC that details what the expected content of a SIP+D2X service record will be. In the test plan I'm currently writing, I'm making the declaration that any NAPTR terminal flag other than "s" is invalid for SIP+D2X records.
    
    Now, here's the part where things get really confusing. What happens if I do a NAPTR lookup for a domain in a SIP URI and I get back a record with empty flags and services (with either a regex or replacement domain)? Since no SIP records were returned, is that an error? Or does that mean I should do another NAPTR lookup on the returned record? After all, the next NAPTR lookup might give me back the SIP records I want. What if I get a mix of empty flags and empty service records AND SIP+D2X records with "s" flag? What then?


- Mark Michelson


On April 8, 2015, 7:37 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4603/
> -----------------------------------------------------------
> 
> (Updated April 8, 2015, 7:37 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24947
>     https://issues.asterisk.org/jira/browse/ASTERISK-24947
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change adds the following:
> 
> 1. A query set implementation. This is an API that allows queries to be executed in parallel and once all have completed a callback is invoked.
> 2. Unit tests for the query set implementation.
> 3. An external PJSIP resolver which uses the DNS core API to do NAPTR, SRV, AAAA, and A lookups.
> 
> For the resolver it will do NAPTR, SRV, and AAAA/A lookups in parallel. If NAPTR or SRV are available it will then do more queries. And so on. Preference is NAPTR > SRV > AAAA/A, with IPv6 preferred over IPv4. For transport it will prefer TLS > TCP > UDP if no explicit transport has been provided. Configured transports on the system are taken into account to eliminate resolved addresses which have no hope of completing.
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_dns_query_set.c PRE-CREATION 
>   /trunk/res/res_pjsip_session.c 434446 
>   /trunk/res/res_pjsip/pjsip_resolver.c PRE-CREATION 
>   /trunk/res/res_pjsip/include/res_pjsip_private.h 434446 
>   /trunk/res/res_pjsip.c 434446 
>   /trunk/main/dns_srv.c 434446 
>   /trunk/main/dns_recurring.c 434446 
>   /trunk/main/dns_query_set.c 434446 
>   /trunk/main/dns_naptr.c 434446 
>   /trunk/main/dns_core.c 434446 
>   /trunk/include/asterisk/dns_query_set.h 434446 
>   /trunk/include/asterisk/dns_internal.h 434446 
>   /trunk/include/asterisk/dns_core.h 434446 
>   /trunk/include/asterisk/autoconfig.h.in 434446 
>   /trunk/configure.ac 434446 
>   /configure UNKNOWN 
> 
> Diff: https://reviewboard.asterisk.org/r/4603/diff/
> 
> 
> Testing
> -------
> 
> Ran unit tests, they pass. Ran testsuite tests, they pass. Did spot checking using my own domains. They resolve as expected.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

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