From b498f767258144a2defc76d1905409dd59492bfd Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Wed, 4 Mar 2020 17:55:55 -0800 Subject: [PATCH 01/12] Add country based filter to NS1 filter chain --- octodns/provider/ns1.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 2a3ae07..2d03b39 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -192,6 +192,9 @@ class Ns1Provider(BaseProvider): }, { 'config': {}, 'filter': u'geofence_regional' + }, { + 'config': {}, + 'filter': u'geofence_country' }, { 'config': {}, 'filter': u'select_first_region' From d68a034a5740e2d682ddb497040b89d711c9b3f6 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Fri, 6 Mar 2020 12:25:07 -0800 Subject: [PATCH 02/12] 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': { From d7c55f15c3482271105dc16e9fe187ada0647664 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Mon, 9 Mar 2020 16:39:02 -0700 Subject: [PATCH 03/12] Handle dynamic filter chains better --- octodns/provider/ns1.py | 92 ++++++++++++++++-------------- tests/test_octodns_provider_ns1.py | 44 ++------------ 2 files changed, 54 insertions(+), 82 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,15 +187,10 @@ 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' - }, { - 'config': {}, - 'filter': u'geofence_country' }, { 'config': {}, 'filter': u'select_first_region' @@ -213,30 +208,18 @@ class Ns1Provider(BaseProvider): }, 'filter': u'select_first_n' }] - - _DEFAULT_DYNAMIC_FILTERS = [{ - 'config': {}, - 'filter': 'up' - }, { + _REGION_FILTER = { 'config': {}, 'filter': u'geofence_regional' - }, { - 'config': {}, - 'filter': u'select_first_region' - }, { - 'config': { - 'eliminate': u'1' - }, - 'filter': 'priority' - }, { + } + _COUNTRY_FILTER = { 'config': {}, - 'filter': u'weighted_shuffle' - }, { - 'config': { - 'N': u'1' - }, - 'filter': u'select_first_n' - }] + 'filter': u'geofence_country' + } + _REGION_FILTER_INDEX = 1 + _COUNTRY_FILTER_INDEX = 1 + _COUNTRY_FILTER_INDEX_WITH_REGION = 2 + _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': { From ee8111ec1af075b04b23fcea56bd361d8d733239 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Mon, 9 Mar 2020 16:41:35 -0700 Subject: [PATCH 04/12] Remove a couple debug prints --- octodns/provider/ns1.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 7d7f798..7a5190d 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -267,11 +267,8 @@ class Ns1Provider(BaseProvider): 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, + return 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) From 789f65c0d19a2f5bdc707b0df69e2bcf7b7b42d1 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Mon, 9 Mar 2020 16:51:32 -0700 Subject: [PATCH 05/12] Lint fixes (long lines) --- octodns/provider/ns1.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 7a5190d..389d845 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -264,11 +264,11 @@ class Ns1Provider(BaseProvider): # - '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] - return filter_config == self._get_updated_filter_chain(has_region, - has_country) + 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] + 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) @@ -279,7 +279,8 @@ class Ns1Provider(BaseProvider): filters.insert(self._COUNTRY_FILTER_INDEX_WITH_REGION, self._COUNTRY_FILTER) else: - filters.insert(self._COUNTRY_FILTER_INDEX, self._COUNTRY_FILTER) + filters.insert(self._COUNTRY_FILTER_INDEX, + self._COUNTRY_FILTER) return filters def _encode_notes(self, data): From 837d3ed4aeeb7690aa5701aeecb036a4f24fe47b Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Thu, 12 Mar 2020 03:01:02 -0700 Subject: [PATCH 06/12] Address review comments, add explicit filter chains --- octodns/provider/ns1.py | 109 ++++++++++++++++++----------- tests/test_octodns_provider_ns1.py | 50 +++++++++++-- 2 files changed, 112 insertions(+), 47 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,38 +238,79 @@ 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' - }, { + } + + _REGION_FILTER = { + 'config': {}, + 'filter': u'geofence_regional' + } + _COUNTRY_FILTER = { + 'config': {}, + 'filter': u'geofence_country' + } + + _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' - }] - _REGION_FILTER = { - 'config': {}, - 'filter': u'geofence_regional' - } - _COUNTRY_FILTER = { - 'config': {}, - 'filter': u'geofence_country' } - _REGION_FILTER_INDEX = 1 - _COUNTRY_FILTER_INDEX = 1 - _COUNTRY_FILTER_INDEX_WITH_REGION = 2 + + _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, } From 5f2fc721ab612927ffe9ed6938655e1993e4b68d Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Wed, 18 Mar 2020 11:15:33 -0700 Subject: [PATCH 07/12] Add disabled flag to filter definition --- octodns/provider/ns1.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 9ff74b7..30bfb42 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -240,20 +240,28 @@ class Ns1Provider(BaseProvider): _UP_FILTER = { 'config': {}, + 'disabled': False, 'filter': 'up' } _REGION_FILTER = { 'config': {}, + 'disabled': False, 'filter': u'geofence_regional' } _COUNTRY_FILTER = { - 'config': {}, + 'config': { + 'remove_no_location': True + }, + 'disabled': False, 'filter': u'geofence_country' } + # In the NS1 UI/portal, this filter is called "SELECT FIRST GROUP" though + # the filter name in the NS1 api is 'select_first_region' _SELECT_FIRST_REGION_FILTER = { 'config': {}, + 'disabled': False, 'filter': u'select_first_region' } @@ -261,11 +269,13 @@ class Ns1Provider(BaseProvider): 'config': { 'eliminate': u'1' }, + 'disabled': False, 'filter': 'priority' } _WEIGHTED_SHUFFLE_FILTER = { 'config': {}, + 'disabled': False, 'filter': u'weighted_shuffle' } @@ -273,6 +283,7 @@ class Ns1Provider(BaseProvider): 'config': { 'N': u'1' }, + 'disabled': False, 'filter': u'select_first_n' } From a7f01d4c76d5fdacca6d163ad0c092b0340f0f7c Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Wed, 25 Mar 2020 12:36:44 -0700 Subject: [PATCH 08/12] Support upgrade of filter chain --- octodns/provider/ns1.py | 238 ++++++++++++++++++----------- tests/test_octodns_provider_ns1.py | 103 ++++++++++++- 2 files changed, 241 insertions(+), 100 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 30bfb42..9ff0a98 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -238,90 +238,101 @@ class Ns1Provider(BaseProvider): ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' - _UP_FILTER = { - 'config': {}, - 'disabled': False, - 'filter': 'up' - } - - _REGION_FILTER = { - 'config': {}, - 'disabled': False, - 'filter': u'geofence_regional' - } - _COUNTRY_FILTER = { - 'config': { - 'remove_no_location': True - }, - 'disabled': False, - 'filter': u'geofence_country' - } + 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 _UP_FILTER(self, with_disabled): + return self._update_filter({ + 'config': {}, + 'filter': 'up' + }, with_disabled) + + def _REGION_FILTER(self, with_disabled): + return self._update_filter({ + 'config': {}, + 'filter': u'geofence_regional' + }, with_disabled) + + def _COUNTRY_FILTER(self, with_disabled): + 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' - _SELECT_FIRST_REGION_FILTER = { - 'config': {}, - 'disabled': False, - 'filter': u'select_first_region' - } - - _PRIORITY_FILTER = { - 'config': { - 'eliminate': u'1' - }, - 'disabled': False, - 'filter': 'priority' - } - - _WEIGHTED_SHUFFLE_FILTER = { - 'config': {}, - 'disabled': False, - 'filter': u'weighted_shuffle' - } + def _SELECT_FIRST_REGION_FILTER(self, with_disabled): + return self._update_filter({ + 'config': {}, + 'filter': u'select_first_region' + }, with_disabled) + + def _PRIORITY_FILTER(self, with_disabled): + return self._update_filter({ + 'config': { + 'eliminate': u'1' + }, + 'filter': 'priority' + }, with_disabled) - _SELECT_FIRST_N_FILTER = { - 'config': { - 'N': u'1' - }, - 'disabled': False, - 'filter': u'select_first_n' - } + def _WEIGHTED_SHUFFLE_FILTER(self, with_disabled): + return self._update_filter({ + 'config': {}, + 'filter': u'weighted_shuffle' + }, with_disabled) - _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 - ] + def _SELECT_FIRST_N_FILTER(self, with_disabled): + return self._update_filter({ + 'config': { + 'N': u'1' + }, + 'filter': u'select_first_n' + }, with_disabled) + + def _BASIC_FILTER_CHAIN(self, with_disabled): + 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) + ] + + def _FILTER_CHAIN_WITH_REGION(self, with_disabled): + 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) + ] + + def _FILTER_CHAIN_WITH_COUNTRY(self, with_disabled): + 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) + ] + + def _FILTER_CHAIN_WITH_REGION_AND_COUNTRY(self, with_disabled): + 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) + ] _REGION_TO_CONTINENT = { 'AFRICA': 'AF', @@ -360,21 +371,27 @@ class Ns1Provider(BaseProvider): self._client = Ns1Client(api_key, parallelism, retry_count, client_config) - def _valid_filter_config(self, 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): + 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 + expected_filter_cfg = self._get_updated_filter_chain(has_region, + has_country, + with_disabled) + return filter_cfg == expected_filter_cfg + + 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 + filter_chain = self._FILTER_CHAIN_WITH_REGION(with_disabled) elif has_country: - filter_chain = self._FILTER_CHAIN_WITH_COUNTRY + filter_chain = self._FILTER_CHAIN_WITH_COUNTRY(with_disabled) else: - filter_chain = self._BASIC_FILTER_CHAIN + filter_chain = self._BASIC_FILTER_CHAIN(with_disabled) + return filter_chain def _encode_notes(self, data): @@ -437,7 +454,7 @@ class Ns1Provider(BaseProvider): def _data_for_dynamic_A(self, _type, record): # First make sure we have the expected filters config - if not self._valid_filter_config(record['filters']): + if not self._valid_filter_config(record['filters'], record['domain']): self.log.error('_data_for_dynamic_A: %s %s has unsupported ' 'filters', record['domain'], _type) raise Ns1Exception('Unrecognized advanced record') @@ -1111,17 +1128,52 @@ 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 = self._client.zones_retrieve(ns1_zone_name) + 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 = 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 = 'Mixed disabled flag in filters for {}'.format( + 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: if record in changed or not getattr(record, 'dynamic', False): # Already changed, or no dynamic , no need to check it continue + # Filter normalization + # 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 = '.'.join([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 + extra.append(Update(record, record)) + continue + for have in self._monitors_for(record).values(): value = have['config']['host'] expected = self._monitor_gen(record, value) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index f7de53e..f2d9908 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -316,6 +316,7 @@ class TestNs1Provider(TestCase): # Fails, general error zone_retrieve_mock.reset_mock() + record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() zone_retrieve_mock.side_effect = ResourceException('boom') with self.assertRaises(ResourceException) as ctx: @@ -324,6 +325,7 @@ class TestNs1Provider(TestCase): # Fails, bad auth zone_retrieve_mock.reset_mock() + record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() zone_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') @@ -334,6 +336,7 @@ class TestNs1Provider(TestCase): # non-existent zone, create zone_retrieve_mock.reset_mock() + record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() zone_retrieve_mock.side_effect = \ ResourceException('server error: zone not found') @@ -364,6 +367,7 @@ class TestNs1Provider(TestCase): # Update & delete zone_retrieve_mock.reset_mock() + record_retrieve_mock.reset_mock() zone_create_mock.reset_mock() ns1_zone = { @@ -389,7 +393,7 @@ class TestNs1Provider(TestCase): } ns1_zone['records'][0]['short_answers'][0] = '2.2.2.2' - record_retrieve_mock.side_effect = [{ + ns1_record = { "domain": "geo.unit.tests", "zone": "unit.tests", "type": "A", @@ -404,8 +408,9 @@ class TestNs1Provider(TestCase): ], 'tier': 3, 'ttl': 34, - }] + } + record_retrieve_mock.side_effect = [ns1_record, ns1_record] zone_retrieve_mock.side_effect = [ns1_zone, ns1_zone] plan = provider.plan(desired) self.assertEquals(3, len(plan.changes)) @@ -427,6 +432,8 @@ class TestNs1Provider(TestCase): None, ] + record_retrieve_mock.side_effect = [ns1_record, ns1_record] + zone_retrieve_mock.side_effect = [ns1_zone, ns1_zone] got_n = provider.apply(plan) self.assertEquals(3, got_n) @@ -958,7 +965,8 @@ class TestNs1ProviderDynamic(TestCase): rule0['geos'] = ['AF', 'EU'] ret, _ = provider._params_for_A(self.record) self.assertEquals(ret['filters'], - Ns1Provider._FILTER_CHAIN_WITH_REGION) + Ns1Provider._FILTER_CHAIN_WITH_REGION(provider, + True)) rule0['geos'] = saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @@ -999,7 +1007,8 @@ 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) + Ns1Provider._FILTER_CHAIN_WITH_COUNTRY(provider, + True)) rule0['geos'] = saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @@ -1031,7 +1040,8 @@ class TestNs1ProviderDynamic(TestCase): # 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) + Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY( + provider, True)) monitors_for_mock.assert_has_calls([call(self.record)]) monitors_sync_mock.assert_has_calls([ @@ -1057,7 +1067,7 @@ class TestNs1ProviderDynamic(TestCase): ns1_record = { 'answers': [], 'domain': 'unit.tests', - 'filters': Ns1Provider._BASIC_FILTER_CHAIN, + 'filters': Ns1Provider._BASIC_FILTER_CHAIN(provider, True), 'regions': {}, 'ttl': 42, } @@ -1191,20 +1201,29 @@ class TestNs1ProviderDynamic(TestCase): self.assertTrue( 'OC-{}'.format(c) in data4['dynamic']['rules'][0]['geos']) + @patch('ns1.rest.records.Records.retrieve') + @patch('ns1.rest.zones.Zones.retrieve') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') - def test_extra_changes(self, monitors_for_mock): + def test_extra_changes(self, monitors_for_mock, zones_retrieve_mock, + records_retrieve_mock): provider = Ns1Provider('test', 'api-key') desired = Zone('unit.tests.', []) # Empty zone and no changes monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() + extra = provider._extra_changes(desired, []) self.assertFalse(extra) monitors_for_mock.assert_not_called() # Simple record, ignored monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() + simple = Record.new(desired, '', { 'ttl': 32, 'type': 'A', @@ -1247,6 +1266,8 @@ class TestNs1ProviderDynamic(TestCase): # untouched, but everything in sync so no change needed monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() # Generate what we expect to have gend = provider._monitor_gen(dynamic, '1.2.3.4') gend.update({ @@ -1265,6 +1286,8 @@ class TestNs1ProviderDynamic(TestCase): # If we don't have a notify list we're broken and we'll expect to see # an Update monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() del gend['notify_list'] monitors_for_mock.side_effect = [{ '1.2.3.4': gend, @@ -1279,6 +1302,8 @@ class TestNs1ProviderDynamic(TestCase): # Add notify_list back and change the healthcheck protocol, we'll still # expect to see an update monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() gend['notify_list'] = 'xyz' dynamic._octodns['healthcheck']['protocol'] = 'HTTPS' del gend['notify_list'] @@ -1294,10 +1319,74 @@ class TestNs1ProviderDynamic(TestCase): # If it's in the changed list, it'll be ignored monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() extra = provider._extra_changes(desired, [update]) self.assertFalse(extra) monitors_for_mock.assert_not_called() + # Test changes in filters + + # No change in filters + monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() + ns1_zone = { + 'records': [{ + "domain": "dyn.unit.tests", + "zone": "unit.tests", + "type": "A", + "tier": 3, + "filters": Ns1Provider._BASIC_FILTER_CHAIN(provider, True) + }], + } + monitors_for_mock.side_effect = [{}] + zones_retrieve_mock.side_effect = [ns1_zone] + records_retrieve_mock.side_effect = ns1_zone['records'] + extra = provider._extra_changes(desired, []) + self.assertFalse(extra) + + # filters need an update + monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() + ns1_zone = { + 'records': [{ + "domain": "dyn.unit.tests", + "zone": "unit.tests", + "type": "A", + "tier": 3, + "filters": Ns1Provider._BASIC_FILTER_CHAIN(provider, False) + }], + } + monitors_for_mock.side_effect = [{}] + zones_retrieve_mock.side_effect = [ns1_zone] + records_retrieve_mock.side_effect = ns1_zone['records'] + extra = provider._extra_changes(desired, []) + self.assertTrue(extra) + + # Mixed disabled in filters. Raise Ns1Exception + monitors_for_mock.reset_mock() + zones_retrieve_mock.reset_mock() + records_retrieve_mock.reset_mock() + ns1_zone = { + 'records': [{ + "domain": "dyn.unit.tests", + "zone": "unit.tests", + "type": "A", + "tier": 3, + "filters": Ns1Provider._BASIC_FILTER_CHAIN(provider, True) + }], + } + 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 + text_type(ctx.exception)) + DESIRED = Zone('unit.tests.', []) SIMPLE = Record.new(DESIRED, 'sim', { From f42300b983b23d6de9957ddc6200abff8950a86d Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Wed, 25 Mar 2020 15:20:51 -0700 Subject: [PATCH 09/12] Apply suggestions from code review Conform to octodns style string concatenation Co-Authored-By: Ross McFarland --- octodns/provider/ns1.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 9ff0a98..cb4420c 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -1138,7 +1138,7 @@ class Ns1Provider(BaseProvider): ns1_record['domain'], ns1_record['type']) if 'filters' in full_rec: - filter_key = ns1_record['domain'] + '.' + filter_key = '{}.'format(ns1_record['domain']) ns1_filters[filter_key] = full_rec['filters'] return ns1_filters @@ -1165,7 +1165,7 @@ 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 = '.'.join([record.name, record.zone.name]) + domain = '{}.{}'format(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): From 438bea46c326b1238c4c74b3f9ffbb47737f0560 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Wed, 25 Mar 2020 15:30:00 -0700 Subject: [PATCH 10/12] Fix typos, add a log for filter updates --- octodns/provider/ns1.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index cb4420c..cecbde0 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -1138,7 +1138,7 @@ class Ns1Provider(BaseProvider): ns1_record['domain'], ns1_record['type']) if 'filters' in full_rec: - filter_key = '{}.'format(ns1_record['domain']) + filter_key = '{}.'.format(ns1_record['domain']) ns1_filters[filter_key] = full_rec['filters'] return ns1_filters @@ -1165,12 +1165,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 = '{}.{}'format(record.name, record.zone.name) + domain = '{}.{}'.format(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 From 72eaf0d3af63abae6e77b55238841d4ba7587c42 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Sat, 28 Mar 2020 02:21:07 -0700 Subject: [PATCH 11/12] Handle non-existent zones case in extra_changes --- octodns/provider/ns1.py | 25 +++++++++++++++---------- tests/test_octodns_provider_ns1.py | 19 ++++++++++++++++++- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index cecbde0..9d378d9 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -1130,16 +1130,21 @@ class Ns1Provider(BaseProvider): def _get_ns1_filters(self, ns1_zone_name): ns1_filters = {} - ns1_zone = self._client.zones_retrieve(ns1_zone_name) - 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 = '{}.'.format(ns1_record['domain']) - ns1_filters[filter_key] = full_rec['filters'] + try: + ns1_zone = self._client.zones_retrieve(ns1_zone_name) + 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 = '{}.'.format(ns1_record['domain']) + ns1_filters[filter_key] = full_rec['filters'] + except ResourceException as e: + if e.message != self.ZONE_NOT_FOUND_MESSAGE: + raise return ns1_filters def _disabled_flag_in_filters(self, filters, domain): diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index f2d9908..fb177c8 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1219,10 +1219,27 @@ class TestNs1ProviderDynamic(TestCase): self.assertFalse(extra) monitors_for_mock.assert_not_called() - # Simple record, ignored + # Non-existent zone. No changes + monitors_for_mock.reset_mock() + zones_retrieve_mock.side_effect = \ + ResourceException('server error: zone not found') + records_retrieve_mock.reset_mock() + extra = provider._extra_changes(desired, []) + self.assertFalse(extra) + + # Unexpected exception message + zones_retrieve_mock.reset_mock() + 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 monitors_for_mock.reset_mock() zones_retrieve_mock.reset_mock() records_retrieve_mock.reset_mock() + zones_retrieve_mock.side_effect = \ + ResourceException('server error: zone not found') simple = Record.new(desired, '', { 'ttl': 32, From 5bb2851002be940f8bcbaf6a1d78af5712e81b52 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Mon, 30 Mar 2020 10:41:00 -0700 Subject: [PATCH 12/12] Keep minimum relevant code in try/except block --- octodns/provider/ns1.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 9d378d9..7e13b28 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -1130,8 +1130,15 @@ class Ns1Provider(BaseProvider): 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 @@ -1142,9 +1149,7 @@ class Ns1Provider(BaseProvider): if 'filters' in full_rec: filter_key = '{}.'.format(ns1_record['domain']) ns1_filters[filter_key] = full_rec['filters'] - except ResourceException as e: - if e.message != self.ZONE_NOT_FOUND_MESSAGE: - raise + return ns1_filters def _disabled_flag_in_filters(self, filters, domain):