diff --git a/octodns/provider/__init__.py b/octodns/provider/__init__.py index 14ccf18..dbbaaa8 100644 --- a/octodns/provider/__init__.py +++ b/octodns/provider/__init__.py @@ -4,3 +4,7 @@ from __future__ import absolute_import, division, print_function, \ unicode_literals + + +class ProviderException(Exception): + pass diff --git a/octodns/provider/base.py b/octodns/provider/base.py index c5a8dc9..16cad7a 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -10,13 +10,15 @@ from six import text_type from ..source.base import BaseSource from ..zone import Zone from .plan import Plan +from . import ProviderException class BaseProvider(BaseSource): def __init__(self, id, apply_disabled=False, update_pcent_threshold=Plan.MAX_SAFE_UPDATE_PCENT, - delete_pcent_threshold=Plan.MAX_SAFE_DELETE_PCENT): + delete_pcent_threshold=Plan.MAX_SAFE_DELETE_PCENT, + strict_supports=False): super(BaseProvider, self).__init__(id) self.log.debug('__init__: id=%s, apply_disabled=%s, ' 'update_pcent_threshold=%.2f, ' @@ -28,26 +30,19 @@ class BaseProvider(BaseSource): self.apply_disabled = apply_disabled self.update_pcent_threshold = update_pcent_threshold self.delete_pcent_threshold = delete_pcent_threshold - - def _include_change(self, change): - ''' - An opportunity for providers to filter out false positives due to - peculiarities in their implementation. E.g. minimum TTLs. - ''' - return True - - def _extra_changes(self, existing, desired, changes): - ''' - An opportunity for providers to add extra changes to the plan that are - necessary to update ancillary record data or configure the zone. E.g. - base NS records. - ''' - return [] + self.strict_supports = strict_supports def _process_desired_zone(self, desired): ''' - Providers can use this method to make any custom changes to the - desired zone. + An opportunity for providers to modify that desired zone records before + planning. + + - Must do their work and then call super with the results of that work + - Must not modify the `desired` parameter or its records and should + make a copy of anything it's modifying + - Must call supports_warn_or_except with information about any changes + that are made to have them logged or throw errors depending on the + configuration ''' if self.SUPPORTS_MUTLIVALUE_PTR: # nothing do here @@ -57,9 +52,11 @@ class BaseProvider(BaseSource): for record in desired.records: if record._type == 'PTR' and len(record.values) > 1: # replace with a single-value copy - self.log.warn('does not support multi-value PTR records; ' - 'will use only %s for %s', record.value, - record.fqdn) + msg = 'multi-value PTR records not supported for {}' \ + .format(record.fqdn) + fallback = 'falling back to single value, {}' \ + .format(record.value) + self.supports_warn_or_except(msg, fallback) record = record.copy() record.values = [record.value] @@ -67,9 +64,32 @@ class BaseProvider(BaseSource): return new_desired + def _include_change(self, change): + ''' + An opportunity for providers to filter out false positives due to + peculiarities in their implementation. E.g. minimum TTLs. + ''' + return True + + def _extra_changes(self, existing, desired, changes): + ''' + An opportunity for providers to add extra changes to the plan that are + necessary to update ancillary record data or configure the zone. E.g. + base NS records. + ''' + return [] + + def supports_warn_or_except(self, msg, fallback): + if self.strict_supports: + raise ProviderException('{}: {}'.format(self.id, msg)) + self.log.warning('{}; {}'.format(msg, fallback)) + def plan(self, desired, processors=[]): self.log.info('plan: desired=%s', desired.name) + # process desired zone for any custom zone/record modification + desired = self._process_desired_zone(desired) + existing = Zone(desired.name, desired.sub_zones) exists = self.populate(existing, target=True, lenient=True) if exists is None: @@ -81,9 +101,6 @@ class BaseProvider(BaseSource): for processor in processors: existing = processor.process_target_zone(existing, target=self) - # process desired zone for any custom zone/record modification - desired = self._process_desired_zone(desired) - # compute the changes at the zone/record level changes = existing.changes(desired, self) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index f1b3b40..477a53e 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -19,6 +19,7 @@ from six import text_type from ..equality import EqualityTupleMixin from ..record import Record, Update from ..record.geo import GeoCodes +from ..zone import Zone from .base import BaseProvider octal_re = re.compile(r'\\(\d\d\d)') @@ -924,6 +925,44 @@ class Route53Provider(BaseProvider): return data + def _process_desired_zone(self, desired): + ret = Zone(desired.name, desired.sub_zones) + for record in desired.records: + if getattr(record, 'dynamic', False): + # Make a copy of the record in case we have to muck with it + record = record.copy() + dynamic = record.dynamic + rules = [] + for i, rule in enumerate(dynamic.rules): + geos = rule.data.get('geos', []) + if not geos: + rules.append(rule) + continue + filtered_geos = [g for g in geos + if not g.startswith('NA-CA-')] + if not filtered_geos: + # We've removed all geos, we'll have to skip this rule + msg = 'NA-CA-* not supported for {}' \ + .format(record.fqdn) + fallback = 'skipping rule {}'.format(i) + self.supports_warn_or_except(msg, fallback) + continue + elif geos != filtered_geos: + msg = 'NA-CA-* not supported for {}' \ + .format(record.fqdn) + fallback = 'filtering rule {} from ({}) to ({})' \ + .format(i, ', '.join(geos), + ', '.join(filtered_geos)) + self.supports_warn_or_except(msg, fallback) + rule.data['geos'] = filtered_geos + rules.append(rule) + + dynamic.rules = rules + + ret.add_record(record) + + return super(Route53Provider, self)._process_desired_zone(ret) + def populate(self, zone, target=False, lenient=False): self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, target, lenient) diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 28abfd3..46afd89 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -6,11 +6,12 @@ from __future__ import absolute_import, division, print_function, \ unicode_literals from logging import getLogger +from mock import MagicMock, call from six import text_type from unittest import TestCase from octodns.processor.base import BaseProcessor -from octodns.provider.base import BaseProvider +from octodns.provider.base import BaseProvider, ProviderException from octodns.provider.plan import Plan, UnsafePlan from octodns.record import Create, Delete, Record, Update from octodns.zone import Zone @@ -21,6 +22,7 @@ class HelperProvider(BaseProvider): SUPPORTS = set(('A',)) id = 'test' + strict_supports = False def __init__(self, extra_changes=[], apply_disabled=False, include_change_callback=None): @@ -443,3 +445,27 @@ class TestBaseProvider(TestCase): delete_pcent_threshold=safe_pcent).raise_if_unsafe() self.assertTrue('Too many deletes' in text_type(ctx.exception)) + + def test_supports_warn_or_except(self): + class MinimalProvider(BaseProvider): + SUPPORTS = set() + SUPPORTS_GEO = False + + def __init__(self, **kwargs): + self.log = MagicMock() + super(MinimalProvider, self).__init__('minimal', **kwargs) + + normal = MinimalProvider(strict_supports=False) + # Should log and not expect + normal.supports_warn_or_except('Hello World!', 'Goodbye') + normal.log.warning.assert_called_once() + normal.log.warning.assert_has_calls([ + call('Hello World!; Goodbye') + ]) + + strict = MinimalProvider(strict_supports=True) + # Should log and not expect + with self.assertRaises(ProviderException) as ctx: + strict.supports_warn_or_except('Hello World!', 'Will not see') + self.assertEquals('minimal: Hello World!', text_type(ctx.exception)) + strict.log.warning.assert_not_called() diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 1bf3332..b3e5ba4 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -394,6 +394,139 @@ class TestRoute53Provider(TestCase): return (provider, stubber) + def test_process_desired_zone(self): + provider, stubber = self._get_stubbed_fallback_auth_provider() + + # No records, essentially a no-op + desired = Zone('unit.tests.', []) + got = provider._process_desired_zone(desired) + self.assertEquals(desired.records, got.records) + + # Record without any geos + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { + 'ttl': 30, + 'type': 'A', + 'value': '1.2.3.4', + 'dynamic': { + 'pools': { + 'one': { + 'values': [{ + 'value': '2.2.3.4', + }], + }, + }, + 'rules': [{ + 'pool': 'one', + }], + }, + }) + desired.add_record(record) + got = provider._process_desired_zone(desired) + self.assertEquals(desired.records, got.records) + self.assertEquals(1, len(list(got.records)[0].dynamic.rules)) + self.assertFalse('geos' in list(got.records)[0].dynamic.rules[0].data) + + # Record where all geos are supported + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { + 'ttl': 30, + 'type': 'A', + 'value': '1.2.3.4', + 'dynamic': { + 'pools': { + 'one': { + 'values': [{ + 'value': '1.2.3.4', + }], + }, + 'two': { + 'values': [{ + 'value': '2.2.3.4', + }], + }, + }, + 'rules': [{ + 'geos': ['EU', 'NA-US-OR'], + 'pool': 'two', + }, { + 'pool': 'one', + }], + }, + }) + desired.add_record(record) + got = provider._process_desired_zone(desired) + self.assertEquals(2, len(list(got.records)[0].dynamic.rules)) + self.assertEquals(['EU', 'NA-US-OR'], + list(got.records)[0].dynamic.rules[0].data['geos']) + self.assertFalse('geos' in list(got.records)[0].dynamic.rules[1].data) + + # Record with NA-CA-* only rule which is removed + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { + 'ttl': 30, + 'type': 'A', + 'value': '1.2.3.4', + 'dynamic': { + 'pools': { + 'one': { + 'values': [{ + 'value': '1.2.3.4', + }], + }, + 'two': { + 'values': [{ + 'value': '2.2.3.4', + }], + }, + }, + 'rules': [{ + 'geos': ['NA-CA-BC'], + 'pool': 'two', + }, { + 'pool': 'one', + }], + }, + }) + desired.add_record(record) + got = provider._process_desired_zone(desired) + self.assertEquals(1, len(list(got.records)[0].dynamic.rules)) + self.assertFalse('geos' in list(got.records)[0].dynamic.rules[0].data) + + # Record with NA-CA-* rule combined with other geos, filtered + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { + 'ttl': 30, + 'type': 'A', + 'value': '1.2.3.4', + 'dynamic': { + 'pools': { + 'one': { + 'values': [{ + 'value': '1.2.3.4', + }], + }, + 'two': { + 'values': [{ + 'value': '2.2.3.4', + }], + }, + }, + 'rules': [{ + 'geos': ['EU', 'NA-CA-NB', 'NA-US-OR'], + 'pool': 'two', + }, { + 'pool': 'one', + }], + }, + }) + desired.add_record(record) + got = provider._process_desired_zone(desired) + self.assertEquals(2, len(list(got.records)[0].dynamic.rules)) + self.assertEquals(['EU', 'NA-US-OR'], + list(got.records)[0].dynamic.rules[0].data['geos']) + self.assertFalse('geos' in list(got.records)[0].dynamic.rules[1].data) + def test_populate_with_fallback(self): provider, stubber = self._get_stubbed_fallback_auth_provider()