From b5e7af0398404c612093a249510da217efac3c51 Mon Sep 17 00:00:00 2001 From: Paul van Brouwershaven Date: Wed, 31 Jan 2018 09:57:36 +0100 Subject: [PATCH 1/5] Option to handle Cloudflare proxied records This change imports records that are marked as proxied so that they can be synced to other DNS providers as described in [this support acticle](https://support.cloudflare.com/hc/en-us/articles/115000830351-How-to-configure-DNS-for-CNAME-partial-setup-when-managing-DNS-externally). Records that use this functionality will be ignored by this provider and not be synced back to Cloudflare as we don't know the origin record values that would be required. This change does not allow you to enable, disable or configure the CDN itself as that would require a lot of metadata to be handled by OctoDNS. The intention of this change is to allow users to run a multi-DNS provider setup without sending any traffic to their origin directly. See also github/octodns#45 --- octodns/provider/cloudflare.py | 58 ++++++++--- tests/test_octodns_provider_cloudflare.py | 117 ++++++++++++++++++++++ 2 files changed, 162 insertions(+), 13 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index 9dfef6d..edce0ff 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -14,14 +14,14 @@ from ..record import Record, Update from .base import BaseProvider -class CloudflareAuthenticationError(Exception): +class CloudflareError(Exception): def __init__(self, data): try: message = data['errors'][0]['message'] except (IndexError, KeyError): message = 'Authentication error' - super(CloudflareAuthenticationError, self).__init__(message) + super(CloudflareError, self).__init__(message) class CloudflareProvider(BaseProvider): @@ -34,6 +34,12 @@ class CloudflareProvider(BaseProvider): email: dns-manager@example.com # The api key (required) token: foo + # Import CDN enabled records as CNAME to {}.cdn.cloudflare.net. Records + # ending at .cdn.cloudflare.net. will be ignored when this provider is + # not used as the source and the cdn option is enabled. + # + # See: https://support.cloudflare.com/hc/en-us/articles/115000830351 + cdn: false ''' SUPPORTS_GEO = False # TODO: support SRV @@ -43,9 +49,10 @@ class CloudflareProvider(BaseProvider): MIN_TTL = 120 TIMEOUT = 15 - def __init__(self, id, email, token, *args, **kwargs): + def __init__(self, id, email, token, cdn=False, *args, **kwargs): self.log = getLogger('CloudflareProvider[{}]'.format(id)) - self.log.debug('__init__: id=%s, email=%s, token=***', id, email) + self.log.debug('__init__: id=%s, email=%s, token=***, cdn=%s', id, + email, cdn) super(CloudflareProvider, self).__init__(id, *args, **kwargs) sess = Session() @@ -53,6 +60,7 @@ class CloudflareProvider(BaseProvider): 'X-Auth-Email': email, 'X-Auth-Key': token, }) + self.cdn = cdn self._sess = sess self._zones = None @@ -65,8 +73,8 @@ class CloudflareProvider(BaseProvider): resp = self._sess.request(method, url, params=params, json=data, timeout=self.TIMEOUT) self.log.debug('_request: status=%d', resp.status_code) - if resp.status_code == 403: - raise CloudflareAuthenticationError(resp.json()) + if resp.status_code == 400 or resp.status_code == 403: + raise CloudflareError(resp.json()) resp.raise_for_status() return resp.json() @@ -88,6 +96,18 @@ class CloudflareProvider(BaseProvider): return self._zones + def _data_for_cdn(self, name, _type, records): + self.log.info('CDN rewrite for %s', records[0]['name']) + _type = "CNAME" + if name == "": + _type = "ALIAS" + + return { + 'ttl': records[0]['ttl'], + 'type': _type, + 'value': '{}.cdn.cloudflare.net.'.format(records[0]['name']), + } + def _data_for_multiple(self, _type, records): return { 'ttl': records[0]['ttl'], @@ -170,8 +190,8 @@ class CloudflareProvider(BaseProvider): return self._zone_records[zone.name] def populate(self, zone, target=False, lenient=False): - self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, - target, lenient) + self.log.debug('populate: name=%s, cdn=%s, target=%s, lenient=%s', + zone.name, self.cdn, target, lenient) before = len(zone.records) records = self.zone_records(zone) @@ -186,12 +206,18 @@ class CloudflareProvider(BaseProvider): for name, types in values.items(): for _type, records in types.items(): - # Cloudflare supports ALIAS semantics with root CNAMEs - if _type == 'CNAME' and name == '': - _type = 'ALIAS' + # rewrite Cloudflare proxied records + if self.cdn and records[0]['proxied']: + data = self._data_for_cdn(name, _type, records) + + else: + # Cloudflare supports ALIAS semantics with root CNAMEs + if _type == 'CNAME' and name == '': + _type = 'ALIAS' + + data_for = getattr(self, '_data_for_{}'.format(_type)) + data = data_for(_type, records) - data_for = getattr(self, '_data_for_{}'.format(_type)) - data = data_for(_type, records) record = Record.new(zone, name, data, source=self, lenient=lenient) zone.add_record(record) @@ -250,6 +276,12 @@ class CloudflareProvider(BaseProvider): if _type == 'ALIAS': _type = 'CNAME' + # If this is a record to enable to Cloudflare CDN don't update as we + # don't know the original values. + if (self.cdn and _type == 'CNAME' and + record.value.endswith('.cdn.cloudflare.net.')): + raise StopIteration + contents_for = getattr(self, '_contents_for_{}'.format(_type)) for content in contents_for(record): content.update({ diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 824af9d..f51f805 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -484,3 +484,120 @@ class TestCloudflareProvider(TestCase): 'ttl': 300, 'type': 'CNAME' }, list(contents)[0]) + + def test_cdn(self): + provider = CloudflareProvider('test', 'email', 'token', True) + + # A CNAME for us to transform to ALIAS + provider.zone_records = Mock(return_value=[ + { + "id": "fc12ab34cd5611334422ab3322997642", + "type": "CNAME", + "name": "cname.unit.tests", + "content": "www.unit.tests", + "proxiable": True, + "proxied": True, + "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": "fc12ab34cd5611334422ab3322997642", + "type": "A", + "name": "a.unit.tests", + "content": "1.1.1.1", + "proxiable": True, + "proxied": True, + "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": "fc12ab34cd5611334422ab3322997642", + "type": "A", + "name": "a.unit.tests", + "content": "1.1.1.2", + "proxiable": True, + "proxied": True, + "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 + } + }, + ]) + + zone = Zone('unit.tests.', []) + provider.populate(zone) + + # the two A records get merged into one CNAME record poining to the CDN + self.assertEquals(2, len(zone.records)) + + record = list(zone.records)[0] + self.assertEquals('cname', record.name) + self.assertEquals('cname.unit.tests.', record.fqdn) + self.assertEquals('CNAME', record._type) + self.assertEquals('cname.unit.tests.cdn.cloudflare.net.', record.value) + + record = list(zone.records)[1] + self.assertEquals('a', record.name) + self.assertEquals('a.unit.tests.', record.fqdn) + self.assertEquals('CNAME', record._type) + self.assertEquals('a.unit.tests.cdn.cloudflare.net.', record.value) + + # CDN enabled records can't be updated, we don't know the real values + contents = provider._gen_contents(record) + self.assertEquals(0, len(list(contents))) + + def test_cdn_alias(self): + provider = CloudflareProvider('test', 'email', 'token', True) + + # A CNAME for us to transform to ALIAS + provider.zone_records = Mock(return_value=[ + { + "id": "fc12ab34cd5611334422ab3322997642", + "type": "CNAME", + "name": "unit.tests", + "content": "www.unit.tests", + "proxiable": True, + "proxied": True, + "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 + } + }, + ]) + + zone = Zone('unit.tests.', []) + provider.populate(zone) + self.assertEquals(1, len(zone.records)) + record = list(zone.records)[0] + self.assertEquals('', record.name) + self.assertEquals('unit.tests.', record.fqdn) + self.assertEquals('ALIAS', record._type) + self.assertEquals('unit.tests.cdn.cloudflare.net.', record.value) + + # CDN enabled records can't be updated, we don't know the real values + contents = provider._gen_contents(record) + self.assertEquals(0, len(list(contents))) From 8a7145f49f48c05e294b9b0767c35cdc9833e639 Mon Sep 17 00:00:00 2001 From: Paul van Brouwershaven Date: Wed, 7 Feb 2018 14:53:18 +0100 Subject: [PATCH 2/5] Changes according to review --- octodns/provider/cloudflare.py | 33 ++++++++++++++--------- tests/test_octodns_provider_cloudflare.py | 31 +++++++++++++++++---- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index edce0ff..42b42b6 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -15,15 +15,19 @@ from .base import BaseProvider class CloudflareError(Exception): - def __init__(self, data): try: message = data['errors'][0]['message'] except (IndexError, KeyError): - message = 'Authentication error' + message = 'Cloudflare error' super(CloudflareError, self).__init__(message) +class CloudflareAuthenticationError(CloudflareError): + def __init__(self, data): + CloudflareError.__init__(self, data) + + class CloudflareProvider(BaseProvider): ''' Cloudflare DNS provider @@ -53,7 +57,7 @@ class CloudflareProvider(BaseProvider): self.log = getLogger('CloudflareProvider[{}]'.format(id)) self.log.debug('__init__: id=%s, email=%s, token=***, cdn=%s', id, email, cdn) - super(CloudflareProvider, self).__init__(id, *args, **kwargs) + super(CloudflareProvider, self).__init__(id, cdn, *args, **kwargs) sess = Session() sess.headers.update({ @@ -73,8 +77,11 @@ class CloudflareProvider(BaseProvider): resp = self._sess.request(method, url, params=params, json=data, timeout=self.TIMEOUT) self.log.debug('_request: status=%d', resp.status_code) - if resp.status_code == 400 or resp.status_code == 403: + if resp.status_code == 400: raise CloudflareError(resp.json()) + if resp.status_code == 403: + raise CloudflareAuthenticationError(resp.json()) + resp.raise_for_status() return resp.json() @@ -190,8 +197,8 @@ class CloudflareProvider(BaseProvider): return self._zone_records[zone.name] def populate(self, zone, target=False, lenient=False): - self.log.debug('populate: name=%s, cdn=%s, target=%s, lenient=%s', - zone.name, self.cdn, target, lenient) + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) before = len(zone.records) records = self.zone_records(zone) @@ -232,6 +239,14 @@ class CloudflareProvider(BaseProvider): new['ttl'] = max(120, new['ttl']) if new == existing: return False + + # If this is a record to enable to Cloudflare CDN don't update as + # we don't know the original values. + if (hasattr(change.new, '_type') and (change.new._type == 'CNAME' or + change.new._type == 'ALIAS') and + change.new.value.endswith('.cdn.cloudflare.net.')): + return False + return True def _contents_for_multiple(self, record): @@ -276,12 +291,6 @@ class CloudflareProvider(BaseProvider): if _type == 'ALIAS': _type = 'CNAME' - # If this is a record to enable to Cloudflare CDN don't update as we - # don't know the original values. - if (self.cdn and _type == 'CNAME' and - record.value.endswith('.cdn.cloudflare.net.')): - raise StopIteration - contents_for = getattr(self, '_contents_for_{}'.format(_type)) for content in contents_for(record): content.update({ diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index f51f805..a8477eb 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -62,7 +62,7 @@ class TestCloudflareProvider(TestCase): with self.assertRaises(Exception) as ctx: zone = Zone('unit.tests.', []) provider.populate(zone) - self.assertEquals('Authentication error', ctx.exception.message) + self.assertEquals('Cloudflare error', ctx.exception.message) # General error with requests_mock() as mock: @@ -562,8 +562,21 @@ class TestCloudflareProvider(TestCase): self.assertEquals('a.unit.tests.cdn.cloudflare.net.', record.value) # CDN enabled records can't be updated, we don't know the real values - contents = provider._gen_contents(record) - self.assertEquals(0, len(list(contents))) + # never point a Cloudflare record to itsself. + wanted = Zone('unit.tests.', []) + wanted.add_record(Record.new(wanted, 'cname', { + 'ttl': 300, + 'type': 'CNAME', + 'value': 'change.unit.tests.cdn.cloudflare.net.' + })) + wanted.add_record(Record.new(wanted, 'new', { + 'ttl': 300, + 'type': 'CNAME', + 'value': 'new.unit.tests.cdn.cloudflare.net.' + })) + + plan = provider.plan(wanted) + self.assertEquals(1, len(plan.changes)) def test_cdn_alias(self): provider = CloudflareProvider('test', 'email', 'token', True) @@ -599,5 +612,13 @@ class TestCloudflareProvider(TestCase): self.assertEquals('unit.tests.cdn.cloudflare.net.', record.value) # CDN enabled records can't be updated, we don't know the real values - contents = provider._gen_contents(record) - self.assertEquals(0, len(list(contents))) + # never point a Cloudflare record to itsself. + wanted = Zone('unit.tests.', []) + wanted.add_record(Record.new(wanted, '', { + 'ttl': 300, + 'type': 'ALIAS', + 'value': 'change.unit.tests.cdn.cloudflare.net.' + })) + + plan = provider.plan(wanted) + self.assertEquals(False, hasattr(plan, 'changes')) From c4179ef0e8c78c5dd9d6ebb242a162400fbb6337 Mon Sep 17 00:00:00 2001 From: Paul van Brouwershaven Date: Wed, 7 Feb 2018 17:35:19 +0100 Subject: [PATCH 3/5] Allow proxied records with the same name --- octodns/provider/cloudflare.py | 21 ++++++++-- tests/test_octodns_provider_cloudflare.py | 49 ++++++++++++++++++++++- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index 42b42b6..3c1f64e 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -227,6 +227,15 @@ class CloudflareProvider(BaseProvider): record = Record.new(zone, name, data, source=self, lenient=lenient) + + # only one rewrite is needed for names where the proxy is + # enabled at multiple records with a different type but + # the same name + if (self.cdn and records[0]['proxied'] and + record in zone._records[name]): + self.log.info('CDN rewrite %s already in zone', name) + continue + zone.add_record(record) self.log.info('populate: found %s records', @@ -240,12 +249,18 @@ class CloudflareProvider(BaseProvider): if new == existing: return False - # If this is a record to enable to Cloudflare CDN don't update as + # If this is a record to enable Cloudflare CDN don't update as # we don't know the original values. - if (hasattr(change.new, '_type') and (change.new._type == 'CNAME' or - change.new._type == 'ALIAS') and + if (hasattr(change.new, '_type') and + (change.new._type == 'CNAME' or + change.new._type == 'ALIAS') and change.new.value.endswith('.cdn.cloudflare.net.')): return False + if (hasattr(change.existing, '_type') and + (change.existing._type == 'CNAME' or + change.existing._type == 'ALIAS') and + change.existing.value.endswith('.cdn.cloudflare.net.')): + return False return True diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index a8477eb..8d07b68 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -541,21 +541,61 @@ class TestCloudflareProvider(TestCase): "auto_added": False } }, + { + "id": "fc12ab34cd5611334422ab3322997642", + "type": "A", + "name": "multi.unit.tests", + "content": "1.1.1.3", + "proxiable": True, + "proxied": True, + "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": "fc12ab34cd5611334422ab3322997642", + "type": "AAAA", + "name": "multi.unit.tests", + "content": "::1", + "proxiable": True, + "proxied": True, + "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 + } + }, ]) zone = Zone('unit.tests.', []) provider.populate(zone) # the two A records get merged into one CNAME record poining to the CDN - self.assertEquals(2, len(zone.records)) + self.assertEquals(3, len(zone.records)) record = list(zone.records)[0] + self.assertEquals('multi', record.name) + self.assertEquals('multi.unit.tests.', record.fqdn) + self.assertEquals('CNAME', record._type) + self.assertEquals('multi.unit.tests.cdn.cloudflare.net.', record.value) + + record = list(zone.records)[1] self.assertEquals('cname', record.name) self.assertEquals('cname.unit.tests.', record.fqdn) self.assertEquals('CNAME', record._type) self.assertEquals('cname.unit.tests.cdn.cloudflare.net.', record.value) - record = list(zone.records)[1] + record = list(zone.records)[2] self.assertEquals('a', record.name) self.assertEquals('a.unit.tests.', record.fqdn) self.assertEquals('CNAME', record._type) @@ -574,6 +614,11 @@ class TestCloudflareProvider(TestCase): 'type': 'CNAME', 'value': 'new.unit.tests.cdn.cloudflare.net.' })) + wanted.add_record(Record.new(wanted, 'created', { + 'ttl': 300, + 'type': 'CNAME', + 'value': 'www.unit.tests.' + })) plan = provider.plan(wanted) self.assertEquals(1, len(plan.changes)) From 6f0b0ddb08c927bb2d65038330d7fb503a4b70ad Mon Sep 17 00:00:00 2001 From: Paul van Brouwershaven Date: Thu, 8 Feb 2018 08:30:27 +0100 Subject: [PATCH 4/5] Test different exception types --- tests/test_octodns_provider_cloudflare.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 8d07b68..e406f94 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -42,6 +42,20 @@ class TestCloudflareProvider(TestCase): def test_populate(self): provider = CloudflareProvider('test', 'email', 'token') + # Bad requests + with requests_mock() as mock: + mock.get(ANY, status_code=400, + text='{"success":false,"errors":[{"code":1101,' + '"message":"request was invalid"}],' + '"messages":[],"result":null}') + + with self.assertRaises(Exception) as ctx: + zone = Zone('unit.tests.', []) + provider.populate(zone) + + self.assertEquals('CloudflareError', type(ctx.exception).__name__) + self.assertEquals('request was invalid', ctx.exception.message) + # Bad auth with requests_mock() as mock: mock.get(ANY, status_code=403, @@ -52,6 +66,8 @@ class TestCloudflareProvider(TestCase): with self.assertRaises(Exception) as ctx: zone = Zone('unit.tests.', []) provider.populate(zone) + self.assertEquals('CloudflareAuthenticationError', + type(ctx.exception).__name__) self.assertEquals('Unknown X-Auth-Key or X-Auth-Email', ctx.exception.message) @@ -62,6 +78,8 @@ class TestCloudflareProvider(TestCase): with self.assertRaises(Exception) as ctx: zone = Zone('unit.tests.', []) provider.populate(zone) + self.assertEquals('CloudflareAuthenticationError', + type(ctx.exception).__name__) self.assertEquals('Cloudflare error', ctx.exception.message) # General error From c6634b3ccc41162e3f07c552fb786431aa0882bc Mon Sep 17 00:00:00 2001 From: Paul van Brouwershaven Date: Fri, 9 Feb 2018 13:44:58 +0100 Subject: [PATCH 5/5] Simplify _include_change check --- octodns/provider/cloudflare.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index 3c1f64e..a9264e9 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -251,15 +251,8 @@ class CloudflareProvider(BaseProvider): # If this is a record to enable Cloudflare CDN don't update as # we don't know the original values. - if (hasattr(change.new, '_type') and - (change.new._type == 'CNAME' or - change.new._type == 'ALIAS') and - change.new.value.endswith('.cdn.cloudflare.net.')): - return False - if (hasattr(change.existing, '_type') and - (change.existing._type == 'CNAME' or - change.existing._type == 'ALIAS') and - change.existing.value.endswith('.cdn.cloudflare.net.')): + if (change.record._type in ('ALIAS', 'CNAME') and + change.record.value.endswith('.cdn.cloudflare.net.')): return False return True