BIND 10 #1298: don't do xfrin upon notify when address is unknown

[BIND 10 Development]

#1298: don't do xfrin upon notify when address is unknown
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  defect                             |             Milestone:
                   Priority:  major  |  Sprint-20111108
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 jelte]:

 > Proposal:
 > [func?] Check master address upon NOTIFY
 > b10-xfrin now checks the address a NOTIFY packet came from, and only
 > performs xfrin when the address and port match those from the master
 > address in the xfrin configuration.

 > But perhaps I need to discuss with jeremy on how to phrase this, as the
 > functionality really was already in the previous release

 We could say something like "the main fix appeared in release XXX, but
 there was deferred cleanups".  But I'd leave it to you and Jeremy.

 As for the category, I'd say it's [bug], and explain what was wrong
 before (more clearly).

 > > - As you also seem to have noticed, I'm not sure if it makes sense to
 > >   compare two tuples of address information:
 > > {{{
 > >                     if notify_addr == master_addr:
 > > }}}
 > >   Note also that we should probably consider the case where the
 > >   address matches but the ports don't.
 >
 > one thing that certainly didn't make sense is comparing the ports as
 > well, but only reporting the address on error, but the above change
 > with the format strings should at least fix the disconnect.

 I'm still not sure if tuple match is the best way.  For example, if
 and when we support IXFR/UDP, comparing socktype will cause confusion.
 But maybe it's too minor and doesn't matter in practice, so I'd leave
 it to you.

 Some other comments:

 - I fixed a minor error and made an editorial change. (btw without the
   fix it didn't pass tests)

 - format_zone_str() looks good, but now I have another suggestion:
   maybe it looks nicer if we omit the final dot in the zone name,
   i.e., instead of 'example.com./IN', 'example.com/IN'.  (but
   apparently we need to update the isc.dns wrapper so that it accepts
   the optional argument for Name.to_text()).

 - I think we need tests for format_zone_str() and format_addrinfo(),
   especially for the latter due to its complexity.

 - For the same reason for UNKNOWN_MASTER, I'd use 'info' (not error) for
   XFRIN_RETRANSFER_UNKNOWN_ZONE.
 - the number of arguments doesn't match between the message and code:
 {{{
                         logger.info(XFRIN_NOTIFY_UNKNOWN_MASTER, zone_str,
                                     notify_addr_str, master_addr_str)

 % XFRIN_NOTIFY_UNKNOWN_MASTER got notification to retransfer zone %1 from
 %2/%3, expected %4/%5
 }}}

-- 
Ticket URL: <http://bind10.isc.org/ticket/1298#comment:9>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development