diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index a023df2..64d7b36 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -312,102 +312,111 @@ 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): - if with_disabled: - filter['disabled'] = False - return (dict(sorted(filter.items(), key=lambda t: t[0]))) - return filter + def _update_filter(self, filter): + filter.setdefault('disabled', False) + return (dict(sorted(filter.items(), key=lambda t: t[0]))) - def _UP_FILTER(self, with_disabled): + @property + def _UP_FILTER(self): return self._update_filter({ 'config': {}, 'filter': 'up' - }, with_disabled) + }) - def _REGION_FILTER(self, with_disabled): + @property + def _REGION_FILTER(self): return self._update_filter({ 'config': { 'remove_no_georegion': True }, 'filter': u'geofence_regional' - }, with_disabled) + }) - def _COUNTRY_FILTER(self, with_disabled): + @property + def _COUNTRY_FILTER(self): return self._update_filter({ '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 + def _SELECT_FIRST_REGION_FILTER(self): return self._update_filter({ 'config': {}, 'filter': u'select_first_region' - }, with_disabled) + }) - def _PRIORITY_FILTER(self, with_disabled): + @property + def _PRIORITY_FILTER(self): return self._update_filter({ 'config': { 'eliminate': u'1' }, 'filter': 'priority' - }, with_disabled) + }) - def _WEIGHTED_SHUFFLE_FILTER(self, with_disabled): + @property + def _WEIGHTED_SHUFFLE_FILTER(self): return self._update_filter({ 'config': {}, 'filter': u'weighted_shuffle' - }, with_disabled) + }) - def _SELECT_FIRST_N_FILTER(self, with_disabled): + @property + def _SELECT_FIRST_N_FILTER(self): return self._update_filter({ '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._SELECT_FIRST_REGION_FILTER(with_disabled), - self._PRIORITY_FILTER(with_disabled), - self._WEIGHTED_SHUFFLE_FILTER(with_disabled), - self._SELECT_FIRST_N_FILTER(with_disabled) + self._UP_FILTER, + self._SELECT_FIRST_REGION_FILTER, + self._PRIORITY_FILTER, + self._WEIGHTED_SHUFFLE_FILTER, + 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._REGION_FILTER(with_disabled), - self._SELECT_FIRST_REGION_FILTER(with_disabled), - self._PRIORITY_FILTER(with_disabled), - self._WEIGHTED_SHUFFLE_FILTER(with_disabled), - self._SELECT_FIRST_N_FILTER(with_disabled) + self._UP_FILTER, + self._REGION_FILTER, + self._SELECT_FIRST_REGION_FILTER, + self._PRIORITY_FILTER, + self._WEIGHTED_SHUFFLE_FILTER, + 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._COUNTRY_FILTER(with_disabled), - self._SELECT_FIRST_REGION_FILTER(with_disabled), - self._PRIORITY_FILTER(with_disabled), - self._WEIGHTED_SHUFFLE_FILTER(with_disabled), - self._SELECT_FIRST_N_FILTER(with_disabled) + self._UP_FILTER, + self._COUNTRY_FILTER, + self._SELECT_FIRST_REGION_FILTER, + self._PRIORITY_FILTER, + self._WEIGHTED_SHUFFLE_FILTER, + 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._REGION_FILTER(with_disabled), - self._COUNTRY_FILTER(with_disabled), - self._SELECT_FIRST_REGION_FILTER(with_disabled), - self._PRIORITY_FILTER(with_disabled), - self._WEIGHTED_SHUFFLE_FILTER(with_disabled), - self._SELECT_FIRST_N_FILTER(with_disabled) + self._UP_FILTER, + self._REGION_FILTER, + self._COUNTRY_FILTER, + self._SELECT_FIRST_REGION_FILTER, + self._PRIORITY_FILTER, + self._WEIGHTED_SHUFFLE_FILTER, + self._SELECT_FIRST_N_FILTER ] _REGION_TO_CONTINENT = { @@ -452,29 +461,27 @@ 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): - with_disabled = self._disabled_flag_in_filters(filter_cfg, domain) - has_region = self._REGION_FILTER(with_disabled) in filter_cfg - has_country = self._COUNTRY_FILTER(with_disabled) in filter_cfg + def _valid_filter_config(self, filter_cfg): + self._disabled_flag_in_filters(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, - with_disabled) + has_country) return filter_cfg == expected_filter_cfg - def _get_updated_filter_chain(self, has_region, has_country, - with_disabled=True): + def _get_updated_filter_chain(self, has_region, has_country): if has_region and has_country: - filter_chain = self._FILTER_CHAIN_WITH_REGION_AND_COUNTRY( - with_disabled) + filter_chain = self._FILTER_CHAIN_WITH_REGION_AND_COUNTRY 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 @@ -546,11 +553,9 @@ class Ns1Provider(BaseProvider): return pool_name def _data_for_dynamic(self, _type, record): - # First make sure we have the expected filters config - if not self._valid_filter_config(record['filters'], record['domain']): - self.log.error('_data_for_dynamic: %s %s has unsupported ' - 'filters', record['domain'], _type) - raise Ns1Exception('Unrecognized advanced record') + # Cache record filters for later use + record_filters = self.record_filters.setdefault(record['domain'], {}) + record_filters[_type] = record['filters'] # All regions (pools) will include the list of default values # (eventually) at higher priorities, we'll just add them to this set to @@ -1394,41 +1399,12 @@ 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 _disabled_flag_in_filters(self, filters): + # fill up filters with disabled=False flag whenever absent + return [self._update_filter(f) for f in 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: @@ -1440,16 +1416,16 @@ 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}' - if domain in ns1_filters: - domain_filters = ns1_filters[domain] - if not self._disabled_flag_in_filters(domain_filters, domain): - # 'disabled' entry absent in filter config. Need to update - # filters. Update record - self.log.info('_extra_changes: change in filters for %s', - domain) - extra.append(Update(record, record)) - continue + domain = record.fqdn[:-1] + _type = record._type + record_filters = self.record_filters.get(domain, {}).get(_type, []) + if not self._valid_filter_config(record_filters): + # unrecognized set of filters, overwrite them by updating the + # record + self.log.info('_extra_changes: unrecognized filters in %s, ' + 'will update record', domain) + extra.append(Update(record, record)) + continue for value, have in self._monitors_for(record).items(): expected = self._monitor_gen(record, value) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 2364d92..08f92ac 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/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, - True)) + provider._FILTER_CHAIN_WITH_REGION) 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, - True) + exp = provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY 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, - True) + exp = provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY 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, - True)) + provider._FILTER_CHAIN_WITH_COUNTRY) @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, True)) + provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY) 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, } @@ -2038,13 +2024,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 = \ @@ -2093,6 +2072,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 @@ -2153,7 +2135,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 = [{}] @@ -2170,16 +2152,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 + # Mixed disabled in filters doesn't trigger an update reset() ns1_zone = { 'records': [{ @@ -2187,16 +2173,19 @@ 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'] 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: - extra = provider._extra_changes(desired, []) - self.assertTrue('Mixed disabled flag in filters' in str(ctx.exception)) + ns1_record = ns1_zone['records'][0] + provider.record_filters[ns1_record['domain']] = { + ns1_record['type']: ns1_record['filters'] + } + extra = provider._extra_changes(desired, []) + self.assertFalse(extra) DESIRED = Zone('unit.tests.', [])