Merge pull request #876 from octodns/root-ns-support

Base support for managing root NS records

CHANGELOG.md

@@ -1,5 +1,20 @@
 ## v0.9.16 - 2022-??-?? - ???
 
+#### Noteworthy changes
+
+* Foundational support for root NS record management.
+   * YamlProvider has it enabled and in general everyone should add root NS
+     records that match what is in their provider(s) as of this release if they
+     aren't already there.
+   * Note that if you created your config files with `octodns-dump`, the records
+     are likely already there and match what was configured at the time of the
+     dump.
+   * Other providers will add root NS support over time once they have had the
+     chance to investigate the functionality and implement management if
+     possible with whatever accomidations are required.
+   * Watch your providers README.md and CHANGELOG.md for support and more
+     information.
+
 #### Stuff
 
 * _AggregateTarget has more complete handling of SUPPORTS* functionality,

README.md

@@ -223,7 +223,7 @@ The table below lists the providers octoDNS supports. They are maintained in the
 | [Selectel](https://selectel.ru/en/services/additional/dns/) | [octodns_selectel](https://github.com/octodns/octodns-selectel/) | |
 | [TransIP](https://www.transip.eu/knowledgebase/entry/155-dns-and-nameservers/) | [octodns_transip](https://github.com/octodns/octodns-transip/) | |
 | [UltraDNS](https://www.home.neustar/dns-services) | [octodns_ultra](https://github.com/octodns/octodns-ultra/) | |
-| [YamlProvider](/octodns/provider/yaml.py) | built-in | config |
+| [YamlProvider](/octodns/provider/yaml.py) | built-in | Supports all record types and core functionality |
 
 ### Updating to use extracted providers
 
@@ -242,7 +242,6 @@ Similar to providers, but can only serve to populate records into a zone, cannot
 | [AxfrSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, LOC, MX, NS, PTR, SPF, SRV, TXT | No | read-only |
 | [ZoneFileSource](/octodns/source/axfr.py) | A, AAAA, CAA, CNAME, MX, NS, PTR, SPF, SRV, TXT | No | read-only |
 | [TinyDnsFileSource](/octodns/source/tinydns.py) | A, CNAME, MX, NS, PTR | No | read-only |
-| [YamlProvider](/octodns/provider/yaml.py) | All | Yes | config |
 
 #### Notes
 

octodns/provider/base.py

@@ -95,8 +95,51 @@ class BaseProvider(BaseSource):
                 record.values = [record.value]
                 desired.add_record(record, replace=True)
 
+        record = desired.root_ns
+        if self.SUPPORTS_ROOT_NS:
+            if not record:
+                self.log.warning('root NS record supported, but no record '
+                                 'is configured for %s', desired.name)
+        else:
+            if record:
+                # we can't manage root NS records, get rid of it
+                msg = \
+                    f'root NS record not supported for {record.fqdn}'
+                fallback = 'ignoring it'
+                self.supports_warn_or_except(msg, fallback)
+                desired.remove_record(record)
+
         return desired
 
+    def _process_existing_zone(self, existing, desired):
+        '''
+        An opportunity for providers to modify the existing zone records before
+        planning. `existing` is a "shallow" copy, see `Zone.copy` for more
+        information
+
+        - `desired` must not be modified in anyway, it is only for reference
+        - Must call `super` at an appropriate point for their work, generally
+          that means as the final step of the method, returning the result of
+          the `super` call.
+        - May modify `existing` directly.
+        - Must not modify records directly, `record.copy` should be called,
+          the results of which can be modified, and then `Zone.add_record` may
+          be used with `replace=True`.
+        - May call `Zone.remove_record` to remove records from `existing`.
+        - Must call supports_warn_or_except with information about any changes
+          that are made to have them logged or throw errors depending on the
+          provider configuration.
+        '''
+
+        existing_root_ns = existing.root_ns
+        if existing_root_ns and (not self.SUPPORTS_ROOT_NS or
+                                 not desired.root_ns):
+            self.log.info('root NS record in existing, but not supported or '
+                          'not configured; ignoring it')
+            existing.remove_record(existing_root_ns)
+
+        return existing
+
     def _include_change(self, change):
         '''
         An opportunity for providers to filter out false positives due to
@@ -120,13 +163,6 @@ class BaseProvider(BaseSource):
     def plan(self, desired, processors=[]):
         self.log.info('plan: desired=%s', desired.name)
 
-        # Make a (shallow) copy of the desired state so that everything from
-        # now on (in this target) can modify it as they see fit without
-        # worrying about impacting other targets.
-        desired = desired.copy()
-
-        desired = self._process_desired_zone(desired)
-
         existing = Zone(desired.name, desired.sub_zones)
         exists = self.populate(existing, target=True, lenient=True)
         if exists is None:
@@ -135,6 +171,15 @@ class BaseProvider(BaseSource):
             self.log.warning('Provider %s used in target mode did not return '
                              'exists', self.id)
 
+        # Make a (shallow) copy of the desired state so that everything from
+        # now on (in this target) can modify it as they see fit without
+        # worrying about impacting other targets.
+        desired = desired.copy()
+
+        desired = self._process_desired_zone(desired)
+
+        existing = self._process_existing_zone(existing, desired)
+
         for processor in processors:
             existing = processor.process_target_zone(existing, target=self)
 

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 self.exists and any(c 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']

octodns/provider/yaml.py

@@ -111,7 +111,8 @@ class YamlProvider(BaseProvider):
                     'URLFWD'))
 
     def __init__(self, id, directory, default_ttl=3600, enforce_order=True,
-                 populate_should_replace=False, *args, **kwargs):
+                 populate_should_replace=False, supports_root_ns=True,
+                 *args, **kwargs):
         klass = self.__class__.__name__
         self.log = logging.getLogger(f'{klass}[{id}]')
         self.log.debug('__init__: id=%s, directory=%s, default_ttl=%d, '
@@ -123,6 +124,11 @@ class YamlProvider(BaseProvider):
         self.default_ttl = default_ttl
         self.enforce_order = enforce_order
         self.populate_should_replace = populate_should_replace
+        self.supports_root_ns = supports_root_ns
+
+    @property
+    def SUPPORTS_ROOT_NS(self):
+        return self.supports_root_ns
 
     def _populate_from_file(self, filename, zone, lenient):
         with open(filename, 'r') as fh:

octodns/source/base.py

@@ -10,6 +10,7 @@ class BaseSource(object):
 
     SUPPORTS_MULTIVALUE_PTR = False
     SUPPORTS_POOL_VALUE_STATUS = False
+    SUPPORTS_ROOT_NS = False
 
     def __init__(self, id):
         self.id = id

octodns/zone.py

@@ -24,12 +24,6 @@ class InvalidNodeException(Exception):
     pass
 
 
-def _is_eligible(record):
-    # Should this record be considered when computing changes
-    # We ignore all top-level NS records
-    return record._type != 'NS' or record.name != ''
-
-
 class Zone(object):
     log = getLogger('Zone')
 
@@ -42,6 +36,7 @@ class Zone(object):
         # We're grouping by node, it allows us to efficiently search for
         # duplicates and detect when CNAMEs co-exist with other records
         self._records = defaultdict(set)
+        self._root_ns = None
         # optional leading . to match empty hostname
         # optional trailing . b/c some sources don't have it on their fqdn
         self._name_re = re.compile(fr'\.?{name}?$')
@@ -59,6 +54,12 @@ class Zone(object):
             return self._origin.records
         return set([r for _, node in self._records.items() for r in node])
 
+    @property
+    def root_ns(self):
+        if self._origin:
+            return self._origin.root_ns
+        return self._root_ns
+
     def hostname_from_fqdn(self, fqdn):
         return self._name_re.sub('', fqdn)
 
@@ -97,11 +98,18 @@ class Zone(object):
                                        f'{record.fqdn} cannot coexist with '
                                        'other records')
 
+        if record._type == 'NS' and record.name == '':
+            self._root_ns = record
+
         node.add(record)
 
     def remove_record(self, record):
         if self._origin:
             self.hydrate()
+
+        if record._type == 'NS' and record.name == '':
+            self._root_ns = None
+
         self._records[record.name].discard(record)
 
     # TODO: delete this
@@ -118,7 +126,7 @@ class Zone(object):
         changes = []
 
         # Find diffs & removes
-        for record in filter(_is_eligible, self.records):
+        for record in self.records:
             if record.ignored:
                 continue
             elif len(record.included) > 0 and \
@@ -168,7 +176,7 @@ class Zone(object):
         # Find additions, things that are in desired, but missing in ourselves.
         # This uses set math and our special __hash__ and __cmp__ functions as
         # well
-        for record in filter(_is_eligible, desired.records - self.records):
+        for record in desired.records - self.records:
             if record.ignored:
                 continue
             elif len(record.included) > 0 and \

tests/config/always-dry-run.yaml

@@ -5,6 +5,7 @@ providers:
   dump:
     class: octodns.provider.yaml.YamlProvider
     directory: env/YAML_TMP_DIR
+    supports_root_ns: False
 zones:
   unit.tests.:
     always-dry-run: true

tests/config/processors.yaml

@@ -5,6 +5,7 @@ providers:
   dump:
     class: octodns.provider.yaml.YamlProvider
     directory: env/YAML_TMP_DIR
+    supports_root_ns: False
   geo:
     class: helpers.GeoProvider
   nosshfp:

tests/config/simple-alias-zone.yaml

@@ -7,6 +7,7 @@ providers:
   dump:
     class: octodns.provider.yaml.YamlProvider
     directory: env/YAML_TMP_DIR
+    supports_root_ns: False
 zones:
   unit.tests.:
     sources:

tests/config/simple.yaml

@@ -4,14 +4,17 @@ providers:
   in:
     class: octodns.provider.yaml.YamlProvider
     directory: tests/config
+    supports_root_ns: False
   dump:
     class: octodns.provider.yaml.YamlProvider
     directory: env/YAML_TMP_DIR
+    supports_root_ns: False
   # This is sort of ugly, but it shouldn't hurt anything. It'll just write out
   # the target file twice where it and dump are both used
   dump2:
     class: octodns.provider.yaml.YamlProvider
     directory: env/YAML_TMP_DIR
+    supports_root_ns: False
   simple:
     class: helpers.SimpleProvider
   geo:

tests/test_octodns_manager.py

@@ -216,14 +216,16 @@ class TestManager(TestCase):
             with open(join(tmpdir.dirname, 'unit.tests.yaml'), 'w') as fh:
                 fh.write('---\n{}')
 
+            # compare doesn't use _process_desired_zone and thus doesn't filter
+            # out root NS records, that seems fine/desirable
             changes = manager.compare(['in'], ['dump'], 'unit.tests.')
-            self.assertEqual(20, len(changes))
+            self.assertEqual(21, len(changes))
 
             # Compound sources with varying support
             changes = manager.compare(['in', 'nosshfp'],
                                       ['dump'],
                                       'unit.tests.')
-            self.assertEqual(19, len(changes))
+            self.assertEqual(20, len(changes))
 
             with self.assertRaises(ManagerException) as ctx:
                 manager.compare(['nope'], ['dump'], 'unit.tests.')

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))

tests/test_octodns_provider_base.py

@@ -20,7 +20,7 @@ from octodns.zone import Zone
 class HelperProvider(BaseProvider):
     log = getLogger('HelperProvider')
 
-    SUPPORTS = set(('A', 'PTR'))
+    SUPPORTS = set(('A', 'NS', 'PTR'))
     SUPPORTS_MULTIVALUE_PTR = False
     SUPPORTS_DYNAMIC = False
 
@@ -36,7 +36,7 @@ class HelperProvider(BaseProvider):
         self.delete_pcent_threshold = Plan.MAX_SAFE_DELETE_PCENT
 
     def populate(self, zone, target=False, lenient=False):
-        pass
+        return True
 
     def _include_change(self, change):
         return not self.include_change_callback or \
@@ -167,6 +167,28 @@ class TestBaseProvider(TestCase):
         self.assertTrue(plan)
         self.assertEqual(1, len(plan.changes))
 
+    def test_plan_with_process_desired_zone_kwarg_fallback(self):
+        ignored = Zone('unit.tests.', [])
+
+        class OldApiProvider(HelperProvider):
+
+            def _process_desired_zone(self, desired):
+                return desired
+
+        # No change, thus no plan
+        provider = OldApiProvider([])
+        self.assertEqual(None, provider.plan(ignored))
+
+        class OtherTypeErrorProvider(HelperProvider):
+
+            def _process_desired_zone(self, desired, exists=False):
+                raise TypeError('foo')
+
+        provider = OtherTypeErrorProvider([])
+        with self.assertRaises(TypeError) as ctx:
+            provider.plan(ignored)
+        self.assertEqual('foo', str(ctx.exception))
+
     def test_plan_with_unsupported_type(self):
         zone = Zone('unit.tests.', [])
 
@@ -229,6 +251,25 @@ class TestBaseProvider(TestCase):
         self.assertEqual(zone.name, another.existing.name)
         self.assertEqual(1, len(another.existing.records))
 
+    def test_plan_with_root_ns(self):
+        zone = Zone('unit.tests.', [])
+        record = Record.new(zone, '', {
+            'ttl': 30,
+            'type': 'NS',
+            'value': '1.2.3.4.',
+        })
+        zone.add_record(record)
+
+        # No root NS support, no change, thus no plan
+        provider = HelperProvider()
+        self.assertEqual(None, provider.plan(zone))
+
+        # set Support root NS records, see the record
+        provider.SUPPORTS_ROOT_NS = True
+        plan = provider.plan(zone)
+        self.assertTrue(plan)
+        self.assertEqual(1, len(plan.changes))
+
     def test_apply(self):
         ignored = Zone('unit.tests.', [])
 
@@ -258,6 +299,37 @@ class TestBaseProvider(TestCase):
         # We filtered out the only change
         self.assertFalse(plan)
 
+    def test_plan_order_of_operations(self):
+
+        class MockProvider(BaseProvider):
+            log = getLogger('mock-provider')
+            SUPPORTS = set(('A',))
+            SUPPORTS_GEO = False
+
+            def __init__(self):
+                super().__init__('mock-provider')
+                self.calls = []
+
+            def populate(self, *args, **kwargs):
+                self.calls.append('populate')
+
+            def _process_desired_zone(self, *args, **kwargs):
+                self.calls.append('_process_desired_zone')
+                return super()._process_desired_zone(*args, **kwargs)
+
+            def _process_existing_zone(self, *args, **kwargs):
+                self.calls.append('_process_existing_zone')
+                return super()._process_existing_zone(*args, **kwargs)
+
+        provider = MockProvider()
+
+        zone = Zone('unit.tests.', [])
+        self.assertFalse(provider.plan(zone))
+        # ensure the calls were made in the expected order, populate comes
+        # first, then desired, then existing
+        self.assertEqual(['populate', '_process_desired_zone',
+                          '_process_existing_zone'], provider.calls)
+
     def test_process_desired_zone(self):
         provider = HelperProvider('test')
 
@@ -278,10 +350,6 @@ class TestBaseProvider(TestCase):
         provider.SUPPORTS_MULTIVALUE_PTR = True
         zone2 = provider._process_desired_zone(zone1.copy())
         record2 = list(zone2.records)[0]
-        from pprint import pprint
-        pprint([
-            record1, record2
-        ])
         self.assertEqual(len(record2.values), 2)
 
         # SUPPORTS_DYNAMIC
@@ -329,6 +397,45 @@ class TestBaseProvider(TestCase):
             'obey'
         )
 
+        # SUPPORTS_ROOT_NS
+        provider.SUPPORTS_ROOT_NS = False
+        zone1 = Zone('unit.tests.', [])
+        record1 = Record.new(zone1, '', {
+            'type': 'NS',
+            'ttl': 3600,
+            'values': ['foo.com.', 'bar.com.'],
+        })
+        zone1.add_record(record1)
+
+        zone2 = provider._process_desired_zone(zone1.copy())
+        self.assertEqual(0, len(zone2.records))
+
+        provider.SUPPORTS_ROOT_NS = True
+        zone2 = provider._process_desired_zone(zone1.copy())
+        self.assertEqual(1, len(zone2.records))
+        self.assertEqual(record1, list(zone2.records)[0])
+
+    def test_process_existing_zone(self):
+        provider = HelperProvider('test')
+
+        # SUPPORTS_ROOT_NS
+        provider.SUPPORTS_ROOT_NS = False
+        zone1 = Zone('unit.tests.', [])
+        record1 = Record.new(zone1, '', {
+            'type': 'NS',
+            'ttl': 3600,
+            'values': ['foo.com.', 'bar.com.'],
+        })
+        zone1.add_record(record1)
+
+        zone2 = provider._process_existing_zone(zone1.copy(), zone1)
+        self.assertEqual(0, len(zone2.records))
+
+        provider.SUPPORTS_ROOT_NS = True
+        zone2 = provider._process_existing_zone(zone1.copy(), zone1)
+        self.assertEqual(1, len(zone2.records))
+        self.assertEqual(record1, list(zone2.records)[0])
+
     def test_safe_none(self):
         # No changes is safe
         Plan(None, None, [], True).raise_if_unsafe()
@@ -552,3 +659,265 @@ class TestBaseProvider(TestCase):
             strict.supports_warn_or_except('Hello World!', 'Will not see')
         self.assertEqual('minimal: Hello World!', str(ctx.exception))
         strict.log.warning.assert_not_called()
+
+
+class TestBaseProviderSupportsRootNs(TestCase):
+
+    class Provider(BaseProvider):
+        log = getLogger('Provider')
+
+        SUPPORTS = set(('A', 'NS'))
+        SUPPORTS_GEO = False
+        SUPPORTS_ROOT_NS = False
+
+        strict_supports = False
+
+        def __init__(self, existing=None):
+            super().__init__('test')
+            self.existing = existing
+
+        def populate(self, zone, target=False, lenient=False):
+            if self.existing:
+                for record in self.existing.records:
+                    zone.add_record(record)
+                return True
+            return False
+
+    zone = Zone('unit.tests.', [])
+    a_record = Record.new(zone, 'ptr', {
+        'type': 'A',
+        'ttl': 3600,
+        'values': ['1.2.3.4', '2.3.4.5'],
+    })
+    ns_record = Record.new(zone, 'sub', {
+        'type': 'NS',
+        'ttl': 3600,
+        'values': ['ns2.foo.com.', 'ns2.bar.com.'],
+    })
+    no_root = zone.copy()
+    no_root.add_record(a_record)
+    no_root.add_record(ns_record)
+
+    root_ns_record = Record.new(zone, '', {
+        'type': 'NS',
+        'ttl': 3600,
+        'values': ['ns1.foo.com.', 'ns1.bar.com.'],
+    })
+    has_root = no_root.copy()
+    has_root.add_record(root_ns_record)
+
+    other_root_ns_record = Record.new(zone, '', {
+        'type': 'NS',
+        'ttl': 3600,
+        'values': ['ns4.foo.com.', 'ns4.bar.com.'],
+    })
+    different_root = no_root.copy()
+    different_root.add_record(other_root_ns_record)
+
+    # False
+
+    def test_supports_root_ns_false_matches(self):
+        # provider has a matching existing root record
+        provider = self.Provider(self.has_root)
+        provider.SUPPORTS_ROOT_NS = False
+
+        # matching root NS in the desired
+        plan = provider.plan(self.has_root)
+
+        # no root ns upport on the target provider so doesn't matter, still no
+        # changes
+        self.assertFalse(plan)
+
+        # plan again with strict_supports enabled, we should get an exception
+        # b/c we have something configured that can't be managed
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.has_root)
+        self.assertEqual('test: root NS record not supported for unit.tests.',
+                         str(ctx.exception))
+
+    def test_supports_root_ns_false_different(self):
+        # provider has a non-matching existing record
+        provider = self.Provider(self.different_root)
+        provider.SUPPORTS_ROOT_NS = False
+
+        # different root is in the desired
+        plan = provider.plan(self.has_root)
+
+        # the mis-match doesn't matter since we can't manage the records
+        # anyway, they will have been removed from the desired and existing.
+        self.assertFalse(plan)
+
+        # plan again with strict_supports enabled, we should get an exception
+        # b/c we have something configured that can't be managed (doesn't
+        # matter that it's a mis-match)
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.has_root)
+        self.assertEqual('test: root NS record not supported for unit.tests.',
+                         str(ctx.exception))
+
+    def test_supports_root_ns_false_missing(self):
+        # provider has an existing record
+        provider = self.Provider(self.has_root)
+        provider.SUPPORTS_ROOT_NS = False
+
+        # desired doesn't have a root
+        plan = provider.plan(self.no_root)
+
+        # the mis-match doesn't matter since we can't manage the records
+        # anyway, they will have been removed from the desired and existing.
+        self.assertFalse(plan)
+
+        # plan again with strict supports enabled, no change since desired
+        # isn't asking to manage root
+        provider.strict_supports = True
+        plan = provider.plan(self.no_root)
+        self.assertFalse(plan)
+
+    def test_supports_root_ns_false_create_zone(self):
+        # provider has no existing records (create)
+        provider = self.Provider()
+        provider.SUPPORTS_ROOT_NS = False
+
+        # case where we have a root NS in the desired
+        plan = provider.plan(self.has_root)
+
+        # no support for root NS so we only create the other two records
+        self.assertTrue(plan)
+        self.assertEqual(2, len(plan.changes))
+
+        # plan again with strict supports enabled, we'll get an exception b/c
+        # the target provider can't manage something in desired
+        provider.strict_supports = True
+        with self.assertRaises(SupportsException) as ctx:
+            provider.plan(self.has_root)
+        self.assertEqual('test: root NS record not supported for unit.tests.',
+                         str(ctx.exception))
+
+    def test_supports_root_ns_false_create_zone_missing(self):
+        # provider has no existing records (create)
+        provider = self.Provider()
+        provider.SUPPORTS_ROOT_NS = False
+
+        # case where we have a root NS in the desired
+        plan = provider.plan(self.no_root)
+
+        # no support for root NS so we only create the other two records
+        self.assertTrue(plan)
+        self.assertEqual(2, len(plan.changes))
+
+        # plan again with strict supports enabled, same result since we're not
+        # asking for a root NS it's just the 2 other changes
+        provider.strict_supports = True
+        plan = provider.plan(self.no_root)
+        self.assertTrue(plan)
+        self.assertEqual(2, len(plan.changes))
+
+    # True
+
+    def test_supports_root_ns_true_matches(self):
+        # provider has a matching existing root record
+        provider = self.Provider(self.has_root)
+        provider.SUPPORTS_ROOT_NS = True
+
+        # same root NS in the desired
+        plan = provider.plan(self.has_root)
+
+        # root NS is supported in the target provider, but they match so no
+        # change
+        self.assertFalse(plan)
+
+        # again with strict supports enabled, no difference
+        provider.strict_supports = True
+        plan = provider.plan(self.has_root)
+        self.assertFalse(plan)
+
+    def test_supports_root_ns_true_different(self):
+        # provider has a non-matching existing record
+        provider = self.Provider(self.different_root)
+        provider.SUPPORTS_ROOT_NS = True
+
+        # non-matching root NS in the desired
+        plan = provider.plan(self.has_root)
+
+        # root NS mismatch in a target provider that supports it, we'll see the
+        # change
+        self.assertTrue(plan)
+        change = plan.changes[0]
+        self.assertEqual(self.other_root_ns_record, change.existing)
+        self.assertEqual(self.root_ns_record, change.new)
+
+        # again with strict supports enabled, no difference, we see the change
+        provider.strict_supports = True
+        plan = provider.plan(self.has_root)
+        self.assertTrue(plan)
+        change = plan.changes[0]
+        self.assertEqual(self.other_root_ns_record, change.existing)
+        self.assertEqual(self.root_ns_record, change.new)
+
+    def test_supports_root_ns_true_missing(self):
+        # provider has a matching existing root record
+        provider = self.Provider(self.has_root)
+        provider.SUPPORTS_ROOT_NS = True
+
+        # there's no root record in the desired
+        plan = provider.plan(self.no_root)
+
+        # the existing root NS in the target is left alone/as is since we
+        # aren't configured with one to manage
+        self.assertFalse(plan)
+
+        # again with strict supports enabled, no difference as non-configured
+        # root NS is a special case that we always just warn about. This is
+        # because we can't known them before it's created and some people may
+        # choose to just leave them unmanaged undefinitely which has been the
+        # behavior up until now.
+        provider.strict_supports = True
+        plan = provider.plan(self.no_root)
+        self.assertFalse(plan)
+
+    def test_supports_root_ns_true_create_zone(self):
+        # provider has no existing records (create)
+        provider = self.Provider()
+        provider.SUPPORTS_ROOT_NS = True
+
+        # case where we have a root NS in the desired
+        plan = provider.plan(self.has_root)
+
+        # there's no existing root record since we're creating the zone so
+        # we'll get a plan that creates everything, including it
+        self.assertTrue(plan)
+        self.assertEqual(3, len(plan.changes))
+        change = [c for c in plan.changes
+                  if c.new.name == '' and c.new._type == 'NS'][0]
+        self.assertFalse(change.existing)
+        self.assertEqual(self.root_ns_record, change.new)
+
+        # again with strict supports enabled, no difference, we see all 3
+        # changes
+        provider.strict_supports = True
+        plan = provider.plan(self.has_root)
+        self.assertTrue(plan)
+        self.assertEqual(3, len(plan.changes))
+        change = [c for c in plan.changes
+                  if c.new.name == '' and c.new._type == 'NS'][0]
+        self.assertFalse(change.existing)
+        self.assertEqual(self.root_ns_record, change.new)
+
+    def test_supports_root_ns_true_create_zone_missing(self):
+        # provider has no existing records (create)
+        provider = self.Provider()
+        provider.SUPPORTS_ROOT_NS = True
+
+        # we don't have a root NS configured so we'll ignore them and just
+        # manage the other records
+        plan = provider.plan(self.no_root)
+        self.assertEqual(2, len(plan.changes))
+
+        # again with strict supports enabled, we'd normally throw an exception,
+        # but since this is a create and we often can't know the root NS values
+        # before the zone is created it's special cased and will only warn
+        provider.strict_supports = True
+        plan = provider.plan(self.no_root)
+        self.assertEqual(2, len(plan.changes))

tests/test_octodns_provider_yaml.py

@@ -53,7 +53,7 @@ class TestYamlProvider(TestCase):
             directory = join(td.dirname, 'sub', 'dir')
             yaml_file = join(directory, 'unit.tests.yaml')
             dynamic_yaml_file = join(directory, 'dynamic.tests.yaml')
-            target = YamlProvider('test', directory)
+            target = YamlProvider('test', directory, supports_root_ns=False)
 
             # We add everything
             plan = target.plan(zone)
@@ -82,6 +82,10 @@ class TestYamlProvider(TestCase):
                 [x for x in reloaded.records
                  if x.name == 'included'][0]._octodns)
 
+            # manually copy over the root since it will have been ignored
+            # when things were written out
+            reloaded.add_record(zone.root_ns)
+
             self.assertFalse(zone.changes(reloaded, target=source))
 
             # A 2nd sync should still create everything
@@ -156,7 +160,8 @@ class TestYamlProvider(TestCase):
                 self.assertEqual([], list(data.keys()))
 
     def test_empty(self):
-        source = YamlProvider('test', join(dirname(__file__), 'config'))
+        source = YamlProvider('test', join(dirname(__file__), 'config'),
+                              supports_root_ns=False)
 
         zone = Zone('empty.', [])
 
@@ -165,7 +170,8 @@ class TestYamlProvider(TestCase):
         self.assertEqual(0, len(zone.records))
 
     def test_unsorted(self):
-        source = YamlProvider('test', join(dirname(__file__), 'config'))
+        source = YamlProvider('test', join(dirname(__file__), 'config'),
+                              supports_root_ns=False)
 
         zone = Zone('unordered.', [])
 
@@ -173,13 +179,14 @@ class TestYamlProvider(TestCase):
             source.populate(zone)
 
         source = YamlProvider('test', join(dirname(__file__), 'config'),
-                              enforce_order=False)
+                              enforce_order=False, supports_root_ns=False)
         # no exception
         source.populate(zone)
         self.assertEqual(2, len(zone.records))
 
     def test_subzone_handling(self):
-        source = YamlProvider('test', join(dirname(__file__), 'config'))
+        source = YamlProvider('test', join(dirname(__file__), 'config'),
+                              supports_root_ns=False)
 
         # If we add `sub` as a sub-zone we'll reject `www.sub`
         zone = Zone('unit.tests.', ['sub'])
@@ -259,7 +266,8 @@ class TestSplitYamlProvider(TestCase):
             zone_dir = join(directory, 'unit.tests.tst')
             dynamic_zone_dir = join(directory, 'dynamic.tests.tst')
             target = SplitYamlProvider('test', directory,
-                                       extension='.tst')
+                                       extension='.tst',
+                                       supports_root_ns=False)
 
             # We add everything
             plan = target.plan(zone)
@@ -287,6 +295,10 @@ class TestSplitYamlProvider(TestCase):
                 [x for x in reloaded.records
                  if x.name == 'included'][0]._octodns)
 
+            # manually copy over the root since it will have been ignored
+            # when things were written out
+            reloaded.add_record(zone.root_ns)
+
             self.assertFalse(zone.changes(reloaded, target=source))
 
             # A 2nd sync should still create everything
@@ -392,9 +404,11 @@ class TestOverridingYamlProvider(TestCase):
     def test_provider(self):
         config = join(dirname(__file__), 'config')
         override_config = join(dirname(__file__), 'config', 'override')
-        base = YamlProvider('base', config, populate_should_replace=False)
+        base = YamlProvider('base', config, populate_should_replace=False,
+                            supports_root_ns=False)
         override = YamlProvider('test', override_config,
-                                populate_should_replace=True)
+                                populate_should_replace=True,
+                                supports_root_ns=False)
 
         zone = Zone('dynamic.tests.', [])
 

tests/test_octodns_source_tinydns.py

@@ -29,10 +29,15 @@ class TestTinyDnsFileSource(TestCase):
                 'ttl': 30,
                 'values': ['10.2.3.4', '10.2.3.5'],
             }),
+            ('', {
+                'type': 'NS',
+                'ttl': 3600,
+                'values': ['ns1.ns.com.', 'ns2.ns.com.'],
+            }),
             ('sub', {
                 'type': 'NS',
                 'ttl': 30,
-                'values': ['ns1.ns.com.', 'ns2.ns.com.'],
+                'values': ['ns3.ns.com.', 'ns4.ns.com.'],
             }),
             ('www', {
                 'type': 'A',

tests/test_octodns_zone.py

@@ -7,7 +7,8 @@ from __future__ import absolute_import, division, print_function, \
 
 from unittest import TestCase
 
-from octodns.record import ARecord, AaaaRecord, Create, Delete, Record, Update
+from octodns.record import ARecord, AaaaRecord, Create, Delete, NsRecord, \
+    Record, Update
 from octodns.zone import DuplicateRecordException, InvalidNodeException, \
     SubzoneRecordException, Zone
 
@@ -410,3 +411,50 @@ class TestZone(TestCase):
         self.assertTrue(copy.hydrate())
         # Doesn't the second
         self.assertFalse(copy.hydrate())
+
+    def test_root_ns(self):
+        zone = Zone('unit.tests.', [])
+
+        a = ARecord(zone, 'a', {'ttl': 42, 'value': '1.1.1.1'})
+        zone.add_record(a)
+        # No root NS yet
+        self.assertFalse(zone.root_ns)
+
+        non_root_ns = NsRecord(zone, 'sub', {'ttl': 42, 'values': (
+            'ns1.unit.tests.',
+            'ns2.unit.tests.',
+        )})
+        zone.add_record(non_root_ns)
+        # No root NS yet b/c this was a sub
+        self.assertFalse(zone.root_ns)
+
+        root_ns = NsRecord(zone, '', {'ttl': 42, 'values': (
+            'ns3.unit.tests.',
+            'ns4.unit.tests.',
+        )})
+        zone.add_record(root_ns)
+        # Now we have a root NS
+        self.assertEqual(root_ns, zone.root_ns)
+
+        # make a copy, it has a root_ns
+        copy = zone.copy()
+        self.assertEqual(root_ns, copy.root_ns)
+
+        # remove the root NS from it and we don't
+        copy.remove_record(root_ns)
+        self.assertFalse(copy.root_ns)
+
+        # original still does though
+        self.assertEqual(root_ns, zone.root_ns)
+
+        # remove the A, still has root NS
+        zone.remove_record(a)
+        self.assertEqual(root_ns, zone.root_ns)
+
+        # remove the sub NS, still has root NS
+        zone.remove_record(non_root_ns)
+        self.assertEqual(root_ns, zone.root_ns)
+
+        # finally remove the root NS, no more
+        zone.remove_record(root_ns)
+        self.assertFalse(zone.root_ns)

tests/zones/tinydns/example.com

@@ -32,8 +32,8 @@ Ccname.other.foo:www.other.foo
 @smtp.example.com::smtp-2-host.example.com:40:1800
 
 # NS
-.sub.example.com::ns1.ns.com:30
-.sub.example.com::ns2.ns.com:30
+.sub.example.com::ns3.ns.com:30
+.sub.example.com::ns4.ns.com:30
 
 # A, under sub
 +www.sub.example.com::1.2.3.4