Merge branch 'main' into dynamic-subnets

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

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

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

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

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

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

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

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
 

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

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

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

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)