Rework unsafe bits, add RootNsChange as a new type of unsafe plan

octodns/provider/plan.py

@@ -12,12 +12,22 @@ from io import StringIO
 
 
 class UnsafePlan(Exception):
+    pass
+
+
+class RootNsChange(UnsafePlan):
+
+    def __init__(self):
+        super().__init__('Root NS record change, force required')
+
+
+class TooMuchChange(UnsafePlan):
 
     def __init__(self, why, update_pcent, update_threshold, change_count,
                  existing_count):
-        msg = f'{why}, {update_pcent:.2f} is over {update_threshold:.2f} %' \
-            f'({change_count}/{existing_count})'
-        super(UnsafePlan, self).__init__(msg)
+        msg = f'{why}, {update_pcent:.2f}% is over {update_threshold:.2f}% ' \
+            f'({change_count}/{existing_count}), force required'
+        super().__init__(msg)
 
 
 class Plan(object):
@@ -67,19 +77,32 @@ class Plan(object):
            len(self.existing.records) >= self.MIN_EXISTING_RECORDS:
 
             existing_record_count = len(self.existing.records)
-            update_pcent = self.change_counts['Update'] / existing_record_count
-            delete_pcent = self.change_counts['Delete'] / existing_record_count
+            if existing_record_count > 0:
+                update_pcent = (self.change_counts['Update'] /
+                                existing_record_count)
+                delete_pcent = (self.change_counts['Delete'] /
+                                existing_record_count)
+            else:
+                update_pcent = 0
+                delete_pcent = 0
 
             if update_pcent > self.update_pcent_threshold:
-                raise UnsafePlan('Too many updates', update_pcent * 100,
-                                 self.update_pcent_threshold * 100,
-                                 self.change_counts['Update'],
-                                 existing_record_count)
+                raise TooMuchChange('Too many updates', update_pcent * 100,
+                                    self.update_pcent_threshold * 100,
+                                    self.change_counts['Update'],
+                                    existing_record_count)
             if delete_pcent > self.delete_pcent_threshold:
-                raise UnsafePlan('Too many deletes', delete_pcent * 100,
-                                 self.delete_pcent_threshold * 100,
-                                 self.change_counts['Delete'],
-                                 existing_record_count)
+                raise TooMuchChange('Too many deletes', delete_pcent * 100,
+                                    self.delete_pcent_threshold * 100,
+                                    self.change_counts['Delete'],
+                                    existing_record_count)
+
+        # If we have any changes of the root NS record for the zone it's a huge
+        # deal and force should always be required for extra care
+        if [True for c in self.changes
+                if c.record and c.record._type == 'NS' and
+                c.record.name == '']:
+            raise RootNsChange()
 
     def __repr__(self):
         creates = self.change_counts['Create']

tests/test_octodns_plan.py

@@ -9,7 +9,8 @@ from io import StringIO
 from logging import getLogger
 from unittest import TestCase
 
-from octodns.provider.plan import Plan, PlanHtml, PlanLogger, PlanMarkdown
+from octodns.provider.plan import Plan, PlanHtml, PlanLogger, PlanMarkdown, \
+    RootNsChange, TooMuchChange
 from octodns.record import Create, Delete, Record, Update
 from octodns.zone import Zone
 
@@ -110,3 +111,156 @@ class TestPlanMarkdown(TestCase):
         self.assertTrue('Update | a | A | 300 | 1.1.1.1;' in out)
         self.assertTrue('NA-US: 6.6.6.6 | test' in out)
         self.assertTrue('Delete | a | A | 300 | 2.2.2.2;' in out)
+
+
+class HelperPlan(Plan):
+
+    def __init__(self, *args, min_existing=0, **kwargs):
+        super().__init__(*args, **kwargs)
+        self.MIN_EXISTING_RECORDS = min_existing
+
+
+class TestPlanSafety(TestCase):
+    existing = Zone('unit.tests.', [])
+    record_1 = Record.new(existing, '1', data={
+        'type': 'A',
+        'ttl': 42,
+        'value': '1.2.3.4',
+    })
+    record_2 = Record.new(existing, '2', data={
+        'type': 'A',
+        'ttl': 42,
+        'value': '1.2.3.4',
+    })
+    record_3 = Record.new(existing, '3', data={
+        'type': 'A',
+        'ttl': 42,
+        'value': '1.2.3.4',
+    })
+    record_4 = Record.new(existing, '4', data={
+        'type': 'A',
+        'ttl': 42,
+        'value': '1.2.3.4',
+    })
+
+    def test_too_many_updates(self):
+        existing = self.existing.copy()
+        changes = []
+
+        # No records, no changes, we're good
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # Four records, no changes, we're good
+        existing.add_record(self.record_1)
+        existing.add_record(self.record_2)
+        existing.add_record(self.record_3)
+        existing.add_record(self.record_4)
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # Creates don't count against us
+        changes.append(Create(self.record_1))
+        changes.append(Create(self.record_2))
+        changes.append(Create(self.record_3))
+        changes.append(Create(self.record_4))
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # One update, still good (25%, default threshold is 33%)
+        changes.append(Update(self.record_1, self.record_1))
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # Two and we're over the threshold
+        changes.append(Update(self.record_2, self.record_2))
+        plan = HelperPlan(existing, None, changes, True)
+        with self.assertRaises(TooMuchChange) as ctx:
+            plan.raise_if_unsafe()
+        self.assertTrue('Too many updates', str(ctx.exception))
+
+        # If we require more records before applying we're still OK though
+        plan = HelperPlan(existing, None, changes, True, min_existing=10)
+        plan.raise_if_unsafe()
+
+    def test_too_many_deletes(self):
+        existing = self.existing.copy()
+        changes = []
+
+        # No records, no changes, we're good
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # Four records, no changes, we're good
+        existing.add_record(self.record_1)
+        existing.add_record(self.record_2)
+        existing.add_record(self.record_3)
+        existing.add_record(self.record_4)
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # Creates don't count against us
+        changes.append(Create(self.record_1))
+        changes.append(Create(self.record_2))
+        changes.append(Create(self.record_3))
+        changes.append(Create(self.record_4))
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # One delete, still good (25%, default threshold is 33%)
+        changes.append(Delete(self.record_1))
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # Two and we're over the threshold
+        changes.append(Delete(self.record_2))
+        plan = HelperPlan(existing, None, changes, True)
+        with self.assertRaises(TooMuchChange) as ctx:
+            plan.raise_if_unsafe()
+        self.assertTrue('Too many deletes', str(ctx.exception))
+
+        # If we require more records before applying we're still OK though
+        plan = HelperPlan(existing, None, changes, True, min_existing=10)
+        plan.raise_if_unsafe()
+
+    def test_root_ns_change(self):
+        existing = self.existing.copy()
+        changes = []
+
+        # No records, no changes, we're good
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        existing.add_record(self.record_1)
+        existing.add_record(self.record_2)
+        existing.add_record(self.record_3)
+        existing.add_record(self.record_4)
+
+        # Non NS changes and we're still good
+        changes.append(Update(self.record_1, self.record_1))
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+
+        # Add a change to a non-root NS record, we're OK
+        ns_record = Record.new(existing, 'sub', data={
+            'type': 'NS',
+            'ttl': 43,
+            'values': ('ns1.unit.tests.', 'ns1.unit.tests.'),
+        })
+        changes.append(Delete(ns_record))
+        plan = HelperPlan(existing, None, changes, True)
+        plan.raise_if_unsafe()
+        # Remove that Delete so that we don't go over the delete threshold
+        changes.pop(-1)
+
+        # Delete the root NS record and we get an unsafe
+        root_ns_record = Record.new(existing, '', data={
+            'type': 'NS',
+            'ttl': 43,
+            'values': ('ns3.unit.tests.', 'ns4.unit.tests.'),
+        })
+        changes.append(Delete(root_ns_record))
+        plan = HelperPlan(existing, None, changes, True)
+        with self.assertRaises(RootNsChange) as ctx:
+            plan.raise_if_unsafe()
+        self.assertTrue('Root Ns record change', str(ctx.exception))