diff --git a/octodns/__init__.py b/octodns/__init__.py index b6287e5..94ef299 100644 --- a/octodns/__init__.py +++ b/octodns/__init__.py @@ -1,6 +1,4 @@ -''' -OctoDNS: DNS as code - Tools for managing DNS across multiple providers -''' +'OctoDNS: DNS as code - Tools for managing DNS across multiple providers' from __future__ import absolute_import, division, print_function, \ unicode_literals diff --git a/octodns/zone.py b/octodns/zone.py index 9d405bb..74e5d9e 100644 --- a/octodns/zone.py +++ b/octodns/zone.py @@ -5,6 +5,7 @@ from __future__ import absolute_import, division, print_function, \ unicode_literals +from collections import defaultdict from logging import getLogger import re @@ -39,13 +40,19 @@ class Zone(object): # Force everyting to lowercase just to be safe self.name = str(name).lower() if name else name self.sub_zones = sub_zones - self.records = set() + # We're grouping by node, it allows us to efficently search for + # duplicates and detect when CNAMEs co-exist with other records + self._records = defaultdict(set) # optional leading . to match empty hostname # optional trailing . b/c some sources don't have it on their fqdn self._name_re = re.compile('\.?{}?$'.format(name)) self.log.debug('__init__: zone=%s, sub_zones=%s', self, sub_zones) + @property + def records(self): + return set([r for _, node in self._records.items() for r in node]) + def hostname_from_fqdn(self, fqdn): return self._name_re.sub('', fqdn) @@ -53,9 +60,6 @@ class Zone(object): name = record.name last = name.split('.')[-1] - if replace and record in self.records: - self.records.remove(record) - if last in self.sub_zones: if name != last: # it's a record for something under a sub-zone @@ -67,19 +71,30 @@ class Zone(object): raise SubzoneRecordException('Record {} a managed sub-zone ' 'and not of type NS' .format(record.fqdn)) - # TODO: this is pretty inefficent - for existing in self.records: - if record == existing: - raise DuplicateRecordException('Duplicate record {}, type {}' - .format(record.fqdn, - record._type)) - elif name == existing.name and (record._type == 'CNAME' or - existing._type == 'CNAME'): - raise InvalidNodeException('Invalid state, CNAME at {} ' - 'cannot coexist with other records' - .format(record.fqdn)) - self.records.add(record) + if replace: + # will remove it if it exists + self._records[name].discard(record) + + node = self._records[name] + if record in node: + # We already have a record at this node of this type + raise DuplicateRecordException('Duplicate record {}, type {}' + .format(record.fqdn, + record._type)) + elif ((record._type == 'CNAME' and len(node) > 0) or + ('CNAME' in map(lambda r: r._type, node))): + # We're adding a CNAME to existing records or adding to an existing + # CNAME + raise InvalidNodeException('Invalid state, CNAME at {} cannot ' + 'coexist with other records' + .format(record.fqdn)) + + node.add(record) + + def _remove_record(self, record): + 'Only for use in tests' + self._records[record.name].discard(record) def changes(self, desired, target): self.log.debug('changes: zone=%s, target=%s', self, target) diff --git a/tests/test_octodns_provider_cloudflare.py b/tests/test_octodns_provider_cloudflare.py index 3f652a1..5dcae30 100644 --- a/tests/test_octodns_provider_cloudflare.py +++ b/tests/test_octodns_provider_cloudflare.py @@ -33,7 +33,7 @@ class TestCloudflareProvider(TestCase): })) for record in list(expected.records): if record.name == 'sub' and record._type == 'NS': - expected.records.remove(record) + expected._remove_record(record) break empty = {'result': [], 'result_info': {'count': 0, 'per_page': 0}} diff --git a/tests/test_octodns_provider_dnsimple.py b/tests/test_octodns_provider_dnsimple.py index 1f62bfd..aed1e8b 100644 --- a/tests/test_octodns_provider_dnsimple.py +++ b/tests/test_octodns_provider_dnsimple.py @@ -33,7 +33,7 @@ class TestDnsimpleProvider(TestCase): })) for record in list(expected.records): if record.name == 'sub' and record._type == 'NS': - expected.records.remove(record) + expected._remove_record(record) break def test_populate(self): diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 0398459..ce1353b 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -196,7 +196,8 @@ class TestNs1Provider(TestCase): provider = Ns1Provider('test', 'api-key') desired = Zone('unit.tests.', []) - desired.records.update(self.expected) + for r in self.expected: + desired.add_record(r) plan = provider.plan(desired) # everything except the root NS diff --git a/tests/test_octodns_provider_powerdns.py b/tests/test_octodns_provider_powerdns.py index 01e7d83..5fcd80a 100644 --- a/tests/test_octodns_provider_powerdns.py +++ b/tests/test_octodns_provider_powerdns.py @@ -253,7 +253,7 @@ class TestPowerDnsProvider(TestCase): plan = provider.plan(expected) self.assertFalse(plan) # remove it now that we don't need the unrelated change any longer - expected.records.remove(unrelated_record) + expected._remove_record(unrelated_record) # ttl diff with requests_mock() as mock: diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index cad58f8..be624ff 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -372,11 +372,11 @@ class TestRoute53Provider(TestCase): # Delete by monkey patching in a populate that includes an extra record def add_extra_populate(existing, target, lenient): for record in self.expected.records: - existing.records.add(record) + existing.add_record(record) record = Record.new(existing, 'extra', {'ttl': 99, 'type': 'A', 'values': ['9.9.9.9']}) - existing.records.add(record) + existing.add_record(record) provider.populate = add_extra_populate change_resource_record_sets_params = { @@ -409,7 +409,7 @@ class TestRoute53Provider(TestCase): def mod_geo_populate(existing, target, lenient): for record in self.expected.records: if record._type != 'A' or not record.geo: - existing.records.add(record) + existing.add_record(record) record = Record.new(existing, '', { 'ttl': 61, 'type': 'A', @@ -420,7 +420,7 @@ class TestRoute53Provider(TestCase): 'NA-US-KY': ['7.2.3.4'] } }) - existing.records.add(record) + existing.add_record(record) provider.populate = mod_geo_populate change_resource_record_sets_params = { @@ -505,7 +505,7 @@ class TestRoute53Provider(TestCase): def mod_add_geo_populate(existing, target, lenient): for record in self.expected.records: if record._type != 'A' or record.geo: - existing.records.add(record) + existing.add_record(record) record = Record.new(existing, 'simple', { 'ttl': 61, 'type': 'A', @@ -514,7 +514,7 @@ class TestRoute53Provider(TestCase): 'OC': ['3.2.3.4', '4.2.3.4'], } }) - existing.records.add(record) + existing.add_record(record) provider.populate = mod_add_geo_populate change_resource_record_sets_params = { diff --git a/tests/test_octodns_zone.py b/tests/test_octodns_zone.py index f310397..8d75100 100644 --- a/tests/test_octodns_zone.py +++ b/tests/test_octodns_zone.py @@ -77,7 +77,7 @@ class TestZone(TestCase): # add a record, delete a record -> [Delete, Create] c = ARecord(before, 'c', {'ttl': 42, 'value': '1.1.1.1'}) after.add_record(c) - after.records.remove(b) + after._remove_record(b) self.assertEquals(after.records, set([a, c])) changes = before.changes(after, target) self.assertEquals(2, len(changes))