Stop relying on manager.config['zones'] being modified. _preprocess_zones mostly moved closer to where it's used

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)

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