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

 Replying to [comment:27 haikuo]:

 > > - The revised code still needed a lost of editorial cleanups.  In
 > 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.

 There are still some:-) see below.  (although I know we cannot be
 perfect for this type of thing)

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

 No, the helper class is a part of the main finder class, so they don't
 have to be public.  But this point is minor based on the overall
 comments below - so please forget this specific point.

 '''About the general design'''

 - The main code still calls things like isNSEC()/isNSEC3().  It's
   not what I expected by the introduction of the helper class; I
   wanted to make the main part as DNSSEC-agnostic as possible.  If the
   main code needs to call methods like `isNSEC`, which is very DNSSEC
   specific, something (design or implementation, or both) is wrong.

 - Also, `FindDNSSECContext` seems to contain things that are
   irrelevant for its main responsibility, like is_origin or non DNSSEC
   options.  In general, I would like to design a class to focus on a
   small set of responsibilities.  Mixing other things into a class
   that may happen to be used along with the use of the class will make
   the class and the resulting code quite unreadable.

 - as for what should happen when NSEC is expected but not found: it's
   a quite broken case so it's probably up to the implementation.  But
   I'd personally prefer just returning NULL in this case.  At least
   throwing an exception isn't aligned with this comment:
 {{{#!cpp
     // If an NSEC3PARAM RR exists at the zone apex, it's quite likely that
     // the zone is signed with NSEC3.  (If not the zone is more or less
 broken,
     // but it's caller's responsibility how to handle such cases).
 }}}
   Also, this comment in the test doesn't make sense if we throw
   `DataSourceError`:
 {{{#!cpp
     // Check that if the DB doesn't support it, the exception from there
     // is not propagated and it only does not include the NSEC
 }}}
   Furthermore, the policy on this point isn't consistent in this
   implementation in that it doesn't throw from getNSECRRset() when it
   expects NSEC but cannot find it.  So, this implementation is just
   incomplete at best.

   Actually, as mentioned above, I'd now suggest handling this case as
   non fatal.  I'm not sure in which case this practically happens, but
   we may have such a situation if and when we support incremental zone
   signing.  Also, this behavior is consistent with BIND 9.

 We've spent a quite a lot of time for this ticket, and apparently I've
 not been very good at explaining my intent.  To this end, please let
 me explain my points with specific complete code: I've created a
 separate branch based on a recent version of trac1579,
 named "trac1579suggest", and made updates fixing all issues I'd otherwise
 point out in the form of review comments.  I made pretty detailed
 commit logs, so it should work as an alternate form of review
 comments.

 Please see the branch as my review comments.  If you agree with the
 points and the changes. please simply merge it, and we're done; if you
 agree with the points but have an issue about specific points of the
 changes, please make your updates on it, either continuing on the
 separate branch or by merging it to the original trac1579, and give
 the ticket back go me; if you don't even agree with the points of my
 comments, then, hmm, we can discuss it:-)

 '''About tests'''

 The latest version has been very much improved, but I still see
 issues:
 - it repeats mostly identical patterns for both NSEC and NSEC3.
 - it doesn't test the case where the zone only has NSEC3PARAM.
 - it doesn't test for the result of findAll().
 - it doesn't test the case for empty names (either for normal name or
 wildcard)

 I've addressed these points in trac1579suggest.  See the latest 3
 commits.

 '''Some minor, general points'''

 - as you probably noticed, there are still quite a lot of editorial
   issues.  Some of them are quite trivial and should be easily
   identified, e.g., if you used a tool, such as spacing policy like
   `if (X){` or `foo(xxx, yyy,zzz)`, and still repeat after the
   previous comment.  Style issues are mostly for consistency, rather
   than good vs bad, so I'm not saying your style is bad, but it's not
   consistent with the rest of our code, and we must be consistent.
   If you are too familiar with different styles to avoid that in
   coding, I'd strongly suggest introducing a grep-like checker tool
   and running it before asking for review.  The need for discussing
   this level of things is unfortunate for both the developers and the
   reviewer.

 - In your changes, unrelated files were updated (more accurately,
   "broken").  Jeremy fixed one thing at commit 77b918b.  Another one
   is this which I fixed at ed4c07d:
 {{{#!diff
 -        // Does the domain have a subdomain (i.e. it is an empty non-
 terminal)
 +        // Does the domain have a subdomain (i.e. it is an empty non-
 terminal)?
 }}}
   Such errors (especially the one fixed at 77b918b, which broke a
   totally unrelated file) should have been able to be avoided if you
   could check your diff before commit.  I suggest you do this check
   before making a change to the repository.

 '''Minor comments on the latest trac1579 branch'''

 These are now probably moot, as I've addressed these in the suggested
 branch.

 - this should be `&&`?
     if (wild & dnssec_ctx.isNSEC3()) {
         flags = (flags | RESULT_NSEC3_SIGNED);
     }

 - IMO, finderp_ should better be a reference than a pointer if we'd
   worry about the case where it's NULL later on, like getNSECRRset
   does (which shouldn't never happen in our usage).

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