From aeb70b24888cd448e8dcf073b391d7c649ab1b46 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 5 Oct 2019 20:01:53 -0700 Subject: [PATCH] Route53Provider python 3, rm incf.countryutils, lots of cmp removal and ordering fixes --- CHANGELOG.md | 6 + octodns/provider/route53.py | 106 +++++-- requirements.txt | 1 - tests/test_octodns_provider_rackspace.py | 1 - tests/test_octodns_provider_route53.py | 342 +++++++++++++---------- 5 files changed, 278 insertions(+), 178 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 76ff8b0..e30d0df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +## v0.9.9 - 2019-??-?? - Python 3.7 Support + +* Route53 _mod_keyer ordering wasn't complete/reliable and in python 3 this + resulted in randomness. This has been addressed and may result in value + reordering on next plan, no actual changes in behavior should occur. + ## v0.9.8 - 2019-09-30 - One with no changes b/c PyPi description problems * No material changes diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 0bc7a99..6d2cfeb 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -8,8 +8,8 @@ from __future__ import absolute_import, division, print_function, \ from boto3 import client from botocore.config import Config from collections import defaultdict -from incf.countryutils.transformations import cca_to_ctca2 from ipaddress import AddressValueError, ip_address +from pycountry_convert import country_alpha2_to_continent_code from uuid import uuid4 import logging import re @@ -20,13 +20,6 @@ from ..record import Record, Update from ..record.geo import GeoCodes from .base import BaseProvider -# TODO: remove when Python 2.x is no longer supported -try: # pragma: no cover - cmp -except NameError: # pragma: no cover - def cmp(x, y): - return (x > y) - (x < y) - octal_re = re.compile(r'\\(\d\d\d)') @@ -155,7 +148,7 @@ class _Route53Record(object): } } - # NOTE: we're using __hash__ and __cmp__ methods that consider + # NOTE: we're using __hash__ and ordering methods that consider # _Route53Records equivalent if they have the same class, fqdn, and _type. # Values are ignored. This is useful when computing diffs/changes. @@ -163,17 +156,34 @@ class _Route53Record(object): '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 __eq__(self, other): + '''Sub-classes should call up to this and return its value if true. + When it's false they should compute their own __eq__, same for other + ordering methods.''' + return self.__class__.__name__ == other.__class__.__name__ and \ + self.fqdn == other.fqdn and \ + self._type == other._type + + def __ne__(self, other): + return self.__class__.__name__ != other.__class__.__name__ or \ + self.fqdn != other.fqdn or \ + self._type != other._type + + def __lt__(self, other): + return (((self.__class__.__name__, self.fqdn, self._type)) < + ((other.__class__.__name__, other.fqdn, other._type))) + + def __le__(self, other): + return (((self.__class__.__name__, self.fqdn, self._type)) <= + ((other.__class__.__name__, other.fqdn, other._type))) + + def __gt__(self, other): + return (((self.__class__.__name__, self.fqdn, self._type)) > + ((other.__class__.__name__, other.fqdn, other._type))) + + def __ge__(self, other): + return (((self.__class__.__name__, self.fqdn, self._type)) >= + ((other.__class__.__name__, other.fqdn, other._type))) def __repr__(self): return '_Route53Record<{} {} {} {}>'.format(self.fqdn, self._type, @@ -514,11 +524,50 @@ class _Route53GeoRecord(_Route53Record): return '{}:{}:{}'.format(self.fqdn, self._type, self.geo.code).__hash__() - def __cmp__(self, other): - ret = super(_Route53GeoRecord, self).__cmp__(other) - if ret != 0: - return ret - return cmp(self.geo.code, other.geo.code) + def __eq__(self, other): + return super(_Route53GeoRecord, self).__eq__(other) and \ + self.geo.code == other.geo.code + + def __ne__(self, other): + # super will handle class != class, so if it's true we have 2 geo + # objects with the same name and type, so just need to compare codes + return super(_Route53GeoRecord, self).__ne__(other) or \ + self.geo.code != other.geo.code + + def __lt__(self, other): + # super eq will check class, name, and type + if super(_Route53GeoRecord, self).__eq__(other): + # if it's True we're dealing with two geo's with the same name and + # type, so we just need to compare codes + return self.geo.code < other.geo.code + # Super is not equal so we'll just let it decide lt + return super(_Route53GeoRecord, self).__lt__(other) + + def __le__(self, other): + # super eq will check class, name, and type + if super(_Route53GeoRecord, self).__eq__(other): + # Just need to compare codes, everything else is equal + return self.geo.code <= other.geo.code + # Super is not equal so geo.code doesn't matter, let it decide with lt, + # can't be eq + return super(_Route53GeoRecord, self).__lt__(other) + + def __gt__(self, other): + # super eq will check class, name, and type + if super(_Route53GeoRecord, self).__eq__(other): + # Just need to compare codes, everything else is equal + return self.geo.code > other.geo.code + # Super is not equal so we'll just let it decide gt + return super(_Route53GeoRecord, self).__gt__(other) + + def __ge__(self, other): + # super eq will check class, name, and type + if super(_Route53GeoRecord, self).__eq__(other): + # Just need to compare codes, everything else is equal + return self.geo.code >= other.geo.code + # Super is not equal so geo.code doesn't matter, let it decide with gt, + # can't be eq + return super(_Route53GeoRecord, self).__gt__(other) def __repr__(self): return '_Route53GeoRecord<{} {} {} {} {}>'.format(self.fqdn, @@ -561,7 +610,10 @@ def _mod_keyer(mod): if rrset.get('GeoLocation', False): unique_id = rrset['SetIdentifier'] else: - unique_id = rrset['Name'] + try: + unique_id = '{}-{}'.format(rrset['Name'], rrset['SetIdentifier']) + except KeyError: + unique_id = rrset['Name'] # Prioritise within the action_priority, ensuring targets come first. if rrset.get('GeoLocation', False): @@ -708,7 +760,7 @@ class Route53Provider(BaseProvider): if cc == '*': # This is the default return - cn = cca_to_ctca2(cc) + cn = country_alpha2_to_continent_code(cc) try: return '{}-{}-{}'.format(cn, cc, loc['SubdivisionCode']) except KeyError: diff --git a/requirements.txt b/requirements.txt index 19d45a8..6b6af09 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,7 +10,6 @@ futures==3.2.0; python_version < '3.0' edgegrid-python==1.1.1 google-cloud-core==0.28.1 google-cloud-dns==0.29.0 -incf.countryutils==1.0 ipaddress==1.0.22 jmespath==0.9.3 msrestazure==0.6.2 diff --git a/tests/test_octodns_provider_rackspace.py b/tests/test_octodns_provider_rackspace.py index b0dcad7..3dfdd5f 100644 --- a/tests/test_octodns_provider_rackspace.py +++ b/tests/test_octodns_provider_rackspace.py @@ -40,7 +40,6 @@ with open('./tests/fixtures/rackspace-sample-recordset-page2.json') as fh: class TestRackspaceProvider(TestCase): def setUp(self): - self.maxDiff = 1000 with requests_mock() as mock: mock.post(ANY, status_code=200, text=AUTH_RESPONSE) self.provider = RackspaceProvider('identity', 'test', 'api-key', diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 87dfd09..b0ee342 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -2091,6 +2091,58 @@ class TestRoute53Records(TestCase): e.__repr__() f.__repr__() + def test_route53_record_ordering(self): + # Matches + a = _Route53Record(None, self.record_a, False) + b = _Route53Record(None, self.record_a, False) + self.assertTrue(a == b) + self.assertFalse(a != b) + self.assertFalse(a < b) + self.assertTrue(a <= b) + self.assertFalse(a > b) + self.assertTrue(a >= b) + + # Change the fqdn is greater + fqdn = _Route53Record(None, self.record_a, False, + fqdn_override='other') + self.assertFalse(a == fqdn) + self.assertTrue(a != fqdn) + self.assertFalse(a < fqdn) + self.assertFalse(a <= fqdn) + self.assertTrue(a > fqdn) + self.assertTrue(a >= fqdn) + + provider = DummyProvider() + geo_a = _Route53GeoRecord(provider, self.record_a, 'NA-US', + self.record_a.geo['NA-US'], False) + geo_b = _Route53GeoRecord(provider, self.record_a, 'NA-US', + self.record_a.geo['NA-US'], False) + self.assertTrue(geo_a == geo_b) + self.assertFalse(geo_a != geo_b) + self.assertFalse(geo_a < geo_b) + self.assertTrue(geo_a <= geo_b) + self.assertFalse(geo_a > geo_b) + self.assertTrue(geo_a >= geo_b) + + # Other base + geo_fqdn = _Route53GeoRecord(provider, self.record_a, 'NA-US', + self.record_a.geo['NA-US'], False) + geo_fqdn.fqdn = 'other' + self.assertFalse(geo_a == geo_fqdn) + self.assertTrue(geo_a != geo_fqdn) + self.assertFalse(geo_a < geo_fqdn) + self.assertFalse(geo_a <= geo_fqdn) + self.assertTrue(geo_a > geo_fqdn) + self.assertTrue(geo_a >= geo_fqdn) + + # Other class + self.assertFalse(a == geo_a) + self.assertTrue(a != geo_a) + self.assertFalse(a < geo_a) + self.assertFalse(a <= geo_a) + self.assertTrue(a > geo_a) + self.assertTrue(a >= geo_a) + def test_dynamic_value_delete(self): provider = DummyProvider() geo = _Route53DynamicValue(provider, self.record_a, 'iad', '2.2.2.2', @@ -2207,136 +2259,38 @@ class TestRoute53Records(TestCase): creating=True) self.assertEquals(18, len(route53_records)) + expected_mods = [r.mod('CREATE', []) for r in route53_records] + # Sort so that we get a consistent order and don't rely on set ordering + expected_mods.sort(key=_mod_keyer) + # Convert the route53_records into mods self.assertEquals([{ 'Action': 'CREATE', 'ResourceRecordSet': { 'HealthCheckId': 'hc42', 'Name': '_octodns-ap-southeast-1-value.unit.tests.', - 'ResourceRecords': [{ - 'Value': '1.4.1.2'}], - 'SetIdentifier': 'ap-southeast-1-001', + 'ResourceRecords': [{'Value': '1.4.1.1'}], + 'SetIdentifier': 'ap-southeast-1-000', 'TTL': 60, 'Type': 'A', - 'Weight': 2 - } + 'Weight': 2} }, { 'Action': 'CREATE', 'ResourceRecordSet': { 'HealthCheckId': 'hc42', 'Name': '_octodns-ap-southeast-1-value.unit.tests.', - 'ResourceRecords': [{ - 'Value': '1.4.1.1'}], - 'SetIdentifier': 'ap-southeast-1-000', + 'ResourceRecords': [{'Value': '1.4.1.2'}], + 'SetIdentifier': 'ap-southeast-1-001', 'TTL': 60, 'Type': 'A', - 'Weight': 2 - } - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45' - }, - 'GeoLocation': { - 'CountryCode': 'JP'}, - 'Name': 'unit.tests.', - 'SetIdentifier': '0-ap-southeast-1-AS-JP', - 'Type': 'A' - } - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-eu-central-1-pool.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'GeoLocation': { - 'CountryCode': 'US', - 'SubdivisionCode': 'FL', - }, - 'Name': 'unit.tests.', - 'SetIdentifier': '1-eu-central-1-NA-US-FL', - 'Type': 'A'} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-us-east-1-pool.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'GeoLocation': { - 'CountryCode': '*'}, - 'Name': 'unit.tests.', - 'SetIdentifier': '2-us-east-1-None', - 'Type': 'A'} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-us-east-1-pool.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'Failover': 'SECONDARY', - 'Name': '_octodns-ap-southeast-1-pool.unit.tests.', - 'SetIdentifier': 'ap-southeast-1-Secondary-us-east-1', - 'Type': 'A'} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'GeoLocation': { - 'CountryCode': 'CN'}, - 'Name': 'unit.tests.', - 'SetIdentifier': '0-ap-southeast-1-AS-CN', - 'Type': 'A'} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-us-east-1-value.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'Failover': 'PRIMARY', - 'Name': '_octodns-us-east-1-pool.unit.tests.', - 'SetIdentifier': 'us-east-1-Primary', - 'Type': 'A'} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-eu-central-1-pool.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'GeoLocation': { - 'ContinentCode': 'EU'}, - 'Name': 'unit.tests.', - 'SetIdentifier': '1-eu-central-1-EU', - 'Type': 'A'} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-eu-central-1-value.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'Failover': 'PRIMARY', - 'Name': '_octodns-eu-central-1-pool.unit.tests.', - 'SetIdentifier': 'eu-central-1-Primary', - 'Type': 'A'} + 'Weight': 2} }, { 'Action': 'CREATE', 'ResourceRecordSet': { 'Name': '_octodns-default-pool.unit.tests.', - 'ResourceRecords': [{ - 'Value': '1.1.2.1'}, - { - 'Value': '1.1.2.2'}], + 'ResourceRecords': [ + {'Value': '1.1.2.1'}, + {'Value': '1.1.2.2'}], 'TTL': 60, 'Type': 'A'} }, { @@ -2344,56 +2298,41 @@ class TestRoute53Records(TestCase): 'ResourceRecordSet': { 'HealthCheckId': 'hc42', 'Name': '_octodns-eu-central-1-value.unit.tests.', - 'ResourceRecords': [{ - 'Value': '1.3.1.2'}], + 'ResourceRecords': [{'Value': '1.3.1.1'}], + 'SetIdentifier': 'eu-central-1-000', + 'TTL': 60, + 'Type': 'A', + 'Weight': 1} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'HealthCheckId': 'hc42', + 'Name': '_octodns-eu-central-1-value.unit.tests.', + 'ResourceRecords': [{'Value': '1.3.1.2'}], 'SetIdentifier': 'eu-central-1-001', 'TTL': 60, 'Type': 'A', 'Weight': 1} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'HealthCheckId': 'hc42', - 'Name': '_octodns-eu-central-1-value.unit.tests.', - 'ResourceRecords': [{ - 'Value': '1.3.1.1'}], - 'SetIdentifier': 'eu-central-1-000', - 'TTL': 60, - 'Type': 'A', - 'Weight': 1} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'AliasTarget': { - 'DNSName': '_octodns-default-pool.unit.tests.', - 'EvaluateTargetHealth': True, - 'HostedZoneId': 'z45'}, - 'Failover': 'SECONDARY', - 'Name': '_octodns-us-east-1-pool.unit.tests.', - 'SetIdentifier': 'us-east-1-Secondary-default', - 'Type': 'A'} }, { 'Action': 'CREATE', 'ResourceRecordSet': { 'HealthCheckId': 'hc42', 'Name': '_octodns-us-east-1-value.unit.tests.', - 'ResourceRecords': [{ - 'Value': '1.5.1.2'}], - 'SetIdentifier': 'us-east-1-001', - 'TTL': 60, - 'Type': 'A', - 'Weight': 1} - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'HealthCheckId': 'hc42', - 'Name': '_octodns-us-east-1-value.unit.tests.', - 'ResourceRecords': [{ - 'Value': '1.5.1.1'}], + 'ResourceRecords': [{'Value': '1.5.1.1'}], 'SetIdentifier': 'us-east-1-000', 'TTL': 60, 'Type': 'A', 'Weight': 1} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'HealthCheckId': 'hc42', + 'Name': '_octodns-us-east-1-value.unit.tests.', + 'ResourceRecords': [{'Value': '1.5.1.2'}], + 'SetIdentifier': 'us-east-1-001', + 'TTL': 60, + 'Type': 'A', + 'Weight': 1} }, { 'Action': 'CREATE', 'ResourceRecordSet': { @@ -2405,6 +2344,39 @@ class TestRoute53Records(TestCase): 'Name': '_octodns-ap-southeast-1-pool.unit.tests.', 'SetIdentifier': 'ap-southeast-1-Primary', 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-eu-central-1-value.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'Failover': 'PRIMARY', + 'Name': '_octodns-eu-central-1-pool.unit.tests.', + 'SetIdentifier': 'eu-central-1-Primary', + 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-us-east-1-value.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'Failover': 'PRIMARY', + 'Name': '_octodns-us-east-1-pool.unit.tests.', + 'SetIdentifier': 'us-east-1-Primary', + 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-us-east-1-pool.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'Failover': 'SECONDARY', + 'Name': '_octodns-ap-southeast-1-pool.unit.tests.', + 'SetIdentifier': 'ap-southeast-1-Secondary-us-east-1', + 'Type': 'A'} }, { 'Action': 'CREATE', 'ResourceRecordSet': { @@ -2416,7 +2388,79 @@ class TestRoute53Records(TestCase): 'Name': '_octodns-eu-central-1-pool.unit.tests.', 'SetIdentifier': 'eu-central-1-Secondary-us-east-1', 'Type': 'A'} - }], [r.mod('CREATE', []) for r in route53_records]) + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-default-pool.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'Failover': 'SECONDARY', + 'Name': '_octodns-us-east-1-pool.unit.tests.', + 'SetIdentifier': 'us-east-1-Secondary-default', + 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'GeoLocation': { + 'CountryCode': 'CN'}, + 'Name': 'unit.tests.', + 'SetIdentifier': '0-ap-southeast-1-AS-CN', + 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-ap-southeast-1-pool.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'GeoLocation': { + 'CountryCode': 'JP'}, + 'Name': 'unit.tests.', + 'SetIdentifier': '0-ap-southeast-1-AS-JP', + 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-eu-central-1-pool.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'GeoLocation': { + 'ContinentCode': 'EU'}, + 'Name': 'unit.tests.', + 'SetIdentifier': '1-eu-central-1-EU', + 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-eu-central-1-pool.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'GeoLocation': { + 'CountryCode': 'US', + 'SubdivisionCode': 'FL'}, + 'Name': 'unit.tests.', + 'SetIdentifier': '1-eu-central-1-NA-US-FL', + 'Type': 'A'} + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'AliasTarget': { + 'DNSName': '_octodns-us-east-1-pool.unit.tests.', + 'EvaluateTargetHealth': True, + 'HostedZoneId': 'z45'}, + 'GeoLocation': { + 'CountryCode': '*'}, + 'Name': 'unit.tests.', + 'SetIdentifier': '2-us-east-1-None', + 'Type': 'A'} + }], expected_mods) for route53_record in route53_records: # Smoke test stringification