Merge pull request #809 from octodns/ns1-explicit-countries

Skip explicit countries when converting continent to countries in NS1

CHANGELOG.md

@@ -6,6 +6,12 @@
 * NS1 provider has received improvements to the dynamic record implementation.
   As a result, if octoDNS is downgraded from this version, any dynamic records
   created or updated using this version will show an update.
+* An edge-case bug related to geo rules involving continents in NS1 provider
+  has been fixed in this version. However, it will not show/fix the records that
+  match this edge-case. See https://github.com/octodns/octodns/pull/809 for
+  more information. If octoDNS is downgraded from this version, any dynamic
+  records created or updated using this version and matching the said edge-case
+  will not be read/parsed correctly by the older version and will show a diff.
 
 ## v0.9.14 - 2021-10-10 - A new supports system
 

octodns/provider/ns1.py

@@ -613,7 +613,7 @@ class Ns1Provider(BaseProvider):
 
         return default, pools
 
-    def _parse_rule_geos(self, meta):
+    def _parse_rule_geos(self, meta, notes):
         geos = set()
 
         for georegion in meta.get('georegion', []):
@@ -629,6 +629,11 @@ class Ns1Provider(BaseProvider):
         # in _CONTINENT_TO_LIST_OF_COUNTRIES[<region>] list are found,
         # set the continent as the region and remove individual countries
 
+        # continents that don't have all countries here because a subset of
+        # them were used in another rule, but we still need this rule to use
+        # continent instead of the remaining subset of its countries
+        continents_from_notes = set(notes.get('continents', '').split(','))
+
         special_continents = dict()
         for country in meta.get('country', []):
             # country_alpha2_to_continent_code fails for Pitcairn ('PN'),
@@ -648,8 +653,8 @@ class Ns1Provider(BaseProvider):
 
         for continent, countries in special_continents.items():
             if countries == self._CONTINENT_TO_LIST_OF_COUNTRIES[
-                    continent]:
-                # All countries found, so add it to geos
+                    continent] or continent in continents_from_notes:
+                # All countries found or continent in notes, so add it to geos
                 geos.add(continent)
             else:
                 # Partial countries found, so just add them as-is to geos
@@ -695,7 +700,7 @@ class Ns1Provider(BaseProvider):
                 }
                 rules[rule_order] = rule
 
-            geos = self._parse_rule_geos(meta)
+            geos = self._parse_rule_geos(meta, notes)
             if geos:
                 # There are geos, combine them with any existing geos for this
                 # pool and recorded the sorted unique set of them
@@ -1248,6 +1253,13 @@ class Ns1Provider(BaseProvider):
         has_region = False
         regions = {}
 
+        explicit_countries = dict()
+        for rule in record.dynamic.rules:
+            for geo in rule.data.get('geos', []):
+                if len(geo) == 5:
+                    con, country = geo.split('-', 1)
+                    explicit_countries.setdefault(con, set()).add(country)
+
         for i, rule in enumerate(record.dynamic.rules):
             pool_name = rule.data['pool']
 
@@ -1288,9 +1300,15 @@ class Ns1Provider(BaseProvider):
                         # 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)
-                            has_country = True
+                        continent_countries = \
+                            self._CONTINENT_TO_LIST_OF_COUNTRIES[geo]
+                        exclude = explicit_countries.get(geo, set())
+                        country.update(continent_countries - exclude)
+                        notes.setdefault('continents', set()).add(geo)
+                        has_country = True
+
+            if 'continents' in notes:
+                notes['continents'] = ','.join(sorted(notes['continents']))
 
             meta = {
                 'note': self._encode_notes(notes),

tests/test_octodns_provider_ns1.py

@@ -2056,6 +2056,56 @@ class TestNs1ProviderDynamic(TestCase):
             'value': None,
         }, data)
 
+    @patch('octodns.provider.ns1.Ns1Provider._monitor_sync')
+    @patch('octodns.provider.ns1.Ns1Provider._monitors_for')
+    def test_dynamic_explicit_countries(self, monitors_for_mock,
+                                        monitors_sync_mock):
+        provider = Ns1Provider('test', 'api-key')
+        record_data = {
+            'dynamic': {
+                'pools': {
+                    'iad': {
+                        'values': [{
+                            'value': 'iad.unit.tests.',
+                            'status': 'up',
+                        }],
+                    },
+                    'lhr': {
+                        'values': [{
+                            'value': 'lhr.unit.tests.',
+                            'status': 'up',
+                        }]
+                    }
+                },
+                'rules': [
+                    {
+                        'geos': ['NA-US'],
+                        'pool': 'iad',
+                    },
+                    {
+                        'geos': ['NA'],
+                        'pool': 'lhr',
+                    },
+                ],
+            },
+            'ttl': 33,
+            'type': 'CNAME',
+            'value': 'value.unit.tests.',
+        }
+        record = Record.new(self.zone, 'foo', record_data)
+
+        ns1_record, _ = provider._params_for_dynamic(record)
+        regions = [
+            r for r in ns1_record['regions'].values()
+            if 'US' in r['meta']['country']
+        ]
+        self.assertEquals(len(regions), 1)
+
+        ns1_record['domain'] = record.fqdn[:-1]
+        data = provider._data_for_dynamic(record._type, ns1_record)['dynamic']
+        self.assertEquals(data['rules'][0]['geos'], ['NA-US'])
+        self.assertEquals(data['rules'][1]['geos'], ['NA'])
+
     @patch('ns1.rest.records.Records.retrieve')
     @patch('ns1.rest.zones.Zones.retrieve')
     @patch('octodns.provider.ns1.Ns1Provider._monitors_for')