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

Comment (by jinmei):

 Replying to [comment:12 jelte]:

 > > '''memory_datasrc.cc'''

 > 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

 Ah, yes, sorry, I was aware of that but forgot to mention it in the
 comment.

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

 In which sense?  Compared to typed tests?  I understand parameterized
 tests (whether it's TYPED_TEST or TEST_P) could make things more
 complicated in general.  It's a tradeoff between reusability and
 complexity due to the additional layer, and I think the former
 generally outweighs the latter.

 If you mean it should be easier to handle with TYPED_TEST, for
 example, and a specific suggestion of rewriting the tests, I have no
 problem about that.  I don't remember why I chose parameterized tests
 instead of typed tests for that - I think I saw some difficulty with
 the latter, but now I simply don't remember.  In any event that would
 be a subject of different task.

 > > - 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 :)

 Ah, okay, I missed the additional *.different.zone.

 Now we have update APIs, we should probably avoid using binary DB
 files as the primary source of test data but generate them from
 human-readable source (such as textual zone files) at build or run
 time.  But that will be a subject of different task.

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

 I'm not sure this is a "bug", but I'd leave it to you (I see we could
 call it a bug in that at least NXDOMAIN wasn't a good response).  The
 second 'zonefinder' should probably be `ZoneFinder`.

 About the revised code:

 - I made a couple of editorial cleanups.
 - I'd suggest changing the exception for `findNSEC3` against
   out-of-zone to `OutOfZone` too.  Suggested patch is attached.

 Otherwise the branch looks okay for merge.

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