From 2df87d7dfe285eaa5330f713943df448357fe5f5 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 29 Apr 2019 08:52:56 -0700 Subject: [PATCH 1/2] Improve Route53 DELETE reliability using existing rrset --- octodns/provider/route53.py | 51 +++++++++----- tests/test_octodns_provider_route53.py | 97 ++++++++++++++++++++++---- 2 files changed, 116 insertions(+), 32 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index f5185b0..a8b47b8 100644 --- a/octodns/provider/route53.py +++ b/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 diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 67fcb76..4a4f90a 100644 --- a/tests/test_octodns_provider_route53.py +++ b/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 From e4fbcf109007c9e42ffa0ba0242c565d1754dbaf Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 29 Apr 2019 09:02:45 -0700 Subject: [PATCH 2/2] Improved Route53Provider deltion for Dynamic Value too --- octodns/provider/route53.py | 17 ++++++++-- tests/test_octodns_provider_route53.py | 43 +++++++++++++++++++++++++- 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index a8b47b8..c2567bb 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -380,6 +380,19 @@ class _Route53DynamicValue(_Route53Record): return '{}-{:03d}'.format(self.pool_name, self.index) def mod(self, action, existing_rrsets): + + if action == 'DELETE': + # When 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 self.identifer == existing.get('SetIdentifier', None): + return { + 'Action': action, + 'ResourceRecordSet': existing, + } + return { 'Action': action, 'ResourceRecordSet': { @@ -442,8 +455,8 @@ class _Route53GeoRecord(_Route53Record): 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 + # When 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: diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 4a4f90a..a5d00e2 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -12,7 +12,8 @@ from mock import patch from octodns.record import Create, Delete, Record, Update from octodns.provider.route53 import Route53Provider, _Route53GeoDefault, \ - _Route53GeoRecord, _Route53Record, _mod_keyer, _octal_replace + _Route53DynamicValue, _Route53GeoRecord, _Route53Record, _mod_keyer, \ + _octal_replace from octodns.zone import Zone from helpers import GeoProvider @@ -2054,6 +2055,46 @@ class TestRoute53Records(TestCase): e.__repr__() f.__repr__() + def test_dynamic_value_delete(self): + provider = DummyProvider() + geo = _Route53DynamicValue(provider, self.record_a, 'iad', '2.2.2.2', + 1, 0, False) + + rrset = { + 'HealthCheckId': 'x12346z', + 'Name': '_octodns-iad-value.unit.tests.', + 'ResourceRecords': [{ + 'Value': '2.2.2.2' + }], + 'SetIdentifier': 'iad-000', + 'TTL': 99, + 'Type': 'A', + 'Weight': 1, + } + + 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 + rrset['HealthCheckId'] = None + mod = geo.mod('DELETE', []) + self.assertEquals(rrset, mod['ResourceRecordSet']) + def test_geo_delete(self): provider = DummyProvider() geo = _Route53GeoRecord(provider, self.record_a, 'NA-US',