[1261] added some error case tests. updated do_xfrin() so that it will

handle exceptions more correctly.  in particular it should have caught
various other exceptions from other modules (e.g. one from pydnspp/isc.dns)
without incorrectly assuming it uses Boost.Python (we don't use it any more).
This change fixes another problem in an existing test, so I also fixed
that test case, too.

src/bin/xfrin/tests/xfrin_test.py

@@ -575,6 +575,19 @@ class TestXfrinConnection(unittest.TestCase):
                 self.assertEqual(diff_exp[1].get_rdata()[0],
                                  diff_actual[1].get_rdata()[0])
 
+    def _create_a(self, address):
+        rrset = RRset(Name('a.example.com'), TEST_RRCLASS, RRType.A(),
+                      RRTTL(3600))
+        rrset.add_rdata(Rdata(RRType.A(), TEST_RRCLASS, address))
+        return rrset
+
+    def _create_soa(self, serial):
+        rrset = RRset(TEST_ZONE_NAME, TEST_RRCLASS, RRType.SOA(),
+                      RRTTL(3600))
+        rdata_str = 'm. r. ' + serial + ' 3600 1800 2419200 7200'
+        rrset.add_rdata(Rdata(RRType.SOA(), TEST_RRCLASS, rdata_str))
+        return rrset
+
 class TestAXFR(TestXfrinConnection):
     def setUp(self):
         super().setUp()
@@ -1019,10 +1032,7 @@ class TestAXFR(TestXfrinConnection):
 
     def test_do_soacheck_broken_response(self):
         self.conn.response_generator = self._create_broken_response_data
-        # XXX: TODO: this test failed here, should xfr not raise an
-        # exception but simply drop and return FAIL?
-        #self.assertEqual(self.conn.do_xfrin(True), XFRIN_FAIL)
-        self.assertRaises(MessageTooShort, self.conn.do_xfrin, True)
+        self.assertEqual(self.conn.do_xfrin(True), XFRIN_FAIL)
 
     def test_do_soacheck_badqid(self):
         # the QID mismatch would internally trigger a XfrinException exception,
@@ -1041,19 +1051,6 @@ class TestIXFRResponse(TestXfrinConnection):
         self.conn._datasrc_client = MockDataSourceClient()
         XfrinInitialSOA().set_xfrstate(self.conn, XfrinInitialSOA())
 
-    def create_a(self, address):
-        rrset = RRset(Name('a.example.com'), TEST_RRCLASS, RRType.A(),
-                      RRTTL(3600))
-        rrset.add_rdata(Rdata(RRType.A(), TEST_RRCLASS, address))
-        return rrset
-
-    def create_soa(self, serial):
-        rrset = RRset(TEST_ZONE_NAME, TEST_RRCLASS, RRType.SOA(),
-                      RRTTL(3600))
-        rdata_str = 'm. r. ' + serial + ' 3600 1800 2419200 7200'
-        rrset.add_rdata(Rdata(RRType.SOA(), TEST_RRCLASS, rdata_str))
-        return rrset
-
     def test_ixfr_response(self):
         '''A simplest form of IXFR response.
 
@@ -1077,33 +1074,33 @@ class TestIXFRResponse(TestXfrinConnection):
             questions=[Question(TEST_ZONE_NAME, TEST_RRCLASS, RRType.IXFR())],
             answers=[soa_rrset,
                      # removing one A in serial 1230
-                     begin_soa_rrset, self.create_a('192.0.2.1'),
+                     begin_soa_rrset, self._create_a('192.0.2.1'),
                      # adding one A in serial 1231
-                     self.create_soa('1231'), self.create_a('192.0.2.2'),
+                     self._create_soa('1231'), self._create_a('192.0.2.2'),
                      # removing one A in serial 1231
-                     self.create_soa('1231'), self.create_a('192.0.2.3'),
+                     self._create_soa('1231'), self._create_a('192.0.2.3'),
                      # adding one A in serial 1232
-                     self.create_soa('1232'), self.create_a('192.0.2.4'),
+                     self._create_soa('1232'), self._create_a('192.0.2.4'),
                      # removing one A in serial 1232
-                     self.create_soa('1232'), self.create_a('192.0.2.5'),
+                     self._create_soa('1232'), self._create_a('192.0.2.5'),
                      # adding one A in serial 1234
-                     soa_rrset, self.create_a('192.0.2.6'),
+                     soa_rrset, self._create_a('192.0.2.6'),
                      soa_rrset])
         self.conn._handle_xfrin_responses()
         self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate()))
         self.assertEqual([], self.conn._datasrc_client.diffs)
         self.check_diffs([[('remove', begin_soa_rrset),
-                           ('remove', self.create_a('192.0.2.1')),
-                           ('add', self.create_soa('1231')),
-                           ('add', self.create_a('192.0.2.2'))],
-                          [('remove', self.create_soa('1231')),
-                           ('remove', self.create_a('192.0.2.3')),
-                           ('add', self.create_soa('1232')),
-                           ('add', self.create_a('192.0.2.4'))],
-                          [('remove', self.create_soa('1232')),
-                           ('remove', self.create_a('192.0.2.5')),
+                           ('remove', self._create_a('192.0.2.1')),
+                           ('add', self._create_soa('1231')),
+                           ('add', self._create_a('192.0.2.2'))],
+                          [('remove', self._create_soa('1231')),
+                           ('remove', self._create_a('192.0.2.3')),
+                           ('add', self._create_soa('1232')),
+                           ('add', self._create_a('192.0.2.4'))],
+                          [('remove', self._create_soa('1232')),
+                           ('remove', self._create_a('192.0.2.5')),
                            ('add', soa_rrset),
-                           ('add', self.create_a('192.0.2.6'))]],
+                           ('add', self._create_a('192.0.2.6'))]],
                          self.conn._datasrc_client.committed_diffs)
 
     def test_ixfr_response_multi_messages(self):
@@ -1118,8 +1115,46 @@ class TestIXFRResponse(TestXfrinConnection):
             answers=[soa_rrset])
         self.conn._handle_xfrin_responses()
         self.assertEqual(type(XfrinIXFREnd()), type(self.conn.get_xfrstate()))
+        self.check_diffs([[('remove', begin_soa_rrset), ('add', soa_rrset)]],
+                         self.conn._datasrc_client.committed_diffs)
+
+    def test_ixfr_response_broken(self):
+        '''Test with a broken response.
+
+        '''
+        # SOA sequence is out-of-sync
+        self.conn.reply_data = self.conn.create_response_data(
+            questions=[Question(TEST_ZONE_NAME, TEST_RRCLASS, RRType.IXFR())],
+            answers=[soa_rrset, begin_soa_rrset, soa_rrset,
+                     self._create_soa('1235')])
+        self.assertRaises(XfrinProtocolError,
+                          self.conn._handle_xfrin_responses)
+        # no diffs should have been committed
+        self.check_diffs([], self.conn._datasrc_client.committed_diffs)
+
+    def test_ixfr_response_extra(self):
+        '''Test with an extra RR after the end of IXFR diff sequences.
+
+        IXFR should be rejected, but complete diff sequences should be
+        committed; it's not clear whether it's compliant to the protocol
+        specification, but it is how BIND 9 works and we do the same.
+        '''
+        self.conn.reply_data = self.conn.create_response_data(
+            questions=[Question(TEST_ZONE_NAME, TEST_RRCLASS, RRType.IXFR())],
+            answers=[soa_rrset, begin_soa_rrset, soa_rrset, soa_rrset,
+                     self._create_a('192.0.2.1')])
+        self.assertRaises(XfrinProtocolError,
+                          self.conn._handle_xfrin_responses)
+        self.check_diffs([[('remove', begin_soa_rrset), ('add', soa_rrset)]],
+                         self.conn._datasrc_client.committed_diffs)
 
 class TestIXFRSession(TestXfrinConnection):
+    '''Tests for a full IXFR session (query and response).
+
+    Detailed corner cases should have been covered in test_create_query()
+    and TestIXFRResponse, so we'll only check some typical cases to confirm
+    the general logic flow.
+    '''
     def setUp(self):
         super().setUp()
 
@@ -1145,6 +1180,26 @@ class TestIXFRSession(TestXfrinConnection):
         self.assertEqual(TEST_ZONE_NAME, qmsg.get_question()[0].get_name())
         self.assertEqual(RRType.IXFR(), qmsg.get_question()[0].get_type())
 
+    def test_do_xfrin_fail(self):
+        '''IXFR fails due to a protocol error.
+
+        '''
+        def create_ixfr_response():
+            self.conn.reply_data = self.conn.create_response_data(
+                questions=[Question(TEST_ZONE_NAME, TEST_RRCLASS,
+                                    RRType.IXFR())],
+                answers=[soa_rrset, begin_soa_rrset, soa_rrset,
+                         self._create_soa('1235')])
+        self.conn.response_generator = create_ixfr_response
+        self.assertEqual(XFRIN_FAIL, self.conn.do_xfrin(False, True))
+
+    def test_do_xfrin_fail(self):
+        '''IXFR fails due to a bogus DNS message.
+
+        '''
+        self._create_broken_response_data()
+        self.assertEqual(XFRIN_FAIL, self.conn.do_xfrin(False, True))
+
 class TestXfrinRecorder(unittest.TestCase):
     def setUp(self):
         self.recorder = XfrinRecorder()

src/bin/xfrin/xfrin.py.in

@@ -505,18 +505,24 @@ class XfrinConnection(asyncore.dispatcher):
                                                 self._handle_axfrin_response)
                     logger.info(XFRIN_AXFR_TRANSFER_SUCCESS, self.zone_str())
 
-        except XfrinException as e:
+        except (XfrinException, XfrinProtocolError) as e:
+            # TODO: rename it: AXFR->XFR (or remove it)
             logger.error(XFRIN_AXFR_TRANSFER_FAILURE, self.zone_str(), str(e))
             ret = XFRIN_FAIL
             #TODO, recover data source.
         except isc.datasrc.sqlite3_ds.Sqlite3DSError as e:
             logger.error(XFRIN_AXFR_DATABASE_FAILURE, self.zone_str(), str(e))
             ret = XFRIN_FAIL
-        except UserWarning as e:
-            # XXX: this is an exception from our C++ library via the
-            # Boost.Python binding.  It would be better to have more more
-            # specific exceptions, but at this moment this is the finest
-            # granularity.
+        except Exception as e:
+            # Catching all possible exceptions like this is generally not a
+            # good practice, but handling an xfr session could result in
+            # so many types of exceptions, including ones from the DNS library
+            # or from the data source library.  Eventually we'd introduce a
+            # hierarchy for exception classes from a base "ISC exception" and
+            # catch it here, but until then we need broadest coverage so that
+            # we won't miss anything.
+
+            # TODO: rename it: both 'AXFR' and 'INTERNAL' are inappropriate
             logger.error(XFRIN_AXFR_INTERNAL_FAILURE, self.zone_str(), str(e))
             ret = XFRIN_FAIL
         finally: