BIND 10 #1576: implement ZoneFinder::findNSEC3 in in-memory data source

[BIND 10 Development]

#1576: implement ZoneFinder::findNSEC3 in in-memory data source
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20120221
                  Component:  data   |            Resolution:
  source                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  6
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:11 vorner]:

 Thanks for the review.

 > First, it fails to compile on my system:
 > {{{
 > rc_unittest.cc:313:22: error: declaration of
 ‘<unnamed>::TestNSEC3HashCreator::TestNSEC3Hash::NSEC3HashMap
 <unnamed>::TestNSEC3HashCreator::TestNSEC3Hash::map’
 > /usr/lib/gcc/x86_64-pc-linux-
 gnu/4.5.3/include/g++-v4/bits/stl_map.h:87:5:
 > error: changes meaning of ‘map’ from ‘class std::map<isc::dns::Name,
 std::basic_string<char> >’
 > }}}

 Oh, okay, I cannot reproduce it, but I've changed the variable name to
 reduce the risk of conflicts.

 > I'm not sure I like the NSAC3HashCreator approach. If it is for testing
 only, it looks like an overkill, introducing yet another abstract base
 class, which is 2 already for the simple thing of calculating a hash. If
 it was to provide some flexibility, it would be OK, but it doesn't. If you
 replace the creator for some experimental hash algorithm, you lose the old
 one. Not to say that this wouldn't work if two people provided two plugins
 for two different algorithms.

 I see points of the overkill argument.  I also didn't like relying on
 the essentially singleton stuff.  But the alternative would be to
 add a some kind of hook in `InMemoryZoneFinder` so the user of it (in
 this case the unit tests) can specify a possibly specialized
 `NSEC3Hash` object, just like we pass `AbstractSession` to
 `ModuleCCSession` even though the only reason for using the base class
 is convenience for tests.  Since the hash calculation logic is
 basically static in practice, however, I didn't like adding a
 tweakable knob for that purpose either (my assumption is that if
 someone wants to play with experimental hash calculator, possibly for
 using a new/experimental hash algorithm, they'll replace the creator
 at the beginning of their application and use it all the time).  Hence
 the current approach.

 > Would it be possible to keep a map from algorithm to creator? Then the
 tests could use some non-existent protocol for the fake hashes and coexist
 peacefully with the rest and it would provide the flexibility (eg. be able
 to add as mane creator as there are algorithms).

 I suspect it's too early to decide how we map application's conditions
 to specific factory.  It may or may not be a switch of hash algorithm
 (as I said before my basic assumption is to (be able to) use the same
 NSEC3Hash object for different algorithms, so switching per algorithm
 is not the most sensible approach).  We could even make it more
 generic, such as passing some opaque blob to the factory, but I
 suspect it's also over generalization at the moment.

 In the end, if you don't like the current creator design, my next
 proposal is to add a new `InMemoryZoneFinder::setNSEC3Hash()` method,
 which tells the finder to use the given hash object regardless of
 other parameters of NSEC3 related RRs (noting it's only for testing
 purposes).  It's ugly, but we can then eliminate the reliance on the
 singleton creator.

 > Is there any reason why the destructor of InMemoryZoneFinderTest is
 virtual?

 I thought the destructor for its base class (in gtest lib) was also
 virtual, and followed the convention we use in other cases.  But we
 probably don't do this for tests too often, so I'm okay with removing
 it if it looks too awkward.  In any case, the behavior wouldn't change
 with or without it.

 > This comment looks odd in this context. Why is it `EXPECT_THROW` if the
 result is guaranteed?
 > {{{#!c++
 >     // Unless NSEC3 for apex isn't added the result in the recursive
 mode
 >     // is guaranteed.
 >     const string ns1_nsec3_text = string(ns1_hash) + ".example.org." +
 >         string(nsec3_common);
 >     EXPECT_EQ(result::SUCCESS,
 zone_finder_.add(textToRRset(ns1_nsec3_text)));
 >     EXPECT_THROW(zone_finder_.findNSEC3(Name("www.example.org"), true),
 >                  DataSourceError);
 > }}}

 Ah, yes, it was broken.  Fixed.

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