mirror/github-octodns
1
0
Fork 0
mirror of https://github.com/github/octodns.git synced 2024-05-11 05:55:00 +00:00
Code Issues Releases Activity

Vastly improved CloudflareProvider _apply_Update, much safer

Browse Source
This commit is contained in:
Ross McFarland
2018-10-15 16:29:36 -07:00
parent c66feba63f
commit e0c4e60c43
2 changed files with 138 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files

145
octodns/provider/cloudflare.py
View File

@@ -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()
data = self._gen_data(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)
# Record the record_id and data for this existing record
existing[data['content']] = {
'record_id': record['id'],
'data': data,
}
# Any remaining adds just need to be created
# 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

76
tests/test_octodns_provider_cloudflare.py
View File

@@ -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'