From d49bf262205e525a484949a2dd489c58011ff559 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 14 May 2019 20:25:14 -0700 Subject: [PATCH 1/7] Handle Route53 extra check much more thoroughly by breaking down name Also adds thorough tests --- octodns/provider/route53.py | 20 ++++++++++++++++---- tests/test_octodns_provider_route53.py | 26 +++++++++++++++++++++++--- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index e8b11fa..1516f43 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -1250,15 +1250,27 @@ class Route53Provider(BaseProvider): '%s', record.fqdn, record._type) fqdn = record.fqdn + _type = record._type # loop through all the r53 rrsets for rrset in self._load_records(zone_id): name = rrset['Name'] + # Break off the first piece of the name, it'll let us figure out if + # this is an rrset we're interested in. + maybe_meta, rest = name.split('.', 1) - if record._type == rrset['Type'] and name.endswith(fqdn) and \ - name.startswith('_octodns-') and '-value.' in name and \ - '-default-' not in name and \ - self._extra_changes_update_needed(record, rrset): + if not maybe_meta.startswith('_octodns-') or \ + not maybe_meta.endswith('-value') or \ + '-default-' in name: + # We're only interested in non-default dynamic value records, + # as that's where healthchecks live + continue + + if rest != fqdn or _type != rrset['Type']: + # rrset isn't for the current record + continue + + if self._extra_changes_update_needed(record, rrset): # no good, doesn't have the right health check, needs an update self.log.info('_extra_changes_dynamic_needs_update: ' 'health-check caused update of %s:%s', diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index added7f..dbd443f 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -1673,7 +1673,7 @@ class TestRoute53Provider(TestCase): desired.add_record(record) list_resource_record_sets_resp = { 'ResourceRecordSets': [{ - # other name + # Not dynamic value and other name 'Name': 'unit.tests.', 'Type': 'A', 'GeoLocation': { @@ -1684,7 +1684,7 @@ class TestRoute53Provider(TestCase): }], 'TTL': 61, }, { - # matching name, other type + # Not dynamic value, matching name, other type 'Name': 'a.unit.tests.', 'Type': 'AAAA', 'ResourceRecords': [{ @@ -1693,7 +1693,7 @@ class TestRoute53Provider(TestCase): 'TTL': 61, }, { # default value pool - 'Name': '_octodns-default-pool.a.unit.tests.', + 'Name': '_octodns-default-value.a.unit.tests.', 'Type': 'A', 'GeoLocation': { 'CountryCode': '*', @@ -1702,6 +1702,26 @@ class TestRoute53Provider(TestCase): 'Value': '1.2.3.4', }], 'TTL': 61, + }, { + # different record + 'Name': '_octodns-two-value.other.unit.tests.', + 'Type': 'A', + 'GeoLocation': { + 'CountryCode': '*', + }, + 'ResourceRecords': [{ + 'Value': '1.2.3.4', + }], + 'TTL': 61, + }, { + # same everything, but different type + 'Name': '_octodns-one-value.a.unit.tests.', + 'Type': 'AAAA', + 'ResourceRecords': [{ + 'Value': '2001:0db8:3c4d:0015:0000:0000:1a2f:1a4b' + }], + 'TTL': 61, + 'HealthCheckId': '42', }, { # match 'Name': '_octodns-one-value.a.unit.tests.', From ee0416de9a77064153b4283d6d4a709c4510125a Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 14 May 2019 20:32:36 -0700 Subject: [PATCH 2/7] Cover more Route53 extra check edge cases and ensure it tests what we're after --- tests/test_octodns_provider_route53.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index dbd443f..849ea2b 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -1683,6 +1683,9 @@ class TestRoute53Provider(TestCase): 'Value': '1.2.3.4', }], 'TTL': 61, + # All the non-matches have a different Id so we'll fail if they + # match + 'HealthCheckId': '33', }, { # Not dynamic value, matching name, other type 'Name': 'a.unit.tests.', @@ -1691,6 +1694,7 @@ class TestRoute53Provider(TestCase): 'Value': '2001:0db8:3c4d:0015:0000:0000:1a2f:1a4b' }], 'TTL': 61, + 'HealthCheckId': '33', }, { # default value pool 'Name': '_octodns-default-value.a.unit.tests.', @@ -1702,6 +1706,7 @@ class TestRoute53Provider(TestCase): 'Value': '1.2.3.4', }], 'TTL': 61, + 'HealthCheckId': '33', }, { # different record 'Name': '_octodns-two-value.other.unit.tests.', @@ -1713,6 +1718,7 @@ class TestRoute53Provider(TestCase): 'Value': '1.2.3.4', }], 'TTL': 61, + 'HealthCheckId': '33', }, { # same everything, but different type 'Name': '_octodns-one-value.a.unit.tests.', @@ -1721,7 +1727,16 @@ class TestRoute53Provider(TestCase): 'Value': '2001:0db8:3c4d:0015:0000:0000:1a2f:1a4b' }], 'TTL': 61, - 'HealthCheckId': '42', + 'HealthCheckId': '33', + }, { + # same everything, sub + 'Name': '_octodns-one-value.sub.a.unit.tests.', + 'Type': 'A', + 'ResourceRecords': [{ + 'Value': '1.2.3.4', + }], + 'TTL': 61, + 'HealthCheckId': '33', }, { # match 'Name': '_octodns-one-value.a.unit.tests.', From 53c2b8d19434b3049accbd7e3fdc08dddde49bec Mon Sep 17 00:00:00 2001 From: ItsAlex Date: Wed, 15 May 2019 12:30:11 +0200 Subject: [PATCH 3/7] fix: prevent digital ocean provider to crash if records type is not supported --- octodns/provider/digitalocean.py | 4 ++++ tests/fixtures/digitalocean-page-1.json | 11 +++++++++++ 2 files changed, 15 insertions(+) diff --git a/octodns/provider/digitalocean.py b/octodns/provider/digitalocean.py index 98a78ad..e192543 100644 --- a/octodns/provider/digitalocean.py +++ b/octodns/provider/digitalocean.py @@ -223,6 +223,10 @@ class DigitalOceanProvider(BaseProvider): values = defaultdict(lambda: defaultdict(list)) for record in self.zone_records(zone): _type = record['type'] + if _type not in self.SUPPORTS: + self.log.warning('populate: skipping unsupported %s record', + _type) + continue values[record['name']][record['type']].append(record) before = len(zone.records) diff --git a/tests/fixtures/digitalocean-page-1.json b/tests/fixtures/digitalocean-page-1.json index db231ba..c931411 100644 --- a/tests/fixtures/digitalocean-page-1.json +++ b/tests/fixtures/digitalocean-page-1.json @@ -1,5 +1,16 @@ { "domain_records": [{ + "id": null, + "type": "SOA", + "name": "@", + "data": null, + "priority": null, + "port": null, + "ttl": null, + "weight": null, + "flags": null, + "tag": null + }, { "id": 11189874, "type": "NS", "name": "@", From 1c08a4d58ea1e9129c660b74c1eac2f47d051894 Mon Sep 17 00:00:00 2001 From: Theo Julienne Date: Tue, 28 May 2019 14:03:33 +1000 Subject: [PATCH 4/7] Adjust Route53 change ordering to strictly order by dependency. --- octodns/provider/route53.py | 76 +++++++++++++++++++------- tests/test_octodns_provider_route53.py | 62 +++++++++++++++------ 2 files changed, 99 insertions(+), 39 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 1516f43..4cf7c99 100644 --- a/octodns/provider/route53.py +++ b/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): - return (action_order, 3, rrset['SetIdentifier']) + unique_id = rrset['SetIdentifier'] + else: + unique_id = rrset['Name'] + + # Prioritise within the action_priority, ensuring targets come first. + if rrset.get('GeoLocation', False): + # 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 a primary, we reference values (P0). + record_priority = 1 + else: + # We're just a plain value, has no dependencies so first. + record_priority = 0 - # We're just a plain value, these come first - return (action_order, 0, rrset['Name']) + 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): diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 849ea2b..265a0a7 100644 --- a/tests/test_octodns_provider_route53.py +++ b/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 From 003e8651cefbad2ba90594d35a86b350d8974bf9 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 16 Jul 2019 06:14:45 -0700 Subject: [PATCH 5/7] Drop dynamic record value weight to 0-16 That's all Dyn supports and it's cleaner to match it than to scale dyn since we'd lose precision we can't get back during populate. --- octodns/record/__init__.py | 2 +- tests/config/dynamic.tests.yaml | 6 +++--- tests/config/split/dynamic.tests./a.yaml | 2 +- tests/config/split/dynamic.tests./cname.yaml | 4 ++-- tests/test_octodns_record.py | 4 ++-- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index dca6100..83632bc 100644 --- a/octodns/record/__init__.py +++ b/octodns/record/__init__.py @@ -514,7 +514,7 @@ class _DynamicMixin(object): try: weight = value['weight'] weight = int(weight) - if weight < 1 or weight > 255: + if weight < 1 or weight > 15: reasons.append('invalid weight "{}" in pool "{}" ' 'value {}'.format(weight, _id, value_num)) diff --git a/tests/config/dynamic.tests.yaml b/tests/config/dynamic.tests.yaml index 3d806f9..f826880 100644 --- a/tests/config/dynamic.tests.yaml +++ b/tests/config/dynamic.tests.yaml @@ -19,7 +19,7 @@ a: - value: 6.6.6.6 weight: 10 - value: 5.5.5.5 - weight: 25 + weight: 15 rules: - geos: - EU-GB @@ -90,9 +90,9 @@ cname: sea: values: - value: target-sea-1.unit.tests. - weight: 100 + weight: 10 - value: target-sea-2.unit.tests. - weight: 175 + weight: 14 rules: - geos: - EU-GB diff --git a/tests/config/split/dynamic.tests./a.yaml b/tests/config/split/dynamic.tests./a.yaml index fd748b4..f182df6 100644 --- a/tests/config/split/dynamic.tests./a.yaml +++ b/tests/config/split/dynamic.tests./a.yaml @@ -23,7 +23,7 @@ a: fallback: null values: - value: 5.5.5.5 - weight: 25 + weight: 15 - value: 6.6.6.6 weight: 10 rules: diff --git a/tests/config/split/dynamic.tests./cname.yaml b/tests/config/split/dynamic.tests./cname.yaml index a84c202..ff85955 100644 --- a/tests/config/split/dynamic.tests./cname.yaml +++ b/tests/config/split/dynamic.tests./cname.yaml @@ -21,9 +21,9 @@ cname: fallback: null values: - value: target-sea-1.unit.tests. - weight: 100 + weight: 10 - value: target-sea-2.unit.tests. - weight: 175 + weight: 14 rules: - geos: - EU-GB diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 53bc5e7..2b11364 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -2460,7 +2460,7 @@ class TestDynamicRecords(TestCase): 'weight': 1, 'value': '6.6.6.6', }, { - 'weight': 256, + 'weight': 16, 'value': '7.7.7.7', }], }, @@ -2484,7 +2484,7 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(['invalid weight "256" in pool "three" value 2'], + self.assertEquals(['invalid weight "16" in pool "three" value 2'], ctx.exception.reasons) # invalid non-int weight From 0040a51f112b666cd6b0199946db852343086b63 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 16 Jul 2019 06:24:43 -0700 Subject: [PATCH 6/7] v0.9.6 version bump and CHANGELOG updates --- CHANGELOG.md | 9 +++++++++ octodns/__init__.py | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index adb1f8c..6e2b243 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,12 @@ +## v0.9.6 - 2019-07-16 - The little one that fixes stuff from the big one + +* Reduced dynamic record value weight range to 0-15 so that Dyn and Route53 + match up behaviors. Dyn is limited to 0-15 and scaling that up would lose + resolution that couldn't be recovered during populate. +* Addressed issues with Route53 change set ordering for dynamic records +* Ignore unsupported record types in DigitalOceanProvider +* Fix bugs in Route53 extra changes handling and health check managagement + ## v0.9.5 - 2019-05-06 - The big one, with all the dynamic stuff * dynamic record support, essentially a v2 version of geo records with a lot diff --git a/octodns/__init__.py b/octodns/__init__.py index 939c293..6422577 100644 --- a/octodns/__init__.py +++ b/octodns/__init__.py @@ -3,4 +3,4 @@ from __future__ import absolute_import, division, print_function, \ unicode_literals -__VERSION__ = '0.9.5' +__VERSION__ = '0.9.6' From 4ae3807627f08de53746c1f01045518632324f5b Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 16 Jul 2019 07:03:50 -0700 Subject: [PATCH 7/7] Render README on pypi as markdown, update twine, fix a couple README bits --- README.md | 4 ++-- requirements-dev.txt | 2 +- setup.py | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a3f3eae..163c723 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ - + ## DNS as code - Tools for managing DNS across multiple providers @@ -275,4 +275,4 @@ GitHub® and its stylized versions and the Invertocat mark are GitHub's Trademar ## Authors -OctoDNS was designed and authored by [Ross McFarland](https://github.com/ross) and [Joe Williams](https://github.com/joewilliams). It is now maintained, reviewed, and tested by Ross, Joe, and the rest of the Site Reliability Engineering team at GitHub. +OctoDNS was designed and authored by [Ross McFarland](https://github.com/ross) and [Joe Williams](https://github.com/joewilliams). It is now maintained, reviewed, and tested by Traffic Engineering team at GitHub. diff --git a/requirements-dev.txt b/requirements-dev.txt index 1afee06..77dd50c 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -6,4 +6,4 @@ pycountry>=18.12.8 pycountry_convert>=0.7.2 pyflakes==1.6.0 requests_mock -twine==1.11.0 +twine==1.13.0 diff --git a/setup.py b/setup.py index 7a9348e..75a39d7 100644 --- a/setup.py +++ b/setup.py @@ -41,6 +41,7 @@ setup( ], license='MIT', long_description=open('README.md').read(), + long_description_content_type='text/markdown', name='octodns', packages=find_packages(), url='https://github.com/github/octodns',