Merge branch 'master' into constellix_healthcheck

CHANGELOG.md

@@ -1,3 +1,12 @@
+
+## v0.9.15 - 202?-??-?? - ??
+
+#### Noteworthy changes
+
+* NS1 provider has received improvements to the dynamic record implementation.
+  As a result, if octoDNS is downgraded from this version, any dynamic records
+  created or updated using this version will show an update.
+
 ## v0.9.14 - 2021-10-10 - A new supports system
 
 #### Noteworthy changes

octodns/provider/ns1.py

@@ -312,102 +312,107 @@ class Ns1Provider(BaseProvider):
     ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found'
     SHARED_NOTIFYLIST_NAME = 'octoDNS NS1 Notify List'
 
-    def _update_filter(self, filter, with_disabled):
+    @property
-        if with_disabled:
+    def _UP_FILTER(self):
-            filter['disabled'] = False
+        return {
-            return (dict(sorted(filter.items(), key=lambda t: t[0])))
-        return filter
-
-    def _UP_FILTER(self, with_disabled):
-        return self._update_filter({
             'config': {},
             'filter': 'up'
-        }, with_disabled)
+        }
 
-    def _REGION_FILTER(self, with_disabled):
+    @property
-        return self._update_filter({
+    def _REGION_FILTER(self):
+        return {
             'config': {
                 'remove_no_georegion': True
             },
             'filter': u'geofence_regional'
-        }, with_disabled)
+        }
 
-    def _COUNTRY_FILTER(self, with_disabled):
+    @property
-        return self._update_filter({
+    def _COUNTRY_FILTER(self):
+        return {
             'config': {
                 'remove_no_location': True
             },
             'filter': u'geofence_country'
-        }, with_disabled)
+        }
 
     # In the NS1 UI/portal, this filter is called "SELECT FIRST GROUP" though
     # the filter name in the NS1 api is 'select_first_region'
-    def _SELECT_FIRST_REGION_FILTER(self, with_disabled):
+    @property
-        return self._update_filter({
+    def _SELECT_FIRST_REGION_FILTER(self):
+        return {
             'config': {},
             'filter': u'select_first_region'
-        }, with_disabled)
+        }
 
-    def _PRIORITY_FILTER(self, with_disabled):
+    @property
-        return self._update_filter({
+    def _PRIORITY_FILTER(self):
+        return {
             'config': {
                 'eliminate': u'1'
             },
             'filter': 'priority'
-        }, with_disabled)
+        }
 
-    def _WEIGHTED_SHUFFLE_FILTER(self, with_disabled):
+    @property
-        return self._update_filter({
+    def _WEIGHTED_SHUFFLE_FILTER(self):
+        return {
             'config': {},
             'filter': u'weighted_shuffle'
-        }, with_disabled)
+        }
 
-    def _SELECT_FIRST_N_FILTER(self, with_disabled):
+    @property
-        return self._update_filter({
+    def _SELECT_FIRST_N_FILTER(self):
+        return {
             'config': {
                 'N': u'1'
             },
             'filter': u'select_first_n'
-        }, with_disabled)
+        }
 
-    def _BASIC_FILTER_CHAIN(self, with_disabled):
+    @property
+    def _BASIC_FILTER_CHAIN(self):
         return [
-            self._UP_FILTER(with_disabled),
+            self._UP_FILTER,
-            self._SELECT_FIRST_REGION_FILTER(with_disabled),
+            self._SELECT_FIRST_REGION_FILTER,
-            self._PRIORITY_FILTER(with_disabled),
+            self._PRIORITY_FILTER,
-            self._WEIGHTED_SHUFFLE_FILTER(with_disabled),
+            self._WEIGHTED_SHUFFLE_FILTER,
-            self._SELECT_FIRST_N_FILTER(with_disabled)
+            self._SELECT_FIRST_N_FILTER
         ]
 
-    def _FILTER_CHAIN_WITH_REGION(self, with_disabled):
+    @property
+    def _FILTER_CHAIN_WITH_REGION(self):
         return [
-            self._UP_FILTER(with_disabled),
+            self._UP_FILTER,
-            self._REGION_FILTER(with_disabled),
+            self._REGION_FILTER,
-            self._SELECT_FIRST_REGION_FILTER(with_disabled),
+            self._SELECT_FIRST_REGION_FILTER,
-            self._PRIORITY_FILTER(with_disabled),
+            self._PRIORITY_FILTER,
-            self._WEIGHTED_SHUFFLE_FILTER(with_disabled),
+            self._WEIGHTED_SHUFFLE_FILTER,
-            self._SELECT_FIRST_N_FILTER(with_disabled)
+            self._SELECT_FIRST_N_FILTER
         ]
 
-    def _FILTER_CHAIN_WITH_COUNTRY(self, with_disabled):
+    @property
+    def _FILTER_CHAIN_WITH_COUNTRY(self):
         return [
-            self._UP_FILTER(with_disabled),
+            self._UP_FILTER,
-            self._COUNTRY_FILTER(with_disabled),
+            self._COUNTRY_FILTER,
-            self._SELECT_FIRST_REGION_FILTER(with_disabled),
+            self._SELECT_FIRST_REGION_FILTER,
-            self._PRIORITY_FILTER(with_disabled),
+            self._PRIORITY_FILTER,
-            self._WEIGHTED_SHUFFLE_FILTER(with_disabled),
+            self._WEIGHTED_SHUFFLE_FILTER,
-            self._SELECT_FIRST_N_FILTER(with_disabled)
+            self._SELECT_FIRST_N_FILTER
         ]
 
-    def _FILTER_CHAIN_WITH_REGION_AND_COUNTRY(self, with_disabled):
+    @property
+    def _FILTER_CHAIN_WITH_REGION_AND_COUNTRY(self):
         return [
-            self._UP_FILTER(with_disabled),
+            self._UP_FILTER,
-            self._REGION_FILTER(with_disabled),
+            self._REGION_FILTER,
-            self._COUNTRY_FILTER(with_disabled),
+            self._COUNTRY_FILTER,
-            self._SELECT_FIRST_REGION_FILTER(with_disabled),
+            self._SELECT_FIRST_REGION_FILTER,
-            self._PRIORITY_FILTER(with_disabled),
+            self._PRIORITY_FILTER,
-            self._WEIGHTED_SHUFFLE_FILTER(with_disabled),
+            self._WEIGHTED_SHUFFLE_FILTER,
-            self._SELECT_FIRST_N_FILTER(with_disabled)
+            self._SELECT_FIRST_N_FILTER
         ]
 
     _REGION_TO_CONTINENT = {
@@ -452,29 +457,34 @@ class Ns1Provider(BaseProvider):
         super(Ns1Provider, self).__init__(id, *args, **kwargs)
         self.monitor_regions = monitor_regions
         self.shared_notifylist = shared_notifylist
+        self.record_filters = dict()
         self._client = Ns1Client(api_key, parallelism, retry_count,
                                  client_config)
 
-    def _valid_filter_config(self, filter_cfg, domain):
+    def _sanitize_disabled_in_filter_config(self, filter_cfg):
-        with_disabled = self._disabled_flag_in_filters(filter_cfg, domain)
+        # remove disabled=False from filters
-        has_region = self._REGION_FILTER(with_disabled) in filter_cfg
+        for filter in filter_cfg:
-        has_country = self._COUNTRY_FILTER(with_disabled) in filter_cfg
+            if 'disabled' in filter and filter['disabled'] is False:
+                del filter['disabled']
+        return filter_cfg
+
+    def _valid_filter_config(self, filter_cfg):
+        self._sanitize_disabled_in_filter_config(filter_cfg)
+        has_region = self._REGION_FILTER in filter_cfg
+        has_country = self._COUNTRY_FILTER in filter_cfg
         expected_filter_cfg = self._get_updated_filter_chain(has_region,
-                                                             has_country,
+                                                             has_country)
-                                                             with_disabled)
         return filter_cfg == expected_filter_cfg
 
-    def _get_updated_filter_chain(self, has_region, has_country,
+    def _get_updated_filter_chain(self, has_region, has_country):
-                                  with_disabled=True):
         if has_region and has_country:
-            filter_chain = self._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(
+            filter_chain = self._FILTER_CHAIN_WITH_REGION_AND_COUNTRY
-                with_disabled)
         elif has_region:
-            filter_chain = self._FILTER_CHAIN_WITH_REGION(with_disabled)
+            filter_chain = self._FILTER_CHAIN_WITH_REGION
         elif has_country:
-            filter_chain = self._FILTER_CHAIN_WITH_COUNTRY(with_disabled)
+            filter_chain = self._FILTER_CHAIN_WITH_COUNTRY
         else:
-            filter_chain = self._BASIC_FILTER_CHAIN(with_disabled)
+            filter_chain = self._BASIC_FILTER_CHAIN
 
         return filter_chain
 
@@ -697,11 +707,9 @@ class Ns1Provider(BaseProvider):
         return rules
 
     def _data_for_dynamic(self, _type, record):
-        # First make sure we have the expected filters config
+        # Cache record filters for later use
-        if not self._valid_filter_config(record['filters'], record['domain']):
+        record_filters = self.record_filters.setdefault(record['domain'], {})
-            self.log.error('_data_for_dynamic: %s %s has unsupported '
+        record_filters[_type] = record['filters']
-                           'filters', record['domain'], _type)
-            raise Ns1Exception('Unrecognized advanced record')
 
         default, pools = self._parse_pools(record['answers'])
         rules = self._parse_rules(pools, record['regions'])
@@ -1431,41 +1439,8 @@ class Ns1Provider(BaseProvider):
                   for v in record.values]
         return {'answers': values, 'ttl': record.ttl}, None
 
-    def _get_ns1_filters(self, ns1_zone_name):
-        ns1_filters = {}
-        ns1_zone = {}
-
-        try:
-            ns1_zone = self._client.zones_retrieve(ns1_zone_name)
-        except ResourceException as e:
-            if e.message != self.ZONE_NOT_FOUND_MESSAGE:
-                raise
-
-        if 'records' in ns1_zone:
-            for ns1_record in ns1_zone['records']:
-                if ns1_record.get('tier', 1) > 1:
-                    # Need to get the full record data for geo records
-                    full_rec = self._client.records_retrieve(
-                        ns1_zone_name,
-                        ns1_record['domain'],
-                        ns1_record['type'])
-                    if 'filters' in full_rec:
-                        filter_key = f'{ns1_record["domain"]}.'
-                        ns1_filters[filter_key] = full_rec['filters']
-
-        return ns1_filters
-
-    def _disabled_flag_in_filters(self, filters, domain):
-        disabled_count = ['disabled' in f for f in filters].count(True)
-        if disabled_count and disabled_count != len(filters):
-            # Some filters have the disabled flag, and some don't. Disallow
-            exception_msg = f'Mixed disabled flag in filters for {domain}'
-            raise Ns1Exception(exception_msg)
-        return disabled_count == len(filters)
-
     def _extra_changes(self, desired, changes, **kwargs):
         self.log.debug('_extra_changes: desired=%s', desired.name)
-        ns1_filters = self._get_ns1_filters(desired.name[:-1])
         changed = set([c.record for c in changes])
         extra = []
         for record in desired.records:
@@ -1477,14 +1452,14 @@ class Ns1Provider(BaseProvider):
             # Check if filters for existing domains need an update
             # Needs an explicit check since there might be no change in the
             # config at all. Filters however might still need an update
-            domain = f'{record.name}.{record.zone.name}'
+            domain = record.fqdn[:-1]
-            if domain in ns1_filters:
+            _type = record._type
-                domain_filters = ns1_filters[domain]
+            record_filters = self.record_filters.get(domain, {}).get(_type, [])
-                if not self._disabled_flag_in_filters(domain_filters, domain):
+            if not self._valid_filter_config(record_filters):
-                    # 'disabled' entry absent in filter config. Need to update
+                # unrecognized set of filters, overwrite them by updating the
-                    # filters. Update record
+                # record
-                    self.log.info('_extra_changes: change in filters for %s',
+                self.log.info('_extra_changes: unrecognized filters in %s, '
-                                  domain)
+                              'will update record', domain)
                 extra.append(Update(record, record))
                 continue
 

tests/test_octodns_provider_ns1.py

@@ -1278,8 +1278,7 @@ class TestNs1ProviderDynamic(TestCase):
         ret, monitor_ids = provider._params_for_A(record)
         self.assertEquals(10, len(ret['answers']))
         self.assertEquals(ret['filters'],
-                          Ns1Provider._FILTER_CHAIN_WITH_REGION(provider,
+                          provider._FILTER_CHAIN_WITH_REGION)
-                                                                True))
         self.assertEquals({
             'iad__catchall': {
                 'meta': {
@@ -1335,8 +1334,7 @@ class TestNs1ProviderDynamic(TestCase):
         rule1['geos'] = ['NA-US-CA', 'NA-CA-NL']
         ret, _ = provider._params_for_A(record)
         self.assertEquals(10, len(ret['answers']))
-        exp = Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(provider,
+        exp = provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY
-                                                                True)
         self.assertEquals(ret['filters'], exp)
         self.assertEquals({
             'iad__catchall': {
@@ -1439,8 +1437,7 @@ class TestNs1ProviderDynamic(TestCase):
                           notes['from:lhr__country'])
 
         # We have both country and region filter chain entries
-        exp = Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(provider,
+        exp = provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY
-                                                                True)
         self.assertEquals(ret['filters'], exp)
 
         # and our region details match the expected behaviors/targeting
@@ -1518,8 +1515,7 @@ class TestNs1ProviderDynamic(TestCase):
         # When rules has 'OC', it is converted to list of countries in the
         # params. Look if the returned filters is the filter chain with country
         self.assertEquals(ret['filters'],
-                          Ns1Provider._FILTER_CHAIN_WITH_COUNTRY(provider,
+                          provider._FILTER_CHAIN_WITH_COUNTRY)
-                                                                 True))
 
     @patch('octodns.provider.ns1.Ns1Provider._monitor_sync')
     @patch('octodns.provider.ns1.Ns1Provider._monitors_for')
@@ -1555,8 +1551,7 @@ class TestNs1ProviderDynamic(TestCase):
         # Given that record has both country and region in the rules,
         # the returned filter chain should be one with region and country
         self.assertEquals(ret['filters'],
-                          Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(
+                          provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY)
-                          provider, True))
 
         monitors_for_mock.assert_has_calls([call(record)])
         monitors_sync_mock.assert_has_calls([
@@ -1610,20 +1605,11 @@ class TestNs1ProviderDynamic(TestCase):
     def test_data_for_dynamic(self):
         provider = Ns1Provider('test', 'api-key')
 
-        # Unexpected filters throws an error
-        ns1_record = {
-            'domain': 'unit.tests',
-            'filters': [],
-        }
-        with self.assertRaises(Ns1Exception) as ctx:
-            provider._data_for_dynamic('A', ns1_record)
-        self.assertEquals('Unrecognized advanced record', str(ctx.exception))
-
         # empty record turns into empty data
         ns1_record = {
             'answers': [],
             'domain': 'unit.tests',
-            'filters': Ns1Provider._BASIC_FILTER_CHAIN(provider, True),
+            'filters': provider._BASIC_FILTER_CHAIN,
             'regions': {},
             'ttl': 42,
         }
@@ -2077,13 +2063,6 @@ class TestNs1ProviderDynamic(TestCase):
         extra = provider._extra_changes(desired, [])
         self.assertFalse(extra)
 
-        # Unexpected exception message
-        reset()
-        zones_retrieve_mock.side_effect = ResourceException('boom')
-        with self.assertRaises(ResourceException) as ctx:
-            extra = provider._extra_changes(desired, [])
-        self.assertEquals(zones_retrieve_mock.side_effect, ctx.exception)
-
         # Simple record, ignored, filter update lookups ignored
         reset()
         zones_retrieve_mock.side_effect = \
@@ -2132,6 +2111,9 @@ class TestNs1ProviderDynamic(TestCase):
         # untouched, but everything in sync so no change needed
         reset()
         # Generate what we expect to have
+        provider.record_filters[dynamic.fqdn[:-1]] = {
+            dynamic._type: provider._get_updated_filter_chain(False, False)
+        }
         gend = provider._monitor_gen(dynamic, '1.2.3.4')
         gend.update({
             'id': 'mid',  # need to add an id
@@ -2192,7 +2174,7 @@ class TestNs1ProviderDynamic(TestCase):
                 "zone": "unit.tests",
                 "type": "A",
                 "tier": 3,
-                "filters": Ns1Provider._BASIC_FILTER_CHAIN(provider, True)
+                "filters": provider._BASIC_FILTER_CHAIN
             }],
         }
         monitors_for_mock.side_effect = [{}]
@@ -2209,16 +2191,20 @@ class TestNs1ProviderDynamic(TestCase):
                 "zone": "unit.tests",
                 "type": "A",
                 "tier": 3,
-                "filters": Ns1Provider._BASIC_FILTER_CHAIN(provider, False)
+                "filters": provider._BASIC_FILTER_CHAIN[:-1]
             }],
         }
         monitors_for_mock.side_effect = [{}]
         zones_retrieve_mock.side_effect = [ns1_zone]
         records_retrieve_mock.side_effect = ns1_zone['records']
+        ns1_record = ns1_zone['records'][0]
+        provider.record_filters[ns1_record['domain']] = {
+            ns1_record['type']: ns1_record['filters']
+        }
         extra = provider._extra_changes(desired, [])
         self.assertTrue(extra)
 
-        # Mixed disabled in filters. Raise Ns1Exception
+        # disabled=False in filters doesn't trigger an update
         reset()
         ns1_zone = {
             'records': [{
@@ -2226,16 +2212,24 @@ class TestNs1ProviderDynamic(TestCase):
                 "zone": "unit.tests",
                 "type": "A",
                 "tier": 3,
-                "filters": Ns1Provider._BASIC_FILTER_CHAIN(provider, True)
+                "filters": provider._BASIC_FILTER_CHAIN
             }],
         }
-        del ns1_zone['records'][0]['filters'][0]['disabled']
+        ns1_zone['records'][0]['filters'][0]['disabled'] = False
         monitors_for_mock.side_effect = [{}]
         zones_retrieve_mock.side_effect = [ns1_zone]
         records_retrieve_mock.side_effect = ns1_zone['records']
-        with self.assertRaises(Ns1Exception) as ctx:
+        ns1_record = ns1_zone['records'][0]
+        provider.record_filters[ns1_record['domain']] = {
+            ns1_record['type']: ns1_record['filters']
+        }
         extra = provider._extra_changes(desired, [])
-        self.assertTrue('Mixed disabled flag in filters' in str(ctx.exception))
+        self.assertFalse(extra)
+
+        # disabled=True in filters does trigger an update
+        ns1_zone['records'][0]['filters'][0]['disabled'] = True
+        extra = provider._extra_changes(desired, [])
+        self.assertTrue(extra)
 
     DESIRED = Zone('unit.tests.', [])