Merge pull request #103 from octodns/create-ds-after-ns

Create DS records after NS for the same node

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

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}

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:

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)