Merge ~blake-rouse/maas:fix-1710308-2.2 into maas:2.2

[Blake Rouse]

Backport eebad83a13bf1d1ec99d2bba4fdcd831cbede3f1 from master.

Fixes LP: #1710308, #1710278 - After BIND reload verify that the serial for all authoritative domains the new value before continuing.

[Blake Rouse (blake-rouse)]

Self-approving backport.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b fix-1710308-2.2 lp:~blake-rouse/maas into -b 2.2 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/238/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b fix-1710308-2.2 lp:~blake-rouse/maas into -b 2.2 lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/239/consoleText

diff --git a/src/maasserver/dns/config.py b/src/maasserver/dns/config.py
index 855cc47..e27773a 100644
--- a/src/maasserver/dns/config.py
+++ b/src/maasserver/dns/config.py
@@ -88,6 +88,12 @@ def dns_update_all_zones(reload_retry=False):
     else:
         bind_reload()
 
+    # Return the current serial and list of domain names.
+    return serial, [
+        domain.name
+        for domain in domains
+    ]
+
 
 def get_upstream_dns():
     """Return the IP addresses of configured upstream DNS servers.
diff --git a/src/maasserver/dns/tests/test_config.py b/src/maasserver/dns/tests/test_config.py
index 029e874..c163420 100644
--- a/src/maasserver/dns/tests/test_config.py
+++ b/src/maasserver/dns/tests/test_config.py
@@ -53,6 +53,7 @@ from testtools.matchers import (
     Contains,
     Equals,
     FileContains,
+    MatchesSetwise,
     MatchesStructure,
 )
 
@@ -302,6 +303,24 @@ class TestDNSConfigModifications(TestDNSServer):
             port=self.bind.config.port, commands=[ns_record, '+short'])
         self.assertEqual(ip, ip_of_ns_record)
 
+    def test_dns_update_all_zones_returns_serial_and_domains(self):
+        self.patch(settings, 'DNS_CONNECT', True)
+        domain = factory.make_Domain()
+        # These domains should not show up. Just to test we create them.
+        for _ in range(3):
+            factory.make_Domain(authoritative=False)
+        node, static = self.create_node_with_static_ip(domain=domain)
+        fake_serial = random.randint(1, 1000)
+        self.patch(
+            dns_config_module,
+            "current_zone_serial").return_value = fake_serial
+        serial, domains = dns_update_all_zones()
+        self.assertThat(serial, Equals(fake_serial))
+        self.assertThat(domains, MatchesSetwise(*[
+            Equals(domain.name)
+            for domain in Domain.objects.filter(authoritative=True)
+        ]))
+
 
 class TestDNSDynamicIPAddresses(TestDNSServer):
     """Allocated nodes with IP addresses in the dynamic range get a DNS
diff --git a/src/maasserver/region_controller.py b/src/maasserver/region_controller.py
index f3a51f3..0f9e181 100644
--- a/src/maasserver/region_controller.py
+++ b/src/maasserver/region_controller.py
@@ -31,16 +31,25 @@ from provisioningserver.logger import LegacyLogger
 from provisioningserver.utils.twisted import (
     asynchronous,
     FOREVER,
+    pause,
 )
 from twisted.application.service import Service
 from twisted.internet import reactor
-from twisted.internet.defer import DeferredList
+from twisted.internet.defer import (
+    DeferredList,
+    inlineCallbacks,
+)
 from twisted.internet.task import LoopingCall
+from twisted.names.client import Resolver
 
 
 log = LegacyLogger()
 
 
+class DNSReloadError(Exception):
+    """Error raised when the bind never fully reloads the zone."""
+
+
 class RegionControllerService(Service):
     """
     A service that controllers external services that are in MAAS's control on
@@ -64,6 +73,9 @@ class RegionControllerService(Service):
         self.needsDNSUpdate = False
         self.needsProxyUpdate = False
         self.postgresListener = postgresListener
+        self.dnsResolver = Resolver(
+            resolv=None, servers=[('127.0.0.1', 53)],
+            timeout=(1,), reactor=clock)
 
     @asynchronous(timeout=FOREVER)
     def startService(self):
@@ -108,9 +120,12 @@ class RegionControllerService(Service):
         if self.needsDNSUpdate:
             self.needsDNSUpdate = False
             d = deferToDatabase(transactional(dns_update_all_zones))
+            d.addCallback(self._check_serial)
             d.addCallback(
-                lambda _: log.msg(
-                    "Successfully configured DNS."))
+                lambda domains: log.msg(
+                    "Successfully configured DNS "
+                    "authoritative zones: %s." % (
+                        ', '.join(domains))))
             d.addErrback(
                 log.err,
                 "Failed configuring DNS.")
@@ -131,3 +146,31 @@ class RegionControllerService(Service):
             self.processingDefer = None
         else:
             return DeferredList(defers)
+
+    @inlineCallbacks
+    def _check_serial(self, result):
+        """Check that the serial of the domain is updated."""
+        serial, domain_names = result
+        not_matching_domains = set(domain_names)
+        loop = 0
+        while len(not_matching_domains) > 0 and loop != 30:
+            for domain in list(not_matching_domains):
+                try:
+                    answers, _, _ = yield self.dnsResolver.lookupAuthority(
+                        domain)
+                except (ValueError, TimeoutError):
+                    answers = []
+                if len(answers) > 0:
+                    if int(answers[0].payload.serial) == int(serial):
+                        not_matching_domains.remove(domain)
+            loop += 1
+            yield pause(2)
+        # 30 retries with 2 second pauses (aka. 60 seconds) has passed and
+        # there still is a domain that has the wrong serial. For now just
+        # raise the error, in the future we should take action and force
+        # restart bind.
+        if len(not_matching_domains) > 0:
+            raise DNSReloadError(
+                "Failed to reload DNS; serial mismatch "
+                "on domains %s" % ', '.join(not_matching_domains))
+        return domain_names
diff --git a/src/maasserver/tests/test_region_controller.py b/src/maasserver/tests/test_region_controller.py
index 9dbe4da..a4e83d2 100644
--- a/src/maasserver/tests/test_region_controller.py
+++ b/src/maasserver/tests/test_region_controller.py
@@ -5,6 +5,7 @@
 
 __all__ = []
 
+import random
 from unittest.mock import (
     ANY,
     call,
@@ -14,7 +15,10 @@ from unittest.mock import (
 
 from crochet import wait_for
 from maasserver import region_controller
-from maasserver.region_controller import RegionControllerService
+from maasserver.region_controller import (
+    DNSReloadError,
+    RegionControllerService,
+)
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASServerTestCase
 from maastesting.matchers import (
@@ -22,6 +26,7 @@ from maastesting.matchers import (
     MockCallsMatch,
     MockNotCalled,
 )
+from testtools import ExpectedException
 from testtools.matchers import MatchesStructure
 from twisted.internet import reactor
 from twisted.internet.defer import (
@@ -29,6 +34,12 @@ from twisted.internet.defer import (
     inlineCallbacks,
     succeed,
 )
+from twisted.names.dns import (
+    A,
+    Record_SOA,
+    RRHeader,
+    SOA,
+)
 
 
 wait_for_reactor = wait_for(30)  # 30 seconds.
@@ -166,16 +177,28 @@ class TestRegionControllerService(MAASServerTestCase):
     def test_process_updates_zones(self):
         service = RegionControllerService(sentinel.listener)
         service.needsDNSUpdate = True
+        dns_result = (
+            random.randint(1, 1000), [
+                factory.make_name('domain')
+                for _ in range(3)
+            ])
         mock_dns_update_all_zones = self.patch(
             region_controller, "dns_update_all_zones")
+        mock_dns_update_all_zones.return_value = dns_result
+        mock_check_serial = self.patch(service, "_check_serial")
+        mock_check_serial.return_value = succeed(dns_result[1])
         mock_msg = self.patch(
             region_controller.log, "msg")
         service.startProcessing()
         yield service.processingDefer
         self.assertThat(mock_dns_update_all_zones, MockCalledOnceWith())
+        self.assertThat(mock_check_serial, MockCalledOnceWith(dns_result))
         self.assertThat(
             mock_msg,
-            MockCalledOnceWith("Successfully configured DNS."))
+            MockCalledOnceWith(
+                "Successfully configured DNS "
+                "authoritative zones: %s." % (
+                    ', '.join(dns_result[1]))))
 
     @wait_for_reactor
     @inlineCallbacks
@@ -236,8 +259,16 @@ class TestRegionControllerService(MAASServerTestCase):
         service = RegionControllerService(sentinel.listener)
         service.needsDNSUpdate = True
         service.needsProxyUpdate = True
+        dns_result = (
+            random.randint(1, 1000), [
+                factory.make_name('domain')
+                for _ in range(3)
+            ])
         mock_dns_update_all_zones = self.patch(
             region_controller, "dns_update_all_zones")
+        mock_dns_update_all_zones.return_value = dns_result
+        mock_check_serial = self.patch(service, "_check_serial")
+        mock_check_serial.return_value = succeed(dns_result[1])
         mock_proxy_update_config = self.patch(
             region_controller, "proxy_update_config")
         mock_proxy_update_config.return_value = succeed(None)
@@ -245,5 +276,97 @@ class TestRegionControllerService(MAASServerTestCase):
         yield service.processingDefer
         self.assertThat(
             mock_dns_update_all_zones, MockCalledOnceWith())
+        self.assertThat(mock_check_serial, MockCalledOnceWith(dns_result))
         self.assertThat(
             mock_proxy_update_config, MockCalledOnceWith(reload_proxy=True))
+
+    def make_soa_result(self, serial):
+        return RRHeader(
+            type=SOA, cls=A, ttl=30, payload=Record_SOA(serial=serial))
+
+    @wait_for_reactor
+    def test__check_serial_doesnt_raise_error_on_successful_serial_match(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = [
+            # First pass no results.
+            succeed(([], [], [])),
+            succeed(([], [], [])),
+            succeed(([], [], [])),
+            # First domain valid result.
+            succeed(([self.make_soa_result(result_serial)], [], [])),
+            succeed(([], [], [])),
+            succeed(([], [], [])),
+            # Second domain wrong serial.
+            succeed(([self.make_soa_result(result_serial - 1)], [], [])),
+            succeed(([], [], [])),
+            # Third domain correct serial.
+            succeed(([], [], [])),
+            succeed(([self.make_soa_result(result_serial)], [], [])),
+            # Second domain correct serial.
+            succeed(([self.make_soa_result(result_serial)], [], [])),
+        ]
+        # Error should not be raised.
+        return service._check_serial((formatted_serial, dns_names))
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test__check_serial_raise_error_after_30_tries(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = lambda *args: succeed(([], [], []))
+        # Error should not be raised.
+        with ExpectedException(DNSReloadError):
+            yield service._check_serial((formatted_serial, dns_names))
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test__check_serial_handles_ValueError(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = ValueError()
+        # Error should not be raised.
+        with ExpectedException(DNSReloadError):
+            yield service._check_serial((formatted_serial, dns_names))
+
+    @wait_for_reactor
+    @inlineCallbacks
+    def test__check_serial_handles_TimeoutError(self):
+        service = RegionControllerService(sentinel.listener)
+        result_serial = random.randint(1, 1000)
+        formatted_serial = '{0:10d}'.format(result_serial)
+        dns_names = [
+            factory.make_name('domain')
+            for _ in range(3)
+        ]
+        # Mock pause so test runs faster.
+        self.patch(region_controller, "pause").return_value = succeed(None)
+        mock_lookup = self.patch(service.dnsResolver, "lookupAuthority")
+        mock_lookup.side_effect = TimeoutError()
+        # Error should not be raised.
+        with ExpectedException(DNSReloadError):
+            yield service._check_serial((formatted_serial, dns_names))