From e1e87a13c0ebac66df06b40681129f2652592386 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 3 Jul 2017 14:52:33 -0700 Subject: [PATCH] First pass through at refactoring to support geo cname - GeoMixin -> GeoValuesMixin - GeoValueMixin added - GeoValue -> GeoBase, GeoValue, and GeoValues - related tests & cleanup --- octodns/record.py | 190 ++++++++++++++++++++++++----------- tests/test_octodns_record.py | 80 +++++++++++++-- 2 files changed, 207 insertions(+), 63 deletions(-) diff --git a/octodns/record.py b/octodns/record.py index 6ee9dff..4f95b41 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -173,45 +173,6 @@ class Record(object): raise NotImplementedError('Abstract base class, __repr__ required') -class GeoValue(object): - geo_re = re.compile(r'^(?P\w\w)(-(?P\w\w)' - r'(-(?P\w\w))?)?$') - - @classmethod - def _validate_geo(cls, code): - reasons = [] - match = cls.geo_re.match(code) - if not match: - reasons.append('invalid geo "{}"'.format(code)) - return reasons - - def __init__(self, geo, values): - self.code = geo - match = self.geo_re.match(geo) - self.continent_code = match.group('continent_code') - self.country_code = match.group('country_code') - self.subdivision_code = match.group('subdivision_code') - self.values = values - - @property - def parents(self): - bits = self.code.split('-')[:-1] - while bits: - yield '-'.join(bits) - bits.pop() - - def __cmp__(self, other): - return 0 if (self.continent_code == other.continent_code and - self.country_code == other.country_code and - self.subdivision_code == other.subdivision_code and - self.values == other.values) else 1 - - def __repr__(self): - return "'Geo {} {} {} {}'".format(self.continent_code, - self.country_code, - self.subdivision_code, self.values) - - class _ValuesMixin(object): @classmethod @@ -260,7 +221,57 @@ class _ValuesMixin(object): self.fqdn, values) -class _GeoMixin(_ValuesMixin): +class _GeoBase(object): + geo_re = re.compile(r'^(?P\w\w)(-(?P\w\w)' + r'(-(?P\w\w))?)?$') + + @classmethod + def _validate_geo(cls, code): + reasons = [] + # TODO: validate legal codes + match = cls.geo_re.match(code) + if not match: + reasons.append('invalid geo "{}"'.format(code)) + return reasons + + def __init__(self, geo): + self.code = geo + match = self.geo_re.match(geo) + self.continent_code = match.group('continent_code') + self.country_code = match.group('country_code') + self.subdivision_code = match.group('subdivision_code') + + @property + def parents(self): + bits = self.code.split('-')[:-1] + while bits: + yield '-'.join(bits) + bits.pop() + + def __cmp__(self, other): + return 0 if (self.continent_code == other.continent_code and + self.country_code == other.country_code and + self.subdivision_code == other.subdivision_code) else 1 + + +class GeoValues(_GeoBase): + + def __init__(self, geo, values): + super(GeoValues, self).__init__(geo) + self.values = values + + def __cmp__(self, other): + return super(GeoValues, self).__cmp__(other) + \ + 0 if self.values == other.values else 1 + + def __repr__(self): + return "'GeoValues {} {} {} {}'".format(self.continent_code, + self.country_code, + self.subdivision_code, + self.values) + + +class _GeoValuesMixin(_ValuesMixin): ''' Adds GeoDNS support to a record. @@ -269,33 +280,29 @@ class _GeoMixin(_ValuesMixin): @classmethod def validate(cls, name, data): - reasons = super(_GeoMixin, cls).validate(name, data) + reasons = super(_GeoValuesMixin, cls).validate(name, data) try: geo = dict(data['geo']) - # TODO: validate legal codes for code, values in geo.items(): - reasons.extend(GeoValue._validate_geo(code)) + reasons.extend(GeoValues._validate_geo(code)) for value in values: reasons.extend(cls._validate_value(value)) except KeyError: pass return reasons - # TODO: support 'value' as well - # TODO: move away from "data" hash to strict params, it's kind of leaking - # the yaml implementation into here and then forcing it back out into - # non-yaml providers during input def __init__(self, zone, name, data, *args, **kwargs): - super(_GeoMixin, self).__init__(zone, name, data, *args, **kwargs) + super(_GeoValuesMixin, self).__init__(zone, name, data, *args, + **kwargs) try: self.geo = dict(data['geo']) except KeyError: self.geo = {} for code, values in self.geo.items(): - self.geo[code] = GeoValue(code, values) + self.geo[code] = GeoValues(code, values) def _data(self): - ret = super(_GeoMixin, self)._data() + ret = super(_GeoValuesMixin, self)._data() if self.geo: geo = {} for code, value in self.geo.items(): @@ -307,7 +314,7 @@ class _GeoMixin(_ValuesMixin): if target.SUPPORTS_GEO: if self.geo != other.geo: return Update(self, other) - return super(_GeoMixin, self).changes(other, target) + return super(_GeoValuesMixin, self).changes(other, target) def __repr__(self): if self.geo: @@ -315,10 +322,10 @@ class _GeoMixin(_ValuesMixin): self._type, self.ttl, self.fqdn, self.values, self.geo) - return super(_GeoMixin, self).__repr__() + return super(_GeoValuesMixin, self).__repr__() -class ARecord(_GeoMixin, Record): +class ARecord(_GeoValuesMixin, Record): _type = 'A' @classmethod @@ -334,7 +341,7 @@ class ARecord(_GeoMixin, Record): return values -class AaaaRecord(_GeoMixin, Record): +class AaaaRecord(_GeoValuesMixin, Record): _type = 'AAAA' @classmethod @@ -398,15 +405,84 @@ class AliasRecord(_ValueMixin, Record): return value -class CnameRecord(_ValueMixin, Record): +class GeoValue(_GeoBase): + + def __init__(self, geo, value): + super(GeoValue, self).__init__(geo) + self.value = value + + def __cmp__(self, other): + return super(GeoValue, self).__cmp__(other) + \ + 0 if self.value == other.value else 1 + + def __repr__(self): + return "'GeoValue {} {} {} {}'".format(self.continent_code, + self.country_code, + self.subdivision_code, + self.value) + + +class _GeoValueMixin(_ValueMixin): + ''' + Adds GeoDNS support to a record. + + Must be included before `Record`. + ''' + + @classmethod + def validate(cls, name, data): + reasons = super(_GeoValueMixin, cls).validate(name, data) + try: + geo = dict(data['geo']) + for code, value in geo.items(): + reasons.extend(GeoValue._validate_geo(code)) + reasons.extend(cls._validate_value(value)) + except KeyError: + pass + return reasons + + # TODO: support 'value' as well + def __init__(self, zone, name, data, *args, **kwargs): + super(_GeoValueMixin, self).__init__(zone, name, data, *args, **kwargs) + try: + self.geo = dict(data['geo']) + except KeyError: + self.geo = {} + for code, value in self.geo.items(): + self.geo[code] = GeoValue(code, value) + + def _data(self): + ret = super(_GeoValueMixin, self)._data() + if self.geo: + geo = {} + for code, value in self.geo.items(): + geo[code] = value.value + ret['geo'] = geo + return ret + + def changes(self, other, target): + if target.SUPPORTS_GEO: + if self.geo != other.geo: + return Update(self, other) + return super(_GeoValueMixin, self).changes(other, target) + + def __repr__(self): + if self.geo: + return '<{} {} {}, {}, {}, {}>'.format(self.__class__.__name__, + self._type, self.ttl, + self.fqdn, self.value, + self.geo) + return super(_GeoValueMixin, self).__repr__() + + +class CnameRecord(_GeoValueMixin, Record): _type = 'CNAME' @classmethod def validate(cls, name, data): - reasons = [] + reasons = super(CnameRecord, cls).validate(name, data) if name == '': reasons.append('root CNAME not allowed') - reasons.extend(super(CnameRecord, cls).validate(name, data)) return reasons @classmethod diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 1d64081..eebf5cd 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -8,8 +8,8 @@ from __future__ import absolute_import, division, print_function, \ from unittest import TestCase from octodns.record import ARecord, AaaaRecord, AliasRecord, CnameRecord, \ - Create, Delete, GeoValue, MxRecord, NaptrRecord, NaptrValue, NsRecord, \ - Record, SshfpRecord, SpfRecord, SrvRecord, TxtRecord, Update, \ + Create, Delete, GeoValue, GeoValues, MxRecord, NaptrRecord, NaptrValue, \ + NsRecord, Record, SshfpRecord, SpfRecord, SrvRecord, TxtRecord, Update, \ ValidationError from octodns.zone import Zone @@ -96,7 +96,7 @@ class TestRecord(TestCase): DummyRecord().__repr__() - def test_geo(self): + def test_a_geo_values(self): geo_data = {'ttl': 42, 'values': ['5.2.3.4', '6.2.3.4'], 'geo': {'AF': ['1.1.1.1'], 'AS-JP': ['2.2.2.2', '3.3.3.3'], @@ -626,8 +626,19 @@ class TestRecord(TestCase): def test_geo_value(self): code = 'NA-US-CA' - values = ['1.2.3.4'] - geo = GeoValue(code, values) + value = '1.2.3.4' + geo = GeoValue(code, value) + self.assertEquals(code, geo.code) + self.assertEquals('NA', geo.continent_code) + self.assertEquals('US', geo.country_code) + self.assertEquals('CA', geo.subdivision_code) + self.assertEquals(value, geo.value) + self.assertEquals(['NA-US', 'NA'], list(geo.parents)) + + def test_geo_values(self): + code = 'NA-US-CA' + values = ['1.2.3.4', '1.2.3.5'] + geo = GeoValues(code, values) self.assertEquals(code, geo.code) self.assertEquals('NA', geo.continent_code) self.assertEquals('US', geo.country_code) @@ -737,7 +748,7 @@ class TestRecordValidation(TestCase): 'invalid ip address "hello"', ], ctx.exception.reasons) - def test_geo(self): + def test_geo_values(self): Record.new(self.zone, '', { 'geo': { 'NA': ['1.2.3.5'], @@ -864,6 +875,11 @@ class TestRecordValidation(TestCase): def test_CNAME(self): # doesn't blow up Record.new(self.zone, 'www', { + 'geo': { + 'NA': 'na.foo.bar.com.', + 'NA-US': 'na-us.foo.bar.com.', + 'NA-US-CA': 'na-us-ca.foo.bar.com.' + }, 'type': 'CNAME', 'ttl': 600, 'value': 'foo.bar.com.', @@ -887,6 +903,58 @@ class TestRecordValidation(TestCase): }) self.assertEquals(['missing trailing .'], ctx.exception.reasons) + # missing trailing . on geo + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'www', { + 'geo': { + 'NA': 'na.foo.bar.com', + }, + 'type': 'CNAME', + 'ttl': 600, + 'value': 'foo.bar.com.', + }) + self.assertEquals(['missing trailing .'], ctx.exception.reasons) + + def test_geo_value(self): + geo_data = {'ttl': 42, 'value': 'foo.bar.com', + 'geo': {'NA': 'na.foo.bar.com.', + 'NA-US': 'na-us.foo.bar.com.', + 'NA-US-CA': 'na-us-ca.foo.bar.com.'}} + geo = CnameRecord(self.zone, 'geo', geo_data) + self.assertEquals(geo_data, geo.data) + + other_data = {'ttl': 42, 'value': 'foo.bar.com', + 'geo': {'NA': 'na.foo.bar.com.', + 'NA-US': 'na-us.foo.bar.com.', + 'NA-US-CA': 'na-us-ca.foo.bar.com.'}} + other = CnameRecord(self.zone, 'geo', other_data) + self.assertEquals(other_data, other.data) + + simple_target = SimpleProvider() + geo_target = GeoProvider() + + # Geo provider doesn't consider identical geo to be changes + self.assertFalse(geo.changes(geo, geo_target)) + + # geo values don't impact equality + other.geo['NA'].value = 'na2.foo.bar.com.' + self.assertTrue(geo == other) + # Non-geo supporting provider doesn't consider geo diffs to be changes + self.assertFalse(geo.changes(other, simple_target)) + # Geo provider does consider geo diffs to be changes + self.assertTrue(geo.changes(other, geo_target)) + + # Object without geo doesn't impact equality + other.geo = {} + self.assertTrue(geo == other) + # Non-geo supporting provider doesn't consider lack of geo a diff + self.assertFalse(geo.changes(other, simple_target)) + # Geo provider does consider lack of geo diffs to be changes + self.assertTrue(geo.changes(other, geo_target)) + + # __repr__ doesn't blow up + geo.__repr__() + def test_MX(self): # doesn't blow up Record.new(self.zone, '', {