Merge pull request #917 from octodns/subzones

Fix issues with sub-zone handling

octodns/manager.py

@@ -9,6 +9,7 @@ from __future__ import (
     unicode_literals,
 )
 
+from collections import deque
 from concurrent.futures import ThreadPoolExecutor
 from importlib import import_module
 from os import environ
@@ -102,6 +103,8 @@ class Manager(object):
         plan = p[1]
         return len(plan.changes[0].record.zone.name) if plan.changes else 0
 
+    # TODO: all of this should get broken up, mainly so that it's not so huge
+    # and each bit can be cleanly tested independently
     def __init__(self, config_file, max_workers=None, include_meta=False):
         version = self._try_version('octodns', version=__VERSION__)
         self.log.info(
@@ -185,25 +188,6 @@ class Manager(object):
                     'Incorrect processor config for ' + processor_name
                 )
 
-        zone_tree = {}
-        # Sort so we iterate on the deepest nodes first, ensuring if a parent
-        # zone exists it will be seen after the subzone, thus we can easily
-        # reparent children to their parent zone from the tree root.
-        for name in sorted(
-            self.config['zones'].keys(), key=lambda s: 0 - s.count('.')
-        ):
-            # Trim the trailing dot from FQDN
-            name = name[:-1]
-            this = {}
-            for sz in [k for k in zone_tree.keys() if k.endswith(name)]:
-                # Found a zone in tree root that is our child, slice the
-                # name and move its tree under ours.
-                this[sz[: -(len(name) + 1)]] = zone_tree.pop(sz)
-            # Add to tree root where it will be reparented as we iterate up
-            # the tree.
-            zone_tree[name] = this
-        self.zone_tree = zone_tree
-
         self.plan_outputs = {}
         plan_outputs = manager_config.get(
             'plan_outputs',
@@ -244,6 +228,8 @@ class Manager(object):
                     'Incorrect plan_output config for ' + plan_output_name
                 )
 
+        self._configured_sub_zones = None
+
     def _try_version(self, module_name, module=None, version=None):
         try:
             # Always try and use the official lookup first
@@ -314,23 +300,36 @@ class Manager(object):
         return kwargs
 
     def configured_sub_zones(self, zone_name):
-        name = zone_name[:-1]
+        if self._configured_sub_zones is None:
-        where = self.zone_tree
+            # First time through we compute all the sub-zones
-        while True:
+
-            # Find parent if it exists
+            configured_sub_zones = {}
-            parent = next((k for k in where if name.endswith(k)), None)
+
-            if not parent:
+            # Get a list of all of our zone names. Sort them from shortest to
-                # The zone_name in the tree has been reached, stop searching.
+            # longest so that parents will always come before their subzones
-                break
+            zones = sorted(
-            # Move down the tree and slice name to get the remainder for the
+                self.config['zones'].keys(), key=lambda z: len(z), reverse=True
-            # next round of the search.
+            )
-            where = where[parent]
+            zones = deque(zones)
-            name = name[: -(len(parent) + 1)]
+            # Until we're done processing zones
-        # `where` is now pointing at the dictionary of children for zone_name
+            while zones:
-        # in the tree
+                # Grab the one we'lre going to work on now
-        sub_zone_names = where.keys()
+                zone = zones.pop()
-        self.log.debug('configured_sub_zones: subs=%s', sub_zone_names)
+                dotted = f'.{zone}'
-        return set(sub_zone_names)
+                trimmer = len(dotted)
+                subs = set()
+                # look at all the zone names that come after it
+                for candidate in zones:
+                    # If they end with this zone's dotted name, it's a sub
+                    if candidate.endswith(dotted):
+                        # We want subs to exclude the zone portion
+                        subs.add(candidate[:-trimmer])
+
+                configured_sub_zones[zone] = subs
+
+            self._configured_sub_zones = configured_sub_zones
+
+        return self._configured_sub_zones.get(zone_name, set())
 
     def _populate_and_plan(
         self,
@@ -708,6 +707,7 @@ class Manager(object):
                 clz = SplitYamlProvider
             target = clz('dump', output_dir)
 
+        # TODO: use get_zone???
         zone = Zone(zone, self.configured_sub_zones(zone))
         for source in sources:
             source.populate(zone, lenient=lenient)

octodns/zone.py

@@ -73,19 +73,23 @@ class Zone(object):
 
         name = record.name
 
-        if not lenient and any((name.endswith(sz) for sz in self.sub_zones)):
+        if not lenient:
-            if name not in self.sub_zones:
+            if name in self.sub_zones:
-                # it's a record for something under a sub-zone
+                # It's an exact match for a sub-zone
-                raise SubzoneRecordException(
+                if not record._type == 'NS':
-                    f'Record {record.fqdn} is under ' 'a managed subzone'
+                    # and not a NS record, this should be in the sub
-                )
+                    raise SubzoneRecordException(
-            elif record._type != 'NS':
+                        f'Record {record.fqdn} is a managed sub-zone and not of type NS'
-                # It's a non NS record for exactly a sub-zone
+                    )
-                raise SubzoneRecordException(
+            else:
-                    f'Record {record.fqdn} a '
+                # It's not an exact match so there has to be a `.` before the
-                    'managed sub-zone and not of '
+                # sub-zone for it to belong in there
-                    'type NS'
+                for sub_zone in self.sub_zones:
-                )
+                    if name.endswith(f'.{sub_zone}'):
+                        # this should be in a sub
+                        raise SubzoneRecordException(
+                            f'Record {record.fqdn} is under a managed subzone'
+                        )
 
         if replace:
             # will remove it if it exists

tests/test_octodns_manager.py

@@ -712,6 +712,125 @@ class TestManager(TestCase):
             ),
         )
 
+    def test_subzone_handling(self):
+        manager = Manager(get_config_filename('simple.yaml'))
+
+        # tree with multiple branches, one that skips
+        manager.config['zones'] = {
+            'unit.tests.': {},
+            'sub.unit.tests.': {},
+            'another.sub.unit.tests.': {},
+            'skipped.alevel.unit.tests.': {},
+        }
+
+        self.assertEqual(
+            {'another.sub', 'sub', 'skipped.alevel'},
+            manager.configured_sub_zones('unit.tests.'),
+        )
+        self.assertEqual(
+            {'another'}, manager.configured_sub_zones('sub.unit.tests.')
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('another.sub.unit.tests.')
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('skipped.alevel.unit.tests.')
+        )
+
+        # unknown zone names return empty set
+        self.assertEqual(set(), manager.configured_sub_zones('unknown.tests.'))
+
+        # two parallel trees, make sure they don't interfere
+        manager.config['zones'] = {
+            'unit.tests.': {},
+            'unit2.tests.': {},
+            'sub.unit.tests.': {},
+            'sub.unit2.tests.': {},
+            'another.sub.unit.tests.': {},
+            'another.sub.unit2.tests.': {},
+            'skipped.alevel.unit.tests.': {},
+            'skipped.alevel.unit2.tests.': {},
+        }
+        manager._configured_sub_zones = None
+        self.assertEqual(
+            {'another.sub', 'sub', 'skipped.alevel'},
+            manager.configured_sub_zones('unit.tests.'),
+        )
+        self.assertEqual(
+            {'another'}, manager.configured_sub_zones('sub.unit.tests.')
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('another.sub.unit.tests.')
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('skipped.alevel.unit.tests.')
+        )
+        self.assertEqual(
+            {'another.sub', 'sub', 'skipped.alevel'},
+            manager.configured_sub_zones('unit2.tests.'),
+        )
+        self.assertEqual(
+            {'another'}, manager.configured_sub_zones('sub.unit2.tests.')
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('another.sub.unit2.tests.')
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('skipped.alevel.unit2.tests.')
+        )
+
+        # zones that end with names of others
+        manager.config['zones'] = {
+            'unit.tests.': {},
+            'uunit.tests.': {},
+            'uuunit.tests.': {},
+        }
+        manager._configured_sub_zones = None
+        self.assertEqual(set(), manager.configured_sub_zones('unit.tests.'))
+        self.assertEqual(set(), manager.configured_sub_zones('uunit.tests.'))
+        self.assertEqual(set(), manager.configured_sub_zones('uuunit.tests.'))
+
+        # skipping multiple levels
+        manager.config['zones'] = {
+            'unit.tests.': {},
+            'foo.bar.baz.unit.tests.': {},
+        }
+        manager._configured_sub_zones = None
+        self.assertEqual(
+            {'foo.bar.baz'}, manager.configured_sub_zones('unit.tests.')
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('foo.bar.baz.unit.tests.')
+        )
+
+        # different TLDs
+        manager.config['zones'] = {
+            'unit.tests.': {},
+            'foo.unit.tests.': {},
+            'unit.org.': {},
+            'bar.unit.org.': {},
+        }
+        manager._configured_sub_zones = None
+        self.assertEqual({'foo'}, manager.configured_sub_zones('unit.tests.'))
+        self.assertEqual(set(), manager.configured_sub_zones('foo.unit.tests.'))
+        self.assertEqual({'bar'}, manager.configured_sub_zones('unit.org.'))
+        self.assertEqual(set(), manager.configured_sub_zones('bar.unit.org.'))
+
+        # starting a beyond 2 levels
+        manager.config['zones'] = {
+            'foo.unit.tests.': {},
+            'bar.foo.unit.tests.': {},
+            'bleep.bloop.foo.unit.tests.': {},
+        }
+        manager._configured_sub_zones = None
+        self.assertEqual(
+            {'bar', 'bleep.bloop'},
+            manager.configured_sub_zones('foo.unit.tests.'),
+        )
+        self.assertEqual(
+            set(), manager.configured_sub_zones('bar.foo.unit.tests.')
+        )
+
 
 class TestMainThreadExecutor(TestCase):
     def test_success(self):

tests/test_octodns_zone.py

@@ -208,6 +208,16 @@ class TestZone(TestCase):
         zone.add_record(record, lenient=True)
         self.assertEqual(set([record]), zone.records)
 
+        # A that happens to end with a string that matches a sub (no .) is OK
+        zone = Zone('unit.tests.', set(['sub', 'barred']))
+        record = Record.new(
+            zone,
+            'foo.bar_sub',
+            {'ttl': 3600, 'type': 'A', 'values': ['1.2.3.4', '2.3.4.5']},
+        )
+        zone.add_record(record)
+        self.assertEqual(1, len(zone.records))
+
     def test_ignored_records(self):
         zone_normal = Zone('unit.tests.', [])
         zone_ignored = Zone('unit.tests.', [])