diff --git a/CHANGELOG.md b/CHANGELOG.md index 901681d..3a38abb 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 #### Stuff 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 e019c94..a44aec0 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 384533e..fbb3ca8 100644 --- a/octodns/record/dynamic.py +++ b/octodns/record/dynamic.py @@ -120,21 +120,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') @@ -220,10 +208,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') @@ -267,11 +259,61 @@ 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 '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/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/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)