From e0c4e60c431352d6dab0c456fc8e65829dad35cc Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 15 Oct 2018 16:29:36 -0700 Subject: [PATCH 1/5] Vastly improved CloudflareProvider _apply_Update, much safer --- octodns/provider/cloudflare.py | 149 +++++++++++++--------- tests/test_octodns_provider_cloudflare.py | 76 +++++++---- 2 files changed, 140 insertions(+), 85 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index ecf3e3f..d008084 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -323,7 +323,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) @@ -345,88 +345,111 @@ class CloudflareProvider(BaseProvider): 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): + def _hash_data(self, data): # 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)) + return hash(dumps(data, 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) + # Note that all CF records have a `content` field the value of which + # appears to be 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. 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 + existing[data['content']] = { + 'record_id': record['id'], + 'data': data, + } + + # Build up a list of new CF records for this Update + new = { + d['content']: 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... + + # 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 diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 7462b9f..71a7e92 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -287,14 +287,16 @@ 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('POST', '/zones/42/dns_records', data={ + 'content': '3.2.3.4', + 'type': 'A', + 'name': 'ttl.unit.tests', + 'ttl': 300 + }), + call('DELETE', + '/zones/42/dns_records/fc12ab34cd5611334422ab3322997655'), call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/' 'dns_records/fc12ab34cd5611334422ab3322997653'), call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/' @@ -351,6 +353,8 @@ class TestCloudflareProvider(TestCase): }, # zone create None, None, + None, + None, ] # Add something and delete something @@ -371,17 +375,35 @@ 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': '3.3.3.3', + 'type': 'A', + 'name': 'a.unit.tests', + 'ttl': 300 + }), + 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('DELETE', '/zones/42/dns_records/' + 'fc12ab34cd5611334422ab3322997653') ]) def test_update_delete(self): @@ -456,12 +478,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,9 +530,9 @@ 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' From c75200585637afc58a0d8213eb5fcf8bb09c6a48 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 15 Oct 2018 16:34:25 -0700 Subject: [PATCH 2/5] CloudflareProvider._hash_data is no longer used --- octodns/provider/cloudflare.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index d008084..6a42668 100644 --- a/octodns/provider/cloudflare.py +++ b/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 @@ -348,11 +347,6 @@ class CloudflareProvider(BaseProvider): for content in self._gen_data(new): self._request('POST', path, data=content) - def _hash_data(self, data): - # 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(data, sort_keys=True)) - def _apply_Update(self, change): # Note that all CF records have a `content` field the value of which # appears to be a unique/hashable string for the record's. It includes From db8e291d5325fc99ad26f4bed6857514f135de51 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 15 Oct 2018 19:51:26 -0700 Subject: [PATCH 3/5] Implement CloudflareProvider create + delete -> update conversion --- octodns/provider/cloudflare.py | 18 ++++++++++++++ tests/test_octodns_provider_cloudflare.py | 30 +++++++++++------------ 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index 6a42668..b6df5c4 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -417,6 +417,24 @@ class CloudflareProvider(BaseProvider): # 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 diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 71a7e92..f03f87c 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -289,14 +289,13 @@ class TestCloudflareProvider(TestCase): self.assertTrue(plan.exists) # creates a the new value and then deletes all the old provider._request.assert_has_calls([ - call('POST', '/zones/42/dns_records', data={ - 'content': '3.2.3.4', - 'type': 'A', - 'name': 'ttl.unit.tests', - 'ttl': 300 - }), - call('DELETE', - '/zones/42/dns_records/fc12ab34cd5611334422ab3322997655'), + 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/' @@ -383,12 +382,6 @@ class TestCloudflareProvider(TestCase): 'jump_start': False, 'name': 'unit.tests' }), - call('POST', '/zones/42/dns_records', data={ - 'content': '3.3.3.3', - 'type': 'A', - 'name': 'a.unit.tests', - 'ttl': 300 - }), call('POST', '/zones/42/dns_records', data={ 'content': '4.4.4.4', 'type': 'A', @@ -402,8 +395,13 @@ class TestCloudflareProvider(TestCase): 'name': 'a.unit.tests', 'ttl': 300 }), - call('DELETE', '/zones/42/dns_records/' - 'fc12ab34cd5611334422ab3322997653') + 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): From 0c33d3acac47fb0bf4bc82dbad20f06fef97c6f6 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 15 Oct 2018 20:18:14 -0700 Subject: [PATCH 4/5] Handle the MX special case around content --- octodns/provider/cloudflare.py | 28 +++++++++++++++-------- tests/test_octodns_provider_cloudflare.py | 14 ++++++++++++ 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index b6df5c4..1d7c1b2 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -340,14 +340,7 @@ class CloudflareProvider(BaseProvider): }) yield 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_data(new): - self._request('POST', path, data=content) - - def _apply_Update(self, change): + def _gen_key(self, data): # Note that all CF records have a `content` field the value of which # appears to be a unique/hashable string for the record's. It includes # all the "value" bits, but not the secondary stuff like TTL's. E.g. @@ -356,7 +349,21 @@ class CloudflareProvider(BaseProvider): # 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's an exception. For some reason MX doesn't include + # priority in its `content` so it's an odd-ball. I haven't found any + # others so hopefully they don't exist :-( + if data['type'] == 'MX': + return '{} {}'.format(data['priority'], data['content']) + 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_data(new): + self._request('POST', path, data=content) + + def _apply_Update(self, change): zone = change.new.zone zone_id = self.zones[zone.name] hostname = zone.hostname_from_fqdn(change.new.fqdn[:-1]) @@ -376,14 +383,15 @@ class CloudflareProvider(BaseProvider): data = self._gen_data(r).next() # Record the record_id and data for this existing record - existing[data['content']] = { + 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 = { - d['content']: d for d in self._gen_data(change.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 diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index f03f87c..a04b6d3 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -536,6 +536,20 @@ class TestCloudflareProvider(TestCase): 'type': 'CNAME' }, list(contents)[0]) + def test_gen_key(self): + provider = CloudflareProvider('test', 'email', 'token') + + self.assertEqual('10 foo.bar.com.', provider._gen_key({ + 'content': 'foo.bar.com.', + 'priority': 10, + 'type': 'MX', + })) + + self.assertEqual('foo.bar.com.', provider._gen_key({ + 'content': 'foo.bar.com.', + 'type': 'CNAME', + })) + def test_cdn(self): provider = CloudflareProvider('test', 'email', 'token', True) From aee786dd017970b37e52ba878edac972205fe414 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 16 Oct 2018 07:08:01 -0700 Subject: [PATCH 5/5] Explicit handling of SRV & CAA in _gen_key, tests for those cases --- octodns/provider/cloudflare.py | 33 +++++++++++-------- tests/test_octodns_provider_cloudflare.py | 39 +++++++++++++++++------ 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index 1d7c1b2..50e4b90 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -341,19 +341,26 @@ class CloudflareProvider(BaseProvider): yield content def _gen_key(self, data): - # Note that all CF records have a `content` field the value of which - # appears to be 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's an exception. For some reason MX doesn't include - # priority in its `content` so it's an odd-ball. I haven't found any - # others so hopefully they don't exist :-( - if data['type'] == 'MX': - return '{} {}'.format(data['priority'], data['content']) + # 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): diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index a04b6d3..0f769a4 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -539,16 +539,35 @@ class TestCloudflareProvider(TestCase): def test_gen_key(self): provider = CloudflareProvider('test', 'email', 'token') - self.assertEqual('10 foo.bar.com.', provider._gen_key({ - 'content': 'foo.bar.com.', - 'priority': 10, - 'type': 'MX', - })) - - self.assertEqual('foo.bar.com.', provider._gen_key({ - 'content': 'foo.bar.com.', - 'type': 'CNAME', - })) + 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)