BIND 10 #1579: Update database ZoneFinder::find() for negative cases of NSEC3-signed zones

[BIND 10 Development]

#1579: Update database ZoneFinder::find() for negative cases of NSEC3-signed zones
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  haikuo
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120320
  medium                             |            Resolution:
                  Component:  data   |             Sensitive:  0
  source                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by haikuo):

 Replying to [comment:15 jinmei]:
 > Replying to [comment:14 haikuo]:
 > > If I do not create #1785, I have to update NSEC3PARAM type RRset to
 database resource.It looks weird if database resource has only NSEC3PARAM
 except other NSEC3 type.
 >
 > For the purpose of this ticket, IMO that doesn't matter.  Surely the
 > zone is broken if it only has NSEC3PARAM but no NSEC3, but that's a
 > concern of the higher layer.  Even if we already had the ability of
 > having NSEC3s in the zone, I wouldn't bother to add them for this
 > test, because it'd just unnecessarily complicate the test case.  But
 > more important point is that we should not allow an easy excuse of not
 > adding tests.  It should be a very high bar; otherwise we lazy
 > programmers tend to skip writing tests.
 >
 > Anyway,
 >
 > > In order to test this ticket, I have updated NSEC3PARAM to the unit
 test in the database resource. you can review it now.
 > > I will close #1785 if the unit test is OK
 >
 > The added test itself looks okay (although it's not sufficient, see
 > below).
 >
 > Here are review comments:
 >
 > First note: before trying to address these, please first add relevant
 > tests.
 >
 > '''bigger issues'''
 >
 > - I just noticed the ticket summary was not clear enough (sorry about
 >   that): We need to set the NSEC3 flag for wildcard cases (even if
 >   they are not "negative"), too.  Even if we forget wildcard itself,
 >   we still need to cover the case where wildcard attempt fails with
 >   NXDOMAIN (see findWildcardMatch).
 >
 Jinmei, you said we should set the NSEC3 flag whenever the wildcard cases
 is "negative".
 why don't we append NSEC3 flag to the result for all cases if the zone is
 signed by NSEC3 in find() function? and that is better for upper layer's
 analysis.

 > - Partly for this reason, I don't think it a good approach to try to
 >   find NSEC3PARAM in findNoNameResult().  It's also a waste to do this
 >   if DNSSEC isn't required.  What I'd do is to extract this process
 >   into a separate thread to share it from multiple cases, and use it
 >   only when DNSSEC is requested.  Also, I'd make sure this process
 >   doesn't happen multiple times in a single call of find().
 >
 > - code like this doesn't seem to be very clean:
 > {{{#!c++
 >     const ConstRRsetPtr nsec = dnssec_data ? findNSECCover(name) :
 >         ConstRRsetPtr();
 >     if (is_nsec3 == true) {
 >         return (ResultContext(NXDOMAIN, nsec,
 >                               nsec ? RESULT_NSEC_SIGNED :
 RESULT_NSEC3_SIGNED));
 >     } else {
 >         return (ResultContext(NXDOMAIN, nsec,
 >                               nsec ? RESULT_NSEC_SIGNED :
 RESULT_DEFAULT));
 >     }
 > }}}
 >   If we know it's an NSEC3-signed zone, I wouldn't bother to look for
 >   NSEC because it's a waste.  I'd introduce a helper method or
 >   something to unify such cases in an efficient way.  Ideally,
 >   - If DNSSEC isn't required, do nothing
 >   - Otherwise, check if the zone is NSEC3-signed.  If so, just set the
 >     NSEC3 flag.  Don't bother to find NSEC.
 >   - Otherwise, find NSEC.  If found, set the NSEC flag and return the
 >     NSEC.  If NSEC isn't found, I'd just set the NSEC flag and return
 >     nothing.
 >
 > - Test isn't sufficient.  We need to also cover the NXRRSET case (and
 >   if we handle wildcard cases, such cases too).  In general, when you
 >   add any code that part of the code must be covered by a test.  TDD
 >   would ensure this almost by design, but even if you didn't start
 >   from a test you could use other tools such as lcov.  That would be
 >   especially helpful if you are not so familiar with writing tests
 >   with developing something.
 >
 > '''smaller issues'''
 >
 > - It's a waste to create nsec3PARAM every time.  I'd suggest doing
 >   something similar to NSEC_TYPES().
 >
 > - There are some editorial/style issues.  I've fixed them directly on
 >   the branch and pushed them.  Some cases are noted in the coding
 >   guideline page (on trac).  If you didn't please check that.
 >

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