From 2a03125830ed031e44fc1998c1609c604bc468ad Mon Sep 17 00:00:00 2001 From: slandry Date: Thu, 23 Sep 2021 00:43:18 -0400 Subject: [PATCH 01/10] properly return and cache none replies from get_zone_by_name --- octodns/provider/route53.py | 7 +- tests/test_octodns_provider_route53.py | 135 ++++++++++++++++++++++++- 2 files changed, 137 insertions(+), 5 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 2adb1f3..a6a3105 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -682,11 +682,11 @@ class Route53Provider(BaseProvider): def _get_zone_id(self, name, create=False): self.log.debug('_get_zone_id: name=%s', name) self.update_r53_zones(name) + id = None if name in self._r53_zones: id = self._r53_zones[name] self.log.debug('_get_zone_id: id=%s', id) - return id - if create: + if create and not id: ref = uuid4().hex del_set = self.delegation_set_id self.log.debug('_get_zone_id: no matching zone, creating, ' @@ -699,8 +699,7 @@ class Route53Provider(BaseProvider): resp = self._conn.create_hosted_zone(Name=name, CallerReference=ref) self._r53_zones[name] = id = resp['HostedZone']['Id'] - return id - return None + return id def _parse_geo(self, rrset): try: diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 9c9ff28..e9ec3bc 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -1847,7 +1847,7 @@ class TestRoute53Provider(TestCase): self.assertEquals([], extra) stubber.assert_no_pending_responses() - def test_plan_with_get_zones_by_name(self): + def test_plan_apply_with_get_zones_by_name_zone_not_exists(self): provider = Route53Provider( 'test', 'abc', '123', get_zones_by_name=True) @@ -1874,6 +1874,139 @@ class TestRoute53Provider(TestCase): plan = provider.plan(self.expected) self.assertEquals(9, len(plan.changes)) + create_hosted_zone_resp = { + 'HostedZone': { + 'Name': 'unit.tests.', + 'Id': 'z42', + 'CallerReference': 'abc', + }, + 'ChangeInfo': { + 'Id': 'a12', + 'Status': 'PENDING', + 'SubmittedAt': '2017-01-29T01:02:03Z', + 'Comment': 'hrm', + }, + 'DelegationSet': { + 'Id': 'b23', + 'CallerReference': 'blip', + 'NameServers': [ + 'n12.unit.tests.', + ], + }, + 'Location': 'us-east-1', + } + stubber.add_response('create_hosted_zone', + create_hosted_zone_resp, { + 'Name': 'unit.tests.', + '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, + 'IsTruncated': False, + 'MaxItems': '100', + 'Marker': '', + }) + + stubber.add_response('change_resource_record_sets', + {'ChangeInfo': { + 'Id': 'id', + 'Status': 'PENDING', + 'SubmittedAt': '2017-01-29T01:02:03Z', + }}, {'HostedZoneId': 'z42', 'ChangeBatch': ANY}) + + self.assertEquals(9, provider.apply(plan)) + stubber.assert_no_pending_responses() + + def test_plan_apply_with_get_zones_by_name_zone_exists(self): + provider = Route53Provider( + 'test', 'abc', '123', get_zones_by_name=True) + + # Use the stubber + stubber = Stubber(provider._conn) + stubber.activate() + + list_hosted_zones_by_name_resp = { + 'HostedZones': [{ + 'Id': 'z42', + 'Name': 'unit.tests.', + 'CallerReference': 'abc', + 'Config': { + 'Comment': 'string', + 'PrivateZone': False + }, + 'ResourceRecordSetCount': 123, + }, ], + 'DNSName': 'unit.tests.', + 'HostedZoneId': 'z42', + 'IsTruncated': False, + 'MaxItems': 'string' + } + + list_resource_record_sets_resp = { + 'ResourceRecordSets': [{ + 'Name': 'a.unit.tests.', + 'Type': 'A', + 'ResourceRecords': [{ + 'Value': '2.2.3.4', + }], + 'TTL': 61, + }], + 'IsTruncated': False, + 'MaxItems': '100', + } + + stubber.add_response( + 'list_hosted_zones_by_name', + list_hosted_zones_by_name_resp, + {'DNSName': 'unit.tests.', 'MaxItems': '1'} + ) + + stubber.add_response('list_resource_record_sets', + list_resource_record_sets_resp, + {'HostedZoneId': 'z42'}) + + plan = provider.plan(self.expected) + self.assertEquals(10, len(plan.changes)) + + stubber.add_response('list_health_checks', + { + 'HealthChecks': self.health_checks, + 'IsTruncated': False, + 'MaxItems': '100', + 'Marker': '', + }) + + stubber.add_response('change_resource_record_sets', + {'ChangeInfo': { + 'Id': 'id', + 'Status': 'PENDING', + 'SubmittedAt': '2017-01-29T01:02:03Z', + }}, {'HostedZoneId': 'z42', 'ChangeBatch': ANY}) + + self.assertEquals(10, provider.apply(plan)) + stubber.assert_no_pending_responses() + def test_extra_change_no_health_check(self): provider, stubber = self._get_stubbed_provider() From 5903e9ba490d21cce258aa730b4b3f4771c3bd0c Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sun, 10 Oct 2021 09:58:44 -0700 Subject: [PATCH 02/10] v0.9.14 version bump and CHANGELOG update --- CHANGELOG.md | 22 +++++++++++++++++++++- README.md | 2 +- octodns/__init__.py | 2 +- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da55eb7..b5b62e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -## v0.9.14 - 2021-??-?? - A new supports system +## v0.9.14 - 2021-10-10 - A new supports system #### Noteworthy changes @@ -25,6 +25,26 @@ new python versions and hasn't seen a release since 2010. May return with https://github.com/octodns/octodns/pull/762 +#### Stuff + +* Fully remove python 2.7 support & sims +* Dynamic record pool status flag: up/down/obey added w/provider support as + possible. +* Support for multi-value PTRs where providers allow them +* Normalize IPv6 addresses to avoid false changes and simplify providers +* Include pure-python wheel distirubtions in release builds +* Improvements and updates to AzureProvider, especially w/respect to dynamic + records. +* NS1Provider support for IPv6 monitors and general caching/performance + improvements +* Route53Provider.get_zones_by_name option to avoid paging through huge lists + and hitting rate limits +* Misc Route53Provider +* Ensure no network access during testing (helps with runtime) +* Sped up the long pole unit tests +* Misc. ConstellixProvider, DigitalOceanProvider, GCoreProvider, and + Route53Provider fixes & improvements + ## v0.9.13 - 2021-07-18 - Processors Alpha #### Noteworthy changes diff --git a/README.md b/README.md index 9b983a2..aa10518 100644 --- a/README.md +++ b/README.md @@ -357,4 +357,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 Traffic Engineering team at GitHub. +OctoDNS was designed and authored by [Ross McFarland](https://github.com/ross) and [Joe Williams](https://github.com/joewilliams). See https://github.com/octodns/octodns/graphs/contributors for a complete list of people who've contributed. diff --git a/octodns/__init__.py b/octodns/__init__.py index 16ec066..5d9f936 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.13' +__VERSION__ = '0.9.14' From 8d95d05f5f3c1162e98010f8c66d79bb45a64d92 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Tue, 12 Oct 2021 13:04:26 -0700 Subject: [PATCH 03/10] Bump upper limit on weights --- octodns/record/__init__.py | 2 +- tests/test_octodns_record.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py index 0545e79..9492eb9 100644 --- a/octodns/record/__init__.py +++ b/octodns/record/__init__.py @@ -557,7 +557,7 @@ class _DynamicMixin(object): try: weight = value['weight'] weight = int(weight) - if weight < 1 or weight > 15: + if weight < 1 or weight > 100: reasons.append(f'invalid weight "{weight}" in ' f'pool "{_id}" value {value_num}') except KeyError: diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 68d3d61..1a4a58c 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -3895,7 +3895,7 @@ class TestDynamicRecords(TestCase): 'weight': 1, 'value': '6.6.6.6', }, { - 'weight': 16, + 'weight': 101, 'value': '7.7.7.7', }], }, @@ -3919,7 +3919,7 @@ class TestDynamicRecords(TestCase): } with self.assertRaises(ValidationError) as ctx: Record.new(self.zone, 'bad', a_data) - self.assertEquals(['invalid weight "16" in pool "three" value 2'], + self.assertEquals(['invalid weight "101" in pool "three" value 2'], ctx.exception.reasons) # invalid non-int weight From 4610d177478f18d7021493783cb5da34a98312f4 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Mon, 18 Oct 2021 15:44:54 -0700 Subject: [PATCH 04/10] Refactor NS1 to breakdown large functions into smaller ones --- octodns/provider/ns1.py | 165 +++++++++++++++++++++++----------------- 1 file changed, 95 insertions(+), 70 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index a023df2..d169290 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -545,21 +545,16 @@ class Ns1Provider(BaseProvider): pass return pool_name - def _data_for_dynamic(self, _type, record): - # First make sure we have the expected filters config - if not self._valid_filter_config(record['filters'], record['domain']): - self.log.error('_data_for_dynamic: %s %s has unsupported ' - 'filters', record['domain'], _type) - raise Ns1Exception('Unrecognized advanced record') - + def _parse_pools(self, answers): # All regions (pools) will include the list of default values # (eventually) at higher priorities, we'll just add them to this set to # we'll have the complete collection. default = set() + # Fill out the pools by walking the answers and looking at their - # region. + # region (< v0.9.11) or notes (> v0.9.11). pools = defaultdict(lambda: {'fallback': None, 'values': []}) - for answer in record['answers']: + for answer in answers: meta = answer['meta'] notes = self._parse_notes(meta.get('note', '')) @@ -579,7 +574,7 @@ class Ns1Provider(BaseProvider): if meta['priority'] != 1: # Ignore all but priority 1 continue - # And use region's pool name as the pool name + # And use region's name as the pool name pool_name = self._parse_dynamic_pool_name(answer['region']) else: # > v0.9.11, use the notes-based name and consider all values @@ -603,18 +598,80 @@ class Ns1Provider(BaseProvider): if fallback is not None: pool['fallback'] = fallback + return default, pools + + def _parse_rule_geos(self, meta): + geos = set() + + for georegion in meta.get('georegion', []): + geos.add(self._REGION_TO_CONTINENT[georegion]) + + # Countries are easy enough to map, we just have to find their + # continent + # + # NOTE: Some continents need special handling since NS1 + # does not supprt them as regions. These are defined under + # _CONTINENT_TO_LIST_OF_COUNTRIES. So the countries for these + # regions will be present in meta['country']. If all the countries + # in _CONTINENT_TO_LIST_OF_COUNTRIES[] list are found, + # set the continent as the region and remove individual countries + + special_continents = dict() + for country in meta.get('country', []): + # country_alpha2_to_continent_code fails for Pitcairn ('PN'), + # United States Minor Outlying Islands ('UM') and + # Sint Maarten ('SX') + if country == 'PN': + con = 'OC' + elif country in ['SX', 'UM']: + con = 'NA' + else: + con = country_alpha2_to_continent_code(country) + + if con in self._CONTINENT_TO_LIST_OF_COUNTRIES: + special_continents.setdefault(con, set()).add(country) + else: + geos.add(f'{con}-{country}') + + for continent, countries in special_continents.items(): + if countries == self._CONTINENT_TO_LIST_OF_COUNTRIES[ + continent]: + # All countries found, so add it to geos + geos.add(continent) + else: + # Partial countries found, so just add them as-is to geos + for c in countries: + geos.add(f'{continent}-{c}') + + # States and provinces are easy too, + # just assume NA-US or NA-CA + for state in meta.get('us_state', []): + geos.add(f'NA-US-{state}') + + for province in meta.get('ca_province', []): + geos.add(f'NA-CA-{province}') + + return geos + + def _parse_rules(self, pools, regions): # The regions objects map to rules, but it's a bit fuzzy since they're # tied to pools on the NS1 side, e.g. we can only have 1 rule per pool, # that may eventually run into problems, but I don't have any use-cases # examples currently where it would rules = {} - for pool_name, region in sorted(record['regions'].items()): + for pool_name, region in sorted(regions.items()): # Get the actual pool name by removing the type pool_name = self._parse_dynamic_pool_name(pool_name) meta = region['meta'] notes = self._parse_notes(meta.get('note', '')) + # The group notes field in the UI is a `note` on the region here, + # that's where we can find our pool's fallback in < v0.9.11 anyway + if 'fallback' in notes: + # set the fallback pool name + pools[pool_name]['fallback'] = notes['fallback'] + rule_order = notes['rule-order'] try: rule = rules[rule_order] @@ -625,67 +682,24 @@ class Ns1Provider(BaseProvider): } rules[rule_order] = rule - # The group notes field in the UI is a `note` on the region here, - # that's where we can find our pool's fallback in < v0.9.11 anyway - if 'fallback' in notes: - # set the fallback pool name - pools[pool_name]['fallback'] = notes['fallback'] - - geos = set() - - for georegion in meta.get('georegion', []): - geos.add(self._REGION_TO_CONTINENT[georegion]) - - # Countries are easy enough to map, we just have to find their - # continent - # - # NOTE: Some continents need special handling since NS1 - # does not supprt them as regions. These are defined under - # _CONTINENT_TO_LIST_OF_COUNTRIES. So the countries for these - # regions will be present in meta['country']. If all the countries - # in _CONTINENT_TO_LIST_OF_COUNTRIES[] list are found, - # set the continent as the region and remove individual countries - - special_continents = dict() - for country in meta.get('country', []): - # country_alpha2_to_continent_code fails for Pitcairn ('PN'), - # United States Minor Outlying Islands ('UM') and - # Sint Maarten ('SX') - if country == 'PN': - con = 'OC' - elif country in ['SX', 'UM']: - con = 'NA' - else: - con = country_alpha2_to_continent_code(country) - - if con in self._CONTINENT_TO_LIST_OF_COUNTRIES: - special_continents.setdefault(con, set()).add(country) - else: - geos.add(f'{con}-{country}') - - for continent, countries in special_continents.items(): - if countries == self._CONTINENT_TO_LIST_OF_COUNTRIES[ - continent]: - # All countries found, so add it to geos - geos.add(continent) - else: - # Partial countries found, so just add them as-is to geos - for c in countries: - geos.add(f'{continent}-{c}') - - # States and provinces are easy too, - # just assume NA-US or NA-CA - for state in meta.get('us_state', []): - geos.add(f'NA-US-{state}') - - for province in meta.get('ca_province', []): - geos.add(f'NA-CA-{province}') - + geos = self._parse_rule_geos(meta) if geos: # There are geos, combine them with any existing geos for this # pool and recorded the sorted unique set of them rule['geos'] = sorted(set(rule.get('geos', [])) | geos) + return rules + + def _data_for_dynamic(self, _type, record): + # First make sure we have the expected filters config + if not self._valid_filter_config(record['filters'], record['domain']): + self.log.error('_data_for_dynamic: %s %s has unsupported ' + 'filters', record['domain'], _type) + raise Ns1Exception('Unrecognized advanced record') + + default, pools = self._parse_pools(record['answers']) + rules = self._parse_rules(pools, record['regions']) + # Order and convert to a list default = sorted(default) # Convert to list and order @@ -1182,10 +1196,8 @@ class Ns1Provider(BaseProvider): } answers.append(answer) - def _params_for_dynamic(self, record): + def _generate_regions(self, record): pools = record.dynamic.pools - - # Convert rules to regions has_country = False has_region = False regions = {} @@ -1267,6 +1279,10 @@ class Ns1Provider(BaseProvider): 'meta': meta, } + return has_country, has_region, regions + + def _generate_answers(self, record, regions): + pools = record.dynamic.pools existing_monitors = self._monitors_for(record) active_monitors = set() @@ -1324,6 +1340,15 @@ class Ns1Provider(BaseProvider): pool_label, pool_answers, pools, priority) + return active_monitors, answers + + def _params_for_dynamic(self, record): + # Convert rules to regions + has_country, has_region, regions = self._generate_regions(record) + + # Convert pools to answers + active_monitors, answers = self._generate_answers(record, regions) + # Update filters as necessary filters = self._get_updated_filter_chain(has_region, has_country) From 9c2a6aecc0c576e1b47de4182872e80e3a4e08ff Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Mon, 18 Oct 2021 23:15:46 -0700 Subject: [PATCH 05/10] Process rules and pools in respective functions --- octodns/provider/ns1.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index d169290..ff2ba04 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -598,6 +598,9 @@ class Ns1Provider(BaseProvider): if fallback is not None: pool['fallback'] = fallback + # Order and convert to a list + default = sorted(default) + return default, pools def _parse_rule_geos(self, meta): @@ -688,6 +691,9 @@ class Ns1Provider(BaseProvider): # pool and recorded the sorted unique set of them rule['geos'] = sorted(set(rule.get('geos', [])) | geos) + # Convert to list and order + rules = sorted(rules.values(), key=lambda r: (r['_order'], r['pool'])) + return rules def _data_for_dynamic(self, _type, record): @@ -700,12 +706,6 @@ class Ns1Provider(BaseProvider): default, pools = self._parse_pools(record['answers']) rules = self._parse_rules(pools, record['regions']) - # Order and convert to a list - default = sorted(default) - # Convert to list and order - rules = list(rules.values()) - rules.sort(key=lambda r: (r['_order'], r['pool'])) - data = { 'dynamic': { 'pools': pools, From 576a6b10e20a2a1ecf67383c91fecdc3416e760a Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Wed, 20 Oct 2021 15:28:00 -0700 Subject: [PATCH 06/10] add python_requires --- setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.py b/setup.py index 41870f8..850c110 100644 --- a/setup.py +++ b/setup.py @@ -80,6 +80,7 @@ setup( long_description_content_type='text/markdown', name='octodns', packages=find_packages(), + python_requires='>=3.6', url='https://github.com/octodns/octodns', version=octodns.__VERSION__, ) From bbcdbde679a890e5ca1d92444dab35c8d8f603d8 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Thu, 21 Oct 2021 11:52:49 -0700 Subject: [PATCH 07/10] replace virtualenv with venv --- README.md | 2 +- script/bootstrap | 11 +++++++---- script/cibuild | 4 +--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index aa10518..a1dc5d4 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ Running through the following commands will install the latest release of OctoDN ```shell $ mkdir dns $ cd dns -$ virtualenv env +$ python -m venv env ... $ source env/bin/activate $ pip install octodns diff --git a/script/bootstrap b/script/bootstrap index b135122..543d7b7 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -15,15 +15,18 @@ if [ ! -d "$VENV_NAME" ]; then if [ -z "$VENV_PYTHON" ]; then VENV_PYTHON=$(command -v python3) fi - virtualenv --python="$VENV_PYTHON" "$VENV_NAME" + "$VENV_PYTHON" -m venv "$VENV_NAME" fi . "$VENV_NAME/bin/activate" -pip install -U 'pip>=10.0.1' -pip install -r requirements.txt +# We're in the venv now, so use the first Python in $PATH. In particular, don't +# use $VENV_PYTHON - that's the Python that *created* the venv, not the python +# *inside* the venv +python -m pip install -U 'pip>=10.0.1' +python -m pip install -r requirements.txt if [ "$ENV" != "production" ]; then - pip install -r requirements-dev.txt + python -m pip install -r requirements-dev.txt fi if [ ! -L ".git/hooks/pre-commit" ]; then diff --git a/script/cibuild b/script/cibuild index a2dc527..f356a84 100755 --- a/script/cibuild +++ b/script/cibuild @@ -8,9 +8,7 @@ script/bootstrap echo "## environment & versions ######################################################" python --version -pip --version -VVER=$(virtualenv --version) -echo "virtualenv $VVER" +python -m pip --version if [ -z "$VENV_NAME" ]; then VENV_NAME="env" From ab897f65d9914f3443c57640d37c91d437b33f04 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Thu, 21 Oct 2021 13:01:32 -0700 Subject: [PATCH 08/10] Invalidate NS1 dynamic records with missing notes --- octodns/provider/ns1.py | 14 +++++++--- tests/test_octodns_provider_ns1.py | 41 +++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index ff2ba04..ba68bfd 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -771,13 +771,21 @@ class Ns1Provider(BaseProvider): def _data_for_CNAME(self, _type, record): if record.get('tier', 1) > 1: - # Advanced dynamic record - return self._data_for_dynamic(_type, record) + # Advanced record, see if it's first answer has a note + try: + first_answer_note = record['answers'][0]['meta']['note'] + except (IndexError, KeyError): + first_answer_note = '' + # If that note includes a `pool` it's a valid dynamic record + if 'pool:' in first_answer_note: + return self._data_for_dynamic(_type, record) + # If not, it can't be parsed. Let it be an empty record try: value = record['short_answers'][0] - except IndexError: + except (IndexError, KeyError): value = None + return { 'ttl': record['ttl'], 'type': _type, diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 2364d92..e368f0e 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1961,7 +1961,7 @@ class TestNs1ProviderDynamic(TestCase): 'meta': { 'priority': 1, 'weight': 12, - 'note': f'from:{catchall_pool_name}', + 'note': f'pool:iad from:{catchall_pool_name}', 'up': {}, }, 'region': catchall_pool_name, @@ -2009,6 +2009,45 @@ class TestNs1ProviderDynamic(TestCase): 'value': 'value.unit.tests.', }, data) + def test_data_for_invalid_dynamic_CNAME(self): + provider = Ns1Provider('test', 'api-key') + + # Potential setup created outside of octoDNS, so it could be missing + # notes and region names can be arbitrary + filters = provider._get_updated_filter_chain(False, False) + ns1_record = { + 'answers': [{ + 'answer': ['iad.unit.tests.'], + 'meta': { + 'priority': 1, + 'weight': 12, + 'up': {}, + }, + 'region': 'global', + }, { + 'answer': ['value.unit.tests.'], + 'meta': { + 'priority': 2, + 'up': {}, + }, + 'region': 'global', + }], + 'domain': 'foo.unit.tests', + 'filters': filters, + 'regions': { + 'global': {}, + }, + 'tier': 3, + 'ttl': 44, + 'type': 'CNAME', + } + data = provider._data_for_CNAME('CNAME', ns1_record) + self.assertEquals({ + 'ttl': 44, + 'type': 'CNAME', + 'value': None, + }, data) + @patch('ns1.rest.records.Records.retrieve') @patch('ns1.rest.zones.Zones.retrieve') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') From 5b93048e0742907b2d1f9de42781af2af1b6c151 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Thu, 21 Oct 2021 13:24:45 -0700 Subject: [PATCH 09/10] Use null/empty instead of garbage values to invalidate broken dynamic records --- octodns/provider/azuredns.py | 42 +++++++++++-------------- tests/test_octodns_provider_azuredns.py | 12 +++---- 2 files changed, 24 insertions(+), 30 deletions(-) diff --git a/octodns/provider/azuredns.py b/octodns/provider/azuredns.py index aff7a25..bbc6973 100644 --- a/octodns/provider/azuredns.py +++ b/octodns/provider/azuredns.py @@ -650,14 +650,12 @@ class AzureProvider(BaseProvider): if azrecord.a_records is None: if azrecord.target_resource.id: return self._data_for_dynamic(azrecord) - else: - # dynamic record alias is broken, return dummy value and apply - # will likely overwrite/fix it - self.log.warn('_data_for_A: Missing Traffic Manager ' - 'alias for dynamic A record %s, forcing ' - 're-link by setting an invalid value', - azrecord.fqdn) - return {'values': ['255.255.255.255']} + + # dynamic record alias is broken, return dummy value and apply + # will likely overwrite/fix it + self.log.warn('_data_for_A: Missing Traffic Manager alias for ' + 'dynamic record %s', azrecord.fqdn) + return {'values': []} return {'values': [ar.ipv4_address for ar in azrecord.a_records]} @@ -665,14 +663,12 @@ class AzureProvider(BaseProvider): if azrecord.aaaa_records is None: if azrecord.target_resource.id: return self._data_for_dynamic(azrecord) - else: - # dynamic record alias is broken, return dummy value and apply - # will likely overwrite/fix it - self.log.warn('_data_for_AAAA: Missing Traffic Manager ' - 'alias for dynamic AAAA record %s, forcing ' - 're-link by setting an invalid value', - azrecord.fqdn) - return {'values': ['::1']} + + # dynamic record alias is broken, return dummy value and apply + # will likely overwrite/fix it + self.log.warn('_data_for_AAAA: Missing Traffic Manager alias for ' + 'dynamic record %s', azrecord.fqdn) + return {'values': []} return {'values': [ar.ipv6_address for ar in azrecord.aaaa_records]} @@ -692,14 +688,12 @@ class AzureProvider(BaseProvider): if azrecord.cname_record is None: if azrecord.target_resource.id: return self._data_for_dynamic(azrecord) - else: - # dynamic record alias is broken, return dummy value and apply - # will likely overwrite/fix it - self.log.warn('_data_for_CNAME: Missing Traffic Manager ' - 'alias for dynamic CNAME record %s, forcing ' - 're-link by setting an invalid value', - azrecord.fqdn) - return {'value': 'iam.invalid.'} + + # dynamic record alias is broken, return dummy value and apply + # will likely overwrite/fix it + self.log.warn('_data_for_CNAME: Missing Traffic Manager alias for ' + 'dynamic record %s', azrecord.fqdn) + return {'value': None} return {'value': _check_endswith_dot(azrecord.cname_record.cname)} diff --git a/tests/test_octodns_provider_azuredns.py b/tests/test_octodns_provider_azuredns.py index 708b229..64bd3ec 100644 --- a/tests/test_octodns_provider_azuredns.py +++ b/tests/test_octodns_provider_azuredns.py @@ -1934,8 +1934,8 @@ class TestAzureDnsProvider(TestCase): ttl=60, target_resource=SubResource(id=None)) azrecord.name = record.name or '@' azrecord.type = f'Microsoft.Network/dnszones/{record._type}' - record2 = provider._populate_record(zone, azrecord) - self.assertEqual(record2.values, ['255.255.255.255']) + record2 = provider._populate_record(zone, azrecord, lenient=True) + self.assertEqual(record2.values, []) # test that same record gets populated back from traffic managers tm_list = provider._tm_client.profiles.list_by_resource_group @@ -2016,8 +2016,8 @@ class TestAzureDnsProvider(TestCase): ttl=60, target_resource=SubResource(id=None)) azrecord.name = record.name or '@' azrecord.type = f'Microsoft.Network/dnszones/{record._type}' - record2 = provider._populate_record(zone, azrecord) - self.assertEqual(record2.values, ['::1']) + record2 = provider._populate_record(zone, azrecord, lenient=True) + self.assertEqual(record2.values, []) # test that same record gets populated back from traffic managers tm_list = provider._tm_client.profiles.list_by_resource_group @@ -2259,8 +2259,8 @@ class TestAzureDnsProvider(TestCase): azrecord.name = record1.name or '@' azrecord.type = f'Microsoft.Network/dnszones/{record1._type}' - record2 = provider._populate_record(zone, azrecord) - self.assertEqual(record2.value, 'iam.invalid.') + record2 = provider._populate_record(zone, azrecord, lenient=True) + self.assertIsNone(record2.value) change = Update(record2, record1) provider._apply_Update(change) From 7cd98591f4e7cbf139ba6a35f5cb5d55fe6379d0 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 22 Oct 2021 13:13:05 -0700 Subject: [PATCH 10/10] Add warn for unparsable dynamic records --- octodns/provider/ns1.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index ba68bfd..d259f93 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -780,11 +780,15 @@ class Ns1Provider(BaseProvider): if 'pool:' in first_answer_note: return self._data_for_dynamic(_type, record) # If not, it can't be parsed. Let it be an empty record - - try: - value = record['short_answers'][0] - except (IndexError, KeyError): + self.log.warn('Cannot parse %s dynamic record due to missing ' + 'pool name in first answer note, treating it as ' + 'an empty record', record['domain']) value = None + else: + try: + value = record['short_answers'][0] + except IndexError: + value = None return { 'ttl': record['ttl'],