From 8c04508a86739d434f2506ee9c46cac9548b2aa8 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 21 Aug 2021 10:11:23 -0700 Subject: [PATCH] Implement & test Zone.copy (shallow) and utilize it in processors --- octodns/processor/acme.py | 9 +++-- octodns/processor/base.py | 3 -- octodns/processor/filter.py | 12 +++---- octodns/processor/ownership.py | 4 +-- octodns/zone.py | 55 ++++++++++++++++++++++++++-- tests/test_octodns_manager.py | 6 ++-- tests/test_octodns_provider_base.py | 6 ++-- tests/test_octodns_zone.py | 56 +++++++++++++++++++++++++++++ 8 files changed, 126 insertions(+), 25 deletions(-) diff --git a/octodns/processor/acme.py b/octodns/processor/acme.py index 685130d..5aa10ec 100644 --- a/octodns/processor/acme.py +++ b/octodns/processor/acme.py @@ -33,7 +33,7 @@ class AcmeMangingProcessor(BaseProcessor): self._owned = set() def process_source_zone(self, zone, *args, **kwargs): - ret = self._clone_zone(zone) + ret = zone.copy() for record in zone.records: if record._type == 'TXT' and \ record.name.startswith('_acme-challenge'): @@ -45,11 +45,11 @@ class AcmeMangingProcessor(BaseProcessor): # This assumes we'll see things as sources before targets, # which is the case... self._owned.add(record) - ret.add_record(record) + ret.add_record(record, replace=True) return ret def process_target_zone(self, zone, *args, **kwargs): - ret = self._clone_zone(zone) + ret = zone.copy() for record in zone.records: # Uses a startswith rather than == to ignore subdomain challenges, # e.g. _acme-challenge.foo.domain.com when managing domain.com @@ -58,7 +58,6 @@ class AcmeMangingProcessor(BaseProcessor): '*octoDNS*' not in record.values and \ record not in self._owned: self.log.info('_process: ignoring %s', record.fqdn) - continue - ret.add_record(record) + ret.remove_record(record) return ret diff --git a/octodns/processor/base.py b/octodns/processor/base.py index 327b1c2..51f19dd 100644 --- a/octodns/processor/base.py +++ b/octodns/processor/base.py @@ -13,9 +13,6 @@ class BaseProcessor(object): def __init__(self, name): self.name = name - def _clone_zone(self, zone): - return Zone(zone.name, sub_zones=zone.sub_zones) - def process_source_zone(self, zone, sources): # sources may be empty, as will be the case for aliased zones return zone diff --git a/octodns/processor/filter.py b/octodns/processor/filter.py index 369e987..456048b 100644 --- a/octodns/processor/filter.py +++ b/octodns/processor/filter.py @@ -15,10 +15,10 @@ class TypeAllowlistFilter(BaseProcessor): self.allowlist = set(allowlist) def _process(self, zone, *args, **kwargs): - ret = self._clone_zone(zone) + ret = zone.copy() for record in zone.records: - if record._type in self.allowlist: - ret.add_record(record) + if record._type not in self.allowlist: + ret.remove_record(record) return ret @@ -33,10 +33,10 @@ class TypeRejectlistFilter(BaseProcessor): self.rejectlist = set(rejectlist) def _process(self, zone, *args, **kwargs): - ret = self._clone_zone(zone) + ret = zone.copy() for record in zone.records: - if record._type not in self.rejectlist: - ret.add_record(record) + if record._type in self.rejectlist: + ret.remove_record(record) return ret diff --git a/octodns/processor/ownership.py b/octodns/processor/ownership.py index 172f349..8281dd6 100644 --- a/octodns/processor/ownership.py +++ b/octodns/processor/ownership.py @@ -25,10 +25,8 @@ class OwnershipProcessor(BaseProcessor): self._txt_values = [txt_value] def process_source_zone(self, zone, *args, **kwargs): - ret = self._clone_zone(zone) + ret = zone.copy() for record in zone.records: - # Always copy over the source records - ret.add_record(record) # Then create and add an ownership TXT for each of them record_name = record.name.replace('*', '_wildcard') if record.name: diff --git a/octodns/zone.py b/octodns/zone.py index 5f099ac..dcc07c3 100644 --- a/octodns/zone.py +++ b/octodns/zone.py @@ -49,16 +49,26 @@ class Zone(object): # optional trailing . b/c some sources don't have it on their fqdn self._name_re = re.compile(r'\.?{}?$'.format(name)) + # Copy-on-write semantics support, when `not None` this property will + # point to a location with records for this `Zone`. Once `hydrated` + # this property will be set to None + self._origin = None + self.log.debug('__init__: zone=%s, sub_zones=%s', self, sub_zones) @property def records(self): + if self._origin: + return self._origin.records return set([r for _, node in self._records.items() for r in node]) def hostname_from_fqdn(self, fqdn): return self._name_re.sub('', fqdn) def add_record(self, record, replace=False, lenient=False): + if self._origin: + self.hydrate() + name = record.name last = name.split('.')[-1] @@ -94,10 +104,14 @@ class Zone(object): node.add(record) - def _remove_record(self, record): - 'Only for use in tests' + def remove_record(self, record): + if self._origin: + self.hydrate() self._records[record.name].discard(record) + # TODO: delete this + _remove_record = remove_record + def changes(self, desired, target): self.log.debug('changes: zone=%s, target=%s', self, target) @@ -184,5 +198,42 @@ class Zone(object): return changes + def hydrate(self): + ''' + Take a shallow copy Zone and make it a deeper copy holding its own + reference to records. These records will still be the originals and + they should not be modified. Changes should be made by calling + `add_record`, often with `replace=True`, and/or `remove_record`. + + Note: This method does not need to be called under normal circumstances + as `add_record` and `remove_record` will automatically call it when + appropriate. + ''' + origin = self._origin + if origin is None: + return False + # Need to clear this before the copy to prevent recursion + self._origin = None + for record in origin.records: + # Use lenient as we're copying origin and should take its records + # regardless + self.add_record(record, lenient=True) + return True + + def copy(self): + ''' + Copy-on-write semantics support. This method will create a shallow + clone of the zone which will be hydrated the first time `add_record` or + `remove_record` is called. + + This allows low-cost copies of things to be made in situations where + changes are unlikely and only incurs the "expense" of actually + copying the records when required. The actual record copy will not be + "deep" meaning that records should not be modified directly. + ''' + copy = Zone(self.name, self.sub_zones) + copy._origin = self + return copy + def __repr__(self): return 'Zone<{}>'.format(self.name) diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 96f67fd..c9362d4 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -464,7 +464,7 @@ class TestManager(TestCase): class MockProcessor(BaseProcessor): def process_source_zone(self, zone, sources): - zone = self._clone_zone(zone) + zone = zone.copy() zone.add_record(record) return zone @@ -480,7 +480,7 @@ class TestManager(TestCase): class MockProcessor(BaseProcessor): def process_target_zone(self, zone, target): - zone = self._clone_zone(zone) + zone = zone.copy() zone.add_record(record) return zone @@ -496,7 +496,7 @@ class TestManager(TestCase): class MockProcessor(BaseProcessor): def process_target_zone(self, zone, target): - zone = self._clone_zone(zone) + zone = zone.copy() zone.add_record(record) return zone diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 46afd89..501177e 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -61,11 +61,11 @@ class TrickyProcessor(BaseProcessor): self.existing = existing self.target = target - new = self._clone_zone(existing) + new = existing.copy() for record in existing.records: - new.add_record(record) + new.add_record(record, replace=True) for record in self.add_during_process_target_zone: - new.add_record(record) + new.add_record(record, replace=True) return new diff --git a/tests/test_octodns_zone.py b/tests/test_octodns_zone.py index 1d000f2..ddc2157 100644 --- a/tests/test_octodns_zone.py +++ b/tests/test_octodns_zone.py @@ -355,3 +355,59 @@ class TestZone(TestCase): self.assertTrue(zone_missing.changes(zone_normal, provider)) self.assertFalse(zone_missing.changes(zone_included, provider)) + + def assertEqualsNameAndValues(self, a, b): + a = dict([(r.name, r.values[0]) for r in a]) + b = dict([(r.name, r.values[0]) for r in b]) + self.assertEquals(a, b) + + def test_copy(self): + zone = Zone('unit.tests.', []) + + a = ARecord(zone, 'a', {'ttl': 42, 'value': '1.1.1.1'}) + zone.add_record(a) + b = ARecord(zone, 'b', {'ttl': 42, 'value': '1.1.1.2'}) + zone.add_record(b) + + # Sanity check + self.assertEqualsNameAndValues(set((a, b)), zone.records) + + copy = zone.copy() + # We have an origin set and it is the source/original zone + self.assertEquals(zone, copy._origin) + # Our records are zone's records to start (references) + self.assertEqualsNameAndValues(zone.records, copy.records) + + # If we try and change something that's already there we realize and + # then get an error about a duplicate + b_prime = ARecord(zone, 'b', {'ttl': 42, 'value': '1.1.1.3'}) + with self.assertRaises(DuplicateRecordException): + copy.add_record(b_prime) + self.assertIsNone(copy._origin) + # Unchanged, straight copies + self.assertEqualsNameAndValues(zone.records, copy.records) + + # If we add with replace things will be realized and the record will + # have changed + copy = zone.copy() + copy.add_record(b_prime, replace=True) + self.assertIsNone(copy._origin) + self.assertEqualsNameAndValues(set((a, b_prime)), copy.records) + + # If we add another record, things are reliazed and it has been added + copy = zone.copy() + c = ARecord(zone, 'c', {'ttl': 42, 'value': '1.1.1.3'}) + copy.add_record(c) + self.assertEqualsNameAndValues(set((a, b, c)), copy.records) + + # If we remove a record, things are reliazed and it has been removed + copy = zone.copy() + copy.remove_record(a) + self.assertEqualsNameAndValues(set((b,)), copy.records) + + # Re-realizing is a noop + copy = zone.copy() + # Happens the first time + self.assertTrue(copy.hydrate()) + # Doesn't the second + self.assertFalse(copy.hydrate())