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      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:9 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).
 >

 [bug] jelte
 b10-xfrin would previously initiate incoming transfers upon receiving
 NOTIFY packets from any address (if the zone was known to b10-xfrin).
 It now only starts a transfer if the source address from the NOTIFY
 packet matches the configured master address and port. This was really
 already fixed in release bind10-devel-20111014, but there were some
 deferred cleanups to add.

 > > > - 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.
 >

 ack, only comparing family, addr and port now. BTW this seems more
 like a big hint that we are using bad datastructures here, but anyway.

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

 ohright, how did that get in. Thanks.

 > - 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()).
 >

 good idea. Added wrapper support (in a separate commit,
 23cfc5b4d9b384172d0eadd2269ed6a6121966a8)

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

 added some.

 > - For the same reason for UNKNOWN_MASTER, I'd use 'info' (not error) for
 >   XFRIN_RETRANSFER_UNKNOWN_ZONE.

 oh right, done

 > - 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
 > }}}

 ah, doh, forgot to update after deciding to go ahead with the format
 functions. Fixed. (yes we still need that checking script :p)

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