From f0bc9add22bd29b814e61d0740ac14ad6e6906ab Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 9 Dec 2019 14:30:02 -0800
Subject: [PATCH 01/21] Rough draft/expirimentation on dynamic creation

---
 octodns/provider/ns1.py | 167 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 163 insertions(+), 4 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index da2d64a..445e612 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -70,13 +70,54 @@ class Ns1Provider(BaseProvider):
         class: octodns.provider.ns1.Ns1Provider
         api_key: env/NS1_API_KEY
     '''
-    SUPPORTS_GEO = True
-    SUPPORTS_DYNAMIC = False
+    SUPPORTS_GEO = False
+    SUPPORTS_DYNAMIC = True
     SUPPORTS = set(('A', 'AAAA', 'ALIAS', 'CAA', 'CNAME', 'MX', 'NAPTR',
                     'NS', 'PTR', 'SPF', 'SRV', 'TXT'))
 
     ZONE_NOT_FOUND_MESSAGE = 'server error: zone not found'
 
+    _DYNAMIC_FILTERS = [{
+        'config': {},
+        'filter': 'up'
+    }, {
+        'config': {},
+        'filter': u'geotarget_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',
+        'EUROPE': 'EU',
+        'SOUTH-AMERICA': 'SA',
+        'US-CENTRAL': 'NA',
+        'US-EAST': 'NA',
+        'US-WEST': 'NA',
+    }
+    _CONTINENT_TO_REGIONS = {
+        'AF': ('AFRICA',),
+        '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'),
+    }
+
     def __init__(self, id, api_key, retry_count=4, *args, **kwargs):
         self.log = getLogger('Ns1Provider[{}]'.format(id))
         self.log.debug('__init__: id=%s, api_key=***, retry_count=%d', id,
@@ -282,9 +323,124 @@ class Ns1Provider(BaseProvider):
                       len(zone.records) - before, exists)
         return exists
 
+    def _encode_notes(self, data):
+        return ' '.join(['{}:{}'.format(k, v)
+                         for k, v in sorted(data.items())])
+
     def _params_for_A(self, record):
-        params = {'answers': record.values, 'ttl': record.ttl}
-        if hasattr(record, 'geo'):
+        params = {'ttl': record.ttl}
+
+        if hasattr(record, 'dynamic'):
+
+            pools = record.dynamic.pools
+
+            # Convert rules to regions
+            regions = {}
+            for i, rule in enumerate(record.dynamic.rules):
+                pool_name = rule.data['pool']
+
+                notes = {
+                    'rule-order': i,
+                }
+
+                fallback = pools[pool_name].data.get('fallback', None)
+                if fallback:
+                    notes['fallback'] = fallback
+
+                country = set()
+                georegion = set()
+                us_state = set()
+
+                for geo in rule.data.get('geos', []):
+                    n = len(geo)
+                    if n == 8:
+                        # US state
+                        us_state.add(geo[-2:])
+                    elif n == 5:
+                        # Country
+                        country.add(geo[-2:])
+                    else:
+                        # Continent
+                        georegion.update(self._CONTINENT_TO_REGIONS[geo])
+
+                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)
+
+                regions[pool_name] = {
+                    'meta': meta,
+                }
+
+            # Build a list of primary values for each pool
+            pool_answers = defaultdict(list)
+            for pool_name, pool in sorted(pools.items()):
+                for value in pool.data['values']:
+                    pool_answers[pool_name].append({
+                        'answer': [value['value']],
+                        'weight': value['weight'],
+                    })
+
+            default_answers = [{
+                'answer': [v],
+                'weight': 1,
+            } for v in record.values]
+
+            # Build our list of answers
+            answers = []
+            for pool_name in sorted(pools.keys()):
+                priority = 1
+
+                # Dynamic/health checked
+                current_pool_name = pool_name
+                while current_pool_name:
+                    pool = pools[current_pool_name]
+                    for answer in pool_answers[current_pool_name]:
+                        answer = {
+                            'answer': answer['answer'],
+                            'meta': {
+                                'priority': priority,
+                                'note': self._encode_notes({
+                                    'from': current_pool_name,
+                                }),
+                                'weight': answer['weight'],
+                            },
+                            'region': pool_name,  # the one we're answering
+                        }
+                        answers.append(answer)
+
+                    current_pool_name = pool.data.get('fallback', None)
+                    priority += 1
+
+                # Static/default
+                for answer in default_answers:
+                    answer = {
+                        'answer': answer['answer'],
+                        'meta': {
+                            'priority': priority,
+                            'note': self._encode_notes({
+                                'from': '--default--',
+                            }),
+                            'weight': 1,
+                        },
+                        'region': pool_name,  # the one we're answering
+                    }
+                    answers.append(answer)
+
+            params.update({
+                'answers': answers,
+                'filters': self._DYNAMIC_FILTERS,
+                'regions': regions,
+            })
+
+            return params
+
+        elif hasattr(record, 'geo'):
             # purposefully set non-geo answers to have an empty meta,
             # so that we know we did this on purpose if/when troubleshooting
             params['answers'] = [{"answer": [x], "meta": {}}
@@ -315,6 +471,9 @@ class Ns1Provider(BaseProvider):
                     {"filter": "select_first_n",
                      "config": {"N": 1}}
                 )
+        else:
+            params['answers'] = record.values
+
         self.log.debug("params for A: %s", params)
         return params
 

From ea2a52c307d5e03d7b1a7550273c1d3e607a2026 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Tue, 10 Dec 2019 12:20:25 -0800
Subject: [PATCH 02/21] Python 3 friendly way to re-raise when tries expire

---
 octodns/provider/ns1.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 445e612..7244c3a 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -31,17 +31,18 @@ class Ns1Client(object):
 
     def _try(self, method, *args, **kwargs):
         tries = self.retry_count
-        while tries:
+        while True:  # We'll raise to break after our tries expire
             try:
                 return method(*args, **kwargs)
             except RateLimitException as e:
+                if tries <= 1:
+                    raise
                 period = float(e.period)
                 self.log.warn('rate limit encountered, pausing '
                               'for %ds and trying again, %d remaining',
                               period, tries)
                 sleep(period)
                 tries -= 1
-        raise
 
     def zones_retrieve(self, name):
         return self._try(self._zones.retrieve, name)

From 7a472506ccdadef44bc164f17829b32c2961c55e Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Tue, 10 Dec 2019 13:50:11 -0800
Subject: [PATCH 03/21] Implement _data_for_dynamic_A w/some related
 refactoring

---
 octodns/provider/ns1.py            | 138 +++++++++++++++++++++++++++--
 tests/test_octodns_provider_ns1.py |  19 +++-
 2 files changed, 147 insertions(+), 10 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 7244c3a..0b6e16a 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -19,6 +19,10 @@ from ..record import Record
 from .base import BaseProvider
 
 
+class Ns1Exception(Exception):
+    pass
+
+
 class Ns1Client(object):
     log = getLogger('NS1Client')
 
@@ -126,7 +130,18 @@ class Ns1Provider(BaseProvider):
         super(Ns1Provider, self).__init__(id, *args, **kwargs)
         self._client = Ns1Client(api_key, retry_count)
 
-    def _data_for_A(self, _type, record):
+    def _encode_notes(self, data):
+        return ' '.join(['{}:{}'.format(k, v)
+                         for k, v in sorted(data.items())])
+
+    def _parse_notes(self, note):
+        data = {}
+        for piece in note.split(' '):
+            k, v = piece.split(':', 1)
+            data[k] = v
+        return data
+
+    def _data_for_geo_A(self, _type, record):
         # record meta (which would include geo information is only
         # returned when getting a record's detail, not from zone detail
         geo = defaultdict(list)
@@ -171,6 +186,116 @@ class Ns1Provider(BaseProvider):
         data['geo'] = geo
         return data
 
+    def _data_for_dynamic_A(self, _type, record):
+        # First make sure we have the expected filters config
+        if self._DYNAMIC_FILTERS != record['filters']:
+            self.log.error('_data_for_dynamic_A: %s %s has unsupported '
+                           'filters', record['domain'], _type)
+            raise Ns1Exception('Unrecognized advanced record')
+
+        # All regions (pools) will include the list of default values
+        # (eventually) at higher priorities, we'll just add them to this set to
+        # we'll have the complete collection.
+        default = set()
+        # Fill out the pools by walking the answers and looking at their
+        # region.
+        pools = defaultdict(lambda: {'fallback': None, 'values': []})
+        for answer in record['answers']:
+            # region (group name in the UI) is the pool name
+            pool_name = answer['region']
+            pool = pools[answer['region']]
+
+            meta = answer['meta']
+            value = text_type(answer['answer'][0])
+            if meta['priority'] == 1:
+                # priority 1 means this answer is part of the pools own values
+                pool['values'].append({
+                    'value': value,
+                    'weight': int(meta.get('weight', 1)),
+                })
+            else:
+                # It's a fallback, we only care about it if it's a
+                # final/default
+                notes = self._parse_notes(meta.get('note', ''))
+                if notes.get('from', False) == '--default--':
+                    default.add(value)
+
+        # The regions objects map to rules, but it's a bit fuzzy since they're
+        # 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 = []
+        for pool_name, region in sorted(record['regions'].items()):
+            meta = region['meta']
+            notes = self._parse_notes(meta.get('note', ''))
+
+            # 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:
+                # set the fallback pool name
+                pools[pool_name]['fallback'] = notes['fallback']
+
+            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 ot find their
+            # continent
+            for country in meta.get('country', []):
+                con = country_alpha2_to_continent_code(country)
+                geos.add('{}-{}'.format(con, country))
+
+            # States are easy too, just assume NA-US (CA providences aren't
+            # supported by octoDNS currently)
+            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'] = geos
+            rules.append(rule)
+
+        # Order and convert to a list
+        default = sorted(default)
+        # Order
+        rules.sort(key=lambda r: (r['_order'], r['pool']))
+
+        return {
+            'dynamic': {
+                'pools': pools,
+                'rules': rules,
+            },
+            'ttl': record['ttl'],
+            'type': _type,
+            'values': sorted(default),
+        }
+
+    def _data_for_A(self, _type, record):
+        if record.get('tier', 1) > 1:
+            # Advanced record, see if it's first answer has a note
+            try:
+                first_answer_note = record['answers'][0]['meta']['note']
+            except (IndexError, KeyError):
+                first_answer_note = ''
+            # If that note includes a `from` (pool name) it's a dynamic record
+            if 'from:' in first_answer_note:
+                return self._data_for_dynamic_A(_type, record)
+            # If not it's an old geo record
+            return self._data_for_geo_A(_type, record)
+
+        # This is a basic record, just convert it
+        return {
+            'ttl': record['ttl'],
+            'type': _type,
+            'values': [text_type(x) for x in record['short_answers']]
+        }
+
     _data_for_AAAA = _data_for_A
 
     def _data_for_SPF(self, _type, record):
@@ -316,23 +441,18 @@ class Ns1Provider(BaseProvider):
                 continue
             data_for = getattr(self, '_data_for_{}'.format(_type))
             name = zone.hostname_from_fqdn(record['domain'])
-            record = Record.new(zone, name, data_for(_type, record),
-                                source=self, lenient=lenient)
+            data = data_for(_type, record)
+            record = Record.new(zone, name, data, source=self, lenient=lenient)
             zone_hash[(_type, name)] = record
         [zone.add_record(r, lenient=lenient) for r in zone_hash.values()]
         self.log.info('populate:   found %s records, exists=%s',
                       len(zone.records) - before, exists)
         return exists
 
-    def _encode_notes(self, data):
-        return ' '.join(['{}:{}'.format(k, v)
-                         for k, v in sorted(data.items())])
-
     def _params_for_A(self, record):
         params = {'ttl': record.ttl}
 
-        if hasattr(record, 'dynamic'):
-
+        if hasattr(record, 'dynamic') and record.dynamic:
             pools = record.dynamic.pools
 
             # Convert rules to regions
diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index 0743943..fedcc2e 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -165,8 +165,9 @@ class TestNs1Provider(TestCase):
         'domain': 'unit.tests.',
     }]
 
+    @patch('ns1.rest.records.Records.retrieve')
     @patch('ns1.rest.zones.Zones.retrieve')
-    def test_populate(self, zone_retrieve_mock):
+    def test_populate(self, zone_retrieve_mock, record_retrieve_mock):
         provider = Ns1Provider('test', 'api-key')
 
         # Bad auth
@@ -197,6 +198,7 @@ class TestNs1Provider(TestCase):
 
         # Existing zone w/o records
         zone_retrieve_mock.reset_mock()
+        record_retrieve_mock.reset_mock()
         ns1_zone = {
             'records': [{
                 "domain": "geo.unit.tests",
@@ -211,17 +213,23 @@ class TestNs1Provider(TestCase):
                     {'answer': ['4.5.6.7'],
                      'meta': {'iso_region_code': ['NA-US-WA']}},
                 ],
+                'tier': 3,
                 'ttl': 34,
             }],
         }
         zone_retrieve_mock.side_effect = [ns1_zone]
+        # Its tier 3 so we'll do a full lookup
+        record_retrieve_mock.side_effect = ns1_zone['records']
         zone = Zone('unit.tests.', [])
         provider.populate(zone)
         self.assertEquals(1, len(zone.records))
         self.assertEquals(('unit.tests',), zone_retrieve_mock.call_args[0])
+        record_retrieve_mock.assert_has_calls([call('unit.tests',
+                                                    'geo.unit.tests', 'A')])
 
         # Existing zone w/records
         zone_retrieve_mock.reset_mock()
+        record_retrieve_mock.reset_mock()
         ns1_zone = {
             'records': self.ns1_records + [{
                 "domain": "geo.unit.tests",
@@ -236,17 +244,23 @@ class TestNs1Provider(TestCase):
                     {'answer': ['4.5.6.7'],
                      'meta': {'iso_region_code': ['NA-US-WA']}},
                 ],
+                'tier': 3,
                 'ttl': 34,
             }],
         }
         zone_retrieve_mock.side_effect = [ns1_zone]
+        # Its tier 3 so we'll do a full lookup
+        record_retrieve_mock.side_effect = ns1_zone['records']
         zone = Zone('unit.tests.', [])
         provider.populate(zone)
         self.assertEquals(self.expected, zone.records)
         self.assertEquals(('unit.tests',), zone_retrieve_mock.call_args[0])
+        record_retrieve_mock.assert_has_calls([call('unit.tests',
+                                                    'geo.unit.tests', 'A')])
 
         # Test skipping unsupported record type
         zone_retrieve_mock.reset_mock()
+        record_retrieve_mock.reset_mock()
         ns1_zone = {
             'records': self.ns1_records + [{
                 'type': 'UNSUPPORTED',
@@ -266,6 +280,7 @@ class TestNs1Provider(TestCase):
                     {'answer': ['4.5.6.7'],
                      'meta': {'iso_region_code': ['NA-US-WA']}},
                 ],
+                'tier': 3,
                 'ttl': 34,
             }],
         }
@@ -274,6 +289,8 @@ class TestNs1Provider(TestCase):
         provider.populate(zone)
         self.assertEquals(self.expected, zone.records)
         self.assertEquals(('unit.tests',), zone_retrieve_mock.call_args[0])
+        record_retrieve_mock.assert_has_calls([call('unit.tests',
+                                                    'geo.unit.tests', 'A')])
 
     @patch('ns1.rest.records.Records.delete')
     @patch('ns1.rest.records.Records.update')

From f6c60b69b72e9538d164ccfa41ceefe82bebc011 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Wed, 11 Dec 2019 15:05:52 -0800
Subject: [PATCH 04/21] WIP monitors management

---
 octodns/provider/ns1.py | 110 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 103 insertions(+), 7 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 0b6e16a..8d230cf 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -32,6 +32,7 @@ class Ns1Client(object):
         client = NS1(apiKey=api_key)
         self._records = client.records()
         self._zones = client.zones()
+        self._monitors = client.monitors()
 
     def _try(self, method, *args, **kwargs):
         tries = self.retry_count
@@ -66,6 +67,17 @@ class Ns1Client(object):
     def records_delete(self, zone, domain, _type):
         return self._try(self._records.delete, zone, domain, _type)
 
+    def monitors_list(self):
+        return self._try(self._monitors.list)
+
+    def monitors_create(self, **params):
+        body = {}  # TODO: not clear what this is supposed to be
+        return self._try(self._monitors.create, body, **params)
+
+    def monitors_update(self, job_id, **params):
+        body = {}  # TODO: not clear what this is supposed to be
+        return self._try(self._monitors.update, job_id, body, **params)
+
 
 class Ns1Provider(BaseProvider):
     '''
@@ -136,9 +148,10 @@ class Ns1Provider(BaseProvider):
 
     def _parse_notes(self, note):
         data = {}
-        for piece in note.split(' '):
-            k, v = piece.split(':', 1)
-            data[k] = v
+        if note:
+            for piece in note.split(' '):
+                k, v = piece.split(':', 1)
+                data[k] = v
         return data
 
     def _data_for_geo_A(self, _type, record):
@@ -258,7 +271,7 @@ class Ns1Provider(BaseProvider):
                 '_order': notes['rule-order'],
             }
             if geos:
-                rule['geos'] = geos
+                rule['geos'] = sorted(geos)
             rules.append(rule)
 
         # Order and convert to a list
@@ -449,6 +462,79 @@ class Ns1Provider(BaseProvider):
                       len(zone.records) - before, exists)
         return exists
 
+    def _extra_changes(self, desired, changes, **kwargs):
+        # TODO: check monitors to see if they need updated
+        return []
+
+    def _monitors_for(self, record):
+        # TODO: should this just be a global cache by fqdn, type, and value?
+        expected_host = record.fqdn[:-1]
+        expected_type = record._type
+
+        monitors = {}
+
+        # TODO: cache here or in Ns1Client
+        for monitor in self._client.monitors_list():
+            data = self._parse_notes(monitor['notes'])
+            if expected_host == data['host'] or \
+               expected_type == data['type']:
+                # This monitor does not belong to this record
+                config = monitor['config']
+                value = config['host']
+                monitors[value] = monitor
+
+        return monitors
+
+    def _sync_monitor(self, record, value, existing):
+        host = record.fqdn[:-1]
+        _type = record._type
+
+        request = 'GET {path} HTTP/1.0\\r\\nHost: {host}\\r\\n' \
+            'User-agent: NS1\\r\\n\\r\\n'.format(path=record.healthcheck_path,
+                                                 host=host)
+
+        expected = {
+            'active': True,
+            'config': {
+                'connect_timeout': 2000,
+                'host': value,
+                'port': record.healthcheck_port,
+                'response_timeout': 10000,
+                'send': request,
+                'ssl': record.healthcheck_protocol == 'HTTPS',
+            },
+            'frequency': 60,
+            'job_type': 'tcp',
+            'name': '{} - {} - {}'.format(host, _type, value),
+            'notes': self._encode_notes({
+                'host': host,
+                'type': _type,
+            }),
+            'policy': 'quorum',
+            'rapid_recheck': False,
+            'region_scope': 'fixed',
+            # TODO: what should we do here dal, sjc, lga, sin, ams
+            'regions': ['lga'],
+            'rules': [{
+                'comparison': 'contains',
+                'key': 'output',
+                'value': '200 OK',
+            }],
+        }
+
+        if existing:
+            monitor_id = existing['id']
+            # See if the monitor needs updating
+            for k, v in expected.items():
+                if existing.get(k, '--missing--') != v:
+                    self._client.monitors_update(monitor_id, **expected)
+                    break
+        else:
+            return self._client.monitors_create(**expected)['id']
+
+        # TODO: this needs to return the feed
+        return None
+
     def _params_for_A(self, record):
         params = {'ttl': record.ttl}
 
@@ -498,13 +584,21 @@ class Ns1Provider(BaseProvider):
                     'meta': meta,
                 }
 
-            # Build a list of primary values for each pool
+            existing_monitors = self._monitors_for(record)
+
+            # Build a list of primary values for each pool, including their
+            # monitor
             pool_answers = defaultdict(list)
             for pool_name, pool in sorted(pools.items()):
                 for value in pool.data['values']:
+                    weight = value['weight']
+                    value = value['value']
+                    existing = existing_monitors.get(value)
+                    monitor_id = self._sync_monitor(record, value, existing)
                     pool_answers[pool_name].append({
-                        'answer': [value['value']],
-                        'weight': value['weight'],
+                        'answer': [value],
+                        'weight': weight,
+                        'monitor_id': monitor_id,
                     })
 
             default_answers = [{
@@ -531,6 +625,7 @@ class Ns1Provider(BaseProvider):
                                 }),
                                 'weight': answer['weight'],
                             },
+                            'up': True,  # TODO: this should be a monitor/feed
                             'region': pool_name,  # the one we're answering
                         }
                         answers.append(answer)
@@ -547,6 +642,7 @@ class Ns1Provider(BaseProvider):
                             'note': self._encode_notes({
                                 'from': '--default--',
                             }),
+                            'up': True,
                             'weight': 1,
                         },
                         'region': pool_name,  # the one we're answering

From 55f4194daf8ece7fb63c3b9a6bf2d2fdf9a8d38a Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Thu, 12 Dec 2019 13:23:35 -0800
Subject: [PATCH 05/21] Functionally complement and untested ns1 dynamic
 support

---
 octodns/provider/ns1.py            | 543 +++++++++++++++++++----------
 tests/test_octodns_provider_ns1.py |   8 +-
 2 files changed, 366 insertions(+), 185 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 8d230cf..e5cb1ed 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -12,10 +12,11 @@ from ns1 import NS1
 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 six import text_type
 
-from ..record import Record
+from ..record import Record, Update
 from .base import BaseProvider
 
 
@@ -33,6 +34,9 @@ class Ns1Client(object):
         self._records = client.records()
         self._zones = client.zones()
         self._monitors = client.monitors()
+        self._notifylists = client.notifylists()
+        self._datasource = client.datasource()
+        self._datafeed = client.datafeed()
 
     def _try(self, method, *args, **kwargs):
         tries = self.retry_count
@@ -49,35 +53,62 @@ class Ns1Client(object):
                 sleep(period)
                 tries -= 1
 
-    def zones_retrieve(self, name):
-        return self._try(self._zones.retrieve, name)
+    def datafeed_create(self, sourceid, name, config):
+        return self._try(self._datafeed.create, sourceid, name, config)
 
-    def zones_create(self, name):
-        return self._try(self._zones.create, name)
+    def datafeed_delete(self, sourceid, feedid):
+        return self._try(self._datafeed.delete, sourceid, feedid)
 
-    def records_retrieve(self, zone, domain, _type):
-        return self._try(self._records.retrieve, zone, domain, _type)
+    def datafeed_list(self, sourceid):
+        return self._try(self._datafeed.list, sourceid)
 
-    def records_create(self, zone, domain, _type, **params):
-        return self._try(self._records.create, zone, domain, _type, **params)
+    def datasource_create(self, **body):
+        return self._try(self._datasource.create, **body)
 
-    def records_update(self, zone, domain, _type, **params):
-        return self._try(self._records.update, zone, domain, _type, **params)
+    def datasource_list(self):
+        return self._try(self._datasource.list)
 
-    def records_delete(self, zone, domain, _type):
-        return self._try(self._records.delete, zone, domain, _type)
+    def monitors_create(self, **params):
+        body = {}
+        return self._try(self._monitors.create, body, **params)
+
+    def monitors_delete(self, jobid):
+        return self._try(self._monitors.delete, jobid)
 
     def monitors_list(self):
         return self._try(self._monitors.list)
 
-    def monitors_create(self, **params):
-        body = {}  # TODO: not clear what this is supposed to be
-        return self._try(self._monitors.create, body, **params)
-
     def monitors_update(self, job_id, **params):
-        body = {}  # TODO: not clear what this is supposed to be
+        body = {}
         return self._try(self._monitors.update, job_id, body, **params)
 
+    def notifylists_delete(self, nlid):
+        return self._try(self._notifylists.delete, nlid)
+
+    def notifylists_create(self, **body):
+        return self._try(self._notifylists.create, body)
+
+    def notifylists_list(self):
+        return self._try(self._notifylists.list)
+
+    def records_create(self, zone, domain, _type, **params):
+        return self._try(self._records.create, zone, domain, _type, **params)
+
+    def records_delete(self, zone, domain, _type):
+        return self._try(self._records.delete, zone, domain, _type)
+
+    def records_retrieve(self, zone, domain, _type):
+        return self._try(self._records.retrieve, zone, domain, _type)
+
+    def records_update(self, zone, domain, _type, **params):
+        return self._try(self._records.update, zone, domain, _type, **params)
+
+    def zones_create(self, name):
+        return self._try(self._zones.create, name)
+
+    def zones_retrieve(self, name):
+        return self._try(self._zones.retrieve, name)
+
 
 class Ns1Provider(BaseProvider):
     '''
@@ -140,7 +171,11 @@ class Ns1Provider(BaseProvider):
         self.log.debug('__init__: id=%s, api_key=***, retry_count=%d', id,
                        retry_count)
         super(Ns1Provider, self).__init__(id, *args, **kwargs)
+
         self._client = Ns1Client(api_key, retry_count)
+        self.__monitors = None
+        self.__datasource_id = None
+        self.__feeds_for_monitors = None
 
     def _encode_notes(self, data):
         return ' '.join(['{}:{}'.format(k, v)
@@ -462,38 +497,139 @@ class Ns1Provider(BaseProvider):
                       len(zone.records) - before, exists)
         return exists
 
-    def _extra_changes(self, desired, changes, **kwargs):
-        # TODO: check monitors to see if they need updated
-        return []
+    def _params_for_geo_A(self, record):
+        # purposefully set non-geo answers to have an empty meta,
+        # so that we know we did this on purpose if/when troubleshooting
+        params = {
+            'answers': [{"answer": [x], "meta": {}} for x in record.values],
+            'ttl': record.ttl,
+        }
+
+        has_country = False
+        for iso_region, target in record.geo.items():
+            key = 'iso_region_code'
+            value = iso_region
+            if not has_country and \
+               len(value.split('-')) > 1:  # pragma: nocover
+                has_country = True
+            for answer in target.values:
+                params['answers'].append(
+                    {
+                        'answer': [answer],
+                        'meta': {key: [value]},
+                    },
+                )
+
+        params['filters'] = []
+        if has_country:
+            params['filters'].append(
+                {"filter": "shuffle", "config": {}}
+            )
+            params['filters'].append(
+                {"filter": "geotarget_country", "config": {}}
+            )
+            params['filters'].append(
+                {"filter": "select_first_n",
+                 "config": {"N": 1}}
+            )
+
+        return params, None
+
+    @property
+    def _monitors(self):
+        # TODO: cache in sync, here and for others
+        if self.__monitors is None:
+            self.__monitors = {m['id']: m
+                               for m in self._client.monitors_list()}
+        return self.__monitors
 
     def _monitors_for(self, record):
-        # TODO: should this just be a global cache by fqdn, type, and value?
-        expected_host = record.fqdn[:-1]
-        expected_type = record._type
-
         monitors = {}
 
-        # TODO: cache here or in Ns1Client
-        for monitor in self._client.monitors_list():
-            data = self._parse_notes(monitor['notes'])
-            if expected_host == data['host'] or \
-               expected_type == data['type']:
-                # This monitor does not belong to this record
-                config = monitor['config']
-                value = config['host']
-                monitors[value] = monitor
+        if getattr(record, 'dynamic', False):
+            # TODO: should this just be a global cache by fqdn, type, and
+            # value?
+            expected_host = record.fqdn[:-1]
+            expected_type = record._type
+
+            # TODO: cache here or in Ns1Client
+            for monitor in self._monitors.values():
+                data = self._parse_notes(monitor['notes'])
+                if expected_host == data['host'] or \
+                   expected_type == data['type']:
+                    # This monitor does not belong to this record
+                    config = monitor['config']
+                    value = config['host']
+                    monitors[value] = monitor
 
         return monitors
 
-    def _sync_monitor(self, record, value, existing):
+    @property
+    def _datasource_id(self):
+        if self.__datasource_id is None:
+            name = 'octoDNS NS1 Data Source'
+            source = None
+            for candidate in self._client.datasource_list():
+                if candidate['name'] == name:
+                    # Found it
+                    source = candidate
+                    break
+
+            if source is None:
+                # We need to create it
+                source = self._client \
+                    .datasource_create(name=name,
+                                       sourcetype='nsone_monitoring')
+
+            self.__datasource_id = source['id']
+
+        return self.__datasource_id
+
+    def _feed_for_monitor(self, monitor):
+        if self.__feeds_for_monitors is None:
+            self.__feeds_for_monitors = {
+                f['config']['jobid']: f['id']
+                for f in self._client.datafeed_list(self._datasource_id)
+            }
+
+        return self.__feeds_for_monitors.get(monitor['id'])
+
+    def _create_monitor(self, monitor):
+        # TODO: looks like length limit is 64 char
+        name = '{} - {}'.format(monitor['name'], uuid4().hex[:6])
+
+        # Create the notify list
+        notify_list = [{
+            'config': {
+                'sourceid': self._datasource_id,
+            },
+            'type': 'datafeed',
+        }]
+        nl = self._client.notifylists_create(name=name,
+                                             notify_list=notify_list)
+
+        # Create the monitor
+        monitor['notify_list'] = nl['id']
+        monitor = self._client.monitors_create(**monitor)
+
+        # Create the data feed
+        config = {
+            'jobid': monitor['id'],
+        }
+        feed = self._client.datafeed_create(self._datasource_id, name,
+                                            config)
+
+        return monitor['id'], feed['id']
+
+    def _monitor_gen(self, record, value):
         host = record.fqdn[:-1]
         _type = record._type
 
         request = 'GET {path} HTTP/1.0\\r\\nHost: {host}\\r\\n' \
             'User-agent: NS1\\r\\n\\r\\n'.format(path=record.healthcheck_path,
-                                                 host=host)
+                                                 host=record.healthcheck_host)
 
-        expected = {
+        return {
             'active': True,
             'config': {
                 'connect_timeout': 2000,
@@ -522,177 +658,189 @@ class Ns1Provider(BaseProvider):
             }],
         }
 
+    def _monitor_is_match(self, expected, have):
+        # Make sure what we have matches what's in expected exactly. Anything
+        # else in have will be ignored.
+        for k, v in expected.items():
+            if have.get(k, '--missing--') != v:
+                return False
+
+        return True
+
+    def _monitor_sync(self, record, value, existing):
+        expected = self._monitor_gen(record, value)
+
         if existing:
             monitor_id = existing['id']
-            # See if the monitor needs updating
-            for k, v in expected.items():
-                if existing.get(k, '--missing--') != v:
-                    self._client.monitors_update(monitor_id, **expected)
-                    break
+
+            if not self._monitor_is_match(expected, existing):
+                # Update the monitor to match expected, everything else will be
+                # left alone and assumed correct
+                self._client.monitors_update(monitor_id, **expected)
+
+            try:
+                feed_id = self._feed_for_monitor(existing)
+            except KeyError:
+                raise Ns1Exception('Failed to find the feed for {} ({})'
+                                   .format(existing['name'], existing['id']))
         else:
-            return self._client.monitors_create(**expected)['id']
+            # We don't have an existing monitor create it (and related bits)
+            monitor_id, feed_id = self._create_monitor(expected)
 
-        # TODO: this needs to return the feed
-        return None
+        return monitor_id, feed_id
 
-    def _params_for_A(self, record):
-        params = {'ttl': record.ttl}
+    def _gc_monitors(self, record, active_monitor_ids=None):
 
-        if hasattr(record, 'dynamic') and record.dynamic:
-            pools = record.dynamic.pools
+        if active_monitor_ids is None:
+            active_monitor_ids = set()
 
-            # Convert rules to regions
-            regions = {}
-            for i, rule in enumerate(record.dynamic.rules):
-                pool_name = rule.data['pool']
+        for monitor in self._monitors_for(record).values():
+            monitor_id = monitor['id']
+            if monitor_id in active_monitor_ids:
+                continue
 
-                notes = {
-                    'rule-order': i,
-                }
+            feed_id = self._feed_for_monitor(monitor)
+            if feed_id:
+                self._client.datafeed_delete(self._datasource_id, feed_id)
 
-                fallback = pools[pool_name].data.get('fallback', None)
-                if fallback:
-                    notes['fallback'] = fallback
+            self._client.monitors_delete(monitor_id)
 
-                country = set()
-                georegion = set()
-                us_state = set()
+            notify_list_id = monitor['notify_list']
+            self._client.notifylists_delete(notify_list_id)
 
-                for geo in rule.data.get('geos', []):
-                    n = len(geo)
-                    if n == 8:
-                        # US state
-                        us_state.add(geo[-2:])
-                    elif n == 5:
-                        # Country
-                        country.add(geo[-2:])
-                    else:
-                        # Continent
-                        georegion.update(self._CONTINENT_TO_REGIONS[geo])
+    def _params_for_dynamic_A(self, record):
+        pools = record.dynamic.pools
 
-                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)
+        # Convert rules to regions
+        regions = {}
+        for i, rule in enumerate(record.dynamic.rules):
+            pool_name = rule.data['pool']
 
-                regions[pool_name] = {
-                    'meta': meta,
-                }
+            notes = {
+                'rule-order': i,
+            }
 
-            existing_monitors = self._monitors_for(record)
+            fallback = pools[pool_name].data.get('fallback', None)
+            if fallback:
+                notes['fallback'] = fallback
 
-            # Build a list of primary values for each pool, including their
-            # monitor
-            pool_answers = defaultdict(list)
-            for pool_name, pool in sorted(pools.items()):
-                for value in pool.data['values']:
-                    weight = value['weight']
-                    value = value['value']
-                    existing = existing_monitors.get(value)
-                    monitor_id = self._sync_monitor(record, value, existing)
-                    pool_answers[pool_name].append({
-                        'answer': [value],
-                        'weight': weight,
-                        'monitor_id': monitor_id,
-                    })
+            country = set()
+            georegion = set()
+            us_state = set()
 
-            default_answers = [{
-                'answer': [v],
-                'weight': 1,
-            } for v in record.values]
+            for geo in rule.data.get('geos', []):
+                n = len(geo)
+                if n == 8:
+                    # US state
+                    us_state.add(geo[-2:])
+                elif n == 5:
+                    # Country
+                    country.add(geo[-2:])
+                else:
+                    # Continent
+                    georegion.update(self._CONTINENT_TO_REGIONS[geo])
 
-            # Build our list of answers
-            answers = []
-            for pool_name in sorted(pools.keys()):
-                priority = 1
+            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)
 
-                # Dynamic/health checked
-                current_pool_name = pool_name
-                while current_pool_name:
-                    pool = pools[current_pool_name]
-                    for answer in pool_answers[current_pool_name]:
-                        answer = {
-                            'answer': answer['answer'],
-                            'meta': {
-                                'priority': priority,
-                                'note': self._encode_notes({
-                                    'from': current_pool_name,
-                                }),
-                                'weight': answer['weight'],
-                            },
-                            'up': True,  # TODO: this should be a monitor/feed
-                            'region': pool_name,  # the one we're answering
-                        }
-                        answers.append(answer)
+            regions[pool_name] = {
+                'meta': meta,
+            }
 
-                    current_pool_name = pool.data.get('fallback', None)
-                    priority += 1
+        existing_monitors = self._monitors_for(record)
+        active_monitors = set()
 
-                # Static/default
-                for answer in default_answers:
+        # Build a list of primary values for each pool, including their
+        # feed_id (monitor)
+        pool_answers = defaultdict(list)
+        for pool_name, pool in sorted(pools.items()):
+            for value in pool.data['values']:
+                weight = value['weight']
+                value = value['value']
+                existing = existing_monitors.get(value)
+                monitor_id, feed_id = self._monitor_sync(record, value,
+                                                         existing)
+                active_monitors.add(monitor_id)
+                pool_answers[pool_name].append({
+                    'answer': [value],
+                    'weight': weight,
+                    'feed_id': feed_id,
+                })
+
+        default_answers = [{
+            'answer': [v],
+            'weight': 1,
+        } for v in record.values]
+
+        # Build our list of answers
+        answers = []
+        for pool_name in sorted(pools.keys()):
+            priority = 1
+
+            # Dynamic/health checked
+            current_pool_name = pool_name
+            while current_pool_name:
+                pool = pools[current_pool_name]
+                for answer in pool_answers[current_pool_name]:
                     answer = {
                         'answer': answer['answer'],
                         'meta': {
                             'priority': priority,
                             'note': self._encode_notes({
-                                'from': '--default--',
+                                'from': current_pool_name,
                             }),
-                            'up': True,
-                            'weight': 1,
+                            'up': {
+                                'feed': answer['feed_id'],
+                            },
+                            'weight': answer['weight'],
                         },
                         'region': pool_name,  # the one we're answering
                     }
                     answers.append(answer)
 
-            params.update({
-                'answers': answers,
-                'filters': self._DYNAMIC_FILTERS,
-                'regions': regions,
-            })
+                current_pool_name = pool.data.get('fallback', None)
+                priority += 1
 
-            return params
+            # Static/default
+            for answer in default_answers:
+                answer = {
+                    'answer': answer['answer'],
+                    'meta': {
+                        'priority': priority,
+                        'note': self._encode_notes({
+                            'from': '--default--',
+                        }),
+                        'up': True,
+                        'weight': 1,
+                    },
+                    'region': pool_name,  # the one we're answering
+                }
+                answers.append(answer)
 
+        return {
+            'answers': answers,
+            'filters': self._DYNAMIC_FILTERS,
+            'regions': regions,
+            'ttl': record.ttl,
+        }, active_monitors
+
+    def _params_for_A(self, record):
+        if getattr(record, 'dynamic', False):
+            return self._params_for_dynamic_A(record)
         elif hasattr(record, 'geo'):
-            # purposefully set non-geo answers to have an empty meta,
-            # so that we know we did this on purpose if/when troubleshooting
-            params['answers'] = [{"answer": [x], "meta": {}}
-                                 for x in record.values]
-            has_country = False
-            for iso_region, target in record.geo.items():
-                key = 'iso_region_code'
-                value = iso_region
-                if not has_country and \
-                   len(value.split('-')) > 1:  # pragma: nocover
-                    has_country = True
-                for answer in target.values:
-                    params['answers'].append(
-                        {
-                            'answer': [answer],
-                            'meta': {key: [value]},
-                        },
-                    )
-            params['filters'] = []
-            if has_country:
-                params['filters'].append(
-                    {"filter": "shuffle", "config": {}}
-                )
-                params['filters'].append(
-                    {"filter": "geotarget_country", "config": {}}
-                )
-                params['filters'].append(
-                    {"filter": "select_first_n",
-                     "config": {"N": 1}}
-                )
-        else:
-            params['answers'] = record.values
+            return self._params_for_geo_A(record)
 
-        self.log.debug("params for A: %s", params)
-        return params
+        return {
+            'answers': record.values,
+            'ttl': record.ttl,
+        }, None
 
     _params_for_AAAA = _params_for_A
     _params_for_NS = _params_for_A
@@ -702,49 +850,81 @@ class Ns1Provider(BaseProvider):
         # escaped in values so we have to strip them here and add
         # them when going the other way
         values = [v.replace('\\;', ';') for v in record.values]
-        return {'answers': values, 'ttl': record.ttl}
+        return {'answers': values, 'ttl': record.ttl}, None
 
     _params_for_TXT = _params_for_SPF
 
     def _params_for_CAA(self, record):
         values = [(v.flags, v.tag, v.value) for v in record.values]
-        return {'answers': values, 'ttl': record.ttl}
+        return {'answers': values, 'ttl': record.ttl}, None
 
+    # TODO: dynamic CNAME support
     def _params_for_CNAME(self, record):
-        return {'answers': [record.value], 'ttl': record.ttl}
+        return {'answers': [record.value], 'ttl': record.ttl}, None
 
     _params_for_ALIAS = _params_for_CNAME
     _params_for_PTR = _params_for_CNAME
 
     def _params_for_MX(self, record):
         values = [(v.preference, v.exchange) for v in record.values]
-        return {'answers': values, 'ttl': record.ttl}
+        return {'answers': values, 'ttl': record.ttl}, None
 
     def _params_for_NAPTR(self, record):
         values = [(v.order, v.preference, v.flags, v.service, v.regexp,
                    v.replacement) for v in record.values]
-        return {'answers': values, 'ttl': record.ttl}
+        return {'answers': values, 'ttl': record.ttl}, None
 
     def _params_for_SRV(self, record):
         values = [(v.priority, v.weight, v.port, v.target)
                   for v in record.values]
-        return {'answers': values, 'ttl': record.ttl}
+        return {'answers': values, 'ttl': record.ttl}, None
+
+    def _extra_changes(self, desired, changes, **kwargs):
+        self.log.debug('_extra_changes: desired=%s', desired.name)
+
+        changed = set([c.record for c in changes])
+
+        extra = []
+        for record in desired.records:
+            if record in changed or not getattr(record, 'dynamic', False):
+                # Already changed, or no dynamic , no need to check it
+                continue
+
+            for have in self._monitors_for(record).values():
+                value = have['config']['host']
+                expected = self._monitor_gen(record, value)
+                if not expected:
+                    self.log.info('_extra_changes: monitor missing for %s',
+                                  expected['name'])
+                    extra.append(Update(record, record))
+                    break
+                if not self._monitor_is_match(expected, have):
+                    self.log.info('_extra_changes: monitor mis-match for %s',
+                                  expected['name'])
+                    extra.append(Update(record, record))
+                    break
+
+        return extra
 
     def _apply_Create(self, ns1_zone, change):
         new = change.new
         zone = new.zone.name[:-1]
         domain = new.fqdn[:-1]
         _type = new._type
-        params = getattr(self, '_params_for_{}'.format(_type))(new)
+        params, active_monitor_ids = \
+            getattr(self, '_params_for_{}'.format(_type))(new)
         self._client.records_create(zone, domain, _type, **params)
+        self._gc_monitors(new, active_monitor_ids)
 
     def _apply_Update(self, ns1_zone, change):
         new = change.new
         zone = new.zone.name[:-1]
         domain = new.fqdn[:-1]
         _type = new._type
-        params = getattr(self, '_params_for_{}'.format(_type))(new)
+        params, active_monitor_ids = \
+            getattr(self, '_params_for_{}'.format(_type))(new)
         self._client.records_update(zone, domain, _type, **params)
+        self._gc_monitors(new, active_monitor_ids)
 
     def _apply_Delete(self, ns1_zone, change):
         existing = change.existing
@@ -752,6 +932,7 @@ class Ns1Provider(BaseProvider):
         domain = existing.fqdn[:-1]
         _type = existing._type
         self._client.records_delete(zone, domain, _type)
+        self._gc_monitors(existing)
 
     def _apply(self, plan):
         desired = plan.desired
diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index fedcc2e..539fcfb 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -474,16 +474,16 @@ class TestNs1Provider(TestCase):
             'type': 'SPF',
             'value': 'foo\\; bar baz\\; blip'
         })
-        self.assertEquals(['foo; bar baz; blip'],
-                          provider._params_for_SPF(record)['answers'])
+        params, _ = provider._params_for_SPF(record)
+        self.assertEquals(['foo; bar baz; blip'], params['answers'])
 
         record = Record.new(zone, 'txt', {
             'ttl': 35,
             'type': 'TXT',
             'value': 'foo\\; bar baz\\; blip'
         })
-        self.assertEquals(['foo; bar baz; blip'],
-                          provider._params_for_TXT(record)['answers'])
+        params, _ = provider._params_for_SPF(record)
+        self.assertEquals(['foo; bar baz; blip'], params['answers'])
 
     def test_data_for_CNAME(self):
         provider = Ns1Provider('test', 'api-key')

From c119f2e802a73e19c1918f1b37d2f1e44e5cac13 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Thu, 12 Dec 2019 14:03:09 -0800
Subject: [PATCH 06/21] Move ns1 caching to client where it's much
 safer/consistent

---
 octodns/provider/ns1.py | 184 +++++++++++++++++++++-------------------
 1 file changed, 98 insertions(+), 86 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index e5cb1ed..321b439 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -38,26 +38,58 @@ class Ns1Client(object):
         self._datasource = client.datasource()
         self._datafeed = client.datafeed()
 
-    def _try(self, method, *args, **kwargs):
-        tries = self.retry_count
-        while True:  # We'll raise to break after our tries expire
-            try:
-                return method(*args, **kwargs)
-            except RateLimitException as e:
-                if tries <= 1:
-                    raise
-                period = float(e.period)
-                self.log.warn('rate limit encountered, pausing '
-                              'for %ds and trying again, %d remaining',
-                              period, tries)
-                sleep(period)
-                tries -= 1
+        self._datasource_id = None
+        self._feeds_for_monitors = None
+        self._monitors_cache = None
+
+    @property
+    def datasource_id(self):
+        if self._datasource_id is None:
+            name = 'octoDNS NS1 Data Source'
+            source = None
+            for candidate in self.datasource_list():
+                if candidate['name'] == name:
+                    # Found it
+                    source = candidate
+                    break
+
+            if source is None:
+                # We need to create it
+                source = self.datasource_create(name=name,
+                                                sourcetype='nsone_monitoring')
+
+            self._datasource_id = source['id']
+
+        return self._datasource_id
+
+    @property
+    def feeds_for_monitors(self):
+        if self._feeds_for_monitors is None:
+            self._feeds_for_monitors = {
+                f['config']['jobid']: f['id']
+                for f in self.datafeed_list(self.datasource_id)
+            }
+
+        return self._feeds_for_monitors
+
+    @property
+    def monitors(self):
+        if self._monitors_cache is None:
+            self._monitors_cache = \
+                {m['id']: m for m in self.monitors_list()}
+        return self._monitors_cache
 
     def datafeed_create(self, sourceid, name, config):
-        return self._try(self._datafeed.create, sourceid, name, config)
+        ret = self._try(self._datafeed.create, sourceid, name, config)
+        self.feeds_for_monitors[config['jobid']] = ret['id']
+        return ret
 
     def datafeed_delete(self, sourceid, feedid):
-        return self._try(self._datafeed.delete, sourceid, feedid)
+        ret = self._try(self._datafeed.delete, sourceid, feedid)
+        self._feeds_for_monitors = {
+            k: v for k, v in self._feeds_for_monitors.items() if v != feedid
+        }
+        return ret
 
     def datafeed_list(self, sourceid):
         return self._try(self._datafeed.list, sourceid)
@@ -70,10 +102,14 @@ class Ns1Client(object):
 
     def monitors_create(self, **params):
         body = {}
-        return self._try(self._monitors.create, body, **params)
+        ret = self._try(self._monitors.create, body, **params)
+        self.monitors[ret['id']] = ret
+        return ret
 
     def monitors_delete(self, jobid):
-        return self._try(self._monitors.delete, jobid)
+        ret = self._try(self._monitors.delete, jobid)
+        self.monitors.pop(jobid)
+        return ret
 
     def monitors_list(self):
         return self._try(self._monitors.list)
@@ -109,6 +145,21 @@ class Ns1Client(object):
     def zones_retrieve(self, name):
         return self._try(self._zones.retrieve, name)
 
+    def _try(self, method, *args, **kwargs):
+        tries = self.retry_count
+        while True:  # We'll raise to break after our tries expire
+            try:
+                return method(*args, **kwargs)
+            except RateLimitException as e:
+                if tries <= 1:
+                    raise
+                period = float(e.period)
+                self.log.warn('rate limit encountered, pausing '
+                              'for %ds and trying again, %d remaining',
+                              period, tries)
+                sleep(period)
+                tries -= 1
+
 
 class Ns1Provider(BaseProvider):
     '''
@@ -173,9 +224,6 @@ class Ns1Provider(BaseProvider):
         super(Ns1Provider, self).__init__(id, *args, **kwargs)
 
         self._client = Ns1Client(api_key, retry_count)
-        self.__monitors = None
-        self.__datasource_id = None
-        self.__feeds_for_monitors = None
 
     def _encode_notes(self, data):
         return ' '.join(['{}:{}'.format(k, v)
@@ -535,25 +583,14 @@ class Ns1Provider(BaseProvider):
 
         return params, None
 
-    @property
-    def _monitors(self):
-        # TODO: cache in sync, here and for others
-        if self.__monitors is None:
-            self.__monitors = {m['id']: m
-                               for m in self._client.monitors_list()}
-        return self.__monitors
-
     def _monitors_for(self, record):
         monitors = {}
 
         if getattr(record, 'dynamic', False):
-            # TODO: should this just be a global cache by fqdn, type, and
-            # value?
             expected_host = record.fqdn[:-1]
             expected_type = record._type
 
-            # TODO: cache here or in Ns1Client
-            for monitor in self._monitors.values():
+            for monitor in self._client.monitors.values():
                 data = self._parse_notes(monitor['notes'])
                 if expected_host == data['host'] or \
                    expected_type == data['type']:
@@ -564,62 +601,35 @@ class Ns1Provider(BaseProvider):
 
         return monitors
 
-    @property
-    def _datasource_id(self):
-        if self.__datasource_id is None:
-            name = 'octoDNS NS1 Data Source'
-            source = None
-            for candidate in self._client.datasource_list():
-                if candidate['name'] == name:
-                    # Found it
-                    source = candidate
-                    break
-
-            if source is None:
-                # We need to create it
-                source = self._client \
-                    .datasource_create(name=name,
-                                       sourcetype='nsone_monitoring')
-
-            self.__datasource_id = source['id']
-
-        return self.__datasource_id
-
-    def _feed_for_monitor(self, monitor):
-        if self.__feeds_for_monitors is None:
-            self.__feeds_for_monitors = {
-                f['config']['jobid']: f['id']
-                for f in self._client.datafeed_list(self._datasource_id)
-            }
-
-        return self.__feeds_for_monitors.get(monitor['id'])
-
-    def _create_monitor(self, monitor):
+    def _create_feed(self, monitor):
         # TODO: looks like length limit is 64 char
         name = '{} - {}'.format(monitor['name'], uuid4().hex[:6])
 
+        # Create the data feed
+        config = {
+            'jobid': monitor['id'],
+        }
+        feed = self._client.datafeed_create(self._client.datasource_id, name,
+                                            config)
+
+        return feed['id']
+
+    def _create_monitor(self, monitor):
         # Create the notify list
         notify_list = [{
             'config': {
-                'sourceid': self._datasource_id,
+                'sourceid': self._client.datasource_id,
             },
             'type': 'datafeed',
         }]
-        nl = self._client.notifylists_create(name=name,
+        nl = self._client.notifylists_create(name=monitor['name'],
                                              notify_list=notify_list)
 
         # Create the monitor
         monitor['notify_list'] = nl['id']
         monitor = self._client.monitors_create(**monitor)
 
-        # Create the data feed
-        config = {
-            'jobid': monitor['id'],
-        }
-        feed = self._client.datafeed_create(self._datasource_id, name,
-                                            config)
-
-        return monitor['id'], feed['id']
+        return monitor['id'], self._create_feed(monitor)
 
     def _monitor_gen(self, record, value):
         host = record.fqdn[:-1]
@@ -678,11 +688,11 @@ class Ns1Provider(BaseProvider):
                 # left alone and assumed correct
                 self._client.monitors_update(monitor_id, **expected)
 
-            try:
-                feed_id = self._feed_for_monitor(existing)
-            except KeyError:
-                raise Ns1Exception('Failed to find the feed for {} ({})'
-                                   .format(existing['name'], existing['id']))
+            feed_id = self._client.feeds_for_monitors.get(monitor_id)
+            if feed_id is None:
+                self.log.warn('_monitor_sync: %s (%s) missing feed, creating',
+                              existing['name'], monitor_id)
+                feed_id = self._create_feed(existing)
         else:
             # We don't have an existing monitor create it (and related bits)
             monitor_id, feed_id = self._create_monitor(expected)
@@ -699,9 +709,10 @@ class Ns1Provider(BaseProvider):
             if monitor_id in active_monitor_ids:
                 continue
 
-            feed_id = self._feed_for_monitor(monitor)
+            feed_id = self._client.feeds_for_monitors.get(monitor_id)
             if feed_id:
-                self._client.datafeed_delete(self._datasource_id, feed_id)
+                self._client.datafeed_delete(self._client.datasource_id,
+                                             feed_id)
 
             self._client.monitors_delete(monitor_id)
 
@@ -893,16 +904,17 @@ class Ns1Provider(BaseProvider):
             for have in self._monitors_for(record).values():
                 value = have['config']['host']
                 expected = self._monitor_gen(record, value)
-                if not expected:
-                    self.log.info('_extra_changes: monitor missing for %s',
-                                  expected['name'])
-                    extra.append(Update(record, record))
-                    break
+                # TODO: find values which have missing monitors
                 if not self._monitor_is_match(expected, have):
                     self.log.info('_extra_changes: monitor mis-match for %s',
                                   expected['name'])
                     extra.append(Update(record, record))
                     break
+                if not have.get('notify_list'):
+                    self.log.info('_extra_changes: broken monitor no notify '
+                                  'list %s (%s)', have['name'], have['id'])
+                    extra.append(Update(record, record))
+                    break
 
         return extra
 

From 674c29fb8b905b2754b8e3f93e811168a33f56a7 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Thu, 12 Dec 2019 14:17:42 -0800
Subject: [PATCH 07/21] Debug logging

---
 octodns/provider/ns1.py | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 321b439..5deb2c9 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -28,6 +28,7 @@ class Ns1Client(object):
     log = getLogger('NS1Client')
 
     def __init__(self, api_key, retry_count=4):
+        self.log.debug('__init__: retry_count=%d', retry_count)
         self.retry_count = retry_count
 
         client = NS1(apiKey=api_key)
@@ -54,9 +55,11 @@ class Ns1Client(object):
                     break
 
             if source is None:
+                self.log.info('datasource_id: creating datasource %s', name)
                 # We need to create it
                 source = self.datasource_create(name=name,
                                                 sourcetype='nsone_monitoring')
+                self.log.info('datasource_id:   id=%s', source['id'])
 
             self._datasource_id = source['id']
 
@@ -65,6 +68,7 @@ class Ns1Client(object):
     @property
     def feeds_for_monitors(self):
         if self._feeds_for_monitors is None:
+            self.log.debug('feeds_for_monitors: fetching & building')
             self._feeds_for_monitors = {
                 f['config']['jobid']: f['id']
                 for f in self.datafeed_list(self.datasource_id)
@@ -75,6 +79,7 @@ class Ns1Client(object):
     @property
     def monitors(self):
         if self._monitors_cache is None:
+            self.log.debug('monitors: fetching & building')
             self._monitors_cache = \
                 {m['id']: m for m in self.monitors_list()}
         return self._monitors_cache
@@ -602,19 +607,24 @@ class Ns1Provider(BaseProvider):
         return monitors
 
     def _create_feed(self, monitor):
+        monitor_id = monitor['id']
+        self.log.debug('_create_feed: monitor=%s', monitor_id)
         # TODO: looks like length limit is 64 char
         name = '{} - {}'.format(monitor['name'], uuid4().hex[:6])
 
         # Create the data feed
         config = {
-            'jobid': monitor['id'],
+            'jobid': monitor_id,
         }
         feed = self._client.datafeed_create(self._client.datasource_id, name,
                                             config)
+        feed_id = feed['id']
+        self.log.debug('_create_feed:   feed=%s', feed_id)
 
-        return feed['id']
+        return feed_id
 
     def _create_monitor(self, monitor):
+        self.log.debug('_create_monitor: monitor="%s"', monitor['name'])
         # Create the notify list
         notify_list = [{
             'config': {
@@ -624,12 +634,16 @@ class Ns1Provider(BaseProvider):
         }]
         nl = self._client.notifylists_create(name=monitor['name'],
                                              notify_list=notify_list)
+        nl_id = nl['id']
+        self.log.debug('_create_monitor:   notify_list=%s', nl_id)
 
         # Create the monitor
-        monitor['notify_list'] = nl['id']
+        monitor['notify_list'] = nl_id
         monitor = self._client.monitors_create(**monitor)
+        monitor_id = monitor['id']
+        self.log.debug('_create_monitor:   monitor=%s', monitor_id)
 
-        return monitor['id'], self._create_feed(monitor)
+        return monitor_id, self._create_feed(monitor)
 
     def _monitor_gen(self, record, value):
         host = record.fqdn[:-1]
@@ -678,12 +692,16 @@ class Ns1Provider(BaseProvider):
         return True
 
     def _monitor_sync(self, record, value, existing):
+        self.log.debug('_monitor_sync: record=%s, value=%s', record.fqdn,
+                       value)
         expected = self._monitor_gen(record, value)
 
         if existing:
+            self.log.debug('_monitor_sync:   existing=%s', existing['id'])
             monitor_id = existing['id']
 
             if not self._monitor_is_match(expected, existing):
+                self.log.debug('_monitor_sync:   existing needs update')
                 # Update the monitor to match expected, everything else will be
                 # left alone and assumed correct
                 self._client.monitors_update(monitor_id, **expected)
@@ -694,12 +712,15 @@ class Ns1Provider(BaseProvider):
                               existing['name'], monitor_id)
                 feed_id = self._create_feed(existing)
         else:
+            self.log.debug('_monitor_sync:   needs create')
             # We don't have an existing monitor create it (and related bits)
             monitor_id, feed_id = self._create_monitor(expected)
 
         return monitor_id, feed_id
 
     def _gc_monitors(self, record, active_monitor_ids=None):
+        self.log.debug('_gc_monitors: record=%s, active_monitor_ids=%s',
+                       record.fqdn, active_monitor_ids)
 
         if active_monitor_ids is None:
             active_monitor_ids = set()
@@ -709,6 +730,8 @@ class Ns1Provider(BaseProvider):
             if monitor_id in active_monitor_ids:
                 continue
 
+            self.log.debug('_gc_monitors:   deleting %s', monitor_id)
+
             feed_id = self._client.feeds_for_monitors.get(monitor_id)
             if feed_id:
                 self._client.datafeed_delete(self._client.datasource_id,

From 6c7abe1fd643a3444d2a261dd27391654b9350fe Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Thu, 12 Dec 2019 14:19:16 -0800
Subject: [PATCH 08/21] Ns1 still SUPPORTS_GEO

---
 octodns/provider/ns1.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 5deb2c9..f18646c 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -174,7 +174,7 @@ class Ns1Provider(BaseProvider):
         class: octodns.provider.ns1.Ns1Provider
         api_key: env/NS1_API_KEY
     '''
-    SUPPORTS_GEO = False
+    SUPPORTS_GEO = True
     SUPPORTS_DYNAMIC = True
     SUPPORTS = set(('A', 'AAAA', 'ALIAS', 'CAA', 'CNAME', 'MX', 'NAPTR',
                     'NS', 'PTR', 'SPF', 'SRV', 'TXT'))

From d7053a2e92012b7e5bee75952c116144c70bbff3 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Fri, 13 Dec 2019 11:58:18 -0800
Subject: [PATCH 09/21] Ns1Client tests for caching and minor logic

---
 octodns/provider/ns1.py            |  11 +-
 tests/test_octodns_provider_ns1.py | 186 +++++++++++++++++++++++++++++
 2 files changed, 194 insertions(+), 3 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index f18646c..a3bd647 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -121,7 +121,9 @@ class Ns1Client(object):
 
     def monitors_update(self, job_id, **params):
         body = {}
-        return self._try(self._monitors.update, job_id, body, **params)
+        ret = self._try(self._monitors.update, job_id, body, **params)
+        self.monitors[ret['id']] = ret
+        return ret
 
     def notifylists_delete(self, nlid):
         return self._try(self._notifylists.delete, nlid)
@@ -238,8 +240,11 @@ class Ns1Provider(BaseProvider):
         data = {}
         if note:
             for piece in note.split(' '):
-                k, v = piece.split(':', 1)
-                data[k] = v
+                try:
+                    k, v = piece.split(':', 1)
+                    data[k] = v
+                except ValueError:
+                    pass
         return data
 
     def _data_for_geo_A(self, _type, record):
diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index 539fcfb..4849684 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -517,6 +517,24 @@ class TestNs1Provider(TestCase):
                          provider._data_for_CNAME(b_record['type'], b_record))
 
 
+class TestNs1ProviderDynamic(TestCase):
+
+    def test_notes(self):
+        provider = Ns1Provider('test', 'api-key')
+
+        self.assertEquals({}, provider._parse_notes(None))
+        self.assertEquals({}, provider._parse_notes(''))
+        self.assertEquals({}, provider._parse_notes('blah-blah-blah'))
+
+        # Round tripping
+        data = {
+            'key': 'value',
+            'priority': '1',
+        }
+        notes = provider._encode_notes(data)
+        self.assertEquals(data, provider._parse_notes(notes))
+
+
 class TestNs1Client(TestCase):
 
     @patch('ns1.rest.zones.Zones.retrieve')
@@ -558,3 +576,171 @@ class TestNs1Client(TestCase):
         with self.assertRaises(RateLimitException) as ctx:
             client.zones_retrieve('unit.tests')
         self.assertEquals('last', text_type(ctx.exception))
+
+    @patch('ns1.rest.data.Source.list')
+    @patch('ns1.rest.data.Source.create')
+    def test_datasource_id(self, datasource_create_mock, datasource_list_mock):
+        client = Ns1Client('dummy-key')
+
+        # First invocation with an empty list create
+        datasource_list_mock.reset_mock()
+        datasource_create_mock.reset_mock()
+        datasource_list_mock.side_effect = [[]]
+        datasource_create_mock.side_effect = [{
+            'id': 'foo',
+        }]
+        self.assertEquals('foo', client.datasource_id)
+        name = 'octoDNS NS1 Data Source'
+        source_type = 'nsone_monitoring'
+        datasource_create_mock.assert_has_calls([call(name=name,
+                                                      sourcetype=source_type)])
+        datasource_list_mock.assert_called_once()
+
+        # 2nd invocation is cached
+        datasource_list_mock.reset_mock()
+        datasource_create_mock.reset_mock()
+        self.assertEquals('foo', client.datasource_id)
+        datasource_create_mock.assert_not_called()
+        datasource_list_mock.assert_not_called()
+
+        # Reset the client's cache
+        client._datasource_id = None
+
+        # First invocation with a match in the list finds it and doesn't call
+        # create
+        datasource_list_mock.reset_mock()
+        datasource_create_mock.reset_mock()
+        datasource_list_mock.side_effect = [[{
+            'id': 'other',
+            'name': 'not a match',
+        }, {
+            'id': 'bar',
+            'name': name,
+        }]]
+        self.assertEquals('bar', client.datasource_id)
+        datasource_create_mock.assert_not_called()
+        datasource_list_mock.assert_called_once()
+
+    @patch('ns1.rest.data.Feed.delete')
+    @patch('ns1.rest.data.Feed.create')
+    @patch('ns1.rest.data.Feed.list')
+    def test_feeds_for_monitors(self, datafeed_list_mock,
+                                datafeed_create_mock,
+                                datafeed_delete_mock):
+        client = Ns1Client('dummy-key')
+
+        # pre-cache datasource_id
+        client._datasource_id = 'foo'
+
+        # Populate the cache and check the results
+        datafeed_list_mock.reset_mock()
+        datafeed_list_mock.side_effect = [[{
+            'config': {
+                'jobid': 'the-job',
+            },
+            'id': 'the-feed',
+        }, {
+            'config': {
+                'jobid': 'the-other-job',
+            },
+            'id': 'the-other-feed',
+        }]]
+        expected = {
+            'the-job': 'the-feed',
+            'the-other-job': 'the-other-feed',
+        }
+        self.assertEquals(expected, client.feeds_for_monitors)
+        datafeed_list_mock.assert_called_once()
+
+        # 2nd call uses cache
+        datafeed_list_mock.reset_mock()
+        self.assertEquals(expected, client.feeds_for_monitors)
+        datafeed_list_mock.assert_not_called()
+
+        # create a feed and make sure it's in the cache/map
+        datafeed_create_mock.reset_mock()
+        datafeed_create_mock.side_effect = [{
+            'id': 'new-feed',
+        }]
+        client.datafeed_create(client.datasource_id, 'new-name', {
+            'jobid': 'new-job',
+        })
+        datafeed_create_mock.assert_has_calls([call('foo', 'new-name', {
+            'jobid': 'new-job',
+        })])
+        new_expected = expected.copy()
+        new_expected['new-job'] = 'new-feed'
+        self.assertEquals(new_expected, client.feeds_for_monitors)
+        datafeed_create_mock.assert_called_once()
+
+        # Delete a feed and make sure it's out of the cache/map
+        datafeed_delete_mock.reset_mock()
+        client.datafeed_delete(client.datasource_id, 'new-feed')
+        self.assertEquals(expected, client.feeds_for_monitors)
+        datafeed_delete_mock.assert_called_once()
+
+    @patch('ns1.rest.monitoring.Monitors.delete')
+    @patch('ns1.rest.monitoring.Monitors.update')
+    @patch('ns1.rest.monitoring.Monitors.create')
+    @patch('ns1.rest.monitoring.Monitors.list')
+    def test_monitors(self, monitors_list_mock, monitors_create_mock,
+                      monitors_update_mock, monitors_delete_mock):
+        client = Ns1Client('dummy-key')
+
+        one = {
+            'id': 'one',
+            'key': 'value',
+        }
+        two = {
+            'id': 'two',
+            'key': 'other-value',
+        }
+
+        # Populate the cache and check the results
+        monitors_list_mock.reset_mock()
+        monitors_list_mock.side_effect = [[one, two]]
+        expected = {
+            'one': one,
+            'two': two,
+        }
+        self.assertEquals(expected, client.monitors)
+        monitors_list_mock.assert_called_once()
+
+        # 2nd round pulls it from cache
+        monitors_list_mock.reset_mock()
+        self.assertEquals(expected, client.monitors)
+        monitors_list_mock.assert_not_called()
+
+        # Create a monitor, make sure it's in the list
+        monitors_create_mock.reset_mock()
+        monitor = {
+            'id': 'new-id',
+            'key': 'new-value',
+        }
+        monitors_create_mock.side_effect = [monitor]
+        self.assertEquals(monitor, client.monitors_create(param='eter'))
+        monitors_create_mock.assert_has_calls([call({}, param='eter')])
+        new_expected = expected.copy()
+        new_expected['new-id'] = monitor
+        self.assertEquals(new_expected, client.monitors)
+
+        # Update a monitor, make sure it's updated in the cache
+        monitors_update_mock.reset_mock()
+        monitor = {
+            'id': 'new-id',
+            'key': 'changed-value',
+        }
+        monitors_update_mock.side_effect = [monitor]
+        self.assertEquals(monitor, client.monitors_update('new-id',
+                                                          key='changed-value'))
+        monitors_update_mock \
+            .assert_has_calls([call('new-id', {}, key='changed-value')])
+        new_expected['new-id'] = monitor
+        self.assertEquals(new_expected, client.monitors)
+
+        # Delete a monitor, make sure it's out of the list
+        monitors_delete_mock.reset_mock()
+        monitors_delete_mock.side_effect = ['deleted']
+        self.assertEquals('deleted', client.monitors_delete('new-id'))
+        monitors_delete_mock.assert_has_calls([call('new-id')])
+        self.assertEquals(expected, client.monitors)

From 8ec84f49bb79d6b55a65ca50aec64f5a7b4bac11 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Fri, 13 Dec 2019 12:39:14 -0800
Subject: [PATCH 10/21] More ns1 code coverage, bug fix for monitor matching

---
 octodns/provider/ns1.py            |   7 +-
 tests/test_octodns_provider_ns1.py | 157 +++++++++++++++++++++++++++++
 2 files changed, 162 insertions(+), 2 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index a3bd647..8f13ee5 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -602,7 +602,7 @@ class Ns1Provider(BaseProvider):
 
             for monitor in self._client.monitors.values():
                 data = self._parse_notes(monitor['notes'])
-                if expected_host == data['host'] or \
+                if expected_host == data['host'] and \
                    expected_type == data['type']:
                     # This monitor does not belong to this record
                     config = monitor['config']
@@ -611,11 +611,14 @@ class Ns1Provider(BaseProvider):
 
         return monitors
 
+    def _uuid(self):
+        return uuid4().hex
+
     def _create_feed(self, monitor):
         monitor_id = monitor['id']
         self.log.debug('_create_feed: monitor=%s', monitor_id)
         # TODO: looks like length limit is 64 char
-        name = '{} - {}'.format(monitor['name'], uuid4().hex[:6])
+        name = '{} - {}'.format(monitor['name'], self._uuid()[:6])
 
         # Create the data feed
         config = {
diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index 4849684..ea7dcf2 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -518,6 +518,36 @@ class TestNs1Provider(TestCase):
 
 
 class TestNs1ProviderDynamic(TestCase):
+    zone = Zone('unit.tests.', [])
+
+    record = Record.new(zone, '', {
+        'dynamic': {
+            'pools': {
+                'iad': {
+                    'values': [{
+                        'value': '1.2.3.4',
+                    }, {
+                        'value': '2.3.4.5',
+                    }],
+                },
+            },
+            'rules': [{
+                'pool': 'iad',
+            }],
+        },
+        'octodns': {
+            'healthcheck': {
+                'host': 'send.me',
+                'path': '/_ping',
+                'port': 80,
+                'protocol': 'HTTP',
+            }
+        },
+        'ttl': 32,
+        'type': 'A',
+        'value': '1.2.3.4',
+        'meta': {},
+    })
 
     def test_notes(self):
         provider = Ns1Provider('test', 'api-key')
@@ -534,6 +564,133 @@ class TestNs1ProviderDynamic(TestCase):
         notes = provider._encode_notes(data)
         self.assertEquals(data, provider._parse_notes(notes))
 
+    def test_monitors_for(self):
+        provider = Ns1Provider('test', 'api-key')
+
+        # pre-populate the client's monitors cache
+        monitor_one = {
+            'config': {
+                'host': '1.2.3.4',
+            },
+            'notes': 'host:unit.tests type:A',
+        }
+        monitor_four = {
+            'config': {
+                'host': '2.3.4.5',
+            },
+            'notes': 'host:unit.tests type:A',
+        }
+        provider._client._monitors_cache = {
+            'one': monitor_one,
+            'two': {
+                'config': {
+                    'host': '8.8.8.8',
+                },
+                'notes': 'host:unit.tests type:AAAA',
+            },
+            'three': {
+                'config': {
+                    'host': '9.9.9.9',
+                },
+                'notes': 'host:other.unit.tests type:A',
+            },
+            'four': monitor_four,
+        }
+
+        # Would match, but won't get there b/c it's not dynamic
+        record = Record.new(self.zone, '', {
+            'ttl': 32,
+            'type': 'A',
+            'value': '1.2.3.4',
+            'meta': {},
+        })
+        self.assertEquals({}, provider._monitors_for(record))
+
+        # Will match some records
+        self.assertEquals({
+            '1.2.3.4': monitor_one,
+            '2.3.4.5': monitor_four,
+        }, provider._monitors_for(self.record))
+
+    def test_uuid(self):
+        # Just a smoke test/for coverage
+        provider = Ns1Provider('test', 'api-key')
+        self.assertTrue(provider._uuid())
+
+    @patch('octodns.provider.ns1.Ns1Provider._uuid')
+    @patch('ns1.rest.data.Feed.create')
+    def test_create_feed(self, datafeed_create_mock, uuid_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 = {}
+
+        uuid_mock.reset_mock()
+        datafeed_create_mock.reset_mock()
+        uuid_mock.side_effect = ['xxxxxxxxxxxxxx']
+        feed = {
+            'id': 'feed',
+        }
+        datafeed_create_mock.side_effect = [feed]
+        monitor = {
+            'id': 'one',
+            'name': 'one name',
+            'config': {
+                'host': '1.2.3.4',
+            },
+            'notes': 'host:unit.tests type:A',
+        }
+        self.assertEquals('feed', provider._create_feed(monitor))
+        datafeed_create_mock.assert_has_calls([call('foo', 'one name - xxxxxx',
+                                                    {'jobid': 'one'})])
+
+    @patch('octodns.provider.ns1.Ns1Provider._create_feed')
+    @patch('octodns.provider.ns1.Ns1Client.monitors_create')
+    @patch('octodns.provider.ns1.Ns1Client.notifylists_create')
+    def test_create_monitor(self, notifylists_create_mock,
+                            monitors_create_mock, create_feed_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 = {}
+
+        notifylists_create_mock.reset_mock()
+        monitors_create_mock.reset_mock()
+        create_feed_mock.reset_mock()
+        notifylists_create_mock.side_effect = [{
+            'id': 'nl-id',
+        }]
+        monitors_create_mock.side_effect = [{
+            'id': 'mon-id',
+        }]
+        create_feed_mock.side_effect = ['feed-id']
+        monitor = {
+            'name': 'test monitor',
+        }
+        monitor_id, feed_id = provider._create_monitor(monitor)
+        self.assertEquals('mon-id', monitor_id)
+        self.assertEquals('feed-id', feed_id)
+        monitors_create_mock.assert_has_calls([call(name='test monitor',
+                                                    notify_list='nl-id')])
+
+    def test_monitor_gen(self):
+        provider = Ns1Provider('test', 'api-key')
+
+        value = '3.4.5.6'
+        monitor = provider._monitor_gen(self.record, value)
+        self.assertEquals(value, monitor['config']['host'])
+        self.assertTrue('\\nHost: send.me\\r' in monitor['config']['send'])
+        self.assertFalse(monitor['config']['ssl'])
+        self.assertEquals('host:unit.tests type:A', monitor['notes'])
+
+        self.record._octodns['healthcheck']['protocol'] = 'HTTPS'
+        monitor = provider._monitor_gen(self.record, value)
+        self.assertTrue(monitor['config']['ssl'])
+
 
 class TestNs1Client(TestCase):
 

From 4022155b72f4f3526145e2c297fe00e2338ec3ff Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Fri, 13 Dec 2019 13:07:32 -0800
Subject: [PATCH 11/21] Method naming consistency, test coverage for feeds and
 monitors

---
 octodns/provider/ns1.py            |  20 ++--
 tests/test_octodns_provider_ns1.py | 154 +++++++++++++++++++++++++++--
 2 files changed, 156 insertions(+), 18 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 8f13ee5..5cb7c2e 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -614,9 +614,9 @@ class Ns1Provider(BaseProvider):
     def _uuid(self):
         return uuid4().hex
 
-    def _create_feed(self, monitor):
+    def _feed_create(self, monitor):
         monitor_id = monitor['id']
-        self.log.debug('_create_feed: monitor=%s', monitor_id)
+        self.log.debug('_feed_create: monitor=%s', monitor_id)
         # TODO: looks like length limit is 64 char
         name = '{} - {}'.format(monitor['name'], self._uuid()[:6])
 
@@ -627,12 +627,12 @@ class Ns1Provider(BaseProvider):
         feed = self._client.datafeed_create(self._client.datasource_id, name,
                                             config)
         feed_id = feed['id']
-        self.log.debug('_create_feed:   feed=%s', feed_id)
+        self.log.debug('_feed_create:   feed=%s', feed_id)
 
         return feed_id
 
-    def _create_monitor(self, monitor):
-        self.log.debug('_create_monitor: monitor="%s"', monitor['name'])
+    def _monitor_create(self, monitor):
+        self.log.debug('_monitor_create: monitor="%s"', monitor['name'])
         # Create the notify list
         notify_list = [{
             'config': {
@@ -643,15 +643,15 @@ class Ns1Provider(BaseProvider):
         nl = self._client.notifylists_create(name=monitor['name'],
                                              notify_list=notify_list)
         nl_id = nl['id']
-        self.log.debug('_create_monitor:   notify_list=%s', nl_id)
+        self.log.debug('_monitor_create:   notify_list=%s', nl_id)
 
         # Create the monitor
         monitor['notify_list'] = nl_id
         monitor = self._client.monitors_create(**monitor)
         monitor_id = monitor['id']
-        self.log.debug('_create_monitor:   monitor=%s', monitor_id)
+        self.log.debug('_monitor_create:   monitor=%s', monitor_id)
 
-        return monitor_id, self._create_feed(monitor)
+        return monitor_id, self._feed_create(monitor)
 
     def _monitor_gen(self, record, value):
         host = record.fqdn[:-1]
@@ -718,11 +718,11 @@ class Ns1Provider(BaseProvider):
             if feed_id is None:
                 self.log.warn('_monitor_sync: %s (%s) missing feed, creating',
                               existing['name'], monitor_id)
-                feed_id = self._create_feed(existing)
+                feed_id = self._feed_create(existing)
         else:
             self.log.debug('_monitor_sync:   needs create')
             # We don't have an existing monitor create it (and related bits)
-            monitor_id, feed_id = self._create_monitor(expected)
+            monitor_id, feed_id = self._monitor_create(expected)
 
         return monitor_id, feed_id
 
diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index ea7dcf2..994b84a 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -619,7 +619,7 @@ class TestNs1ProviderDynamic(TestCase):
 
     @patch('octodns.provider.ns1.Ns1Provider._uuid')
     @patch('ns1.rest.data.Feed.create')
-    def test_create_feed(self, datafeed_create_mock, uuid_mock):
+    def test_feed_create(self, datafeed_create_mock, uuid_mock):
         provider = Ns1Provider('test', 'api-key')
 
         # pre-fill caches to avoid extranious calls (things we're testing
@@ -642,15 +642,15 @@ class TestNs1ProviderDynamic(TestCase):
             },
             'notes': 'host:unit.tests type:A',
         }
-        self.assertEquals('feed', provider._create_feed(monitor))
+        self.assertEquals('feed', provider._feed_create(monitor))
         datafeed_create_mock.assert_has_calls([call('foo', 'one name - xxxxxx',
                                                     {'jobid': 'one'})])
 
-    @patch('octodns.provider.ns1.Ns1Provider._create_feed')
+    @patch('octodns.provider.ns1.Ns1Provider._feed_create')
     @patch('octodns.provider.ns1.Ns1Client.monitors_create')
     @patch('octodns.provider.ns1.Ns1Client.notifylists_create')
-    def test_create_monitor(self, notifylists_create_mock,
-                            monitors_create_mock, create_feed_mock):
+    def test_monitor_create(self, notifylists_create_mock,
+                            monitors_create_mock, feed_create_mock):
         provider = Ns1Provider('test', 'api-key')
 
         # pre-fill caches to avoid extranious calls (things we're testing
@@ -660,18 +660,18 @@ class TestNs1ProviderDynamic(TestCase):
 
         notifylists_create_mock.reset_mock()
         monitors_create_mock.reset_mock()
-        create_feed_mock.reset_mock()
+        feed_create_mock.reset_mock()
         notifylists_create_mock.side_effect = [{
             'id': 'nl-id',
         }]
         monitors_create_mock.side_effect = [{
             'id': 'mon-id',
         }]
-        create_feed_mock.side_effect = ['feed-id']
+        feed_create_mock.side_effect = ['feed-id']
         monitor = {
             'name': 'test monitor',
         }
-        monitor_id, feed_id = provider._create_monitor(monitor)
+        monitor_id, feed_id = provider._monitor_create(monitor)
         self.assertEquals('mon-id', monitor_id)
         self.assertEquals('feed-id', feed_id)
         monitors_create_mock.assert_has_calls([call(name='test monitor',
@@ -691,6 +691,144 @@ class TestNs1ProviderDynamic(TestCase):
         monitor = provider._monitor_gen(self.record, value)
         self.assertTrue(monitor['config']['ssl'])
 
+    def test_monitor_is_match(self):
+        provider = Ns1Provider('test', 'api-key')
+
+        # Empty matches empty
+        self.assertTrue(provider._monitor_is_match({}, {}))
+
+        # Anything matches empty
+        self.assertTrue(provider._monitor_is_match({}, {
+            'anything': 'goes'
+        }))
+
+        # Missing doesn't match
+        self.assertFalse(provider._monitor_is_match({
+            'exepct': 'this',
+        }, {
+            'anything': 'goes'
+        }))
+
+        # Identical matches
+        self.assertTrue(provider._monitor_is_match({
+            'exepct': 'this',
+        }, {
+            'exepct': 'this',
+        }))
+
+        # Different values don't match
+        self.assertFalse(provider._monitor_is_match({
+            'exepct': 'this',
+        }, {
+            'exepct': 'that',
+        }))
+
+        # Different sub-values don't match
+        self.assertFalse(provider._monitor_is_match({
+            'exepct': {
+                'this': 'to-be',
+            },
+        }, {
+            'exepct': {
+                'this': 'something-else',
+            },
+        }))
+
+    @patch('octodns.provider.ns1.Ns1Provider._feed_create')
+    @patch('octodns.provider.ns1.Ns1Client.monitors_update')
+    @patch('octodns.provider.ns1.Ns1Provider._monitor_create')
+    @patch('octodns.provider.ns1.Ns1Provider._monitor_gen')
+    def test_monitor_sync(self, monitor_gen_mock, monitor_create_mock,
+                          monitors_update_mock, feed_create_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',
+        }
+
+        # No existing monitor
+        monitor_gen_mock.reset_mock()
+        monitor_create_mock.reset_mock()
+        monitors_update_mock.reset_mock()
+        feed_create_mock.reset_mock()
+        monitor_gen_mock.side_effect = [{'key': 'value'}]
+        monitor_create_mock.side_effect = [('mon-id', 'feed-id')]
+        value = '1.2.3.4'
+        monitor_id, feed_id = provider._monitor_sync(self.record, value, None)
+        self.assertEquals('mon-id', monitor_id)
+        self.assertEquals('feed-id', feed_id)
+        monitor_gen_mock.assert_has_calls([call(self.record, value)])
+        monitor_create_mock.assert_has_calls([call({'key': 'value'})])
+        monitors_update_mock.assert_not_called()
+        feed_create_mock.assert_not_called()
+
+        # Existing monitor that doesn't need updates
+        monitor_gen_mock.reset_mock()
+        monitor_create_mock.reset_mock()
+        monitors_update_mock.reset_mock()
+        feed_create_mock.reset_mock()
+        monitor = {
+            'id': 'mon-id',
+            'key': 'value',
+            'name': 'monitor name',
+        }
+        monitor_gen_mock.side_effect = [monitor]
+        monitor_id, feed_id = provider._monitor_sync(self.record, value,
+                                                     monitor)
+        self.assertEquals('mon-id', monitor_id)
+        self.assertEquals('feed-id', feed_id)
+        monitor_gen_mock.assert_called_once()
+        monitor_create_mock.assert_not_called()
+        monitors_update_mock.assert_not_called()
+        feed_create_mock.assert_not_called()
+
+        # Existing monitor that doesn't need updates, but is missing its feed
+        monitor_gen_mock.reset_mock()
+        monitor_create_mock.reset_mock()
+        monitors_update_mock.reset_mock()
+        feed_create_mock.reset_mock()
+        monitor = {
+            'id': 'mon-id2',
+            'key': 'value',
+            'name': 'monitor name',
+        }
+        monitor_gen_mock.side_effect = [monitor]
+        feed_create_mock.side_effect = ['feed-id2']
+        monitor_id, feed_id = provider._monitor_sync(self.record, value,
+                                                     monitor)
+        self.assertEquals('mon-id2', monitor_id)
+        self.assertEquals('feed-id2', feed_id)
+        monitor_gen_mock.assert_called_once()
+        monitor_create_mock.assert_not_called()
+        monitors_update_mock.assert_not_called()
+        feed_create_mock.assert_has_calls([call(monitor)])
+
+        # Existing monitor that needs updates
+        monitor_gen_mock.reset_mock()
+        monitor_create_mock.reset_mock()
+        monitors_update_mock.reset_mock()
+        feed_create_mock.reset_mock()
+        monitor = {
+            'id': 'mon-id',
+            'key': 'value',
+            'name': 'monitor name',
+        }
+        gened = {
+            'other': 'thing',
+        }
+        monitor_gen_mock.side_effect = [gened]
+        monitor_id, feed_id = provider._monitor_sync(self.record, value,
+                                                     monitor)
+        self.assertEquals('mon-id', monitor_id)
+        self.assertEquals('feed-id', feed_id)
+        monitor_gen_mock.assert_called_once()
+        monitor_create_mock.assert_not_called()
+        monitors_update_mock.assert_has_calls([call('mon-id', other='thing')])
+        feed_create_mock.assert_not_called()
+
 
 class TestNs1Client(TestCase):
 

From 0f298e51bef5b93f0722d335f8f9515964eb51db Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Fri, 13 Dec 2019 13:22:54 -0800
Subject: [PATCH 12/21] Tests for ns1 _monitors_gc

---
 octodns/provider/ns1.py            | 12 ++---
 tests/test_octodns_provider_ns1.py | 83 ++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+), 6 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index 5cb7c2e..e8b8d17 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -726,8 +726,8 @@ class Ns1Provider(BaseProvider):
 
         return monitor_id, feed_id
 
-    def _gc_monitors(self, record, active_monitor_ids=None):
-        self.log.debug('_gc_monitors: record=%s, active_monitor_ids=%s',
+    def _monitors_gc(self, record, active_monitor_ids=None):
+        self.log.debug('_monitors_gc: record=%s, active_monitor_ids=%s',
                        record.fqdn, active_monitor_ids)
 
         if active_monitor_ids is None:
@@ -738,7 +738,7 @@ class Ns1Provider(BaseProvider):
             if monitor_id in active_monitor_ids:
                 continue
 
-            self.log.debug('_gc_monitors:   deleting %s', monitor_id)
+            self.log.debug('_monitors_gc:   deleting %s', monitor_id)
 
             feed_id = self._client.feeds_for_monitors.get(monitor_id)
             if feed_id:
@@ -957,7 +957,7 @@ class Ns1Provider(BaseProvider):
         params, active_monitor_ids = \
             getattr(self, '_params_for_{}'.format(_type))(new)
         self._client.records_create(zone, domain, _type, **params)
-        self._gc_monitors(new, active_monitor_ids)
+        self._monitors_gc(new, active_monitor_ids)
 
     def _apply_Update(self, ns1_zone, change):
         new = change.new
@@ -967,7 +967,7 @@ class Ns1Provider(BaseProvider):
         params, active_monitor_ids = \
             getattr(self, '_params_for_{}'.format(_type))(new)
         self._client.records_update(zone, domain, _type, **params)
-        self._gc_monitors(new, active_monitor_ids)
+        self._monitors_gc(new, active_monitor_ids)
 
     def _apply_Delete(self, ns1_zone, change):
         existing = change.existing
@@ -975,7 +975,7 @@ class Ns1Provider(BaseProvider):
         domain = existing.fqdn[:-1]
         _type = existing._type
         self._client.records_delete(zone, domain, _type)
-        self._gc_monitors(existing)
+        self._monitors_gc(existing)
 
     def _apply(self, plan):
         desired = plan.desired
diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index 994b84a..68e87c9 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -829,6 +829,89 @@ class TestNs1ProviderDynamic(TestCase):
         monitors_update_mock.assert_has_calls([call('mon-id', other='thing')])
         feed_create_mock.assert_not_called()
 
+    @patch('octodns.provider.ns1.Ns1Client.notifylists_delete')
+    @patch('octodns.provider.ns1.Ns1Client.monitors_delete')
+    @patch('octodns.provider.ns1.Ns1Client.datafeed_delete')
+    @patch('octodns.provider.ns1.Ns1Provider._monitors_for')
+    def test_monitors_gc(self, monitors_for_mock, datafeed_delete_mock,
+                         monitors_delete_mock, notifylists_delete_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',
+        }
+
+        # No active monitors and no existing, nothing will happen
+        monitors_for_mock.reset_mock()
+        datafeed_delete_mock.reset_mock()
+        monitors_delete_mock.reset_mock()
+        notifylists_delete_mock.reset_mock()
+        monitors_for_mock.side_effect = [{}]
+        provider._monitors_gc(self.record)
+        monitors_for_mock.assert_has_calls([call(self.record)])
+        datafeed_delete_mock.assert_not_called()
+        monitors_delete_mock.assert_not_called()
+        notifylists_delete_mock.assert_not_called()
+
+        # No active monitors and one existing, delete all the things
+        monitors_for_mock.reset_mock()
+        datafeed_delete_mock.reset_mock()
+        monitors_delete_mock.reset_mock()
+        notifylists_delete_mock.reset_mock()
+        monitors_for_mock.side_effect = [{
+            'x': {
+                'id': 'mon-id',
+                'notify_list': 'nl-id',
+            }
+        }]
+        provider._monitors_gc(self.record)
+        monitors_for_mock.assert_has_calls([call(self.record)])
+        datafeed_delete_mock.assert_has_calls([call('foo', 'feed-id')])
+        monitors_delete_mock.assert_has_calls([call('mon-id')])
+        notifylists_delete_mock.assert_has_calls([call('nl-id')])
+
+        # Same existing, this time in active list, should be noop
+        monitors_for_mock.reset_mock()
+        datafeed_delete_mock.reset_mock()
+        monitors_delete_mock.reset_mock()
+        notifylists_delete_mock.reset_mock()
+        monitors_for_mock.side_effect = [{
+            'x': {
+                'id': 'mon-id',
+                'notify_list': 'nl-id',
+            }
+        }]
+        provider._monitors_gc(self.record, {'mon-id'})
+        monitors_for_mock.assert_has_calls([call(self.record)])
+        datafeed_delete_mock.assert_not_called()
+        monitors_delete_mock.assert_not_called()
+        notifylists_delete_mock.assert_not_called()
+
+        # Non-active monitor w/o a feed, and another monitor that's left alone
+        # b/c it's active
+        monitors_for_mock.reset_mock()
+        datafeed_delete_mock.reset_mock()
+        monitors_delete_mock.reset_mock()
+        notifylists_delete_mock.reset_mock()
+        monitors_for_mock.side_effect = [{
+            'x': {
+                'id': 'mon-id',
+                'notify_list': 'nl-id',
+            },
+            'y': {
+                'id': 'mon-id2',
+                'notify_list': 'nl-id2',
+            },
+        }]
+        provider._monitors_gc(self.record, {'mon-id'})
+        monitors_for_mock.assert_has_calls([call(self.record)])
+        datafeed_delete_mock.assert_not_called()
+        monitors_delete_mock.assert_has_calls([call('mon-id2')])
+        notifylists_delete_mock.assert_has_calls([call('nl-id2')])
+
 
 class TestNs1Client(TestCase):
 

From 561a6ca2d98633023a131e491f3b24212ce447fb Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 6 Jan 2020 08:31:35 -0800
Subject: [PATCH 13/21] Test coverage for Ns1Provider _params_for_dynamic_A

---
 tests/test_octodns_provider_ns1.py | 45 ++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index 68e87c9..bc40f8a 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -523,6 +523,12 @@ class TestNs1ProviderDynamic(TestCase):
     record = Record.new(zone, '', {
         'dynamic': {
             'pools': {
+                'lhr': {
+                    'fallback': 'iad',
+                    'values': [{
+                        'value': '3.4.5.6',
+                    }],
+                },
                 'iad': {
                     'values': [{
                         'value': '1.2.3.4',
@@ -532,6 +538,13 @@ class TestNs1ProviderDynamic(TestCase):
                 },
             },
             'rules': [{
+                'geos': [
+                    'AF',
+                    'EU-GB',
+                    'NA-US-FL'
+                ],
+                'pool': 'lhr',
+            }, {
                 'pool': 'iad',
             }],
         },
@@ -912,6 +925,38 @@ 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(self, monitors_for_mock, monitors_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',
+        }
+
+        monitors_for_mock.reset_mock()
+        monitors_sync_mock.reset_mock()
+        monitors_for_mock.side_effect = [{
+            '3.4.5.6': 'mid-3',
+        }]
+        monitors_sync_mock.side_effect = [
+            ('mid-1', 'fid-1'),
+            ('mid-2', 'fid-2'),
+            ('mid-3', 'fid-3'),
+        ]
+        # This indirectly calls into _params_for_dynamic_A and tests the
+        # handling to get there
+        provider._params_for_A(self.record)
+        monitors_for_mock.assert_has_calls([call(self.record)])
+        monitors_sync_mock.assert_has_calls([
+            call(self.record, '1.2.3.4', None),
+            call(self.record, '2.3.4.5', None),
+            call(self.record, '3.4.5.6', 'mid-3'),
+        ])
+
 
 class TestNs1Client(TestCase):
 

From 69cd30a1832c30a998528c4c6549f460ad1ee128 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 6 Jan 2020 09:18:10 -0800
Subject: [PATCH 14/21] Coverage for Ns1Provider _data_for_dynamic_A

---
 tests/test_octodns_provider_ns1.py | 136 ++++++++++++++++++++++++++++-
 1 file changed, 135 insertions(+), 1 deletion(-)

diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index bc40f8a..a985c13 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -13,7 +13,7 @@ from six import text_type
 from unittest import TestCase
 
 from octodns.record import Delete, Record, Update
-from octodns.provider.ns1 import Ns1Client, Ns1Provider
+from octodns.provider.ns1 import Ns1Client, Ns1Exception, Ns1Provider
 from octodns.zone import Zone
 
 
@@ -957,6 +957,140 @@ class TestNs1ProviderDynamic(TestCase):
             call(self.record, '3.4.5.6', 'mid-3'),
         ])
 
+    def test_data_for_dynamic_A(self):
+        provider = Ns1Provider('test', 'api-key')
+
+        # Unexpected filters throws an error
+        ns1_record = {
+            'domain': 'unit.tests',
+            'filters': [],
+        }
+        with self.assertRaises(Ns1Exception) as ctx:
+            provider._data_for_dynamic_A('A', ns1_record)
+        self.assertEquals('Unrecognized advanced record',
+                          text_type(ctx.exception))
+
+        # empty record turns into empty data
+        ns1_record = {
+            'answers': [],
+            'domain': 'unit.tests',
+            'filters': Ns1Provider._DYNAMIC_FILTERS,
+            'regions': {},
+            'ttl': 42,
+        }
+        data = provider._data_for_dynamic_A('A', ns1_record)
+        self.assertEquals({
+            'dynamic': {
+                'pools': {},
+                'rules': [],
+            },
+            'ttl': 42,
+            'type': 'A',
+            'values': [],
+        }, data)
+
+        # Test out a small, but realistic setup that covers all the options
+        ns1_record = {
+            'answers': [{
+                'answer': ['3.4.5.6'],
+                'meta': {
+                    'priority': 1,
+                    'note': 'from:lhr',
+                },
+                'region': 'lhr',
+            }, {
+                'answer': ['2.3.4.5'],
+                'meta': {
+                    'priority': 2,
+                    'weight': 12,
+                    'note': 'from:iad',
+                },
+                'region': 'lhr',
+            }, {
+                'answer': ['1.2.3.4'],
+                'meta': {
+                    'priority': 3,
+                    'note': 'from:--default--',
+                },
+                'region': 'lhr',
+            }, {
+                'answer': ['2.3.4.5'],
+                'meta': {
+                    'priority': 1,
+                    'weight': 12,
+                    'note': 'from:iad',
+                },
+                'region': 'iad',
+            }, {
+                'answer': ['1.2.3.4'],
+                'meta': {
+                    'priority': 2,
+                    'note': 'from:--default--',
+                },
+                'region': 'iad',
+            }],
+            'domain': 'unit.tests',
+            'filters': Ns1Provider._DYNAMIC_FILTERS,
+            'regions': {
+                'lhr': {
+                    'meta': {
+                        'note': 'rule-order:1 fallback:iad',
+                        'country': ['CA'],
+                        'georegion': ['AFRICA'],
+                        'us_state': ['OR'],
+                    },
+                },
+                'iad': {
+                    'meta': {
+                        'note': 'rule-order:2',
+                    },
+                }
+            },
+            'tier': 3,
+            'ttl': 42,
+        }
+        data = provider._data_for_dynamic_A('A', ns1_record)
+        self.assertEquals({
+            'dynamic': {
+                'pools': {
+                    'iad': {
+                        'fallback': None,
+                        'values': [{
+                            'value': '2.3.4.5',
+                            'weight': 12,
+                        }],
+                    },
+                    'lhr': {
+                        'fallback': 'iad',
+                        'values': [{
+                            'weight': 1,
+                            'value': '3.4.5.6',
+                        }],
+                    },
+                },
+                'rules': [{
+                    '_order': '1',
+                    'geos': [
+                        'AF',
+                        'NA-CA',
+                        'NA-US-OR',
+                    ],
+                    'pool': 'lhr',
+                }, {
+                    '_order': '2',
+                    'pool': 'iad',
+                }],
+            },
+            'ttl': 42,
+            'type': 'A',
+            'values': ['1.2.3.4'],
+        }, data)
+
+        # Same answer if we go through _data_for_A which out sources the job to
+        # _data_for_dynamic_A
+        data2 = provider._data_for_A('A', ns1_record)
+        self.assertEquals(data, data2)
+
 
 class TestNs1Client(TestCase):
 

From eefd83de80de20ed4937f892f7da235c63d1a0e2 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 6 Jan 2020 10:04:07 -0800
Subject: [PATCH 15/21] Coverage for Ns1Provider _extra_changes

---
 tests/test_octodns_provider_ns1.py | 107 +++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index a985c13..4c505e4 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -1091,6 +1091,113 @@ class TestNs1ProviderDynamic(TestCase):
         data2 = provider._data_for_A('A', ns1_record)
         self.assertEquals(data, data2)
 
+    @patch('octodns.provider.ns1.Ns1Provider._monitors_for')
+    def test_extra_changes(self, monitors_for_mock):
+        provider = Ns1Provider('test', 'api-key')
+
+        desired = Zone('unit.tests.', [])
+
+        # Empty zone and no changes
+        monitors_for_mock.reset_mock()
+        extra = provider._extra_changes(desired, [])
+        self.assertFalse(extra)
+        monitors_for_mock.assert_not_called()
+
+        # Simple record, ignored
+        monitors_for_mock.reset_mock()
+        simple = Record.new(desired, '', {
+            'ttl': 32,
+            'type': 'A',
+            'value': '1.2.3.4',
+            'meta': {},
+        })
+        desired.add_record(simple)
+        extra = provider._extra_changes(desired, [])
+        self.assertFalse(extra)
+        monitors_for_mock.assert_not_called()
+
+        # Dynamic record, inspectable
+        dynamic = Record.new(desired, 'dyn', {
+            'dynamic': {
+                'pools': {
+                    'iad': {
+                        'values': [{
+                            'value': '1.2.3.4',
+                        }],
+                    },
+                },
+                'rules': [{
+                    'pool': 'iad',
+                }],
+            },
+            'octodns': {
+                'healthcheck': {
+                    'host': 'send.me',
+                    'path': '/_ping',
+                    'port': 80,
+                    'protocol': 'HTTP',
+                }
+            },
+            'ttl': 32,
+            'type': 'A',
+            'value': '1.2.3.4',
+            'meta': {},
+        })
+        desired.add_record(dynamic)
+
+        # untouched, but everything in sync so no change needed
+        monitors_for_mock.reset_mock()
+        # Generate what we expect to have
+        gend = provider._monitor_gen(dynamic, '1.2.3.4')
+        gend.update({
+            'id': 'mid',  # need to add an id
+            'notify_list': 'xyz',  # need to add a notify list (for now)
+        })
+        monitors_for_mock.side_effect = [{
+            '1.2.3.4': gend,
+        }]
+        extra = provider._extra_changes(desired, [])
+        self.assertFalse(extra)
+        monitors_for_mock.assert_has_calls([call(dynamic)])
+
+        update = Update(dynamic, dynamic)
+
+        # If we don't have a notify list we're broken and we'll expect to see
+        # an Update
+        monitors_for_mock.reset_mock()
+        del gend['notify_list']
+        monitors_for_mock.side_effect = [{
+            '1.2.3.4': gend,
+        }]
+        extra = provider._extra_changes(desired, [])
+        self.assertEquals(1, len(extra))
+        extra = list(extra)[0]
+        self.assertIsInstance(extra, Update)
+        self.assertEquals(dynamic, extra.new)
+        monitors_for_mock.assert_has_calls([call(dynamic)])
+
+        # Add notify_list back and change the healthcheck protocol, we'll still
+        # expect to see an update
+        monitors_for_mock.reset_mock()
+        gend['notify_list'] = 'xyz'
+        dynamic._octodns['healthcheck']['protocol'] = 'HTTPS'
+        del gend['notify_list']
+        monitors_for_mock.side_effect = [{
+            '1.2.3.4': gend,
+        }]
+        extra = provider._extra_changes(desired, [])
+        self.assertEquals(1, len(extra))
+        extra = list(extra)[0]
+        self.assertIsInstance(extra, Update)
+        self.assertEquals(dynamic, extra.new)
+        monitors_for_mock.assert_has_calls([call(dynamic)])
+
+        # If it's in the changed list, it'll be ignored
+        monitors_for_mock.reset_mock()
+        extra = provider._extra_changes(desired, [update])
+        self.assertFalse(extra)
+        monitors_for_mock.assert_not_called()
+
 
 class TestNs1Client(TestCase):
 

From f91cac3ef47595f40cb53a96e88c27a59bdf27c4 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 6 Jan 2020 10:13:58 -0800
Subject: [PATCH 16/21] coverage for Ns1Client notifylist methods

---
 tests/test_octodns_provider_ns1.py | 45 ++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py
index 4c505e4..0d456fe 100644
--- a/tests/test_octodns_provider_ns1.py
+++ b/tests/test_octodns_provider_ns1.py
@@ -1408,3 +1408,48 @@ class TestNs1Client(TestCase):
         self.assertEquals('deleted', client.monitors_delete('new-id'))
         monitors_delete_mock.assert_has_calls([call('new-id')])
         self.assertEquals(expected, client.monitors)
+
+    @patch('ns1.rest.monitoring.NotifyLists.delete')
+    @patch('ns1.rest.monitoring.NotifyLists.create')
+    @patch('ns1.rest.monitoring.NotifyLists.list')
+    def test_notifylists(self, notifylists_list_mock, notifylists_create_mock,
+                         notifylists_delete_mock):
+        client = Ns1Client('dummy-key')
+
+        notifylists_list_mock.reset_mock()
+        notifylists_create_mock.reset_mock()
+        notifylists_delete_mock.reset_mock()
+        notifylists_create_mock.side_effect = ['bar']
+        notify_list = [{
+            'config': {
+                'sourceid': 'foo',
+            },
+            'type': 'datafeed',
+        }]
+        nl = client.notifylists_create(name='some name',
+                                       notify_list=notify_list)
+        self.assertEquals('bar', nl)
+        notifylists_list_mock.assert_not_called()
+        notifylists_create_mock.assert_has_calls([
+            call({'name': 'some name', 'notify_list': notify_list})
+        ])
+        notifylists_delete_mock.assert_not_called()
+
+        notifylists_list_mock.reset_mock()
+        notifylists_create_mock.reset_mock()
+        notifylists_delete_mock.reset_mock()
+        client.notifylists_delete('nlid')
+        notifylists_list_mock.assert_not_called()
+        notifylists_create_mock.assert_not_called()
+        notifylists_delete_mock.assert_has_calls([call('nlid')])
+
+        notifylists_list_mock.reset_mock()
+        notifylists_create_mock.reset_mock()
+        notifylists_delete_mock.reset_mock()
+        expected = ['one', 'two', 'three']
+        notifylists_list_mock.side_effect = [expected]
+        nls = client.notifylists_list()
+        self.assertEquals(expected, nls)
+        notifylists_list_mock.assert_has_calls([call()])
+        notifylists_create_mock.assert_not_called()
+        notifylists_delete_mock.assert_not_called()

From 95f51114871ba552d3e42d408237a8424df30820 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 6 Jan 2020 10:18:56 -0800
Subject: [PATCH 17/21] NS1 geo records will always use 'answers'

---
 octodns/provider/ns1.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index e8b8d17..e9efd3e 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -256,8 +256,6 @@ class Ns1Provider(BaseProvider):
             'type': _type,
         }
         values, codes = [], []
-        if 'answers' not in record:
-            values = record['short_answers']
         for answer in record.get('answers', []):
             meta = answer.get('meta', {})
             if meta:

From e0debc963e2b120d599cff5537a39db053ca8f92 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 6 Jan 2020 10:22:18 -0800
Subject: [PATCH 18/21] Update Dynamic support to include NS1, remove Geo
 mentions

---
 README.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/README.md b/README.md
index c3ddb0b..f884557 100644
--- a/README.md
+++ b/README.md
@@ -169,7 +169,7 @@ The above command pulled the existing data out of Route53 and placed the results
 
 ## Supported providers
 
-| Provider | Requirements | Record Support | Dynamic/Geo Support | Notes |
+| Provider | Requirements | Record Support | Dynamic | Notes |
 |--|--|--|--|--|
 | [AzureProvider](/octodns/provider/azuredns.py) | azure-mgmt-dns | A, AAAA, CAA, CNAME, MX, NS, PTR, SRV, TXT | No | |
 | [Akamai](/octodns/provider/fastdns.py) | edgegrid-python | A, AAAA, CNAME, MX, NAPTR, NS, PTR, SPF, SRV, SSHFP, TXT | No | |
@@ -182,7 +182,7 @@ The above command pulled the existing data out of Route53 and placed the results
 | [EtcHostsProvider](/octodns/provider/etc_hosts.py) | | A, AAAA, ALIAS, CNAME | No | |
 | [GoogleCloudProvider](/octodns/provider/googlecloud.py) | google-cloud-dns | A, AAAA, CAA, CNAME, MX, NAPTR, NS, PTR, SPF, 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 | Partial Geo | No health checking for GeoDNS |
+| [Ns1Provider](/octodns/provider/ns1.py) | ns1-python | All | Yes | No CNAME support, missing `NA` geo target |
 | [OVH](/octodns/provider/ovh.py) | ovh | A, AAAA, 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 |  |

From 391ef583ae3f3a63ddda82f84e00490a7fc66061 Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 6 Jan 2020 10:22:41 -0800
Subject: [PATCH 19/21] Ns1 should use geofence_regional to avoid nearest
 matching

---
 octodns/provider/ns1.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index e9efd3e..b442271 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -188,7 +188,7 @@ class Ns1Provider(BaseProvider):
         'filter': 'up'
     }, {
         'config': {},
-        'filter': u'geotarget_regional'
+        'filter': u'geofence_regional'
     }, {
         'config': {},
         'filter': u'select_first_region'

From 01a9fa87b190128c40f6ff6a96a6eb015be4119d Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 13 Jan 2020 07:29:38 -0800
Subject: [PATCH 20/21] Address Ns1Provider review feedback

---
 octodns/provider/ns1.py | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py
index b442271..0694627 100644
--- a/octodns/provider/ns1.py
+++ b/octodns/provider/ns1.py
@@ -655,9 +655,9 @@ class Ns1Provider(BaseProvider):
         host = record.fqdn[:-1]
         _type = record._type
 
-        request = 'GET {path} HTTP/1.0\\r\\nHost: {host}\\r\\n' \
-            'User-agent: NS1\\r\\n\\r\\n'.format(path=record.healthcheck_path,
-                                                 host=record.healthcheck_host)
+        request = r'GET {path} HTTP/1.0\r\nHost: {host}\r\n' \
+            r'User-agent: NS1\r\n\r\n'.format(path=record.healthcheck_path,
+                                              host=record.healthcheck_host)
 
         return {
             'active': True,
@@ -771,13 +771,13 @@ class Ns1Provider(BaseProvider):
             for geo in rule.data.get('geos', []):
                 n = len(geo)
                 if n == 8:
-                    # US state
+                    # US state, e.g. NA-US-KY
                     us_state.add(geo[-2:])
                 elif n == 5:
-                    # Country
+                    # Country, e.g. EU-FR
                     country.add(geo[-2:])
                 else:
-                    # Continent
+                    # Continent, e.g. AS
                     georegion.update(self._CONTINENT_TO_REGIONS[geo])
 
             meta = {
@@ -826,7 +826,9 @@ class Ns1Provider(BaseProvider):
 
             # Dynamic/health checked
             current_pool_name = pool_name
-            while current_pool_name:
+            seen = set()
+            while current_pool_name and current_pool_name not in seen:
+                seen.add(current_pool_name)
                 pool = pools[current_pool_name]
                 for answer in pool_answers[current_pool_name]:
                     answer = {

From c950503868ded420ee3d2a2e5df5d2e1d64fba8b Mon Sep 17 00:00:00 2001
From: Ross McFarland <ross@github.com>
Date: Mon, 13 Jan 2020 07:29:56 -0800
Subject: [PATCH 21/21] Pass through the CHANGELOG

---
 CHANGELOG.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1cb7b1b..491370f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,11 @@
+## v0.9.10 - ????-??-?? - ???
+
+* Added support for dynamic records to Ns1Provider, updated client and rate
+  limiting implementation
+* Moved CI to use GitHub Actions
+* Set up dependabot to automatically PR requirements updates
+* Pass at bumping all of the requirements
+
 ## v0.9.9 - 2019-11-04 - Python 3.7 Support
 
 * Extensive pass through the whole codebase to support Python 3