From e299ceced245af72b6195954556022be42813478 Mon Sep 17 00:00:00 2001 From: Sachi King Date: Thu, 23 Jun 2022 18:40:01 +1000 Subject: [PATCH 1/4] Prepare tests with failing "managed subzone" error The is not a zone between delegated.subzone.unit.tests. and unit.tests., however we get a delegated subzone error. This modifies the tests to succeed with the added record, however the tests fail as it incorrectly throws the managed subzone error. Change the name of delegated.subzone, and the tests will pass cleanly. --- tests/config/simple.yaml | 5 +++++ tests/config/sub.txt.unit.tests.yaml | 1 + tests/config/unit.tests.yaml | 5 +++++ tests/test_octodns_manager.py | 14 +++++++------- tests/test_octodns_provider_yaml.py | 9 +++++---- 5 files changed, 23 insertions(+), 11 deletions(-) create mode 100644 tests/config/sub.txt.unit.tests.yaml diff --git a/tests/config/simple.yaml b/tests/config/simple.yaml index fc4ad9f..4cd19c8 100644 --- a/tests/config/simple.yaml +++ b/tests/config/simple.yaml @@ -33,6 +33,11 @@ zones: targets: - dump - dump2 + sub.txt.unit.tests.: + sources: + - in + targets: + - dump empty.: sources: - in diff --git a/tests/config/sub.txt.unit.tests.yaml b/tests/config/sub.txt.unit.tests.yaml new file mode 100644 index 0000000..ed97d53 --- /dev/null +++ b/tests/config/sub.txt.unit.tests.yaml @@ -0,0 +1 @@ +--- diff --git a/tests/config/unit.tests.yaml b/tests/config/unit.tests.yaml index aa28ee5..b88ff82 100644 --- a/tests/config/unit.tests.yaml +++ b/tests/config/unit.tests.yaml @@ -162,6 +162,11 @@ sub: values: - 6.2.3.4. - 7.2.3.4. +sub.txt: + type: 'NS' + values: + - ns1.test. + - ns2.test. txt: ttl: 600 type: TXT diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 1c01af9..64aab9a 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -121,12 +121,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')) \ @@ -141,18 +141,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(32, tc) def test_eligible_sources(self): with TemporaryDirectory() as tmpdir: @@ -220,13 +220,13 @@ class TestManager(TestCase): # 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)) + self.assertEqual(22, len(changes)) # Compound sources with varying support changes = manager.compare(['in', 'nosshfp'], ['dump'], 'unit.tests.') - self.assertEqual(20, len(changes)) + self.assertEqual(21, len(changes)) with self.assertRaises(ManagerException) as ctx: manager.compare(['nope'], ['dump'], 'unit.tests.') diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py index 51e55eb..05fe90b 100644 --- a/tests/test_octodns_provider_yaml.py +++ b/tests/test_octodns_provider_yaml.py @@ -34,7 +34,7 @@ class TestYamlProvider(TestCase): # without it we see everything source.populate(zone) - self.assertEqual(23, len(zone.records)) + self.assertEqual(24, len(zone.records)) source.populate(dynamic_zone) self.assertEqual(6, len(dynamic_zone.records)) @@ -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 @@ -90,7 +90,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: @@ -111,6 +111,7 @@ class TestYamlProvider(TestCase): self.assertTrue('values' in data.pop('txt')) self.assertTrue('values' in data.pop('loc')) self.assertTrue('values' in data.pop('urlfwd')) + self.assertTrue('values' in data.pop('sub.txt')) # these are stored as singular 'value' self.assertTrue('value' in data.pop('_imap._tcp')) self.assertTrue('value' in data.pop('_pop3._tcp')) From 5592f5da96a297f6563c82ea35d74f790f6dedd3 Mon Sep 17 00:00:00 2001 From: Sachi King Date: Thu, 23 Jun 2022 20:31:49 +1000 Subject: [PATCH 2/4] Support dotted subdomains for subzones Currently if there are two zones configured; - example.com. - delegated.subdomain.example.com. When an NS record is created in example.com.yaml as such: delegated.subdomain: type: NS values: - ns1.example.org. The NS record for delegated.subdomain.example.com cannot be created as it throws an exception: octodns.zone.SubzoneRecordException: Record delegated.subdomain.example.com is under a managed subzone Additionally, all records other than NS are rejected for subdomain.example.com.. This is caused by zone_tree being the result of all zones split on '.' and being added to the tree, even if a zone does not exist at that point. To support records where a subzone is dotted, the the map is built such that each node represents the subdomain of the closest subzone. Before: {"com", {"example": {"subdomain": {"delegated": {}}}}} After: {"example.com": {"delegated.subdomain": {}}} Fixes: #378 --- octodns/manager.py | 50 ++++++++++++++++++---------------------------- octodns/zone.py | 6 +++--- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/octodns/manager.py b/octodns/manager.py index e640a8d..27b384d 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -162,23 +162,18 @@ class Manager(object): processor_name) zone_tree = {} - # sort by reversed strings so that parent zones always come first - for name in sorted(self.config['zones'].keys(), key=lambda s: s[::-1]): - # ignore trailing dots, and reverse - pieces = name[:-1].split('.')[::-1] - # where starts out at the top - where = zone_tree - # for all the pieces - for piece in pieces: - try: - where = where[piece] - # our current piece already exists, just point where at - # it's value - except KeyError: - # our current piece doesn't exist, create it - where[piece] = {} - # and then point where at it's newly created value - where = where[piece] + # 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('.')): + name = name[:-1] + this = {} + for sz in filter( + lambda k: k.endswith(name), set(zone_tree.keys()) + ): + this[sz[:-(len(name) + 1)]] = zone_tree.pop(sz) + zone_tree[name] = this self.zone_tree = zone_tree self.plan_outputs = {} @@ -274,21 +269,14 @@ class Manager(object): return kwargs def configured_sub_zones(self, zone_name): - # Reversed pieces of the zone name - pieces = zone_name[:-1].split('.')[::-1] - # Point where at the root of the tree + name = zone_name[:-1] where = self.zone_tree - # Until we've hit the bottom of this zone - try: - while pieces: - # Point where at the value of our current piece - where = where[pieces.pop(0)] - except KeyError: - self.log.debug('configured_sub_zones: unknown zone, %s, no subs', - zone_name) - return set() - # We're not pointed at the dict for our name, the keys of which will be - # any subzones + while True: + parent = next(filter(lambda k: name.endswith(k), where), None) + if not parent: + break + where = where[parent] + name = name[:-(len(parent) + 1)] sub_zone_names = where.keys() self.log.debug('configured_sub_zones: subs=%s', sub_zone_names) return set(sub_zone_names) diff --git a/octodns/zone.py b/octodns/zone.py index 4cd5e91..41cd6ec 100644 --- a/octodns/zone.py +++ b/octodns/zone.py @@ -68,10 +68,10 @@ class Zone(object): self.hydrate() name = record.name - last = name.split('.')[-1] - if not lenient and last in self.sub_zones: - if name != last: + if not lenient and any(map(lambda sz: name.endswith(sz), + self.sub_zones)): + if name not in self.sub_zones: # it's a record for something under a sub-zone raise SubzoneRecordException(f'Record {record.fqdn} is under ' 'a managed subzone') From d5363e8045f1f810d187e50df8ea6f274de1f249 Mon Sep 17 00:00:00 2001 From: Sachi King Date: Mon, 27 Jun 2022 10:50:05 +1000 Subject: [PATCH 3/4] Add comments and use list comprehensions Per PR review, use list comprehensions as they are prefered in py3 over use of filter. Add comments to describe the building of the zone tree. --- octodns/manager.py | 17 +++++++++++++---- octodns/zone.py | 3 +-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/octodns/manager.py b/octodns/manager.py index 27b384d..030298e 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -167,12 +167,15 @@ class Manager(object): # 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 filter( - lambda k: k.endswith(name), set(zone_tree.keys()) - ): + 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 @@ -272,11 +275,17 @@ class Manager(object): name = zone_name[:-1] where = self.zone_tree while True: - parent = next(filter(lambda k: name.endswith(k), where), None) + # Find parent if it exists + parent = next((k for k in where if name.endswith(k)), None) if not parent: + # The zone_name in the tree has been reached, stop searching. break + # Move down the tree and slice name to get the remainder for the + # next round of the search. where = where[parent] name = name[:-(len(parent) + 1)] + # `where` is now pointing at the dictionary of children for zone_name + # in the tree sub_zone_names = where.keys() self.log.debug('configured_sub_zones: subs=%s', sub_zone_names) return set(sub_zone_names) diff --git a/octodns/zone.py b/octodns/zone.py index 41cd6ec..7e42fa3 100644 --- a/octodns/zone.py +++ b/octodns/zone.py @@ -69,8 +69,7 @@ class Zone(object): name = record.name - if not lenient and any(map(lambda sz: name.endswith(sz), - self.sub_zones)): + if not lenient and any((name.endswith(sz) for sz in self.sub_zones)): if name not in self.sub_zones: # it's a record for something under a sub-zone raise SubzoneRecordException(f'Record {record.fqdn} is under ' From 04be906c3cb53711b61b41521117477c4c17dfe9 Mon Sep 17 00:00:00 2001 From: Sachi King Date: Mon, 27 Jun 2022 10:56:20 +1000 Subject: [PATCH 4/4] Add test to validate non-dotted subdomain zones are vaild This confirms that in addition to the recently added support for dotted subdomains that subdomains that are not dotted are supported. From RFC1034 Section 3.5 this would be a that contains a single