From a97818b6ec745a624eeca6e2ace71f93dfc65918 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Fri, 23 Jun 2017 09:01:25 -0700 Subject: [PATCH] populating existing provider state is lenient - adds lenient flag to Record.new, problems during validation are just warnings if it's true - target populate calls during the plan phase pass lenient=True - make all of the provider.populate call logging consistent including both target and lenient - add source=self to Record.new in a few places that were missing it --- octodns/provider/base.py | 2 +- octodns/provider/cloudflare.py | 8 +++++--- octodns/provider/dnsimple.py | 8 +++++--- octodns/provider/dyn.py | 9 ++++++--- octodns/provider/ns1.py | 8 +++++--- octodns/provider/powerdns.py | 7 ++++--- octodns/provider/route53.py | 9 ++++++--- octodns/provider/yaml.py | 9 ++++++--- octodns/record.py | 15 ++++++++++----- octodns/source/base.py | 8 ++++++++ octodns/source/tinydns.py | 19 +++++++++++-------- tests/helpers.py | 4 ++-- tests/test_octodns_provider_base.py | 4 ++-- tests/test_octodns_provider_route53.py | 6 +++--- tests/test_octodns_record.py | 16 ++++++++++++++++ 15 files changed, 90 insertions(+), 42 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 385fe36..2fd4349 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -104,7 +104,7 @@ class BaseProvider(BaseSource): self.log.info('plan: desired=%s', desired.name) existing = Zone(desired.name, desired.sub_zones) - self.populate(existing, target=True) + self.populate(existing, target=True, lenient=True) # compute the changes at the zone/record level changes = existing.changes(desired, self) diff --git a/octodns/provider/cloudflare.py b/octodns/provider/cloudflare.py index a45bd44..51b2171 100644 --- a/octodns/provider/cloudflare.py +++ b/octodns/provider/cloudflare.py @@ -154,8 +154,9 @@ class CloudflareProvider(BaseProvider): return self._zone_records[zone.name] - def populate(self, zone, target=False): - self.log.debug('populate: name=%s', zone.name) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) before = len(zone.records) records = self.zone_records(zone) @@ -171,7 +172,8 @@ class CloudflareProvider(BaseProvider): for _type, records in types.items(): data_for = getattr(self, '_data_for_{}'.format(_type)) data = data_for(_type, records) - record = Record.new(zone, name, data, source=self) + record = Record.new(zone, name, data, source=self, + lenient=lenient) zone.add_record(record) self.log.info('populate: found %s records', diff --git a/octodns/provider/dnsimple.py b/octodns/provider/dnsimple.py index cb0f2d7..91bd638 100644 --- a/octodns/provider/dnsimple.py +++ b/octodns/provider/dnsimple.py @@ -234,8 +234,9 @@ class DnsimpleProvider(BaseProvider): return self._zone_records[zone.name] - def populate(self, zone, target=False): - self.log.debug('populate: name=%s', zone.name) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) values = defaultdict(lambda: defaultdict(list)) for record in self.zone_records(zone): @@ -252,7 +253,8 @@ class DnsimpleProvider(BaseProvider): for name, types in values.items(): for _type, records in types.items(): data_for = getattr(self, '_data_for_{}'.format(_type)) - record = Record.new(zone, name, data_for(_type, records)) + record = Record.new(zone, name, data_for(_type, records), + source=self, lenient=lenient) zone.add_record(record) self.log.info('populate: found %s records', diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 673e8d0..5e0e1f3 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -338,8 +338,10 @@ class DynProvider(BaseProvider): return td_records - def populate(self, zone, target=False): - self.log.info('populate: zone=%s', zone.name) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) + before = len(zone.records) self._check_dyn_sess() @@ -364,7 +366,8 @@ class DynProvider(BaseProvider): for _type, records in types.items(): data_for = getattr(self, '_data_for_{}'.format(_type)) data = data_for(_type, records) - record = Record.new(zone, name, data, source=self) + record = Record.new(zone, name, data, source=self, + lenient=lenient) if record not in td_records: zone.add_record(record) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index c50341d..33fb19c 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -111,8 +111,9 @@ class Ns1Provider(BaseProvider): 'values': values, } - def populate(self, zone, target=False): - self.log.debug('populate: name=%s', zone.name) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) try: nsone_zone = self._client.loadZone(zone.name[:-1]) @@ -127,7 +128,8 @@ class Ns1Provider(BaseProvider): _type = record['type'] data_for = getattr(self, '_data_for_{}'.format(_type)) name = zone.hostname_from_fqdn(record['domain']) - record = Record.new(zone, name, data_for(_type, record)) + record = Record.new(zone, name, data_for(_type, record), + source=self, lenient=lenient) zone.add_record(record) self.log.info('populate: found %s records', diff --git a/octodns/provider/powerdns.py b/octodns/provider/powerdns.py index 21e4d44..d8cccae 100644 --- a/octodns/provider/powerdns.py +++ b/octodns/provider/powerdns.py @@ -146,8 +146,9 @@ class PowerDnsBaseProvider(BaseProvider): 'ttl': rrset['ttl'] } - def populate(self, zone, target=False): - self.log.debug('populate: name=%s', zone.name) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) resp = None try: @@ -177,7 +178,7 @@ class PowerDnsBaseProvider(BaseProvider): data_for = getattr(self, '_data_for_{}'.format(_type)) record_name = zone.hostname_from_fqdn(rrset['name']) record = Record.new(zone, record_name, data_for(rrset), - source=self) + source=self, lenient=lenient) zone.add_record(record) self.log.info('populate: found %s records', diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 12c1aff..3875bd6 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -418,8 +418,10 @@ class Route53Provider(BaseProvider): return self._r53_rrsets[zone_id] - def populate(self, zone, target=False): - self.log.debug('populate: name=%s', zone.name) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) + before = len(zone.records) zone_id = self._get_zone_id(zone.name) @@ -449,7 +451,8 @@ class Route53Provider(BaseProvider): data['geo'] = geo else: data = data[0] - record = Record.new(zone, name, data, source=self) + record = Record.new(zone, name, data, source=self, + lenient=lenient) zone.add_record(record) self.log.info('populate: found %s records', diff --git a/octodns/provider/yaml.py b/octodns/provider/yaml.py index c728caf..fe1a406 100644 --- a/octodns/provider/yaml.py +++ b/octodns/provider/yaml.py @@ -45,8 +45,10 @@ class YamlProvider(BaseProvider): self.default_ttl = default_ttl self.enforce_order = enforce_order - def populate(self, zone, target=False): - self.log.debug('populate: zone=%s, target=%s', zone.name, target) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) + if target: # When acting as a target we ignore any existing records so that we # create a completely new copy @@ -63,7 +65,8 @@ class YamlProvider(BaseProvider): for d in data: if 'ttl' not in d: d['ttl'] = self.default_ttl - record = Record.new(zone, name, d, source=self) + record = Record.new(zone, name, d, source=self, + lenient=lenient) zone.add_record(record) self.log.info('populate: found %s records', diff --git a/octodns/record.py b/octodns/record.py index 827ad0a..3f07c39 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -56,10 +56,12 @@ class Delete(Change): class ValidationError(Exception): + @classmethod + def build_message(cls, fqdn, reasons): + return 'Invalid record {}\n - {}'.format(fqdn, '\n - '.join(reasons)) + def __init__(self, fqdn, reasons): - message = 'Invalid record {}\n - {}' \ - .format(fqdn, '\n - '.join(reasons)) - super(Exception, self).__init__(message) + super(Exception, self).__init__(self.build_message(fqdn, reasons)) self.fqdn = fqdn self.reasons = reasons @@ -68,7 +70,7 @@ class Record(object): log = getLogger('Record') @classmethod - def new(cls, zone, name, data, source=None): + def new(cls, zone, name, data, source=None, lenient=False): fqdn = '{}.{}'.format(name, zone.name) if name else zone.name try: _type = data['type'] @@ -107,7 +109,10 @@ class Record(object): raise Exception('Unknown record type: "{}"'.format(_type)) reasons = _class.validate(name, data) if reasons: - raise ValidationError(fqdn, reasons) + if lenient: + cls.log.warn(ValidationError.build_message(fqdn, reasons)) + else: + raise ValidationError(fqdn, reasons) return _class(zone, name, data, source=source) @classmethod diff --git a/octodns/source/base.py b/octodns/source/base.py index 42d214b..2e2c5c2 100644 --- a/octodns/source/base.py +++ b/octodns/source/base.py @@ -23,6 +23,14 @@ class BaseSource(object): def populate(self, zone, target=False): ''' Loads all zones the provider knows about + + When `target` is True the populate call is being made to load the + current state of the provider. + + When `lenient` is True the populate call may skip record validation and + do a "best effort" load of data. That will allow through some common, + but not best practices stuff that we otherwise would reject. E.g. no + trailing . or mising escapes for ;. ''' raise NotImplementedError('Abstract base class, populate method ' 'missing') diff --git a/octodns/source/tinydns.py b/octodns/source/tinydns.py index 6805378..1b98092 100644 --- a/octodns/source/tinydns.py +++ b/octodns/source/tinydns.py @@ -81,19 +81,21 @@ class TinyDnsBaseSource(BaseSource): 'values': ['{}.'.format(r[0]) for r in records] } - def populate(self, zone, target=False): - self.log.debug('populate: zone=%s', zone.name) + def populate(self, zone, target=False, lenient=False): + self.log.debug('populate: name=%s, target=%s, lenient=%s', zone.name, + target, lenient) + before = len(zone.records) if zone.name.endswith('in-addr.arpa.'): - self._populate_in_addr_arpa(zone) + self._populate_in_addr_arpa(zone, lenient) else: - self._populate_normal(zone) + self._populate_normal(zone, lenient) self.log.info('populate: found %s records', len(zone.records) - before) - def _populate_normal(self, zone): + def _populate_normal(self, zone, lenient): type_map = { '=': 'A', '^': None, @@ -129,14 +131,15 @@ class TinyDnsBaseSource(BaseSource): data_for = getattr(self, '_data_for_{}'.format(_type)) data = data_for(_type, d) if data: - record = Record.new(zone, name, data, source=self) + record = Record.new(zone, name, data, source=self, + lenient=lenient) try: zone.add_record(record) except SubzoneRecordException: self.log.debug('_populate_normal: skipping subzone ' 'record=%s', record) - def _populate_in_addr_arpa(self, zone): + def _populate_in_addr_arpa(self, zone, lenient): name_re = re.compile('(?P.+)\.{}$'.format(zone.name[:-1])) for line in self._lines(): @@ -170,7 +173,7 @@ class TinyDnsBaseSource(BaseSource): 'ttl': ttl, 'type': 'PTR', 'value': value - }, source=self) + }, source=self, lenient=lenient) try: zone.add_record(record) except DuplicateRecordException: diff --git a/tests/helpers.py b/tests/helpers.py index a8aafa3..adac81d 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -22,7 +22,7 @@ class SimpleProvider(object): def __init__(self, id='test'): pass - def populate(self, zone, source=True): + def populate(self, zone, source=False, lenient=False): pass def supports(self, record): @@ -38,7 +38,7 @@ class GeoProvider(object): def __init__(self, id='test'): pass - def populate(self, zone, source=True): + def populate(self, zone, source=False, lenient=False): pass def supports(self, record): diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 766bf65..bd134bc 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -24,7 +24,7 @@ class HelperProvider(BaseProvider): self.apply_disabled = apply_disabled self.include_change_callback = include_change_callback - def populate(self, zone, target=False): + def populate(self, zone, target=False, lenient=False): pass def _include_change(self, change): @@ -72,7 +72,7 @@ class TestBaseProvider(TestCase): class HasPopulate(HasSupports): - def populate(self, zone, target=False): + def populate(self, zone, target=False, lenient=False): zone.add_record(Record.new(zone, '', { 'ttl': 60, 'type': 'A', diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 5982b74..0a769f9 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -370,7 +370,7 @@ class TestRoute53Provider(TestCase): stubber.assert_no_pending_responses() # Delete by monkey patching in a populate that includes an extra record - def add_extra_populate(existing, target): + def add_extra_populate(existing, target, lenient): for record in self.expected.records: existing.records.add(record) record = Record.new(existing, 'extra', @@ -406,7 +406,7 @@ class TestRoute53Provider(TestCase): # Update by monkey patching in a populate that modifies the A record # with geos - def mod_geo_populate(existing, target): + def mod_geo_populate(existing, target, lenient): for record in self.expected.records: if record._type != 'A' or not record.geo: existing.records.add(record) @@ -502,7 +502,7 @@ class TestRoute53Provider(TestCase): # Update converting to non-geo by monkey patching in a populate that # modifies the A record with geos - def mod_add_geo_populate(existing, target): + def mod_add_geo_populate(existing, target, lenient): for record in self.expected.records: if record._type != 'A' or record.geo: existing.records.add(record) diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 99a502e..6e40e18 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -643,6 +643,7 @@ class TestRecordValidation(TestCase): 'value': '1.2.3.4', }) self.assertEquals(['missing ttl'], ctx.exception.reasons) + # invalid ttl with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'www', { @@ -653,6 +654,21 @@ class TestRecordValidation(TestCase): self.assertEquals('www.unit.tests.', ctx.exception.fqdn) self.assertEquals(['invalid ttl'], ctx.exception.reasons) + # no exception if we're in lenient mode + Record.new(self.zone, 'www', { + 'type': 'A', + 'ttl': -1, + 'value': '1.2.3.4', + }, lenient=True) + + # __init__ may still blow up, even if validation is lenient + with self.assertRaises(KeyError) as ctx: + Record.new(self.zone, 'www', { + 'type': 'A', + 'ttl': -1, + }, lenient=True) + self.assertEquals(('value',), ctx.exception.args) + def test_A_and_values_mixin(self): # doesn't blow up Record.new(self.zone, '', {