BIND 10 #2439: update xfrin so it performs post transfer checks

[BIND 10 Development]

#2439: update xfrin so it performs post transfer checks
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  xfrin         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130205
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  loadzone-ng
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''changelog'''

 I suspect there is actually no possibility of missing SOA (that would
 be identified by this check) due to the protocol characteristics of
 XFRs.  Maybe a minor point and no one cares, but to be very accurate
 we might say, e.g., just "such as missing NS (at zone origin)".

 '''xfrin.py.in'''

 - This comment seems to have to be updated:
 {{{#!python
                 # The final part is there. Check all was signed
                 # and commit it to the database.
 }}}
   It may not necessarily be considered "incorrect", but due to the
   change in the code that follows, I see some gap between the actual
   code and the comment. (e.g., in the new code it's not immediately
   clear why it mentions "signed" here).
 - Same for the docstring for finish_message().
 - I've noticed a slight gap from BIND 9 (based on code inspection, so
   maybe I'm wrong): in case of IXFR, BIND 9 commits the diffs to
   before post-xfr checks.  So, even if it fails the persistent DB is
   still updated.  It still drops the in-memory DB, but doesn't revert
   to the pre-xfr version.  So queries to that zone will result in
   SERVFAIL after that.  I don't know the rationale about this behavior
   (if I'm correct), but I suggest (1) check if my understanding is
   correct yourself (2) (as usual) ask why BIND 9 behaves that way (3)
   then, think about what we should do. (4) as a result of that, update
   the implementation if necessary.

 '''xfrin_test.py'''

 I'd test some more cases:
 - Confirm warnings (by themselves) don't result in rejection.
 - IXFR case
 - check at least one rejection case with sqlite3.  it's redundant in
   some sense, but sometimes we (I) find real-world cases have an issue
   that aren't revealed just with mocks.

 '''xfrin_messages.mes'''

 I'm not sure if we call failure of post-transfer checks a "protocol
 error".  At least there was no protocol error in the xfr session.  If
 we keep reusing this message (and the exception), I'd clarify that in
 the detailed description for XFRIN_XFR_TRANSFER_PROTOCOL_ERROR.

 Also, wherever we note that, I'd also describe some more things:
 - clarify that the new zone is dropped and won't be used (and if there
   was an old version it will still be used)
 - when mentioning the possibility of post-zone check failure, refer to
   XFRIN_ZONE_INVALID (suggesting to check the log to see if there's
   some of that).
 - depending on what to do with the gap from BIND 9 discussed above,
   explain the difference from BIND 9 and the rationale.
 - related, although not directly related to this task: there's another
   gap from BIND 9: we don't commit IXFR unless all IXFR responses are
   processed.  So there's another type of difference in the DB state
   if IXFR is rejected in the middle of it.  I'd explain the difference
   and the rationale here, too.

 '''diff.py'''

 - This part of docstring is awkward:
 {{{
         [...] Then it creates
         and returns an RRsetCollection on top of the corresponding zone
         updater and returns it.
 }}}
    Maybe just remove "and returns it".

 '''diff_tests.py'''

 - the comment should be "it is applied"?
 {{{#!python
         # Check it is commited
         self.assertEqual(1, len(self.__data_operations))
         self.assertEqual('add', self.__data_operations[0][0])
 }}}
 - As for this:
 {{{#!python
         # Any idea why python doesn't agree with inheriting from
         # dns.RRsetCollection?
 }}}
   I don't know what you wanted to do by inheriting from
   `dns.RRsetCollection` (not from the base), but if it's really
   necessary you should specify `Py_TPFLAGS_BASETYPE` in the `tp_flags`
   field of the corresponding `PyTypeObject`.  See the difference of
   this member between `rrset_collection_base_type` and
   `rrset_collection_type`.  But in any event, in this particular case
   I don't see the need for that trick: the current mock seems to be
   sufficient.

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