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

 Replying to [comment:11 jinmei]:
 > 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.

 Yes, it passes now.

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

 I think the documentation should match the behaviour as much as we can
 make it.
 It is quite frustrating when one writes a code that should be correct by
 the
 documentation, but it doesn't work. After all, it is said that a bug in
 documentation is worth two in the code.

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

 But currently such scenario would throw anyway. I'm not saying this would
 be
 for the long-term, since there'll be some substantial changes for merging
 RRsets. I thought for now. But if you still think it is not worth it, then
 we
 can probably leave it as it is.

 > 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_);
 >         }
 > 9// 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.

 I think this would look better.

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