From 02ee7518fa3e73203bb045afabaa9a336255e9a1 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 17 Feb 2022 08:50:42 -0800 Subject: [PATCH] Rework root NS logic to ignore when unconfigured, more testing --- octodns/provider/base.py | 39 +++++++++----- tests/test_octodns_provider_base.py | 79 ++++++++++++++++------------- 2 files changed, 68 insertions(+), 50 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 3bc8094..0af75ef 100644 --- a/octodns/provider/base.py +++ b/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) diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 24dee84..2498dca 100644 --- a/tests/test_octodns_provider_base.py +++ b/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))