diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 1516f43..4cf7c99 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -519,37 +519,71 @@ 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. This ensures that Route53 won't see a RRSet that - # targets another that hasn't been seen yet. I.e. targets must come before - # things that target them. We sort on types of things rather than - # explicitly looking for targeting relationships since that's sufficent and - # easier to grok/do. + # Route53 requires that changes are ordered such that a target of an + # AliasTarget is created or upserted prior to the record that targets it. + # This is complicated by "UPSERT" appearing to be implemented as "DELETE" + # before all changes, followed by a "CREATE", internally in the AWS API. + # Because of this, we order changes as follows: + # - Delete any records that we wish to delete that are GEOS + # (because they are never targetted by anything) + # - Delete any records that we wish to delete that are SECONDARY + # (because they are no longer targetted by GEOS) + # - Delete any records that we wish to delete that are PRIMARY + # (because they are no longer targetted by SECONDARY) + # - Delete any records that we wish to delete that are VALUES + # (because they are no longer targetted by PRIMARY) + # - CREATE/UPSERT any records that are VALUES + # (because they don't depend on other records) + # - CREATE/UPSERT any records that are PRIMARY + # (because they always point to VALUES which now exist) + # - CREATE/UPSERT any records that are SECONDARY + # (because they now have PRIMARY records to target) + # - CREATE/UPSERT any records that are GEOS + # (because they now have all their PRIMARY pools to target) + # - :tada: + # + # In theory we could also do this based on actual target reference + # checking, but that's more complex. Since our rules have a known + # dependency order, we just rely on that. + # Get the unique ID from the name/id to get a consistent ordering. if rrset.get('GeoLocation', False): - return (action_order, 3, rrset['SetIdentifier']) + unique_id = rrset['SetIdentifier'] + else: + unique_id = rrset['Name'] + + # Prioritise within the action_priority, ensuring targets come first. + if rrset.get('GeoLocation', False): + # Geos reference pools, so they come last. + record_priority = 3 elif rrset.get('AliasTarget', False): # We use an alias if rrset.get('Failover', False) == 'SECONDARY': - # We're a secondary we'll ref primaries - return (action_order, 2, rrset['Name']) + # We're a secondary, which reference the primary (failover, P1). + record_priority = 2 else: - # We're a primary we'll ref values - return (action_order, 1, rrset['Name']) + # We're a primary, we reference values (P0). + record_priority = 1 + else: + # We're just a plain value, has no dependencies so first. + record_priority = 0 - # We're just a plain value, these come first - return (action_order, 0, rrset['Name']) + if mod['Action'] == 'DELETE': + # Delete things first, so we can never trounce our own additions + action_priority = 0 + # Delete in the reverse order of priority, e.g. start with the deepest + # reference and work back to the values, rather than starting at the + # values (still ref'd). + record_priority = -record_priority + else: + # For CREATE and UPSERT, Route53 seems to treat them the same, so + # interleave these, keeping the reference order described above. + action_priority = 1 + + return (action_priority, record_priority, unique_id) def _parse_pool_name(n): diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 849ea2b..265a0a7 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -700,18 +700,6 @@ class TestRoute53Provider(TestCase): 'TTL': 61, 'Type': 'A' } - }, { - 'Action': 'CREATE', - 'ResourceRecordSet': { - 'GeoLocation': {'CountryCode': 'US', - 'SubdivisionCode': 'CA'}, - 'HealthCheckId': u'44', - 'Name': 'unit.tests.', - 'ResourceRecords': [{'Value': '7.2.3.4'}], - 'SetIdentifier': 'NA-US-CA', - 'TTL': 61, - 'Type': 'A' - } }, { 'Action': 'UPSERT', 'ResourceRecordSet': { @@ -735,6 +723,18 @@ class TestRoute53Provider(TestCase): 'TTL': 61, 'Type': 'A' } + }, { + 'Action': 'CREATE', + 'ResourceRecordSet': { + 'GeoLocation': {'CountryCode': 'US', + 'SubdivisionCode': 'CA'}, + 'HealthCheckId': u'44', + 'Name': 'unit.tests.', + 'ResourceRecords': [{'Value': '7.2.3.4'}], + 'SetIdentifier': 'NA-US-CA', + 'TTL': 61, + 'Type': 'A' + } }, { 'Action': 'UPSERT', 'ResourceRecordSet': { @@ -2426,7 +2426,7 @@ class TestModKeyer(TestCase): def test_mod_keyer(self): - # First "column" + # First "column" is the action priority for C/R/U # Deletes come first self.assertEquals((0, 0, 'something'), _mod_keyer({ @@ -2444,8 +2444,8 @@ class TestModKeyer(TestCase): } })) - # Then upserts - self.assertEquals((2, 0, 'last'), _mod_keyer({ + # Upserts are the same as creates + self.assertEquals((1, 0, 'last'), _mod_keyer({ 'Action': 'UPSERT', 'ResourceRecordSet': { 'Name': 'last', @@ -2455,7 +2455,7 @@ class TestModKeyer(TestCase): # Second "column" value records tested above # AliasTarget primary second (to value) - self.assertEquals((0, 1, 'thing'), _mod_keyer({ + self.assertEquals((0, -1, 'thing'), _mod_keyer({ 'Action': 'DELETE', 'ResourceRecordSet': { 'AliasTarget': 'some-target', @@ -2464,8 +2464,17 @@ class TestModKeyer(TestCase): } })) + self.assertEquals((1, 1, 'thing'), _mod_keyer({ + 'Action': 'UPSERT', + 'ResourceRecordSet': { + 'AliasTarget': 'some-target', + 'Failover': 'PRIMARY', + 'Name': 'thing', + } + })) + # AliasTarget secondary third - self.assertEquals((0, 2, 'thing'), _mod_keyer({ + self.assertEquals((0, -2, 'thing'), _mod_keyer({ 'Action': 'DELETE', 'ResourceRecordSet': { 'AliasTarget': 'some-target', @@ -2474,8 +2483,17 @@ class TestModKeyer(TestCase): } })) + self.assertEquals((1, 2, 'thing'), _mod_keyer({ + 'Action': 'UPSERT', + 'ResourceRecordSet': { + 'AliasTarget': 'some-target', + 'Failover': 'SECONDARY', + 'Name': 'thing', + } + })) + # GeoLocation fourth - self.assertEquals((0, 3, 'some-id'), _mod_keyer({ + self.assertEquals((0, -3, 'some-id'), _mod_keyer({ 'Action': 'DELETE', 'ResourceRecordSet': { 'GeoLocation': 'some-target', @@ -2483,4 +2501,12 @@ class TestModKeyer(TestCase): } })) + self.assertEquals((1, 3, 'some-id'), _mod_keyer({ + 'Action': 'UPSERT', + 'ResourceRecordSet': { + 'GeoLocation': 'some-target', + 'SetIdentifier': 'some-id', + } + })) + # The third "column" has already been tested above, Name/SetIdentifier