Update country filter conditionally instead of changing the default

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

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': {