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):

 Thanks for the review.

 Replying to [comment:15 vorner]:

 > **Configuration**
 >
 > First, two loosely related topics. They don't require work on this
 branch.
 >
 > We probably should start think seriously about some way to migrate old
 configuration to new one. I don't think we will stop having these kinds of
 deprecations or moving stuff around and these need to be solved
 automatically somehow. It is possible to use some merging tools with
 config files in /etc. But the JSON configuration file is not really
 supposed to be edited by user, so it shouldn't be needed to modify it. And
 changing the configuration of several zones will be tedious and boring.

 As a general matter, I agree.  For this particular ticket, one
 possible issue to discuss is whether we ignore the obsolete use_ixfr
 item possibly with a warning log message or we reject it.  The current
 branch takes the latter approach (as I thought we agreed on jabber or
 ML), but without providing offline config update mechanism it means
 the user will need to edit the json file by hand.  On the other hand,
 it's more likely that users won't notice the migration if we simply
 ignore the deprecated item (and we know users don't read log
 messages).

 Is that okay to keep the current branch's behavior?

 > The other one is, we may want to have an `enum` type in configuration.
 It would be mostly a string, but with restricted values. It could support
 validation on the bindctl end and also tab completion.

 I don't have a strong opinion, but this sounds like a reasonable
 suggestion.

 > **Style**
 >
 > There are several places with tabs:

 Hmm, I found tabs in lettuce config introduced in this branch, which I
 removed.

 I didn't find a tab in xfrin.spec introduced in the branch.  There are
 some tabs already existing in the file, but for completeness I removed
 them too.

 {{{
 > --- a/src/bin/xfrin/xfrin.spec
 > +++ b/src/bin/xfrin/xfrin.spec
 }}}

 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.

 > The indentation seems wrong here (possibly more tabs?):
 > {{{
 > +        logger.info(XFRIN_INITIAL_AXFR, format_zone_str(zname, zclass),
 > +                     AddressFormatter(master_addr))
 > +        return RRType.AXFR
 > }}}
 >
 > **Others**
 >
 > This seems to be a very complicated way of writing `check_soa = command
 != 'retransfer'`:
 > {{{#!python
 > check_soa = False if command == 'retransfer' else True
 > }}}

 I don't think it was introduced in the branch, but in any case these
 two cases are more explicitly separated in a later part of the branch,
 so it's already moot.

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

 > 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`?

 > Should this be an error instead? Treating it the same as not-yet-
 transfered zone seems wrong. Also, where's the check? I didn't see it in
 the code.
 >
 > {{{
 >     When the zone has an SOA RR, this method makes sure that it's
 >     valid, i.e., it has exactly one RDATA; if it is not the case
 >     this method returns None.
 > }}}

 Regarding where, maybe you're referring to an intermediate point of
 the branch?  In the latest version the check is the following part of
 `_get_zone_soa`:
 {{{#!python
     if soa_rrset.get_rdata_count() != 1:
         logger.warn(XFRIN_ZONE_MULTIPLE_SOA,
                     format_zone_str(zone_name, zone_class),
                     soa_rrset.get_rdata_count())
         return None
 }}}

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

 > This note seems to be outdated now:
 > {{{
 > (Note also that the part of
 > providing the compatible behavior uses the old data source API.
 > We'll deprecate this API in a near future, too).
 > }}}

 Ah, right, removed now.

 > This should be threading, not threaving:
 > {{{
 >           shutdown_event (threaving.Event): used for synchronization
 with
 >             parent thread.
 > }}}

 Right, corrected.

 > Some comment talks about asyncore. Does xfrin really use both threads
 and asyncore? (not that it would be caused by this branch, I'm just
 surprised).

 Yes, it does.  I don't know why it was introduced (at least unrelated
 to this branch), but according to the code it seems to introduce a
 timeout in reading XFR response messages.

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