BIND 10 #2267: new generateRRsetFromIterator doesn't handle RRSIGs correctly

[BIND 10 Development]

#2267: new generateRRsetFromIterator doesn't handle RRSIGs correctly
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:
  jinmei                             |                Status:  new
                       Type:         |             Milestone:
  defect                             |  Sprint-20120925
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:  data   |           Sub-Project:  DNS
  source                             |  Estimated Difficulty:  6
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Description changed by jinmei:

Old description:

> The new (memory-efficient) version of in memory loader from another
> data source has a bug.  It cannot load RRSIGs correctly if a single
> name has multiple types of RRsets and their RRSIGs.
>
> It can be confirmed by applying the following patch to the unit test.
> load() will fail with an exception.
>
> {{{#!diff
>  const char* rrset_data[] = {
>      "example.org. 3600 IN SOA   ns1.example.org. bugs.x.w.example.org.
> 68 3600 300 3600000 3600",
>      "a.example.org.                     3600 IN A      192.168.0.1",
> +    "a.example.org. 3600 IN RRSIG A 5 3 3600 20150420235959
> 20051021000000 "
> +    "40430 example.org. FAKEFAKEFAKE",
>      "a.example.org.                     3600 IN MX     10
> mail.example.org.",
>      NULL
>  };
> }}}
>
> Even ignoring this bug, the current implementation doesn't seem to be
> very understandable due to the last_rrset_ member variable.  It's
> quite difficult to keep track of this thing.
>
> My suggested fix is this:
>
> - simplify generateRRsetFromIterator() so it now just passes each
>   RRset in the returned order.
> - implement the buffer logic in `InMemoryClientImpl::addFromLoad()`.
>   it ensures a matched pair of RRset and (if signed) its RRSIG RRset
>   is generated.  (it can assume if the different names of RRsets aren't
>   interleaved.)  To implement this, it's probably better to introduce
>   a helper `Loader` class.
> - revise `InMemoryClientImpl::addFromLoad()` so it takes RRset and its
>   RRSIG.  Then we won't need the `last_rrset_` magic.
> - `InMemoryClient::add()` (and maybe some tests) will need to be updated.
>
> Also, we should merge the older loadFromIterator test.
>
> I'd also deprecate the impl class.  The in-memory client is now almost
> private itself, so the advantages of pimpl aren't that big.

New description:

 The new (memory-efficient) version of in memory loader from another
 data source has a bug.  It cannot load RRSIGs correctly if a single
 name has multiple types of RRsets and their RRSIGs.

 It can be confirmed by applying the following patch to the unit test.
 load() will fail with an exception.

 {{{#!diff
  const char* rrset_data[] = {
      "example.org. 3600 IN SOA   ns1.example.org. bugs.x.w.example.org. 68
 3600 300 3600000 3600",
      "a.example.org.                     3600 IN A      192.168.0.1",
 +    "a.example.org. 3600 IN RRSIG A 5 3 3600 20150420235959
 20051021000000 "
 +    "40430 example.org. FAKEFAKEFAKE",
      "a.example.org.                     3600 IN MX     10
 mail.example.org.",
      NULL
  };
 }}}

 Even ignoring this bug, the current implementation doesn't seem to be
 very understandable due to the last_rrset_ member variable.  It's
 quite difficult to keep track of this thing.

 My suggested fix is this:

 - simplify generateRRsetFromIterator() so it now just passes each
   RRset in the returned order.
 - implement the buffer logic in `InMemoryClientImpl::addFromLoad()`.
   it ensures a matched pair of RRset and (if signed) its RRSIG RRset
   is generated.  (it can assume if the different names of RRsets aren't
   interleaved.)  To implement this, it's probably better to introduce
   a helper `Loader` class.
 - revise `InMemoryClientImpl::addFromLoad()` so it takes RRset and its
   RRSIG.  Then we won't need the `last_rrset_` magic.
 - `InMemoryClient::add()` (and maybe some tests) will need to be updated.

 Also, we should merge the older loadFromIterator test.

--

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