From fd9af2bd25ff6a38344773d507f4cfadd21da3a6 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 27 Dec 2017 09:54:18 -0800 Subject: [PATCH 1/2] Major reworking of Cloudflare record update --- octodns/provider/cloudflare.py | 107 +++++++++++++++++++--- tests/test_octodns_provider_cloudflare.py | 13 ++- 2 files changed, 101 insertions(+), 19 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index dd53b3a..a7ec5d8 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -7,6 +7,7 @@ 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 @@ -232,25 +233,107 @@ class CloudflareProvider(BaseProvider): 'content': value.exchange } + def _gen_contents(self, record): + name = record.fqdn[:-1] + _type = record._type + ttl = max(self.MIN_TTL, record.ttl) + + contents_for = getattr(self, '_contents_for_{}'.format(_type)) + for content in contents_for(record): + content.update({ + 'name': name, + 'type': _type, + 'ttl': ttl, + }) + yield content + def _apply_Create(self, change): new = change.new zone_id = self.zones[new.zone.name] - contents_for = getattr(self, '_contents_for_{}'.format(new._type)) path = '/zones/{}/dns_records'.format(zone_id) - name = new.fqdn[:-1] - for content in contents_for(change.new): - content.update({ - 'name': name, - 'type': new._type, - # Cloudflare has a min ttl of 120s - 'ttl': max(self.MIN_TTL, new.ttl), - }) + for content in self._gen_contents(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): - # Create the new and delete the old - self._apply_Create(change) - self._apply_Delete(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) + } + + # We need a list of keys to consider for diffs, use the first content + # before we muck with anything + keys = existing_contents.values()[0].keys() + + # 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_id = self.zones[change.new.zone.name] + + # Find things we need to remove + name = change.new.fqdn[:-1] + _type = change.new._type + # OK, work through each record from the zone + for record in self.zone_records(change.new.zone): + if name == record['name'] and _type == record['type']: + # This is match for our name and type, we need to look at + # contents now, build a dict of the relevant keys and vals + content = {} + for k in keys: + content[k] = record[k] + # :-( + if _type in ('CNAME', 'MX', 'NS'): + content['content'] += '.' + # 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 + 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) 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 ef8a51c..defcdea 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -267,13 +267,12 @@ class TestCloudflareProvider(TestCase): self.assertEquals(2, provider.apply(plan)) # recreate for update, and deletes for the 2 parts of the other 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/ff12ab34cd5611334422ab3322997650/' - 'dns_records/fc12ab34cd5611334422ab3322997655'), + call('PUT', '/zones/ff12ab34cd5611334422ab3322997650/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/' From 3c3f63b450e5e66ea8e070bd7055fcc73b91c114 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 6 Jan 2018 15:51:48 -0800 Subject: [PATCH 2/2] Unit tests for reworked Cloudflare updates --- tests/test_octodns_provider_cloudflare.py | 166 +++++++++++++++++++++- 1 file changed, 165 insertions(+), 1 deletion(-) diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index defcdea..8a17083 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -11,7 +11,8 @@ from requests import HTTPError from requests_mock import ANY, mock as requests_mock from unittest import TestCase -from octodns.record import Record +from octodns.record import Record, Update +from octodns.provider.base import Plan from octodns.provider.cloudflare import CloudflareProvider from octodns.provider.yaml import YamlProvider from octodns.zone import Zone @@ -278,3 +279,166 @@ class TestCloudflareProvider(TestCase): call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/' 'dns_records/fc12ab34cd5611334422ab3322997654') ]) + + def test_update_add_swap(self): + provider = CloudflareProvider('test', 'email', 'token') + + provider.zone_records = Mock(return_value=[ + { + "id": "fc12ab34cd5611334422ab3322997653", + "type": "A", + "name": "a.unit.tests", + "content": "1.1.1.1", + "proxiable": True, + "proxied": False, + "ttl": 300, + "locked": False, + "zone_id": "ff12ab34cd5611334422ab3322997650", + "zone_name": "unit.tests", + "modified_on": "2017-03-11T18:01:43.420689Z", + "created_on": "2017-03-11T18:01:43.420689Z", + "meta": { + "auto_added": False + } + }, + { + "id": "fc12ab34cd5611334422ab3322997654", + "type": "A", + "name": "a.unit.tests", + "content": "2.2.2.2", + "proxiable": True, + "proxied": False, + "ttl": 300, + "locked": False, + "zone_id": "ff12ab34cd5611334422ab3322997650", + "zone_name": "unit.tests", + "modified_on": "2017-03-11T18:01:43.420689Z", + "created_on": "2017-03-11T18:01:43.420689Z", + "meta": { + "auto_added": False + } + }, + ]) + + provider._request = Mock() + provider._request.side_effect = [ + self.empty, # no zones + { + 'result': { + 'id': 42, + } + }, # zone create + None, + None, + ] + + # Add something and delete something + zone = Zone('unit.tests.', []) + existing = Record.new(zone, 'a', { + 'ttl': 300, + 'type': 'A', + # This matches the zone data above, one to swap, one to leave + 'values': ['1.1.1.1', '2.2.2.2'], + }) + new = Record.new(zone, 'a', { + 'ttl': 300, + 'type': 'A', + # This leaves one, swaps ones, and adds one + 'values': ['2.2.2.2', '3.3.3.3', '4.4.4.4'], + }) + change = Update(existing, new) + plan = Plan(zone, zone, [change]) + provider._apply(plan) + + 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}) + ]) + + def test_update_delete(self): + # We need another run so that we can delete, we can't both add and + # delete in one go b/c of swaps + provider = CloudflareProvider('test', 'email', 'token') + + provider.zone_records = Mock(return_value=[ + { + "id": "fc12ab34cd5611334422ab3322997653", + "type": "NS", + "name": "unit.tests", + "content": "ns1.foo.bar", + "proxiable": True, + "proxied": False, + "ttl": 300, + "locked": False, + "zone_id": "ff12ab34cd5611334422ab3322997650", + "zone_name": "unit.tests", + "modified_on": "2017-03-11T18:01:43.420689Z", + "created_on": "2017-03-11T18:01:43.420689Z", + "meta": { + "auto_added": False + } + }, + { + "id": "fc12ab34cd5611334422ab3322997654", + "type": "NS", + "name": "unit.tests", + "content": "ns2.foo.bar", + "proxiable": True, + "proxied": False, + "ttl": 300, + "locked": False, + "zone_id": "ff12ab34cd5611334422ab3322997650", + "zone_name": "unit.tests", + "modified_on": "2017-03-11T18:01:43.420689Z", + "created_on": "2017-03-11T18:01:43.420689Z", + "meta": { + "auto_added": False + } + }, + ]) + + provider._request = Mock() + provider._request.side_effect = [ + self.empty, # no zones + { + 'result': { + 'id': 42, + } + }, # zone create + None, + None, + ] + + # Add something and delete something + zone = Zone('unit.tests.', []) + existing = Record.new(zone, '', { + 'ttl': 300, + 'type': 'NS', + # This matches the zone data above, one to delete, one to leave + 'values': ['ns1.foo.bar.', 'ns2.foo.bar.'], + }) + new = Record.new(zone, '', { + 'ttl': 300, + 'type': 'NS', + # This leaves one and deletes one + 'value': 'ns2.foo.bar.', + }) + change = Update(existing, new) + plan = Plan(zone, zone, [change]) + provider._apply(plan) + + 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') + ])