diff --git a/octodns/provider/plan.py b/octodns/provider/plan.py index 09ccc73..f3416b8 100644 --- a/octodns/provider/plan.py +++ b/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'] diff --git a/tests/test_octodns_plan.py b/tests/test_octodns_plan.py index 4dfad54..6c01bb8 100644 --- a/tests/test_octodns_plan.py +++ b/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))