Merge pull request #731 from meghashyamps/NA-limitation-fix

fix for NA continent geo target limitation on NS1

CHANGELOG.md

@@ -7,6 +7,12 @@
   to data. See the [octodns/processor/](/octodns/processor) directory for
   examples. The change has been designed to have no impact on the process
   unless the `processors` key is present in zone configs.
+* Fixes NS1 provider's geotarget limitation of using `NA` continent. Now, when
+  `NA` is used in geos it considers **all** the countries of `North America`
+  insted of just `us-east`, `us-west` and `us-central` regions
+* `SX' & 'UM` country support added to NS1Provider, not yet in the North 
+   America list for backwards compatibility reasons. They will be added in the
+   next releaser.
 
 ## v0.9.12 - 2021-04-30 - Enough time has passed
 

README.md

@@ -207,7 +207,7 @@ The above command pulled the existing data out of Route53 and placed the results
 | [GoogleCloudProvider](/octodns/provider/googlecloud.py) | google-cloud-dns | A, AAAA, CAA, CNAME, MX, NAPTR, NS, PTR, SPF, SRV, TXT  | No | |
 | [HetznerProvider](/octodns/provider/hetzner.py) | | A, AAAA, CAA, CNAME, MX, NS, SRV, TXT | No | |
 | [MythicBeastsProvider](/octodns/provider/mythicbeasts.py) | Mythic Beasts | A, AAAA, ALIAS, CNAME, MX, NS, SRV, SSHFP, CAA, TXT | No | |
-| [Ns1Provider](/octodns/provider/ns1.py) | ns1-python | All | Yes | Missing `NA` geo target |
+| [Ns1Provider](/octodns/provider/ns1.py) | ns1-python | All | Yes | |
 | [OVH](/octodns/provider/ovh.py) | ovh | A, AAAA, CAA, CNAME, MX, NAPTR, NS, PTR, SPF, SRV, SSHFP, TXT, DKIM | No | |
 | [PowerDnsProvider](/octodns/provider/powerdns.py) | | All | No | |
 | [Rackspace](/octodns/provider/rackspace.py) | | A, AAAA, ALIAS, CNAME, MX, NS, PTR, SPF, TXT | No |  |

octodns/provider/ns1.py

@@ -341,6 +341,9 @@ class Ns1Provider(BaseProvider):
         'ASIAPAC': 'AS',
         'EUROPE': 'EU',
         'SOUTH-AMERICA': 'SA',
+        # continent NA has been handled as part of Geofence Country filter
+        # starting from v0.9.13. These below US-* just need to continue to
+        # exist here so it doesn't break the ugrade path
         'US-CENTRAL': 'NA',
         'US-EAST': 'NA',
         'US-WEST': 'NA',
@@ -350,8 +353,6 @@ class Ns1Provider(BaseProvider):
         'AS': ('ASIAPAC',),
         'EU': ('EUROPE',),
         'SA': ('SOUTH-AMERICA',),
-        # TODO: what about CA, MX, and all the other NA countries?
-        'NA': ('US-CENTRAL', 'US-EAST', 'US-WEST'),
     }
 
     # Necessary for handling unsupported continents in _CONTINENT_TO_REGIONS
@@ -359,6 +360,10 @@ class Ns1Provider(BaseProvider):
         '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'},
+        'NA': {'DO', 'DM', 'BB', 'BL', 'BM', 'HT', 'KN', 'JM', 'VC', 'HN',
+               'BS', 'BZ', 'PR', 'NI', 'LC', 'TT', 'VG', 'PA', 'TC', 'PM',
+               'GT', 'AG', 'GP', 'AI', 'VI', 'CA', 'GD', 'AW', 'CR', 'GL',
+               'CU', 'MF', 'SV', 'US', 'MQ', 'MS', 'KY', 'MX', 'CW', 'BQ'}
     }
 
     def __init__(self, id, api_key, retry_count=4, monitor_regions=None,
@@ -549,42 +554,45 @@ class Ns1Provider(BaseProvider):
 
             geos = set()
 
-            # continents are mapped (imperfectly) to regions, but what about
-            # Canada/North America
             for georegion in meta.get('georegion', []):
                 geos.add(self._REGION_TO_CONTINENT[georegion])
 
             # 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 = set()
+            # NOTE: Some continents need special handling since NS1
+            # does not supprt them as regions. These are defined under
+            # _CONTINENT_TO_LIST_OF_COUNTRIES. So the countries for these
+            # regions will be present in meta['country']. If all the countries
+            # in _CONTINENT_TO_LIST_OF_COUNTRIES[<region>] list are found,
+            # set the continent as the region and remove individual countries
+
+            special_continents = dict()
             for country in meta.get('country', []):
-                # country_alpha2_to_continent_code fails for Pitcairn ('PN')
+                # country_alpha2_to_continent_code fails for Pitcairn ('PN'),
+                # United States Minor Outlying Islands ('UM') and
+                # Sint Maarten ('SX')
                 if country == 'PN':
                     con = 'OC'
+                elif country in ['SX', 'UM']:
+                    con = 'NA'
                 else:
                     con = country_alpha2_to_continent_code(country)
 
-                if con == 'OC':
-                    oc_countries.add(country)
+                if con in self._CONTINENT_TO_LIST_OF_COUNTRIES:
+                    special_continents.setdefault(con, set()).add(country)
                 else:
-                    # Adding only non-OC countries here to geos
                     geos.add('{}-{}'.format(con, country))
 
-            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')
+            for continent, countries in special_continents.items():
+                if countries == self._CONTINENT_TO_LIST_OF_COUNTRIES[
+                        continent]:
+                    # All countries found, so add it to geos
+                    geos.add(continent)
                 else:
-                    # Partial OC countries found, just add them as-is to geos
-                    for c in oc_countries:
-                        geos.add('{}-{}'.format('OC', c))
+                    # Partial countries found, so just add them as-is to geos
+                    for c in countries:
+                        geos.add('{}-{}'.format(continent, c))
 
             # States are easy too, just assume NA-US (CA providences aren't
             # supported by octoDNS currently)

tests/test_octodns_provider_ns1.py

@@ -1034,7 +1034,7 @@ class TestNs1ProviderDynamic(TestCase):
         rule0 = record.data['dynamic']['rules'][0]
         rule1 = record.data['dynamic']['rules'][1]
         rule0['geos'] = ['AF', 'EU']
-        rule1['geos'] = ['NA']
+        rule1['geos'] = ['AS']
         ret, monitor_ids = provider._params_for_A(record)
         self.assertEquals(10, len(ret['answers']))
         self.assertEquals(ret['filters'],
@@ -1048,7 +1048,7 @@ class TestNs1ProviderDynamic(TestCase):
             },
             'iad__georegion': {
                 'meta': {
-                    'georegion': ['US-CENTRAL', 'US-EAST', 'US-WEST'],
+                    'georegion': ['ASIAPAC'],
                     'note': 'rule-order:1'
                 }
             },
@@ -1150,7 +1150,7 @@ class TestNs1ProviderDynamic(TestCase):
         rule0 = record.data['dynamic']['rules'][0]
         rule1 = record.data['dynamic']['rules'][1]
         rule0['geos'] = ['AF', 'EU', 'NA-US-CA']
-        rule1['geos'] = ['NA', 'NA-US']
+        rule1['geos'] = ['AS', 'AS-IN']
         ret, _ = provider._params_for_A(record)
 
         self.assertEquals(17, len(ret['answers']))
@@ -1210,13 +1210,13 @@ class TestNs1ProviderDynamic(TestCase):
             },
             'iad__country': {
                 'meta': {
-                    'country': ['US'],
+                    'country': ['IN'],
                     'note': 'rule-order:1'
                 }
             },
             'iad__georegion': {
                 'meta': {
-                    'georegion': ['US-CENTRAL', 'US-EAST', 'US-WEST'],
+                    'georegion': ['ASIAPAC'],
                     'note': 'rule-order:1'
                 }
             },
@@ -1562,6 +1562,24 @@ class TestNs1ProviderDynamic(TestCase):
             self.assertTrue(
                 'OC-{}'.format(c) in data4['dynamic']['rules'][0]['geos'])
 
+        # NA test cases
+        # 1. Full list of countries should return 'NA' in geos
+        na_countries = Ns1Provider._CONTINENT_TO_LIST_OF_COUNTRIES['NA']
+        del ns1_record['regions']['lhr__country']['meta']['us_state']
+        ns1_record['regions']['lhr__country']['meta']['country'] = \
+            list(na_countries)
+        data5 = provider._data_for_A('A', ns1_record)
+        self.assertTrue('NA' in data5['dynamic']['rules'][0]['geos'])
+
+        # 2. Partial list of countries should return just those
+        partial_na_cntry_list = list(na_countries)[:5] + ['SX', 'UM']
+        ns1_record['regions']['lhr__country']['meta']['country'] = \
+            partial_na_cntry_list
+        data6 = provider._data_for_A('A', ns1_record)
+        for c in partial_na_cntry_list:
+            self.assertTrue(
+                'NA-{}'.format(c) in data6['dynamic']['rules'][0]['geos'])
+
         # Test out fallback only pools and new-style notes
         ns1_record = {
             'answers': [{