From d7c55f15c3482271105dc16e9fe187ada0647664 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Mon, 9 Mar 2020 16:39:02 -0700 Subject: [PATCH] Handle dynamic filter chains better --- octodns/provider/ns1.py | 120 +++++++++++++++-------------- tests/test_octodns_provider_ns1.py | 44 ++--------- 2 files changed, 68 insertions(+), 96 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index e95d551..7d7f798 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -187,56 +187,39 @@ class Ns1Provider(BaseProvider): ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' - _SUPPORTED_FILTER_CONFIGS = [{ + # See comment before _valid_filter_config() for details on filter indices + _BASIC_DYNAMIC_FILTERS = [{ 'config': {}, 'filter': 'up' }, { 'config': {}, - 'filter': u'geofence_regional' + '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' + } + _COUNTRY_FILTER = { 'config': {}, 'filter': u'geofence_country' - }, { - '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_INDEX = 1 + _COUNTRY_FILTER_INDEX = 1 + _COUNTRY_FILTER_INDEX_WITH_REGION = 2 - _DEFAULT_DYNAMIC_FILTERS = [{ - 'config': {}, - 'filter': 'up' - }, { - 'config': {}, - 'filter': u'geofence_regional' - }, { - '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_TO_CONTINENT = { 'AFRICA': 'AF', 'ASIAPAC': 'AS', @@ -272,6 +255,36 @@ class Ns1Provider(BaseProvider): self._client = Ns1Client(api_key, retry_count) + # 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_config): + has_region = 'geofence_regional' in [f['filter'] for f in filter_config] + has_country = 'geofence_country' in [f['filter'] for f in filter_config] + expected_filter_config = self._get_updated_filter_chain(has_region, + has_country) + self.log.debug("input %s", ','.join([f['filter'] for f in filter_config])) + self.log.debug("expected %s", ','.join([f['filter'] for f in expected_filter_config])) + return filter_config == expected_filter_config + + 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 + def _encode_notes(self, data): return ' '.join(['{}:{}'.format(k, v) for k, v in sorted(data.items())]) @@ -332,9 +345,7 @@ class Ns1Provider(BaseProvider): def _data_for_dynamic_A(self, _type, record): # First make sure we have the expected filters config - unsupported_fcs = False in [fc in self._SUPPORTED_FILTER_CONFIGS for - fc in record['filters']] - if not record['filters'] or unsupported_fcs: + if not self._valid_filter_config(record['filters']): self.log.error('_data_for_dynamic_A: %s %s has unsupported ' 'filters', record['domain'], _type) raise Ns1Exception('Unrecognized advanced record') @@ -820,6 +831,7 @@ class Ns1Provider(BaseProvider): # Convert rules to regions has_country = False + has_region = False regions = {} for i, rule in enumerate(record.dynamic.rules): pool_name = rule.data['pool'] @@ -842,6 +854,9 @@ class Ns1Provider(BaseProvider): if n == 8: # US state, e.g. NA-US-KY us_state.add(geo[-2:]) + # For filtering. State filtering is done by the country + # filter + has_country = True elif n == 5: # Country, e.g. EU-FR country.add(geo[-2:]) @@ -850,6 +865,7 @@ class Ns1Provider(BaseProvider): # Continent, e.g. AS if geo in self._CONTINENT_TO_REGIONS: georegion.update(self._CONTINENT_TO_REGIONS[geo]) + has_region = True else: # No maps for geo in _CONTINENT_TO_REGIONS. # Use the country list @@ -945,18 +961,8 @@ class Ns1Provider(BaseProvider): } answers.append(answer) - # Adjust filters as necessary - # Make a copy the filters since we might modify it - filters = deepcopy(self._DEFAULT_DYNAMIC_FILTERS) - if has_country: - self.log.debug('Adding country filter') - country_filter = { - 'config': {}, - 'filter': u'geofence_country' - } - # We want the 'UP' filter first (pos 0) followed by the 'REGION' - # filter (pos 1). The country filter comes next at pos 2 - filters.insert(2, country_filter) + # Update filters as necessary + filters = self._get_updated_filter_chain(has_region, has_country) return { 'answers': answers, diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 25833be..c257ea3 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -926,42 +926,6 @@ 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_no_country(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'), - ] - - # For complete test coverage, need a test case where the country is - # NOT set. - rule0 = self.record.data['dynamic']['rules'][0] - saved_geos = rule0['geos'] - rule0['geos'] = ['AF'] - ret, _ = provider._params_for_A(self.record) - self.assertNotEquals(ret['filters'][2]['filter'], 'geofence_country') - 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, @@ -1000,7 +964,7 @@ class TestNs1ProviderDynamic(TestCase): # 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'][2]['filter'], 'geofence_country') + self.assertEquals(ret['filters'][1]['filter'], 'geofence_country') rule0['geos'] = saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @@ -1057,7 +1021,7 @@ class TestNs1ProviderDynamic(TestCase): ns1_record = { 'answers': [], 'domain': 'unit.tests', - 'filters': Ns1Provider._DEFAULT_DYNAMIC_FILTERS, + 'filters': Ns1Provider._BASIC_DYNAMIC_FILTERS, 'regions': {}, 'ttl': 42, } @@ -1073,6 +1037,8 @@ class TestNs1ProviderDynamic(TestCase): }, data) # Test out a small, but realistic setup that covers all the options + # We have country and region in the test config + filters = provider._get_updated_filter_chain(True, True) ns1_record = { 'answers': [{ 'answer': ['3.4.5.6'], @@ -1113,7 +1079,7 @@ class TestNs1ProviderDynamic(TestCase): 'region': 'iad', }], 'domain': 'unit.tests', - 'filters': Ns1Provider._DEFAULT_DYNAMIC_FILTERS, + 'filters': filters, 'regions': { 'lhr': { 'meta': {