diff --git a/octodns/manager.py b/octodns/manager.py index f2a2da6..c7173c6 100644 --- a/octodns/manager.py +++ b/octodns/manager.py @@ -263,7 +263,7 @@ class Manager(object): except TypeError as e: if "keyword argument 'lenient'" not in text_type(e): raise - self.log.warn(': provider %s does not accept lenient ' + self.log.warn('provider %s does not accept lenient ' 'param', source.__class__.__name__) source.populate(zone) @@ -281,9 +281,15 @@ class Manager(object): 'value': 'provider={}'.format(target.id) }) zone.add_record(meta, replace=True) - # TODO: if someone has overrriden plan already this will be a - # breaking change so we probably need to try both ways - plan = target.plan(zone, processors=processors) + try: + plan = target.plan(zone, processors=processors) + except TypeError as e: + if "keyword argument 'processors'" not in text_type(e): + raise + self.log.warn('provider.plan %s does not accept processors ' + 'param', target.__class__.__name__) + plan = target.plan(zone) + for processor in processors: plan = processor.process_plan(plan, sources=sources, target=target) diff --git a/octodns/processor/__init__.py b/octodns/processor/__init__.py new file mode 100644 index 0000000..14ccf18 --- /dev/null +++ b/octodns/processor/__init__.py @@ -0,0 +1,6 @@ +# +# +# + +from __future__ import absolute_import, division, print_function, \ + unicode_literals diff --git a/octodns/processors/__init__.py b/octodns/processor/base.py similarity index 100% rename from octodns/processors/__init__.py rename to octodns/processor/base.py diff --git a/octodns/processors/filters.py b/octodns/processor/filters.py similarity index 100% rename from octodns/processors/filters.py rename to octodns/processor/filters.py diff --git a/octodns/processors/ownership.py b/octodns/processor/ownership.py similarity index 100% rename from octodns/processors/ownership.py rename to octodns/processor/ownership.py diff --git a/tests/config/processors-missing-class.yaml b/tests/config/processors-missing-class.yaml new file mode 100644 index 0000000..4594307 --- /dev/null +++ b/tests/config/processors-missing-class.yaml @@ -0,0 +1,23 @@ +providers: + config: + class: octodns.provider.yaml.YamlProvider + directory: tests/config + dump: + class: octodns.provider.yaml.YamlProvider + directory: env/YAML_TMP_DIR + geo: + class: helpers.GeoProvider + nosshfp: + class: helpers.NoSshFpProvider + +processors: + no-class: {} + +zones: + unit.tests.: + processors: + - noop + sources: + - in + targets: + - dump diff --git a/tests/config/processors-wants-config.yaml b/tests/config/processors-wants-config.yaml new file mode 100644 index 0000000..53fc397 --- /dev/null +++ b/tests/config/processors-wants-config.yaml @@ -0,0 +1,25 @@ +providers: + config: + class: octodns.provider.yaml.YamlProvider + directory: tests/config + dump: + class: octodns.provider.yaml.YamlProvider + directory: env/YAML_TMP_DIR + geo: + class: helpers.GeoProvider + nosshfp: + class: helpers.NoSshFpProvider + +processors: + # valid class, but it wants a param and we're not passing it + wants-config: + class: helpers.WantsConfigProcessor + +zones: + unit.tests.: + processors: + - noop + sources: + - in + targets: + - dump diff --git a/tests/config/processors.yaml b/tests/config/processors.yaml new file mode 100644 index 0000000..097024b --- /dev/null +++ b/tests/config/processors.yaml @@ -0,0 +1,33 @@ +providers: + config: + class: octodns.provider.yaml.YamlProvider + directory: tests/config + dump: + class: octodns.provider.yaml.YamlProvider + directory: env/YAML_TMP_DIR + geo: + class: helpers.GeoProvider + nosshfp: + class: helpers.NoSshFpProvider + +processors: + # Just testing config so any processor will do + noop: + class: octodns.processor.base.BaseProcessor + +zones: + unit.tests.: + processors: + - noop + sources: + - config + targets: + - dump + + bad.unit.tests.: + processors: + - doesnt-exist + sources: + - in + targets: + - dump diff --git a/tests/helpers.py b/tests/helpers.py index ff7f7cc..eedfd8b 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -7,6 +7,10 @@ from __future__ import absolute_import, division, print_function, \ from shutil import rmtree from tempfile import mkdtemp +from logging import getLogger + +from octodns.processor.base import BaseProcessor +from octodns.provider.base import BaseProvider class SimpleSource(object): @@ -90,3 +94,29 @@ class TemporaryDirectory(object): rmtree(self.dirname) else: raise Exception(self.dirname) + + +class WantsConfigProcessor(BaseProcessor): + + def __init__(self, name, some_config): + super(WantsConfigProcessor, self).__init__(name) + + +class PlannableProvider(BaseProvider): + log = getLogger('PlannableProvider') + + SUPPORTS_GEO = False + SUPPORTS_DYNAMIC = False + SUPPORTS = set(('A',)) + + def __init__(self, *args, **kwargs): + super(PlannableProvider, self).__init__(*args, **kwargs) + + def populate(self, zone, source=False, target=False, lenient=False): + pass + + def supports(self, record): + return True + + def __repr__(self): + return self.__class__.__name__ diff --git a/tests/test_octodns_manager.py b/tests/test_octodns_manager.py index e4b5211..069bc0b 100644 --- a/tests/test_octodns_manager.py +++ b/tests/test_octodns_manager.py @@ -9,9 +9,10 @@ from os import environ from os.path import dirname, join from six import text_type -from octodns.record import Record from octodns.manager import _AggregateTarget, MainThreadExecutor, Manager, \ ManagerException +from octodns.processor.base import BaseProcessor +from octodns.record import Create, Delete, Record from octodns.yaml import safe_load from octodns.zone import Zone @@ -19,7 +20,7 @@ from mock import MagicMock, patch from unittest import TestCase from helpers import DynamicProvider, GeoProvider, NoSshFpProvider, \ - SimpleProvider, TemporaryDirectory + PlannableProvider, SimpleProvider, TemporaryDirectory config_dir = join(dirname(__file__), 'config') @@ -358,20 +359,48 @@ class TestManager(TestCase): class NoLenient(SimpleProvider): - def populate(self, zone, source=False): + def populate(self, zone): pass # This should be ok, we'll fall back to not passing it manager._populate_and_plan('unit.tests.', [], [NoLenient()], []) - class NoZone(SimpleProvider): + class OtherType(SimpleProvider): - def populate(self, lenient=False): + def populate(self, zone, lenient=False): + raise TypeError('something else') + + # This will blow up, we don't fallback for source + with self.assertRaises(TypeError) as ctx: + manager._populate_and_plan('unit.tests.', [], [OtherType()], + []) + self.assertEquals('something else', text_type(ctx.exception)) + + def test_plan_processors_fallback(self): + with TemporaryDirectory() as tmpdir: + environ['YAML_TMP_DIR'] = tmpdir.dirname + # Only allow a target that doesn't exist + manager = Manager(get_config_filename('simple.yaml')) + + class NoProcessors(SimpleProvider): + + def plan(self, zone): pass + # This should be ok, we'll fall back to not passing it + manager._populate_and_plan('unit.tests.', [], [], + [NoProcessors()]) + + class OtherType(SimpleProvider): + + def plan(self, zone, processors): + raise TypeError('something else') + # This will blow up, we don't fallback for source - with self.assertRaises(TypeError): - manager._populate_and_plan('unit.tests.', [NoZone()], []) + with self.assertRaises(TypeError) as ctx: + manager._populate_and_plan('unit.tests.', [], [], + [OtherType()]) + self.assertEquals('something else', text_type(ctx.exception)) @patch('octodns.manager.Manager._get_named_class') def test_sync_passes_file_handle(self, mock): @@ -391,6 +420,92 @@ class TestManager(TestCase): _, kwargs = plan_output_mock.run.call_args self.assertEqual(fh_mock, kwargs.get('fh')) + def test_processor_config(self): + # Smoke test loading a valid config + manager = Manager(get_config_filename('processors.yaml')) + self.assertEquals(['noop'], list(manager.processors.keys())) + # This zone specifies a valid processor + manager.sync(['unit.tests.']) + + with self.assertRaises(ManagerException) as ctx: + # This zone specifies a non-existant processor + manager.sync(['bad.unit.tests.']) + self.assertTrue('Zone bad.unit.tests., unknown processor: ' + 'doesnt-exist' in text_type(ctx.exception)) + + with self.assertRaises(ManagerException) as ctx: + Manager(get_config_filename('processors-missing-class.yaml')) + self.assertTrue('Processor no-class is missing class' in + text_type(ctx.exception)) + + with self.assertRaises(ManagerException) as ctx: + Manager(get_config_filename('processors-wants-config.yaml')) + self.assertTrue('Incorrect processor config for wants-config' in + text_type(ctx.exception)) + + def test_processors(self): + manager = Manager(get_config_filename('simple.yaml')) + + targets = [PlannableProvider('prov')] + + zone = Zone('unit.tests.', []) + record = Record.new(zone, 'a', { + 'ttl': 30, + 'type': 'A', + 'value': '1.2.3.4', + }) + + # muck with sources + class MockProcessor(BaseProcessor): + + def process_source_zone(self, zone, sources): + zone = self._clone_zone(zone) + zone.add_record(record) + return zone + + mock = MockProcessor('mock') + plans, zone = manager._populate_and_plan('unit.tests.', [mock], [], + targets) + # Our mock was called and added the record + self.assertEquals(record, list(zone.records)[0]) + # We got a create for the thing added to the expected state (source) + self.assertIsInstance(plans[0][1].changes[0], Create) + + # muck with targets + class MockProcessor(BaseProcessor): + + def process_target_zone(self, zone, target): + zone = self._clone_zone(zone) + zone.add_record(record) + return zone + + mock = MockProcessor('mock') + plans, zone = manager._populate_and_plan('unit.tests.', [mock], [], + targets) + # No record added since it's target this time + self.assertFalse(zone.records) + # We got a delete for the thing added to the existing state (target) + self.assertIsInstance(plans[0][1].changes[0], Delete) + + # muck with plans + class MockProcessor(BaseProcessor): + + def process_target_zone(self, zone, target): + zone = self._clone_zone(zone) + zone.add_record(record) + return zone + + def process_plan(self, plans, sources, target): + # get rid of the change + plans.changes.pop(0) + + mock = MockProcessor('mock') + plans, zone = manager._populate_and_plan('unit.tests.', [mock], [], + targets) + # We planned a delete again, but this time removed it from the plan, so + # no plans + self.assertFalse(plans) + class TestMainThreadExecutor(TestCase): diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index f33db0f..4dfce48 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -9,9 +9,10 @@ from logging import getLogger from six import text_type from unittest import TestCase -from octodns.record import Create, Delete, Record, Update +from octodns.processor.base import BaseProcessor from octodns.provider.base import BaseProvider from octodns.provider.plan import Plan, UnsafePlan +from octodns.record import Create, Delete, Record, Update from octodns.zone import Zone @@ -21,7 +22,7 @@ class HelperProvider(BaseProvider): SUPPORTS = set(('A',)) id = 'test' - def __init__(self, extra_changes, apply_disabled=False, + def __init__(self, extra_changes=[], apply_disabled=False, include_change_callback=None): self.__extra_changes = extra_changes self.apply_disabled = apply_disabled @@ -43,6 +44,29 @@ class HelperProvider(BaseProvider): pass +class TrickyProcessor(BaseProcessor): + + def __init__(self, name, add_during_process_target_zone): + super(TrickyProcessor, self).__init__(name) + self.add_during_process_target_zone = add_during_process_target_zone + self.reset() + + def reset(self): + self.existing = None + self.target = None + + def process_target_zone(self, existing, target): + self.existing = existing + self.target = target + + new = self._clone_zone(existing) + for record in existing.records: + new.add_record(record) + for record in self.add_during_process_target_zone: + new.add_record(record) + return new + + class TestBaseProvider(TestCase): def test_base_provider(self): @@ -138,6 +162,45 @@ class TestBaseProvider(TestCase): self.assertTrue(plan) self.assertEquals(1, len(plan.changes)) + def test_plan_with_processors(self): + zone = Zone('unit.tests.', []) + + record = Record.new(zone, 'a', { + 'ttl': 30, + 'type': 'A', + 'value': '1.2.3.4', + }) + provider = HelperProvider() + # Processor that adds a record to the zone, which planning will then + # delete since it won't know anything about it + tricky = TrickyProcessor('tricky', [record]) + plan = provider.plan(zone, processors=[tricky]) + self.assertTrue(plan) + self.assertEquals(1, len(plan.changes)) + self.assertIsInstance(plan.changes[0], Delete) + # Called processor stored its params + self.assertTrue(tricky.existing) + self.assertEquals(zone.name, tricky.existing.name) + + # Chain of processors happen one after the other + other = Record.new(zone, 'b', { + 'ttl': 30, + 'type': 'A', + 'value': '5.6.7.8', + }) + # Another processor will add its record, thus 2 deletes + another = TrickyProcessor('tricky', [other]) + plan = provider.plan(zone, processors=[tricky, another]) + self.assertTrue(plan) + self.assertEquals(2, len(plan.changes)) + self.assertIsInstance(plan.changes[0], Delete) + self.assertIsInstance(plan.changes[1], Delete) + # 2nd processor stored its params, and we'll see the record the + # first one added + self.assertTrue(another.existing) + self.assertEquals(zone.name, another.existing.name) + self.assertEquals(1, len(another.existing.records)) + def test_apply(self): ignored = Zone('unit.tests.', [])