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
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:9 jinmei]:
 > 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)".

 Right, I'll remove the mention of SOA in the changelog.

 > - 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.

 Well, I spent something like an hour trying to get through all the
 indirect
 calls, macros and gotos of bind9. After that time I gave up, deciding I
 have no
 clue what is done with the database or with the in-memory image.

 I'll write an email about that. But I suspect it was just easier solution
 for bind9, while this is easier for us. At least, I don't think we have a
 way to store it in the database and not use it now and storing the wrong
 one seems incorrect. We probably could wipe the whole zone out, but that
 doesn't feel right either. And it doesn't matter much. Anyway, having the
 db version and the in-memory version inconsistent seems like a bad idea.

 > - 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.

 I did these as lettuce tests. And I did find a bug in xfrin, but it seems
 unrelated to the new changes, so I'll create a new ticket for it. When the
 download fails (tested with validation error, but I think it happens if it
 fails for other reason too) and the zone didn't exist before, an empty
 zone (that doesn't work) is created and makes the server SERVFAIL on
 query. We should either roll back (eg. remove the zone again) like in
 b10-loadzone or restart the discussion about interface for creating zones,
 and provide a way to create the zone in the same transaction as filling in
 the data, so we can just rollback the transaction as a whole.

 > 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.

 I created a new exception and log message for it, so it is now separated.

 > Also, wherever we note that, I'd also describe some more things:
 > - 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.

 This one was discussed and was decided we need to do it this way, because
 there might be TSIG-unsigned messages in the middle and we can't really
 commit them until the whole session is properly signed. I don't know if
 bind9 supports the mode where some messages in the middle aren't signed or
 how it is solved there.

 Anyway, I don't feel a log message description is the right place for
 describing these kinds of differences. Maybe we should have a section in
 the guide somewhere for this?

 Also, is that difference important enough? And, at how many places does
 bind9 describe differences from bind8? I mean, we are creating new, stand-
 alone product, not a mere substitute for bind9. In that sense, I'm not
 against describing differences, but some level of differences could be
 expected. Also, if doing so, it would make sense to describe differences
 to NSD or Knot or other servers.

 > - 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.

 Actually, I was slightly confused (I'm still not used to calling the base
 class „Base“ in its name, I'm used to calling the base class by generic
 name and then differing the implementations) and wrongly tried to used
 that one. And I was surprised by the error message and because I didn't
 know about the existence of the flag, so I guessed all the wrapper classes
 are not possible to inherit. I did use the „Base“ one now, which acts the
 way I want (with a little catch that I had to create the constructor and
 not call the inherited one).

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