Merge pull request #991 from octodns/improved-dynamic-validations

Improved dynamic validations

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
 

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:

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))

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'},

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('}')

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]

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)