From 0a6b2e2e3bf17fcbd90748c00ef54ad95f4277fc Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 1 Apr 2019 10:09:43 -0700 Subject: [PATCH] Implement Route53Provider mod ordering via a sort This will ensure that deletes come before creates which are before upserts and that records that uses aliases always come after their target (though implicitly based on sorting types and not explicitly by looking at them.) --- octodns/provider/route53.py | 34 +++++++++ tests/test_octodns_provider_route53.py | 100 ++++++++++++++++++++----- 2 files changed, 116 insertions(+), 18 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 5c942f6..7291a6b 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -235,6 +235,35 @@ class _Route53GeoRecord(_Route53Record): self.values) +_mod_keyer_action_order = { + 'DELETE': '0', # Delete things first + 'CREATE': '1', # Then Create things + 'UPSERT': '2', # Upsert things last +} + + +def _mod_keyer(mod): + rrset = mod['ResourceRecordSet'] + action_order = _mod_keyer_action_order[mod['Action']] + + # We're sorting by 3 "columns", the action, the rrset type, and finally the + # name/id of the rrset + + if rrset.get('GeoLocation', False): + return '{}3{}'.format(action_order, rrset['SetIdentifier']) + elif rrset.get('AliasTarget', False): + # We use an alias + if rrset.get('Failover', False) == 'SECONDARY': + # We're a secondary we'll ref primaries + return '{}2{}'.format(action_order, rrset['Name']) + else: + # We're a primary we'll ref values + return '{}1{}'.format(action_order, rrset['Name']) + + # We're just a plain value, these come first + return '{}0{}'.format(action_order, rrset['Name']) + + class Route53Provider(BaseProvider): ''' AWS Route53 Provider @@ -827,6 +856,11 @@ class Route53Provider(BaseProvider): # Generate the mods for this change mod_type = getattr(self, '_mod_{}'.format(c.__class__.__name__)) mods = mod_type(c, zone_id) + + # Order our mods to make sure targets exist before alises point to + # them and we CRUD in the desired order + mods.sort(key=_mod_keyer) + mods_rs_count = sum( [len(m['ResourceRecordSet']['ResourceRecords']) for m in mods] ) diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 7fe8834..1d48891 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -12,7 +12,7 @@ from mock import patch from octodns.record import Create, Delete, Record, Update from octodns.provider.route53 import Route53Provider, _Route53GeoDefault, \ - _Route53GeoRecord, _Route53Record, _octal_replace + _Route53GeoRecord, _Route53Record, _mod_keyer, _octal_replace from octodns.zone import Zone from helpers import GeoProvider @@ -529,23 +529,23 @@ class TestRoute53Provider(TestCase): }, { 'Action': 'UPSERT', 'ResourceRecordSet': { - 'GeoLocation': {'CountryCode': '*'}, + 'GeoLocation': {'CountryCode': 'US'}, + 'HealthCheckId': u'43', 'Name': 'unit.tests.', - 'ResourceRecords': [{'Value': '2.2.3.4'}, - {'Value': '3.2.3.4'}], - 'SetIdentifier': 'default', + 'ResourceRecords': [{'Value': '5.2.3.4'}, + {'Value': '6.2.3.4'}], + 'SetIdentifier': 'NA-US', 'TTL': 61, 'Type': 'A' } }, { 'Action': 'UPSERT', 'ResourceRecordSet': { - 'GeoLocation': {'CountryCode': 'US'}, - 'HealthCheckId': u'43', + 'GeoLocation': {'CountryCode': '*'}, 'Name': 'unit.tests.', - 'ResourceRecords': [{'Value': '5.2.3.4'}, - {'Value': '6.2.3.4'}], - 'SetIdentifier': 'NA-US', + 'ResourceRecords': [{'Value': '2.2.3.4'}, + {'Value': '3.2.3.4'}], + 'SetIdentifier': 'default', 'TTL': 61, 'Type': 'A' } @@ -588,21 +588,21 @@ class TestRoute53Provider(TestCase): 'Changes': [{ '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'} }, { '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'} }, { @@ -1623,3 +1623,67 @@ class TestRoute53Records(TestCase): a.__repr__() e.__repr__() f.__repr__() + + +class TestModKeyer(TestCase): + + def test_mod_keyer(self): + + # First "column" + + # Deletes come first + self.assertEquals('00something', _mod_keyer({ + 'Action': 'DELETE', + 'ResourceRecordSet': { + 'Name': 'something', + } + })) + + # Creates come next + self.assertEquals('10another', _mod_keyer({ + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'Name': 'another', + } + })) + + # Then upserts + self.assertEquals('20last', _mod_keyer({ + 'Action': 'UPSERT', + 'ResourceRecordSet': { + 'Name': 'last', + } + })) + + # Second "column" value records tested above + + # AliasTarget primary second (to value) + self.assertEquals('01thing', _mod_keyer({ + 'Action': 'DELETE', + 'ResourceRecordSet': { + 'AliasTarget': 'some-target', + 'Failover': 'PRIMARY', + 'Name': 'thing', + } + })) + + # AliasTarget secondary third + self.assertEquals('02thing', _mod_keyer({ + 'Action': 'DELETE', + 'ResourceRecordSet': { + 'AliasTarget': 'some-target', + 'Failover': 'SECONDARY', + 'Name': 'thing', + } + })) + + # GeoLocation fourth + self.assertEquals('03some-id', _mod_keyer({ + 'Action': 'DELETE', + 'ResourceRecordSet': { + 'GeoLocation': 'some-target', + 'SetIdentifier': 'some-id', + } + })) + + # The third "column" has already been tested above, Name/SetIdentifier