From 71a277f6ad9e443e3ed56b44f05028fb6cf996d5 Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Tue, 3 Mar 2020 11:07:29 -0800 Subject: [PATCH 1/2] NS1 doesn't support region OC. Handle it explicitly in the provider --- octodns/provider/ns1.py | 44 +++++++++++++++++++++-- tests/test_octodns_provider_ns1.py | 57 ++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index a25911d..04c52f6 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -227,6 +227,13 @@ class Ns1Provider(BaseProvider): 'NA': ('US-CENTRAL', 'US-EAST', 'US-WEST'), } + # Necessary for handling unsupported continents in _CONTINENT_TO_REGIONS + _CONTINENT_TO_LIST_OF_COUNTRIES = { + 'OC': ['FJ', 'NC', 'PG', 'SB', 'VU', 'AU', 'NF', 'NZ', 'FM', 'GU', + 'KI', 'MH', 'MP', 'NR', 'PW', 'AS', 'CK', 'NU', 'PF', 'PN', + 'TK', 'TO', 'TV', 'WF', 'WS'], + } + def __init__(self, id, api_key, retry_count=4, monitor_regions=None, *args, **kwargs): self.log = getLogger('Ns1Provider[{}]'.format(id)) @@ -351,12 +358,35 @@ class Ns1Provider(BaseProvider): for georegion in meta.get('georegion', []): geos.add(self._REGION_TO_CONTINENT[georegion]) - # Countries are easy enough to map, we just have ot find their + # Countries are easy enough to map, we just have to find their # continent + # + # NOTE: Special handling for Oceania + # NS1 doesn't support Oceania as a region. So the Oceania countries + # will be present in meta['country']. If all the countries in the + # Oceania countries list are found, set the region to OC and remove + # individual oceania country entries + + oc_countries = [] for country in meta.get('country', []): - con = country_alpha2_to_continent_code(country) + # country_alpha2_to_continent_code fails for Pitcairn ('PN') + if country == 'PN': + con = 'OC' + else: + con = country_alpha2_to_continent_code(country) + + if con == 'OC': + oc_countries.append(country) geos.add('{}-{}'.format(con, country)) + if len(oc_countries): + all_oc_countries = self._CONTINENT_TO_LIST_OF_COUNTRIES['OC'] + if sorted(oc_countries) == sorted(all_oc_countries): + # Remove oceania countries from the map, add 'OC' region + for c in oc_countries: + geos.remove('{}-{}'.format('OC', c)) + geos.add('OC') + # States are easy too, just assume NA-US (CA providences aren't # supported by octoDNS currently) for state in meta.get('us_state', []): @@ -782,7 +812,15 @@ class Ns1Provider(BaseProvider): country.add(geo[-2:]) else: # Continent, e.g. AS - georegion.update(self._CONTINENT_TO_REGIONS[geo]) + if geo in self._CONTINENT_TO_REGIONS: + georegion.update(self._CONTINENT_TO_REGIONS[geo]) + else: + # No maps for geo in _CONTINENT_TO_REGIONS. + # Use the country list + self.log.debug('Converting geo {} to country list'. + format(geo)) + for c in self._CONTINENT_TO_LIST_OF_COUNTRIES[geo]: + country.add(c) meta = { 'note': self._encode_notes(notes), diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 1d4e8f9..e20b7b9 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -521,6 +521,12 @@ class TestNs1Provider(TestCase): class TestNs1ProviderDynamic(TestCase): zone = Zone('unit.tests.', []) + _CONTINENT_TO_LIST_OF_COUNTRIES = { + 'OC': ['FJ', 'NC', 'PG', 'SB', 'VU', 'AU', 'NF', 'NZ', 'FM', 'GU', + 'KI', 'MH', 'MP', 'NR', 'PW', 'AS', 'CK', 'NU', 'PF', 'PN', + 'TK', 'TO', 'TV', 'WF', 'WS'], + } + record = Record.new(zone, '', { 'dynamic': { 'pools': { @@ -926,6 +932,43 @@ 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_oceania(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'), + ] + + # 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] + saved_geos = rule0['geos'] + rule0['geos'] = ['OC'] + ret, _ = provider._params_for_A(self.record) + self.assertEquals(sorted(ret['regions']['lhr']['meta']['country']), + sorted(self._CONTINENT_TO_LIST_OF_COUNTRIES['OC'])) + rule0['geos'] = saved_geos + @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') def test_params_for_dynamic(self, monitors_for_mock, monitors_sync_mock): @@ -1092,6 +1135,20 @@ class TestNs1ProviderDynamic(TestCase): data2 = provider._data_for_A('A', ns1_record) self.assertEquals(data, data2) + # Oceania test cases + # 1. Full list of countries should return 'OC' in geos + oc_country_list = self._CONTINENT_TO_LIST_OF_COUNTRIES['OC'] + ns1_record['regions']['lhr']['meta']['country'] = oc_country_list + data3 = provider._data_for_A('A', ns1_record) + assert('OC' in data3['dynamic']['rules'][0]['geos']) + + # 2. Partial list of countries should return just those + partial_oc_cntry_list = oc_country_list[:5] + ns1_record['regions']['lhr']['meta']['country'] = partial_oc_cntry_list + data4 = provider._data_for_A('A', ns1_record) + for c in partial_oc_cntry_list: + assert('OC-{}'.format(c) in data4['dynamic']['rules'][0]['geos']) + @patch('octodns.provider.ns1.Ns1Provider._monitors_for') def test_extra_changes(self, monitors_for_mock): provider = Ns1Provider('test', 'api-key') From 0daa37578b3b4a02239cdc97ee199f92307e4beb Mon Sep 17 00:00:00 2001 From: Pavan Chandrashekar Date: Tue, 3 Mar 2020 16:36:08 -0800 Subject: [PATCH 2/2] Address review comments --- octodns/provider/ns1.py | 25 ++++++++++++++----------- tests/test_octodns_provider_ns1.py | 21 ++++++++------------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 04c52f6..2a3ae07 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -229,9 +229,9 @@ class Ns1Provider(BaseProvider): # Necessary for handling unsupported continents in _CONTINENT_TO_REGIONS _CONTINENT_TO_LIST_OF_COUNTRIES = { - 'OC': ['FJ', 'NC', 'PG', 'SB', 'VU', 'AU', 'NF', 'NZ', 'FM', 'GU', + 'OC': {'FJ', 'NC', 'PG', 'SB', 'VU', 'AU', 'NF', 'NZ', 'FM', 'GU', 'KI', 'MH', 'MP', 'NR', 'PW', 'AS', 'CK', 'NU', 'PF', 'PN', - 'TK', 'TO', 'TV', 'WF', 'WS'], + 'TK', 'TO', 'TV', 'WF', 'WS'}, } def __init__(self, id, api_key, retry_count=4, monitor_regions=None, *args, @@ -367,7 +367,7 @@ class Ns1Provider(BaseProvider): # Oceania countries list are found, set the region to OC and remove # individual oceania country entries - oc_countries = [] + oc_countries = set() for country in meta.get('country', []): # country_alpha2_to_continent_code fails for Pitcairn ('PN') if country == 'PN': @@ -376,16 +376,19 @@ class Ns1Provider(BaseProvider): con = country_alpha2_to_continent_code(country) if con == 'OC': - oc_countries.append(country) - geos.add('{}-{}'.format(con, country)) + oc_countries.add(country) + else: + # Adding only non-OC countries here to geos + geos.add('{}-{}'.format(con, country)) - if len(oc_countries): - all_oc_countries = self._CONTINENT_TO_LIST_OF_COUNTRIES['OC'] - if sorted(oc_countries) == sorted(all_oc_countries): - # Remove oceania countries from the map, add 'OC' region - for c in oc_countries: - geos.remove('{}-{}'.format('OC', c)) + if oc_countries: + if oc_countries == self._CONTINENT_TO_LIST_OF_COUNTRIES['OC']: + # All OC countries found, so add 'OC' to geos geos.add('OC') + else: + # Partial OC countries found, just add them as-is to geos + for c in oc_countries: + geos.add('{}-{}'.format('OC', c)) # States are easy too, just assume NA-US (CA providences aren't # supported by octoDNS currently) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index e20b7b9..8126c23 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -521,12 +521,6 @@ class TestNs1Provider(TestCase): class TestNs1ProviderDynamic(TestCase): zone = Zone('unit.tests.', []) - _CONTINENT_TO_LIST_OF_COUNTRIES = { - 'OC': ['FJ', 'NC', 'PG', 'SB', 'VU', 'AU', 'NF', 'NZ', 'FM', 'GU', - 'KI', 'MH', 'MP', 'NR', 'PW', 'AS', 'CK', 'NU', 'PF', 'PN', - 'TK', 'TO', 'TV', 'WF', 'WS'], - } - record = Record.new(zone, '', { 'dynamic': { 'pools': { @@ -965,8 +959,8 @@ class TestNs1ProviderDynamic(TestCase): saved_geos = rule0['geos'] rule0['geos'] = ['OC'] ret, _ = provider._params_for_A(self.record) - self.assertEquals(sorted(ret['regions']['lhr']['meta']['country']), - sorted(self._CONTINENT_TO_LIST_OF_COUNTRIES['OC'])) + self.assertEquals(set(ret['regions']['lhr']['meta']['country']), + Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC']) rule0['geos'] = saved_geos @patch('octodns.provider.ns1.Ns1Provider._monitor_sync') @@ -1137,17 +1131,18 @@ class TestNs1ProviderDynamic(TestCase): # Oceania test cases # 1. Full list of countries should return 'OC' in geos - oc_country_list = self._CONTINENT_TO_LIST_OF_COUNTRIES['OC'] - ns1_record['regions']['lhr']['meta']['country'] = oc_country_list + oc_countries = Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['OC'] + ns1_record['regions']['lhr']['meta']['country'] = list(oc_countries) data3 = provider._data_for_A('A', ns1_record) - assert('OC' in data3['dynamic']['rules'][0]['geos']) + self.assertTrue('OC' in data3['dynamic']['rules'][0]['geos']) # 2. Partial list of countries should return just those - partial_oc_cntry_list = oc_country_list[:5] + partial_oc_cntry_list = list(oc_countries)[:5] ns1_record['regions']['lhr']['meta']['country'] = partial_oc_cntry_list data4 = provider._data_for_A('A', ns1_record) for c in partial_oc_cntry_list: - assert('OC-{}'.format(c) in data4['dynamic']['rules'][0]['geos']) + self.assertTrue( + 'OC-{}'.format(c) in data4['dynamic']['rules'][0]['geos']) @patch('octodns.provider.ns1.Ns1Provider._monitors_for') def test_extra_changes(self, monitors_for_mock):