Improve Route53 DELETE reliability using existing rrset

octodns/provider/route53.py

@@ -136,7 +136,7 @@ class _Route53Record(object):
         values_for = getattr(self, '_values_for_{}'.format(self._type))
         self.values = values_for(record)
 
-    def mod(self, action):
+    def mod(self, action, existing_rrsets):
         return {
             'Action': action,
             'ResourceRecordSet': {
@@ -268,7 +268,7 @@ class _Route53DynamicPool(_Route53Record):
                                      self.target_name)
         return '{}-{}'.format(self.pool_name, self.mode)
 
-    def mod(self, action):
+    def mod(self, action, existing_rrsets):
         return {
             'Action': action,
             'ResourceRecordSet': {
@@ -311,7 +311,7 @@ class _Route53DynamicRule(_Route53Record):
     def identifer(self):
         return '{}-{}-{}'.format(self.index, self.pool_name, self.geo)
 
-    def mod(self, action):
+    def mod(self, action, existing_rrsets):
         rrset = {
             'AliasTarget': {
                 'DNSName': self.target_dns_name,
@@ -379,7 +379,7 @@ class _Route53DynamicValue(_Route53Record):
     def identifer(self):
         return '{}-{:03d}'.format(self.pool_name, self.index)
 
-    def mod(self, action):
+    def mod(self, action, existing_rrsets):
         return {
             'Action': action,
             'ResourceRecordSet': {
@@ -404,7 +404,7 @@ class _Route53DynamicValue(_Route53Record):
 
 class _Route53GeoDefault(_Route53Record):
 
-    def mod(self, action):
+    def mod(self, action, existing_rrsets):
         return {
             'Action': action,
             'ResourceRecordSet': {
@@ -437,15 +437,29 @@ class _Route53GeoRecord(_Route53Record):
         self.health_check_id = provider.get_health_check_id(record, value,
                                                             creating)
 
-    def mod(self, action):
+    def mod(self, action, existing_rrsets):
         geo = self.geo
+        set_identifier = geo.code
+
+        if action == 'DELETE':
+            # We deleting records try and find the original rrset so that we're
+            # 100% sure to have the complete & accurate data (this mostly
+            # ensures we have the right health check id when there's multiple
+            # potential matches)
+            for existing in existing_rrsets:
+                if set_identifier == existing.get('SetIdentifier', None):
+                    return {
+                        'Action': action,
+                        'ResourceRecordSet': existing,
+                    }
+
         rrset = {
             'Name': self.fqdn,
             'GeoLocation': {
                 'CountryCode': '*'
             },
             'ResourceRecords': [{'Value': v} for v in geo.values],
-            'SetIdentifier': geo.code,
+            'SetIdentifier': set_identifier,
             'TTL': self.ttl,
             'Type': self._type,
         }
@@ -927,11 +941,11 @@ class Route53Provider(BaseProvider):
                       len(zone.records) - before, exists)
         return exists
 
-    def _gen_mods(self, action, records):
+    def _gen_mods(self, action, records, existing_rrsets):
         '''
         Turns `_Route53*`s in to `change_resource_record_sets` `Changes`
         '''
-        return [r.mod(action) for r in records]
+        return [r.mod(action, existing_rrsets) for r in records]
 
     @property
     def health_checks(self):
@@ -1117,15 +1131,15 @@ class Route53Provider(BaseProvider):
         '''
         return _Route53Record.new(self, record, zone_id, creating)
 
-    def _mod_Create(self, change, zone_id):
+    def _mod_Create(self, change, zone_id, existing_rrsets):
         # New is the stuff that needs to be created
         new_records = self._gen_records(change.new, zone_id, creating=True)
         # Now is a good time to clear out any unused health checks since we
         # know what we'll be using going forward
         self._gc_health_checks(change.new, new_records)
-        return self._gen_mods('CREATE', new_records)
+        return self._gen_mods('CREATE', new_records, existing_rrsets)
 
-    def _mod_Update(self, change, zone_id):
+    def _mod_Update(self, change, zone_id, existing_rrsets):
         # See comments in _Route53Record for how the set math is made to do our
         # bidding here.
         existing_records = self._gen_records(change.existing, zone_id,
@@ -1148,18 +1162,18 @@ class Route53Provider(BaseProvider):
             if new_record in existing_records:
                 upserts.add(new_record)
 
-        return self._gen_mods('DELETE', deletes) + \
-            self._gen_mods('CREATE', creates) + \
-            self._gen_mods('UPSERT', upserts)
+        return self._gen_mods('DELETE', deletes, existing_rrsets) + \
+            self._gen_mods('CREATE', creates, existing_rrsets) + \
+            self._gen_mods('UPSERT', upserts, existing_rrsets)
 
-    def _mod_Delete(self, change, zone_id):
+    def _mod_Delete(self, change, zone_id, existing_rrsets):
         # Existing is the thing that needs to be deleted
         existing_records = self._gen_records(change.existing, zone_id,
                                              creating=False)
         # Now is a good time to clear out all the health checks since we know
         # we're done with them
         self._gc_health_checks(change.existing, [])
-        return self._gen_mods('DELETE', existing_records)
+        return self._gen_mods('DELETE', existing_records, existing_rrsets)
 
     def _extra_changes_update_needed(self, record, rrset):
         healthcheck_host = record.healthcheck_host
@@ -1271,10 +1285,11 @@ class Route53Provider(BaseProvider):
         batch = []
         batch_rs_count = 0
         zone_id = self._get_zone_id(desired.name, True)
+        existing_rrsets = self._load_records(zone_id)
         for c in changes:
             # Generate the mods for this change
             mod_type = getattr(self, '_mod_{}'.format(c.__class__.__name__))
-            mods = mod_type(c, zone_id)
+            mods = mod_type(c, zone_id, existing_rrsets)
 
             # Order our mods to make sure targets exist before alises point to
             # them and we CRUD in the desired order

tests/test_octodns_provider_route53.py

@@ -874,6 +874,25 @@ class TestRoute53Provider(TestCase):
                                  'CallerReference': ANY,
                              })
 
+        list_resource_record_sets_resp = {
+            'ResourceRecordSets': [{
+                'Name': 'a.unit.tests.',
+                'Type': 'A',
+                'GeoLocation': {
+                    'ContinentCode': 'NA',
+                },
+                'ResourceRecords': [{
+                    'Value': '2.2.3.4',
+                }],
+                'TTL': 61,
+            }],
+            'IsTruncated': False,
+            'MaxItems': '100',
+        }
+        stubber.add_response('list_resource_record_sets',
+                             list_resource_record_sets_resp,
+                             {'HostedZoneId': 'z42'})
+
         stubber.add_response('list_health_checks',
                              {
                                  'HealthChecks': self.health_checks,
@@ -1236,7 +1255,7 @@ class TestRoute53Provider(TestCase):
             'HealthCheckId': '44',
         })
         change = Create(record)
-        provider._mod_Create(change, 'z43')
+        provider._mod_Create(change, 'z43', [])
         stubber.assert_no_pending_responses()
 
         # gc through _mod_Update
@@ -1245,7 +1264,7 @@ class TestRoute53Provider(TestCase):
         })
         # first record is ignored for our purposes, we have to pass something
         change = Update(record, record)
-        provider._mod_Create(change, 'z43')
+        provider._mod_Create(change, 'z43', [])
         stubber.assert_no_pending_responses()
 
         # gc through _mod_Delete, expect 3 to go away, can't check order
@@ -1260,7 +1279,7 @@ class TestRoute53Provider(TestCase):
             'HealthCheckId': ANY,
         })
         change = Delete(record)
-        provider._mod_Delete(change, 'z43')
+        provider._mod_Delete(change, 'z43', [])
         stubber.assert_no_pending_responses()
 
         # gc only AAAA, leave the A's alone
@@ -1804,40 +1823,45 @@ class TestRoute53Provider(TestCase):
 
     # _get_test_plan() returns a plan with 11 modifications, 17 RRs
 
+    @patch('octodns.provider.route53.Route53Provider._load_records')
     @patch('octodns.provider.route53.Route53Provider._really_apply')
-    def test_apply_1(self, really_apply_mock):
+    def test_apply_1(self, really_apply_mock, _):
 
         # 18 RRs with max of 19 should only get applied in one call
         provider, plan = self._get_test_plan(19)
         provider.apply(plan)
         really_apply_mock.assert_called_once()
 
+    @patch('octodns.provider.route53.Route53Provider._load_records')
     @patch('octodns.provider.route53.Route53Provider._really_apply')
-    def test_apply_2(self, really_apply_mock):
+    def test_apply_2(self, really_apply_mock, _):
 
         # 18 RRs with max of 17 should only get applied in two calls
         provider, plan = self._get_test_plan(18)
         provider.apply(plan)
         self.assertEquals(2, really_apply_mock.call_count)
 
+    @patch('octodns.provider.route53.Route53Provider._load_records')
     @patch('octodns.provider.route53.Route53Provider._really_apply')
-    def test_apply_3(self, really_apply_mock):
+    def test_apply_3(self, really_apply_mock, _):
 
         # with a max of seven modifications, four calls
         provider, plan = self._get_test_plan(7)
         provider.apply(plan)
         self.assertEquals(4, really_apply_mock.call_count)
 
+    @patch('octodns.provider.route53.Route53Provider._load_records')
     @patch('octodns.provider.route53.Route53Provider._really_apply')
-    def test_apply_4(self, really_apply_mock):
+    def test_apply_4(self, really_apply_mock, _):
 
         # with a max of 11 modifications, two calls
         provider, plan = self._get_test_plan(11)
         provider.apply(plan)
         self.assertEquals(2, really_apply_mock.call_count)
 
+    @patch('octodns.provider.route53.Route53Provider._load_records')
     @patch('octodns.provider.route53.Route53Provider._really_apply')
-    def test_apply_bad(self, really_apply_mock):
+    def test_apply_bad(self, really_apply_mock, _):
 
         # with a max of 1 modifications, fail
         provider, plan = self._get_test_plan(1)
@@ -1939,6 +1963,12 @@ class TestRoute53Provider(TestCase):
             }], [r.data for r in record.dynamic.rules])
 
 
+class DummyProvider(object):
+
+    def get_health_check_id(self, *args, **kwargs):
+        return None
+
+
 class TestRoute53Records(TestCase):
     existing = Zone('unit.tests.', [])
     record_a = Record.new(existing, '', {
@@ -2005,11 +2035,6 @@ class TestRoute53Records(TestCase):
         e = _Route53GeoDefault(None, self.record_a, False)
         self.assertNotEquals(a, e)
 
-        class DummyProvider(object):
-
-            def get_health_check_id(self, *args, **kwargs):
-                return None
-
         provider = DummyProvider()
         f = _Route53GeoRecord(provider, self.record_a, 'NA-US',
                               self.record_a.geo['NA-US'], False)
@@ -2029,6 +2054,50 @@ class TestRoute53Records(TestCase):
         e.__repr__()
         f.__repr__()
 
+    def test_geo_delete(self):
+        provider = DummyProvider()
+        geo = _Route53GeoRecord(provider, self.record_a, 'NA-US',
+                                self.record_a.geo['NA-US'], False)
+
+        rrset = {
+            'GeoLocation': {
+                'CountryCode': 'US'
+            },
+            'HealthCheckId': 'x12346z',
+            'Name': 'unit.tests.',
+            'ResourceRecords': [{
+                'Value': '2.2.2.2'
+            }, {
+                'Value': '3.3.3.3'
+            }],
+            'SetIdentifier': 'NA-US',
+            'TTL': 99,
+            'Type': 'A'
+        }
+
+        candidates = [
+            # Empty, will test no SetIdentifier
+            {},
+            {
+                'SetIdentifier': 'not-a-match',
+            },
+            rrset,
+        ]
+
+        # Provide a matching rrset so that we'll just use it for the delete
+        # rathr than building up an almost identical one, note the way we'll
+        # know that we got the one we passed in is that it'll have a
+        # HealthCheckId and one that was created wouldn't since DummyProvider
+        # stubs out the lookup for them
+        mod = geo.mod('DELETE', candidates)
+        self.assertEquals('x12346z', mod['ResourceRecordSet']['HealthCheckId'])
+
+        # If we don't provide the candidate rrsets we get back exactly what we
+        # put in minus the healthcheck
+        del rrset['HealthCheckId']
+        mod = geo.mod('DELETE', [])
+        self.assertEquals(rrset, mod['ResourceRecordSet'])
+
     def test_new_dynamic(self):
         provider = Route53Provider('test', 'abc', '123')
 
@@ -2259,7 +2328,7 @@ 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])
+        }], [r.mod('CREATE', []) for r in route53_records])
 
         for route53_record in route53_records:
             # Smoke test stringification