Cache encountered zones in GoogleCloudProvider

Cache googleclouds zones so that populate dont have to list all each time called.

octodns/provider/googlecloud.py

@@ -53,6 +53,8 @@ class GoogleCloudProvider(BaseProvider):
         self.log = getLogger('GoogleCloudProvider[{}]'.format(id))
         self.id = id
 
+        self._gcloud_zones = {}
+
         super(GoogleCloudProvider, self).__init__(id, *args, **kwargs)
 
     def _apply(self, plan):
@@ -70,7 +72,10 @@ class GoogleCloudProvider(BaseProvider):
                        len(changes))
 
         # Get gcloud zone, or create one if none existed before.
-        gcloud_zone = self._get_gcloud_zone(desired.name, create=True)
+        if desired.name not in self.gcloud_zones:
+            gcloud_zone = self._create_gcloud_zone(desired.name)
+        else:
+            gcloud_zone = self.gcloud_zones.get(desired.name)
 
         gcloud_changes = gcloud_zone.changes()
 
@@ -117,13 +122,19 @@ class GoogleCloudProvider(BaseProvider):
         # and only contain lowercase letters, digits or dashes
         zone_name = re.sub("[^a-z0-9-]", "",
                            dns_name[:-1].replace('.', "-"))
-        # make sure that the end result did not end up wo leading letter
-        if re.match('[^a-z]', zone_name[0]):
-            # I cannot think of a situation where a zone name derived from
-            # a domain name would'nt start with leading letter and thereby
-            # violate the constraint, however if such a situation is
-            # encountered, add a leading "a" here.
-            zone_name = "a%s" % zone_name
+
+        # Check if there is another zone in google cloud which has the same
+        # name as the new one
+        while zone_name in [z.name for z in self.gcloud_zones.values()]:
+            # If there is a zone in google cloud alredy, then try suffixing the
+            # name with a -i where i is a number which keeps increasing until
+            # a free name has been reached.
+            m = re.match("^(.+)-([0-9]+$)", zone_name)
+            if m:
+                i = int(m.group(2)) + 1
+                zone_name = "{}-{!s}".format(m.group(1), i)
+            else:
+                zone_name += "-2"
 
         gcloud_zone = self.gcloud_client.zone(
             name=zone_name,
@@ -131,6 +142,9 @@ class GoogleCloudProvider(BaseProvider):
         )
         gcloud_zone.create(client=self.gcloud_client)
 
+        # add this new zone to the list of zones.
+        self._gcloud_zones[gcloud_zone.dns_name] = gcloud_zone
+
         self.log.info("Created zone %s. Fqdn %s." %
                       (zone_name, dns_name))
 
@@ -159,39 +173,25 @@ class GoogleCloudProvider(BaseProvider):
                 # yield from is in python 3 only.
                 yield gcloud_record
 
-    def _get_gcloud_zone(self, dns_name, page_token=None, create=False):
-        """Return the ManagedZone which has has the matching dns_name, or
-            None if no such zone exist, unless create=True, then create a new
-            one and return it.
-
-            :param dns_name: fqdn of dns name for zone to get.
-            :type dns_name: str
-            :param page_token: page token for the page to get
-            :type page_token: str
-            :param create: if true, create ManagedZone if it does not exist
-                           already
+    def _get_cloud_zones(self, page_token=None):
+        """Load all ManagedZones into the self._gcloud_zones dict which is
+        mapped with the dns_name as key.
 
-            :type return: new google.cloud.dns.ManagedZone
+        :return: void
         """
-        # Find the google name for the incoming zone
+
         gcloud_zones = self.gcloud_client.list_zones(page_token=page_token)
         for gcloud_zone in gcloud_zones:
-            if gcloud_zone.dns_name == dns_name:
-                return gcloud_zone
-        else:
-            # Zone not found. Check if there are more results which could be
-            # retrieved by checking "next_page_token".
-            if gcloud_zones.next_page_token:
-                return self._get_gcloud_zone(dns_name,
-                                             gcloud_zones.next_page_token)
-            else:
-                # Nothing found, either return None or else create zone and
-                # return that one (if create=True)
-                self.log.debug('_get_gcloud_zone: zone name=%s, '
-                               'was not found by %s.',
-                               dns_name, self.gcloud_client)
-                if create:
-                    return self._create_gcloud_zone(dns_name)
+            self._gcloud_zones[gcloud_zone.dns_name] = gcloud_zone
+
+        if gcloud_zones.next_page_token:
+            self._get_cloud_zones(gcloud_zones.next_page_token)
+
+    @property
+    def gcloud_zones(self):
+        if not self._gcloud_zones:
+            self._get_cloud_zones()
+        return self._gcloud_zones
 
     def populate(self, zone, target=False, lenient=False):
         """Required function of manager.py to collect records from zone.
@@ -210,7 +210,7 @@ class GoogleCloudProvider(BaseProvider):
                        target, lenient)
         before = len(zone.records)
 
-        gcloud_zone = self._get_gcloud_zone(zone.name)
+        gcloud_zone = self.gcloud_zones.get(zone.name)
 
         if gcloud_zone:
             for gcloud_record in self._get_gcloud_records(gcloud_zone):

tests/test_octodns_provider_googlecloud.py

@@ -164,8 +164,9 @@ class DummyResourceRecordSet:
 
 
 class DummyGoogleCloudZone:
-    def __init__(self, dns_name):
+    def __init__(self, dns_name, name=""):
         self.dns_name = dns_name
+        self.name = name
 
     def resource_record_set(self, *args):
         return DummyResourceRecordSet(*args)
@@ -173,6 +174,9 @@ class DummyGoogleCloudZone:
     def list_resource_record_sets(self, *args):
         pass
 
+    def create(self, *args, **kwargs):
+        pass
+
 
 class DummyIterator:
     """Returns a mock DummyIterator object to use in testing.
@@ -239,7 +243,7 @@ class TestGoogleCloudProvider(TestCase):
             'type': 'A',
             'values': ['1.4.3.2']})
 
-        gcloud_zone_mock = DummyGoogleCloudZone("unit.tests.")
+        gcloud_zone_mock = DummyGoogleCloudZone("unit.tests.", "unit-tests")
         status_mock = Mock()
         return_values_for_status = iter(
             ['', '', '', '', '', '', '', '', '', '', '', '', '', '', '',
@@ -250,10 +254,9 @@ class TestGoogleCloudProvider(TestCase):
 
         provider = self._get_provider()
         provider.gcloud_client = Mock()
-        provider._get_gcloud_zone = Mock(
-            return_value=gcloud_zone_mock)
+        provider._gcloud_zones = {"unit.tests.": gcloud_zone_mock}
         desired = Mock()
-        desired.name = Mock(return_value="unit.tests.")
+        desired.name = "unit.tests."
         changes = []
         changes.append(Create(create_r))
         changes.append(Delete(delete_r))
@@ -343,7 +346,7 @@ class TestGoogleCloudProvider(TestCase):
         google_cloud_zone.list_resource_record_sets = Mock(
             side_effect=_get_mock_record_sets)
 
-        self.assertEqual(provider._get_gcloud_zone("unit.tests.").dns_name,
+        self.assertEqual(provider.gcloud_zones.get("unit.tests.").dns_name,
                          "unit.tests.")
 
         test_zone = Zone('unit.tests.', [])
@@ -374,8 +377,8 @@ class TestGoogleCloudProvider(TestCase):
 
         provider._get_gcloud_records = Mock(
             side_effect=[not_same_fqdn])
-        provider._get_gcloud_zone = Mock(return_value=DummyGoogleCloudZone(
-            dns_name="unit.tests."))
+        provider._gcloud_zones = {
+            "unit.tests.": DummyGoogleCloudZone("unit.tests.", "unit-tests")}
 
         provider.populate(test_zone)
 
@@ -391,7 +394,7 @@ class TestGoogleCloudProvider(TestCase):
         provider.gcloud_client.list_zones = Mock(
             return_value=DummyIterator([]))
 
-        self.assertIsNone(provider._get_gcloud_zone("nonexistant.xone"),
+        self.assertIsNone(provider.gcloud_zones.get("nonexistant.xone"),
                           msg="Check that nonexistant zones return None when"
                               "there's no create=True flag")
 
@@ -413,22 +416,33 @@ class TestGoogleCloudProvider(TestCase):
         provider.gcloud_client.list_zones = Mock(
             return_value=DummyIterator([]))
 
-        mock_zone = provider._get_gcloud_zone(
-            'nonexistant.zone.mock', create=True)
+        mock_zone = provider._create_gcloud_zone("nonexistant.zone.mock")
 
         mock_zone.create.assert_called()
         provider.gcloud_client.zone.assert_called()
         provider.gcloud_client.zone.assert_called_once_with(
             dns_name=u'nonexistant.zone.mock', name=u'nonexistant-zone-moc')
 
-    def test__create_zone_with_numbers_in_name(self):
+    def test__create_zone_with_duplicate_names(self):
+
+        def _create_dummy_zone(name, dns_name):
+            return DummyGoogleCloudZone(name=name, dns_name=dns_name)
+
         provider = self._get_provider()
 
         provider.gcloud_client = Mock()
+        provider.gcloud_client.zone = Mock(side_effect=_create_dummy_zone)
         provider.gcloud_client.list_zones = Mock(
             return_value=DummyIterator([]))
 
-        provider._get_gcloud_zone(
-            '111.', create=True)
-        provider.gcloud_client.zone.assert_called_once_with(
-            dns_name=u'111.', name=u'a111')
+        _gcloud_zones = {
+            'unit-tests': DummyGoogleCloudZone("a.unit-tests.", "unit-tests")
+        }
+
+        provider._gcloud_zones = _gcloud_zones
+
+        test_zone_1 = provider._create_gcloud_zone("unit.tests.")
+        self.assertEqual(test_zone_1.name, "unit-tests-2")
+
+        test_zone_2 = provider._create_gcloud_zone("unit.tests.")
+        self.assertEqual(test_zone_2.name, "unit-tests-3")