From d68a034a5740e2d682ddb497040b89d711c9b3f6 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Fri, 6 Mar 2020 12:25:07 -0800 Subject: [PATCH] Update country filter conditionally instead of changing the default --- octodns/provider/ns1.py | 50 +++++++++++++++++++++++++++-- tests/test_octodns_provider_ns1.py | 51 ++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 6 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 2d03b39..e95d551 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -13,6 +13,7 @@ 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 @@ -186,7 +187,7 @@ class Ns1Provider(BaseProvider): ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' - _DYNAMIC_FILTERS = [{ + _SUPPORTED_FILTER_CONFIGS = [{ 'config': {}, 'filter': 'up' }, { @@ -212,6 +213,30 @@ class Ns1Provider(BaseProvider): }, 'filter': u'select_first_n' }] + + _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', @@ -307,7 +332,9 @@ class Ns1Provider(BaseProvider): def _data_for_dynamic_A(self, _type, record): # First make sure we have the expected filters config - if self._DYNAMIC_FILTERS != record['filters']: + unsupported_fcs = False in [fc in self._SUPPORTED_FILTER_CONFIGS for + fc in record['filters']] + if not record['filters'] or unsupported_fcs: self.log.error('_data_for_dynamic_A: %s %s has unsupported ' 'filters', record['domain'], _type) raise Ns1Exception('Unrecognized advanced record') @@ -792,6 +819,7 @@ class Ns1Provider(BaseProvider): pools = record.dynamic.pools # Convert rules to regions + has_country = False regions = {} for i, rule in enumerate(record.dynamic.rules): pool_name = rule.data['pool'] @@ -809,6 +837,7 @@ class Ns1Provider(BaseProvider): us_state = set() for geo in rule.data.get('geos', []): + n = len(geo) if n == 8: # US state, e.g. NA-US-KY @@ -816,6 +845,7 @@ class Ns1Provider(BaseProvider): elif n == 5: # Country, e.g. EU-FR country.add(geo[-2:]) + has_country = True else: # Continent, e.g. AS if geo in self._CONTINENT_TO_REGIONS: @@ -827,6 +857,7 @@ class Ns1Provider(BaseProvider): format(geo)) for c in self._CONTINENT_TO_LIST_OF_COUNTRIES[geo]: country.add(c) + has_country = True meta = { 'note': self._encode_notes(notes), @@ -914,9 +945,22 @@ 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) + return { 'answers': answers, - 'filters': self._DYNAMIC_FILTERS, + 'filters': filters, 'regions': regions, 'ttl': record.ttl, }, active_monitors diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 8126c23..25833be 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -926,6 +926,42 @@ 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, @@ -961,6 +997,10 @@ class TestNs1ProviderDynamic(TestCase): ret, _ = provider._params_for_A(self.record) 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'][2]['filter'], 'geofence_country') rule0['geos'] = saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @@ -987,7 +1027,12 @@ class TestNs1ProviderDynamic(TestCase): ] # This indirectly calls into _params_for_dynamic_A and tests the # handling to get there - provider._params_for_A(self.record) + 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') + monitors_for_mock.assert_has_calls([call(self.record)]) monitors_sync_mock.assert_has_calls([ call(self.record, '1.2.3.4', None), @@ -1012,7 +1057,7 @@ class TestNs1ProviderDynamic(TestCase): ns1_record = { 'answers': [], 'domain': 'unit.tests', - 'filters': Ns1Provider._DYNAMIC_FILTERS, + 'filters': Ns1Provider._DEFAULT_DYNAMIC_FILTERS, 'regions': {}, 'ttl': 42, } @@ -1068,7 +1113,7 @@ class TestNs1ProviderDynamic(TestCase): 'region': 'iad', }], 'domain': 'unit.tests', - 'filters': Ns1Provider._DYNAMIC_FILTERS, + 'filters': Ns1Provider._DEFAULT_DYNAMIC_FILTERS, 'regions': { 'lhr': { 'meta': {