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-20120417
  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:25 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.
 add some cases and append some comments, change dbNegativeCaseFind to
 nsec3FlagFindDB
 > 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.
 the codes has been modified as the helper class to wrap the operations
 > '''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).
 as the discussion at jabber, my suggestion is that we write right examples
 at codingguideline,
 and we can put the examples as reference or standard, then the style
 problem will be easy to solved.
 I have adjusted my codes, hope that is OK.
 > - 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.
 > }}}
 done
 > - things like isNSEC() and isNSEC3() should be private, although if
 >   you adopt my suggestion above this point will be moot anyway.
 If these functions is called only in the Find class, I agree your
 suggestion. But these functions are invoked at the helper class now, so it
 is better to be public.

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