Merge pull request #366 from github/swap-r53-batch-order

Adjust Route53 change ordering to strictly order by dependency.

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):
+        unique_id = rrset['SetIdentifier']
+    else:
+        unique_id = rrset['Name']
 
+    # Prioritise within the action_priority, ensuring targets come first.
     if rrset.get('GeoLocation', False):
-        return (action_order, 3, rrset['SetIdentifier'])
+        # 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 just a plain value, these come first
-    return (action_order, 0, 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
+
+    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):

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