Merge pull request #757 from octodns/process-desired-zone

Provider.process_desired_zone with warning & exception support

octodns/provider/__init__.py

@@ -4,3 +4,7 @@
 
 from __future__ import absolute_import, division, print_function, \
     unicode_literals
+
+
+class ProviderException(Exception):
+    pass

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,6 +30,39 @@ class BaseProvider(BaseSource):
         self.apply_disabled = apply_disabled
         self.update_pcent_threshold = update_pcent_threshold
         self.delete_pcent_threshold = delete_pcent_threshold
+        self.strict_supports = strict_supports
+
+    def _process_desired_zone(self, desired):
+        '''
+        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
+            return desired
+
+        new_desired = Zone(desired.name, desired.sub_zones)
+        for record in desired.records:
+            if record._type == 'PTR' and len(record.values) > 1:
+                # replace with a single-value copy
+                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]
+
+            new_desired.add_record(record)
+
+        return new_desired
 
     def _include_change(self, change):
         '''
@@ -44,32 +79,17 @@ class BaseProvider(BaseSource):
         '''
         return []
 
-    def _process_desired_zone(self, desired):
-        '''
-        Providers can use this method to make any custom changes to the
-        desired zone.
-        '''
-        if self.SUPPORTS_MUTLIVALUE_PTR:
-            # nothing do here
-            return desired
-
-        new_desired = Zone(desired.name, desired.sub_zones)
-        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)
-                record = record.copy()
-                record.values = [record.value]
-
-            new_desired.add_record(record)
-
-        return new_desired
+    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)
 

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)

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()

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()