BIND 10 #1535: notify_out shouldn't look for NS addresses for out of zone NS names

[BIND 10 Development]

#1535: notify_out shouldn't look for NS addresses for out of zone NS names
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120403
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  DNS
  xfrout                             |  Estimated Difficulty:  3
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by jelte):

 * owner:  jelte => jinmei


Comment:

 Replying to [comment:10 jinmei]:
 > '''database.cc'''
 > - you don't have to explicitly call toText() for getOrigin():
 > {{{#!c++
 >         isc_throw(OutOfZoneFind, name.toText() << " not in " <<
 >                                  getOrigin().toText());
 > }}}
 >   same for other similar cases.
 >

 ack, fixed

 > '''memory_datasrc.cc'''
 >
 > - I'd catch this in findNode().  If I understand it correctly, if at
 >   the end of this method it's not either EXACTMATCH or PARTIALMATCH
 >   it's out-of-zone.  I prefer this because we can integrate the
 >   out-of-zone check in the normal lookup.  Since it's generally better
 >   to avoid any overhead in the lookups path for the in-memory zone for
 >   performance, and since the out-of-zone case should be exceptional,
 >   I'd like to minimize the overhead.

 it's exceptional, but not for findNode() in all cases atm; it is also used
 in addAdditional processing during loading (where out-of-zone data is
 pretty common)

 we could catch the exception there, instead of what I did now (copied that
 original check whether it's a match or subdomain to addAdditional, and
 skip the lookup if it's not in or below the domain), but in this case it
 is quite expected, and hence it seems cleaner to me to check before
 calling findNode in addAdditional

 (btw, debugging exceptions thrown in parameterized gtests constructors is
 a PITA)

 > - regarding the performance, I'd also like to do benchmark tests to
 >   measure the overhead of this check.
 >

 well, it's been removed now :)

 > '''database_unittest.cc'''
 >
 > - ZoneFinder::FIND_DEFAULT can be omitted for ZoneFinder::find().
 >   same for memory_datasrc_unittest.cc, and (I believe) datasrc_test.py.

 ack, removed

 > - this comment should be revised:
 > {{{#!c++
 >         // Note: find() rejects out-of-zone query name with NXDOMAIN
 >         // regardless of whether adding the RR succeeded, so this check
 >         // actually doesn't confirm it.
 > }}}
 >   (there may be other similar cases.  maybe you want to check them by
 >   searching for "NXDOMAIN" throughout the code).
 >

 if found a couple, hopefully that's all of them

 > '''zone.h'''
 > - `OutOfZoneFind` maybe renamed to `OutOfZone`, merged with in-memory
 >   specific exception of that name, and for an error about out-of-zone
 >   data in general?  Not a strong opinion, just an idea.
 >

 Done (second commit)

 > '''notify_out.py'''
 >
 > Code looks okay, but I think we could (should) do some tests:
 >
 > - add a test zone that contains out-of-zone NS names (from a quick
 >   look there's no such case right now, right?).  Confirm that it
 >   results in exception for the current version of notify_out.py, and
 >   that it doesn't happen for the revised version.

 three should be, but it's hidden in the sqlite3 data blob; i had added 2
 NS records to different zones to example.com. Since these aren't returned
 the test itself doesn't show that :)

 but if you revert to the original notify_out.py this should show. (or you
 can see directly with sqlite3 select * from records where rdata="NS")

 > - also add a test zone that contains out-of-zone NS names whose zone
 >   is however also an authoritative zone of the server.  check that
 >   notify_out now correctly include these addresses too.
 >

 ah, did not have that one; removed the b.dns.example.com records, and
 updated b.dns.example.com NS to have rdata b.dns.example.net instead of
 b.dns.example.com
 (this was the smallest difference to get the desired results; the two
 zones share a lot of data in terms of addresses)

 > '''Others'''
 >
 > - I made a couple of editorial changes.

 thnx

 > - I think we need a changelog for this task.  Since it involves API
 >   change, we'd need '*'.

 quite. Perhaps it even needs 2 (one for the API change, one for the change
 in behaviour of notify_out)

 [bug]*
 The implementations of Zonefinder::find() now throw an OutOfZone exception
 when the name argument is not in or below the zone this zonefinder
 contains.

 and
 [bug]
 The notify-out code now looks up notify targets in their correct zones
 (and no longer just in the zone that the notify is about).

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