BIND 10 #1372: IXFR-out protocol handling: AXFR style IXFR

[BIND 10 Development]

#1372: IXFR-out protocol handling: AXFR style IXFR
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20111122
                  Component:         |            Resolution:
  xfrout                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  4
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 vorner]:

 Thanks for the prompt review!

 > I have few notes about it:
 >  * The `find_zone` in tests, the docstring is wrong. I don't see it
 returning PARTIALMATCH anywhere.

 Ah, that's a result of naive copy-paste (and adjust).  Fixed.

 >  * In the `find` method in tests, what is the different between the
 first if case and the else case?

 Another good catch.  It resulted in awkward code as a result of
 incremental development.  I believe the latest version makes more
 sense.

 >  * About the tests generally, I noticed quite some code from them was
 removed. Is it OK to do? Did the functionality tested disappear or did the
 tests move somewhere else?

 Hmm, as far as I can see few (or even no) tests were removed in the
 *body* of this branch.  Maybe you are referring to changes in the
 first commit (315f499) that merges #1370?  (1370 itself wasn't a
 dependency for this task but I merge it to minimize the diff at the
 merge time).  Since we stopped skipping intermediate TSIGs, some
 corresponding tests were removed in #1370.  (See also the next
 bullet)

 >  * Is it right the sign-every-nth message is gone and everything is
 signed now? I'd guess so by reading the changes, is that true? If so, what
 was the reason? Is it OK to do in this branch?

 See #1370 (and changelog in master for this).  I should have noted
 this in the pre-review comment, but forgot to mention that, which may
 have confused you.  If so, sorry about that.

 In any case, it's a result of merge (which is now in master), and not
 directly related to this branch.

 >  * This thing:
 > {{{#!python
 > # Make sure the question is valid.  This should be ensured by
 > # the auth server, but since it's far from xfrout itself, we check
 > # it by ourselves.  A viloation would be an internal bug, so we
 > # raise and stop here rather than returning a FORMERR or SERVFAIL.
 > }}}
 >    Will anything at all be returned to the client, or will we just drop
 the connection in such case? I believe even an internal bug requires a
 SERVFAIL, doesn't it?

 Actually, I thought in this case there is even *no client* as long as
 the auth server does the right thing, so this bug should be a more
 serious thing.  That's why I thought it would make more sense to do
 nothing for this case (and the case of unexpected request type).  In
 general, I agree an internal error would result in SERVFAIL.

 But in this specific case I'm also okay with following the general
 practice if you prefer it.

 >  * You call the xfrout a „daemon“ in the log message descriptions. I
 believe this might be misleading a little bit, for bind10 itself can run
 in foreground and xfrout is only part of the system anyway.

 Hmm, actually I just followed the seeming convention adopted in this
 message file.  I'm okay with changing daemon to e.g., server, but if
 we do so the change should apply throughout the message file.

 >  * Is it OK to convert a general std::exception (like std::bad_alloc) to
 isc.datasrc.Error? Shouldn't it be a generic Exception, then?

 Hmm, maybe you're right.  I adopted the convention used in
 DataSourceClient_getUpdater(), but a generic Exception seems to make
 more sense in these cases.  I've updated getJournalReader.  Maybe
 we should do the same for getUpdater(), but for now I've not touched it.

 >  * Should it really throw in case of no diffs table? Shouldn't it say
 NotImplemented or pretend there are no diffs? Who creates the table
 anyway? If it's xfrin, it might mean that no diffs were stored yet just.
 What will happen in this case anyway? Will it fallback to AXFR-like?

 As we discussed at bind10-dev separately, I personally hesitate to add
 more code for compatibility hack (like throwing NotImplemented due to
 schema mismatch) in this early stage.  In this particular case (adding
 a "diffs" table), my suggestion was to add an update script and have
 the user apply it.  And, for xfrout, I'd rather handle it like other
 data source level failure, i.e., returning SERVFAIL (it's (as a
 general case with exception) tested in
 test_dns_xfrout_start_datasrc_servfail).

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