Dyn support for configrable healthchecks and _extra_changes support

More thorough unit tests while I'm in here. Ended up doing some hacks/monkey
patching of DSFMonitor as the client library's object doesn't seem to be fully
functional/useful and has inconsitent behavior.

octodns/provider/dyn.py

@@ -17,10 +17,61 @@ from logging import getLogger
 from threading import Lock
 from uuid import uuid4
 
-from ..record import Record
+from ..record import Record, Update
 from .base import BaseProvider
 
 
+###############################################################################
+#
+# The following monkey patching is to work around functionality that is lacking
+# from DSFMonitor. You cannot set host or path (which we need) and there's no
+# update method. What's more host & path aren't publically accessible on the
+# object so you can't see their current values and depending on how the object
+# came to be (constructor vs pulled from the api) the "private" location of
+# those fields varies :-(
+#
+###############################################################################
+def _monitor_host_get(self):
+    return self._host or self._options['host']
+DSFMonitor.host = property(_monitor_host_get)
+
+
+def _monitor_host_set(self, value):
+    if self._options is None:
+        self._options = {}
+    self._host = self._options['host'] = value
+DSFMonitor.host = DSFMonitor.host.setter(_monitor_host_set)
+
+
+def _monitor_path_get(self):
+    return self._path or self._options['path']
+DSFMonitor.path = property(_monitor_path_get)
+
+
+def _monitor_path_set(self, value):
+    if self._options is None:
+        self._options = {}
+    self._path = self._options['path'] = value
+DSFMonitor.path = DSFMonitor.path.setter(_monitor_path_set)
+
+
+def _monitor_update(self, host, path):
+    # I can't see how to actually do this with the client lib so
+    # I'm having to hack around it. Have to provide all the
+    # options or else things complain
+    return self._update({
+        'options': {
+            'host': host,
+            'path': path,
+            'port': DynProvider.MONITOR_PORT,
+            'timeout': DynProvider.MONITOR_TIMEOUT,
+            'header': DynProvider.MONITOR_HEADER,
+        }
+    })
+DSFMonitor.update = _monitor_update
+###############################################################################
+
+
 class _CachingDynZone(DynZone):
     log = getLogger('_CachingDynZone')
 
@@ -373,6 +424,33 @@ class DynProvider(BaseProvider):
         self.log.info('populate:   found %s records',
                       len(zone.records) - before)
 
+    def _extra_changes(self, _, desired, changes):
+        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, 'geo', False):
+                # Already changed, or no geo, no need to check it
+                continue
+            try:
+                monitor = self.traffic_director_monitors[record.fqdn]
+            except KeyError:
+                self.log.info('_extra_changes: health-check missing for %s:%s',
+                              record.fqdn, record._type)
+                extra.append(Update(record, record))
+                continue
+            # Heh, when pulled from the API host & path live under options, but
+            # when you create with the constructor they're top-level :-(
+            if monitor.host != record.healthcheck_host or \
+               monitor.path != record.healthcheck_path:
+                self.log.info('_extra_changes: health-check mis-match for '
+                              '%s:%s', record.fqdn, record._type)
+                extra.append(Update(record, record))
+
+        return extra
+
     def _kwargs_for_A(self, record):
         return [{
             'address': v,
@@ -451,30 +529,24 @@ class DynProvider(BaseProvider):
 
     _kwargs_for_TXT = _kwargs_for_SPF
 
-    def _traffic_director_monitor(self, record):
+    @property
+    def traffic_director_monitors(self):
         if self._traffic_director_monitors is None:
             self._traffic_director_monitors = \
                 {m.label: m for m in get_all_dsf_monitors()}
 
+        return self._traffic_director_monitors
+
+    def _traffic_director_monitor(self, record):
         fqdn = record.fqdn
         try:
-            monitor = self._traffic_director_monitors[fqdn]
-            if monitor._host != record.healthcheck_host or \
-               monitor._path != record.healthcheck_path:
+            monitor = self.traffic_director_monitors[fqdn]
+            if monitor.host != record.healthcheck_host or \
+               monitor.path != record.healthcheck_path:
                 self.log.info('_traffic_director_monitor: updating monitor '
                               'for %s:%s', record.fqdn, record._type)
-                # I can't see how to actually do this with the client lib so
-                # I'm having to hack around it. Have to provide all the
-                # options or else things complain
-                monitor._update({
-                    'options': {
-                        'host': record.healthcheck_host,
-                        'path': record.healthcheck_path,
-                        'port': self.MONITOR_PORT,
-                        'timeout': self.MONITOR_TIMEOUT,
-                        'header': self.MONITOR_HEADER,
-                    }
-                })
+                monitor.update(record.healthcheck_host,
+                               record.healthcheck_path)
             return monitor
         except KeyError:
             self.log.info('_traffic_director_monitor: creating monitor '

octodns/provider/route53.py

@@ -646,7 +646,7 @@ class Route53Provider(BaseProvider):
                     pass
                 # no good, doesn't have the right health check, needs an update
                 self.log.info('_extra_changes:     health-check caused '
-                              'update')
+                              'update of %s:%s', record.fqdn, record._type)
                 extra.append(Update(record, record))
                 # We don't need to process this record any longer
                 break

tests/test_octodns_provider_dyn.py

@@ -13,7 +13,7 @@ from unittest import TestCase
 
 from octodns.record import Create, Delete, Record, Update
 from octodns.provider.base import Plan
-from octodns.provider.dyn import DynProvider, _CachingDynZone
+from octodns.provider.dyn import DynProvider, _CachingDynZone, DSFMonitor
 from octodns.zone import Zone
 
 from helpers import SimpleProvider
@@ -776,8 +776,87 @@ class TestDynProviderGeo(TestCase):
         self.assertTrue('unit.tests.' in
                         provider._traffic_director_monitors)
         monitor = provider._traffic_director_monitors['unit.tests.']
-        self.assertEquals('bleep.bloop', monitor._host)
-        self.assertEquals('/_nope', monitor._path)
+        from pprint import pprint
+        pprint(monitor.__dict__)
+        self.assertEquals('bleep.bloop', monitor.host)
+        self.assertEquals('/_nope', monitor.path)
+
+    @patch('dyn.core.SessionEngine.execute')
+    def test_extra_changes(self, mock):
+        provider = DynProvider('test', 'cust', 'user', 'pass', True)
+        # short-circuit session checking
+        provider._dyn_sess = True
+
+        mock.side_effect = [self.monitors_response]
+
+        # non-geo
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, '', {
+            'ttl': 60,
+            'type': 'A',
+            'value': '1.2.3.4',
+        })
+        desired.add_record(record)
+        extra = provider._extra_changes(None, desired, [Create(record)])
+        self.assertEquals(0, len(extra))
+
+        # in changes, noop
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, '', {
+            'geo': {
+                'NA': ['1.2.3.4'],
+            },
+            'ttl': 60,
+            'type': 'A',
+            'value': '1.2.3.4',
+        })
+        desired.add_record(record)
+        extra = provider._extra_changes(None, desired, [Create(record)])
+        self.assertEquals(0, len(extra))
+
+        # no diff, no extra
+        extra = provider._extra_changes(None, desired, [])
+        self.assertEquals(0, len(extra))
+
+        # diff in healthcheck, gets extra
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, '', {
+            'geo': {
+                'NA': ['1.2.3.4'],
+            },
+            'octodns': {
+                'healthcheck': {
+                    'host': 'foo.bar',
+                    'path': '/_ready'
+                }
+            },
+            'ttl': 60,
+            'type': 'A',
+            'value': '1.2.3.4',
+        })
+        desired.add_record(record)
+        extra = provider._extra_changes(None, desired, [])
+        self.assertEquals(1, len(extra))
+        extra = extra[0]
+        self.assertIsInstance(extra, Update)
+        self.assertEquals(record, extra.record)
+
+        # missing health check
+        desired = Zone('unit.tests.', [])
+        record = Record.new(desired, 'geo', {
+            'geo': {
+                'NA': ['1.2.3.4'],
+            },
+            'ttl': 60,
+            'type': 'A',
+            'value': '1.2.3.4',
+        })
+        desired.add_record(record)
+        extra = provider._extra_changes(None, desired, [])
+        self.assertEquals(1, len(extra))
+        extra = extra[0]
+        self.assertIsInstance(extra, Update)
+        self.assertEquals(record, extra.record)
 
     @patch('dyn.core.SessionEngine.execute')
     def test_populate_traffic_directors_empty(self, mock):
@@ -1335,3 +1414,46 @@ class TestDynProviderAlias(TestCase):
         execute_mock.assert_has_calls([call('/Zone/unit.tests/', 'GET', {}),
                                        call('/Zone/unit.tests/', 'GET', {})])
         self.assertEquals(2, len(plan.changes))
+
+
+# Need a class that doesn't do all the "real" stuff, but gets our monkey
+# patching
+class DummyDSFMonitor(DSFMonitor):
+
+    def __init__(self, host=None, path=None, options_host=None,
+                 options_path=None):
+        # not calling super on purpose
+        self._host = host
+        self._path = path
+        if options_host:
+            self._options = {
+                'host': options_host,
+                'path': options_path,
+            }
+        else:
+            self._options = None
+
+
+class TestDSFMonitorMonkeyPatching(TestCase):
+
+    def test_host(self):
+        monitor = DummyDSFMonitor(host='host.com', path='/path')
+        self.assertEquals('host.com', monitor.host)
+        self.assertEquals('/path', monitor.path)
+
+        monitor = DummyDSFMonitor(options_host='host.com',
+                                  options_path='/path')
+        self.assertEquals('host.com', monitor.host)
+        self.assertEquals('/path', monitor.path)
+
+        monitor.host = 'other.com'
+        self.assertEquals('other.com', monitor.host)
+        monitor.path = '/other-path'
+        self.assertEquals('/other-path', monitor.path)
+
+        monitor = DummyDSFMonitor()
+        monitor.host = 'other.com'
+        self.assertEquals('other.com', monitor.host)
+        monitor = DummyDSFMonitor()
+        monitor.path = '/other-path'
+        self.assertEquals('/other-path', monitor.path)