From bbe4dc2d3e220aab1f4cbf66d415dbd55d0f7da7 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 23 Jun 2020 09:49:37 -0700 Subject: [PATCH] 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'],