From b84b933eb0b4612c66ba3847d8b481584277280f Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 21 Aug 2021 13:41:10 -0700 Subject: [PATCH] Copy zones early on and allow modifications after that. Doc requirements. --- octodns/processor/acme.py | 18 ++++---- octodns/processor/base.py | 54 ++++++++++++++++++++--- octodns/processor/filter.py | 10 ++--- octodns/processor/ownership.py | 11 +++-- octodns/provider/base.py | 27 ++++++------ octodns/provider/route53.py | 5 +-- tests/test_octodns_processor_filter.py | 16 +++---- tests/test_octodns_processor_ownership.py | 4 +- 8 files changed, 92 insertions(+), 53 deletions(-) diff --git a/octodns/processor/acme.py b/octodns/processor/acme.py index 5aa10ec..2d7c101 100644 --- a/octodns/processor/acme.py +++ b/octodns/processor/acme.py @@ -32,9 +32,8 @@ class AcmeMangingProcessor(BaseProcessor): self._owned = set() - def process_source_zone(self, zone, *args, **kwargs): - ret = zone.copy() - for record in zone.records: + def process_source_zone(self, desired, *args, **kwargs): + for record in desired.records: if record._type == 'TXT' and \ record.name.startswith('_acme-challenge'): # We have a managed acme challenge record (owned by octoDNS) so @@ -45,12 +44,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, replace=True) - return ret + desired.add_record(record, replace=True) + return desired - def process_target_zone(self, zone, *args, **kwargs): - ret = zone.copy() - for record in zone.records: + def process_target_zone(self, existing, *args, **kwargs): + for record in existing.records: # Uses a startswith rather than == to ignore subdomain challenges, # e.g. _acme-challenge.foo.domain.com when managing domain.com if record._type == 'TXT' and \ @@ -58,6 +56,6 @@ class AcmeMangingProcessor(BaseProcessor): '*octoDNS*' not in record.values and \ record not in self._owned: self.log.info('_process: ignoring %s', record.fqdn) - ret.remove_record(record) + existing.remove_record(record) - return ret + return existing diff --git a/octodns/processor/base.py b/octodns/processor/base.py index 7e8a63a..98f2baa 100644 --- a/octodns/processor/base.py +++ b/octodns/processor/base.py @@ -11,14 +11,58 @@ class BaseProcessor(object): def __init__(self, name): self.name = name - def process_source_zone(self, zone, sources): - # sources may be empty, as will be the case for aliased zones - return zone + def process_source_zone(self, desired, sources): + ''' + Called after all sources have completed populate. Provides an + opportunity for the processor to modify the desired `Zone` that targets + will recieve. - def process_target_zone(self, zone, target): - return zone + - Will see `desired` after any modifications done by + `Provider._process_desired_zone` and processors configured to run + before this one. + - May modify `desired` directly. + - Must return `desired` which will normally be the `desired` param. + - Must not modify records directly, `record.copy` should be called, + the results of which can be modified, and then `Zone.add_record` may + be used with `replace=True`. + - May call `Zone.remove_record` to remove records from `desired`. + - Sources may be empty, as will be the case for aliased zones. + ''' + return desired + + def process_target_zone(self, existing, target): + ''' + Called after a target has completed `populate`, before changes are + computed between `existing` and `desired`. This provides an opportunity + to modify the `existing` `Zone`. + + - Will see `existing` after any modifrications done by processors + configured to run before this one. + - May modify `existing` directly. + - Must return `existing` which will normally be the `existing` param. + - Must not modify records directly, `record.copy` should be called, + the results of which can be modified, and then `Zone.add_record` may + be used with `replace=True`. + - May call `Zone.remove_record` to remove records from `existing`. + ''' + return existing def process_plan(self, plan, sources, target): + ''' + Called after the planning phase has completed. Provides an opportunity + for the processors to modify the plan thus changing the actions that + will be displayed and potentially applied. + + - `plan` may be None if no changes were detected, if so a `Plan` may + still be created and returned. + - May modify `plan.changes` directly or create a new `Plan`. + - Does not have to modify `plan.desired` and/or `plan.existing` to line + up with any modifications made to `plan.changes`. + - Should copy over `plan.exists`, `plan.update_pcent_threshold`, and + `plan.delete_pcent_threshold` when creating a new `Plan`. + - Must return a `Plan` which may be `plan` or can be a newly created + one `plan.desired` and `plan.existing` copied over as-is or modified. + ''' # plan may be None if no changes were detected up until now, the # process may still create a plan. # sources may be empty, as will be the case for aliased zones diff --git a/octodns/processor/filter.py b/octodns/processor/filter.py index 456048b..d9b8ee3 100644 --- a/octodns/processor/filter.py +++ b/octodns/processor/filter.py @@ -15,12 +15,11 @@ class TypeAllowlistFilter(BaseProcessor): self.allowlist = set(allowlist) def _process(self, zone, *args, **kwargs): - ret = zone.copy() for record in zone.records: if record._type not in self.allowlist: - ret.remove_record(record) + zone.remove_record(record) - return ret + return zone process_source_zone = _process process_target_zone = _process @@ -33,12 +32,11 @@ class TypeRejectlistFilter(BaseProcessor): self.rejectlist = set(rejectlist) def _process(self, zone, *args, **kwargs): - ret = zone.copy() for record in zone.records: if record._type in self.rejectlist: - ret.remove_record(record) + zone.remove_record(record) - return ret + return zone process_source_zone = _process process_target_zone = _process diff --git a/octodns/processor/ownership.py b/octodns/processor/ownership.py index 8281dd6..42f041d 100644 --- a/octodns/processor/ownership.py +++ b/octodns/processor/ownership.py @@ -24,9 +24,8 @@ class OwnershipProcessor(BaseProcessor): self.txt_value = txt_value self._txt_values = [txt_value] - def process_source_zone(self, zone, *args, **kwargs): - ret = zone.copy() - for record in zone.records: + def process_source_zone(self, desired, *args, **kwargs): + for record in desired.records: # Then create and add an ownership TXT for each of them record_name = record.name.replace('*', '_wildcard') if record.name: @@ -34,14 +33,14 @@ class OwnershipProcessor(BaseProcessor): record_name) else: name = '{}.{}'.format(self.txt_name, record._type) - txt = Record.new(zone, name, { + txt = Record.new(desired, name, { 'type': 'TXT', 'ttl': 60, 'value': self.txt_value, }) - ret.add_record(txt) + desired.add_record(txt) - return ret + return desired def _is_ownership(self, record): return record._type == 'TXT' and \ diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 3d6c62e..c862e2d 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -38,25 +38,22 @@ class BaseProvider(BaseSource): planning. `desired` is a "shallow" copy, see `Zone.copy` for more information - - Must do their work and then call `super` with the results of that - work, returning the result of the `super` call. - - Must not modify `desired` directly, should call `desired.copy` and - modify the shallow copy returned from that. + - Must call `super` at an appropriate point for their work, generally + that means as the final step of the method, returning the result of + the `super` call. + - May modify `desired` directly. - Must not modify records directly, `record.copy` should be called, the results of which can be modified, and then `Zone.add_record` may - be used with `replace=True` - - Must call `Zone.remove_record` to remove records from the copy of - `desired` + be used with `replace=True`. + - May call `Zone.remove_record` to remove records from `desired`. - Must call supports_warn_or_except with information about any changes that are made to have them logged or throw errors depending on the - provider configuration + provider configuration. ''' if self.SUPPORTS_MUTLIVALUE_PTR: # nothing do here return desired - # Shallow copy - new_desired = desired.copy() for record in desired.records: if record._type == 'PTR' and len(record.values) > 1: # replace with a single-value copy @@ -67,9 +64,9 @@ class BaseProvider(BaseSource): self.supports_warn_or_except(msg, fallback) record = record.copy() record.values = [record.value] - new_desired.add_record(record, replace=True) + desired.add_record(record, replace=True) - return new_desired + return desired def _include_change(self, change): ''' @@ -94,7 +91,11 @@ class BaseProvider(BaseSource): def plan(self, desired, processors=[]): self.log.info('plan: desired=%s', desired.name) - # process desired zone for any custom zone/record modification + # Make a (shallow) copy of the desired state so that everything from + # now on (in this target) can modify it as they see fit without + # worrying about impacting other targets. + desired = desired.copy() + desired = self._process_desired_zone(desired) existing = Zone(desired.name, desired.sub_zones) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 37ce413..06f7d8c 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -925,7 +925,6 @@ class Route53Provider(BaseProvider): return data def _process_desired_zone(self, desired): - ret = desired.copy() for record in desired.records: if getattr(record, 'dynamic', False): # Make a copy of the record in case we have to muck with it @@ -958,9 +957,9 @@ class Route53Provider(BaseProvider): if rules != dynamic.rules: record = record.copy() record.dynamic.rules = rules - ret.add_record(record, replace=True) + desired.add_record(record, replace=True) - return super(Route53Provider, self)._process_desired_zone(ret) + return super(Route53Provider, self)._process_desired_zone(desired) def populate(self, zone, target=False, lenient=False): self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, diff --git a/tests/test_octodns_processor_filter.py b/tests/test_octodns_processor_filter.py index f421676..176f7d1 100644 --- a/tests/test_octodns_processor_filter.py +++ b/tests/test_octodns_processor_filter.py @@ -47,20 +47,20 @@ class TestTypeAllowListFilter(TestCase): def test_basics(self): filter_a = TypeAllowlistFilter('only-a', set(('A'))) - got = filter_a.process_source_zone(zone) + got = filter_a.process_source_zone(zone.copy()) self.assertEquals(['a', 'a2'], sorted([r.name for r in got.records])) filter_aaaa = TypeAllowlistFilter('only-aaaa', ('AAAA',)) - got = filter_aaaa.process_source_zone(zone) + got = filter_aaaa.process_source_zone(zone.copy()) self.assertEquals(['aaaa'], sorted([r.name for r in got.records])) filter_txt = TypeAllowlistFilter('only-txt', ['TXT']) - got = filter_txt.process_target_zone(zone) + got = filter_txt.process_target_zone(zone.copy()) self.assertEquals(['txt', 'txt2'], sorted([r.name for r in got.records])) filter_a_aaaa = TypeAllowlistFilter('only-aaaa', set(('A', 'AAAA'))) - got = filter_a_aaaa.process_target_zone(zone) + got = filter_a_aaaa.process_target_zone(zone.copy()) self.assertEquals(['a', 'a2', 'aaaa'], sorted([r.name for r in got.records])) @@ -70,21 +70,21 @@ class TestTypeRejectListFilter(TestCase): def test_basics(self): filter_a = TypeRejectlistFilter('not-a', set(('A'))) - got = filter_a.process_source_zone(zone) + got = filter_a.process_source_zone(zone.copy()) self.assertEquals(['aaaa', 'txt', 'txt2'], sorted([r.name for r in got.records])) filter_aaaa = TypeRejectlistFilter('not-aaaa', ('AAAA',)) - got = filter_aaaa.process_source_zone(zone) + got = filter_aaaa.process_source_zone(zone.copy()) self.assertEquals(['a', 'a2', 'txt', 'txt2'], sorted([r.name for r in got.records])) filter_txt = TypeRejectlistFilter('not-txt', ['TXT']) - got = filter_txt.process_target_zone(zone) + got = filter_txt.process_target_zone(zone.copy()) self.assertEquals(['a', 'a2', 'aaaa'], sorted([r.name for r in got.records])) filter_a_aaaa = TypeRejectlistFilter('not-a-aaaa', set(('A', 'AAAA'))) - got = filter_a_aaaa.process_target_zone(zone) + got = filter_a_aaaa.process_target_zone(zone.copy()) self.assertEquals(['txt', 'txt2'], sorted([r.name for r in got.records])) diff --git a/tests/test_octodns_processor_ownership.py b/tests/test_octodns_processor_ownership.py index 959f4c2..e6b248b 100644 --- a/tests/test_octodns_processor_ownership.py +++ b/tests/test_octodns_processor_ownership.py @@ -55,7 +55,7 @@ class TestOwnershipProcessor(TestCase): def test_process_source_zone(self): ownership = OwnershipProcessor('ownership') - got = ownership.process_source_zone(zone) + got = ownership.process_source_zone(zone.copy()) self.assertEquals([ '', '*', @@ -88,7 +88,7 @@ class TestOwnershipProcessor(TestCase): self.assertFalse(ownership.process_plan(None)) # Nothing exists create both records and ownership - ownership_added = ownership.process_source_zone(zone) + ownership_added = ownership.process_source_zone(zone.copy()) plan = provider.plan(ownership_added) self.assertTrue(plan) # Double the number of records