From bbe4dc2d3e220aab1f4cbf66d415dbd55d0f7da7 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 23 Jun 2020 09:49:37 -0700 Subject: [PATCH 1/6] NS1 georegion, country, and catchall need to be separate groups --- octodns/provider/ns1.py | 94 +++++++++++++++++------------- tests/test_octodns_provider_ns1.py | 45 +++++++++++++- 2 files changed, 98 insertions(+), 41 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index b1efe00..749686d 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -237,7 +237,6 @@ class Ns1Provider(BaseProvider): 'NS', 'PTR', 'SPF', 'SRV', 'TXT')) ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found' - CATCHALL_PREFIX = 'catchall__' def _update_filter(self, filter, with_disabled): if with_disabled: @@ -455,6 +454,13 @@ class Ns1Provider(BaseProvider): data['geo'] = geo return data + def _parse_dynamic_pool_name(self, pool_name): + try: + pool_name, _ = pool_name.rsplit('__', 1) + except ValueError: + pass + return pool_name + 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'], record['domain']): @@ -472,9 +478,8 @@ class Ns1Provider(BaseProvider): for answer in record['answers']: # region (group name in the UI) is the pool name pool_name = answer['region'] - # Get the actual pool name from the constructed pool name in case - # of the catchall - pool_name = pool_name.replace(self.CATCHALL_PREFIX, '') + # Get the actual pool name by removing the type + pool_name = self._parse_dynamic_pool_name(pool_name) pool = pools[pool_name] meta = answer['meta'] @@ -501,15 +506,24 @@ class Ns1Provider(BaseProvider): # tied to pools on the NS1 side, e.g. we can only have 1 rule per pool, # that may eventually run into problems, but I don't have any use-cases # examples currently where it would - rules = [] + rules = {} for pool_name, region in sorted(record['regions'].items()): - # Rules that refer to the catchall pool would have the - # CATCHALL_PREFIX in the pool name. Strip the prefix to get back - # the pool name as in the config - pool_name = pool_name.replace(self.CATCHALL_PREFIX, '') + # Get the actual pool name by removing the type + pool_name = self._parse_dynamic_pool_name(pool_name) + meta = region['meta'] notes = self._parse_notes(meta.get('note', '')) + rule_order = notes['rule-order'] + try: + rule = rules[rule_order] + except KeyError: + rule = { + 'pool': pool_name, + '_order': rule_order, + } + rules[rule_order] = rule + # The group notes field in the UI is a `note` on the region here, # that's where we can find our pool's fallback. if 'fallback' in notes: @@ -560,17 +574,15 @@ class Ns1Provider(BaseProvider): for state in meta.get('us_state', []): geos.add('NA-US-{}'.format(state)) - rule = { - 'pool': pool_name, - '_order': notes['rule-order'], - } if geos: - rule['geos'] = sorted(geos) - rules.append(rule) + # There are geos, combine them with any existing geos for this + # pool and recorded the sorted unique set of them + rule['geos'] = sorted(set(rule.get('geos', [])) | geos) # Order and convert to a list default = sorted(default) - # Order + # Convert to list and order + rules = list(rules.values()) rules.sort(key=lambda r: (r['_order'], r['pool'])) return { @@ -1050,29 +1062,34 @@ class Ns1Provider(BaseProvider): meta = { 'note': self._encode_notes(notes), } + if georegion: - meta['georegion'] = sorted(georegion) - if country: - meta['country'] = sorted(country) - if us_state: - meta['us_state'] = sorted(us_state) + georegion_meta = dict(meta) + georegion_meta['georegion'] = sorted(georegion) + regions['{}__georegion'.format(pool_name)] = { + 'meta': georegion_meta, + } + + if country or us_state: + # If there's country and/or states its a country pool, + # countries and states can coexist as they're handled by the + # same step in the filterchain (countries and georegions + # cannot as they're seperate stages and run the risk of + # eliminating all options) + country_state_meta = dict(meta) + if country: + country_state_meta['country'] = sorted(country) + if us_state: + country_state_meta['us_state'] = sorted(us_state) + regions['{}__country'.format(pool_name)] = { + 'meta': country_state_meta, + } if not georegion and not country and not us_state: - # This is the catchall pool. Modify the pool name in the record - # being pushed - # NS1 regions are indexed by pool names. Any reuse of pool - # names in the rules will result in overwriting of the pool. - # Reuse of pools is in general disallowed but for the case of - # the catchall pool - to allow legitimate usecases. - # The pool name renaming is done to accommodate for such a - # reuse. - # (We expect only one catchall per record. Any associated - # validation is expected to covered under record validation) - pool_name = '{}{}'.format(self.CATCHALL_PREFIX, pool_name) - - regions[pool_name] = { - 'meta': meta, - } + # If there's no targeting it's a catchall + regions['{}__catchall'.format(pool_name)] = { + 'meta': meta, + } existing_monitors = self._monitors_for(record) active_monitors = set() @@ -1102,15 +1119,14 @@ class Ns1Provider(BaseProvider): # Build our list of answers # The regions dictionary built above already has the required pool # names. Iterate over them and add answers. - # In the case of the catchall, original pool name can be obtained - # by stripping the CATCHALL_PREFIX from the pool name answers = [] for pool_name in sorted(regions.keys()): priority = 1 # Dynamic/health checked pool_label = pool_name - pool_name = pool_name.replace(self.CATCHALL_PREFIX, '') + # Remove the pool type from the end of the name + pool_name = self._parse_dynamic_pool_name(pool_name) self._add_answers_for_pool(answers, default_answers, pool_name, pool_label, pool_answers, pools, priority) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 336c985..1245c1c 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -985,6 +985,46 @@ class TestNs1ProviderDynamic(TestCase): rule0['geos'] = rule0_saved_geos rule1['geos'] = rule1_saved_geos + @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') + @patch('octodns.provider.ns1.Ns1Provider._monitors_for') + def test_params_for_dynamic_state_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] + rule1 = self.record.data['dynamic']['rules'][1] + rule0_saved_geos = rule0['geos'] + rule1_saved_geos = rule1['geos'] + rule0['geos'] = ['AF', 'EU'] + rule1['geos'] = ['NA-US-CA'] + ret, _ = provider._params_for_A(self.record) + exp = Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(provider, + True) + self.assertEquals(ret['filters'], exp) + rule0['geos'] = rule0_saved_geos + rule1['geos'] = rule1_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, @@ -1018,7 +1058,8 @@ class TestNs1ProviderDynamic(TestCase): saved_geos = rule0['geos'] rule0['geos'] = ['OC'] ret, _ = provider._params_for_A(self.record) - self.assertEquals(set(ret['regions']['lhr']['meta']['country']), + got = set(ret['regions']['lhr__country']['meta']['country']) + self.assertEquals(got, Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC']) # 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 @@ -1111,7 +1152,7 @@ class TestNs1ProviderDynamic(TestCase): # 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) - catchall_pool_name = '{}{}'.format(provider.CATCHALL_PREFIX, 'iad') + catchall_pool_name = '{}__catchall'.format('iad') ns1_record = { 'answers': [{ 'answer': ['3.4.5.6'], From d84aace823d3e81abe3a81179453b949fa3bcc68 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 23 Jun 2020 13:16:09 -0700 Subject: [PATCH 2/6] Don't modify a shared record, causes cascading test failures --- tests/test_octodns_provider_ns1.py | 164 +++++++++++++++-------------- 1 file changed, 87 insertions(+), 77 deletions(-) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 1245c1c..f092e76 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -528,52 +528,55 @@ class TestNs1Provider(TestCase): class TestNs1ProviderDynamic(TestCase): zone = Zone('unit.tests.', []) - record = Record.new(zone, '', { - 'dynamic': { - 'pools': { - 'lhr': { - 'fallback': 'iad', - 'values': [{ - 'value': '3.4.5.6', - }], - }, - 'iad': { - 'values': [{ - 'value': '1.2.3.4', - }, { - 'value': '2.3.4.5', - }], + def record(self): + # return a new object each time so we can mess with it without causing + # problems from test to test + return Record.new(self.zone, '', { + 'dynamic': { + 'pools': { + 'lhr': { + 'fallback': 'iad', + 'values': [{ + 'value': '3.4.5.6', + }], + }, + 'iad': { + 'values': [{ + 'value': '1.2.3.4', + }, { + 'value': '2.3.4.5', + }], + }, }, + 'rules': [{ + 'geos': [ + 'AF', + 'EU-GB', + 'NA-US-FL' + ], + 'pool': 'lhr', + }, { + 'geos': [ + 'AF-ZW', + ], + 'pool': 'iad', + }, { + 'pool': 'iad', + }], }, - 'rules': [{ - 'geos': [ - 'AF', - 'EU-GB', - 'NA-US-FL' - ], - 'pool': 'lhr', - }, { - 'geos': [ - 'AF-ZW', - ], - 'pool': 'iad', - }, { - 'pool': 'iad', - }], - }, - 'octodns': { - 'healthcheck': { - 'host': 'send.me', - 'path': '/_ping', - 'port': 80, - 'protocol': 'HTTP', - } - }, - 'ttl': 32, - 'type': 'A', - 'value': '1.2.3.4', - 'meta': {}, - }) + 'octodns': { + 'healthcheck': { + 'host': 'send.me', + 'path': '/_ping', + 'port': 80, + 'protocol': 'HTTP', + } + }, + 'ttl': 32, + 'type': 'A', + 'value': '1.2.3.4', + 'meta': {}, + }) def test_notes(self): provider = Ns1Provider('test', 'api-key') @@ -636,7 +639,7 @@ class TestNs1ProviderDynamic(TestCase): self.assertEquals({ '1.2.3.4': monitor_one, '2.3.4.5': monitor_four, - }, provider._monitors_for(self.record)) + }, provider._monitors_for(self.record())) def test_uuid(self): # Just a smoke test/for coverage @@ -707,18 +710,19 @@ class TestNs1ProviderDynamic(TestCase): provider = Ns1Provider('test', 'api-key') value = '3.4.5.6' - monitor = provider._monitor_gen(self.record, value) + record = self.record() + monitor = provider._monitor_gen(record, value) self.assertEquals(value, monitor['config']['host']) self.assertTrue('\\nHost: send.me\\r' in monitor['config']['send']) self.assertFalse(monitor['config']['ssl']) self.assertEquals('host:unit.tests type:A', monitor['notes']) - self.record._octodns['healthcheck']['protocol'] = 'HTTPS' - monitor = provider._monitor_gen(self.record, value) + record._octodns['healthcheck']['protocol'] = 'HTTPS' + monitor = provider._monitor_gen(record, value) self.assertTrue(monitor['config']['ssl']) - self.record._octodns['healthcheck']['protocol'] = 'TCP' - monitor = provider._monitor_gen(self.record, value) + record._octodns['healthcheck']['protocol'] = 'TCP' + monitor = provider._monitor_gen(record, value) # No http send done self.assertFalse('send' in monitor['config']) # No http response expected @@ -790,10 +794,11 @@ class TestNs1ProviderDynamic(TestCase): monitor_gen_mock.side_effect = [{'key': 'value'}] monitor_create_mock.side_effect = [('mon-id', 'feed-id')] value = '1.2.3.4' - monitor_id, feed_id = provider._monitor_sync(self.record, value, None) + record = self.record() + monitor_id, feed_id = provider._monitor_sync(record, value, None) self.assertEquals('mon-id', monitor_id) self.assertEquals('feed-id', feed_id) - monitor_gen_mock.assert_has_calls([call(self.record, value)]) + monitor_gen_mock.assert_has_calls([call(record, value)]) monitor_create_mock.assert_has_calls([call({'key': 'value'})]) monitors_update_mock.assert_not_called() feed_create_mock.assert_not_called() @@ -809,7 +814,7 @@ class TestNs1ProviderDynamic(TestCase): 'name': 'monitor name', } monitor_gen_mock.side_effect = [monitor] - monitor_id, feed_id = provider._monitor_sync(self.record, value, + monitor_id, feed_id = provider._monitor_sync(record, value, monitor) self.assertEquals('mon-id', monitor_id) self.assertEquals('feed-id', feed_id) @@ -830,7 +835,7 @@ class TestNs1ProviderDynamic(TestCase): } monitor_gen_mock.side_effect = [monitor] feed_create_mock.side_effect = ['feed-id2'] - monitor_id, feed_id = provider._monitor_sync(self.record, value, + monitor_id, feed_id = provider._monitor_sync(record, value, monitor) self.assertEquals('mon-id2', monitor_id) self.assertEquals('feed-id2', feed_id) @@ -853,7 +858,7 @@ class TestNs1ProviderDynamic(TestCase): 'other': 'thing', } monitor_gen_mock.side_effect = [gened] - monitor_id, feed_id = provider._monitor_sync(self.record, value, + monitor_id, feed_id = provider._monitor_sync(record, value, monitor) self.assertEquals('mon-id', monitor_id) self.assertEquals('feed-id', feed_id) @@ -883,8 +888,9 @@ class TestNs1ProviderDynamic(TestCase): monitors_delete_mock.reset_mock() notifylists_delete_mock.reset_mock() monitors_for_mock.side_effect = [{}] - provider._monitors_gc(self.record) - monitors_for_mock.assert_has_calls([call(self.record)]) + record = self.record() + provider._monitors_gc(record) + monitors_for_mock.assert_has_calls([call(record)]) datafeed_delete_mock.assert_not_called() monitors_delete_mock.assert_not_called() notifylists_delete_mock.assert_not_called() @@ -900,8 +906,8 @@ class TestNs1ProviderDynamic(TestCase): 'notify_list': 'nl-id', } }] - provider._monitors_gc(self.record) - monitors_for_mock.assert_has_calls([call(self.record)]) + provider._monitors_gc(record) + monitors_for_mock.assert_has_calls([call(record)]) datafeed_delete_mock.assert_has_calls([call('foo', 'feed-id')]) monitors_delete_mock.assert_has_calls([call('mon-id')]) notifylists_delete_mock.assert_has_calls([call('nl-id')]) @@ -917,8 +923,8 @@ class TestNs1ProviderDynamic(TestCase): 'notify_list': 'nl-id', } }] - provider._monitors_gc(self.record, {'mon-id'}) - monitors_for_mock.assert_has_calls([call(self.record)]) + provider._monitors_gc(record, {'mon-id'}) + monitors_for_mock.assert_has_calls([call(record)]) datafeed_delete_mock.assert_not_called() monitors_delete_mock.assert_not_called() notifylists_delete_mock.assert_not_called() @@ -939,8 +945,8 @@ class TestNs1ProviderDynamic(TestCase): 'notify_list': 'nl-id2', }, }] - provider._monitors_gc(self.record, {'mon-id'}) - monitors_for_mock.assert_has_calls([call(self.record)]) + provider._monitors_gc(record, {'mon-id'}) + monitors_for_mock.assert_has_calls([call(record)]) datafeed_delete_mock.assert_not_called() monitors_delete_mock.assert_has_calls([call('mon-id2')]) notifylists_delete_mock.assert_has_calls([call('nl-id2')]) @@ -972,13 +978,14 @@ class TestNs1ProviderDynamic(TestCase): ('mid-3', 'fid-3'), ] - rule0 = self.record.data['dynamic']['rules'][0] - rule1 = self.record.data['dynamic']['rules'][1] + record = self.record() + rule0 = record.data['dynamic']['rules'][0] + rule1 = record.data['dynamic']['rules'][1] rule0_saved_geos = rule0['geos'] rule1_saved_geos = rule1['geos'] rule0['geos'] = ['AF', 'EU'] rule1['geos'] = ['NA'] - ret, _ = provider._params_for_A(self.record) + ret, _ = provider._params_for_A(record) self.assertEquals(ret['filters'], Ns1Provider._FILTER_CHAIN_WITH_REGION(provider, True)) @@ -1012,13 +1019,14 @@ class TestNs1ProviderDynamic(TestCase): ('mid-3', 'fid-3'), ] - rule0 = self.record.data['dynamic']['rules'][0] - rule1 = self.record.data['dynamic']['rules'][1] + record = self.record() + rule0 = record.data['dynamic']['rules'][0] + rule1 = record.data['dynamic']['rules'][1] rule0_saved_geos = rule0['geos'] rule1_saved_geos = rule1['geos'] rule0['geos'] = ['AF', 'EU'] rule1['geos'] = ['NA-US-CA'] - ret, _ = provider._params_for_A(self.record) + ret, _ = provider._params_for_A(record) exp = Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(provider, True) self.assertEquals(ret['filters'], exp) @@ -1054,10 +1062,11 @@ class TestNs1ProviderDynamic(TestCase): # Set geos to 'OC' in rules[0] (pool - 'lhr') # Check returned dict has list of countries under 'OC' - rule0 = self.record.data['dynamic']['rules'][0] + record = self.record() + rule0 = record.data['dynamic']['rules'][0] saved_geos = rule0['geos'] rule0['geos'] = ['OC'] - ret, _ = provider._params_for_A(self.record) + ret, _ = provider._params_for_A(record) got = set(ret['regions']['lhr__country']['meta']['country']) self.assertEquals(got, Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC']) @@ -1092,19 +1101,20 @@ class TestNs1ProviderDynamic(TestCase): ] # This indirectly calls into _params_for_dynamic_A and tests the # handling to get there - ret, _ = provider._params_for_A(self.record) + record = self.record() + ret, _ = provider._params_for_A(record) - # Given that self.record has both country and region in the rules, + # 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)) - monitors_for_mock.assert_has_calls([call(self.record)]) + monitors_for_mock.assert_has_calls([call(record)]) monitors_sync_mock.assert_has_calls([ - call(self.record, '1.2.3.4', None), - call(self.record, '2.3.4.5', None), - call(self.record, '3.4.5.6', 'mid-3'), + call(record, '1.2.3.4', None), + call(record, '2.3.4.5', None), + call(record, '3.4.5.6', 'mid-3'), ]) record = Record.new(self.zone, 'geo', { From 680cd95e7360d323827881450a85d5e83a85804f Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 23 Jun 2020 13:16:42 -0700 Subject: [PATCH 3/6] Remove fragile save & restore record junk --- tests/test_octodns_provider_ns1.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index f092e76..2b4dbdd 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -981,16 +981,12 @@ class TestNs1ProviderDynamic(TestCase): record = self.record() rule0 = record.data['dynamic']['rules'][0] rule1 = record.data['dynamic']['rules'][1] - rule0_saved_geos = rule0['geos'] - rule1_saved_geos = rule1['geos'] rule0['geos'] = ['AF', 'EU'] rule1['geos'] = ['NA'] ret, _ = provider._params_for_A(record) self.assertEquals(ret['filters'], Ns1Provider._FILTER_CHAIN_WITH_REGION(provider, True)) - rule0['geos'] = rule0_saved_geos - rule1['geos'] = rule1_saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') @@ -1022,16 +1018,12 @@ class TestNs1ProviderDynamic(TestCase): record = self.record() rule0 = record.data['dynamic']['rules'][0] rule1 = record.data['dynamic']['rules'][1] - rule0_saved_geos = rule0['geos'] - rule1_saved_geos = rule1['geos'] rule0['geos'] = ['AF', 'EU'] rule1['geos'] = ['NA-US-CA'] ret, _ = provider._params_for_A(record) exp = Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(provider, True) self.assertEquals(ret['filters'], exp) - rule0['geos'] = rule0_saved_geos - rule1['geos'] = rule1_saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') @@ -1064,7 +1056,6 @@ class TestNs1ProviderDynamic(TestCase): # Check returned dict has list of countries under 'OC' record = self.record() rule0 = record.data['dynamic']['rules'][0] - saved_geos = rule0['geos'] rule0['geos'] = ['OC'] ret, _ = provider._params_for_A(record) got = set(ret['regions']['lhr__country']['meta']['country']) @@ -1075,7 +1066,6 @@ class TestNs1ProviderDynamic(TestCase): self.assertEquals(ret['filters'], Ns1Provider._FILTER_CHAIN_WITH_COUNTRY(provider, True)) - rule0['geos'] = saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') From a8cb831d2994227a9ff4ad1b80b0720207773d00 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 23 Jun 2020 13:47:46 -0700 Subject: [PATCH 4/6] Much more thorough testing of NS1 _params_for_dynamic_A --- tests/test_octodns_provider_ns1.py | 155 ++++++++++++++++++++++++++++- 1 file changed, 154 insertions(+), 1 deletion(-) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 2b4dbdd..0bbdbd6 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -983,10 +983,31 @@ class TestNs1ProviderDynamic(TestCase): rule1 = record.data['dynamic']['rules'][1] rule0['geos'] = ['AF', 'EU'] rule1['geos'] = ['NA'] - ret, _ = provider._params_for_A(record) + 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)) + self.assertEquals({ + 'iad__catchall': { + 'meta': { + 'note': 'rule-order:2' + } + }, + 'iad__georegion': { + 'meta': { + 'georegion': ['US-CENTRAL', 'US-EAST', 'US-WEST'], + 'note': 'rule-order:1' + } + }, + 'lhr__georegion': { + 'meta': { + 'georegion': ['AFRICA', 'EUROPE'], + 'note': 'fallback:iad rule-order:0' + } + } + }, ret['regions']) + self.assertEquals({'mid-1', 'mid-2', 'mid-3'}, monitor_ids) @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') @@ -1021,10 +1042,139 @@ class TestNs1ProviderDynamic(TestCase): rule0['geos'] = ['AF', 'EU'] rule1['geos'] = ['NA-US-CA'] ret, _ = provider._params_for_A(record) + self.assertEquals(10, len(ret['answers'])) + exp = Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(provider, + True) + self.assertEquals(ret['filters'], exp) + self.assertEquals({ + 'iad__catchall': { + 'meta': { + 'note': 'rule-order:2' + } + }, + 'iad__country': { + 'meta': { + 'note': 'rule-order:1', + 'us_state': ['CA'] + } + }, + 'lhr__georegion': { + 'meta': { + 'georegion': ['AFRICA', 'EUROPE'], + 'note': 'fallback:iad rule-order:0' + } + } + }, ret['regions']) + + @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') + @patch('octodns.provider.ns1.Ns1Provider._monitors_for') + def test_params_for_dynamic_contient_and_countries(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'), + ] + + record = self.record() + rule0 = record.data['dynamic']['rules'][0] + rule1 = record.data['dynamic']['rules'][1] + rule0['geos'] = ['AF', 'EU', 'NA-US-CA'] + rule1['geos'] = ['NA', 'NA-US'] + ret, _ = provider._params_for_A(record) + + self.assertEquals(17, len(ret['answers'])) + # Deeply check the answers we have here + # group the answers based on where they came from + notes = defaultdict(list) + for answer in ret['answers']: + notes[answer['meta']['note']].append(answer) + # Remove the meta and region part since it'll vary based on the + # exact pool, that'll let us == them down below + del answer['meta'] + del answer['region'] + + # Expected groups. iad has occurances in here: a country and region + # that was split out based on targeting a continent and a state. It + # finally has a catchall. Those are examples of the two ways pools get + # expanded. + # + # lhr splits in two, with a region and country. + # + # well as both lhr georegion (for contients) and country. The first is + # an example of a repeated target pool in a rule (only allowed when the + # 2nd is a catchall.) + self.assertEquals(['from:--default--', 'from:iad__catchall', + 'from:iad__country', 'from:iad__georegion', + 'from:lhr__country', 'from:lhr__georegion'], + sorted(notes.keys())) + + # All the iad's should match (after meta and region were removed) + self.assertEquals(notes['from:iad__catchall'], + notes['from:iad__country']) + self.assertEquals(notes['from:iad__catchall'], + notes['from:iad__georegion']) + + # The lhrs should match each other too + self.assertEquals(notes['from:lhr__georegion'], + notes['from:lhr__country']) + + # We have both country and region filter chain entries exp = Ns1Provider._FILTER_CHAIN_WITH_REGION_AND_COUNTRY(provider, True) self.assertEquals(ret['filters'], exp) + # and our region details match the expected behaviors/targeting + self.assertEquals({ + 'iad__catchall': { + 'meta': { + 'note': 'rule-order:2' + } + }, + 'iad__country': { + 'meta': { + 'country': ['US'], + 'note': 'rule-order:1' + } + }, + 'iad__georegion': { + 'meta': { + 'georegion': ['US-CENTRAL', 'US-EAST', 'US-WEST'], + 'note': 'rule-order:1' + } + }, + 'lhr__country': { + 'meta': { + 'note': 'fallback:iad rule-order:0', + 'us_state': ['CA'] + } + }, + 'lhr__georegion': { + 'meta': { + 'georegion': ['AFRICA', 'EUROPE'], + 'note': 'fallback:iad rule-order:0' + } + } + }, ret['regions']) + @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') def test_params_for_dynamic_oceania(self, monitors_for_mock, @@ -1058,9 +1208,12 @@ class TestNs1ProviderDynamic(TestCase): rule0 = record.data['dynamic']['rules'][0] rule0['geos'] = ['OC'] ret, _ = provider._params_for_A(record) + + # Make sure the country list expanded into all the OC countries got = set(ret['regions']['lhr__country']['meta']['country']) self.assertEquals(got, Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC']) + # 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'], From 2938c7bf6abd3c2226679b058c832e87aa0e9da7 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 23 Jun 2020 13:57:14 -0700 Subject: [PATCH 5/6] Test out the new naming/code paths for NS1 region populate/combining --- tests/test_octodns_provider_ns1.py | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 0bbdbd6..575ce32 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1311,7 +1311,7 @@ class TestNs1ProviderDynamic(TestCase): 'answer': ['3.4.5.6'], 'meta': { 'priority': 1, - 'note': 'from:lhr', + 'note': 'from:lhr__country', }, 'region': 'lhr', }, { @@ -1363,14 +1363,24 @@ class TestNs1ProviderDynamic(TestCase): 'domain': 'unit.tests', 'filters': filters, 'regions': { - 'lhr': { + # lhr will use the new-split style names (and that will require + # combining in the code to produce the expected answer + 'lhr__georegion': { 'meta': { 'note': 'rule-order:1 fallback:iad', - 'country': ['CA'], 'georegion': ['AFRICA'], + }, + }, + 'lhr__country': { + 'meta': { + 'note': 'rule-order:1 fallback:iad', + 'country': ['CA'], 'us_state': ['OR'], }, }, + # iad will use the old style "plain" region naming. We won't + # see mixed names like this in practice, but this should + # exercise both paths 'iad': { 'meta': { 'note': 'rule-order:2', @@ -1437,13 +1447,15 @@ class TestNs1ProviderDynamic(TestCase): # Oceania test cases # 1. Full list of countries should return 'OC' in geos oc_countries = Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC'] - ns1_record['regions']['lhr']['meta']['country'] = list(oc_countries) + ns1_record['regions']['lhr__country']['meta']['country'] = \ + list(oc_countries) data3 = provider._data_for_A('A', ns1_record) self.assertTrue('OC' in data3['dynamic']['rules'][0]['geos']) # 2. Partial list of countries should return just those partial_oc_cntry_list = list(oc_countries)[:5] - ns1_record['regions']['lhr']['meta']['country'] = partial_oc_cntry_list + ns1_record['regions']['lhr__country']['meta']['country'] = \ + partial_oc_cntry_list data4 = provider._data_for_A('A', ns1_record) for c in partial_oc_cntry_list: self.assertTrue( From 0830b9c1145a4f283f038fecf481d9d9fa1c5ad9 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 23 Jun 2020 14:54:40 -0700 Subject: [PATCH 6/6] Handle and test for old-style NS1 catchall naming pattern --- octodns/provider/ns1.py | 3 +++ tests/test_octodns_provider_ns1.py | 12 +++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 749686d..6cea185 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -455,6 +455,9 @@ class Ns1Provider(BaseProvider): return data def _parse_dynamic_pool_name(self, pool_name): + if pool_name.startswith('catchall__'): + # Special case for the old-style catchall prefix + return pool_name[10:] try: pool_name, _ = pool_name.rsplit('__', 1) except ValueError: diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 575ce32..00b068b 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1305,7 +1305,7 @@ class TestNs1ProviderDynamic(TestCase): # 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) - catchall_pool_name = '{}__catchall'.format('iad') + catchall_pool_name = 'iad__catchall' ns1_record = { 'answers': [{ 'answer': ['3.4.5.6'], @@ -1444,6 +1444,16 @@ class TestNs1ProviderDynamic(TestCase): data2 = provider._data_for_A('A', ns1_record) self.assertEquals(data, data2) + # Same answer if we have an old-style catchall name + old_style_catchall_pool_name = 'catchall__iad' + ns1_record['answers'][-2]['region'] = old_style_catchall_pool_name + ns1_record['answers'][-1]['region'] = old_style_catchall_pool_name + ns1_record['regions'][old_style_catchall_pool_name] = \ + ns1_record['regions'][catchall_pool_name] + del ns1_record['regions'][catchall_pool_name] + data3 = provider._data_for_dynamic_A('A', ns1_record) + self.assertEquals(data, data2) + # Oceania test cases # 1. Full list of countries should return 'OC' in geos oc_countries = Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC']