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

 - 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:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development