Rework root NS logic to ignore when unconfigured, more testing

octodns/provider/base.py

@@ -96,22 +96,29 @@ class BaseProvider(BaseSource):
                 desired.add_record(record, replace=True)
 
         record = desired.root_ns
-        if record and not self.SUPPORTS_ROOT_NS:
-            # ignore, we can't manage root NS records
-            msg = \
-                f'root NS record not supported for {record.fqdn}'
-            fallback = 'ignoring it'
-            self.supports_warn_or_except(msg, fallback)
-            desired.remove_record(record)
+        if self.SUPPORTS_ROOT_NS:
+            if not record:
+                self.log.warning('%s: root NS record supported by provider, '
+                                 'but no record is configured for %s', self.id,
+                                 desired.name)
+        else:
+            if record:
+                # ignore, we can't manage root NS records
+                msg = \
+                    f'root NS record not supported for {record.fqdn}'
+                fallback = 'ignoring it'
+                self.supports_warn_or_except(msg, fallback)
+                desired.remove_record(record)
 
         return desired
 
-    def _process_existing_zone(self, existing):
+    def _process_existing_zone(self, existing, desired):
         '''
         An opportunity for providers to modify the existing zone records before
         planning. `existing` is a "shallow" copy, see `Zone.copy` for more
         information
 
+        - `desired` must not be modified in anyway, it is only for reference
         - Must call `super` at an appropriate point for their work, generally
           that means as the final step of the method, returning the result of
           the `super` call.
@@ -125,14 +132,18 @@ class BaseProvider(BaseSource):
           provider configuration.
         '''
 
-        record = existing.root_ns
-        if record and not self.SUPPORTS_ROOT_NS:
-            # ignore, we can't manage root NS records
+        existing_root_ns = existing.root_ns
+        if existing_root_ns and (not desired.root_ns or not
+                                 self.SUPPORTS_ROOT_NS):
+            # we have an existing root NS record and either the provider
+            # doesn't support managing them or our desired state doesn't
+            # include one, either way we'll exclude the existing one from
+            # consideration
             msg = \
-                f'root NS record not supported for {record.fqdn}'
+                f'root NS record not supported for {existing_root_ns.fqdn}'
             fallback = 'ignoring it'
             self.supports_warn_or_except(msg, fallback)
-            existing.remove_record(record)
+            existing.remove_record(existing_root_ns)
 
         return existing
 
@@ -174,7 +185,7 @@ class BaseProvider(BaseSource):
 
         desired = self._process_desired_zone(desired)
 
-        existing = self._process_existing_zone(existing)
+        existing = self._process_existing_zone(existing, desired)
 
         for processor in processors:
             existing = processor.process_target_zone(existing, target=self)

tests/test_octodns_provider_base.py

@@ -406,11 +406,11 @@ class TestBaseProvider(TestCase):
         })
         zone1.add_record(record1)
 
-        zone2 = provider._process_existing_zone(zone1.copy())
+        zone2 = provider._process_existing_zone(zone1.copy(), zone1)
         self.assertEqual(0, len(zone2.records))
 
         provider.SUPPORTS_ROOT_NS = True
-        zone2 = provider._process_existing_zone(zone1.copy())
+        zone2 = provider._process_existing_zone(zone1.copy(), zone1)
         self.assertEqual(1, len(zone2.records))
         self.assertEqual(record1, list(zone2.records)[0])
 
@@ -701,13 +701,18 @@ class TestBaseProviderSupportsRootNs(TestCase):
 
         # matching root NS in the desired
         plan = provider.plan(self.has_root)
-        # there's an existing root record that matches the desired. we don't
-        # support them, so it doesn't matter whether it matches or not.
-        # _process_desired_zone will have removed the desired one since it
-        # can't be managed. this will not result in a plan as
-        # _process_existing_zone will have done so as well.
+
+        # no root ns upport on the target provider so doesn't matter, still no
+        # changes
         self.assertFalse(plan)
 
+        # plan again with strict_supports enabled, we should get an exception
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.has_root)
+        self.assertEqual('test: root NS record not supported for unit.tests.',
+                         str(ctx.exception))
+
     def test_supports_root_ns_false_different(self):
         # provider has a non-matching existing record
         provider = self.Provider(self.different_root)
@@ -715,6 +720,7 @@ class TestBaseProviderSupportsRootNs(TestCase):
 
         # different root is in the desired
         plan = provider.plan(self.has_root)
+
         # the mis-match doesn't matter since we can't manage the records
         # anyway, they will have been removed from the desired and existing.
         self.assertFalse(plan)
@@ -726,6 +732,7 @@ class TestBaseProviderSupportsRootNs(TestCase):
 
         # desired doesn't have a root
         plan = provider.plan(self.no_root)
+
         # the mis-match doesn't matter since we can't manage the records
         # anyway, they will have been removed from the desired and existing.
         self.assertFalse(plan)
@@ -734,10 +741,11 @@ class TestBaseProviderSupportsRootNs(TestCase):
         # provider has no existing records (create)
         provider = self.Provider()
         provider.SUPPORTS_ROOT_NS = False
+
         # case where we have a root NS in the desired
         plan = provider.plan(self.has_root)
-        # there's no existing root record since we're creating the zone so
-        # we'll get a plan that creates the other 2 records only
+
+        # no support for root NS so we only create the other two records
         self.assertTrue(plan)
         self.assertEqual(2, len(plan.changes))
 
@@ -747,45 +755,49 @@ class TestBaseProviderSupportsRootNs(TestCase):
         # provider has a matching existing root record
         provider = self.Provider(self.has_root)
         provider.SUPPORTS_ROOT_NS = True
-        # case where we have a root NS in the desired
+
+        # same root NS in the desired
         plan = provider.plan(self.has_root)
-        # there's an existing root record that matches the desired state so no
-        # changes are needed
+
+        # root NS is supported in the target provider, but they match so no
+        # change
         self.assertFalse(plan)
 
     def test_supports_root_ns_true_different(self):
         # provider has a non-matching existing record
         provider = self.Provider(self.different_root)
         provider.SUPPORTS_ROOT_NS = True
-        # case where we have a root NS in the desired
+
+        # non-matching root NS in the desired
         plan = provider.plan(self.has_root)
-        # there's an existing root record that doesn't match the desired state.
-        # we'll get a plan that modifies it.
+
+        # root NS mismatch in a target provider that supports it, we'll see the
+        # change
         self.assertTrue(plan)
         change = plan.changes[0]
         self.assertEqual(self.other_root_ns_record, change.existing)
         self.assertEqual(self.root_ns_record, change.new)
 
-    # TODO: rework
-    def _test_supports_root_ns_true_missing(self):
+    def test_supports_root_ns_true_missing(self):
         # provider has a matching existing root record
         provider = self.Provider(self.has_root)
         provider.SUPPORTS_ROOT_NS = True
-        # case where we don't have a configured/desired root NS record. this is
-        # dangerous so we expect an exception to be thrown to prevent trying to
-        # delete the critical record
-        with self.assertRaises(SupportsException) as ctx:
-            provider.plan(self.no_root)
-        self.assertEqual('test: provider supports root NS record management, '
-                         'but no record configured in unit.tests.; aborting',
-                         str(ctx.exception))
+
+        # there's no root record in the desired
+        plan = provider.plan(self.no_root)
+
+        # the existing root NS in the target is left alone/as is since we
+        # aren't configured with one to manage
+        self.assertFalse(plan)
 
     def test_supports_root_ns_true_create_zone(self):
         # provider has no existing records (create)
         provider = self.Provider()
         provider.SUPPORTS_ROOT_NS = True
+
         # case where we have a root NS in the desired
         plan = provider.plan(self.has_root)
+
         # there's no existing root record since we're creating the zone so
         # we'll get a plan that creates everything, including it
         self.assertTrue(plan)
@@ -795,17 +807,12 @@ class TestBaseProviderSupportsRootNs(TestCase):
         self.assertFalse(change.existing)
         self.assertEqual(self.root_ns_record, change.new)
 
-    # TODO: rework
-    def _test_supports_root_ns_true_create_zone_missing(self):
+    def test_supports_root_ns_true_create_zone_missing(self):
         # provider has no existing records (create)
         provider = self.Provider()
         provider.SUPPORTS_ROOT_NS = True
-        # case where we don't have a configured/desired root NS record. this is
-        # dangerous so we expect an exception to be thrown to prevent trying to
-        # create a zone without the critical record being managed (if they
-        # didn't get it now they'd get it on the next sync)
-        with self.assertRaises(SupportsException) as ctx:
-            provider.plan(self.no_root)
-        self.assertEqual('test: provider supports root NS record management, '
-                         'but no record configured in unit.tests.; aborting',
-                         str(ctx.exception))
+
+        # we don't have a root NS configured so we'll ignore them and just
+        # manage the other records
+        plan = provider.plan(self.no_root)
+        self.assertEqual(2, len(plan.changes))