diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 1b454ba..8f19e0f 100644 --- a/octodns/provider/dyn.py +++ b/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 ' diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 85d35e6..675c0a8 100644 --- a/octodns/provider/route53.py +++ b/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 diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index acd248f..1f6f805 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/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)