From 536c0c68ecf4f6c22b8235cfad82467d48efaa3a Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 17 Aug 2021 19:15:34 -0700 Subject: [PATCH 1/6] no-op Provider.process_desired_zone --- octodns/provider/base.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 729c9ee..55b1cb4 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -44,9 +44,14 @@ class BaseProvider(BaseSource): ''' return [] + def process_desired_zone(self, desired): + return desired + def plan(self, desired, processors=[]): self.log.info('plan: desired=%s', desired.name) + 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: From c81450682cc33b6558b83cc0c40b99c0d0e5ed96 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 17 Aug 2021 20:26:51 -0700 Subject: [PATCH 2/6] Implement and test Route53Provider.process_desired_zone checking of NA-CA-* --- octodns/provider/base.py | 10 ++ octodns/provider/route53.py | 39 ++++++++ tests/test_octodns_provider_route53.py | 133 +++++++++++++++++++++++++ 3 files changed, 182 insertions(+) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 55b1cb4..238a68a 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -12,6 +12,10 @@ from ..zone import Zone from .plan import Plan +class ProviderException(Exception): + pass + + class BaseProvider(BaseSource): def __init__(self, id, apply_disabled=False, @@ -44,6 +48,12 @@ class BaseProvider(BaseSource): ''' return [] + def supports_warn_or_except(self, msg): + # TODO: base class param to control warn vs except + if False: + raise ProviderException(msg) + self.log.warning(msg) + def process_desired_zone(self, desired): return desired diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index f1b3b40..6b2fecb 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() + from pprint import pprint + pprint((record, record.data)) + 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 resulting in ' \ + 'empty geo target, skipping rule {}'.format(i) + self.supports_warn_or_except(msg) + continue + elif geos != filtered_geos: + msg = 'NA-CA-* not supported resulting in ' \ + 'empty geo target, skipping rule {}'.format(i) + self.supports_warn_or_except(msg) + rule.data['geos'] = filtered_geos + rules.append(rule) + + dynamic.rules = rules + + pprint((record, record.data)) + + 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_route53.py b/tests/test_octodns_provider_route53.py index 1bf3332..34124ee 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() From d77e7d485b83664783ab03aa107c27d7397c95ba Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 17 Aug 2021 20:32:14 -0700 Subject: [PATCH 3/6] Remove some debugging pprints --- octodns/provider/route53.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 6b2fecb..8841df4 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -931,8 +931,6 @@ class Route53Provider(BaseProvider): if getattr(record, 'dynamic', False): # Make a copy of the record in case we have to muck with it record = record.copy() - from pprint import pprint - pprint((record, record.data)) dynamic = record.dynamic rules = [] for i, rule in enumerate(dynamic.rules): @@ -957,8 +955,6 @@ class Route53Provider(BaseProvider): dynamic.rules = rules - pprint((record, record.data)) - ret.add_record(record) return super(Route53Provider, self).process_desired_zone(ret) From 5b0e47f31fc0c0e0c6e4d59ffcb1b4ec0a10032e Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 18 Aug 2021 10:07:21 -0700 Subject: [PATCH 4/6] Cleanup and test of _process_desired_zone and supports_warn_or_except --- octodns/provider/__init__.py | 4 ++++ octodns/provider/base.py | 33 ++++++++++++++++---------- octodns/provider/route53.py | 4 ++-- tests/test_octodns_provider_base.py | 27 ++++++++++++++++++++- tests/test_octodns_provider_route53.py | 10 ++++---- 5 files changed, 58 insertions(+), 20 deletions(-) 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 238a68a..433eab8 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -10,17 +10,15 @@ from six import text_type from ..source.base import BaseSource from ..zone import Zone from .plan import Plan - - -class ProviderException(Exception): - pass +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, ' @@ -32,6 +30,21 @@ 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 + ''' + return desired def _include_change(self, change): ''' @@ -49,18 +62,14 @@ class BaseProvider(BaseSource): return [] def supports_warn_or_except(self, msg): - # TODO: base class param to control warn vs except - if False: - raise ProviderException(msg) + if self.strict_supports: + raise ProviderException('{}: {}'.format(self.id, msg)) self.log.warning(msg) - def process_desired_zone(self, desired): - return desired - def plan(self, desired, processors=[]): self.log.info('plan: desired=%s', desired.name) - desired = self.process_desired_zone(desired) + desired = self._process_desired_zone(desired) existing = Zone(desired.name, desired.sub_zones) exists = self.populate(existing, target=True, lenient=True) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 8841df4..8552a8c 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -925,7 +925,7 @@ class Route53Provider(BaseProvider): return data - def process_desired_zone(self, desired): + def _process_desired_zone(self, desired): ret = Zone(desired.name, desired.sub_zones) for record in desired.records: if getattr(record, 'dynamic', False): @@ -957,7 +957,7 @@ class Route53Provider(BaseProvider): ret.add_record(record) - return super(Route53Provider, self).process_desired_zone(ret) + 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, diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 4dfce48..b748762 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 @@ -429,3 +430,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!') + normal.log.warning.assert_called_once() + normal.log.warning.assert_has_calls([ + call('Hello World!') + ]) + + strict = MinimalProvider(strict_supports=True) + # Should log and not expect + with self.assertRaises(ProviderException) as ctx: + strict.supports_warn_or_except('Hello World!') + 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 34124ee..b3e5ba4 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -399,7 +399,7 @@ class TestRoute53Provider(TestCase): # No records, essentially a no-op desired = Zone('unit.tests.', []) - got = provider.process_desired_zone(desired) + got = provider._process_desired_zone(desired) self.assertEquals(desired.records, got.records) # Record without any geos @@ -422,7 +422,7 @@ class TestRoute53Provider(TestCase): }, }) desired.add_record(record) - got = provider.process_desired_zone(desired) + 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) @@ -455,7 +455,7 @@ class TestRoute53Provider(TestCase): }, }) desired.add_record(record) - got = provider.process_desired_zone(desired) + 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']) @@ -489,7 +489,7 @@ class TestRoute53Provider(TestCase): }, }) desired.add_record(record) - got = provider.process_desired_zone(desired) + 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) @@ -521,7 +521,7 @@ class TestRoute53Provider(TestCase): }, }) desired.add_record(record) - got = provider.process_desired_zone(desired) + 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']) From 65f0bfc2435ffcdf7738692915d0cd42b6d66c7d Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 18 Aug 2021 12:35:49 -0700 Subject: [PATCH 5/6] Update multi-value PTR warn to supports_warn_or_except --- octodns/provider/base.py | 7 ++++--- tests/test_octodns_provider_base.py | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index e952f30..548662a 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -52,9 +52,10 @@ 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) + self.supports_warn_or_except('does not support multi-value ' + 'PTR records; will use only {} ' + 'for {}'.format(record.value, + record.fqdn)) record = record.copy() record.values = [record.value] diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 0927878..62e6587 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -22,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): From 08f9ec56a3e898f1324fb9d9d679e3d7469da889 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 18 Aug 2021 16:25:13 -0700 Subject: [PATCH 6/6] Rework supports_warn_or_except to msg and fallback --- octodns/provider/base.py | 13 +++++++------ octodns/provider/route53.py | 16 ++++++++++------ tests/test_octodns_provider_base.py | 6 +++--- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 548662a..16cad7a 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -52,10 +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.supports_warn_or_except('does not support multi-value ' - 'PTR records; will use only {} ' - 'for {}'.format(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] @@ -78,10 +79,10 @@ class BaseProvider(BaseSource): ''' return [] - def supports_warn_or_except(self, msg): + def supports_warn_or_except(self, msg, fallback): if self.strict_supports: raise ProviderException('{}: {}'.format(self.id, msg)) - self.log.warning(msg) + self.log.warning('{}; {}'.format(msg, fallback)) def plan(self, desired, processors=[]): self.log.info('plan: desired=%s', desired.name) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 8552a8c..477a53e 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -942,14 +942,18 @@ class Route53Provider(BaseProvider): 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 resulting in ' \ - 'empty geo target, skipping rule {}'.format(i) - self.supports_warn_or_except(msg) + 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 resulting in ' \ - 'empty geo target, skipping rule {}'.format(i) - self.supports_warn_or_except(msg) + 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) diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 62e6587..46afd89 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -457,15 +457,15 @@ class TestBaseProvider(TestCase): normal = MinimalProvider(strict_supports=False) # Should log and not expect - normal.supports_warn_or_except('Hello World!') + normal.supports_warn_or_except('Hello World!', 'Goodbye') normal.log.warning.assert_called_once() normal.log.warning.assert_has_calls([ - call('Hello World!') + 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!') + 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()