Merge pull request #517 from github/dynamic-pool-validation

Dynamic pool validation

octodns/record/__init__.py

@@ -507,6 +507,8 @@ class _DynamicMixin(object):
         except KeyError:
             pools = {}
 
+        pools_exist = set()
+        pools_seen = set()
         if not isinstance(pools, dict):
             reasons.append('pools must be a dict')
         elif not pools:
@@ -522,6 +524,8 @@ class _DynamicMixin(object):
                     reasons.append('pool "{}" is missing values'.format(_id))
                     continue
 
+                pools_exist.add(_id)
+
                 for i, value in enumerate(values):
                     value_num = i + 1
                     try:
@@ -578,7 +582,6 @@ class _DynamicMixin(object):
             seen_default = False
 
             # TODO: don't allow 'default' as a pool name, reserved
-            # TODO: warn or error on unused pools?
             for i, rule in enumerate(rules):
                 rule_num = i + 1
                 try:
@@ -590,9 +593,15 @@ class _DynamicMixin(object):
                 if not isinstance(pool, string_types):
                     reasons.append('rule {} invalid pool "{}"'
                                    .format(rule_num, pool))
-                elif pool not in pools:
-                    reasons.append('rule {} undefined pool "{}"'
-                                   .format(rule_num, pool))
+                else:
+                    if pool not in pools:
+                        reasons.append('rule {} undefined pool "{}"'
+                                       .format(rule_num, pool))
+                        pools_seen.add(pool)
+                    elif pool in pools_seen:
+                        reasons.append('rule {} invalid, target pool "{}" '
+                                       'reused'.format(rule_num, pool))
+                    pools_seen.add(pool)
 
                 try:
                     geos = rule['geos']
@@ -611,6 +620,11 @@ class _DynamicMixin(object):
                         reasons.extend(GeoCodes.validate(geo, 'rule {} '
                                                          .format(rule_num)))
 
+        unused = pools_exist - pools_seen
+        if unused:
+            unused = '", "'.join(sorted(unused))
+            reasons.append('unused pools: "{}"'.format(unused))
+
         return reasons
 
     def __init__(self, zone, name, data, *args, **kwargs):

tests/config/dynamic.tests.yaml

@@ -23,7 +23,7 @@ a:
     rules:
       - geos:
         - EU-GB
-        pool: iad
+        pool: lax
       - geos:
         - EU
         pool: ams
@@ -60,7 +60,7 @@ aaaa:
     rules:
       - geos:
         - EU-GB
-        pool: iad
+        pool: lax
       - geos:
         - EU
         pool: ams
@@ -96,7 +96,7 @@ cname:
     rules:
       - geos:
         - EU-GB
-        pool: iad
+        pool: lax
       - geos:
         - EU
         pool: ams

tests/config/split/dynamic.tests./a.yaml

@@ -29,7 +29,7 @@ a:
     rules:
     - geos:
       - EU-GB
-      pool: iad
+      pool: lax
     - geos:
       - EU
       pool: ams

tests/config/split/dynamic.tests./aaaa.yaml

@@ -29,7 +29,7 @@ aaaa:
     rules:
     - geos:
       - EU-GB
-      pool: iad
+      pool: lax
     - geos:
       - EU
       pool: ams

tests/config/split/dynamic.tests./cname.yaml

@@ -27,7 +27,7 @@ cname:
     rules:
     - geos:
       - EU-GB
-      pool: iad
+      pool: lax
     - geos:
       - EU
       pool: ams

tests/test_octodns_record.py

@@ -3075,7 +3075,7 @@ class TestDynamicRecords(TestCase):
             'invalid IPv4 address "blip"',
         ], ctx.exception.reasons)
 
-        # missing rules
+        # missing rules, and unused pools
         a_data = {
             'dynamic': {
                 'pools': {
@@ -3102,7 +3102,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['missing rules'], ctx.exception.reasons)
+        self.assertEquals([
+            'missing rules',
+            'unused pools: "one", "two"',
+        ], ctx.exception.reasons)
 
         # empty rules
         a_data = {
@@ -3132,7 +3135,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['missing rules'], ctx.exception.reasons)
+        self.assertEquals([
+            'missing rules',
+            'unused pools: "one", "two"',
+        ], ctx.exception.reasons)
 
         # rules not a list/tuple
         a_data = {
@@ -3162,7 +3168,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['rules must be a list'], ctx.exception.reasons)
+        self.assertEquals([
+            'rules must be a list',
+            'unused pools: "one", "two"',
+        ], ctx.exception.reasons)
 
         # rule without pool
         a_data = {
@@ -3196,7 +3205,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['rule 1 missing pool'], ctx.exception.reasons)
+        self.assertEquals([
+            'rule 1 missing pool',
+            'unused pools: "two"',
+        ], ctx.exception.reasons)
 
         # rule with non-string pools
         a_data = {
@@ -3231,8 +3243,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(['rule 1 invalid pool "[]"'],
-                          ctx.exception.reasons)
+        self.assertEquals([
+            'rule 1 invalid pool "[]"',
+            'unused pools: "two"',
+        ], ctx.exception.reasons)
 
         # rule references non-existent pool
         a_data = {
@@ -3267,8 +3281,10 @@ class TestDynamicRecords(TestCase):
         }
         with self.assertRaises(ValidationError) as ctx:
             Record.new(self.zone, 'bad', a_data)
-        self.assertEquals(["rule 1 undefined pool \"non-existent\""],
-                          ctx.exception.reasons)
+        self.assertEquals([
+            "rule 1 undefined pool \"non-existent\"",
+            'unused pools: "two"',
+        ], ctx.exception.reasons)
 
         # rule with invalid geos
         a_data = {
@@ -3377,6 +3393,45 @@ class TestDynamicRecords(TestCase):
         self.assertEquals(['rule 2 duplicate default'],
                           ctx.exception.reasons)
 
+        # repeated pool in rules
+        a_data = {
+            'dynamic': {
+                'pools': {
+                    'one': {
+                        'values': [{
+                            'value': '3.3.3.3',
+                        }]
+                    },
+                    'two': {
+                        'values': [{
+                            'value': '4.4.4.4',
+                        }, {
+                            'value': '5.5.5.5',
+                        }]
+                    },
+                },
+                'rules': [{
+                    'geos': ['EU'],
+                    'pool': 'two',
+                }, {
+                    'geos': ['AF'],
+                    'pool': 'one',
+                }, {
+                    'pool': 'one',
+                }],
+            },
+            'ttl': 60,
+            'type': 'A',
+            'values': [
+                '1.1.1.1',
+                '2.2.2.2',
+            ],
+        }
+        with self.assertRaises(ValidationError) as ctx:
+            Record.new(self.zone, 'bad', a_data)
+        self.assertEquals(['rule 3 invalid, target pool "one" reused'],
+                          ctx.exception.reasons)
+
     def test_dynamic_lenient(self):
         # Missing pools
         a_data = {