From 4334217058e5ffdacc575b9a10942abb3b6e5c01 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 11 Oct 2025 20:42:08 -0700 Subject: [PATCH] Stop relying on manager.config['zones'] being modified. _preprocess_zones mostly moved closer to where it's used --- octodns/manager.py | 199 ++++++++++++++++++---------------- tests/test_octodns_manager.py | 54 +++++---- 2 files changed, 143 insertions(+), 110 deletions(-) diff --git a/octodns/manager.py b/octodns/manager.py index 061a697..4305451 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -213,15 +213,103 @@ class Manager(object): if self._zones is None: zones = self.config['zones'] - zones = self._preprocess_zones(zones, self.active_sources) + zones = IdnaDict(self._preprocess_zones(zones)) if self.active_zones: - zones = {n: zones.get(n) for n in self.active_zones} + zones = IdnaDict({n: zones.get(n) for n in self.active_zones}) - self._zones = IdnaDict(zones) + self._zones = zones return self._zones + def _preprocess_zones(self, zones): + ''' + This may modify the passed in zone object, it should be ignored after + the call and the zones returned from this function should be used + instead. + ''' + + # we're going to be modifying the zones dict during the course of this + # method and then return the results. we'll take a shallow copy here so + # that we can safely so without impacting the original config. We don't + # momdify any of the values, just copy them as-is, so shallow should be + # fine. + zones = dict(zones) + + source_zones = {} + + # list since we'll be modifying zones in the loop + for name, config in list(zones.items()): + if name[0] != '*': + # this isn't a dynamic zone config, move along + continue + + # it's dynamic, get a list of zone names from the configured sources + sources = self._get_sources(name, config) + self.log.info( + '_preprocess_zones: dynamic zone=%s, sources=%s', + name, + (s.id for s in sources), + ) + candidates = set() + for source in sources: + if source.id not in source_zones: + if not hasattr(source, 'list_zones'): + raise ManagerException( + f'dynamic zone={name} includes a source, {source.id}, that does not support `list_zones`' + ) + # get this source's zones + listed_zones = set(source.list_zones()) + # cache them + source_zones[source.id] = listed_zones + self.log.debug( + '_preprocess_zones: source=%s, list_zones=%s', + source.id, + listed_zones, + ) + # add this source's zones to the candidates + candidates |= source_zones[source.id] + + self.log.debug( + '_preprocess_zones: name=%s, candidates=%s', name, candidates + ) + + # remove any zones that are already configured, either explicitly or + # from a previous dyanmic config + candidates -= set(zones.keys()) + + if glob := config.pop('glob', None): + self.log.debug( + '_preprocess_zones: name=%s, glob=%s', name, glob + ) + candidates = set(fnmatch_filter(candidates, glob)) + elif regex := config.pop('regex', None): + self.log.debug( + '_preprocess_zones: name=%s, regex=%s', name, regex + ) + regex = re_compile(regex) + self.log.debug( + '_preprocess_zones: name=%s, compiled=%s', name, regex + ) + candidates = set(z for z in candidates if regex.search(z)) + else: + # old-style wildcard that uses everything + self.log.debug( + '_preprocess_zones: name=%s, old semantics, catch all', name + ) + + self.log.debug( + '_preprocess_zones: name=%s, matches=%s', name, candidates + ) + + for match in candidates: + zones[match] = config + + # remove the dynamic config element so we don't try and populate it + del zones[name] + + return zones + def _config_executor(self, manager_config, max_workers=None): max_workers = ( manager_config.get('max_workers') or 1 @@ -614,87 +702,6 @@ class Manager(object): return sources - def _preprocess_zones(self, zones, sources=None): - ''' - This may modify the passed in zone object, it should be ignored after - the call and the zones returned from this function should be used - instead. - ''' - - source_zones = {} - - # list since we'll be modifying zones in the loop - for name, config in list(zones.items()): - if name[0] != '*': - # this isn't a dynamic zone config, move along - continue - - # it's dynamic, get a list of zone names from the configured sources - found_sources = sources or self._get_sources(name, config) - self.log.info( - '_preprocess_zones: dynamic zone=%s, sources=%s', - name, - (s.id for s in found_sources), - ) - candidates = set() - for source in found_sources: - if source.id not in source_zones: - if not hasattr(source, 'list_zones'): - raise ManagerException( - f'dynamic zone={name} includes a source, {source.id}, that does not support `list_zones`' - ) - # get this source's zones - listed_zones = set(source.list_zones()) - # cache them - source_zones[source.id] = listed_zones - self.log.debug( - '_preprocess_zones: source=%s, list_zones=%s', - source.id, - listed_zones, - ) - # add this source's zones to the candidates - candidates |= source_zones[source.id] - - self.log.debug( - '_preprocess_zones: name=%s, candidates=%s', name, candidates - ) - - # remove any zones that are already configured, either explicitly or - # from a previous dyanmic config - candidates -= set(zones.keys()) - - if glob := config.pop('glob', None): - self.log.debug( - '_preprocess_zones: name=%s, glob=%s', name, glob - ) - candidates = set(fnmatch_filter(candidates, glob)) - elif regex := config.pop('regex', None): - self.log.debug( - '_preprocess_zones: name=%s, regex=%s', name, regex - ) - regex = re_compile(regex) - self.log.debug( - '_preprocess_zones: name=%s, compiled=%s', name, regex - ) - candidates = set(z for z in candidates if regex.search(z)) - else: - # old-style wildcard that uses everything - self.log.debug( - '_preprocess_zones: name=%s, old semantics, catch all', name - ) - - self.log.debug( - '_preprocess_zones: name=%s, matches=%s', name, candidates - ) - - for match in candidates: - zones[match] = config - - # remove the dynamic config element so we don't try and populate it - del zones[name] - - return zones - def sync( self, dry_run=True, force=False, plan_output_fh=stdout, checksum=None ): @@ -730,6 +737,8 @@ class Manager(object): delayed_arpa = [] futures = [] + config_zones = self.config['zones'] + for zone_name, config in zones.items(): if config is None: raise ManagerException( @@ -740,14 +749,22 @@ class Manager(object): if 'alias' in config: source_zone = config['alias'] + # look up the zone in both our processed zones (including + # dynamic) and the original config as it may not be in + # processed b/c of active_zones filtering. Here we're only + # concerned with the config validation. We'll check and error + # handle the case where it's not active later in when the time + # to actually copy the data comes + zone = zones.get(source_zone, config_zones.get(source_zone)) + # Check that the source zone is defined. - if source_zone not in self.config['zones']: + if not zone: msg = f'Invalid alias zone {decoded_zone_name}: source zone {idna_decode(source_zone)} does not exist' self.log.error(msg) raise ManagerException(msg) # Check that the source zone is not an alias zone itself. - if 'alias' in self.config['zones'][source_zone]: + if 'alias' in zone: msg = f'Invalid alias zone {decoded_zone_name}: source zone {idna_decode(source_zone)} is an alias zone' self.log.error(msg) raise ManagerException(msg) @@ -843,8 +860,8 @@ class Manager(object): # Populate aliases zones. futures = [] for zone_name, zone_source in aliased_zones.items(): - source_config = self.config['zones'][zone_source] try: + source_config = self.zones[zone_source] desired_config = desired[zone_source] except KeyError: raise ManagerException( @@ -915,7 +932,7 @@ class Manager(object): total_changes = 0 self.log.debug('sync: applying') - zones = self.config['zones'] + zones = self.zones for target, plan in plans: zone_name = plan.existing.decoded_name if zones[zone_name].get('always-dry-run', False): @@ -1045,7 +1062,7 @@ class Manager(object): source_zone = config.get('alias') if source_zone: - if source_zone not in self.config['zones']: + if source_zone not in self.zones: self.log.exception('Invalid alias zone') raise ManagerException( f'Invalid alias zone {decoded_zone_name}: ' @@ -1053,7 +1070,7 @@ class Manager(object): 'not exist' ) - if 'alias' in self.config['zones'][source_zone]: + if 'alias' in self.zones[source_zone]: self.log.exception('Invalid alias zone') raise ManagerException( f'Invalid alias zone {decoded_zone_name}: ' @@ -1109,7 +1126,7 @@ class Manager(object): f'Invalid zone name {idna_decode(zone_name)}, missing ending dot' ) - zone = self.config['zones'].get(zone_name) + zone = self.zones.get(zone_name) if zone is not None: sub_zones = self.configured_sub_zones(zone_name) update_pcent_threshold = zone.get("update_pcent_threshold", None) diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 614b8aa..0fe7248 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -1380,22 +1380,27 @@ class TestManager(TestCase): self.assertIsNone(zone_with_defaults.update_pcent_threshold) self.assertIsNone(zone_with_defaults.delete_pcent_threshold) - def test_preprocess_zones_original(self): + @patch('octodns.manager.Manager._get_sources') + def test_preprocess_zones_original(self, get_sources_mock): # these will be unused environ['YAML_TMP_DIR'] = '/tmp' environ['YAML_TMP_DIR2'] = '/tmp' manager = Manager(get_config_filename('simple.yaml')) - # nothing returns nothing mock_source = MagicMock() - got = manager._preprocess_zones({}, sources=[mock_source]) + + # nothing returns nothing + get_sources_mock.return_value = [mock_source] + mock_source.reset_mock() + got = manager._preprocess_zones({}) self.assertEqual({}, got) mock_source.list_zones.assert_not_called() # non-dynamic returns as-is, no calls to sources + get_sources_mock.return_value = [mock_source] mock_source.reset_mock() zones = {'unit.tests.': {}} - got = manager._preprocess_zones(zones, sources=[mock_source]) + got = manager._preprocess_zones(zones) self.assertEqual(zones, got) mock_source.list_zones.assert_not_called() @@ -1404,10 +1409,10 @@ class TestManager(TestCase): id = 'simple-source' # dynamic with a source that doesn't support it - mock_source.reset_mock() + get_sources_mock.return_value = [SimpleSource()] zones = {'*': {}} with self.assertRaises(ManagerException) as ctx: - manager._preprocess_zones(zones, sources=[SimpleSource()]) + manager._preprocess_zones(zones) self.assertEqual( 'dynamic zone=* includes a source, simple-source, that does not support `list_zones`', str(ctx.exception), @@ -1415,37 +1420,41 @@ class TestManager(TestCase): mock_source.list_zones.assert_not_called() # same, but w/a source supports it + get_sources_mock.return_value = [mock_source] mock_source.reset_mock() config = {'foo': 42} zones = {'*': config} mock_source.list_zones.return_value = ['one', 'two', 'three'] - got = manager._preprocess_zones(zones, sources=[mock_source]) + got = manager._preprocess_zones(zones) self.assertEqual({'one': config, 'two': config, 'three': config}, got) mock_source.list_zones.assert_called_once() # same, but one of the zones is expliticly configured, so left alone + get_sources_mock.return_value = [mock_source] mock_source.reset_mock() config = {'foo': 42} zones = {'*': config, 'two': {'bar': 43}} mock_source.list_zones.return_value = ['one', 'two', 'three'] - got = manager._preprocess_zones(zones, sources=[mock_source]) + got = manager._preprocess_zones(zones) self.assertEqual( {'one': config, 'two': {'bar': 43}, 'three': config}, got ) mock_source.list_zones.assert_called_once() # doesn't matter what the actual name is, just that it starts with a *, + get_sources_mock.return_value = [mock_source] mock_source.reset_mock() config = {'foo': 42} zones = {'*SDFLKJSDFL': config, 'two': {'bar': 43}} mock_source.list_zones.return_value = ['one', 'two', 'three'] - got = manager._preprocess_zones(zones, sources=[mock_source]) + got = manager._preprocess_zones(zones) self.assertEqual( {'one': config, 'two': {'bar': 43}, 'three': config}, got ) mock_source.list_zones.assert_called_once() - def test_preprocess_zones_multiple_single_source(self): + @patch('octodns.manager.Manager._get_sources') + def test_preprocess_zones_multiple_single_source(self, get_sources_mock): # these will be unused environ['YAML_TMP_DIR'] = '/tmp' environ['YAML_TMP_DIR2'] = '/tmp' @@ -1463,7 +1472,8 @@ class TestManager(TestCase): mock_source.list_zones.side_effect = [ ['one.a.com.', 'two.a.com.', 'one.b.com.', 'two.b.com.'] ] - got = manager._preprocess_zones(zones, sources=[]) + get_sources_mock.return_value = [] + got = manager._preprocess_zones(zones) # each zone will have it's sources looked up self.assertEqual(2, manager._get_sources.call_count) # but there's only one source so it's zones will be cached @@ -1480,7 +1490,8 @@ class TestManager(TestCase): got, ) - def test_preprocess_zones_multiple_seperate_sources(self): + @patch('octodns.manager.Manager._get_sources') + def test_preprocess_zones_multiple_seperate_sources(self, get_sources_mock): # these will be unused environ['YAML_TMP_DIR'] = '/tmp' environ['YAML_TMP_DIR2'] = '/tmp' @@ -1499,7 +1510,8 @@ class TestManager(TestCase): zones = {'*.a.com.': config_a, '*.b.com.': config_b} mock_source_a.list_zones.side_effect = [['one.a.com.', 'two.a.com.']] mock_source_b.list_zones.side_effect = [['one.b.com.', 'two.b.com.']] - got = manager._preprocess_zones(zones, sources=[]) + get_sources_mock.return_value = [] + got = manager._preprocess_zones(zones) # each zone will have it's sources looked up self.assertEqual(2, manager._get_sources.call_count) # so each mock will be called once @@ -1554,7 +1566,7 @@ class TestManager(TestCase): # matched by c, catch all 'ignored.com.', ] - got = manager._preprocess_zones(zones, sources=[]) + got = manager._preprocess_zones(zones) # 4 configs self.assertEqual(4, manager._get_sources.call_count) # 1 shared source @@ -1578,7 +1590,7 @@ class TestManager(TestCase): '*.a.com.': config_a, '*.b.com.': config_b, } - got = manager._preprocess_zones(zones, sources=[]) + got = manager._preprocess_zones(zones) self.assertEqual( { 'one.a.com.': config_c, @@ -1590,7 +1602,8 @@ class TestManager(TestCase): got, ) - def test_preprocess_zones_regex(self): + @patch('octodns.manager.Manager._get_sources') + def test_preprocess_zones_regex(self, get_sources_mock): # these will be unused environ['YAML_TMP_DIR'] = '/tmp' environ['YAML_TMP_DIR2'] = '/tmp' @@ -1616,7 +1629,8 @@ class TestManager(TestCase): 'ignored.com.', ] ] - got = manager._preprocess_zones(zones, sources=[]) + get_sources_mock.return_value = [] + got = manager._preprocess_zones(zones) self.assertEqual(2, manager._get_sources.call_count) self.assertEqual(1, mock_source.list_zones.call_count) # a will regex match .a.com., b will .b.com., ignored.com. won't match @@ -1631,7 +1645,8 @@ class TestManager(TestCase): got, ) - def test_preprocess_zones_regex_claimed(self): + @patch('octodns.manager.Manager._get_sources') + def test_preprocess_zones_regex_claimed(self, get_sources_mock): # these will be unused environ['YAML_TMP_DIR'] = '/tmp' environ['YAML_TMP_DIR2'] = '/tmp' @@ -1664,7 +1679,8 @@ class TestManager(TestCase): 'ignored.com.', ] ] - got = manager._preprocess_zones(zones, sources=[]) + get_sources_mock.return_value = [] + got = manager._preprocess_zones(zones) self.assertEqual(2, manager._get_sources.call_count) self.assertEqual(1, mock_source.list_zones.call_count) # a will regex match .a.com., b will .b.com., ignored.com. won't match