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

Comment (by jinmei):

 Replying to [comment:8 vorner]:

 First, about the crash in tests.

 > 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)
 > }}}

 I've found a bug in the test code and fixed it at bb38dbf.  It should
 fix this issue.  I also noticed minor typo in test names and fixed it.

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

 For now, yes.  But we'll need to be flexible soon, in the context of
 the generic parser/loader feature.  See below about passing RRs of the
 same name separately.

 > 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());
 > }}}

 Okay, fixed at commit f31f6dd.

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

 Fixed at commit 4263ba5.

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

 Right now, this would result in an exception.  But as I noted at the
 beginning of this response, we'll need to be more flexible about as
 (advanced) part of generic zone parser/loader.  We could note that
 this behavior is for expected future changes if you want, but I
 thought it's okay to mention it at this stage.

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

 I actually thought about it, but again, NSEC3 and its RRSIG can be
 passed separately (interleaved with records of other names), so at
 least we cannot simply ignore such RRSIG.  Leaving a log might be
 okay, but a warning is probably too noisy as we cannot assume in which
 order the RRs are coming.

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

 I know it may look overkill, but I generally want to make any of code
 as safe as possible.  I also understand that sometimes brevity
 outweighs safety in tests, but in this case (admittedly subjectively)
 I thought the additional complexity is acceptable.

 I think we can unify the call to destroy() and the check for memory
 leak in the destructor this way:
 {{{#!cpp
     ~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_);
         }
         // catch any leak here.
         if (mem_sgmt_.allMemoryDeallocated()) {
             ADD_FAILURE() << "Memory leak detected";
         }
     }
 }}}
 (and remove `TearDown()`).  If this is acceptable I'm okay with
 changing it this way.

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