diff --git a/CHANGELOG.md b/CHANGELOG.md index 40d44ea..e39172f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## v0.0.? - 2024-??-?? - ??? + +* Create DS records after their sibling NS records to appease Cloudflare's + validations +* Throw an error when trying to create a DS without a coresponding NS, + `strict_supports: false` will omit the DS instead + ## v0.0.6 - 2024-05-22 - Deal with unknowns and make more knowns * Fix handling of unsupported record types during apply diff --git a/octodns_cloudflare/__init__.py b/octodns_cloudflare/__init__.py index ac7ad79..dfb3898 100644 --- a/octodns_cloudflare/__init__.py +++ b/octodns_cloudflare/__init__.py @@ -161,9 +161,13 @@ def _request(self, method, path, params=None, data=None): return resp.json() def _change_keyer(self, change): - key = change.__class__.__name__ - order = {'Delete': 0, 'Create': 1, 'Update': 2} - return order[key] + _type = change.record._type + if _type == 'DS' and isinstance(change, Create): + # when creating records in CF the NS for a node must come before the + # DS so we need to flip their order. when deleting they'll already + # be in the required order + _type = 'ZDS' + return (change.CLASS_ORDERING, change.record.name, _type) @property def zones(self): @@ -618,6 +622,24 @@ def _include_change(self, change): return True + def _process_desired_zone(self, desired): + dses = {} + nses = set() + for record in desired.records: + if record._type == 'DS': + dses[record.name] = record + elif record._type == 'NS': + nses.add(record.name) + + for name, record in dses.items(): + if name not in nses: + msg = f'DS record {record.fqdn} does not have coresponding NS record and Cloudflare requires it' + fallback = 'omitting the record' + self.supports_warn_or_except(msg, fallback) + desired.remove_record(record) + + return desired + def _contents_for_multiple(self, record): for value in record.values: yield {'content': value} diff --git a/tests/config/unit.tests.yaml b/tests/config/unit.tests.yaml index 9ed6aae..25c3131 100644 --- a/tests/config/unit.tests.yaml +++ b/tests/config/unit.tests.yaml @@ -73,13 +73,16 @@ dname: type: DNAME value: unit.tests. ds: - ttl: 300 - type: DS - value: - algorithm: 13 - digest: "B5BB9D8014A0F9B1D61E21E796D78DCCDF1352F23CD32812F4850B878AE4944C" - digest_type: 2 - key_tag: 1 + - ttl: 300 + type: DS + value: + algorithm: 13 + digest: "B5BB9D8014A0F9B1D61E21E796D78DCCDF1352F23CD32812F4850B878AE4944C" + digest_type: 2 + key_tag: 1 + - ttl: 301 + type: NS + value: ns1.unit.tests. excluded: octodns: excluded: diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 8d31d7b..de7810a 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -244,8 +244,8 @@ def test_populate(self): changes = self.expected.changes(zone, provider) - # delete a urlfwd, create 3 urlfwd, and create 1 spf - self.assertEqual(9, len(changes)) + # delete a urlfwd, create 3 urlfwd, and create 1 spf, delete 1 NS + self.assertEqual(10, len(changes)) # re-populating the same zone/records comes out of cache, no calls again = Zone('unit.tests.', []) @@ -264,12 +264,12 @@ def test_apply(self): {'result': {'id': 42}}, # zone create ] + [ None - ] * 31 # individual record creates + ] * 32 # individual record creates # non-existent zone, create everything plan = provider.plan(self.expected) - self.assertEqual(19, len(plan.changes)) - self.assertEqual(19, provider.apply(plan)) + self.assertEqual(20, len(plan.changes)) + self.assertEqual(20, provider.apply(plan)) self.assertFalse(plan.exists) provider._request.assert_has_calls( @@ -333,7 +333,7 @@ def test_apply(self): True, ) # expected number of total calls - self.assertEqual(33, provider._request.call_count) + self.assertEqual(34, provider._request.call_count) provider._request.reset_mock() @@ -575,12 +575,12 @@ def test_apply(self): {'result': {'id': 42}}, # zone create ] + [ None - ] * 31 # individual record creates + ] * 32 # individual record creates # non-existent zone, create everything plan = provider.plan(self.expected) - self.assertEqual(19, len(plan.changes)) - self.assertEqual(19, provider.apply(plan)) + self.assertEqual(20, len(plan.changes)) + self.assertEqual(20, provider.apply(plan)) self.assertFalse(plan.exists) provider._request.assert_has_calls( @@ -648,7 +648,7 @@ def test_apply(self): True, ) # expected number of total calls - self.assertEqual(33, provider._request.call_count) + self.assertEqual(34, provider._request.call_count) def test_update_add_swap(self): provider = CloudflareProvider('test', 'email', 'token', retry_period=0) @@ -1290,10 +1290,6 @@ def test_txt(self): # missing content, equivilent to empty from CF data = provider._data_for_TXT('TXT', [{'ttl': 42}]) - from pprint import pprint - - pprint(data) - self.assertEqual({'ttl': 42, 'type': 'TXT', 'values': ['']}, data) def test_alias(self): @@ -2723,3 +2719,96 @@ def test_update_comment(self): extra_changes[0].new._octodns['cloudflare']['comment'], 'a new comment', ) + + def test_change_keyer(self): + provider = CloudflareProvider('test', 'email', 'token') + + zone = Zone('unit.tests.', []) + + ds = Record.new( + zone, + 'subber', + { + 'ttl': 300, + 'type': 'DS', + 'value': { + 'key_tag': 23, + 'algorithm': 2, + 'digest_type': 3, + 'digest': 'abcdefg', + }, + }, + ) + self.assertEqual( + (1, 'subber', 'ZDS'), provider._change_keyer(Create(ds)) + ) + self.assertEqual( + (0, 'subber', 'DS'), provider._change_keyer(Delete(ds)) + ) + + ns = Record.new( + zone, + 'subber', + {'ttl': 300, 'type': 'NS', 'value': 'ns1.unit.tests.'}, + ) + self.assertEqual( + (1, 'subber', 'NS'), provider._change_keyer(Create(ns)) + ) + self.assertEqual( + (0, 'subber', 'NS'), provider._change_keyer(Delete(ns)) + ) + + def test_process_desired_zone(self): + provider = CloudflareProvider( + 'test', 'email', 'token', strict_supports=False + ) + + zone = Zone('unit.tests.', []) + + ds = Record.new( + zone, + 'subber', + { + 'ttl': 300, + 'type': 'DS', + 'value': { + 'key_tag': 23, + 'algorithm': 2, + 'digest_type': 3, + 'digest': 'abcdefg', + }, + }, + ) + ns = Record.new( + zone, + 'subber', + {'ttl': 300, 'type': 'NS', 'value': 'ns1.unit.tests.'}, + ) + + # has both + desired = zone.copy() + desired.add_record(ds) + desired.add_record(ns) + self.assertEqual( + {ds, ns}, provider._process_desired_zone(desired).records + ) + + # just NS + desired = zone.copy() + desired.add_record(ns) + self.assertEqual({ns}, provider._process_desired_zone(desired).records) + + # just DS, will be removed + desired = zone.copy() + desired.add_record(ds) + self.assertEqual(set(), provider._process_desired_zone(desired).records) + + # when in strict mode will error + provider.strict_supports = True + desired = zone.copy() + desired.add_record(ds) + with self.assertRaises(SupportsException) as ctx: + provider._process_desired_zone(desired) + msg = str(ctx.exception) + self.assertTrue('subber.unit.tests.' in msg) + self.assertTrue('coresponding NS record' in msg)