From 33a10eada434070b79712d398a0e7a4dda8a72b4 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 8 Feb 2022 10:26:49 -0800 Subject: [PATCH 01/21] Base support for managing root NS records * Zone object no longer treats them special, some tests needed adjusting b/c of this, some provider's tests may also need adjusting, though they should not plan changes since they won't (yet) have SUPPORTS_ROOT_NS * _process_desired_zone filters and warns when not supported * YamlProvider supports them * TinyDnsBaseSource supports them --- octodns/provider/base.py | 8 ++++++++ octodns/provider/yaml.py | 1 + octodns/source/base.py | 1 + octodns/zone.py | 10 ++-------- tests/test_octodns_manager.py | 14 +++++++------- tests/test_octodns_provider_base.py | 27 +++++++++++++++++++++------ tests/test_octodns_provider_yaml.py | 12 ++++++------ tests/test_octodns_source_tinydns.py | 7 ++++++- tests/zones/tinydns/example.com | 4 ++-- 9 files changed, 54 insertions(+), 30 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 250ec26..951daf3 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -94,6 +94,14 @@ 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) return desired diff --git a/octodns/provider/yaml.py b/octodns/provider/yaml.py index 57fae3b..4c146bb 100644 --- a/octodns/provider/yaml.py +++ b/octodns/provider/yaml.py @@ -106,6 +106,7 @@ class YamlProvider(BaseProvider): SUPPORTS_DYNAMIC = True SUPPORTS_POOL_VALUE_STATUS = True SUPPORTS_MULTIVALUE_PTR = True + SUPPORTS_ROOT_NS = True SUPPORTS = set(('A', 'AAAA', 'ALIAS', 'CAA', 'CNAME', 'DNAME', 'LOC', 'MX', 'NAPTR', 'NS', 'PTR', 'SSHFP', 'SPF', 'SRV', 'TXT', 'URLFWD')) diff --git a/octodns/source/base.py b/octodns/source/base.py index 569a1b4..dfc1613 100644 --- a/octodns/source/base.py +++ b/octodns/source/base.py @@ -10,6 +10,7 @@ class BaseSource(object): SUPPORTS_MULTIVALUE_PTR = False SUPPORTS_POOL_VALUE_STATUS = False + SUPPORTS_ROOT_NS = False def __init__(self, id): self.id = id diff --git a/octodns/zone.py b/octodns/zone.py index a6cdab3..c1929cf 100644 --- a/octodns/zone.py +++ b/octodns/zone.py @@ -24,12 +24,6 @@ class InvalidNodeException(Exception): pass -def _is_eligible(record): - # Should this record be considered when computing changes - # We ignore all top-level NS records - return record._type != 'NS' or record.name != '' - - class Zone(object): log = getLogger('Zone') @@ -118,7 +112,7 @@ class Zone(object): changes = [] # Find diffs & removes - for record in filter(_is_eligible, self.records): + for record in self.records: if record.ignored: continue elif len(record.included) > 0 and \ @@ -168,7 +162,7 @@ class Zone(object): # Find additions, things that are in desired, but missing in ourselves. # This uses set math and our special __hash__ and __cmp__ functions as # well - for record in filter(_is_eligible, desired.records - self.records): + for record in desired.records - self.records: if record.ignored: continue elif len(record.included) > 0 and \ diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index d63e84d..4900a04 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -120,12 +120,12 @@ class TestManager(TestCase): environ['YAML_TMP_DIR'] = tmpdir.dirname tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False) - self.assertEqual(26, tc) + self.assertEqual(27, tc) # try with just one of the zones tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, eligible_zones=['unit.tests.']) - self.assertEqual(20, tc) + self.assertEqual(21, tc) # the subzone, with 2 targets tc = Manager(get_config_filename('simple.yaml')) \ @@ -140,18 +140,18 @@ class TestManager(TestCase): # Again with force tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, force=True) - self.assertEqual(26, tc) + self.assertEqual(27, tc) # Again with max_workers = 1 tc = Manager(get_config_filename('simple.yaml'), max_workers=1) \ .sync(dry_run=False, force=True) - self.assertEqual(26, tc) + self.assertEqual(27, tc) # Include meta tc = Manager(get_config_filename('simple.yaml'), max_workers=1, include_meta=True) \ .sync(dry_run=False, force=True) - self.assertEqual(30, tc) + self.assertEqual(31, tc) def test_eligible_sources(self): with TemporaryDirectory() as tmpdir: @@ -217,13 +217,13 @@ class TestManager(TestCase): fh.write('---\n{}') changes = manager.compare(['in'], ['dump'], 'unit.tests.') - self.assertEqual(20, len(changes)) + self.assertEqual(21, len(changes)) # Compound sources with varying support changes = manager.compare(['in', 'nosshfp'], ['dump'], 'unit.tests.') - self.assertEqual(19, len(changes)) + self.assertEqual(20, len(changes)) with self.assertRaises(ManagerException) as ctx: manager.compare(['nope'], ['dump'], 'unit.tests.') diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index bd6c361..6218076 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -20,7 +20,7 @@ from octodns.zone import Zone class HelperProvider(BaseProvider): log = getLogger('HelperProvider') - SUPPORTS = set(('A', 'PTR')) + SUPPORTS = set(('A', 'NS', 'PTR')) SUPPORTS_MULTIVALUE_PTR = False SUPPORTS_DYNAMIC = False @@ -36,7 +36,7 @@ class HelperProvider(BaseProvider): self.delete_pcent_threshold = Plan.MAX_SAFE_DELETE_PCENT def populate(self, zone, target=False, lenient=False): - pass + return True def _include_change(self, change): return not self.include_change_callback or \ @@ -229,6 +229,25 @@ class TestBaseProvider(TestCase): self.assertEqual(zone.name, another.existing.name) self.assertEqual(1, len(another.existing.records)) + def test_plan_with_root_ns(self): + zone = Zone('unit.tests.', []) + record = Record.new(zone, '', { + 'ttl': 30, + 'type': 'NS', + 'value': '1.2.3.4.', + }) + zone.add_record(record) + + # No root NS support, no change, thus no plan + provider = HelperProvider() + self.assertEqual(None, provider.plan(zone)) + + # set Support root NS records, see the record + provider.SUPPORTS_ROOT_NS = True + plan = provider.plan(zone) + self.assertTrue(plan) + self.assertEqual(1, len(plan.changes)) + def test_apply(self): ignored = Zone('unit.tests.', []) @@ -278,10 +297,6 @@ class TestBaseProvider(TestCase): provider.SUPPORTS_MULTIVALUE_PTR = True zone2 = provider._process_desired_zone(zone1.copy()) record2 = list(zone2.records)[0] - from pprint import pprint - pprint([ - record1, record2 - ]) self.assertEqual(len(record2.values), 2) # SUPPORTS_DYNAMIC diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py index bffe4c5..3dbeacb 100644 --- a/tests/test_octodns_provider_yaml.py +++ b/tests/test_octodns_provider_yaml.py @@ -57,12 +57,12 @@ class TestYamlProvider(TestCase): # We add everything plan = target.plan(zone) - self.assertEqual(20, len([c for c in plan.changes + self.assertEqual(21, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isfile(yaml_file)) # Now actually do it - self.assertEqual(20, target.apply(plan)) + self.assertEqual(21, target.apply(plan)) self.assertTrue(isfile(yaml_file)) # Dynamic plan @@ -86,7 +86,7 @@ class TestYamlProvider(TestCase): # A 2nd sync should still create everything plan = target.plan(zone) - self.assertEqual(20, len([c for c in plan.changes + self.assertEqual(21, len([c for c in plan.changes if isinstance(c, Create)])) with open(yaml_file) as fh: @@ -263,12 +263,12 @@ class TestSplitYamlProvider(TestCase): # We add everything plan = target.plan(zone) - self.assertEqual(17, len([c for c in plan.changes + self.assertEqual(18, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isdir(zone_dir)) # Now actually do it - self.assertEqual(17, target.apply(plan)) + self.assertEqual(18, target.apply(plan)) # Dynamic plan plan = target.plan(dynamic_zone) @@ -291,7 +291,7 @@ class TestSplitYamlProvider(TestCase): # A 2nd sync should still create everything plan = target.plan(zone) - self.assertEqual(17, len([c for c in plan.changes + self.assertEqual(18, len([c for c in plan.changes if isinstance(c, Create)])) yaml_file = join(zone_dir, '$unit.tests.yaml') diff --git a/tests/test_octodns_source_tinydns.py b/tests/test_octodns_source_tinydns.py index c17721e..bfc2cf1 100644 --- a/tests/test_octodns_source_tinydns.py +++ b/tests/test_octodns_source_tinydns.py @@ -29,10 +29,15 @@ class TestTinyDnsFileSource(TestCase): 'ttl': 30, 'values': ['10.2.3.4', '10.2.3.5'], }), + ('', { + 'type': 'NS', + 'ttl': 3600, + 'values': ['ns1.ns.com.', 'ns2.ns.com.'], + }), ('sub', { 'type': 'NS', 'ttl': 30, - 'values': ['ns1.ns.com.', 'ns2.ns.com.'], + 'values': ['ns3.ns.com.', 'ns4.ns.com.'], }), ('www', { 'type': 'A', diff --git a/tests/zones/tinydns/example.com b/tests/zones/tinydns/example.com index 32781ca..ae8b5bd 100755 --- a/tests/zones/tinydns/example.com +++ b/tests/zones/tinydns/example.com @@ -32,8 +32,8 @@ Ccname.other.foo:www.other.foo @smtp.example.com::smtp-2-host.example.com:40:1800 # NS -.sub.example.com::ns1.ns.com:30 -.sub.example.com::ns2.ns.com:30 +.sub.example.com::ns3.ns.com:30 +.sub.example.com::ns4.ns.com:30 # A, under sub +www.sub.example.com::1.2.3.4 From 3bcb6c8cecf9072a7b4a939d42382ba9c4853d85 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 8 Feb 2022 13:46:04 -0800 Subject: [PATCH 02/21] Add Provider._process_existing_zone to mirror _process_desired_zone --- octodns/provider/base.py | 33 ++++++++++++++++++++++++ tests/test_octodns_provider_base.py | 39 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 951daf3..62d6c68 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -105,6 +105,37 @@ class BaseProvider(BaseSource): return desired + def _process_existing_zone(self, existing): + ''' + An opportunity for providers to modify the existing zone records before + planning. `existing` is a "shallow" copy, see `Zone.copy` for more + information + + - 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. + - May modify `existing` directly. + - Must not modify records directly, `record.copy` should be called, + the results of which can be modified, and then `Zone.add_record` may + be used with `replace=True`. + - May call `Zone.remove_record` to remove records from `existing`. + - Must call supports_warn_or_except with information about any changes + that are made to have them logged or throw errors depending on the + provider configuration. + ''' + + for record in existing.records: + if 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) + existing.remove_record(record) + + return existing + def _include_change(self, change): ''' An opportunity for providers to filter out false positives due to @@ -143,6 +174,8 @@ class BaseProvider(BaseSource): self.log.warning('Provider %s used in target mode did not return ' 'exists', self.id) + existing = self._process_existing_zone(existing) + 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 6218076..6ba9b72 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -344,6 +344,45 @@ class TestBaseProvider(TestCase): 'obey' ) + # SUPPORTS_ROOT_NS + provider.SUPPORTS_ROOT_NS = False + zone1 = Zone('unit.tests.', []) + record1 = Record.new(zone1, '', { + 'type': 'NS', + 'ttl': 3600, + 'values': ['foo.com.', 'bar.com.'], + }) + zone1.add_record(record1) + + zone2 = provider._process_desired_zone(zone1.copy()) + self.assertEqual(0, len(zone2.records)) + + provider.SUPPORTS_ROOT_NS = True + zone2 = provider._process_desired_zone(zone1.copy()) + self.assertEqual(1, len(zone2.records)) + self.assertEqual(record1, list(zone2.records)[0]) + + def test_process_existing_zone(self): + provider = HelperProvider('test') + + # SUPPORTS_ROOT_NS + provider.SUPPORTS_ROOT_NS = False + zone1 = Zone('unit.tests.', []) + record1 = Record.new(zone1, '', { + 'type': 'NS', + 'ttl': 3600, + 'values': ['foo.com.', 'bar.com.'], + }) + zone1.add_record(record1) + + zone2 = provider._process_existing_zone(zone1.copy()) + self.assertEqual(0, len(zone2.records)) + + provider.SUPPORTS_ROOT_NS = True + zone2 = provider._process_existing_zone(zone1.copy()) + self.assertEqual(1, len(zone2.records)) + self.assertEqual(record1, list(zone2.records)[0]) + def test_safe_none(self): # No changes is safe Plan(None, None, [], True).raise_if_unsafe() From f1fd63205e9eb890d69598b2a8e341f7966cf641 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 10 Feb 2022 11:58:48 -0800 Subject: [PATCH 03/21] Remove _process_existing_zone as it's not currently needed --- octodns/provider/base.py | 33 ----------------------------- tests/test_octodns_provider_base.py | 21 ------------------ 2 files changed, 54 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 62d6c68..951daf3 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -105,37 +105,6 @@ class BaseProvider(BaseSource): return desired - def _process_existing_zone(self, existing): - ''' - An opportunity for providers to modify the existing zone records before - planning. `existing` is a "shallow" copy, see `Zone.copy` for more - information - - - 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. - - May modify `existing` directly. - - Must not modify records directly, `record.copy` should be called, - the results of which can be modified, and then `Zone.add_record` may - be used with `replace=True`. - - May call `Zone.remove_record` to remove records from `existing`. - - Must call supports_warn_or_except with information about any changes - that are made to have them logged or throw errors depending on the - provider configuration. - ''' - - for record in existing.records: - if 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) - existing.remove_record(record) - - return existing - def _include_change(self, change): ''' An opportunity for providers to filter out false positives due to @@ -174,8 +143,6 @@ class BaseProvider(BaseSource): self.log.warning('Provider %s used in target mode did not return ' 'exists', self.id) - existing = self._process_existing_zone(existing) - 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 6ba9b72..c58d603 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -362,27 +362,6 @@ class TestBaseProvider(TestCase): self.assertEqual(1, len(zone2.records)) self.assertEqual(record1, list(zone2.records)[0]) - def test_process_existing_zone(self): - provider = HelperProvider('test') - - # SUPPORTS_ROOT_NS - provider.SUPPORTS_ROOT_NS = False - zone1 = Zone('unit.tests.', []) - record1 = Record.new(zone1, '', { - 'type': 'NS', - 'ttl': 3600, - 'values': ['foo.com.', 'bar.com.'], - }) - zone1.add_record(record1) - - zone2 = provider._process_existing_zone(zone1.copy()) - self.assertEqual(0, len(zone2.records)) - - provider.SUPPORTS_ROOT_NS = True - zone2 = provider._process_existing_zone(zone1.copy()) - self.assertEqual(1, len(zone2.records)) - self.assertEqual(record1, list(zone2.records)[0]) - def test_safe_none(self): # No changes is safe Plan(None, None, [], True).raise_if_unsafe() From 08f98a5e65cbf982679e99dfd89b13d390334add Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 10 Feb 2022 13:50:37 -0800 Subject: [PATCH 04/21] Revert "Remove _process_existing_zone as it's not currently needed" This reverts commit f1fd63205e9eb890d69598b2a8e341f7966cf641. --- octodns/provider/base.py | 33 +++++++++++++++++++++++++++++ tests/test_octodns_provider_base.py | 21 ++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 951daf3..62d6c68 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -105,6 +105,37 @@ class BaseProvider(BaseSource): return desired + def _process_existing_zone(self, existing): + ''' + An opportunity for providers to modify the existing zone records before + planning. `existing` is a "shallow" copy, see `Zone.copy` for more + information + + - 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. + - May modify `existing` directly. + - Must not modify records directly, `record.copy` should be called, + the results of which can be modified, and then `Zone.add_record` may + be used with `replace=True`. + - May call `Zone.remove_record` to remove records from `existing`. + - Must call supports_warn_or_except with information about any changes + that are made to have them logged or throw errors depending on the + provider configuration. + ''' + + for record in existing.records: + if 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) + existing.remove_record(record) + + return existing + def _include_change(self, change): ''' An opportunity for providers to filter out false positives due to @@ -143,6 +174,8 @@ class BaseProvider(BaseSource): self.log.warning('Provider %s used in target mode did not return ' 'exists', self.id) + existing = self._process_existing_zone(existing) + 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 c58d603..6ba9b72 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -362,6 +362,27 @@ class TestBaseProvider(TestCase): self.assertEqual(1, len(zone2.records)) self.assertEqual(record1, list(zone2.records)[0]) + def test_process_existing_zone(self): + provider = HelperProvider('test') + + # SUPPORTS_ROOT_NS + provider.SUPPORTS_ROOT_NS = False + zone1 = Zone('unit.tests.', []) + record1 = Record.new(zone1, '', { + 'type': 'NS', + 'ttl': 3600, + 'values': ['foo.com.', 'bar.com.'], + }) + zone1.add_record(record1) + + zone2 = provider._process_existing_zone(zone1.copy()) + self.assertEqual(0, len(zone2.records)) + + provider.SUPPORTS_ROOT_NS = True + zone2 = provider._process_existing_zone(zone1.copy()) + self.assertEqual(1, len(zone2.records)) + self.assertEqual(record1, list(zone2.records)[0]) + def test_safe_none(self): # No changes is safe Plan(None, None, [], True).raise_if_unsafe() From 5215930109f6c9cc6915099e6fd2a34b6c9dae6c Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 10 Feb 2022 14:51:01 -0800 Subject: [PATCH 05/21] 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, From 728ab2af898c325fee4de97b987fa63e1c99479a Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 12 Feb 2022 12:27:51 -0800 Subject: [PATCH 06/21] _process_desired_zone after populate, test/enforce order --- octodns/provider/base.py | 14 ++++++------- tests/test_octodns_provider_base.py | 31 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 88f40b1..51cda9e 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -168,13 +168,6 @@ class BaseProvider(BaseSource): def plan(self, desired, processors=[]): self.log.info('plan: desired=%s', desired.name) - # Make a (shallow) copy of the desired state so that everything from - # now on (in this target) can modify it as they see fit without - # worrying about impacting other targets. - desired = desired.copy() - - desired = self._process_desired_zone(desired) - existing = Zone(desired.name, desired.sub_zones) exists = self.populate(existing, target=True, lenient=True) if exists is None: @@ -183,6 +176,13 @@ class BaseProvider(BaseSource): self.log.warning('Provider %s used in target mode did not return ' 'exists', self.id) + # Make a (shallow) copy of the desired state so that everything from + # now on (in this target) can modify it as they see fit without + # worrying about impacting other targets. + desired = desired.copy() + + desired = self._process_desired_zone(desired) + existing = self._process_existing_zone(existing) for processor in processors: diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index fbe4cf7..128372e 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -277,6 +277,37 @@ class TestBaseProvider(TestCase): # We filtered out the only change self.assertFalse(plan) + def test_plan_order_of_operations(self): + + class MockProvider(BaseProvider): + log = getLogger('mock-provider') + SUPPORTS = set(('A',)) + SUPPORTS_GEO = False + + def __init__(self): + super().__init__('mock-provider') + self.calls = [] + + def populate(self, *args, **kwargs): + self.calls.append('populate') + + def _process_desired_zone(self, *args, **kwargs): + self.calls.append('_process_desired_zone') + return super()._process_desired_zone(*args, **kwargs) + + def _process_existing_zone(self, *args, **kwargs): + self.calls.append('_process_existing_zone') + return super()._process_existing_zone(*args, **kwargs) + + provider = MockProvider() + + zone = Zone('unit.tests.', []) + self.assertFalse(provider.plan(zone)) + # ensure the calls were made in the expected order, populate comes + # first, then desired, then existing + self.assertEqual(['populate', '_process_desired_zone', + '_process_existing_zone'], provider.calls) + def test_process_desired_zone(self): provider = HelperProvider('test') From 51d4b1ba7a130b22c949b1686530c973dd027f3c Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 14 Feb 2022 10:29:44 -0800 Subject: [PATCH 07/21] Few more root ns test cases --- tests/test_octodns_provider_base.py | 33 +++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 128372e..acd4592 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -730,6 +730,17 @@ class TestBaseProviderSupportsRootNs(TestCase): # anyway, they will have been removed from the desired and existing. self.assertFalse(plan) + def test_supports_root_ns_false_create_zone(self): + # 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 + self.assertTrue(plan) + self.assertEqual(2, len(plan.changes)) + # True def test_supports_root_ns_true_matches(self): @@ -755,6 +766,19 @@ class TestBaseProviderSupportsRootNs(TestCase): self.assertEqual(self.other_root_ns_record, 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)) + def test_supports_root_ns_true_create_zone(self): # provider has no existing records (create) provider = self.Provider() @@ -770,13 +794,14 @@ class TestBaseProviderSupportsRootNs(TestCase): 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) + 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 - # delete the critical record + # 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, ' From 3bb67806a0c8fff080a47c4ba94bb9ea3ec83f89 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 14 Feb 2022 10:40:52 -0800 Subject: [PATCH 08/21] CHANGELOG and README updates for root NS support --- CHANGELOG.md | 18 ++++++++++++++++++ README.md | 3 +-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5ab92f..ccf7c24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,23 @@ ## v0.9.16 - 2022-??-?? - ??? +#### Noteworthy changes + +* Foundational support for root NS record management. + * YamlProvider has it enabled and in general everyone should add root NS + records that match what is in their provider(s) as of this release if they + an're already there. + * Note that if you created you config files with `octodns-dump` the records + are likely already there and match what was configured at the time of the + dump. + * Other providers will add root NS support over time once they have had the + chance to investigate the functionality and implement management if + possible with whatever accomidations are required. + * Once you are using this release and an provider version that supports root + NS management you will get errors if you do not have root NS records + included by your source(s). + * Watch your providers README.md and CHANGELOG.md for support and more + information. + #### Stuff * _AggregateTarget has more complete handling of SUPPORTS* functionality, diff --git a/README.md b/README.md index 7687391..43e1db1 100644 --- a/README.md +++ b/README.md @@ -223,7 +223,7 @@ The table below lists the providers octoDNS supports. They are maintained in the | [Selectel](https://selectel.ru/en/services/additional/dns/) | [octodns_selectel](https://github.com/octodns/octodns-selectel/) | | | [TransIP](https://www.transip.eu/knowledgebase/entry/155-dns-and-nameservers/) | [octodns_transip](https://github.com/octodns/octodns-transip/) | | | [UltraDNS](https://www.home.neustar/dns-services) | [octodns_ultra](https://github.com/octodns/octodns-ultra/) | | -| [YamlProvider](/octodns/provider/yaml.py) | built-in | config | +| [YamlProvider](/octodns/provider/yaml.py) | built-in | Supports all record types and core functionality | ### Updating to use extracted providers @@ -242,7 +242,6 @@ Similar to providers, but can only serve to populate records into a zone, cannot | [AxfrSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, LOC, MX, NS, PTR, SPF, SRV, TXT | No | read-only | | [ZoneFileSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, MX, NS, PTR, SPF, SRV, TXT | No | read-only | | [TinyDnsFileSource](/octodns/source/tinydns.py) | A, CNAME, MX, NS, PTR | No | read-only | -| [YamlProvider](/octodns/provider/yaml.py) | All | Yes | config | #### Notes From 40820f351edc4de75da9b5b329cefb8e096b0a67 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 16 Feb 2022 13:41:35 -0800 Subject: [PATCH 09/21] Implement and test Zone.root_ns helper property --- octodns/zone.py | 14 +++++++++++ tests/test_octodns_zone.py | 50 +++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/octodns/zone.py b/octodns/zone.py index c1929cf..4cd5e91 100644 --- a/octodns/zone.py +++ b/octodns/zone.py @@ -36,6 +36,7 @@ class Zone(object): # We're grouping by node, it allows us to efficiently search for # duplicates and detect when CNAMEs co-exist with other records self._records = defaultdict(set) + self._root_ns = None # optional leading . to match empty hostname # optional trailing . b/c some sources don't have it on their fqdn self._name_re = re.compile(fr'\.?{name}?$') @@ -53,6 +54,12 @@ class Zone(object): return self._origin.records return set([r for _, node in self._records.items() for r in node]) + @property + def root_ns(self): + if self._origin: + return self._origin.root_ns + return self._root_ns + def hostname_from_fqdn(self, fqdn): return self._name_re.sub('', fqdn) @@ -91,11 +98,18 @@ class Zone(object): f'{record.fqdn} cannot coexist with ' 'other records') + if record._type == 'NS' and record.name == '': + self._root_ns = record + node.add(record) def remove_record(self, record): if self._origin: self.hydrate() + + if record._type == 'NS' and record.name == '': + self._root_ns = None + self._records[record.name].discard(record) # TODO: delete this diff --git a/tests/test_octodns_zone.py b/tests/test_octodns_zone.py index e032369..9385984 100644 --- a/tests/test_octodns_zone.py +++ b/tests/test_octodns_zone.py @@ -7,7 +7,8 @@ from __future__ import absolute_import, division, print_function, \ from unittest import TestCase -from octodns.record import ARecord, AaaaRecord, Create, Delete, Record, Update +from octodns.record import ARecord, AaaaRecord, Create, Delete, NsRecord, \ + Record, Update from octodns.zone import DuplicateRecordException, InvalidNodeException, \ SubzoneRecordException, Zone @@ -410,3 +411,50 @@ class TestZone(TestCase): self.assertTrue(copy.hydrate()) # Doesn't the second self.assertFalse(copy.hydrate()) + + def test_root_ns(self): + zone = Zone('unit.tests.', []) + + a = ARecord(zone, 'a', {'ttl': 42, 'value': '1.1.1.1'}) + zone.add_record(a) + # No root NS yet + self.assertFalse(zone.root_ns) + + non_root_ns = NsRecord(zone, 'sub', {'ttl': 42, 'values': ( + 'ns1.unit.tests.', + 'ns2.unit.tests.', + )}) + zone.add_record(non_root_ns) + # No root NS yet b/c this was a sub + self.assertFalse(zone.root_ns) + + root_ns = NsRecord(zone, '', {'ttl': 42, 'values': ( + 'ns3.unit.tests.', + 'ns4.unit.tests.', + )}) + zone.add_record(root_ns) + # Now we have a root NS + self.assertEqual(root_ns, zone.root_ns) + + # make a copy, it has a root_ns + copy = zone.copy() + self.assertEqual(root_ns, copy.root_ns) + + # remove the root NS from it and we don't + copy.remove_record(root_ns) + self.assertFalse(copy.root_ns) + + # original still does though + self.assertEqual(root_ns, zone.root_ns) + + # remove the A, still has root NS + zone.remove_record(a) + self.assertEqual(root_ns, zone.root_ns) + + # remove the sub NS, still has root NS + zone.remove_record(non_root_ns) + self.assertEqual(root_ns, zone.root_ns) + + # finally remove the root NS, no more + zone.remove_record(root_ns) + self.assertFalse(zone.root_ns) From 38b51fb456ef8808cd24d40b004f32970814de96 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 16 Feb 2022 14:18:51 -0800 Subject: [PATCH 10/21] Rework unsafe bits, add RootNsChange as a new type of unsafe plan --- octodns/provider/plan.py | 49 ++++++++---- tests/test_octodns_plan.py | 156 ++++++++++++++++++++++++++++++++++++- 2 files changed, 191 insertions(+), 14 deletions(-) diff --git a/octodns/provider/plan.py b/octodns/provider/plan.py index 09ccc73..f3416b8 100644 --- a/octodns/provider/plan.py +++ b/octodns/provider/plan.py @@ -12,12 +12,22 @@ from io import StringIO class UnsafePlan(Exception): + pass + + +class RootNsChange(UnsafePlan): + + def __init__(self): + super().__init__('Root NS record change, force required') + + +class TooMuchChange(UnsafePlan): def __init__(self, why, update_pcent, update_threshold, change_count, existing_count): - msg = f'{why}, {update_pcent:.2f} is over {update_threshold:.2f} %' \ - f'({change_count}/{existing_count})' - super(UnsafePlan, self).__init__(msg) + msg = f'{why}, {update_pcent:.2f}% is over {update_threshold:.2f}% ' \ + f'({change_count}/{existing_count}), force required' + super().__init__(msg) class Plan(object): @@ -67,19 +77,32 @@ class Plan(object): len(self.existing.records) >= self.MIN_EXISTING_RECORDS: existing_record_count = len(self.existing.records) - update_pcent = self.change_counts['Update'] / existing_record_count - delete_pcent = self.change_counts['Delete'] / existing_record_count + if existing_record_count > 0: + update_pcent = (self.change_counts['Update'] / + existing_record_count) + delete_pcent = (self.change_counts['Delete'] / + existing_record_count) + else: + update_pcent = 0 + delete_pcent = 0 if update_pcent > self.update_pcent_threshold: - raise UnsafePlan('Too many updates', update_pcent * 100, - self.update_pcent_threshold * 100, - self.change_counts['Update'], - existing_record_count) + raise TooMuchChange('Too many updates', update_pcent * 100, + self.update_pcent_threshold * 100, + self.change_counts['Update'], + existing_record_count) if delete_pcent > self.delete_pcent_threshold: - raise UnsafePlan('Too many deletes', delete_pcent * 100, - self.delete_pcent_threshold * 100, - self.change_counts['Delete'], - existing_record_count) + raise TooMuchChange('Too many deletes', delete_pcent * 100, + self.delete_pcent_threshold * 100, + self.change_counts['Delete'], + existing_record_count) + + # If we have any changes of the root NS record for the zone it's a huge + # deal and force should always be required for extra care + if [True for c in self.changes + if c.record and c.record._type == 'NS' and + c.record.name == '']: + raise RootNsChange() def __repr__(self): creates = self.change_counts['Create'] diff --git a/tests/test_octodns_plan.py b/tests/test_octodns_plan.py index 4dfad54..6c01bb8 100644 --- a/tests/test_octodns_plan.py +++ b/tests/test_octodns_plan.py @@ -9,7 +9,8 @@ from io import StringIO from logging import getLogger from unittest import TestCase -from octodns.provider.plan import Plan, PlanHtml, PlanLogger, PlanMarkdown +from octodns.provider.plan import Plan, PlanHtml, PlanLogger, PlanMarkdown, \ + RootNsChange, TooMuchChange from octodns.record import Create, Delete, Record, Update from octodns.zone import Zone @@ -110,3 +111,156 @@ class TestPlanMarkdown(TestCase): self.assertTrue('Update | a | A | 300 | 1.1.1.1;' in out) self.assertTrue('NA-US: 6.6.6.6 | test' in out) self.assertTrue('Delete | a | A | 300 | 2.2.2.2;' in out) + + +class HelperPlan(Plan): + + def __init__(self, *args, min_existing=0, **kwargs): + super().__init__(*args, **kwargs) + self.MIN_EXISTING_RECORDS = min_existing + + +class TestPlanSafety(TestCase): + existing = Zone('unit.tests.', []) + record_1 = Record.new(existing, '1', data={ + 'type': 'A', + 'ttl': 42, + 'value': '1.2.3.4', + }) + record_2 = Record.new(existing, '2', data={ + 'type': 'A', + 'ttl': 42, + 'value': '1.2.3.4', + }) + record_3 = Record.new(existing, '3', data={ + 'type': 'A', + 'ttl': 42, + 'value': '1.2.3.4', + }) + record_4 = Record.new(existing, '4', data={ + 'type': 'A', + 'ttl': 42, + 'value': '1.2.3.4', + }) + + def test_too_many_updates(self): + existing = self.existing.copy() + changes = [] + + # No records, no changes, we're good + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # Four records, no changes, we're good + existing.add_record(self.record_1) + existing.add_record(self.record_2) + existing.add_record(self.record_3) + existing.add_record(self.record_4) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # Creates don't count against us + changes.append(Create(self.record_1)) + changes.append(Create(self.record_2)) + changes.append(Create(self.record_3)) + changes.append(Create(self.record_4)) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # One update, still good (25%, default threshold is 33%) + changes.append(Update(self.record_1, self.record_1)) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # Two and we're over the threshold + changes.append(Update(self.record_2, self.record_2)) + plan = HelperPlan(existing, None, changes, True) + with self.assertRaises(TooMuchChange) as ctx: + plan.raise_if_unsafe() + self.assertTrue('Too many updates', str(ctx.exception)) + + # If we require more records before applying we're still OK though + plan = HelperPlan(existing, None, changes, True, min_existing=10) + plan.raise_if_unsafe() + + def test_too_many_deletes(self): + existing = self.existing.copy() + changes = [] + + # No records, no changes, we're good + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # Four records, no changes, we're good + existing.add_record(self.record_1) + existing.add_record(self.record_2) + existing.add_record(self.record_3) + existing.add_record(self.record_4) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # Creates don't count against us + changes.append(Create(self.record_1)) + changes.append(Create(self.record_2)) + changes.append(Create(self.record_3)) + changes.append(Create(self.record_4)) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # One delete, still good (25%, default threshold is 33%) + changes.append(Delete(self.record_1)) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # Two and we're over the threshold + changes.append(Delete(self.record_2)) + plan = HelperPlan(existing, None, changes, True) + with self.assertRaises(TooMuchChange) as ctx: + plan.raise_if_unsafe() + self.assertTrue('Too many deletes', str(ctx.exception)) + + # If we require more records before applying we're still OK though + plan = HelperPlan(existing, None, changes, True, min_existing=10) + plan.raise_if_unsafe() + + def test_root_ns_change(self): + existing = self.existing.copy() + changes = [] + + # No records, no changes, we're good + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + existing.add_record(self.record_1) + existing.add_record(self.record_2) + existing.add_record(self.record_3) + existing.add_record(self.record_4) + + # Non NS changes and we're still good + changes.append(Update(self.record_1, self.record_1)) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + + # Add a change to a non-root NS record, we're OK + ns_record = Record.new(existing, 'sub', data={ + 'type': 'NS', + 'ttl': 43, + 'values': ('ns1.unit.tests.', 'ns1.unit.tests.'), + }) + changes.append(Delete(ns_record)) + plan = HelperPlan(existing, None, changes, True) + plan.raise_if_unsafe() + # Remove that Delete so that we don't go over the delete threshold + changes.pop(-1) + + # Delete the root NS record and we get an unsafe + root_ns_record = Record.new(existing, '', data={ + 'type': 'NS', + 'ttl': 43, + 'values': ('ns3.unit.tests.', 'ns4.unit.tests.'), + }) + changes.append(Delete(root_ns_record)) + plan = HelperPlan(existing, None, changes, True) + with self.assertRaises(RootNsChange) as ctx: + plan.raise_if_unsafe() + self.assertTrue('Root Ns record change', str(ctx.exception)) From f43833e9bbc817b0a98698fee1b7f63c437b9675 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 16 Feb 2022 14:38:10 -0800 Subject: [PATCH 11/21] Make YamlProvider.SUPPORTS_ROOT_NS configurable, default True This will result in less churn in tests for root NS support and allow us to enable/disable things easily as needed. --- octodns/provider/yaml.py | 9 +++++++-- tests/config/always-dry-run.yaml | 1 + tests/config/processors.yaml | 1 + tests/config/simple-alias-zone.yaml | 1 + tests/config/simple.yaml | 2 ++ tests/test_octodns_manager.py | 18 +++++++++--------- 6 files changed, 21 insertions(+), 11 deletions(-) diff --git a/octodns/provider/yaml.py b/octodns/provider/yaml.py index 4c146bb..16a4bc1 100644 --- a/octodns/provider/yaml.py +++ b/octodns/provider/yaml.py @@ -106,13 +106,13 @@ class YamlProvider(BaseProvider): SUPPORTS_DYNAMIC = True SUPPORTS_POOL_VALUE_STATUS = True SUPPORTS_MULTIVALUE_PTR = True - SUPPORTS_ROOT_NS = True SUPPORTS = set(('A', 'AAAA', 'ALIAS', 'CAA', 'CNAME', 'DNAME', 'LOC', 'MX', 'NAPTR', 'NS', 'PTR', 'SSHFP', 'SPF', 'SRV', 'TXT', 'URLFWD')) def __init__(self, id, directory, default_ttl=3600, enforce_order=True, - populate_should_replace=False, *args, **kwargs): + populate_should_replace=False, supports_root_ns=True, + *args, **kwargs): klass = self.__class__.__name__ self.log = logging.getLogger(f'{klass}[{id}]') self.log.debug('__init__: id=%s, directory=%s, default_ttl=%d, ' @@ -124,6 +124,11 @@ class YamlProvider(BaseProvider): self.default_ttl = default_ttl self.enforce_order = enforce_order self.populate_should_replace = populate_should_replace + self.supports_root_ns = supports_root_ns + + @property + def SUPPORTS_ROOT_NS(self): + return self.supports_root_ns def _populate_from_file(self, filename, zone, lenient): with open(filename, 'r') as fh: diff --git a/tests/config/always-dry-run.yaml b/tests/config/always-dry-run.yaml index 466c26b..333ecff 100644 --- a/tests/config/always-dry-run.yaml +++ b/tests/config/always-dry-run.yaml @@ -5,6 +5,7 @@ providers: dump: class: octodns.provider.yaml.YamlProvider directory: env/YAML_TMP_DIR + supports_root_ns: False zones: unit.tests.: always-dry-run: true diff --git a/tests/config/processors.yaml b/tests/config/processors.yaml index 097024b..4229707 100644 --- a/tests/config/processors.yaml +++ b/tests/config/processors.yaml @@ -5,6 +5,7 @@ providers: dump: class: octodns.provider.yaml.YamlProvider directory: env/YAML_TMP_DIR + supports_root_ns: False geo: class: helpers.GeoProvider nosshfp: diff --git a/tests/config/simple-alias-zone.yaml b/tests/config/simple-alias-zone.yaml index 32154d5..d3e7612 100644 --- a/tests/config/simple-alias-zone.yaml +++ b/tests/config/simple-alias-zone.yaml @@ -7,6 +7,7 @@ providers: dump: class: octodns.provider.yaml.YamlProvider directory: env/YAML_TMP_DIR + supports_root_ns: False zones: unit.tests.: sources: diff --git a/tests/config/simple.yaml b/tests/config/simple.yaml index cf970a9..af54fc9 100644 --- a/tests/config/simple.yaml +++ b/tests/config/simple.yaml @@ -7,11 +7,13 @@ providers: dump: class: octodns.provider.yaml.YamlProvider directory: env/YAML_TMP_DIR + supports_root_ns: False # This is sort of ugly, but it shouldn't hurt anything. It'll just write out # the target file twice where it and dump are both used dump2: class: octodns.provider.yaml.YamlProvider directory: env/YAML_TMP_DIR + supports_root_ns: False simple: class: helpers.SimpleProvider geo: diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 4d537cd..c94b476 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -113,45 +113,45 @@ 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(4, tc) + self.assertEqual(3, 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(30, tc) + self.assertEqual(26, tc) # try with just one of the zones tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, eligible_zones=['unit.tests.']) - self.assertEqual(21, tc) + self.assertEqual(20, tc) # the subzone, with 2 targets tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, eligible_zones=['subzone.unit.tests.']) - self.assertEqual(8, tc) + self.assertEqual(6, tc) - # and finally the empty zone (only root NS) + # and finally the empty zone tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, eligible_zones=['empty.']) - self.assertEqual(1, tc) + self.assertEqual(0, tc) # Again with force tc = Manager(get_config_filename('simple.yaml')) \ .sync(dry_run=False, force=True) - self.assertEqual(30, tc) + self.assertEqual(26, tc) # Again with max_workers = 1 tc = Manager(get_config_filename('simple.yaml'), max_workers=1) \ .sync(dry_run=False, force=True) - self.assertEqual(30, tc) + self.assertEqual(26, tc) # Include meta tc = Manager(get_config_filename('simple.yaml'), max_workers=1, include_meta=True) \ .sync(dry_run=False, force=True) - self.assertEqual(34, tc) + self.assertEqual(30, tc) def test_eligible_sources(self): with TemporaryDirectory() as tmpdir: From adb01a982c89b0dc79b67a2df9fe6d01bbdbe533 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 16 Feb 2022 15:00:21 -0800 Subject: [PATCH 12/21] WIP: Backing out a lot of test churn now that YamlProvider has SUPPORTS_ROOT_NS flag --- octodns/manager.py | 3 ++ octodns/provider/base.py | 22 +++------ 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 | 2 +- tests/test_octodns_provider_base.py | 6 ++- tests/test_octodns_provider_yaml.py | 50 +++++++++++--------- 9 files changed, 42 insertions(+), 62 deletions(-) delete mode 100644 tests/config/split/dynamic.tests.tst/ns.yaml diff --git a/octodns/manager.py b/octodns/manager.py index d9e4093..d7b1932 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -12,6 +12,7 @@ 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 @@ -519,6 +520,8 @@ 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) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 51cda9e..b57f371 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -49,7 +49,6 @@ 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}' @@ -96,21 +95,12 @@ class BaseProvider(BaseSource): record.values = [record.value] desired.add_record(record, replace=True) 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') + # 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 diff --git a/tests/config/dynamic.tests.yaml b/tests/config/dynamic.tests.yaml index 35b3117..d25f63a 100644 --- a/tests/config/dynamic.tests.yaml +++ b/tests/config/dynamic.tests.yaml @@ -1,9 +1,4 @@ --- -? '' -: - 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 f7b4285..ed97d53 100644 --- a/tests/config/empty.yaml +++ b/tests/config/empty.yaml @@ -1,6 +1 @@ --- -? '' -: - 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 deleted file mode 100644 index 48dd39a..0000000 --- a/tests/config/split/dynamic.tests.tst/ns.yaml +++ /dev/null @@ -1,6 +0,0 @@ ---- -? '' -: - 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 c9516d1..932ac28 100644 --- a/tests/config/subzone.unit.tests.yaml +++ b/tests/config/subzone.unit.tests.yaml @@ -1,9 +1,4 @@ --- -? '' -: - 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 c94b476..e558dfe 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -302,7 +302,7 @@ class TestManager(TestCase): with open(join(tmpdir.dirname, 'empty.yaml')) as fh: data = safe_load(fh, False) # just to root NS - self.assertEqual(1, len(data)) + self.assertEqual(0, 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 acd4592..24dee84 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -766,7 +766,8 @@ class TestBaseProviderSupportsRootNs(TestCase): self.assertEqual(self.other_root_ns_record, change.existing) self.assertEqual(self.root_ns_record, change.new) - def test_supports_root_ns_true_missing(self): + # TODO: rework + 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 @@ -794,7 +795,8 @@ class TestBaseProviderSupportsRootNs(TestCase): self.assertFalse(change.existing) self.assertEqual(self.root_ns_record, change.new) - def test_supports_root_ns_true_create_zone_missing(self): + # TODO: rework + def _test_supports_root_ns_true_create_zone_missing(self): # provider has no existing records (create) provider = self.Provider() provider.SUPPORTS_ROOT_NS = True diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py index 8fda388..f810493 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(7, len(dynamic_zone.records)) + self.assertEqual(6, 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 @@ -53,25 +53,25 @@ class TestYamlProvider(TestCase): directory = join(td.dirname, 'sub', 'dir') yaml_file = join(directory, 'unit.tests.yaml') dynamic_yaml_file = join(directory, 'dynamic.tests.yaml') - target = YamlProvider('test', directory) + target = YamlProvider('test', directory, supports_root_ns=False) # We add everything plan = target.plan(zone) - self.assertEqual(21, len([c for c in plan.changes + self.assertEqual(20, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isfile(yaml_file)) # Now actually do it - self.assertEqual(21, target.apply(plan)) + self.assertEqual(20, target.apply(plan)) self.assertTrue(isfile(yaml_file)) # Dynamic plan plan = target.plan(dynamic_zone) - self.assertEqual(7, len([c for c in plan.changes + self.assertEqual(6, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isfile(dynamic_yaml_file)) # Apply it - self.assertEqual(7, target.apply(plan)) + self.assertEqual(6, target.apply(plan)) self.assertTrue(isfile(dynamic_yaml_file)) # There should be no changes after the round trip @@ -160,16 +160,18 @@ class TestYamlProvider(TestCase): self.assertEqual([], list(data.keys())) def test_empty(self): - source = YamlProvider('test', join(dirname(__file__), 'config')) + source = YamlProvider('test', join(dirname(__file__), 'config'), + supports_root_ns=False) zone = Zone('empty.', []) - # without it we see everything (root NS record) + # without it we see everything source.populate(zone) - self.assertEqual(1, len(zone.records)) + self.assertEqual(0, len(zone.records)) def test_unsorted(self): - source = YamlProvider('test', join(dirname(__file__), 'config')) + source = YamlProvider('test', join(dirname(__file__), 'config'), + supports_root_ns=False) zone = Zone('unordered.', []) @@ -177,13 +179,14 @@ class TestYamlProvider(TestCase): source.populate(zone) source = YamlProvider('test', join(dirname(__file__), 'config'), - enforce_order=False) + enforce_order=False, supports_root_ns=False) # no exception source.populate(zone) self.assertEqual(2, len(zone.records)) def test_subzone_handling(self): - source = YamlProvider('test', join(dirname(__file__), 'config')) + source = YamlProvider('test', join(dirname(__file__), 'config'), + supports_root_ns=False) # If we add `sub` as a sub-zone we'll reject `www.sub` zone = Zone('unit.tests.', ['sub']) @@ -255,7 +258,7 @@ class TestSplitYamlProvider(TestCase): self.assertEqual(20, len(zone.records)) source.populate(dynamic_zone) - self.assertEqual(6, len(dynamic_zone.records)) + self.assertEqual(5, len(dynamic_zone.records)) with TemporaryDirectory() as td: # Add some subdirs to make sure that it can create them @@ -263,24 +266,25 @@ class TestSplitYamlProvider(TestCase): zone_dir = join(directory, 'unit.tests.tst') dynamic_zone_dir = join(directory, 'dynamic.tests.tst') target = SplitYamlProvider('test', directory, - extension='.tst') + extension='.tst', + supports_root_ns=False) # We add everything plan = target.plan(zone) - self.assertEqual(18, len([c for c in plan.changes + self.assertEqual(17, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isdir(zone_dir)) # Now actually do it - self.assertEqual(18, target.apply(plan)) + self.assertEqual(17, target.apply(plan)) # Dynamic plan plan = target.plan(dynamic_zone) - self.assertEqual(6, len([c for c in plan.changes + self.assertEqual(5, len([c for c in plan.changes if isinstance(c, Create)])) self.assertFalse(isdir(dynamic_zone_dir)) # Apply it - self.assertEqual(6, target.apply(plan)) + self.assertEqual(5, target.apply(plan)) self.assertTrue(isdir(dynamic_zone_dir)) # There should be no changes after the round trip @@ -396,16 +400,18 @@ class TestOverridingYamlProvider(TestCase): def test_provider(self): config = join(dirname(__file__), 'config') override_config = join(dirname(__file__), 'config', 'override') - base = YamlProvider('base', config, populate_should_replace=False) + base = YamlProvider('base', config, populate_should_replace=False, + supports_root_ns=False) override = YamlProvider('test', override_config, - populate_should_replace=True) + populate_should_replace=True, + supports_root_ns=False) zone = Zone('dynamic.tests.', []) # Load the base, should see the 5 records base.populate(zone) got = {r.name: r for r in zone.records} - self.assertEqual(7, len(got)) + self.assertEqual(6, len(got)) # We get the "dynamic" A from the base config self.assertTrue('dynamic' in got['a'].data) # No added @@ -414,7 +420,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(8, len(got)) + self.assertEqual(7, len(got)) # 'a' was replaced with a generic record self.assertEqual({ 'ttl': 3600, From 02296652cab2d0ea4096349587f53d1d1b630f7e Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 16 Feb 2022 15:23:55 -0800 Subject: [PATCH 13/21] WIP: Backing out a more test churn now that YamlProvider has SUPPORTS_ROOT_NS flag --- octodns/manager.py | 1 - tests/config/simple.yaml | 1 + tests/test_octodns_manager.py | 3 +-- tests/test_octodns_provider_yaml.py | 16 ++++++++++------ 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/octodns/manager.py b/octodns/manager.py index d7b1932..c8a4263 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -522,7 +522,6 @@ class Manager(object): 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/tests/config/simple.yaml b/tests/config/simple.yaml index af54fc9..fc4ad9f 100644 --- a/tests/config/simple.yaml +++ b/tests/config/simple.yaml @@ -4,6 +4,7 @@ providers: in: class: octodns.provider.yaml.YamlProvider directory: tests/config + supports_root_ns: False dump: class: octodns.provider.yaml.YamlProvider directory: env/YAML_TMP_DIR diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index e558dfe..1e65386 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -301,8 +301,7 @@ class TestManager(TestCase): with open(join(tmpdir.dirname, 'empty.yaml')) as fh: data = safe_load(fh, False) - # just to root NS - self.assertEqual(0, len(data)) + self.assertFalse(data) def test_dump_split(self): with TemporaryDirectory() as tmpdir: diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py index f810493..51e55eb 100644 --- a/tests/test_octodns_provider_yaml.py +++ b/tests/test_octodns_provider_yaml.py @@ -82,11 +82,15 @@ class TestYamlProvider(TestCase): [x for x in reloaded.records if x.name == 'included'][0]._octodns) + # manually copy over the root since it will have been ignored + # when things were written out + reloaded.add_record(zone.root_ns) + self.assertFalse(zone.changes(reloaded, target=source)) # A 2nd sync should still create everything plan = target.plan(zone) - self.assertEqual(21, len([c for c in plan.changes + self.assertEqual(20, len([c for c in plan.changes if isinstance(c, Create)])) with open(yaml_file) as fh: @@ -152,10 +156,6 @@ 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())) @@ -295,11 +295,15 @@ class TestSplitYamlProvider(TestCase): [x for x in reloaded.records if x.name == 'included'][0]._octodns) + # manually copy over the root since it will have been ignored + # when things were written out + reloaded.add_record(zone.root_ns) + self.assertFalse(zone.changes(reloaded, target=source)) # A 2nd sync should still create everything plan = target.plan(zone) - self.assertEqual(18, len([c for c in plan.changes + self.assertEqual(17, len([c for c in plan.changes if isinstance(c, Create)])) yaml_file = join(zone_dir, '$unit.tests.yaml') From 1b543c675f58d3bdb7422bee0f3b2d83fee77b98 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 16 Feb 2022 15:37:16 -0800 Subject: [PATCH 14/21] Finish backing out test churn w/YamlProvider's optional SUPPORTS_ROOT_NS --- octodns/provider/base.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index b57f371..357dc83 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -94,7 +94,8 @@ class BaseProvider(BaseSource): record = record.copy() record.values = [record.value] desired.add_record(record, replace=True) - elif record._type == 'NS' and record.name == '': + 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}' From 14fc13778844fea9038530582dc90960068b577a Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 16 Feb 2022 15:46:46 -0800 Subject: [PATCH 15/21] Use Zone.root_ns to avoid lookping/searching --- octodns/provider/base.py | 34 +++++++++++++++++----------------- tests/test_octodns_manager.py | 2 ++ 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 357dc83..3bc8094 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -94,14 +94,15 @@ 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) + + 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) return desired @@ -124,15 +125,14 @@ class BaseProvider(BaseSource): provider configuration. ''' - for record in existing.records: - if 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) - existing.remove_record(record) + record = existing.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) + existing.remove_record(record) return existing diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 1e65386..9e68030 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -216,6 +216,8 @@ class TestManager(TestCase): with open(join(tmpdir.dirname, 'unit.tests.yaml'), 'w') as fh: fh.write('---\n{}') + # compare doesn't use _process_desired_zone and thus doesn't filter + # out root NS records, that seems fine/desirable changes = manager.compare(['in'], ['dump'], 'unit.tests.') self.assertEqual(21, len(changes)) From 02ee7518fa3e73203bb045afabaa9a336255e9a1 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 17 Feb 2022 08:50:42 -0800 Subject: [PATCH 16/21] 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)) From 81dd4d714b7363d098904e56f6162bd878e92957 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 17 Feb 2022 09:34:48 -0800 Subject: [PATCH 17/21] Only throw RootNsChange UnsafePlan on existing zones --- octodns/provider/plan.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/octodns/provider/plan.py b/octodns/provider/plan.py index f3416b8..9852ef5 100644 --- a/octodns/provider/plan.py +++ b/octodns/provider/plan.py @@ -99,9 +99,9 @@ class Plan(object): # If we have any changes of the root NS record for the zone it's a huge # deal and force should always be required for extra care - if [True for c in self.changes - if c.record and c.record._type == 'NS' and - c.record.name == '']: + if self.exists and [True for c in self.changes + if c.record and c.record._type == 'NS' and + c.record.name == '']: raise RootNsChange() def __repr__(self): From 0544e9ed7d350d257247a93bb4889b1904809f33 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 17 Feb 2022 13:00:28 -0800 Subject: [PATCH 18/21] More thorough testing of root ns cases and associated improvements --- octodns/provider/base.py | 23 ++++---- tests/test_octodns_provider_base.py | 83 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 14 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 0af75ef..c5c81e4 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -98,12 +98,13 @@ class BaseProvider(BaseSource): record = desired.root_ns 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) + msg = 'root NS record supported, but no record is ' \ + f'configured for {desired.name}' + fallback = 'ignoring it' + self.supports_warn_or_except(msg, fallback) else: if record: - # ignore, we can't manage root NS records + # we can't manage root NS records, get rid of it msg = \ f'root NS record not supported for {record.fqdn}' fallback = 'ignoring it' @@ -133,16 +134,10 @@ class BaseProvider(BaseSource): ''' 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 {existing_root_ns.fqdn}' - fallback = 'ignoring it' - self.supports_warn_or_except(msg, fallback) + if existing_root_ns and (not self.SUPPORTS_ROOT_NS or + not desired.root_ns): + self.log.info('root NS record in existing, but not supported or ' + 'not configured; ignoring it') existing.remove_record(existing_root_ns) return existing diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 2498dca..027a07c 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -707,6 +707,7 @@ class TestBaseProviderSupportsRootNs(TestCase): self.assertFalse(plan) # plan again with strict_supports enabled, we should get an exception + # b/c we have something configured that can't be managed provider.strict_supports = True with self.assertRaises(SupportsException) as ctx: provider.plan(self.has_root) @@ -725,6 +726,15 @@ class TestBaseProviderSupportsRootNs(TestCase): # anyway, they will have been removed from the desired and existing. self.assertFalse(plan) + # plan again with strict_supports enabled, we should get an exception + # b/c we have something configured that can't be managed (doesn't + # matter that it's a mis-match) + 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_missing(self): # provider has an existing record provider = self.Provider(self.has_root) @@ -737,6 +747,12 @@ class TestBaseProviderSupportsRootNs(TestCase): # anyway, they will have been removed from the desired and existing. self.assertFalse(plan) + # plan again with strict supports enabled, no change since desired + # isn't asking to manage root + provider.strict_supports = True + plan = provider.plan(self.no_root) + self.assertFalse(plan) + def test_supports_root_ns_false_create_zone(self): # provider has no existing records (create) provider = self.Provider() @@ -749,6 +765,33 @@ class TestBaseProviderSupportsRootNs(TestCase): self.assertTrue(plan) self.assertEqual(2, len(plan.changes)) + # plan again with strict supports enabled, we'll get an exception b/c + # the target provider can't manage something in desired + 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_create_zone_missing(self): + # 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.no_root) + + # no support for root NS so we only create the other two records + self.assertTrue(plan) + self.assertEqual(2, len(plan.changes)) + + # plan again with strict supports enabled, same result since we're not + # asking for a root NS it's just the 2 other changes + provider.strict_supports = True + plan = provider.plan(self.no_root) + self.assertTrue(plan) + self.assertEqual(2, len(plan.changes)) + # True def test_supports_root_ns_true_matches(self): @@ -763,6 +806,11 @@ class TestBaseProviderSupportsRootNs(TestCase): # change self.assertFalse(plan) + # again with strict supports enabled, no difference + provider.strict_supports = True + plan = provider.plan(self.has_root) + self.assertFalse(plan) + def test_supports_root_ns_true_different(self): # provider has a non-matching existing record provider = self.Provider(self.different_root) @@ -778,6 +826,14 @@ class TestBaseProviderSupportsRootNs(TestCase): self.assertEqual(self.other_root_ns_record, change.existing) self.assertEqual(self.root_ns_record, change.new) + # again with strict supports enabled, no difference, we see the change + provider.strict_supports = True + plan = provider.plan(self.has_root) + 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_missing(self): # provider has a matching existing root record provider = self.Provider(self.has_root) @@ -790,6 +846,14 @@ class TestBaseProviderSupportsRootNs(TestCase): # aren't configured with one to manage self.assertFalse(plan) + # again with strict supports enabled, this time we throw an exception + # b/c it's not being managed and could be + provider.strict_supports = True + with self.assertRaises(SupportsException) as ctx: + provider.plan(self.no_root) + self.assertEqual('test: root NS record supported, but no record is ' + 'configured for unit.tests.', str(ctx.exception)) + def test_supports_root_ns_true_create_zone(self): # provider has no existing records (create) provider = self.Provider() @@ -807,6 +871,17 @@ class TestBaseProviderSupportsRootNs(TestCase): self.assertFalse(change.existing) self.assertEqual(self.root_ns_record, change.new) + # again with strict supports enabled, no difference, we see all 3 + # changes + provider.strict_supports = True + plan = provider.plan(self.has_root) + 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_create_zone_missing(self): # provider has no existing records (create) provider = self.Provider() @@ -816,3 +891,11 @@ class TestBaseProviderSupportsRootNs(TestCase): # manage the other records plan = provider.plan(self.no_root) self.assertEqual(2, len(plan.changes)) + + # again with strict supports enabled, this time we throw an exception + # b/c it's not being managed and could be + provider.strict_supports = True + with self.assertRaises(SupportsException) as ctx: + provider.plan(self.no_root) + self.assertEqual('test: root NS record supported, but no record is ' + 'configured for unit.tests.', str(ctx.exception)) From e3edae8466de391099c5e40b199a17817739db64 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 17 Feb 2022 13:24:34 -0800 Subject: [PATCH 19/21] Non-configured root NS when supported is always a warning --- octodns/provider/base.py | 6 ++-- tests/test_octodns_provider_base.py | 46 +++++++++++++++++++++-------- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index c5c81e4..b524017 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -98,10 +98,8 @@ class BaseProvider(BaseSource): record = desired.root_ns if self.SUPPORTS_ROOT_NS: if not record: - msg = 'root NS record supported, but no record is ' \ - f'configured for {desired.name}' - fallback = 'ignoring it' - self.supports_warn_or_except(msg, fallback) + self.log.warning('root NS record supported, but no record ' + 'is configured for %s', desired.name) else: if record: # we can't manage root NS records, get rid of it diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 027a07c..398e300 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -167,6 +167,28 @@ class TestBaseProvider(TestCase): self.assertTrue(plan) self.assertEqual(1, len(plan.changes)) + def test_plan_with_process_desired_zone_kwarg_fallback(self): + ignored = Zone('unit.tests.', []) + + class OldApiProvider(HelperProvider): + + def _process_desired_zone(self, desired): + return desired + + # No change, thus no plan + provider = OldApiProvider([]) + self.assertEqual(None, provider.plan(ignored)) + + class OtherTypeErrorProvider(HelperProvider): + + def _process_desired_zone(self, desired, exists=False): + raise TypeError('foo') + + provider = OtherTypeErrorProvider([]) + with self.assertRaises(TypeError) as ctx: + provider.plan(ignored) + self.assertEqual('foo', str(ctx.exception)) + def test_plan_with_unsupported_type(self): zone = Zone('unit.tests.', []) @@ -846,13 +868,14 @@ class TestBaseProviderSupportsRootNs(TestCase): # aren't configured with one to manage self.assertFalse(plan) - # again with strict supports enabled, this time we throw an exception - # b/c it's not being managed and could be + # again with strict supports enabled, no difference as non-configured + # root NS is a special case that we always just warn about. This is + # because we can't known them before it's created and some people may + # choose to just leave them unmanaged undefinitely which has been the + # behavior up until now. provider.strict_supports = True - with self.assertRaises(SupportsException) as ctx: - provider.plan(self.no_root) - self.assertEqual('test: root NS record supported, but no record is ' - 'configured for unit.tests.', str(ctx.exception)) + plan = provider.plan(self.no_root) + self.assertFalse(plan) def test_supports_root_ns_true_create_zone(self): # provider has no existing records (create) @@ -892,10 +915,9 @@ class TestBaseProviderSupportsRootNs(TestCase): plan = provider.plan(self.no_root) self.assertEqual(2, len(plan.changes)) - # again with strict supports enabled, this time we throw an exception - # b/c it's not being managed and could be + # again with strict supports enabled, we'd normally throw an exception, + # but since this is a create and we often can't know the root NS values + # before the zone is created it's special cased and will only warn provider.strict_supports = True - with self.assertRaises(SupportsException) as ctx: - provider.plan(self.no_root) - self.assertEqual('test: root NS record supported, but no record is ' - 'configured for unit.tests.', str(ctx.exception)) + plan = provider.plan(self.no_root) + self.assertEqual(2, len(plan.changes)) From 8e70695eb21cc19f9fcf9463ec63173073582de0 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 23 Feb 2022 09:32:20 -0800 Subject: [PATCH 20/21] Apply suggestions/corrections from a round of code review --- CHANGELOG.md | 5 +---- octodns/provider/plan.py | 6 +++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e8ec67..3075cba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,16 +5,13 @@ * Foundational support for root NS record management. * YamlProvider has it enabled and in general everyone should add root NS records that match what is in their provider(s) as of this release if they - an're already there. + aren't already there. * Note that if you created you config files with `octodns-dump` the records are likely already there and match what was configured at the time of the dump. * Other providers will add root NS support over time once they have had the chance to investigate the functionality and implement management if possible with whatever accomidations are required. - * Once you are using this release and an provider version that supports root - NS management you will get errors if you do not have root NS records - included by your source(s). * Watch your providers README.md and CHANGELOG.md for support and more information. diff --git a/octodns/provider/plan.py b/octodns/provider/plan.py index 9852ef5..2d39c80 100644 --- a/octodns/provider/plan.py +++ b/octodns/provider/plan.py @@ -99,9 +99,9 @@ class Plan(object): # If we have any changes of the root NS record for the zone it's a huge # deal and force should always be required for extra care - if self.exists and [True for c in self.changes - if c.record and c.record._type == 'NS' and - c.record.name == '']: + if self.exists and any(c for c in self.changes + if c.record and c.record._type == 'NS' and + c.record.name == ''): raise RootNsChange() def __repr__(self): From 9913211746980e8633a88c1f639d41c37a2a35bd Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 23 Feb 2022 12:20:00 -0800 Subject: [PATCH 21/21] Apply suggestions from final round of code review Co-authored-by: Viranch Mehta --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3075cba..add77cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ * YamlProvider has it enabled and in general everyone should add root NS records that match what is in their provider(s) as of this release if they aren't already there. - * Note that if you created you config files with `octodns-dump` the records + * Note that if you created your config files with `octodns-dump`, the records are likely already there and match what was configured at the time of the dump. * Other providers will add root NS support over time once they have had the