Merge pull request #277 from github/fix-cf-record-update-order

Vastly improved CloudflareProvider _apply_Update, much safer

octodns/provider/cloudflare.py

@@ -7,7 +7,6 @@ from __future__ import absolute_import, division, print_function, \
 
 from collections import defaultdict
 from logging import getLogger
-from json import dumps
 from requests import Session
 
 from ..record import Record, Update
@@ -323,7 +322,7 @@ class CloudflareProvider(BaseProvider):
                 }
             }
 
-    def _gen_contents(self, record):
+    def _gen_data(self, record):
         name = record.fqdn[:-1]
         _type = record._type
         ttl = max(self.MIN_TTL, record.ttl)
@@ -341,92 +340,143 @@ class CloudflareProvider(BaseProvider):
             })
             yield content
 
+    def _gen_key(self, data):
+        # Note that most CF record data has a `content` field the value of
+        # which is a unique/hashable string for the record's. It includes all
+        # the "value" bits, but not the secondary stuff like TTL's. E.g.  for
+        # an A it'll include the value, for a CAA it'll include the flags, tag,
+        # and value, ... We'll take advantage of this to try and match up old &
+        # new records cleanly. In general when there are multiple records for a
+        # name & type each will have a distinct/consistent `content` that can
+        # serve as a unique identifier.
+        # BUT... there are exceptions. MX, CAA, and SRV don't have a simple
+        # content as things are currently implemented so we need to handle
+        # those explicitly and create unique/hashable strings for them.
+        _type = data['type']
+        if _type == 'MX':
+            return '{priority} {content}'.format(**data)
+        elif _type == 'CAA':
+            data = data['data']
+            return '{flags} {tag} {value}'.format(**data)
+        elif _type == 'SRV':
+            data = data['data']
+            return '{port} {priority} {target} {weight}'.format(**data)
+        return data['content']
+
     def _apply_Create(self, change):
         new = change.new
         zone_id = self.zones[new.zone.name]
         path = '/zones/{}/dns_records'.format(zone_id)
-        for content in self._gen_contents(new):
+        for content in self._gen_data(new):
             self._request('POST', path, data=content)
 
-    def _hash_content(self, content):
-        # Some of the dicts are nested so this seems about as good as any
-        # option we have for consistently hashing them (within a single run)
-        return hash(dumps(content, sort_keys=True))
-
     def _apply_Update(self, change):
-
-        # Ugh, this is pretty complicated and ugly, mainly due to the
-        # sub-optimal API/semantics. Ideally we'd have a batch change API like
-        # Route53's to make this 100% clean and safe without all this PITA, but
-        # we don't so we'll have to work around that and manually do it as
-        # safely as possible. Note this still isn't perfect as we don't/can't
-        # practically take into account things like the different "types" of
-        # CAA records so when we "swap" there may be brief periods where things
-        # are invalid or even worse Cloudflare may update their validations to
-        # prevent dups. I see no clean way around that short of making this
-        # understand 100% of the details of each record type and develop an
-        # individual/specific ordering of changes that prevents it. That'd
-        # probably result in more code than this whole provider currently has
-        # so... :-(
-
-        existing_contents = {
-            self._hash_content(c): c
-            for c in self._gen_contents(change.existing)
-        }
-        new_contents = {
-            self._hash_content(c): c
-            for c in self._gen_contents(change.new)
-        }
-
-        # Find the things we need to add
-        adds = []
-        for k, content in new_contents.items():
-            try:
-                existing_contents.pop(k)
-                self.log.debug('_apply_Update: leaving %s', content)
-            except KeyError:
-                adds.append(content)
-
         zone = change.new.zone
         zone_id = self.zones[zone.name]
-
-        # Find things we need to remove
         hostname = zone.hostname_from_fqdn(change.new.fqdn[:-1])
         _type = change.new._type
-        # OK, work through each record from the zone
+
+        existing = {}
+        # Find all of the existing CF records for this name & type
         for record in self.zone_records(zone):
             name = zone.hostname_from_fqdn(record['name'])
             # Use the _record_for so that we include all of standard
             # conversion logic
             r = self._record_for(zone, name, record['type'], [record], True)
             if hostname == r.name and _type == r._type:
-
                 # Round trip the single value through a record to contents flow
-                # to get a consistent _gen_contents result that matches what
+                # to get a consistent _gen_data result that matches what
                 # went in to new_contents
-                content = self._gen_contents(r).next()
-
-                # If the hash of that dict isn't in new this record isn't
-                # needed
-                if self._hash_content(content) not in new_contents:
-                    rid = record['id']
-                    path = '/zones/{}/dns_records/{}'.format(record['zone_id'],
-                                                             rid)
-                    try:
-                        add_content = adds.pop(0)
-                        self.log.debug('_apply_Update: swapping %s -> %s, %s',
-                                       content, add_content, rid)
-                        self._request('PUT', path, data=add_content)
-                    except IndexError:
-                        self.log.debug('_apply_Update: removing %s, %s',
-                                       content, rid)
-                        self._request('DELETE', path)
-
-        # Any remaining adds just need to be created
+                data = self._gen_data(r).next()
+
+                # Record the record_id and data for this existing record
+                key = self._gen_key(data)
+                existing[key] = {
+                    'record_id': record['id'],
+                    'data': data,
+                }
+
+        # Build up a list of new CF records for this Update
+        new = {
+            self._gen_key(d): d for d in self._gen_data(change.new)
+        }
+
+        # OK we now have a picture of the old & new CF records, our next step
+        # is to figure out which records need to be deleted
+        deletes = {}
+        for key, info in existing.items():
+            if key not in new:
+                deletes[key] = info
+        # Now we need to figure out which records will need to be created
+        creates = {}
+        # And which will be updated
+        updates = {}
+        for key, data in new.items():
+            if key in existing:
+                # To update we need to combine the new data and existing's
+                # record_id. old_data is just for debugging/logging purposes
+                old_info = existing[key]
+                updates[key] = {
+                    'record_id': old_info['record_id'],
+                    'data': data,
+                    'old_data': old_info['data'],
+                }
+            else:
+                creates[key] = data
+
+        # To do this as safely as possible we'll add new things first, update
+        # existing things, and then remove old things. This should (try) and
+        # ensure that we have as many value CF records in their system as
+        # possible at any given time. Ideally we'd have a "batch" API that
+        # would allow create, delete, and upsert style stuff so operations
+        # could be done atomically, but that's not available so we made the
+        # best of it...
+
+        # However, there are record types like CNAME that can only have a
+        # single value. B/c of that our create and then delete approach isn't
+        # actually viable. To address this we'll convert as many creates &
+        # deletes as we can to updates. This will have a minor upside of
+        # resulting in fewer ops and in the case of things like CNAME where
+        # there's a single create and delete result in a single update instead.
+        create_keys = sorted(creates.keys())
+        delete_keys = sorted(deletes.keys())
+        for i in range(0, min(len(create_keys), len(delete_keys))):
+            create_key = create_keys[i]
+            create_data = creates.pop(create_key)
+            delete_info = deletes.pop(delete_keys[i])
+            updates[create_key] = {
+                'record_id': delete_info['record_id'],
+                'data': create_data,
+                'old_data': delete_info['data'],
+            }
+
+        # The sorts ensure a consistent order of operations, they're not
+        # otherwise required, just makes things deterministic
+
+        # Creates
         path = '/zones/{}/dns_records'.format(zone_id)
-        for content in adds:
-            self.log.debug('_apply_Update: adding %s', content)
-            self._request('POST', path, data=content)
+        for _, data in sorted(creates.items()):
+            self.log.debug('_apply_Update: creating %s', data)
+            self._request('POST', path, data=data)
+
+        # Updates
+        for _, info in sorted(updates.items()):
+            record_id = info['record_id']
+            data = info['data']
+            old_data = info['old_data']
+            path = '/zones/{}/dns_records/{}'.format(zone_id, record_id)
+            self.log.debug('_apply_Update: updating %s, %s -> %s',
+                           record_id, data, old_data)
+            self._request('PUT', path, data=data)
+
+        # Deletes
+        for _, info in sorted(deletes.items()):
+            record_id = info['record_id']
+            old_data = info['data']
+            path = '/zones/{}/dns_records/{}'.format(zone_id, record_id)
+            self.log.debug('_apply_Update: removing %s, %s', record_id,
+                           old_data)
+            self._request('DELETE', path)
 
     def _apply_Delete(self, change):
         existing = change.existing

tests/test_octodns_provider_cloudflare.py

@@ -287,14 +287,15 @@ class TestCloudflareProvider(TestCase):
         self.assertEquals(2, len(plan.changes))
         self.assertEquals(2, provider.apply(plan))
         self.assertTrue(plan.exists)
-        # recreate for update, and deletes for the 2 parts of the other
+        # creates a the new value and then deletes all the old
         provider._request.assert_has_calls([
-            call('PUT', '/zones/ff12ab34cd5611334422ab3322997650/dns_records/'
-                 'fc12ab34cd5611334422ab3322997655',
-                 data={'content': '3.2.3.4',
-                       'type': 'A',
-                       'name': 'ttl.unit.tests',
-                       'ttl': 300}),
+            call('PUT', '/zones/42/dns_records/'
+                 'fc12ab34cd5611334422ab3322997655', data={
+                     'content': '3.2.3.4',
+                     'type': 'A',
+                     'name': 'ttl.unit.tests',
+                     'ttl': 300
+                 }),
             call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/'
                  'dns_records/fc12ab34cd5611334422ab3322997653'),
             call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/'
@@ -351,6 +352,8 @@ class TestCloudflareProvider(TestCase):
             },  # zone create
             None,
             None,
+            None,
+            None,
         ]
 
         # Add something and delete something
@@ -371,17 +374,34 @@ class TestCloudflareProvider(TestCase):
         plan = Plan(zone, zone, [change], True)
         provider._apply(plan)
 
+        # get the list of zones, create a zone, add some records, update
+        # something, and delete something
         provider._request.assert_has_calls([
             call('GET', '/zones', params={'page': 1}),
-            call('POST', '/zones', data={'jump_start': False,
-                                         'name': 'unit.tests'}),
-            call('PUT', '/zones/ff12ab34cd5611334422ab3322997650/dns_records/'
-                 'fc12ab34cd5611334422ab3322997653',
-                 data={'content': '4.4.4.4', 'type': 'A', 'name':
-                       'a.unit.tests', 'ttl': 300}),
-            call('POST', '/zones/42/dns_records',
-                 data={'content': '3.3.3.3', 'type': 'A',
-                       'name': 'a.unit.tests', 'ttl': 300})
+            call('POST', '/zones', data={
+                'jump_start': False,
+                'name': 'unit.tests'
+            }),
+            call('POST', '/zones/42/dns_records', data={
+                'content': '4.4.4.4',
+                'type': 'A',
+                'name': 'a.unit.tests',
+                'ttl': 300
+            }),
+            call('PUT', '/zones/42/dns_records/'
+                 'fc12ab34cd5611334422ab3322997654', data={
+                     'content': '2.2.2.2',
+                     'type': 'A',
+                     'name': 'a.unit.tests',
+                     'ttl': 300
+                 }),
+            call('PUT', '/zones/42/dns_records/'
+                 'fc12ab34cd5611334422ab3322997653', data={
+                     'content': '3.3.3.3',
+                     'type': 'A',
+                     'name': 'a.unit.tests',
+                     'ttl': 300
+                 }),
         ])
 
     def test_update_delete(self):
@@ -456,12 +476,22 @@ class TestCloudflareProvider(TestCase):
         plan = Plan(zone, zone, [change], True)
         provider._apply(plan)
 
+        # Get zones, create zone, create a record, delete a record
         provider._request.assert_has_calls([
             call('GET', '/zones', params={'page': 1}),
-            call('POST', '/zones',
-                 data={'jump_start': False, 'name': 'unit.tests'}),
-            call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/'
-                 'dns_records/fc12ab34cd5611334422ab3322997653')
+            call('POST', '/zones', data={
+                'jump_start': False,
+                'name': 'unit.tests'
+            }),
+            call('PUT', '/zones/42/dns_records/'
+                 'fc12ab34cd5611334422ab3322997654', data={
+                     'content': 'ns2.foo.bar.',
+                     'type': 'NS',
+                     'name': 'unit.tests',
+                     'ttl': 300
+                 }),
+            call('DELETE', '/zones/42/dns_records/'
+                 'fc12ab34cd5611334422ab3322997653')
         ])
 
     def test_alias(self):
@@ -498,14 +528,47 @@ class TestCloudflareProvider(TestCase):
         self.assertEquals('www.unit.tests.', record.value)
 
         # Make sure we transform back to CNAME going the other way
-        contents = provider._gen_contents(record)
+        contents = provider._gen_data(record)
         self.assertEquals({
-            'content': u'www.unit.tests.',
+            'content': 'www.unit.tests.',
             'name': 'unit.tests',
             'ttl': 300,
             'type': 'CNAME'
         }, list(contents)[0])
 
+    def test_gen_key(self):
+        provider = CloudflareProvider('test', 'email', 'token')
+
+        for expected, data in (
+            ('foo.bar.com.', {
+                'content': 'foo.bar.com.',
+                'type': 'CNAME',
+            }),
+            ('10 foo.bar.com.', {
+                'content': 'foo.bar.com.',
+                'priority': 10,
+                'type': 'MX',
+            }),
+            ('0 tag some-value', {
+                'data': {
+                    'flags': 0,
+                    'tag': 'tag',
+                    'value': 'some-value',
+                },
+                'type': 'CAA',
+            }),
+            ('42 100 thing-were-pointed.at 101', {
+                'data': {
+                    'port': 42,
+                    'priority': 100,
+                    'target': 'thing-were-pointed.at',
+                    'weight': 101,
+                },
+                'type': 'SRV',
+            }),
+        ):
+            self.assertEqual(expected, provider._gen_key(data))
+
     def test_cdn(self):
         provider = CloudflareProvider('test', 'email', 'token', True)