BIND 10 #2420: allow loading zones containing an orphan RRSIG

[BIND 10 Development]

#2420: allow loading zones containing an orphan RRSIG
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20121204
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:  data   |           Sub-Project:  DNS
  source                             |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  High   |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 The size was actually quite OK (the changes were related to each other). I
 just want to confirm, the incoming RRsets need to be (still) grouped by
 the name, right?

 There's one serious thing. The tests don't pass:
 {{{
 [ RUN      ] ZoneDataUpdaterTest.rrisgForNSEC3Only
 unknown file: Failure
 C++ exception with description "Unknown NSEC3 algorithm: 200" thrown in
 the test body.
 [  FAILED  ] ZoneDataUpdaterTest.rrisgForNSEC3Only (0 ms)
 }}}

 Also, several smaller ones.

 In this diff, it looks the comment was there already, but adding the new
 line
 makes it look the comment is about the new code:
 {{{#!diff
  // Ok, we just put it in.
 +        const bool is_origin = (node == zone_data_.getOriginNode());
 }}}

 Regarding this logging:
 {{{#!diff
      LOG_DEBUG(logger, DBG_TRACE_DATA, DATASRC_MEMORY_MEM_ADD_RRSET).
 -        arg(rrset->getName()).arg(rrset->getType()).arg(zone_name_);
 +        arg(name).arg(rrtype).arg(zone_name_);
 }}}

 I think it might be confusing in case the only RRsig is added. Then it'll
 look
 like the real RRset is being added, but that one does not exist.

 I'm not sure it works with adding the RRset and RRSIG separately, as the
 comment suggests. Won't it throw that such type is already there?
 {{{
     /// At least one of \c rrset or \c sig_rrset must be non NULL.
     /// \c sig_rrset can be reasonably NULL when \c rrset is not signed in
     /// the zone; it's unusual that \c rrset is NULL, but is still
 possible
     /// if these RRsets are given separately to the loader, or if even the
     /// zone is half broken and really contains an RRSIG that doesn't have
     /// any covered RRset.  This implementation supports these cases.
 }}}

 About the „tricky case“ with NSEC3 ‒ in the reality, there is no way to
 get
 such RRSIG out, even if the adding was impemented, right? Wouldn't it be
 better
 to just warn and ignore the RRSIG? And thinking about it, would it be
 worth
 warning about the orphan RRSIGs?

 Isn't this just unneeded complexity?
 {{{#!c++
     ~ZoneDataUpdaterTest() {
         // Make sure zone data is destroyed even if a test results in
 exception
         if (zone_data_ != NULL) {
             ZoneData::destroy(mem_sgmt_, zone_data_, zclass_);
         }
     }
 }}}

 I mean, they are tests only. They terminate in a very short while, so the
 resource leak is not problematic from the point of view of eating a lot of
 memory and not using it. And if there's an exception, the tests fail
 anyway, so
 it is not needed from the point of view to have clear valgrind run
 (failure
 like failure). It looks slightly awkward to have the ZoneData::destroy
 twice,
 once in the destructor and once in TearDown. Or, could the check for
 memory
 leak be done in the destructor?

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