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

 * owner:  vorner => jinmei


Comment:

 Hello

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

 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.

 **Style**

 There are several places with tabs:
 {{{
 --- a/src/bin/xfrin/xfrin.spec
 +++ b/src/bin/xfrin/xfrin.spec
 @@ -61,7 +61,36 @@
      "commands": [
       {
          "command_name": "retransfer",
 -        "command_description": "retransfer a single zone without checking
 zone serial number",
 +        "command_description": "retransfer a single zone without checking
 zone serial number, always using AXFR",
 +        "command_args": [ {
 +            "item_name": "zone_name",
 +            "item_type": "string",
 +            "item_optional": false,
 +            "item_default": ""
 +          },
 +          {
 +            "item_name": "zone_class",
 +            "item_type": "string",
 +            "item_optional": true,
 +            "item_default": "IN"
 +          },
 +          {
 +            "item_name": "master",
 +            "item_type": "string",
 +            "item_optional": true,
 +            "item_default": ""
 +          },
 +          {
 +            "item_name": "port",
 +            "item_type": "integer",
 +            "item_optional": true,
 +            "item_default": 53
 +          }
 +        ]
 +      },
 +     {
 +        "command_name": "refresh",
 +        "command_description": "transfer a single zone with checking zone
 serial number and honoring the request_ixfr policy",
          "command_args": [ {
              "item_name": "zone_name",
              "item_type": "string",---
 a/tests/lettuce/configurations/xfrin/retransfer_slave_notify.conf.orig
 +++ b/tests/lettuce/configurations/xfrin/retransfer_slave_notify.conf.orig
 @@ -28,7 +28,8 @@
          "zones": [ {
              "name": "example.org",
              "master_addr": "::1",
 -            "master_port": 47807
 +            "master_port": 47807,
 +           "request_ixfr": "no"
          } ]
      },
      "Zonemgr": {
 diff --git
 a/tests/lettuce/configurations/xfrin/retransfer_slave_notify_v4.conf
 b/tests/lettuce/configurations/xfrin/retransfer_slave_notify_v4.conf
 index 67ebfd3..ee593e2 100644
 --- a/tests/lettuce/configurations/xfrin/retransfer_slave_notify_v4.conf
 +++ b/tests/lettuce/configurations/xfrin/retransfer_slave_notify_v4.conf
 @@ -28,7 +28,8 @@
          "zones": [ {
              "name": "example.org",
              "master_addr": "127.0.0.1",
 -            "master_port": 47809
 +            "master_port": 47809,
 +           "request_ixfr": "no"
          } ]
      },
      "Zonemgr": {
 }}}

 {{{#!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)
 }}}

 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 see no threads mentioned in `fb3b94fa5eec711ff84ef281370eb2bfa95a0b5a`
 (except the commit message). Did you mean methods?

 Does the current way of creating zones still have the problem of leaving
 an empty zone upon failed attempt?

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

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

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

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

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