diff --git a/CHANGELOG.md b/CHANGELOG.md index 296c33c..43a8e39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ longer as they were considered private/protected. * Beta support for auto-arpa has been added, See the [auto-arpa documentation](/docs/auto_arpa.md) for more information. +* Enhanced validations on dynamic rules to encourage best practices + * The last rule should be a catch-all w/o any targeted geos + * Geos should not be repeated in multiple rules + * Geos in rules subsequent rules should be ordered most to least specific, + e.g. NA-US-TN must come before NA-US, which must occur before NA * Support for subnet targeting in dynamic records added. #### Stuff diff --git a/README.md b/README.md index bbbe6fe..87fe467 100644 --- a/README.md +++ b/README.md @@ -228,7 +228,7 @@ The table below lists the providers octoDNS supports. They are maintained in the | [Scaleway](https://www.scaleway.com/en/dns/) | [octodns_scaleway](https://github.com/scaleway/octodns-scaleway) | | | [Selectel](https://selectel.ru/en/services/additional/dns/) | [octodns_selectel](https://github.com/octodns/octodns-selectel/) | | | [TransIP](https://www.transip.eu/knowledgebase/entry/155-dns-and-nameservers/) | [octodns_transip](https://github.com/octodns/octodns-transip/) | | -| [UltraDNS](https://www.home.neustar/dns-services) | [octodns_ultra](https://github.com/octodns/octodns-ultra/) | | +| [UltraDNS](https://vercara.com/authoritative-dns) | [octodns_ultra](https://github.com/octodns/octodns-ultra/) | | | [YamlProvider](/octodns/provider/yaml.py) | built-in | Supports all record types and core functionality | ### Updating to use extracted providers diff --git a/docs/assets/dynamic-rules-and-pools.jpg b/docs/assets/dynamic-rules-and-pools.jpg new file mode 100644 index 0000000..005383c Binary files /dev/null and b/docs/assets/dynamic-rules-and-pools.jpg differ diff --git a/docs/dynamic_records.md b/docs/dynamic_records.md index bee3939..d2fee7c 100644 --- a/docs/dynamic_records.md +++ b/docs/dynamic_records.md @@ -39,25 +39,45 @@ test: rules: - geos: # Geos used in matching queries + - AF-ZA - AS - OC # The pool to service the query from pool: apac - geos: + # AF-ZA was sent to apac above and the rest of AF else goes to eu here, + # sub-locations (e.g. AF-ZA) should come before their parents (AF.) If a + # more specific geo occured after a general one requests in that + # location would have already matched the previous rule. For the same + # reasons locations may not be repeated in multiple rules. - AF - EU pool: eu - # No geos means match all queries + # No geos means match all queries, the final rule should generally be a + # "catch-all", served to any requests that didn't match the preceeding + # rules. The catch-all is the only case where a pool may be re-used. - pool: na ttl: 60 type: A - # These values become a non-healthchecked default pool + # These values become a non-healthchecked default pool, generally it should be + # a superset of the catch-all pool and include enough capacity to try and + # serve all global requests (with degraded performance.) The main case they + # will come into play is if all dynamic healthchecks are failing, either on + # the service side or if the providers systems are expeiencing problems. values: + - 3.3.3.3 + - 4.4.4.4 - 5.5.5.5 - 6.6.6.6 - 7.7.7.7 ``` +If you encounter validation errors in dynamic records suggesting best practices that you have specific reasons for not following see [docs/records.md#Lenience](/docs/records.md#Lenience) for how to turn the errors into warnings. Doing so is taking on the burden of thoroughly testing and verifying that what you're doing behaves the way you expect. You may well encounter situations where the octoDNS providers and/or the underlying DNS services do not behave as desired. + +#### Visual Representation of the Rules and Pools + +![Diagram of the example records rules and pools](assets/dynamic-rules-and-pools.jpg) + #### Geo Codes Geo codes consist of one to three parts depending on the scope of the area being targeted. Examples of these look like: diff --git a/octodns/record/dynamic.py b/octodns/record/dynamic.py index 90982ae..eeaa4d2 100644 --- a/octodns/record/dynamic.py +++ b/octodns/record/dynamic.py @@ -125,21 +125,9 @@ class _DynamicMixin(object): ) @classmethod - def validate(cls, name, fqdn, data): - reasons = super().validate(name, fqdn, data) - - if 'dynamic' not in data: - return reasons - elif 'geo' in data: - reasons.append('"dynamic" record with "geo" content') - - try: - pools = data['dynamic']['pools'] - except KeyError: - pools = {} - + def _validate_pools(cls, pools): + reasons = [] pools_exist = set() - pools_seen = set() pools_seen_as_fallback = set() if not isinstance(pools, dict): reasons.append('pools must be a dict') @@ -225,10 +213,14 @@ class _DynamicMixin(object): break seen.append(fallback) - try: - rules = data['dynamic']['rules'] - except KeyError: - rules = [] + return reasons, pools_exist, pools_seen_as_fallback + + @classmethod + def _validate_rules(cls, pools, rules): + reasons = [] + pools_seen = set() + + geos_seen = {} if not isinstance(rules, (list, tuple)): reasons.append('rules must be a list') @@ -270,11 +262,29 @@ class _DynamicMixin(object): if not isinstance(geos, (list, tuple)): reasons.append(f'rule {rule_num} geos must be a list') else: - for geo in geos: + # sorted so that NA would come before NA-US so that the code + # below can detect rules that have needlessly targeted a + # more specific location along with it's parent/ancestor + for geo in sorted(geos): reasons.extend( GeoCodes.validate(geo, f'rule {rule_num} ') ) + # have we ever seen a broader version of the geo we're + # currently looking at, e.g. geo=NA-US and there was a + # previous rule with NA + for seen, where in geos_seen.items(): + if geo == seen: + reasons.append( + f'rule {rule_num} targets geo {geo} which has previously been seen in rule {where}' + ) + elif geo.startswith(seen): + reasons.append( + f'rule {rule_num} targets geo {geo} which is more specific than the previously seen {seen} in rule {where}' + ) + + geos_seen[geo] = rule_num + if not isinstance(subnets, (list, tuple)): reasons.append(f'rule {rule_num} subnets must be a list') else: @@ -283,6 +293,38 @@ class _DynamicMixin(object): Subnets.validate(subnet, f'rule {rule_num} ') ) + if 'geos' in rules[-1]: + reasons.append('final rule has "geos" and is not catchall') + + return reasons, pools_seen + + @classmethod + def validate(cls, name, fqdn, data): + reasons = super().validate(name, fqdn, data) + + if 'dynamic' not in data: + return reasons + elif 'geo' in data: + reasons.append('"dynamic" record with "geo" content') + + try: + pools = data['dynamic']['pools'] + except KeyError: + pools = {} + + pool_reasons, pools_exist, pools_seen_as_fallback = cls._validate_pools( + pools + ) + reasons.extend(pool_reasons) + + try: + rules = data['dynamic']['rules'] + except KeyError: + rules = [] + + rule_reasons, pools_seen = cls._validate_rules(pools, rules) + reasons.extend(rule_reasons) + unused = pools_exist - pools_seen - pools_seen_as_fallback if unused: unused = '", "'.join(sorted(unused)) diff --git a/octodns/record/geo_data.py b/octodns/record/geo_data.py index 0b0407c..d78eabf 100644 --- a/octodns/record/geo_data.py +++ b/octodns/record/geo_data.py @@ -207,7 +207,7 @@ geo_data = { 'PE': {'name': 'Prince Edward Island'}, 'QC': {'name': 'Quebec'}, 'SK': {'name': 'Saskatchewan'}, - 'YT': {'name': 'Yukon Territory'}, + 'YT': {'name': 'Yukon'}, }, }, 'CR': {'name': 'Costa Rica'}, @@ -291,7 +291,7 @@ geo_data = { 'UM': {'name': 'United States Minor Outlying Islands'}, 'UT': {'name': 'Utah'}, 'VA': {'name': 'Virginia'}, - 'VI': {'name': 'Virgin Islands'}, + 'VI': {'name': 'Virgin Islands, U.S.'}, 'VT': {'name': 'Vermont'}, 'WA': {'name': 'Washington'}, 'WI': {'name': 'Wisconsin'}, diff --git a/requirements-dev.txt b/requirements-dev.txt index 6573491..068d690 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,3 +1,4 @@ +# DO NOT EDIT THIS FILE DIRECTLY - use ./script/update-requirements to update Pygments==2.13.0 attrs==22.1.0 black==22.10.0 diff --git a/requirements.txt b/requirements.txt index cb174a4..22f9237 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ +# DO NOT EDIT THIS FILE DIRECTLY - use ./script/update-requirements to update PyYAML==6.0 dnspython==2.3.0 fqdn==1.5.1 diff --git a/script/format b/script/format index 1e9fe4f..2a8438d 100755 --- a/script/format +++ b/script/format @@ -2,7 +2,7 @@ set -e -SOURCES=$(find *.py octodns tests -name "*.py") +SOURCES="$(find *.py octodns tests -name '*.py') $(grep --files-with-matches '^#!.*python' script/*)" . env/bin/activate diff --git a/script/generate-geo-data b/script/generate-geo-data index 8767e49..ea0951d 100755 --- a/script/generate-geo-data +++ b/script/generate-geo-data @@ -1,11 +1,10 @@ #!/usr/bin/env python from collections import defaultdict -from pprint import pformat + from pycountry import countries, subdivisions from pycountry_convert import country_alpha2_to_continent_code - subs = defaultdict(dict) for subdivision in subdivisions: # Route53 only supports US states, Dyn (and others) support US states and CA provinces @@ -15,7 +14,6 @@ for subdivision in subdivisions: 'name': subdivision.name } subs = dict(subs) -#pprint(subs) # These are best guesses at things pycountry_convert doesn't map continent_backups = { @@ -38,21 +36,27 @@ for country in countries: continent_code = continent_backups[country.alpha_2] except KeyError: raise - print('{} {} {}'.format(country.alpha_2, country.name, getattr(country, 'official_name', ''))) + print( + '{} {} {}'.format( + country.alpha_2, + country.name, + getattr(country, 'official_name', ''), + ) + ) - geos[continent_code][country.alpha_2] = { - 'name': country.name - } + geos[continent_code][country.alpha_2] = {'name': country.name} try: - geos[continent_code][country.alpha_2]['provinces'] = subs[country.alpha_2] + geos[continent_code][country.alpha_2]['provinces'] = subs[ + country.alpha_2 + ] except KeyError: pass geos = dict(geos) -data = pformat(geos).replace('\n', '\n ') -print('''# +print( + '''# # -*- coding: utf-8 -*- # # This file is generated using @@ -60,5 +64,25 @@ print('''# # do not modify it directly # -geo_data = \\ - {}'''.format(data)) +geo_data = {''' +) + +for continent, details in sorted(geos.items()): + print(f" '{continent}': {{") + for country, info in sorted(details.items()): + name = info['name'] + quoted_name = f'"{name}"' if "'" in name else f"'{name}'" + if 'provinces' in info: + print(f" '{country}': {{") + print(f" 'name': {quoted_name},") + print(" 'provinces': {") + for prov, info in sorted(info['provinces'].items()): + name = info['name'] + quoted_name = f'"{name}"' if "'" in name else f"'{name}'" + print(f" '{prov}': {{'name': {quoted_name}}},") + print(' },') + print(' },') + else: + print(f" '{country}': {{'name': {quoted_name}}},") + print(' },') +print('}') diff --git a/script/lint b/script/lint index 24c7643..fd0d36c 100755 --- a/script/lint +++ b/script/lint @@ -15,6 +15,6 @@ if [ ! -f "$ACTIVATE" ]; then fi . "$ACTIVATE" -SOURCES="*.py octodns/*.py octodns/*/*.py tests/*.py" +SOURCES="$(find *.py octodns tests -name '*.py') $(grep --files-with-matches '^#!.*python' script/*)" pyflakes $SOURCES diff --git a/script/update-requirements b/script/update-requirements index 6275c05..7696f6d 100755 --- a/script/update-requirements +++ b/script/update-requirements @@ -1,9 +1,10 @@ #!/usr/bin/env python3 +import re from os.path import join from subprocess import check_call, check_output +from sys import argv from tempfile import TemporaryDirectory -import re def print_packages(packages, heading): @@ -37,16 +38,21 @@ with TemporaryDirectory() as tmpdir: # pip installs the module itself along with deps so we need to get that out of # our list by finding the thing that was file installed during dev frozen = sorted([p for p in frozen if not p.startswith(our_package_name)]) -dev_frozen = sorted([p for p in dev_frozen - if not p.startswith(our_package_name)]) +dev_frozen = sorted( + [p for p in dev_frozen if not p.startswith(our_package_name)] +) print_packages(frozen, 'frozen') print_packages(dev_frozen, 'dev_frozen') +script = argv[0] + with open('requirements.txt', 'w') as fh: + fh.write(f'# DO NOT EDIT THIS FILE DIRECTLY - use {script} to update\n') fh.write('\n'.join(frozen)) fh.write('\n') with open('requirements-dev.txt', 'w') as fh: + fh.write(f'# DO NOT EDIT THIS FILE DIRECTLY - use {script} to update\n') fh.write('\n'.join(dev_frozen)) fh.write('\n') diff --git a/tests/config/dynamic.tests.yaml b/tests/config/dynamic.tests.yaml index d25f63a..f30a2ed 100644 --- a/tests/config/dynamic.tests.yaml +++ b/tests/config/dynamic.tests.yaml @@ -129,6 +129,7 @@ pool-only-in-fallback: - geos: - AS-SG pool: three + - pool: one ttl: 300 type: A values: [4.4.4.4] diff --git a/tests/test_octodns_record_dynamic.py b/tests/test_octodns_record_dynamic.py index 6c16881..ceb65bf 100644 --- a/tests/test_octodns_record_dynamic.py +++ b/tests/test_octodns_record_dynamic.py @@ -11,7 +11,12 @@ from octodns.record import Record from octodns.record.a import ARecord, Ipv4Value from octodns.record.aaaa import AaaaRecord from octodns.record.cname import CnameRecord -from octodns.record.dynamic import _Dynamic, _DynamicPool, _DynamicRule +from octodns.record.dynamic import ( + _Dynamic, + _DynamicMixin, + _DynamicPool, + _DynamicRule, +) from octodns.record.exception import ValidationError from octodns.zone import Zone @@ -941,6 +946,7 @@ class TestRecordDynamic(TestCase): {'geos': ['EU'], 'pool': 'two'}, {'geos': ['AF'], 'pool': 'one'}, {'geos': ['OC'], 'pool': 'one'}, + {'pool': 'one'}, ], }, 'ttl': 60, @@ -1282,3 +1288,66 @@ class TestRecordDynamic(TestCase): }, cname.dynamic.pools['two'].data, ) + + def test_dynamic_mixin_validate_rules(self): + # this one is fine we get more generic with subsequent rules + pools = {'iad', 'sfo'} + rules = [ + {'geos': ('AS', 'NA-CA', 'NA-US-OR'), 'pool': 'sfo'}, + {'geos': ('EU', 'NA'), 'pool': 'iad'}, + {'pool': 'iad'}, + ] + reasons, pools_seen = _DynamicMixin._validate_rules(pools, rules) + self.assertFalse(reasons) + self.assertEqual({'sfo', 'iad'}, pools_seen) + + # this one targets NA in rule 0 and then NA-Ca in rule 1 + pools = {'iad', 'sfo'} + rules = [ + {'geos': ('AS', 'NA'), 'pool': 'sfo'}, + {'geos': ('EU', 'NA-CA'), 'pool': 'iad'}, + {'pool': 'iad'}, + ] + reasons, pools_seen = _DynamicMixin._validate_rules(pools, rules) + self.assertEqual( + [ + 'rule 2 targets geo NA-CA which is more specific than the previously seen NA in rule 1' + ], + reasons, + ) + + # this one targets NA and NA-US in rule 0 + pools = {'iad', 'sfo'} + rules = [ + {'geos': ('AS', 'NA-US', 'NA'), 'pool': 'sfo'}, + {'pool': 'iad'}, + ] + reasons, pools_seen = _DynamicMixin._validate_rules(pools, rules) + self.assertEqual( + [ + 'rule 1 targets geo NA-US which is more specific than the previously seen NA in rule 1' + ], + reasons, + ) + + # this one targets the same geo in multiple rules + pools = {'iad', 'sfo'} + rules = [ + {'geos': ('AS', 'NA'), 'pool': 'sfo'}, + {'geos': ('EU', 'NA'), 'pool': 'iad'}, + {'pool': 'iad'}, + ] + reasons, pools_seen = _DynamicMixin._validate_rules(pools, rules) + self.assertEqual( + ['rule 2 targets geo NA which has previously been seen in rule 1'], + reasons, + ) + + # this one doesn't have a catch-all rule at the end + pools = {'iad', 'sfo'} + rules = [ + {'geos': ('AS', 'NA-CA', 'NA-US-OR'), 'pool': 'sfo'}, + {'geos': ('EU', 'NA'), 'pool': 'iad'}, + ] + reasons, pools_seen = _DynamicMixin._validate_rules(pools, rules) + self.assertEqual(['final rule has "geos" and is not catchall'], reasons)