diff --git a/octodns/record/dynamic.py b/octodns/record/dynamic.py index 369260e..100efec 100644 --- a/octodns/record/dynamic.py +++ b/octodns/record/dynamic.py @@ -215,6 +215,8 @@ class _DynamicMixin(object): reasons = [] pools_seen = set() + geos_seen = {} + if not isinstance(rules, (list, tuple)): reasons.append('rules must be a list') elif not rules: @@ -257,11 +259,25 @@ 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.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 + return reasons, pools_seen @classmethod diff --git a/tests/test_octodns_record_dynamic.py b/tests/test_octodns_record_dynamic.py index 6c16881..51bde9c 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 @@ -1282,3 +1287,37 @@ 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'}, + ] + reasons, pools_seen = _DynamicMixin._validate_rules(pools, rules) + self.assertFalse(reasons) + self.assertEqual({'sfo', 'iad'}, pools_seen) + + pools = {'iad', 'sfo'} + rules = [ + {'geos': ('AS', 'NA'), 'pool': 'sfo'}, + {'geos': ('EU', 'NA-CA'), '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, + ) + + pools = {'sfo'} + rules = [{'geos': ('AS', 'NA-US', 'NA'), 'pool': 'sfo'}] + 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, + )