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

 First, about the test: I was not sure what you really tried to do, so
 please first explain more details about your intent.  Please also add
 more comments to the test.  A few notes at the moment though:
 - dbNegativeCaseFind is getting too big.  Unless there's a reason
   everything is in a single test case, I suggest dividing it into
   multiple test cases.
 - creating an updater multiple times may not work as you intended for
   the `MockAccessor` (after all, it's a tiny, incomplete mock for
   testing purposes).  That may be a reason for your trouble.

 Regarding the main code, the revised version looks much cleaner
 overall.  I have still some design level concerns though.
 - find() and findAll() share almost the same code (duplicate is
   generally bad).  Also, such expensive check would be better to
   deferred until it's necessary.  At least we can defer it to
   post-delegation check in findinternal().
 - I still don't like many parts of the code have complicated condition
   like this:
 {{{#!c++
     return (ResultContext(NXDOMAIN, nsec,
                           nsec ? RESULT_NSEC_SIGNED : (need_nsec3 ?
                           RESULT_NSEC3_SIGNED : RESULT_DEFAULT)));
 }}}
   I'd unify the logic as much as possible, and keep the rest of the
   code agnostic about the DNSSEC details of the zone.
 - I don't think it a valid behavior to throw an exception when it
   finds both NSEC and NSEC3(PARAM).  It could happen when a zone
   migrates from NSEC to NSEC3 (or vice versa) using incremental
   signing.

 Considering these, my latest suggestion is as follows:

 - Introduce a helper class like this:
 {{{#!c++
 class DatabaseClient::Finder::DNSSECContext {
 public:
     // In the constructor, it identifies whether the zone is signed with
     // NSEC or NSEC3 or none.  If options doesn't have FIND_DNSSEC set, it
     // treat the zone as unsigned.
     FindDNSSECContext(DatabaseClient::Finder& finder, FindOptions
 options);

     // Return appropriate DNSSEC-related RRset fro found_set depending on
 the
     // context.
     // It's NSEC RRset if it's NSEC signed; otherwise a NULL RRSetPtr.
     ConstRRsetPtr getDNSSECRRset(FoundIterator found_set) const;

     // Same as the other version, but for the given name.
     ConstRRsetPtr getDNSSECRRset(const Name& name) const;

     // Return either RESULT_NSEC_SIGNED, RESULT_NSEC3_SIGNED, or
 RESULT_DEFAULT,
     // depending on the result of most recent getDNSSECRRset.
     FindResultFlags getResultFlags() const;
 };
 }}}
 - Instantiate `DNSSECContext` in findInternal() as follows:
 {{{#!c++
     if (dresult.rrset) {
         // In this case no special flags are needed.
         return (ResultContext(dresult.code, dresult.rrset));
     }

     // Figure out whether we need to handle DNSSEC, and if so, whether the
     // is signed and in which way (with NSEC or NSEC3).
     DNSSECContext dnssec_ctx(*this, options);
 }}}
 - pass dnssec_ctx to `findOnNameResult()` and `findNoNameResult()`.
   Use the context when DNSSEC related processing is necessary.  For
   example, the final part of `findOnNameResult()` would not be
   simplified like this:
 {{{#!c++
     ConstRRsetPtr dnssec_rrset = wild ?
 dnssec_ctx.getDNSSECRRset(*wildname) :
         dnssec_ctx.getDNSSECRRset(found);
     flags = flags | dnssec_ctx.getResultFlags();
     if (dnssec_rrset) {
         return (logAndCreateResult(name, NULL, type, NXRRSET,
 dnssec_rrset,
                                    DATASRC_DATABASE_FOUND_NXRRSET_NSEC,
 flags));
     }
     return (logAndCreateResult(name, wildname, type, NXRRSET,
 dnssec_rrset,
                                wild ? DATASRC_DATABASE_WILDCARD_NXRRSET :
                                DATASRC_DATABASE_FOUND_NXRRSET, flags));
 }}}
   (To this end, I'd rather deprecate the
     DATASRC_DATABASE_FOUND_NXRRSET_NSEC log ID, but that's a different
 topic).
 - Note that we don't need `isNSEC[3]()`and is_nsec3 variable any more.

 '''Others'''

 - The revised code still needed a lost of editorial cleanups.  In
   particular, trivial things like white space at EOL or inconsistent
   spacing policy (e.g. if (X){ or foo(xxx, yyy,zzz) are too trivial
   and it's waste of time that we need to discuss such level of things
   in every round of code review.  For space at EOL, I suggest you turn
   on git coloring and check your diff before asking review.  For
   spacing, I'm not sure if there's an effective way to avoid that
   other than carefully writing the code, but maybe you want to
   introduce some tool like detecting such things (check if there's a
   space after ',' if it's not EOL, etc).
 - comments like this should be updated, too:
 {{{#!c++
     // TODO: this part should be revised when we support NSEC3; ideally we
     // should use more effective and efficient way to identify (whether
 and)
     // in which way the zone is signed.
 }}}
 - things like isNSEC() and isNSEC3() should be private, although if
   you adopt my suggestion above this point will be moot anyway.

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