From 7958233fccf9ea22d95e2fd06c48d7d0a4529e26 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 7 Oct 2019 09:17:48 -0700 Subject: [PATCH] Consistently order changes :-/ Many providers make their modifications in the order that changes comes. In python3 this causes things to be inconsistently ordered. That mostly works, but could result in hidenbugs (e.g. Route53Provider's batching could be completely different based on the order it sees changes.) Sorting changes consistently is a good thing and it shouldn't hurt situations where providers are already doing their own ordering. All-in-all more consistent is better and we have to be explicit with python 3. --- octodns/provider/plan.py | 6 +- octodns/record/__init__.py | 6 ++ tests/test_octodns_provider_cloudflare.py | 2 +- tests/test_octodns_provider_digitalocean.py | 15 ++- tests/test_octodns_provider_dnsimple.py | 27 +++++- tests/test_octodns_provider_dnsmadeeasy.py | 22 ++++- tests/test_octodns_provider_ns1.py | 10 +- tests/test_octodns_provider_ovh.py | 101 ++++++++++---------- tests/test_octodns_provider_route53.py | 4 +- 9 files changed, 130 insertions(+), 63 deletions(-) diff --git a/octodns/provider/plan.py b/octodns/provider/plan.py index d4589f2..5b8be68 100644 --- a/octodns/provider/plan.py +++ b/octodns/provider/plan.py @@ -28,7 +28,11 @@ class Plan(object): delete_pcent_threshold=MAX_SAFE_DELETE_PCENT): self.existing = existing self.desired = desired - self.changes = changes + # Sort changes to ensure we always have a consistent ordering for + # things that make assumptions about that. Many providers will do their + # own ordering to ensure things happen in a way that makes sense to + # them and/or is as safe as possible. + self.changes = sorted(changes) self.exists = exists self.update_pcent_threshold = update_pcent_threshold self.delete_pcent_threshold = delete_pcent_threshold diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index a782018..47d2e9b 100644 --- a/octodns/record/__init__.py +++ b/octodns/record/__init__.py @@ -25,6 +25,12 @@ class Change(object): 'Returns new if we have one, existing otherwise' return self.new or self.existing + def __lt__(self, other): + self_record = self.record + other_record = other.record + return ((self_record.name, self_record._type) < + (other_record.name, other_record._type)) + class Create(Change): diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 013c5f7..3581033 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -313,7 +313,7 @@ class TestCloudflareProvider(TestCase): 'dns_records/fc12ab34cd5611334422ab3322997653'), call('DELETE', '/zones/ff12ab34cd5611334422ab3322997650/' 'dns_records/fc12ab34cd5611334422ab3322997654') - ], any_order=True) + ]) def test_update_add_swap(self): provider = CloudflareProvider('test', 'email', 'token') diff --git a/tests/test_octodns_provider_digitalocean.py b/tests/test_octodns_provider_digitalocean.py index 448b26e..ebb5319 100644 --- a/tests/test_octodns_provider_digitalocean.py +++ b/tests/test_octodns_provider_digitalocean.py @@ -176,7 +176,20 @@ class TestDigitalOceanProvider(TestCase): call('GET', '/domains/unit.tests/records', {'page': 1}), # delete the initial A record call('DELETE', '/domains/unit.tests/records/11189877'), - # created at least one of the record with expected data + # created at least some of the record with expected data + call('POST', '/domains/unit.tests/records', data={ + 'data': '1.2.3.4', + 'name': '@', + 'ttl': 300, 'type': 'A'}), + call('POST', '/domains/unit.tests/records', data={ + 'data': '1.2.3.5', + 'name': '@', + 'ttl': 300, 'type': 'A'}), + call('POST', '/domains/unit.tests/records', data={ + 'data': 'ca.unit.tests.', + 'flags': 0, 'name': '@', + 'tag': 'issue', + 'ttl': 3600, 'type': 'CAA'}), call('POST', '/domains/unit.tests/records', data={ 'name': '_srv._tcp', 'weight': 20, diff --git a/tests/test_octodns_provider_dnsimple.py b/tests/test_octodns_provider_dnsimple.py index 9e8586c..e3a9b8d 100644 --- a/tests/test_octodns_provider_dnsimple.py +++ b/tests/test_octodns_provider_dnsimple.py @@ -139,7 +139,32 @@ class TestDnsimpleProvider(TestCase): provider._client._request.assert_has_calls([ # created the domain call('POST', '/domains', data={'name': 'unit.tests'}), - # created at least one of the record with expected data + # created at least some of the record with expected data + call('POST', '/zones/unit.tests/records', data={ + 'content': '1.2.3.4', + 'type': 'A', + 'name': '', + 'ttl': 300}), + call('POST', '/zones/unit.tests/records', data={ + 'content': '1.2.3.5', + 'type': 'A', + 'name': '', + 'ttl': 300}), + call('POST', '/zones/unit.tests/records', data={ + 'content': '0 issue "ca.unit.tests"', + 'type': 'CAA', + 'name': '', + 'ttl': 3600}), + call('POST', '/zones/unit.tests/records', data={ + 'content': '1 1 7491973e5f8b39d5327cd4e08bc81b05f7710b49', + 'type': 'SSHFP', + 'name': '', + 'ttl': 3600}), + call('POST', '/zones/unit.tests/records', data={ + 'content': '1 1 bf6b6825d2977c511a475bbefb88aad54a92ac73', + 'type': 'SSHFP', + 'name': '', + 'ttl': 3600}), call('POST', '/zones/unit.tests/records', data={ 'content': '20 30 foo-1.unit.tests.', 'priority': 10, diff --git a/tests/test_octodns_provider_dnsmadeeasy.py b/tests/test_octodns_provider_dnsmadeeasy.py index ea14376..ba61b94 100644 --- a/tests/test_octodns_provider_dnsmadeeasy.py +++ b/tests/test_octodns_provider_dnsmadeeasy.py @@ -149,7 +149,27 @@ class TestDnsMadeEasyProvider(TestCase): call('POST', '/', data={'name': 'unit.tests'}), # get all domains to build the cache call('GET', '/'), - # created at least one of the record with expected data + # created at least some of the record with expected data + call('POST', '/123123/records', data={ + 'type': 'A', + 'name': '', + 'value': '1.2.3.4', + 'ttl': 300}), + call('POST', '/123123/records', data={ + 'type': 'A', + 'name': '', + 'value': '1.2.3.5', + 'ttl': 300}), + call('POST', '/123123/records', data={ + 'type': 'ANAME', + 'name': '', + 'value': 'aname.unit.tests.', + 'ttl': 1800}), + call('POST', '/123123/records', data={ + 'name': '', + 'value': 'ca.unit.tests', + 'issuerCritical': 0, 'caaType': 'issue', + 'ttl': 3600, 'type': 'CAA'}), call('POST', '/123123/records', data={ 'name': '_srv._tcp', 'weight': 20, diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 91d1a3f..9d23806 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -400,9 +400,9 @@ class TestNs1Provider(TestCase): self.assertEquals(3, got_n) nsone_zone.loadRecord.assert_has_calls([ call('unit.tests', u'A'), - call('geo', u'A'), call('delete-me', u'A'), - ], any_order=True) + call('geo', u'A'), + ]) mock_record.assert_has_calls([ call.update(answers=[{'answer': [u'1.2.3.4'], 'meta': {}}], filters=[], @@ -410,6 +410,8 @@ class TestNs1Provider(TestCase): call.update(answers=[{u'answer': [u'1.2.3.4'], u'meta': {}}], filters=[], ttl=32), + call.delete(), + call.delete(), call.update( answers=[ {u'answer': [u'101.102.103.104'], u'meta': {}}, @@ -427,9 +429,7 @@ class TestNs1Provider(TestCase): {u'filter': u'select_first_n', u'config': {u'N': 1}}, ], ttl=34), - call.delete(), - call.delete() - ], any_order=True) + ]) def test_escaping(self): provider = Ns1Provider('test', 'api-key') diff --git a/tests/test_octodns_provider_ovh.py b/tests/test_octodns_provider_ovh.py index 1f41abf..924591f 100644 --- a/tests/test_octodns_provider_ovh.py +++ b/tests/test_octodns_provider_ovh.py @@ -382,65 +382,64 @@ class TestOvhProvider(TestCase): get_mock.side_effect = [[100], [101], [102], [103]] provider.apply(plan) wanted_calls = [ - call(u'/domain/zone/unit.tests/record', fieldType=u'TXT', - subDomain='txt', target=u'TXT text', ttl=1400), - call(u'/domain/zone/unit.tests/record', fieldType=u'DKIM', - subDomain='dkim', target=self.valid_dkim_key, - ttl=1300), - call(u'/domain/zone/unit.tests/record', fieldType=u'A', - subDomain=u'', target=u'1.2.3.4', ttl=100), - call(u'/domain/zone/unit.tests/record', fieldType=u'SRV', + call('/domain/zone/unit.tests/record', fieldType='A', + subDomain='', target='1.2.3.4', ttl=100), + call('/domain/zone/unit.tests/record', fieldType='AAAA', + subDomain='', target='1:1ec:1::1', ttl=200), + call('/domain/zone/unit.tests/record', fieldType='MX', + subDomain='', target='10 mx1.unit.tests.', ttl=400), + call('/domain/zone/unit.tests/record', fieldType='SPF', + subDomain='', + target='v=spf1 include:unit.texts.redirect ~all', + ttl=1000), + call('/domain/zone/unit.tests/record', fieldType='SSHFP', + subDomain='', + target='1 1 bf6b6825d2977c511a475bbefb88aad54a92ac73', + ttl=1100), + call('/domain/zone/unit.tests/record', fieldType='PTR', + subDomain='4', target='unit.tests.', ttl=900), + call('/domain/zone/unit.tests/record', fieldType='SRV', subDomain='_srv._tcp', - target=u'10 20 30 foo-1.unit.tests.', ttl=800), - call(u'/domain/zone/unit.tests/record', fieldType=u'SRV', + target='10 20 30 foo-1.unit.tests.', ttl=800), + call('/domain/zone/unit.tests/record', fieldType='SRV', subDomain='_srv._tcp', - target=u'40 50 60 foo-2.unit.tests.', ttl=800), - call(u'/domain/zone/unit.tests/record', fieldType=u'PTR', - subDomain='4', target=u'unit.tests.', ttl=900), - call(u'/domain/zone/unit.tests/record', fieldType=u'NS', - subDomain='www3', target=u'ns3.unit.tests.', ttl=700), - call(u'/domain/zone/unit.tests/record', fieldType=u'NS', - subDomain='www3', target=u'ns4.unit.tests.', ttl=700), - call(u'/domain/zone/unit.tests/record', - fieldType=u'SSHFP', subDomain=u'', ttl=1100, - target=u'1 1 bf6b6825d2977c511a475bbefb88a' - u'ad54' - u'a92ac73', - ), - call(u'/domain/zone/unit.tests/record', fieldType=u'AAAA', - subDomain=u'', target=u'1:1ec:1::1', ttl=200), - call(u'/domain/zone/unit.tests/record', fieldType=u'MX', - subDomain=u'', target=u'10 mx1.unit.tests.', ttl=400), - call(u'/domain/zone/unit.tests/record', fieldType=u'CNAME', - subDomain='www2', target=u'unit.tests.', ttl=300), - call(u'/domain/zone/unit.tests/record', fieldType=u'SPF', - subDomain=u'', ttl=1000, - target=u'v=spf1 include:unit.texts.' - u'redirect ~all', - ), - call(u'/domain/zone/unit.tests/record', fieldType=u'A', - subDomain='sub', target=u'1.2.3.4', ttl=200), - call(u'/domain/zone/unit.tests/record', fieldType=u'NAPTR', - subDomain='naptr', ttl=500, - target=u'10 100 "S" "SIP+D2U" "!^.*$!sip:' - u'info@bar' - u'.example.com!" .' - ), - call(u'/domain/zone/unit.tests/refresh')] - - post_mock.assert_has_calls(wanted_calls, any_order=True) + target='40 50 60 foo-2.unit.tests.', ttl=800), + call('/domain/zone/unit.tests/record', fieldType='DKIM', + subDomain='dkim', + target='p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCxLaG' + '16G4SaEcXVdiIxTg7gKSGbHKQLm30CHib1h9FzS9nkcyvQSyQj1r' + 'MFyqC//tft3ohx3nvJl+bGCWxdtLYDSmir9PW54e5CTdxEh8MWRk' + 'BO3StF6QG/tAh3aTGDmkqhIJGLb87iHvpmVKqURmEUzJPv5KPJfW' + 'LofADI+q9lQIDAQAB', ttl=1300), + call('/domain/zone/unit.tests/record', fieldType='NAPTR', + subDomain='naptr', + target='10 100 "S" "SIP+D2U" "!^.*$!sip:info@bar.exam' + 'ple.com!" .', ttl=500), + call('/domain/zone/unit.tests/record', fieldType='A', + subDomain='sub', target='1.2.3.4', ttl=200), + call('/domain/zone/unit.tests/record', fieldType='TXT', + subDomain='txt', target='TXT text', ttl=1400), + call('/domain/zone/unit.tests/record', fieldType='CNAME', + subDomain='www2', target='unit.tests.', ttl=300), + call('/domain/zone/unit.tests/record', fieldType='NS', + subDomain='www3', target='ns3.unit.tests.', ttl=700), + call('/domain/zone/unit.tests/record', fieldType='NS', + subDomain='www3', target='ns4.unit.tests.', ttl=700), + call('/domain/zone/unit.tests/refresh')] + + post_mock.assert_has_calls(wanted_calls) # Get for delete calls wanted_get_calls = [ - call(u'/domain/zone/unit.tests/record', fieldType=u'TXT', - subDomain='txt'), - call(u'/domain/zone/unit.tests/record', fieldType=u'DKIM', - subDomain='dkim'), call(u'/domain/zone/unit.tests/record', fieldType=u'A', subDomain=u''), + call(u'/domain/zone/unit.tests/record', fieldType=u'DKIM', + subDomain='dkim'), call(u'/domain/zone/unit.tests/record', fieldType=u'A', - subDomain='fake')] - get_mock.assert_has_calls(wanted_get_calls, any_order=True) + subDomain='fake'), + call(u'/domain/zone/unit.tests/record', fieldType=u'TXT', + subDomain='txt')] + get_mock.assert_has_calls(wanted_get_calls) # 4 delete calls for update and delete delete_mock.assert_has_calls( [call(u'/domain/zone/unit.tests/record/100'), diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index b0ee342..7691804 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -1882,10 +1882,10 @@ class TestRoute53Provider(TestCase): @patch('octodns.provider.route53.Route53Provider._really_apply') def test_apply_3(self, really_apply_mock, _): - # with a max of seven modifications, four calls + # with a max of seven modifications, three calls provider, plan = self._get_test_plan(7) provider.apply(plan) - self.assertEquals(4, really_apply_mock.call_count) + self.assertEquals(3, really_apply_mock.call_count) @patch('octodns.provider.route53.Route53Provider._load_records') @patch('octodns.provider.route53.Route53Provider._really_apply')