From 93a00f12a9e0c34eeb6f6d5e8e032f11fa6d1d48 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 28 Nov 2023 15:42:53 -0800 Subject: [PATCH 1/4] Add Processor.process_source_and_target_zones --- CHANGELOG.md | 2 ++ octodns/processor/base.py | 29 +++++++++++++++++++++- octodns/provider/base.py | 5 ++++ tests/test_octodns_manager.py | 46 ++++++++++++++++++++++++++++++++++- 4 files changed, 80 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc3fef1..1d32bd6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ ## v1.?.0 - 2023-??-?? - * Record.lenient property added similar to other common/standard _octodns data +* Processor.process_source_and_target_zones added to support modifying both the + desired and/or existing zones just prior to computing changes. ## v1.3.0 - 2023-11-14 - New and improved processors diff --git a/octodns/processor/base.py b/octodns/processor/base.py index f0890e0..eb584d5 100644 --- a/octodns/processor/base.py +++ b/octodns/processor/base.py @@ -37,7 +37,7 @@ class BaseProcessor(object): computed between `existing` and `desired`. This provides an opportunity to modify the `existing` `Zone`. - - Will see `existing` after any modifrications done by processors + - Will see `existing` after any modifications done by processors configured to run before this one. - May modify `existing` directly. - Must return `existing` which will normally be the `existing` param. @@ -48,6 +48,33 @@ class BaseProcessor(object): ''' return existing + def process_source_and_target_zones(self, desired, existing, target): + ''' + Called just prior to computing changes for `target` between `desired` + and `existing`. Provides an opportunity for the processor to modify + either the desired or existing `Zone`s that will be used to compute the + changes and create the initial plan. + + - Will see `desired` after any modifications done by + `Provider._process_desired_zone` and all processors via + `Processor.process_source_zone` + - Will see `existing` after any modifications done by all processors + via `Processor.process_target_zone` + - Will see both `desired` and `existing` after any modifications done by + any processors configured to run before this one via + `Processor.process_source_and_target_zones`. + - May modify `desired` directly. + - Must return `desired` which will normally be the `desired` param. + - 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 `desired`. + - May call `Zone.remove_record` to remove records from `existing`. + ''' + return desired, existing + def process_plan(self, plan, sources, target): ''' Called after the planning phase has completed. Provides an opportunity diff --git a/octodns/provider/base.py b/octodns/provider/base.py index e1e2234..9696c57 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -243,6 +243,11 @@ class BaseProvider(BaseSource): for processor in processors: existing = processor.process_target_zone(existing, target=self) + for processor in processors: + desired, existing = processor.process_source_and_target_zones( + desired, existing, self + ) + # compute the changes at the zone/record level changes = existing.changes(desired, self) diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index 2bec87e..72f0ec5 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -25,7 +25,7 @@ from octodns.manager import ( _AggregateTarget, ) from octodns.processor.base import BaseProcessor -from octodns.record import Create, Delete, Record +from octodns.record import Create, Delete, Record, Update from octodns.yaml import safe_load from octodns.zone import Zone @@ -723,6 +723,50 @@ class TestManager(TestCase): # We got a delete for the thing added to the existing state (target) self.assertIsInstance(plans[0][1].changes[0], Delete) + # source & target + record2 = Record.new( + zone, 'a2', {'ttl': 31, 'type': 'A', 'value': '1.2.3.4'} + ) + record3 = Record.new( + zone, 'a3', {'ttl': 32, 'type': 'A', 'value': '1.2.3.4'} + ) + + class MockProcessor(BaseProcessor): + def process_source_and_target_zones( + self, desired, existing, target + ): + # add something to desired + desired.add_record(record2) + # add something to existing + existing.add_record(record3) + # add something to both, but with a modification + desired.add_record(record) + mod = record.copy() + mod.ttl += 1 + existing.add_record(mod) + return desired, existing + + mock = MockProcessor('mock') + plans, zone = manager._populate_and_plan( + 'unit.tests.', [mock], [], targets + ) + # we should see a plan + self.assertTrue(plans) + plan = plans[0][1] + # it shoudl have a create, an update, and a delete + self.assertEqual( + 'a', + next(c.record.name for c in plan.changes if isinstance(c, Update)), + ) + self.assertEqual( + 'a2', + next(c.record.name for c in plan.changes if isinstance(c, Create)), + ) + self.assertEqual( + 'a3', + next(c.record.name for c in plan.changes if isinstance(c, Delete)), + ) + # muck with plans class MockProcessor(BaseProcessor): def process_target_zone(self, zone, target): From e612e2b88518fb1f444b4a380362c50d85501e87 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 28 Nov 2023 16:00:23 -0800 Subject: [PATCH 2/4] Fix MetaProcessor/Manager.include_meta where include_provider --- CHANGELOG.md | 2 ++ octodns/processor/meta.py | 6 +++--- tests/test_octodns_processor_meta.py | 16 +++++++++++----- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d32bd6..efc77c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ * Record.lenient property added similar to other common/standard _octodns data * Processor.process_source_and_target_zones added to support modifying both the desired and/or existing zones just prior to computing changes. +* Fix an issue in MetaProcessor/Manager.include_meta where include_provider + wasn't correctly taking effect ## v1.3.0 - 2023-11-14 - New and improved processors diff --git a/octodns/processor/meta.py b/octodns/processor/meta.py index 0389388..06ce614 100644 --- a/octodns/processor/meta.py +++ b/octodns/processor/meta.py @@ -111,13 +111,13 @@ class MetaProcessor(BaseProcessor): self.record_name, {'ttl': self.ttl, 'type': 'TXT', 'values': self.values}, # we may be passing in empty values here to be filled out later in - # process_target_zone + # process_source_and_target_zones lenient=True, ) desired.add_record(meta) return desired - def process_target_zone(self, existing, target): + def process_source_and_target_zones(self, desired, existing, target): if self.include_provider: # look for the meta record for record in sorted(existing.records): @@ -129,7 +129,7 @@ class MetaProcessor(BaseProcessor): existing.add_record(record, replace=True) break - return existing + return desired, existing def _up_to_date(self, change): # existing state, if there is one diff --git a/tests/test_octodns_processor_meta.py b/tests/test_octodns_processor_meta.py index c5af501..c5b5f78 100644 --- a/tests/test_octodns_processor_meta.py +++ b/tests/test_octodns_processor_meta.py @@ -124,12 +124,12 @@ class TestMetaProcessor(TestCase): self.assertEqual(self.meta_up_to_date, record) self.assertEqual(['time=the-time'], record.values) - def test_process_target_zone(self): + def test_process_source_and_target_zones(self): proc = MetaProcessor('test') # with defaults, not enabled zone = self.zone.copy() - processed = proc.process_target_zone(zone, None) + processed, _ = proc.process_source_and_target_zones(zone, zone, None) self.assertFalse(processed.records) # enable provider @@ -140,7 +140,9 @@ class TestMetaProcessor(TestCase): # enabled provider, no meta record, shouldn't happen, but also shouldn't # blow up - processed = proc.process_target_zone(zone, DummyTarget()) + processed, _ = proc.process_source_and_target_zones( + zone, zone, DummyTarget() + ) self.assertFalse(processed.records) # enabled provider, should now look for and update the provider value, @@ -150,7 +152,9 @@ class TestMetaProcessor(TestCase): zone = self.zone.copy() meta = self.meta_up_to_date.copy() zone.add_record(meta) - processed = proc.process_target_zone(zone, DummyTarget()) + processed, _ = proc.process_source_and_target_zones( + zone, zone, DummyTarget() + ) record = next(iter(processed.records)) self.assertEqual(['provider=dummy', 'time=xxx'], record.values) @@ -160,7 +164,9 @@ class TestMetaProcessor(TestCase): meta = self.meta_up_to_date.copy() zone.add_record(meta) zone.add_record(self.not_meta) - processed = proc.process_target_zone(zone, DummyTarget()) + processed, _ = proc.process_source_and_target_zones( + zone, zone, DummyTarget() + ) self.assertEqual(2, len(processed.records)) record = [r for r in processed.records if r.name == proc.record_name][0] self.assertEqual(['provider=dummy', 'time=xxx'], record.values) From 03f37e3ae9e6be54ba885289a4258563373fc919 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 28 Nov 2023 16:27:02 -0800 Subject: [PATCH 3/4] Fix bug with Record.copy when values is an empty list [] --- CHANGELOG.md | 1 + octodns/record/base.py | 5 ++++- tests/test_octodns_record.py | 18 ++++++++++++++---- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efc77c5..b0a738c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ desired and/or existing zones just prior to computing changes. * Fix an issue in MetaProcessor/Manager.include_meta where include_provider wasn't correctly taking effect +* Fix bug with Record.copy when values is an empty list [] ## v1.3.0 - 2023-11-14 - New and improved processors diff --git a/octodns/record/base.py b/octodns/record/base.py index 6f83578..d203cea 100644 --- a/octodns/record/base.py +++ b/octodns/record/base.py @@ -296,7 +296,10 @@ class ValuesMixin(object): try: values = data['values'] except KeyError: - values = [data['value']] + try: + values = [data['value']] + except KeyError: + values = [] self.values = sorted(self._value_type.process(values)) def changes(self, other, target): diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 66f0b9f..0fe3d57 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -338,6 +338,17 @@ class TestRecord(TestCase): del b._octodns['key']['second'] self.assertNotEqual(a.data, b.data) + def test_record_copy_with_no_values(self): + txt = Record.new( + self.zone, + 'txt', + {'ttl': 45, 'type': 'TXT', 'values': []}, + lenient=True, + ) + + dup = txt.copy() + self.assertEqual(txt.values, dup.values) + def test_change(self): existing = Record.new( self.zone, 'txt', {'ttl': 44, 'type': 'TXT', 'value': 'some text'} @@ -631,10 +642,9 @@ class TestRecordValidation(TestCase): lenient=True, ) - # __init__ may still blow up, even if validation is lenient - with self.assertRaises(KeyError) as ctx: - Record.new(self.zone, 'www', {'type': 'A', 'ttl': -1}, lenient=True) - self.assertEqual(('value',), ctx.exception.args) + # empty values is allowed with lenient + r = Record.new(self.zone, 'www', {'type': 'A', 'ttl': -1}, lenient=True) + self.assertEqual([], r.values) # no exception if we're in lenient mode from config Record.new( From 5b6b53bca0d824cc9b9034cc6a523cef3baa5061 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 28 Nov 2023 16:28:22 -0800 Subject: [PATCH 4/4] modify desired, not existing --- octodns/processor/meta.py | 4 ++-- tests/test_octodns_processor_meta.py | 25 +++++++++++++++---------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/octodns/processor/meta.py b/octodns/processor/meta.py index 06ce614..4c71724 100644 --- a/octodns/processor/meta.py +++ b/octodns/processor/meta.py @@ -120,13 +120,13 @@ class MetaProcessor(BaseProcessor): def process_source_and_target_zones(self, desired, existing, target): if self.include_provider: # look for the meta record - for record in sorted(existing.records): + for record in sorted(desired.records): if record.name == self.record_name and record._type == 'TXT': # we've found it, make a copy we can modify record = record.copy() record.values = record.values + [f'provider={target.id}'] record.values.sort() - existing.add_record(record, replace=True) + desired.add_record(record, replace=True) break return desired, existing diff --git a/tests/test_octodns_processor_meta.py b/tests/test_octodns_processor_meta.py index c5b5f78..b99dd19 100644 --- a/tests/test_octodns_processor_meta.py +++ b/tests/test_octodns_processor_meta.py @@ -128,8 +128,11 @@ class TestMetaProcessor(TestCase): proc = MetaProcessor('test') # with defaults, not enabled - zone = self.zone.copy() - processed, _ = proc.process_source_and_target_zones(zone, zone, None) + existing = self.zone.copy() + desired = self.zone.copy() + processed, _ = proc.process_source_and_target_zones( + existing, desired, None + ) self.assertFalse(processed.records) # enable provider @@ -141,7 +144,7 @@ class TestMetaProcessor(TestCase): # enabled provider, no meta record, shouldn't happen, but also shouldn't # blow up processed, _ = proc.process_source_and_target_zones( - zone, zone, DummyTarget() + existing, desired, DummyTarget() ) self.assertFalse(processed.records) @@ -149,23 +152,25 @@ class TestMetaProcessor(TestCase): # - only record so nothing to skip over # - time value in there to be skipped over proc = MetaProcessor('test', include_provider=True) - zone = self.zone.copy() + existing = self.zone.copy() + desired = self.zone.copy() meta = self.meta_up_to_date.copy() - zone.add_record(meta) + existing.add_record(meta) processed, _ = proc.process_source_and_target_zones( - zone, zone, DummyTarget() + existing, desired, DummyTarget() ) record = next(iter(processed.records)) self.assertEqual(['provider=dummy', 'time=xxx'], record.values) # add another unrelated record that needs to be skipped proc = MetaProcessor('test', include_provider=True) - zone = self.zone.copy() + existing = self.zone.copy() + desired = self.zone.copy() meta = self.meta_up_to_date.copy() - zone.add_record(meta) - zone.add_record(self.not_meta) + existing.add_record(meta) + existing.add_record(self.not_meta) processed, _ = proc.process_source_and_target_zones( - zone, zone, DummyTarget() + existing, desired, DummyTarget() ) self.assertEqual(2, len(processed.records)) record = [r for r in processed.records if r.name == proc.record_name][0]