From a9c6d8c6baa46d2b72753478575b5342c0c56019 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 29 May 2017 08:26:38 -0700 Subject: [PATCH] Rework _Route53Record to avoid a bunch of hacks They were working around the lack of class hierarchy, this addresses that by adding 2 child classes. It gets rid of a bunch of ugly default params and if-this junk in the main class that was trying to deal with plain & geo records. Also as a positive side effect it gets rid of some hacks that lived in Route53Provider dealing with the fact that the old setup couldn't detect going to/from a GeoDNS record correctly. --- octodns/provider/route53.py | 237 +++++++++++++++---------- tests/test_octodns_provider_route53.py | 102 ++++++++--- 2 files changed, 221 insertions(+), 118 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 3849561..c42a394 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -16,27 +16,71 @@ from ..record import Record, Update from .base import BaseProvider +octal_re = re.compile(r'\\(\d\d\d)') + + +def _octal_replace(s): + # See http://docs.aws.amazon.com/Route53/latest/DeveloperGuide/ + # DomainNameFormat.html + return octal_re.sub(lambda m: chr(int(m.group(1), 8)), s) + + class _Route53Record(object): - def __init__(self, fqdn, _type, ttl, record=None, values=None, geo=None, - health_check_id=None): - self.fqdn = fqdn - self._type = _type - self.ttl = ttl - # From here on things are a little ugly, it works, but would be nice to - # clean up someday. - if record: - values_for = getattr(self, '_values_for_{}'.format(self._type)) - self.values = values_for(record) + @classmethod + def new(self, provider, record, creating): + ret = set() + if getattr(record, 'geo', False): + ret.add(_Route53GeoDefault(provider, record, creating)) + for ident, geo in record.geo.items(): + ret.add(_Route53GeoRecord(provider, record, ident, geo, + creating)) else: - self.values = values - self.geo = geo - self.health_check_id = health_check_id - self.is_geo_default = False + ret.add(_Route53Record(provider, record, creating)) + return ret - @property - def _geo_code(self): - return getattr(self.geo, 'code', '') + def __init__(self, provider, record, creating): + self.fqdn = record.fqdn + self._type = record._type + self.ttl = record.ttl + + values_for = getattr(self, '_values_for_{}'.format(self._type)) + self.values = values_for(record) + + def mod(self, action): + return { + 'Action': action, + 'ResourceRecordSet': { + 'Name': self.fqdn, + 'ResourceRecords': [{'Value': v} for v in self.values], + 'TTL': self.ttl, + 'Type': self._type, + } + } + + # NOTE: we're using __hash__ and __cmp__ methods that consider + # _Route53Records equivalent if they have the same class, fqdn, and _type. + # Values are ignored. This is usful when computing diffs/changes. + + def __hash__(self): + 'sub-classes should never use this method' + return '{}:{}'.format(self.fqdn, self._type).__hash__() + + def __cmp__(self, other): + '''sub-classes should call up to this and return its value if non-zero. + When it's zero they should compute their own __cmp__''' + if self.__class__ != other.__class__: + return cmp(self.__class__, other.__class__) + elif self.fqdn != other.fqdn: + return cmp(self.fqdn, other.fqdn) + elif self._type != other._type: + return cmp(self._type, other._type) + # We're ignoring ttl, it's not an actual differentiator + return 0 + + def __repr__(self): + return '_Route53Record<{} {} {} {}>'.format(self.fqdn, self._type, + self.ttl, self.values) def _values_for_values(self, record): return record.values @@ -75,68 +119,91 @@ class _Route53Record(object): v.target) for v in record.values] + +class _Route53GeoDefault(_Route53Record): + + def mod(self, action): + return { + 'Action': action, + 'ResourceRecordSet': { + 'Name': self.fqdn, + 'GeoLocation': { + 'CountryCode': '*' + }, + 'ResourceRecords': [{'Value': v} for v in self.values], + 'SetIdentifier': 'default', + 'TTL': self.ttl, + 'Type': self._type, + } + } + + def __hash__(self): + return '{}:{}:default'.format(self.fqdn, self._type).__hash__() + + def __repr__(self): + return '_Route53GeoDefault<{} {} {} {}>'.format(self.fqdn, self._type, + self.ttl, self.values) + + +class _Route53GeoRecord(_Route53Record): + + def __init__(self, provider, record, ident, geo, creating): + super(_Route53GeoRecord, self).__init__(provider, record, creating) + self.geo = geo + + self.health_check_id = provider.get_health_check_id(record, ident, + geo, creating) + def mod(self, action): + geo = self.geo rrset = { 'Name': self.fqdn, - 'Type': self._type, + 'GeoLocation': { + 'CountryCode': '*' + }, + 'ResourceRecords': [{'Value': v} for v in geo.values], + 'SetIdentifier': geo.code, 'TTL': self.ttl, - 'ResourceRecords': [{'Value': v} for v in self.values], + 'Type': self._type, } - if self.is_geo_default: + + if self.health_check_id: + rrset['HealthCheckId'] = self.health_check_id + + if geo.subdivision_code: rrset['GeoLocation'] = { - 'CountryCode': '*' + 'CountryCode': geo.country_code, + 'SubdivisionCode': geo.subdivision_code + } + elif geo.country_code: + rrset['GeoLocation'] = { + 'CountryCode': geo.country_code + } + else: + rrset['GeoLocation'] = { + 'ContinentCode': geo.continent_code } - rrset['SetIdentifier'] = 'default' - elif self.geo: - geo = self.geo - rrset['SetIdentifier'] = geo.code - if self.health_check_id: - rrset['HealthCheckId'] = self.health_check_id - if geo.subdivision_code: - rrset['GeoLocation'] = { - 'CountryCode': geo.country_code, - 'SubdivisionCode': geo.subdivision_code - } - elif geo.country_code: - rrset['GeoLocation'] = { - 'CountryCode': geo.country_code - } - else: - rrset['GeoLocation'] = { - 'ContinentCode': geo.continent_code - } return { 'Action': action, 'ResourceRecordSet': rrset, } - # NOTE: we're using __hash__ and __cmp__ methods that consider - # _Route53Records equivalent if they have the same fqdn, _type, and - # geo.ident. Values are ignored. This is usful when computing - # diffs/changes. - def __hash__(self): return '{}:{}:{}'.format(self.fqdn, self._type, - self._geo_code).__hash__() + self.geo.code).__hash__() def __cmp__(self, other): - return 0 if (self.fqdn == other.fqdn and - self._type == other._type and - self._geo_code == other._geo_code) else 1 + ret = super(_Route53GeoRecord, self).__cmp__(other) + if ret != 0: + return ret + return cmp(self.geo.code, other.geo.code) def __repr__(self): - return '_Route53Record<{} {:>5} {:8} {}>' \ - .format(self.fqdn, self._type, self._geo_code, self.values) - - -octal_re = re.compile(r'\\(\d\d\d)') - - -def _octal_replace(s): - # See http://docs.aws.amazon.com/Route53/latest/DeveloperGuide/ - # DomainNameFormat.html - return octal_re.sub(lambda m: chr(int(m.group(1), 8)), s) + return '_Route53GeoRecord<{} {} {} {} {}>'.format(self.fqdn, + self._type, self.ttl, + self.geo.code, + self.values) class Route53Provider(BaseProvider): @@ -388,7 +455,7 @@ class Route53Provider(BaseProvider): def _gen_mods(self, action, records): ''' - Turns `_Route53Record`s in to `change_resource_record_sets` `Changes` + Turns `_Route53*`s in to `change_resource_record_sets` `Changes` ''' return [r.mod(action) for r in records] @@ -417,14 +484,14 @@ class Route53Provider(BaseProvider): # We've got a cached version use it return self._health_checks - def _get_health_check_id(self, record, ident, geo, create): + def get_health_check_id(self, record, ident, geo, create): # fqdn & the first value are special, we use them to match up health # checks to their records. Route53 health checks check a single ip and # we're going to assume that ips are interchangeable to avoid # health-checking each one independently fqdn = record.fqdn first_value = geo.values[0] - self.log.debug('_get_health_check_id: fqdn=%s, type=%s, geo=%s, ' + self.log.debug('get_health_check_id: fqdn=%s, type=%s, geo=%s, ' 'first_value=%s', fqdn, record._type, ident, first_value) @@ -470,7 +537,7 @@ class Route53Provider(BaseProvider): # store the new health check so that we'll be able to find it in the # future self._health_checks[id] = health_check - self.log.info('_get_health_check_id: created id=%s, host=%s, ' + self.log.info('get_health_check_id: created id=%s, host=%s, ' 'first_value=%s', id, host, first_value) return id @@ -479,8 +546,9 @@ class Route53Provider(BaseProvider): # Find the health checks we're using for the new route53 records in_use = set() for r in new: - if r.health_check_id: - in_use.add(r.health_check_id) + hc_id = getattr(r, 'health_check_id', False) + if hc_id: + in_use.add(hc_id) self.log.debug('_gc_health_checks: in_use=%s', in_use) # Now we need to run through ALL the health checks looking for those # that apply to this record, deleting any that do and are no longer in @@ -499,23 +567,9 @@ class Route53Provider(BaseProvider): def _gen_records(self, record, creating=False): ''' - Turns an octodns.Record into one or more `_Route53Record`s + Turns an octodns.Record into one or more `_Route53*`s ''' - records = set() - base = _Route53Record(record.fqdn, record._type, record.ttl, - record=record) - records.add(base) - if getattr(record, 'geo', False): - base.is_geo_default = True - for ident, geo in record.geo.items(): - health_check_id = self._get_health_check_id(record, ident, geo, - creating) - records.add(_Route53Record(record.fqdn, record._type, - record.ttl, values=geo.values, - geo=geo, - health_check_id=health_check_id)) - - return records + return _Route53Record.new(self, record, creating) def _mod_Create(self, change): # New is the stuff that needs to be created @@ -541,24 +595,11 @@ class Route53Provider(BaseProvider): # things that haven't actually changed, but that's for another day. # We can't use set math here b/c we won't be able to control which of # the two objects will be in the result and we need to ensure it's the - # new one and we have to include some special handling when converting - # to/from a GEO enabled record + # new one. upserts = set() - existing_records = {r: r for r in existing_records} for new_record in new_records: - try: - existing_record = existing_records[new_record] - if new_record.is_geo_default != existing_record.is_geo_default: - # going from normal to geo or geo to normal, need a delete - # and create - deletes.add(existing_record) - creates.add(new_record) - else: - # just an update - upserts.add(new_record) - except KeyError: - # Completely new record, ignore - pass + if new_record in existing_records: + upserts.add(new_record) return self._gen_mods('DELETE', deletes) + \ self._gen_mods('CREATE', creates) + \ diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index e1a73f8..36b3070 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -11,8 +11,8 @@ from unittest import TestCase from mock import patch from octodns.record import Create, Delete, Record, Update -from octodns.provider.route53 import _Route53Record, Route53Provider, \ - _octal_replace +from octodns.provider.route53 import Route53Provider, _Route53GeoDefault, \ + _Route53GeoRecord, _Route53Record, _octal_replace from octodns.zone import Zone from helpers import GeoProvider @@ -522,21 +522,21 @@ class TestRoute53Provider(TestCase): 'Changes': [{ 'Action': 'DELETE', 'ResourceRecordSet': { - 'GeoLocation': {'ContinentCode': 'OC'}, + 'GeoLocation': {'CountryCode': '*'}, 'Name': 'simple.unit.tests.', - 'ResourceRecords': [{'Value': '3.2.3.4'}, - {'Value': '4.2.3.4'}], - 'SetIdentifier': 'OC', + 'ResourceRecords': [{'Value': '1.2.3.4'}, + {'Value': '2.2.3.4'}], + 'SetIdentifier': 'default', 'TTL': 61, 'Type': 'A'} }, { 'Action': 'DELETE', 'ResourceRecordSet': { - 'GeoLocation': {'CountryCode': '*'}, + 'GeoLocation': {'ContinentCode': 'OC'}, 'Name': 'simple.unit.tests.', - 'ResourceRecords': [{'Value': '1.2.3.4'}, - {'Value': '2.2.3.4'}], - 'SetIdentifier': 'default', + 'ResourceRecords': [{'Value': '3.2.3.4'}, + {'Value': '4.2.3.4'}], + 'SetIdentifier': 'OC', 'TTL': 61, 'Type': 'A'} }, { @@ -694,8 +694,7 @@ class TestRoute53Provider(TestCase): 'AF': ['4.2.3.4'], } }) - id = provider._get_health_check_id(record, 'AF', record.geo['AF'], - True) + id = provider.get_health_check_id(record, 'AF', record.geo['AF'], True) self.assertEquals('42', id) def test_health_check_create(self): @@ -765,13 +764,12 @@ class TestRoute53Provider(TestCase): }) # if not allowed to create returns none - id = provider._get_health_check_id(record, 'AF', record.geo['AF'], - False) + id = provider.get_health_check_id(record, 'AF', record.geo['AF'], + False) self.assertFalse(id) # when allowed to create we do - id = provider._get_health_check_id(record, 'AF', record.geo['AF'], - True) + id = provider.get_health_check_id(record, 'AF', record.geo['AF'], True) self.assertEquals('42', id) stubber.assert_no_pending_responses() @@ -1106,10 +1104,6 @@ class TestRoute53Provider(TestCase): self.assertEquals(0, len(extra)) stubber.assert_no_pending_responses() - def test_route_53_record(self): - # Just make sure it doesn't blow up - _Route53Record('foo.unit.tests.', 'A', 30).__repr__() - def _get_test_plan(self, max_changes): provider = Route53Provider('test', 'abc', '123', max_changes) @@ -1217,3 +1211,71 @@ class TestRoute53Provider(TestCase): with self.assertRaises(Exception) as ctx: provider.apply(plan) self.assertTrue('modifications' in ctx.exception.message) + + +class TestRoute53Records(TestCase): + + def test_route53_record(self): + existing = Zone('unit.tests.', []) + record_a = Record.new(existing, '', { + 'geo': { + 'NA-US': ['2.2.2.2', '3.3.3.3'], + 'OC': ['4.4.4.4', '5.5.5.5'] + }, + 'ttl': 99, + 'type': 'A', + 'values': ['9.9.9.9'] + }) + a = _Route53Record(None, record_a, False) + self.assertEquals(a, a) + b = _Route53Record(None, Record.new(existing, '', + {'ttl': 32, 'type': 'A', + 'values': ['8.8.8.8', + '1.1.1.1']}), + False) + self.assertEquals(b, b) + c = _Route53Record(None, Record.new(existing, 'other', + {'ttl': 99, 'type': 'A', + 'values': ['9.9.9.9']}), + False) + self.assertEquals(c, c) + d = _Route53Record(None, Record.new(existing, '', + {'ttl': 42, 'type': 'CNAME', + 'value': 'foo.bar.'}), + False) + self.assertEquals(d, d) + + # Same fqdn & type is same record + self.assertEquals(a, b) + # Same name & different type is not the same + self.assertNotEquals(a, d) + # Different name & same type is not the same + self.assertNotEquals(a, c) + + # Same everything, different class is not the same + e = _Route53GeoDefault(None, record_a, False) + self.assertNotEquals(a, e) + + class DummyProvider(object): + + def get_health_check_id(self, *args, **kwargs): + return None + + provider = DummyProvider() + f = _Route53GeoRecord(provider, record_a, 'NA-US', + record_a.geo['NA-US'], False) + self.assertEquals(f, f) + g = _Route53GeoRecord(provider, record_a, 'OC', + record_a.geo['OC'], False) + self.assertEquals(g, g) + + # Geo and non-geo are not the same, using Geo as primary to get it's + # __cmp__ + self.assertNotEquals(f, a) + # Same everything, different geo's is not the same + self.assertNotEquals(f, g) + + # Make sure it doesn't blow up + a.__repr__() + e.__repr__() + f.__repr__()