BIND 10 #534: DNAME support in MemoryZone

[BIND 10 Development]

#534: DNAME support in MemoryZone
-------------------------------------+-------------------------------------
                 Reporter:  vorner   |                Owner:  jinmei
                     Type:  task     |               Status:  reviewing
                 Priority:  major    |            Milestone:  A-Team-
                Component:  data     |  Sprint-20110209
  source                             |           Resolution:
                 Keywords:           |            Sensitive:  0
Estimated Number of Hours:  3.0      |  Add Hours to Ticket:  0
                Billable?:  1        |          Total Hours:  0
                Internal?:  0        |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:5 vorner]:

 (I'm okaying the points I'm not explicitly responding to below.)

 > > this function is now quite big and getting unreadable.  I guess we
 > > might want to move the CNAME/DNAME specific checks in the middle of
 > > the function to a separate helper method.
 >
 > Done

 Okay, but I found the comment about exception guarantee should now better
 be in find().  I also found the new function can/should be a const member
 function.  I've made these changes locally, and am going to push them.

 > > '''cutCallback()'''
 > >  - why do we have both zonecut_node_ and dname_node_?
 [snip]
 > >   we could do this by the RRset's RR type.  See also the next bullet.
 >
 > We could, but the code would be more complicated. This seems to be a
 cleaner design anyway (there are two different thinks I want to store,
 therefore I use two different variables, even when at most one of the
 values will be used).
 >
 > Anyway, if you are worried about performance reasons here, I'd like to
 point out that the variable is small, is located on the tack (therefore
 probably in cache). And, provided the function is template (find)
 instantiated only at this place, it will be inlined and the existence of
 the variable completely eliminated by data/flow tracing optimisations.

 I didn't worry about performance, and, in fact, my main concern was
 clarity/complexity.  Having more variables makes the code more
 incomprehensible because the reader needs to think about combinations
 of possible variable states (with two variables that can be NULL or
 non NULL, we need to consider 4 cases, and if the code (seemingly)
 ignores some of the cases we need to think about whether such a case
 is really impossible or whether it's overlooked, etc.).  But at the
 same time, I agree we should use separate variables to represent
 different things and I also agree the semantics of zonecut_node_ and
 dname_node_ would be different to some extent (while they also have
 similarity).  So, to me, this is a tradeoff issue.

 (continuing to the next point)

 > >  - on a related note, is it okay to only check zonecut_node_ at the
 > >    beginning of this function? what if we have an NS based delegation
 > >    and a DNAME under the zone cut?  (And what happens if we search
 > >    something under a zone cut with glue OK and hit a DNAME?)
 >
 > If we have one above another (and don't have the glue OK), then the
 search stops right there and we won't get to the lower one, which is AFAIK
 correct.
 >
 > If there are in the same node, then it is the only case allowed, at the
 apex and DNAME has precedence.
 >
 > If we have the glue OK mode, I'm not entirely sure, but don't we want to
 ignore the NS delegations completely, therefore getting to the first DNAME
 (under which is nothing) and terminating there? That's why DNAME is
 checked first in the PARTIALMATCH case.

 Actually, the only real question is about the case where we have DNAME
 under NS, and glue under DNAME:

 {{{
 dchild.example.com.     IN      NS      ns.dname.dchild.example.com.
 dname.dchild.example.com. IN    DNAME   example.org.
 ns.dname.dchild.example.com. IN A       192.0.2.39
 }}}
 and when we ask for the glue in the "glue OK mode".

 and I don't know what we should do in this case, either.  BIND 9
 ignores the DNAME and returns the glue A record for the query of
 ns.dname.../A with the "glue OK" mode.  NSD's zonec refuses to compile
 such a zone because it has a name under the owner name of the DNAME
 RR.  Spec-wise, NSD's behavior is probably the best, and we should
 eventually implement that.  But if we don't refuse it at load time, I
 see some point in BIND 9's behavior.  It would be less surprising if
 we return the glue anyway rather than simply stripping it from the
 response.

 So, I don't know the right answer.  Since this is basically about a
 pathological configuration, I think it's okay to act in an
 implementation dependent manner.  With pointing out these, I'd leave
 the decision to you.

 Now going back to whether to separate zonecut_node_ and dname_node_:
 The subtle ordering of check for DNAME and NS in the PARTIALMATCH case
 is in fact one specific example of "complexity".  By separating these,
 we need to stop and think: what if both dname_node_ and zonecut_node_
 are non NULL?  Is it ever possible?  If so, is it okay to always
 prefer dname_node_ over zonecut_node_?  If it's impossible, why do we
 have two variables?

 Again, with pointing out all of above, I'd leave the decision to you.
 But if you decide to keep separating these two variables, please add
 comments around the PARTIALMATCH handling code that clarify the
 questions I listed in the previous paragraph.

 > > '''find()'''
 > >
 > > In the following, name comparison may be a bit expensive (considering
 > > it's a performance sensitive operation):
 > > {{{
 > >         // If the node callback is enabled, this may be a zone cut.
 If it
 > >         // has a NS RR, we should return a delegation, but not in the
 apex.
 > >         if (node->isCallbackEnabled() && node->getName() != origin_) {
 > > }}}
 > >    I wonder we might maintain a pointer to the origin node and compare
 > >    'node' and that pointer.  Keeping an origin pointer may be useful
 > >    for other purposes.
 >
 > I added the pointer. But I wouldn't worry about performance there. We
 just searched the tree and compared the name several times. And this one
 was done only in case the node was marked for callback.

 Ack.  I wonder we might want to create the node at the creation of the
 zone (which is what BIND 9 does).  In any valid zone that node should
 exist and should not be deleted throughout the lifetime of the zone,
 so we should safely be able to do so, and it's slightly more
 efficient (and makes the code a bit simpler).

 > > '''Unittests'''
 > >  - related to the question about DNAME under a delegation in the code,
 > >    it would be nice to have a test case for that scenario.
 >
 > Is my understanding that we should not note the NS ones and act based on
 the DNAME in that case? If so, I'll write one.

 See above.  Please make your decision, and update the test accordingly.

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