BIND 10 #2911: don't fallback from ixfr to axfr due to data source error

[BIND 10 Development]

#2911: don't fallback from ixfr to axfr due to data source error
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  defect        |  jinmei
            Priority:  medium        |                       Status:
           Component:  xfrin         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130611
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:18 vorner]:

 > I think rejecting it is fine for now. Most people probably didn't set
 the flag and I doubt there are many real-world instances either.

 Okay.

 > > And I don't understand why this part is cited: is it a tab issue or
 > > something else?
 > >
 > > > {{{#!python
 > > >     def test_config_handler_use_ixfr(self):
 > > >         # use_ixfr was deprecated and explicitly rejected for now.
 > > >         config = { 'zones': [
 > > >                    { 'name': 'test.example.',
 > > >                     'master_addr': '192.0.2.1',
 > > >                     'master_port': 53,
 > > >                      'use_ixfr': True
 > > >                    }
 > > >                  ]}
 > > >
 self.assertEqual(self.xfr.config_handler(config)['result'][0], 1)
 > > > }}}
 > >
 > > There's not a tab, but the indentation is certainly incorrect.  I
 > > guess it's a simple mistyping.  Fixed.
 >
 > It may have not been a tab, tabs just show in the diff as differently
 indented (because of the `+` at the beginning). In such case it was just
 bad indentation.

 Regarding test_config_handler_use_ixfr, I see the alignment is awkward
 (but basically not because of the branch - this part is basically
 copy-pasted).  If you mean that, I've fixed it with one other similar
 case.  If you meant something else, please feel free to edit it as you
 like.  That's probably the fastest way to fix them.

 > However, I don't see any fix for that in the branch and it seems still
 broken in my copy.

 Sorry, that "fixed" was about ec99eda3e051a84756e35e47ef2ad480a5ba410e
 not test_config_..., and I realized I didn't even actually fixed that
 one.  Now it's really fixed.

 > > > I see no threads mentioned in
 `fb3b94fa5eec711ff84ef281370eb2bfa95a0b5a` (except the commit message).
 Did you mean methods?
 > >
 > > Ah, right.  Should I update the commit using rebase?  (Since the
 > > branch is already shared I've not touched the history at the moment).
 >
 > Yes, unless it's a lot of work, as it's only a minor detail.

 Hmm, it's not clear this "yes" means I should update it or not.  Can
 you clarify?

 > > > Does the current way of creating zones still have the problem of
 leaving an empty zone upon failed attempt?
 > >
 > > I don't understand the question...is this about config_handler()
 > > replacing `_zones`?
 >
 > No. I mean the fact that we create a new zone if there wasn't one before
 in the database. And it's not inside the transaction, so if the transfer
 fails for some reason, the data is not committed, but the new zone is
 still left in the database, empty.

 Ah, okay, that's right.  I'm inclined to say it's out of scope for
 this ticket, as it's not something introduced by this branch and we
 are also expecting fundamental rewrite of xfr's with zone management
 framework.  But if you think we should update it within this task
 strongly, I'm okay with doing it too.

 > > As for what we should do in this case may be debatable.  But I don't
 > > think it a bad idea to continue the transfer using AXFR in such a case
 > > (which is what the current branch should do).
 >
 > Hmm. What will happen if the master copy is broken this way? Will it
 reject this on xfr-in, effectively making the transfer fail and retry soon
 (and infinitely so)?

 Yes, and I think that's actually the right behavior in general.  If
 the primary sends some broken version, secondaries should simply keep
 retrying (with some intervals) until it's eventually fixed at the
 primary side.  Another side note: in this particular case, it
 shouldn't have been due to a bad prior transfer in the first place as
 the zone checker should have rejected it.  It can only happen due to
 something like manually adding a redundant SOA.

 In any case, like some other points raised in the review, this is also
 not a new change introduced in this branch either.  If we want to
 change the behavior, I think it should go to a separate ticket...

 ...but on looking it more closely, I noticed there's a small
 discrepancy with the new behavior of this branch and what's described
 for the log (ID XFRIN_ZONE_MULTIPLE_SOA).  So I just updated that one
 to make it match the latest behavior.

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