From aa58631dcd6c5faf2108787d2f79736596d86f07 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Fri, 20 Mar 2020 13:15:14 -0700
Subject: [PATCH] Validate dynamic rules do not reuse pools

---
 octodns/record/__init__.py                   | 13 +++++--
 tests/config/dynamic.tests.yaml              |  6 +--
 tests/config/split/dynamic.tests./a.yaml     |  2 +-
 tests/config/split/dynamic.tests./aaaa.yaml  |  2 +-
 tests/config/split/dynamic.tests./cname.yaml |  2 +-
 tests/test_octodns_record.py                 | 40 ++++++++++++++++++++
 6 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py
index 98c1836..70066fe 100644
--- a/octodns/record/__init__.py
+++ b/octodns/record/__init__.py
@@ -579,6 +579,7 @@ class _DynamicMixin(object):
 
             # TODO: don't allow 'default' as a pool name, reserved
             # TODO: warn or error on unused pools?
+            pools_seen = set()
             for i, rule in enumerate(rules):
                 rule_num = i + 1
                 try:
@@ -590,9 +591,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']
diff --git a/tests/config/dynamic.tests.yaml b/tests/config/dynamic.tests.yaml
index f826880..4bd97a7 100644
--- a/tests/config/dynamic.tests.yaml
+++ b/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
diff --git a/tests/config/split/dynamic.tests./a.yaml b/tests/config/split/dynamic.tests./a.yaml
index f182df6..3027686 100644
--- a/tests/config/split/dynamic.tests./a.yaml
+++ b/tests/config/split/dynamic.tests./a.yaml
@@ -29,7 +29,7 @@ a:
     rules:
     - geos:
       - EU-GB
-      pool: iad
+      pool: lax
     - geos:
       - EU
       pool: ams
diff --git a/tests/config/split/dynamic.tests./aaaa.yaml b/tests/config/split/dynamic.tests./aaaa.yaml
index 3b1dcb7..a2d8779 100644
--- a/tests/config/split/dynamic.tests./aaaa.yaml
+++ b/tests/config/split/dynamic.tests./aaaa.yaml
@@ -29,7 +29,7 @@ aaaa:
     rules:
     - geos:
       - EU-GB
-      pool: iad
+      pool: lax
     - geos:
       - EU
       pool: ams
diff --git a/tests/config/split/dynamic.tests./cname.yaml b/tests/config/split/dynamic.tests./cname.yaml
index ff85955..b716ad9 100644
--- a/tests/config/split/dynamic.tests./cname.yaml
+++ b/tests/config/split/dynamic.tests./cname.yaml
@@ -27,7 +27,7 @@ cname:
     rules:
     - geos:
       - EU-GB
-      pool: iad
+      pool: lax
     - geos:
       - EU
       pool: ams
diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py
index f313342..749e686 100644
--- a/tests/test_octodns_record.py
+++ b/tests/test_octodns_record.py
@@ -3377,6 +3377,46 @@ 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',
+            ],
+        }
+        print('\n*** start ***')
+        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 = {