diff --git a/CHANGELOG.md b/CHANGELOG.md index 673537c..3a7df78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## v1.?.0 - 2023-??-?? - +* Fix for bug in MetaProcessor _up_to_date check that was failing when there was + a plan with a single change type with a single value, e.g. CNAME. * Support added for config env variable expansion on nested levels, not just top-level provider/processor keys diff --git a/octodns/processor/meta.py b/octodns/processor/meta.py index 4c71724..64a53f5 100644 --- a/octodns/processor/meta.py +++ b/octodns/processor/meta.py @@ -62,11 +62,11 @@ class MetaProcessor(BaseProcessor): ''' @classmethod - def now(cls): + def get_time(cls): return datetime.now(UTC).isoformat() @classmethod - def uuid(cls): + def get_uuid(cls): return str(uuid4()) def __init__( @@ -91,61 +91,58 @@ class MetaProcessor(BaseProcessor): ttl, ) self.record_name = record_name - values = [] - if include_time: - time = self.now() - values.append(f'time={time}') - if include_uuid: - uuid = self.uuid() if include_uuid else None - values.append(f'uuid={uuid}') - if include_version: - values.append(f'octodns-version={__version__}') + self.time = self.get_time() if include_time else None + self.uuid = self.get_uuid() if include_uuid else None + self.include_version = include_version self.include_provider = include_provider - values.sort() - self.values = values self.ttl = ttl - def process_source_zone(self, desired, sources): + def values(self, target_id): + ret = [] + if self.include_version: + ret.append(f'octodns-version={__version__}') + if self.include_provider: + ret.append(f'provider={target_id}') + if self.time: + ret.append(f'time={self.time}') + if self.uuid: + ret.append(f'uuid={self.uuid}') + return ret + + def process_source_and_target_zones(self, desired, existing, target): meta = Record.new( desired, self.record_name, - {'ttl': self.ttl, 'type': 'TXT', 'values': self.values}, + {'ttl': self.ttl, 'type': 'TXT', 'values': self.values(target.id)}, # we may be passing in empty values here to be filled out later in # process_source_and_target_zones lenient=True, ) desired.add_record(meta) - return desired - - def process_source_and_target_zones(self, desired, existing, target): - if self.include_provider: - # look for the meta record - 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() - desired.add_record(record, replace=True) - break - return desired, existing - def _up_to_date(self, change): + def _is_up_to_date_meta(self, change, target_id): + # always something so we can see if its type and name + record = change.record # existing state, if there is one existing = getattr(change, 'existing', None) - return existing is not None and _keys(existing.values) == _keys( - self.values + return ( + record._type == 'TXT' + and record.name == self.record_name + and existing is not None + # don't care about the values here, just the fields/keys + and _keys(self.values(target_id)) == _keys(existing.values) ) def process_plan(self, plan, sources, target): if ( plan and len(plan.changes) == 1 - and self._up_to_date(plan.changes[0]) + and self._is_up_to_date_meta(plan.changes[0], target.id) ): # the only change is the meta record, and it's not meaningfully - # changing so we don't actually want to make the change + # changing so we don't actually want to make the update, meta should + # only be enough to cause a plan on its own if the fields changed return None # There's more than one thing changing so meta should update and/or meta diff --git a/tests/test_octodns_processor_meta.py b/tests/test_octodns_processor_meta.py index b99dd19..1bbc9fd 100644 --- a/tests/test_octodns_processor_meta.py +++ b/tests/test_octodns_processor_meta.py @@ -12,6 +12,13 @@ from octodns.record import Create, Record, Update from octodns.zone import Zone +class DummyTarget: + id = 'dummy' + + +dummy_target = DummyTarget() + + class TestMetaProcessor(TestCase): zone = Zone('unit.tests.', []) @@ -48,35 +55,43 @@ class TestMetaProcessor(TestCase): }, ) - @patch('octodns.processor.meta.MetaProcessor.now') - @patch('octodns.processor.meta.MetaProcessor.uuid') - def test_args_and_values(self, uuid_mock, now_mock): + not_txt = Record.new( + zone, + 'cname', + {'type': 'CNAME', 'ttl': 61, 'value': 'points.to.something.'}, + ) + + @patch('octodns.processor.meta.MetaProcessor.get_time') + @patch('octodns.processor.meta.MetaProcessor.get_uuid') + def test_args_and_values(self, get_uuid_mock, get_time_mock): # defaults, just time - uuid_mock.side_effect = [Exception('not used')] - now_mock.side_effect = ['the-time'] + get_uuid_mock.side_effect = [Exception('not used')] + get_time_mock.side_effect = ['the-time'] proc = MetaProcessor('test') - self.assertEqual(['time=the-time'], proc.values) + self.assertEqual(['time=the-time'], proc.values('dummy')) # just uuid - uuid_mock.side_effect = ['abcdef-1234567890'] - now_mock.side_effect = [Exception('not used')] + get_uuid_mock.side_effect = ['abcdef-1234567890'] + get_time_mock.side_effect = [Exception('not used')] proc = MetaProcessor('test', include_time=False, include_uuid=True) - self.assertEqual(['uuid=abcdef-1234567890'], proc.values) + self.assertEqual(['uuid=abcdef-1234567890'], proc.values('dummy')) # just version - uuid_mock.side_effect = [Exception('not used')] - now_mock.side_effect = [Exception('not used')] + get_uuid_mock.side_effect = [Exception('not used')] + get_time_mock.side_effect = [Exception('not used')] proc = MetaProcessor('test', include_time=False, include_version=True) - self.assertEqual([f'octodns-version={__version__}'], proc.values) + self.assertEqual( + [f'octodns-version={__version__}'], proc.values('dummy') + ) # just provider proc = MetaProcessor('test', include_time=False, include_provider=True) self.assertTrue(proc.include_provider) - self.assertFalse(proc.values) + self.assertEqual(['provider=dummy'], proc.values('dummy')) # everything - uuid_mock.side_effect = ['abcdef-1234567890'] - now_mock.side_effect = ['the-time'] + get_uuid_mock.side_effect = ['abcdef-1234567890'] + get_time_mock.side_effect = ['the-time'] proc = MetaProcessor( 'test', include_time=True, @@ -87,94 +102,110 @@ class TestMetaProcessor(TestCase): self.assertEqual( [ f'octodns-version={__version__}', + 'provider=dummy-x', 'time=the-time', 'uuid=abcdef-1234567890', ], - proc.values, + list(proc.values('dummy-x')), ) self.assertTrue(proc.include_provider) def test_uuid(self): proc = MetaProcessor('test', include_time=False, include_uuid=True) - self.assertEqual(1, len(proc.values)) - self.assertTrue(proc.values[0].startswith('uuid')) - # uuid's have 4 - - self.assertEqual(4, proc.values[0].count('-')) + self.assertTrue(proc.uuid) + self.assertFalse(proc.time) + self.assertFalse(proc.include_provider) + self.assertFalse(proc.include_version) - def test_up_to_date(self): + values = list(proc.values('dummy')) + self.assertEqual(1, len(values)) + value = values[0] + self.assertEqual(f'uuid={proc.uuid}', value) + + def test_is_up_to_date_meta(self): proc = MetaProcessor('test') # Creates always need to happen - self.assertFalse(proc._up_to_date(Create(self.meta_needs_update))) - self.assertFalse(proc._up_to_date(Create(self.meta_up_to_date))) + self.assertFalse( + proc._is_up_to_date_meta(Create(self.meta_needs_update), 'dummy') + ) + self.assertFalse( + proc._is_up_to_date_meta(Create(self.meta_up_to_date), 'dummy') + ) # Updates depend on the contents - self.assertFalse(proc._up_to_date(Update(self.meta_needs_update, None))) - self.assertTrue(proc._up_to_date(Update(self.meta_up_to_date, None))) + self.assertFalse( + proc._is_up_to_date_meta( + Update(self.meta_needs_update, None), 'dummy' + ) + ) + self.assertTrue( + proc._is_up_to_date_meta( + Update(self.meta_up_to_date, None), 'dummy' + ) + ) + + # not a meta txt + self.assertFalse( + proc._is_up_to_date_meta(Update(self.not_meta, None), 'dummy') + ) + + # not even a txt record + self.assertFalse( + proc._is_up_to_date_meta(Update(self.not_txt, None), 'dummy') + ) + + @patch('octodns.processor.meta.MetaProcessor.get_time') + def test_process_source_and_target_zones(self, get_time_mock): + get_time_mock.side_effect = [ + 'the-time', + 'the-time-2', + 'the-time-3', + 'the-time-4', + ] - @patch('octodns.processor.meta.MetaProcessor.now') - def test_process_source_zone(self, now_mock): - now_mock.side_effect = ['the-time'] proc = MetaProcessor('test') + self.assertFalse(proc.uuid) + self.assertTrue(proc.time) + self.assertFalse(proc.include_provider) + self.assertFalse(proc.include_version) - # meta record was added + existing = self.zone.copy() desired = self.zone.copy() - processed = proc.process_source_zone(desired, None) + processed, _ = proc.process_source_and_target_zones( + desired, existing, dummy_target + ) record = next(iter(processed.records)) self.assertEqual(self.meta_up_to_date, record) self.assertEqual(['time=the-time'], record.values) - def test_process_source_and_target_zones(self): - proc = MetaProcessor('test') - # with defaults, not enabled existing = self.zone.copy() desired = self.zone.copy() processed, _ = proc.process_source_and_target_zones( - existing, desired, None + desired, existing, dummy_target ) - self.assertFalse(processed.records) + records = processed.records + self.assertEqual(1, len(records)) + record = next(iter(records)) + self.assertEqual(proc.record_name, record.name) + self.assertEqual('TXT', record._type) + self.assertEqual(['time=the-time'], record.values) # enable provider proc = MetaProcessor('test', include_provider=True) + self.assertFalse(proc.uuid) + self.assertTrue(proc.time) + self.assertTrue(proc.include_provider) + self.assertFalse(proc.include_version) - class DummyTarget: - id = 'dummy' - - # enabled provider, no meta record, shouldn't happen, but also shouldn't - # blow up - processed, _ = proc.process_source_and_target_zones( - existing, desired, DummyTarget() - ) - self.assertFalse(processed.records) - - # enabled provider, should now look for and update the provider value, - # - only record so nothing to skip over - # - time value in there to be skipped over - proc = MetaProcessor('test', include_provider=True) existing = self.zone.copy() desired = self.zone.copy() - meta = self.meta_up_to_date.copy() - existing.add_record(meta) processed, _ = proc.process_source_and_target_zones( - existing, desired, DummyTarget() + existing, desired, dummy_target ) 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) - existing = self.zone.copy() - desired = self.zone.copy() - meta = self.meta_up_to_date.copy() - existing.add_record(meta) - existing.add_record(self.not_meta) - processed, _ = proc.process_source_and_target_zones( - existing, desired, 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) + self.assertEqual(['provider=dummy', 'time=the-time-2'], record.values) def test_process_plan(self): proc = MetaProcessor('test') @@ -182,23 +213,24 @@ class TestMetaProcessor(TestCase): # no plan, shouldn't happen, but we shouldn't blow up self.assertFalse(proc.process_plan(None, None, None)) - # plan with just an up to date meta record, should kill off the plan + # plan with only a meta record that has the correct config/keys plan = Plan( - None, - None, - [Update(self.meta_up_to_date, self.meta_needs_update)], + None, # ignored + None, # ignored + [Update(self.meta_up_to_date, self.meta_up_to_date)], True, ) - self.assertFalse(proc.process_plan(plan, None, None)) + self.assertFalse(proc.process_plan(plan, [], dummy_target)) - # plan with an out of date meta record, should leave the plan alone + # plan with only a meta record that has the wrong config/keys and thus + # needs updating plan = Plan( None, None, [Update(self.meta_needs_update, self.meta_up_to_date)], True, ) - self.assertEqual(plan, proc.process_plan(plan, None, None)) + self.assertEqual(plan, proc.process_plan(plan, [], dummy_target)) # plan with other changes preserved even if meta was somehow up to date plan = Plan( @@ -210,4 +242,43 @@ class TestMetaProcessor(TestCase): ], True, ) - self.assertEqual(plan, proc.process_plan(plan, None, None)) + self.assertEqual(plan, proc.process_plan(plan, [], dummy_target)) + + def test_flow(self): + proc = MetaProcessor( + 'test', + record_name='special', + include_version=True, + include_provider=True, + include_time=False, + ) + + desired = self.zone.copy() + # start out with no records + self.assertFalse(desired.records) + + # now process source and target zones (existing isn't touched) + desired, _ = proc.process_source_and_target_zones( + desired, [], dummy_target + ) + records = desired.records + self.assertEqual(1, len(records)) + meta = next(iter(records)) + # has the expected type and name & type + self.assertEqual(proc.record_name, meta.name) + self.assertEqual('TXT', meta._type) + # at this point values will just have version, no provider yet b/c it + # wasn't known + self.assertEqual( + [f'octodns-version={__version__}', 'provider=dummy'], meta.values + ) + + # process the plan (Create) + plan = Plan(desired, self.zone, [Create(meta)], True) + got = proc.process_plan(plan, [], dummy_target) + self.assertTrue(got) + + # process the plan (Update w/no changes) + plan = Plan(desired, self.zone, [Update(meta, meta)], True) + got = proc.process_plan(plan, [], dummy_target) + self.assertFalse(got)