BIND 10 #1261: IXFR-in protocol handling

[BIND 10 Development]

#1261: IXFR-in protocol handling
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20111011
  blocker                            |            Resolution:
                  Component:  xfrin  |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:6 jelte]:
 > I took the liberty of correcting a few typos, see commit
 > cf136247fad510f55ba230f746558274fada1de6

 Looks good, thanks.

 > Code looks good, i like the use of the state design pattern.
 >
 > Just a few comments:
 >
 > xfrin.py.in:
 >
 > just a note about the doc on line 215:
 > handle_rr() technically, should it not be overridden, it returns None
 due to the pass in the default implementation. (i think it could even
 raise an exception if we don't expect the superclass to be called)

 Yeah I actually thought that option, and now that you also pointed it
 out, it would be a better idea.  Revised it that way.

 > 232: s/given/received/ ?

 Okay, changed.

 > Wouldn't XfrinFirstData fail (or rather, behave strangely and NOT fail)
 if the second RR in the response is a SOA with a different serial than we
 currently have?

 Actually this is an interesting point, but in this context I believe
 we can keep the current behavior.  I've provided more details about this
 point in the style of documentation.  Please check it.

 My personal take is to add a check in the not-yet-implemented AXFREnd
 state about the SOA consistency and just leave a log message if they
 mismatch (but accept it); or we could reject the transfer at the point.

 > Should we also make a note somewhere on which errors we should fall back
 to AXFR (and how such errors fall into the design pattern)?

 I'd leave it to the "fall back task".
 >
 > I wonder (line 444), that if we find that we have a clearly inconsistent
 database, whether it is acceptable to just drop it and AXFR-in a fresh
 one.

 I'm not sure which revision of line 444 you're referring to:-), but are
 you
 talking about this one?
 {{{
             # Especially for database-based zones, a working zone may be
 in
             # a broken state where it has more than one SOA RR.  We
 proactively
             # check the condition and abort the xfr attempt if we identify
 it.
             if soa_rrset.get_rdata_count() != 1:
                 raise XfrinException('Invalid number of SOA RRs for ' +
                                      self.zone_str() + ': ' +
                                      str(soa_rrset.get_rdata_count()))
 }}}

 If so, falling back to AXFR in this case could be an option.  I think
 this will be done at one level higher, maybe reporting the error to
 the zone manager and let it decide the next action.  In any case I'd
 leave this to the "fallback ticket", too.

 > Come to think of it, should the exception we raise when we don't have
 the zone or its SOA at all be a more specific one (so we don't need to
 fall back on any xfrinexception)? Technically this may not even be enough
 of an exceptional case to warrant an exception in the first place, though
 I don't feel to strongly about that.

 Yeah perhaps.  I was thinking about something similar.  We should probably
 introduce an exception that indicates an IXFR failure that should (better)
 be followed by a fall-back AXFR.

 As for whether to use an exception or not, I don't have a strong
 opinion.  I heard it's actually "Pythononic" to use exceptions to
 handle general error cases, rather than handling it as an error code, etc.

 > xfrin.py.in:550  logstr is never used? leftover from old logging?

 Ah, good catch...actually it's not used even before this branch, but
 it would be good to clean it up at this opportunity.  So I did it.

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