From 837d3ed4aeeb7690aa5701aeecb036a4f24fe47b Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Thu, 12 Mar 2020 03:01:02 -0700 Subject: [PATCH] Address review comments, add explicit filter chains --- octodns/provider/ns1.py | 119 ++++++++++++++++++----------- tests/test_octodns_provider_ns1.py | 50 ++++++++++-- 2 files changed, 117 insertions(+), 52 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 30155aa..9ff74b7 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -13,7 +13,6 @@ from ns1.rest.errors import RateLimitException, ResourceException from pycountry_convert import country_alpha2_to_continent_code from time import sleep from uuid import uuid4 -from copy import deepcopy from six import text_type @@ -239,27 +238,11 @@ class Ns1Provider(BaseProvider): ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' - # See comment before _valid_filter_config() for details on filter indices - _BASIC_DYNAMIC_FILTERS = [{ + _UP_FILTER = { 'config': {}, 'filter': 'up' - }, { - 'config': {}, - 'filter': u'select_first_region' - }, { - 'config': { - 'eliminate': u'1' - }, - 'filter': 'priority' - }, { - 'config': {}, - 'filter': u'weighted_shuffle' - }, { - 'config': { - 'N': u'1' - }, - 'filter': u'select_first_n' - }] + } + _REGION_FILTER = { 'config': {}, 'filter': u'geofence_regional' @@ -268,9 +251,66 @@ class Ns1Provider(BaseProvider): 'config': {}, 'filter': u'geofence_country' } - _REGION_FILTER_INDEX = 1 - _COUNTRY_FILTER_INDEX = 1 - _COUNTRY_FILTER_INDEX_WITH_REGION = 2 + + _SELECT_FIRST_REGION_FILTER = { + 'config': {}, + 'filter': u'select_first_region' + } + + _PRIORITY_FILTER = { + 'config': { + 'eliminate': u'1' + }, + 'filter': 'priority' + } + + _WEIGHTED_SHUFFLE_FILTER = { + 'config': {}, + 'filter': u'weighted_shuffle' + } + + _SELECT_FIRST_N_FILTER = { + 'config': { + 'N': u'1' + }, + 'filter': u'select_first_n' + } + + _BASIC_FILTER_CHAIN = [ + _UP_FILTER, + _SELECT_FIRST_REGION_FILTER, + _PRIORITY_FILTER, + _WEIGHTED_SHUFFLE_FILTER, + _SELECT_FIRST_N_FILTER + ] + + _FILTER_CHAIN_WITH_REGION = [ + _UP_FILTER, + _REGION_FILTER, + _SELECT_FIRST_REGION_FILTER, + _PRIORITY_FILTER, + _WEIGHTED_SHUFFLE_FILTER, + _SELECT_FIRST_N_FILTER + ] + + _FILTER_CHAIN_WITH_COUNTRY = [ + _UP_FILTER, + _COUNTRY_FILTER, + _SELECT_FIRST_REGION_FILTER, + _PRIORITY_FILTER, + _WEIGHTED_SHUFFLE_FILTER, + _SELECT_FIRST_N_FILTER + ] + + _FILTER_CHAIN_WITH_REGION_AND_COUNTRY = [ + _UP_FILTER, + _REGION_FILTER, + _COUNTRY_FILTER, + _SELECT_FIRST_REGION_FILTER, + _PRIORITY_FILTER, + _WEIGHTED_SHUFFLE_FILTER, + _SELECT_FIRST_N_FILTER + ] _REGION_TO_CONTINENT = { 'AFRICA': 'AF', @@ -309,33 +349,22 @@ class Ns1Provider(BaseProvider): self._client = Ns1Client(api_key, parallelism, retry_count, client_config) - # Allowed filter configurations: - # 1. Filter chain is the basic filter chain - # 2. In addition, it could have the 'geofence_country' filter and or - # 'geofence_regional' filter only at index 1 in the filter chain - # 3. If both country and regional filters are present, they are required - # to be as below: - # - 'geofence_regional' at index 1 - # - 'geofence_country' at index 2 - # Any other deviation is returned as an unsupported filter configuration def _valid_filter_config(self, filter_cfg): - has_region = 'geofence_regional' in [f['filter'] for f in filter_cfg] - has_country = 'geofence_country' in [f['filter'] for f in filter_cfg] + has_region = self._REGION_FILTER in filter_cfg + has_country = self._COUNTRY_FILTER in filter_cfg return filter_cfg == self._get_updated_filter_chain(has_region, has_country) def _get_updated_filter_chain(self, has_region, has_country): - filters = deepcopy(self._BASIC_DYNAMIC_FILTERS) - if has_region: - filters.insert(self._REGION_FILTER_INDEX, self._REGION_FILTER) - if has_country: - if has_region: - filters.insert(self._COUNTRY_FILTER_INDEX_WITH_REGION, - self._COUNTRY_FILTER) - else: - filters.insert(self._COUNTRY_FILTER_INDEX, - self._COUNTRY_FILTER) - return filters + if has_region and has_country: + filter_chain = self._FILTER_CHAIN_WITH_REGION_AND_COUNTRY + elif has_region: + filter_chain = self._FILTER_CHAIN_WITH_REGION + elif has_country: + filter_chain = self._FILTER_CHAIN_WITH_COUNTRY + else: + filter_chain = self._BASIC_FILTER_CHAIN + return filter_chain def _encode_notes(self, data): return ' '.join(['{}:{}'.format(k, v) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index d3f4ebe..f7de53e 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -926,6 +926,41 @@ class TestNs1ProviderDynamic(TestCase): monitors_delete_mock.assert_has_calls([call('mon-id2')]) notifylists_delete_mock.assert_has_calls([call('nl-id2')]) + @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') + @patch('octodns.provider.ns1.Ns1Provider._monitors_for') + def test_params_for_dynamic_region_only(self, monitors_for_mock, + monitor_sync_mock): + provider = Ns1Provider('test', 'api-key') + + # pre-fill caches to avoid extranious calls (things we're testing + # elsewhere) + provider._client._datasource_id = 'foo' + provider._client._feeds_for_monitors = { + 'mon-id': 'feed-id', + } + + # provider._params_for_A() calls provider._monitors_for() and + # provider._monitor_sync(). Mock their return values so that we don't + # make NS1 API calls during tests + monitors_for_mock.reset_mock() + monitor_sync_mock.reset_mock() + monitors_for_mock.side_effect = [{ + '3.4.5.6': 'mid-3', + }] + monitor_sync_mock.side_effect = [ + ('mid-1', 'fid-1'), + ('mid-2', 'fid-2'), + ('mid-3', 'fid-3'), + ] + + rule0 = self.record.data['dynamic']['rules'][0] + saved_geos = rule0['geos'] + rule0['geos'] = ['AF', 'EU'] + ret, _ = provider._params_for_A(self.record) + self.assertEquals(ret['filters'], + Ns1Provider._FILTER_CHAIN_WITH_REGION) + rule0['geos'] = saved_geos + @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') def test_params_for_dynamic_oceania(self, monitors_for_mock, @@ -962,9 +997,9 @@ class TestNs1ProviderDynamic(TestCase): self.assertEquals(set(ret['regions']['lhr']['meta']['country']), Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC']) # When rules has 'OC', it is converted to list of countries in the - # params. Look for filter 'geofence_country' in the returned filters at - # index 2 - self.assertEquals(ret['filters'][1]['filter'], 'geofence_country') + # params. Look if the returned filters is the filter chain with country + self.assertEquals(ret['filters'], + Ns1Provider._FILTER_CHAIN_WITH_COUNTRY) rule0['geos'] = saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @@ -993,9 +1028,10 @@ class TestNs1ProviderDynamic(TestCase): # handling to get there ret, _ = provider._params_for_A(self.record) - # Given that self.record has a country in the rules, we should find - # the filter 'geofence_country' in the returned filters at index 2 - self.assertEquals(ret['filters'][2]['filter'], 'geofence_country') + # Given that self.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) monitors_for_mock.assert_has_calls([call(self.record)]) monitors_sync_mock.assert_has_calls([ @@ -1021,7 +1057,7 @@ class TestNs1ProviderDynamic(TestCase): ns1_record = { 'answers': [], 'domain': 'unit.tests', - 'filters': Ns1Provider._BASIC_DYNAMIC_FILTERS, + 'filters': Ns1Provider._BASIC_FILTER_CHAIN, 'regions': {}, 'ttl': 42, }