From 5215930109f6c9cc6915099e6fd2a34b6c9dae6c Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 10 Feb 2022 14:51:01 -0800 Subject: [PATCH] Always require a root NS in desired, sketch out SUPPORTS_ROOT_NS tests --- octodns/manager.py | 4 +- octodns/provider/base.py | 25 +++- tests/config/dynamic.tests.yaml | 5 + tests/config/empty.yaml | 5 + tests/config/split/dynamic.tests.tst/ns.yaml | 6 + tests/config/subzone.unit.tests.yaml | 5 + tests/test_octodns_manager.py | 19 +-- tests/test_octodns_provider_base.py | 145 +++++++++++++++++++ tests/test_octodns_provider_yaml.py | 24 +-- 9 files changed, 208 insertions(+), 30 deletions(-) create mode 100644 tests/config/split/dynamic.tests.tst/ns.yaml diff --git a/octodns/manager.py b/octodns/manager.py index c8a4263..d9e4093 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -12,7 +12,6 @@ from sys import stdout import logging from .provider.base import BaseProvider -from .provider.plan import Plan from .provider.yaml import SplitYamlProvider, YamlProvider from .record import Record from .yaml import safe_load @@ -520,8 +519,7 @@ class Manager(object): source.populate(zone, lenient=lenient) plan = target.plan(zone) - if plan is None: - plan = Plan(zone, zone, [], False) + # We require at least root NS so there'll always be a plan target.apply(plan) def validate_configs(self): diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 62d6c68..88f40b1 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -49,6 +49,7 @@ class BaseProvider(BaseSource): provider configuration. ''' + have_root_ns = False for record in desired.records: if record._type not in self.SUPPORTS: msg = f'{record._type} records not supported for {record.fqdn}' @@ -94,14 +95,22 @@ class BaseProvider(BaseSource): record = record.copy() record.values = [record.value] desired.add_record(record, replace=True) - elif record._type == 'NS' and record.name == '' 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) + elif record._type == 'NS' and record.name == '': + if self.SUPPORTS_ROOT_NS: + # record that we saw a root NS record + have_root_ns = True + else: + # 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 and not have_root_ns: + raise SupportsException(f'{self.id}: provider supports root NS ' + 'record management, but no record ' + f'configured in {desired.name}; aborting') return desired diff --git a/tests/config/dynamic.tests.yaml b/tests/config/dynamic.tests.yaml index d25f63a..35b3117 100644 --- a/tests/config/dynamic.tests.yaml +++ b/tests/config/dynamic.tests.yaml @@ -1,4 +1,9 @@ --- +? '' +: - type: NS + values: + - 8.8.8.8. + - 9.9.9.9. a: dynamic: pools: diff --git a/tests/config/empty.yaml b/tests/config/empty.yaml index ed97d53..f7b4285 100644 --- a/tests/config/empty.yaml +++ b/tests/config/empty.yaml @@ -1 +1,6 @@ --- +? '' +: - type: NS + values: + - 4.4.4.4. + - 5.5.5.5. diff --git a/tests/config/split/dynamic.tests.tst/ns.yaml b/tests/config/split/dynamic.tests.tst/ns.yaml new file mode 100644 index 0000000..48dd39a --- /dev/null +++ b/tests/config/split/dynamic.tests.tst/ns.yaml @@ -0,0 +1,6 @@ +--- +? '' +: - type: NS + values: + - 8.8.8.8. + - 9.9.9.9. diff --git a/tests/config/subzone.unit.tests.yaml b/tests/config/subzone.unit.tests.yaml index 932ac28..c9516d1 100644 --- a/tests/config/subzone.unit.tests.yaml +++ b/tests/config/subzone.unit.tests.yaml @@ -1,4 +1,9 @@ --- +? '' +: - type: NS + values: + - 6.6.6.6. + - 7.7.7.7. 2: type: A value: 2.4.4.4 diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 4900a04..4d537cd 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -113,14 +113,14 @@ class TestManager(TestCase): tc = Manager(get_config_filename('always-dry-run.yaml')) \ .sync(dry_run=False) # only the stuff from subzone, unit.tests. is always-dry-run - self.assertEqual(3, tc) + self.assertEqual(4, tc) def test_simple(self): with TemporaryDirectory() as tmpdir: environ['YAML_TMP_DIR'] = tmpdir.dirname tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False) - self.assertEqual(27, tc) + self.assertEqual(30, tc) # try with just one of the zones tc = Manager(get_config_filename('simple.yaml')) \ @@ -130,28 +130,28 @@ class TestManager(TestCase): # the subzone, with 2 targets tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, eligible_zones=['subzone.unit.tests.']) - self.assertEqual(6, tc) + self.assertEqual(8, tc) - # and finally the empty zone + # and finally the empty zone (only root NS) tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, eligible_zones=['empty.']) - self.assertEqual(0, tc) + self.assertEqual(1, tc) # Again with force tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, force=True) - self.assertEqual(27, tc) + self.assertEqual(30, tc) # Again with max_workers = 1 tc = Manager(get_config_filename('simple.yaml'), max_workers=1) \ .sync(dry_run=False, force=True) - self.assertEqual(27, tc) + self.assertEqual(30, tc) # Include meta tc = Manager(get_config_filename('simple.yaml'), max_workers=1, include_meta=True) \ .sync(dry_run=False, force=True) - self.assertEqual(31, tc) + self.assertEqual(34, tc) def test_eligible_sources(self): with TemporaryDirectory() as tmpdir: @@ -301,7 +301,8 @@ class TestManager(TestCase): with open(join(tmpdir.dirname, 'empty.yaml')) as fh: data = safe_load(fh, False) - self.assertFalse(data) + # just to root NS + self.assertEqual(1, len(data)) def test_dump_split(self): with TemporaryDirectory() as tmpdir: diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 6ba9b72..fbe4cf7 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -606,3 +606,148 @@ class TestBaseProvider(TestCase): strict.supports_warn_or_except('Hello World!', 'Will not see') self.assertEqual('minimal: Hello World!', str(ctx.exception)) strict.log.warning.assert_not_called() + + +class TestBaseProviderSupportsRootNs(TestCase): + + class Provider(BaseProvider): + log = getLogger('Provider') + + SUPPORTS = set(('A', 'NS')) + SUPPORTS_GEO = False + SUPPORTS_ROOT_NS = False + + strict_supports = False + + def __init__(self, existing=None): + super().__init__('test') + self.existing = existing + + def populate(self, zone, target=False, lenient=False): + if self.existing: + for record in self.existing.records: + zone.add_record(record) + return True + return False + + zone = Zone('unit.tests.', []) + a_record = Record.new(zone, 'ptr', { + 'type': 'A', + 'ttl': 3600, + 'values': ['1.2.3.4', '2.3.4.5'], + }) + ns_record = Record.new(zone, 'sub', { + 'type': 'NS', + 'ttl': 3600, + 'values': ['ns2.foo.com.', 'ns2.bar.com.'], + }) + no_root = zone.copy() + no_root.add_record(a_record) + no_root.add_record(ns_record) + + root_ns_record = Record.new(zone, '', { + 'type': 'NS', + 'ttl': 3600, + 'values': ['ns1.foo.com.', 'ns1.bar.com.'], + }) + has_root = no_root.copy() + has_root.add_record(root_ns_record) + + other_root_ns_record = Record.new(zone, '', { + 'type': 'NS', + 'ttl': 3600, + 'values': ['ns4.foo.com.', 'ns4.bar.com.'], + }) + different_root = no_root.copy() + different_root.add_record(other_root_ns_record) + + # False + + def test_supports_root_ns_false_matches(self): + # provider has a matching existing root record + provider = self.Provider(self.has_root) + provider.SUPPORTS_ROOT_NS = False + + # 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. + self.assertFalse(plan) + + def test_supports_root_ns_false_different(self): + # provider has a non-matching existing record + provider = self.Provider(self.different_root) + provider.SUPPORTS_ROOT_NS = False + + # 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) + + def test_supports_root_ns_false_missing(self): + # provider has an existing record + provider = self.Provider(self.has_root) + provider.SUPPORTS_ROOT_NS = False + + # 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) + + # True + + def test_supports_root_ns_true_matches(self): + # 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 + plan = provider.plan(self.has_root) + # there's an existing root record that matches the desired state so no + # changes are needed + 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 + 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. + self.assertTrue(plan) + change = plan.changes[0] + self.assertEqual(self.other_root_ns_record, change.existing) + self.assertEqual(self.root_ns_record, change.new) + + 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) + self.assertEqual(3, len(plan.changes)) + change = [c for c in plan.changes + if c.new.name == '' and c.new._type == 'NS'][0] + self.assertFalse(change.existing) + self.assertEqual(self.root_ns_record, change.new) + + 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)) diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py index 3dbeacb..8fda388 100644 --- a/tests/test_octodns_provider_yaml.py +++ b/tests/test_octodns_provider_yaml.py @@ -37,7 +37,7 @@ class TestYamlProvider(TestCase): self.assertEqual(23, len(zone.records)) source.populate(dynamic_zone) - self.assertEqual(6, len(dynamic_zone.records)) + self.assertEqual(7, len(dynamic_zone.records)) # Assumption here is that a clean round-trip means that everything # worked as expected, data that went in came back out and could be @@ -67,11 +67,11 @@ class TestYamlProvider(TestCase): # Dynamic plan plan = target.plan(dynamic_zone) - self.assertEqual(6, len([c for c in plan.changes + self.assertEqual(7, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isfile(dynamic_yaml_file)) # Apply it - self.assertEqual(6, target.apply(plan)) + self.assertEqual(7, target.apply(plan)) self.assertTrue(isfile(dynamic_yaml_file)) # There should be no changes after the round trip @@ -152,6 +152,10 @@ class TestYamlProvider(TestCase): self.assertTrue('value' in dyna) # self.assertTrue('dynamic' in dyna) + dyna = data.pop('') + self.assertTrue('values' in dyna) + # self.assertTrue('dynamic' in dyna) + # make sure nothing is left self.assertEqual([], list(data.keys())) @@ -160,9 +164,9 @@ class TestYamlProvider(TestCase): zone = Zone('empty.', []) - # without it we see everything + # without it we see everything (root NS record) source.populate(zone) - self.assertEqual(0, len(zone.records)) + self.assertEqual(1, len(zone.records)) def test_unsorted(self): source = YamlProvider('test', join(dirname(__file__), 'config')) @@ -251,7 +255,7 @@ class TestSplitYamlProvider(TestCase): self.assertEqual(20, len(zone.records)) source.populate(dynamic_zone) - self.assertEqual(5, len(dynamic_zone.records)) + self.assertEqual(6, len(dynamic_zone.records)) with TemporaryDirectory() as td: # Add some subdirs to make sure that it can create them @@ -272,11 +276,11 @@ class TestSplitYamlProvider(TestCase): # Dynamic plan plan = target.plan(dynamic_zone) - self.assertEqual(5, len([c for c in plan.changes + self.assertEqual(6, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isdir(dynamic_zone_dir)) # Apply it - self.assertEqual(5, target.apply(plan)) + self.assertEqual(6, target.apply(plan)) self.assertTrue(isdir(dynamic_zone_dir)) # There should be no changes after the round trip @@ -401,7 +405,7 @@ class TestOverridingYamlProvider(TestCase): # Load the base, should see the 5 records base.populate(zone) got = {r.name: r for r in zone.records} - self.assertEqual(6, len(got)) + self.assertEqual(7, len(got)) # We get the "dynamic" A from the base config self.assertTrue('dynamic' in got['a'].data) # No added @@ -410,7 +414,7 @@ class TestOverridingYamlProvider(TestCase): # Load the overrides, should replace one and add 1 override.populate(zone) got = {r.name: r for r in zone.records} - self.assertEqual(7, len(got)) + self.assertEqual(8, len(got)) # 'a' was replaced with a generic record self.assertEqual({ 'ttl': 3600,