From e90aeb5d34fe0b1d423daffb2cdd2b831fbf1775 Mon Sep 17 00:00:00 2001
From: Ross McFarland <rwmcfa1@gmail.com>
Date: Thu, 22 Apr 2021 18:45:14 -0700
Subject: [PATCH] pools used as fallbacks should count as seen

---
 octodns/record/__init__.py          | 12 ++++++++----
 tests/config/dynamic.tests.yaml     | 23 +++++++++++++++++++++++
 tests/test_octodns_provider_yaml.py | 14 +++++++++-----
 3 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/octodns/record/__init__.py b/octodns/record/__init__.py
index 8ee2eaa..8c8760a 100644
--- a/octodns/record/__init__.py
+++ b/octodns/record/__init__.py
@@ -531,6 +531,7 @@ class _DynamicMixin(object):
 
         pools_exist = set()
         pools_seen = set()
+        pools_seen_as_fallback = set()
         if not isinstance(pools, dict):
             reasons.append('pools must be a dict')
         elif not pools:
@@ -573,9 +574,12 @@ class _DynamicMixin(object):
                                        'value {}'.format(_id, value_num))
 
                 fallback = pool.get('fallback', None)
-                if fallback is not None and fallback not in pools:
-                    reasons.append('undefined fallback "{}" for pool "{}"'
-                                   .format(fallback, _id))
+                if fallback is not None:
+                    if fallback in pools:
+                        pools_seen_as_fallback.add(fallback)
+                    else:
+                        reasons.append('undefined fallback "{}" for pool "{}"'
+                                       .format(fallback, _id))
 
                 # Check for loops
                 fallback = pools[_id].get('fallback', None)
@@ -644,7 +648,7 @@ class _DynamicMixin(object):
                         reasons.extend(GeoCodes.validate(geo, 'rule {} '
                                                          .format(rule_num)))
 
-        unused = pools_exist - pools_seen
+        unused = pools_exist - pools_seen - pools_seen_as_fallback
         if unused:
             unused = '", "'.join(sorted(unused))
             reasons.append('unused pools: "{}"'.format(unused))
diff --git a/tests/config/dynamic.tests.yaml b/tests/config/dynamic.tests.yaml
index 4bd97a7..d25f63a 100644
--- a/tests/config/dynamic.tests.yaml
+++ b/tests/config/dynamic.tests.yaml
@@ -109,6 +109,29 @@ cname:
       - pool: iad
   type: CNAME
   value: target.unit.tests.
+pool-only-in-fallback:
+  dynamic:
+    pools:
+      one:
+        fallback: two
+        values:
+          - value: 1.1.1.1
+      three:
+        values:
+          - value: 3.3.3.3
+      two:
+        values:
+          - value: 2.2.2.2
+    rules:
+      - geos:
+        - NA-US
+        pool: one
+      - geos:
+        - AS-SG
+        pool: three
+  ttl: 300
+  type: A
+  values: [4.4.4.4]
 real-ish-a:
   dynamic:
     pools:
diff --git a/tests/test_octodns_provider_yaml.py b/tests/test_octodns_provider_yaml.py
index 2ce9b4a..872fcca 100644
--- a/tests/test_octodns_provider_yaml.py
+++ b/tests/test_octodns_provider_yaml.py
@@ -38,7 +38,7 @@ class TestYamlProvider(TestCase):
         self.assertEquals(22, len(zone.records))
 
         source.populate(dynamic_zone)
-        self.assertEquals(5, len(dynamic_zone.records))
+        self.assertEquals(6, len(dynamic_zone.records))
 
         # Assumption here is that a clean round-trip means that everything
         # worked as expected, data that went in came back out and could be
@@ -68,11 +68,11 @@ class TestYamlProvider(TestCase):
 
             # Dynamic plan
             plan = target.plan(dynamic_zone)
-            self.assertEquals(5, len([c for c in plan.changes
+            self.assertEquals(6, len([c for c in plan.changes
                                       if isinstance(c, Create)]))
             self.assertFalse(isfile(dynamic_yaml_file))
             # Apply it
-            self.assertEquals(5, target.apply(plan))
+            self.assertEquals(6, target.apply(plan))
             self.assertTrue(isfile(dynamic_yaml_file))
 
             # There should be no changes after the round trip
@@ -148,6 +148,10 @@ class TestYamlProvider(TestCase):
                 self.assertTrue('value' in dyna)
                 # self.assertTrue('dynamic' in dyna)
 
+                dyna = data.pop('pool-only-in-fallback')
+                self.assertTrue('value' in dyna)
+                # self.assertTrue('dynamic' in dyna)
+
                 # make sure nothing is left
                 self.assertEquals([], list(data.keys()))
 
@@ -397,7 +401,7 @@ class TestOverridingYamlProvider(TestCase):
         # Load the base, should see the 5 records
         base.populate(zone)
         got = {r.name: r for r in zone.records}
-        self.assertEquals(5, len(got))
+        self.assertEquals(6, len(got))
         # We get the "dynamic" A from the base config
         self.assertTrue('dynamic' in got['a'].data)
         # No added
@@ -406,7 +410,7 @@ class TestOverridingYamlProvider(TestCase):
         # Load the overrides, should replace one and add 1
         override.populate(zone)
         got = {r.name: r for r in zone.records}
-        self.assertEquals(6, len(got))
+        self.assertEquals(7, len(got))
         # 'a' was replaced with a generic record
         self.assertEquals({
             'ttl': 3600,