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:  jelte
  jinmei                             |                Status:  closed
                       Type:         |             Milestone:
  defect                             |  Sprint-20120403
                   Priority:         |            Resolution:  fixed
  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):

 * status:  reviewing => closed
 * resolution:   => fixed


Comment:

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


 oh, i was not suggesting we change it, but the problem is that through all
 the templating and internal error handling of gtest, i found it quite hard
 to find where the exception was actually thrown.

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

 ack

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

 Yeah should indeed be ZoneFinder. I do think NXDOMAIN is wrong enough to
 consider it a bug :)

 > About the revised code:
 >
 > - I made a couple of editorial cleanups.

 I suspect you forgot to push this... I'm gonna apply your proposed patch
 and merge it now anyway (so i can see if any test build fails during the
 rest of my day). I assume these changes are trivial enough to go directly
 to master later :)

 > - I'd suggest changing the exception for `findNSEC3` against
 >   out-of-zone to `OutOfZone` too.  Suggested patch is attached.
 >

 Ack, applied.

 > Otherwise the branch looks okay for merge.

 Done

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