From 670d7eef17d2375a40d8dee3e18904c804e23db0 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 15 Jun 2017 09:04:35 -0700 Subject: [PATCH 01/23] Record.healtcheck_(host|path) --- octodns/record.py | 21 ++++++++++++++-- tests/test_octodns_record.py | 48 ++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/octodns/record.py b/octodns/record.py index cacb147..e86de2d 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -112,8 +112,7 @@ class Record(object): raise Exception('Invalid record {}, missing ttl'.format(self.fqdn)) self.source = source - octodns = data.get('octodns', {}) - self.ignored = octodns.get('ignored', False) + self._octodns = data.get('octodns', {}) def _data(self): return {'ttl': self.ttl} @@ -128,6 +127,24 @@ class Record(object): return '{}.{}'.format(self.name, self.zone.name) return self.zone.name + @property + def ignored(self): + return self._octodns.get('ignored', False) + + @property + def healthcheck_path(self): + try: + return self._octodns['healthcheck']['path'] + except KeyError: + return '/_dns' + + @property + def healthcheck_host(self): + try: + return self._octodns['healthcheck']['host'] + except KeyError: + return self.fqdn[:-1] + def changes(self, other, target): # We're assuming we have the same name and type if we're being compared if self.ttl != other.ttl: diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 52505cb..9c72413 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -794,3 +794,51 @@ class TestRecord(TestCase): self.assertEquals('CA', geo.subdivision_code) self.assertEquals(values, geo.values) self.assertEquals(['NA-US', 'NA'], list(geo.parents)) + + def test_inored(self): + new = Record.new(self.zone, 'txt', { + 'ttl': 44, + 'type': 'TXT', + 'value': 'some change', + 'octodns': { + 'ignored': True, + } + }) + self.assertTrue(new.ignored) + new = Record.new(self.zone, 'txt', { + 'ttl': 44, + 'type': 'TXT', + 'value': 'some change', + 'octodns': { + 'ignored': False, + } + }) + self.assertFalse(new.ignored) + new = Record.new(self.zone, 'txt', { + 'ttl': 44, + 'type': 'TXT', + 'value': 'some change', + }) + self.assertFalse(new.ignored) + + def test_healthcheck(self): + new = Record.new(self.zone, 'a', { + 'ttl': 44, + 'type': 'A', + 'value': '1.2.3.4', + 'octodns': { + 'healthcheck': { + 'path': '/_ready', + 'host': 'bleep.bloop', + } + } + }) + self.assertEquals('/_ready', new.healthcheck_path) + self.assertEquals('bleep.bloop', new.healthcheck_host) + new = Record.new(self.zone, 'a', { + 'ttl': 44, + 'type': 'A', + 'value': '1.2.3.4', + }) + self.assertEquals('/_dns', new.healthcheck_path) + self.assertEquals('a.unit.tests', new.healthcheck_host) From 4761da9c33a1ca899c8cc76542377e3f47e5ffb0 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 15 Jun 2017 09:57:40 -0700 Subject: [PATCH 02/23] Add custom host & path support to Route53 Geo healthchecks - This reworks the CallerReference structure for Route53 health checks in a non-backwards compatible way. This means records will create new healthchecks for themselves which should be fine. - Since we're pre 1.0, support has NOT been added to cleanup the old healthchecks. That could be done reasonably easy, BUT we'd have to keep that around forever. The hope is that the new ref format/usage will prevent this problem going forward since enough info exists in the ref to identify things fully. :fingers_crossed: - healthcheck GC is much cleaner and more robust thanks to ^ - overall the healthcheck management code is a bit easier to follow and more robust now. --- octodns/provider/route53.py | 71 ++++++++++++++++---------- tests/test_octodns_provider_route53.py | 33 +++++++++++- 2 files changed, 74 insertions(+), 30 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 4d1b2e9..6f9cb8e 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -156,7 +156,7 @@ class Route53Provider(BaseProvider): # This should be bumped when there are underlying changes made to the # health check config. - HEALTH_CHECK_VERSION = '0000' + HEALTH_CHECK_VERSION = '0001' def __init__(self, id, access_key_id, secret_access_key, max_changes=1000, *args, **kwargs): @@ -420,6 +420,13 @@ class Route53Provider(BaseProvider): # We've got a cached version use it return self._health_checks + def _health_check_equivilent(self, host, path, health_check, + first_value=None): + config = health_check['HealthCheckConfig'] + return host == config['FullyQualifiedDomainName'] and \ + path == config['ResourcePath'] and \ + (first_value is None or first_value == config['IPAddress']) + def _get_health_check_id(self, record, ident, geo, create): # fqdn & the first value are special, we use them to match up health # checks to their records. Route53 health checks check a single ip and @@ -431,20 +438,20 @@ class Route53Provider(BaseProvider): 'first_value=%s', fqdn, record._type, ident, first_value) - # health check host can't end with a . - host = fqdn[:-1] + healthcheck_host = record.healthcheck_host + healthcheck_path = record.healthcheck_path + # we're looking for a healthcheck with the current version & our record # type, we'll ignore anything else - expected_version_and_type = '{}:{}:'.format(self.HEALTH_CHECK_VERSION, - record._type) + expected_ref = '{}:{}:{}:'.format(self.HEALTH_CHECK_VERSION, + record._type, record.name) for id, health_check in self.health_checks.items(): - if not health_check['CallerReference'] \ - .startswith(expected_version_and_type): + if not health_check['CallerReference'].startswith(expected_ref): # not a version & type match, ignore continue - config = health_check['HealthCheckConfig'] - if host == config['FullyQualifiedDomainName'] and \ - first_value == config['IPAddress']: + if self._health_check_equivilent(healthcheck_host, + healthcheck_path, health_check, + first_value=first_value): # this is the health check we're looking for return id @@ -456,16 +463,16 @@ class Route53Provider(BaseProvider): config = { 'EnableSNI': True, 'FailureThreshold': 6, - 'FullyQualifiedDomainName': host, + 'FullyQualifiedDomainName': healthcheck_host, 'IPAddress': first_value, 'MeasureLatency': True, 'Port': 443, 'RequestInterval': 10, - 'ResourcePath': '/_dns', + 'ResourcePath': healthcheck_path, 'Type': 'HTTPS', } - ref = '{}:{}:{}'.format(self.HEALTH_CHECK_VERSION, record._type, - uuid4().hex[:16]) + ref = '{}:{}:{}:{}'.format(self.HEALTH_CHECK_VERSION, record._type, + record.name, uuid4().hex[:16]) resp = self._conn.create_health_check(CallerReference=ref, HealthCheckConfig=config) health_check = resp['HealthCheck'] @@ -473,11 +480,14 @@ class Route53Provider(BaseProvider): # store the new health check so that we'll be able to find it in the # future self._health_checks[id] = health_check - self.log.info('_get_health_check_id: created id=%s, host=%s, ' - 'first_value=%s', id, host, first_value) + self.log.info('_get_health_check_id: created id=%s, host=%s, path=%s' + 'first_value=%s', id, healthcheck_host, healthcheck_path, + first_value) return id def _gc_health_checks(self, record, new): + if record._type not in ('A', 'AAAA'): + return self.log.debug('_gc_health_checks: record=%s', record) # Find the health checks we're using for the new route53 records in_use = set() @@ -488,14 +498,12 @@ class Route53Provider(BaseProvider): # Now we need to run through ALL the health checks looking for those # that apply to this record, deleting any that do and are no longer in # use - host = record.fqdn[:-1] + expected_re = re.compile(r'^\d\d\d\d:{}:{}:' + .format(record._type, record.name)) for id, health_check in self.health_checks.items(): - config = health_check['HealthCheckConfig'] - _type = health_check['CallerReference'].split(':', 2)[1] - # if host and the pulled out type match it applies - if host == config['FullyQualifiedDomainName'] and \ - _type == record._type and id not in in_use: - # this is a health check for our fqdn & type but not one we're + ref = health_check['CallerReference'] + if expected_re.match(ref) and id not in in_use: + # this is a health check for this record, but not one we're # planning to use going forward self.log.info('_gc_health_checks: deleting id=%s', id) self._conn.delete_health_check(HealthCheckId=id) @@ -598,7 +606,11 @@ class Route53Provider(BaseProvider): # b/c of a health check version bump self.log.debug('_extra_changes: inspecting=%s, %s', record.fqdn, record._type) + + healthcheck_host = record.healthcheck_host + healthcheck_path = record.healthcheck_path fqdn = record.fqdn + # loop through all the r53 rrsets for rrset in self._load_records(zone_id): if fqdn != rrset['Name'] or record._type != rrset['Type']: @@ -611,12 +623,15 @@ class Route53Provider(BaseProvider): # we expect a healtcheck now try: health_check_id = rrset['HealthCheckId'] - caller_ref = \ - self.health_checks[health_check_id]['CallerReference'] + health_check = self.health_checks[health_check_id] + caller_ref = health_check['CallerReference'] if caller_ref.startswith(self.HEALTH_CHECK_VERSION): - # it has the right health check - continue - except KeyError: + if self._health_check_equivilent(healthcheck_host, + healthcheck_path, + health_check): + # it has the right health check + continue + except (IndexError, KeyError): # no health check id or one that isn't the right version pass # no good, doesn't have the right health check, needs an update diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index b05c0fc..5f063c7 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -81,7 +81,7 @@ class TestRoute53Provider(TestCase): record = Record.new(expected, name, data) expected.add_record(record) - caller_ref = '{}:A:1324'.format(Route53Provider.HEALTH_CHECK_VERSION) + caller_ref = '{}:A::1324'.format(Route53Provider.HEALTH_CHECK_VERSION) health_checks = [{ 'Id': '42', 'CallerReference': caller_ref, @@ -89,6 +89,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }, { @@ -98,6 +99,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '5.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 42, }, { @@ -107,6 +109,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '5.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }, { @@ -116,6 +119,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '7.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }, { @@ -126,6 +130,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '7.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }] @@ -639,6 +644,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }, { @@ -648,6 +654,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '9.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }] @@ -667,6 +674,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '8.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }] @@ -712,6 +720,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }, { @@ -721,6 +730,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }] @@ -736,6 +746,7 @@ class TestRoute53Provider(TestCase): 'FailureThreshold': 6, 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', 'MeasureLatency': True, 'Port': 443, 'RequestInterval': 10, @@ -995,6 +1006,7 @@ class TestRoute53Provider(TestCase): 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '2.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }], @@ -1093,8 +1105,9 @@ class TestRoute53Provider(TestCase): 'CallerReference': self.caller_ref, 'HealthCheckConfig': { 'Type': 'HTTPS', - 'FullyQualifiedDomainName': 'unit.tests', + 'FullyQualifiedDomainName': 'a.unit.tests', 'IPAddress': '2.2.3.4', + 'ResourcePath': '/_dns', }, 'HealthCheckVersion': 2, }], @@ -1106,6 +1119,22 @@ class TestRoute53Provider(TestCase): self.assertEquals(0, len(extra)) stubber.assert_no_pending_responses() + # change b/c of healthcheck path + record._octodns['healthcheck'] = { + 'path': '/_ready' + } + extra = provider._extra_changes(existing, []) + self.assertEquals(1, len(extra)) + stubber.assert_no_pending_responses() + + # change b/c of healthcheck host + record._octodns['healthcheck'] = { + 'host': 'foo.bar.io' + } + extra = provider._extra_changes(existing, []) + self.assertEquals(1, len(extra)) + stubber.assert_no_pending_responses() + def test_route_53_record(self): # Just make sure it doesn't blow up _Route53Record('foo.unit.tests.', 'A', 30).__repr__() From 0c1a8fe9642f4a4ee5f9e2772077cfe11cca0274 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 15 Jun 2017 10:12:17 -0700 Subject: [PATCH 03/23] Add custom healtcheck host & path to Dyn geo records --- octodns/provider/dyn.py | 11 ++++++----- tests/test_octodns_provider_dyn.py | 25 +++++++++++++++++++++---- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index ac0e21b..61b56bc 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -447,19 +447,20 @@ class DynProvider(BaseProvider): _kwargs_for_TXT = _kwargs_for_SPF - def _traffic_director_monitor(self, fqdn): + def _traffic_director_monitor(self, record): if self._traffic_director_monitors is None: self._traffic_director_monitors = \ {m.label: m for m in get_all_dsf_monitors()} + fqdn = record.fqdn try: return self._traffic_director_monitors[fqdn] except KeyError: monitor = DSFMonitor(fqdn, protocol='HTTPS', response_count=2, probe_interval=60, retries=2, port=443, - active='Y', host=fqdn[:-1], timeout=10, - header='User-Agent: Dyn Monitor', - path='/_dns') + active='Y', host=record.healthcheck_host, + timeout=10, header='User-Agent: Dyn Monitor', + path=record.healthcheck_path) self._traffic_director_monitors[fqdn] = monitor return monitor @@ -533,7 +534,7 @@ class DynProvider(BaseProvider): } ruleset.add_response_pool(pool.response_pool_id) - monitor_id = self._traffic_director_monitor(new.fqdn).dsf_monitor_id + monitor_id = self._traffic_director_monitor(new).dsf_monitor_id # Geos ordered least to most specific so that parents will always be # created before their children (and thus can be referenced geos = sorted(new.geo.items(), key=lambda d: d[0]) diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index 307e640..f48e4bf 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/tests/test_octodns_provider_dyn.py @@ -637,6 +637,7 @@ class TestDynProviderGeo(TestCase): provider = DynProvider('test', 'cust', 'user', 'pass', True) # short-circuit session checking provider._dyn_sess = True + existing = Zone('unit.tests.', []) # no monitors, will try and create geo_monitor_id = '42x' @@ -669,7 +670,18 @@ class TestDynProviderGeo(TestCase): }] # ask for a monitor that doesn't exist - monitor = provider._traffic_director_monitor('geo.unit.tests.') + record = Record.new(existing, 'geo', { + 'ttl': 60, + 'type': 'A', + 'value': '1.2.3.4', + 'octodns': { + 'healthcheck': { + 'host': 'foo.bar', + 'path': '/_ready' + } + } + }) + monitor = provider._traffic_director_monitor(record) self.assertEquals(geo_monitor_id, monitor.dsf_monitor_id) # should see a request for the list and a create mock.assert_has_calls([ @@ -682,8 +694,8 @@ class TestDynProviderGeo(TestCase): 'probe_interval': 60, 'active': 'Y', 'options': { - 'path': '/_dns', - 'host': 'geo.unit.tests', + 'path': '/_ready', + 'host': 'foo.bar', 'header': 'User-Agent: Dyn Monitor', 'port': 443, 'timeout': 10 @@ -698,8 +710,13 @@ class TestDynProviderGeo(TestCase): provider._traffic_director_monitors) # now ask for a monitor that does exist + record = Record.new(existing, '', { + 'ttl': 60, + 'type': 'A', + 'value': '1.2.3.4' + }) mock.reset_mock() - monitor = provider._traffic_director_monitor('unit.tests.') + monitor = provider._traffic_director_monitor(record) self.assertEquals(self.monitor_id, monitor.dsf_monitor_id) # should have resulted in no calls b/c exists & we've cached the list mock.assert_not_called() From d0b8b25cdd4c78474dc56cadbd1c27cadcb60ab0 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 20 Jun 2017 11:44:13 -0700 Subject: [PATCH 04/23] Existing and desired to _extra_changes, desired used by Route53 to get configed Fixes an issue where we'd be looking for custom healthcheck config on the existing record object (from the provider) which would never have a custom setup. Instead looking at desired lets us find what's actually configured to be the case --- octodns/provider/base.py | 4 +-- octodns/provider/powerdns.py | 2 +- octodns/provider/route53.py | 14 ++++---- tests/test_octodns_provider_base.py | 2 +- tests/test_octodns_provider_route53.py | 44 +++++++++++++------------- 5 files changed, 33 insertions(+), 33 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index 385fe36..e73c01d 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -92,7 +92,7 @@ class BaseProvider(BaseSource): ''' return True - def _extra_changes(self, existing, changes): + def _extra_changes(self, existing, desired, changes): ''' An opportunity for providers to add extra changes to the plan that are necessary to update ancilary record data or configure the zone. E.g. @@ -117,7 +117,7 @@ class BaseProvider(BaseSource): self.log.info('plan: filtered out %s changes', before - after) # allow the provider to add extra changes it needs - extra = self._extra_changes(existing, changes) + extra = self._extra_changes(existing, desired, changes) if extra: self.log.info('plan: extra changes\n %s', '\n ' .join([str(c) for c in extra])) diff --git a/octodns/provider/powerdns.py b/octodns/provider/powerdns.py index 4ff2568..8208833 100644 --- a/octodns/provider/powerdns.py +++ b/octodns/provider/powerdns.py @@ -259,7 +259,7 @@ class PowerDnsBaseProvider(BaseProvider): def _get_nameserver_record(self, existing): return None - def _extra_changes(self, existing, _): + def _extra_changes(self, existing, _, __): self.log.debug('_extra_changes: zone=%s', existing.name) ns = self._get_nameserver_record(existing) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 6f9cb8e..ee9d05d 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -583,18 +583,18 @@ class Route53Provider(BaseProvider): self._gc_health_checks(change.existing, []) return self._gen_mods('DELETE', existing_records) - def _extra_changes(self, existing, changes): - self.log.debug('_extra_changes: existing=%s', existing.name) - zone_id = self._get_zone_id(existing.name) + def _extra_changes(self, existing, desired, changes): + self.log.debug('_extra_changes: desired=%s', desired.name) + zone_id = self._get_zone_id(desired.name) if not zone_id: # zone doesn't exist so no extras to worry about return [] # we'll skip extra checking for anything we're already going to change changed = set([c.record for c in changes]) # ok, now it's time for the reason we're here, we need to go over all - # the existing records + # the desired records extra = [] - for record in existing.records: + for record in desired.records: if record in changed: # already have a change for it, skipping continue @@ -635,8 +635,8 @@ class Route53Provider(BaseProvider): # no health check id or one that isn't the right version pass # no good, doesn't have the right health check, needs an update - self.log.debug('_extra_changes: health-check caused ' - 'update') + self.log.info('_extra_changes: health-check caused ' + 'update') extra.append(Update(record, record)) # We don't need to process this record any longer break diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index c7836c8..d4e153b 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -29,7 +29,7 @@ class HelperProvider(BaseProvider): return not self.include_change_callback or \ self.include_change_callback(change) - def _extra_changes(self, existing, changes): + def _extra_changes(self, existing, desired, changes): return self.__extra_changes def _apply(self, plan): diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 5f063c7..7ec0d34 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -882,26 +882,26 @@ class TestRoute53Provider(TestCase): stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {}) # empty is empty - existing = Zone('unit.tests.', []) - extra = provider._extra_changes(existing, []) + desired = Zone('unit.tests.', []) + extra = provider._extra_changes(None, desired, []) self.assertEquals([], extra) stubber.assert_no_pending_responses() # single record w/o geo is empty - existing = Zone('unit.tests.', []) - record = Record.new(existing, 'a', { + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { 'ttl': 30, 'type': 'A', 'value': '1.2.3.4', }) - existing.add_record(record) - extra = provider._extra_changes(existing, []) + desired.add_record(record) + extra = provider._extra_changes(None, desired, []) self.assertEquals([], extra) stubber.assert_no_pending_responses() # short-circuit for unknown zone other = Zone('other.tests.', []) - extra = provider._extra_changes(other, []) + extra = provider._extra_changes(None, other, []) self.assertEquals([], extra) stubber.assert_no_pending_responses() @@ -921,8 +921,8 @@ class TestRoute53Provider(TestCase): stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {}) # record with geo and no health check returns change - existing = Zone('unit.tests.', []) - record = Record.new(existing, 'a', { + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { 'ttl': 30, 'type': 'A', 'value': '1.2.3.4', @@ -930,7 +930,7 @@ class TestRoute53Provider(TestCase): 'NA': ['2.2.3.4'], } }) - existing.add_record(record) + desired.add_record(record) list_resource_record_sets_resp = { 'ResourceRecordSets': [{ 'Name': 'a.unit.tests.', @@ -949,7 +949,7 @@ class TestRoute53Provider(TestCase): stubber.add_response('list_resource_record_sets', list_resource_record_sets_resp, {'HostedZoneId': 'z42'}) - extra = provider._extra_changes(existing, []) + extra = provider._extra_changes(None, desired, []) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() @@ -969,8 +969,8 @@ class TestRoute53Provider(TestCase): stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {}) # record with geo and no health check returns change - existing = Zone('unit.tests.', []) - record = Record.new(existing, 'a', { + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { 'ttl': 30, 'type': 'A', 'value': '1.2.3.4', @@ -978,7 +978,7 @@ class TestRoute53Provider(TestCase): 'NA': ['2.2.3.4'], } }) - existing.add_record(record) + desired.add_record(record) list_resource_record_sets_resp = { 'ResourceRecordSets': [{ 'Name': 'a.unit.tests.', @@ -1014,12 +1014,12 @@ class TestRoute53Provider(TestCase): 'MaxItems': '100', 'Marker': '', }) - extra = provider._extra_changes(existing, []) + extra = provider._extra_changes(None, desired, []) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() for change in (Create(record), Update(record, record), Delete(record)): - extra = provider._extra_changes(existing, [change]) + extra = provider._extra_changes(None, desired, [change]) self.assertEquals(0, len(extra)) stubber.assert_no_pending_responses() @@ -1039,8 +1039,8 @@ class TestRoute53Provider(TestCase): stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {}) # record with geo and no health check returns change - existing = Zone('unit.tests.', []) - record = Record.new(existing, 'a', { + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'a', { 'ttl': 30, 'type': 'A', 'value': '1.2.3.4', @@ -1048,7 +1048,7 @@ class TestRoute53Provider(TestCase): 'NA': ['2.2.3.4'], } }) - existing.add_record(record) + desired.add_record(record) list_resource_record_sets_resp = { 'ResourceRecordSets': [{ # other name @@ -1115,7 +1115,7 @@ class TestRoute53Provider(TestCase): 'MaxItems': '100', 'Marker': '', }) - extra = provider._extra_changes(existing, []) + extra = provider._extra_changes(None, desired, []) self.assertEquals(0, len(extra)) stubber.assert_no_pending_responses() @@ -1123,7 +1123,7 @@ class TestRoute53Provider(TestCase): record._octodns['healthcheck'] = { 'path': '/_ready' } - extra = provider._extra_changes(existing, []) + extra = provider._extra_changes(None, desired, []) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() @@ -1131,7 +1131,7 @@ class TestRoute53Provider(TestCase): record._octodns['healthcheck'] = { 'host': 'foo.bar.io' } - extra = provider._extra_changes(existing, []) + extra = provider._extra_changes(None, desired, []) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() From 9506c682cc0b49483ba21dc4b01a20783350db1b Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 20 Jun 2017 11:46:17 -0700 Subject: [PATCH 05/23] Go head and support cleaning up v0000 Route53 health check versions --- octodns/provider/route53.py | 10 ++++ tests/test_octodns_provider_route53.py | 80 +++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index ee9d05d..85d35e6 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -500,6 +500,10 @@ class Route53Provider(BaseProvider): # use expected_re = re.compile(r'^\d\d\d\d:{}:{}:' .format(record._type, record.name)) + # Until the v1.0 release we'll clean out the previous version of + # Route53 health checks as best as we can. + expected_legacy_host = record.fqdn[:-1] + expected_legacy = '0000:{}:'.format(record._type) for id, health_check in self.health_checks.items(): ref = health_check['CallerReference'] if expected_re.match(ref) and id not in in_use: @@ -507,6 +511,12 @@ class Route53Provider(BaseProvider): # planning to use going forward self.log.info('_gc_health_checks: deleting id=%s', id) self._conn.delete_health_check(HealthCheckId=id) + elif ref.startswith(expected_legacy): + config = health_check['HealthCheckConfig'] + if expected_legacy_host == config['FullyQualifiedDomainName']: + self.log.info('_gc_health_checks: deleting legacy id=%s', + id) + self._conn.delete_health_check(HealthCheckId=id) def _gen_records(self, record, creating=False): ''' diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 7ec0d34..5d34f50 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -18,6 +18,12 @@ from octodns.zone import Zone from helpers import GeoProvider +class DummyR53Record(object): + + def __init__(self, health_check_id): + self.health_check_id = health_check_id + + class TestOctalReplace(TestCase): def test_basic(self): @@ -807,18 +813,13 @@ class TestRoute53Provider(TestCase): } }) - class DummyRecord(object): - - def __init__(self, health_check_id): - self.health_check_id = health_check_id - # gc no longer in_use records (directly) stubber.add_response('delete_health_check', {}, { 'HealthCheckId': '44', }) provider._gc_health_checks(record, [ - DummyRecord('42'), - DummyRecord('43'), + DummyR53Record('42'), + DummyR53Record('43'), ]) stubber.assert_no_pending_responses() @@ -866,6 +867,71 @@ class TestRoute53Provider(TestCase): provider._gc_health_checks(record, []) stubber.assert_no_pending_responses() + def test_legacy_health_check_gc(self): + provider, stubber = self._get_stubbed_provider() + + old_caller_ref = '0000:A:3333' + health_checks = [{ + 'Id': '42', + 'CallerReference': self.caller_ref, + 'HealthCheckConfig': { + 'Type': 'HTTPS', + 'FullyQualifiedDomainName': 'unit.tests', + 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', + }, + 'HealthCheckVersion': 2, + }, { + 'Id': '43', + 'CallerReference': old_caller_ref, + 'HealthCheckConfig': { + 'Type': 'HTTPS', + 'FullyQualifiedDomainName': 'unit.tests', + 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', + }, + 'HealthCheckVersion': 2, + }, { + 'Id': '44', + 'CallerReference': old_caller_ref, + 'HealthCheckConfig': { + 'Type': 'HTTPS', + 'FullyQualifiedDomainName': 'other.unit.tests', + 'IPAddress': '4.2.3.4', + 'ResourcePath': '/_dns', + }, + 'HealthCheckVersion': 2, + }] + + stubber.add_response('list_health_checks', { + 'HealthChecks': health_checks, + 'IsTruncated': False, + 'MaxItems': '100', + 'Marker': '', + }) + + # No changes to the record itself + record = Record.new(self.expected, '', { + 'ttl': 61, + 'type': 'A', + 'values': ['2.2.3.4', '3.2.3.4'], + 'geo': { + 'AF': ['4.2.3.4'], + 'NA-US': ['5.2.3.4', '6.2.3.4'], + 'NA-US-CA': ['7.2.3.4'] + } + }) + + # Expect to delete the legacy hc for our record, but not touch the new + # one or the other legacy record + stubber.add_response('delete_health_check', {}, { + 'HealthCheckId': '43', + }) + + provider._gc_health_checks(record, [ + DummyR53Record('42'), + ]) + def test_no_extra_changes(self): provider, stubber = self._get_stubbed_provider() From 0e8bc9a3d70f507a2409b1bca732f15711aee8aa Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 20 Jun 2017 14:43:27 -0700 Subject: [PATCH 06/23] Dyn monitor config support, includes ability to update Also fixes some mocking data to match what the Dyn client libs are expecting. --- octodns/provider/dyn.py | 33 +++++++++-- tests/test_octodns_provider_dyn.py | 92 ++++++++++++++++++++++++------ 2 files changed, 104 insertions(+), 21 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 61b56bc..1b454ba 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -133,6 +133,10 @@ class DynProvider(BaseProvider): 'AN': 17, # Continental Antartica } + MONITOR_HEADER = 'User-Agent: Dyn Monitor' + MONITOR_PORT = 443 + MONITOR_TIMEOUT = 10 + _sess_create_lock = Lock() def __init__(self, id, customer, username, password, @@ -454,12 +458,33 @@ class DynProvider(BaseProvider): fqdn = record.fqdn try: - return self._traffic_director_monitors[fqdn] + 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, + } + }) + return monitor except KeyError: + self.log.info('_traffic_director_monitor: creating monitor ' + 'for %s:%s', record.fqdn, record._type) monitor = DSFMonitor(fqdn, protocol='HTTPS', response_count=2, - probe_interval=60, retries=2, port=443, - active='Y', host=record.healthcheck_host, - timeout=10, header='User-Agent: Dyn Monitor', + probe_interval=60, retries=2, + port=self.MONITOR_PORT, active='Y', + host=record.healthcheck_host, + timeout=self.MONITOR_TIMEOUT, + header=self.MONITOR_HEADER, path=record.healthcheck_path) self._traffic_director_monitors[fqdn] = monitor return monitor diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index f48e4bf..acd248f 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/tests/test_octodns_provider_dyn.py @@ -527,21 +527,22 @@ class TestDynProviderGeo(TestCase): monitors_response = { 'data': [{ 'active': 'Y', + 'agent_scheme': 'geo', 'dsf_monitor_id': monitor_id, 'endpoints': [], 'label': 'unit.tests.', - 'notifier': '', - 'options': { - 'expected': '', - 'header': 'User-Agent: Dyn Monitor', - 'host': 'unit.tests', - 'path': '/_dns', - 'port': '443', - 'timeout': '10'}, + 'notifier': [], + 'expected': '', + 'header': 'User-Agent: Dyn Monitor', + 'host': 'unit.tests', + 'path': '/_dns', + 'port': '443', + 'timeout': '10', 'probe_interval': '60', 'protocol': 'HTTPS', 'response_count': '2', - 'retries': '2' + 'retries': '2', + 'services': ['12311'] }], 'job_id': 3376281406, 'msgs': [{ @@ -648,14 +649,12 @@ class TestDynProviderGeo(TestCase): 'endpoints': [], 'label': 'geo.unit.tests.', 'notifier': '', - 'options': { - 'expected': '', - 'header': 'User-Agent: Dyn Monitor', - 'host': 'geo.unit.tests.', - 'path': '/_dns', - 'port': '443', - 'timeout': '10' - }, + 'expected': '', + 'header': 'User-Agent: Dyn Monitor', + 'host': 'geo.unit.tests.', + 'path': '/_dns', + 'port': '443', + 'timeout': '10', 'probe_interval': '60', 'protocol': 'HTTPS', 'response_count': '2', @@ -721,6 +720,65 @@ class TestDynProviderGeo(TestCase): # should have resulted in no calls b/c exists & we've cached the list mock.assert_not_called() + # and finally for a monitor that exists, but with a differing config + record = Record.new(existing, '', { + 'octodns': { + 'healthcheck': { + 'host': 'bleep.bloop', + 'path': '/_nope' + } + }, + 'ttl': 60, + 'type': 'A', + 'value': '1.2.3.4' + }) + mock.reset_mock() + mock.side_effect = [{ + 'data': { + 'active': 'Y', + 'dsf_monitor_id': self.monitor_id, + 'endpoints': [], + 'label': 'unit.tests.', + 'notifier': '', + 'expected': '', + 'header': 'User-Agent: Dyn Monitor', + 'host': 'bleep.bloop', + 'path': '/_nope', + 'port': '443', + 'timeout': '10', + 'probe_interval': '60', + 'protocol': 'HTTPS', + 'response_count': '2', + 'retries': '2' + }, + 'job_id': 3376259461, + 'msgs': [{'ERR_CD': None, + 'INFO': 'add: Here is the new monitor', + 'LVL': 'INFO', + 'SOURCE': 'BLL'}], + 'status': 'success' + }] + monitor = provider._traffic_director_monitor(record) + self.assertEquals(self.monitor_id, monitor.dsf_monitor_id) + # should have resulted an update + mock.assert_has_calls([ + call('/DSFMonitor/42a/', 'PUT', { + 'options': { + 'path': '/_nope', + 'host': 'bleep.bloop', + 'header': 'User-Agent: Dyn Monitor', + 'port': 443, + 'timeout': 10 + } + }) + ]) + # cached monitor should have been updated + 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) + @patch('dyn.core.SessionEngine.execute') def test_populate_traffic_directors_empty(self, mock): provider = DynProvider('test', 'cust', 'user', 'pass', From 72eff706daafdaf28d534db3f04792660279f914 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 21 Jun 2017 12:40:31 -0700 Subject: [PATCH 07/23] 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 | 106 ++++++++++++++++++++---- octodns/provider/route53.py | 2 +- tests/test_octodns_provider_dyn.py | 128 ++++++++++++++++++++++++++++- 3 files changed, 215 insertions(+), 21 deletions(-) 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) From b6cd08e659c441cbe4eb8f7ab2d38922e56e2ba5 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 21 Jun 2017 14:34:21 -0700 Subject: [PATCH 08/23] Update DSFMonitor labels to include type Would have prevented both A and AAAA exisiting on the same node. --- octodns/provider/dyn.py | 34 +++++++--- octodns/provider/route53.py | 4 +- tests/test_octodns_provider_dyn.py | 83 ++++++++++++++++++++++--- tests/test_octodns_provider_powerdns.py | 1 - 4 files changed, 99 insertions(+), 23 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 8f19e0f..714f2eb 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -434,19 +434,20 @@ class DynProvider(BaseProvider): if record in changed or not getattr(record, 'geo', False): # Already changed, or no geo, no need to check it continue + label = '{}:{}'.format(record.fqdn, record._type) try: - monitor = self.traffic_director_monitors[record.fqdn] + monitor = self.traffic_director_monitors[label] except KeyError: - self.log.info('_extra_changes: health-check missing for %s:%s', - record.fqdn, record._type) + self.log.info('_extra_changes: health-check missing for %s', + label) 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) + self.log.info('_extra_changes: health-check mis-match for %s', + label) extra.append(Update(record, record)) return extra @@ -532,6 +533,7 @@ class DynProvider(BaseProvider): @property def traffic_director_monitors(self): if self._traffic_director_monitors is None: + self.log.debug('traffic_director_monitors: loading') self._traffic_director_monitors = \ {m.label: m for m in get_all_dsf_monitors()} @@ -539,26 +541,38 @@ class DynProvider(BaseProvider): def _traffic_director_monitor(self, record): fqdn = record.fqdn + label = '{}:{}'.format(fqdn, record._type) try: - monitor = self.traffic_director_monitors[fqdn] + try: + monitor = self.traffic_director_monitors[label] + except KeyError: + # UNTIL 1.0 We don't have one for the new label format, see if + # we still have one for the old and update it + monitor = self.traffic_director_monitors[fqdn] + self.log.info('_traffic_director_monitor: upgrading label ' + 'to %s', label) + monitor.label = label + self.traffic_director_monitors[label] = \ + self.traffic_director_monitors[fqdn] + del 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) + 'for %s', label) monitor.update(record.healthcheck_host, record.healthcheck_path) return monitor except KeyError: self.log.info('_traffic_director_monitor: creating monitor ' - 'for %s:%s', record.fqdn, record._type) - monitor = DSFMonitor(fqdn, protocol='HTTPS', response_count=2, + 'for %s', label) + monitor = DSFMonitor(label, protocol='HTTPS', response_count=2, probe_interval=60, retries=2, port=self.MONITOR_PORT, active='Y', host=record.healthcheck_host, timeout=self.MONITOR_TIMEOUT, header=self.MONITOR_HEADER, path=record.healthcheck_path) - self._traffic_director_monitors[fqdn] = monitor + self._traffic_director_monitors[label] = monitor return monitor def _find_or_create_pool(self, td, pools, label, _type, values, diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 675c0a8..35a1db8 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -500,8 +500,8 @@ class Route53Provider(BaseProvider): # use expected_re = re.compile(r'^\d\d\d\d:{}:{}:' .format(record._type, record.name)) - # Until the v1.0 release we'll clean out the previous version of - # Route53 health checks as best as we can. + # UNITL 1.0: we'll clean out the previous version of Route53 health + # checks as best as we can. expected_legacy_host = record.fqdn[:-1] expected_legacy = '0000:{}:'.format(record._type) for id, health_check in self.health_checks.items(): diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index 1f6f805..4b9d8b7 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/tests/test_octodns_provider_dyn.py @@ -530,7 +530,7 @@ class TestDynProviderGeo(TestCase): 'agent_scheme': 'geo', 'dsf_monitor_id': monitor_id, 'endpoints': [], - 'label': 'unit.tests.', + 'label': 'unit.tests.:A', 'notifier': [], 'expected': '', 'header': 'User-Agent: Dyn Monitor', @@ -543,6 +543,24 @@ class TestDynProviderGeo(TestCase): 'response_count': '2', 'retries': '2', 'services': ['12311'] + }, { + 'active': 'Y', + 'agent_scheme': 'geo', + 'dsf_monitor_id': 'b52', + 'endpoints': [], + 'label': 'old-label.unit.tests.', + 'notifier': [], + 'expected': '', + 'header': 'User-Agent: Dyn Monitor', + 'host': 'old-label.unit.tests', + 'path': '/_dns', + 'port': '443', + 'timeout': '10', + 'probe_interval': '60', + 'protocol': 'HTTPS', + 'response_count': '2', + 'retries': '2', + 'services': ['12312'] }], 'job_id': 3376281406, 'msgs': [{ @@ -647,7 +665,7 @@ class TestDynProviderGeo(TestCase): 'active': 'Y', 'dsf_monitor_id': geo_monitor_id, 'endpoints': [], - 'label': 'geo.unit.tests.', + 'label': 'geo.unit.tests.:A', 'notifier': '', 'expected': '', 'header': 'User-Agent: Dyn Monitor', @@ -689,7 +707,7 @@ class TestDynProviderGeo(TestCase): 'retries': 2, 'protocol': 'HTTPS', 'response_count': 2, - 'label': 'geo.unit.tests.', + 'label': 'geo.unit.tests.:A', 'probe_interval': 60, 'active': 'Y', 'options': { @@ -702,10 +720,10 @@ class TestDynProviderGeo(TestCase): }) ]) # created monitor is now cached - self.assertTrue('geo.unit.tests.' in + self.assertTrue('geo.unit.tests.:A' in provider._traffic_director_monitors) # pre-existing one is there too - self.assertTrue('unit.tests.' in + self.assertTrue('unit.tests.:A' in provider._traffic_director_monitors) # now ask for a monitor that does exist @@ -738,7 +756,7 @@ class TestDynProviderGeo(TestCase): 'active': 'Y', 'dsf_monitor_id': self.monitor_id, 'endpoints': [], - 'label': 'unit.tests.', + 'label': 'unit.tests.:A', 'notifier': '', 'expected': '', 'header': 'User-Agent: Dyn Monitor', @@ -773,14 +791,56 @@ class TestDynProviderGeo(TestCase): }) ]) # cached monitor should have been updated - self.assertTrue('unit.tests.' in + self.assertTrue('unit.tests.:A' in provider._traffic_director_monitors) - monitor = provider._traffic_director_monitors['unit.tests.'] - from pprint import pprint - pprint(monitor.__dict__) + monitor = provider._traffic_director_monitors['unit.tests.:A'] self.assertEquals('bleep.bloop', monitor.host) self.assertEquals('/_nope', monitor.path) + # test upgrading an old label + record = Record.new(existing, 'old-label', { + 'ttl': 60, + 'type': 'A', + 'value': '1.2.3.4' + }) + mock.reset_mock() + mock.side_effect = [{ + 'data': { + 'active': 'Y', + 'dsf_monitor_id': self.monitor_id, + 'endpoints': [], + 'label': 'old-label.unit.tests.:A', + 'notifier': '', + 'expected': '', + 'header': 'User-Agent: Dyn Monitor', + 'host': 'old-label.unit.tests', + 'path': '/_dns', + 'port': '443', + 'timeout': '10', + 'probe_interval': '60', + 'protocol': 'HTTPS', + 'response_count': '2', + 'retries': '2' + }, + 'job_id': 3376259461, + 'msgs': [{'ERR_CD': None, + 'INFO': 'add: Here is the new monitor', + 'LVL': 'INFO', + 'SOURCE': 'BLL'}], + 'status': 'success' + }] + monitor = provider._traffic_director_monitor(record) + self.assertEquals(self.monitor_id, monitor.dsf_monitor_id) + # should have resulted an update + mock.assert_has_calls([ + call('/DSFMonitor/b52/', 'PUT', { + 'label': 'old-label.unit.tests.:A' + }) + ]) + # cached monitor should have been updated + self.assertTrue('old-label.unit.tests.:A' in + provider._traffic_director_monitors) + @patch('dyn.core.SessionEngine.execute') def test_extra_changes(self, mock): provider = DynProvider('test', 'cust', 'user', 'pass', True) @@ -818,6 +878,9 @@ class TestDynProviderGeo(TestCase): extra = provider._extra_changes(None, desired, []) self.assertEquals(0, len(extra)) + # monitors should have been fetched now + mock.assert_called_once() + # diff in healthcheck, gets extra desired = Zone('unit.tests.', []) record = Record.new(desired, '', { diff --git a/tests/test_octodns_provider_powerdns.py b/tests/test_octodns_provider_powerdns.py index 01e7d83..c7db997 100644 --- a/tests/test_octodns_provider_powerdns.py +++ b/tests/test_octodns_provider_powerdns.py @@ -52,7 +52,6 @@ class TestPowerDnsProvider(TestCase): with self.assertRaises(Exception) as ctx: zone = Zone('unit.tests.', []) provider.populate(zone) - print(ctx.exception.message) self.assertTrue('unauthorized' in ctx.exception.message) # General error From ced6350cc99aba723b5207dd0845e08301d159d3 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Thu, 22 Mar 2018 08:57:32 +0100 Subject: [PATCH 09/23] Fix linting issues --- octodns/provider/dyn.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 51be18a..374f6a0 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -33,6 +33,7 @@ from .base import BaseProvider ############################################################################### def _monitor_host_get(self): return self._host or self._options['host'] + DSFMonitor.host = property(_monitor_host_get) @@ -40,11 +41,13 @@ 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) @@ -52,6 +55,7 @@ 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) @@ -68,6 +72,7 @@ def _monitor_update(self, host, path): 'header': DynProvider.MONITOR_HEADER, } }) + DSFMonitor.update = _monitor_update ############################################################################### From 9cde37993ba78f3b63d39788d10079b412b1c790 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Thu, 22 Mar 2018 09:01:04 +0100 Subject: [PATCH 10/23] Really fix lint issue, should read better --- octodns/provider/dyn.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 374f6a0..2e3f619 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -34,6 +34,7 @@ from .base import BaseProvider def _monitor_host_get(self): return self._host or self._options['host'] + DSFMonitor.host = property(_monitor_host_get) @@ -42,12 +43,14 @@ def _monitor_host_set(self, value): 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) @@ -56,6 +59,7 @@ def _monitor_path_set(self, value): self._options = {} self._path = self._options['path'] = value + DSFMonitor.path = DSFMonitor.path.setter(_monitor_path_set) @@ -73,6 +77,7 @@ def _monitor_update(self, host, path): } }) + DSFMonitor.update = _monitor_update ############################################################################### From a180f246a2cf6879df41ded5209c09ec7f9f517c Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Thu, 22 Mar 2018 09:03:32 +0100 Subject: [PATCH 11/23] Use proper object --- octodns/record.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/octodns/record.py b/octodns/record.py index 29e7a3b..e904c89 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -127,9 +127,9 @@ class Record(object): self.ttl = int(data['ttl']) self._octodns = data.get('octodns', {}) - self.ignored = octodns.get('ignored', False) - self.excluded = octodns.get('excluded', []) - self.included = octodns.get('included', []) + self.ignored = self._octodns.get('ignored', False) + self.excluded = self._octodns.get('excluded', []) + self.included = self._octodns.get('included', []) def _data(self): return {'ttl': self.ttl} From 182c9538751e0595cdd48ff32f6c22184497f7b2 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 31 Mar 2018 12:21:25 -0700 Subject: [PATCH 12/23] Remove extraxted Record.ignored, extract excluded and included Make the Record.octodns bits more consistent --- octodns/record.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/octodns/record.py b/octodns/record.py index e904c89..c60ecd6 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -127,9 +127,6 @@ class Record(object): self.ttl = int(data['ttl']) self._octodns = data.get('octodns', {}) - self.ignored = self._octodns.get('ignored', False) - self.excluded = self._octodns.get('excluded', []) - self.included = self._octodns.get('included', []) def _data(self): return {'ttl': self.ttl} @@ -148,6 +145,14 @@ class Record(object): def ignored(self): return self._octodns.get('ignored', False) + @property + def excluded(self): + return self._octodns.get('excluded', []) + + @property + def included(self): + return self._octodns.get('included', []) + @property def healthcheck_path(self): try: From 5372e86e1cd1b97b3ed3f7f470889f2148a4f7c5 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 31 Mar 2018 12:39:32 -0700 Subject: [PATCH 13/23] Switch to named params in _extra_changes --- octodns/provider/base.py | 3 ++- octodns/provider/dyn.py | 2 +- octodns/provider/powerdns.py | 2 +- octodns/provider/route53.py | 2 +- tests/test_octodns_provider_base.py | 2 +- tests/test_octodns_provider_dyn.py | 12 +++++++----- tests/test_octodns_provider_route53.py | 18 +++++++++--------- 7 files changed, 22 insertions(+), 19 deletions(-) diff --git a/octodns/provider/base.py b/octodns/provider/base.py index e2d7a04..ada0c7a 100644 --- a/octodns/provider/base.py +++ b/octodns/provider/base.py @@ -64,7 +64,8 @@ class BaseProvider(BaseSource): self.log.info('plan: filtered out %s changes', before - after) # allow the provider to add extra changes it needs - extra = self._extra_changes(existing, desired, changes) + extra = self._extra_changes(existing=existing, desired=desired, + changes=changes) if extra: self.log.info('plan: extra changes\n %s', '\n ' .join([unicode(c) for c in extra])) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index a379a31..e9c7bca 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -454,7 +454,7 @@ class DynProvider(BaseProvider): len(zone.records) - before, exists) return exists - def _extra_changes(self, _, desired, changes): + def _extra_changes(self, desired, changes, **kwargs): self.log.debug('_extra_changes: desired=%s', desired.name) changed = set([c.record for c in changes]) diff --git a/octodns/provider/powerdns.py b/octodns/provider/powerdns.py index a081681..02319e5 100644 --- a/octodns/provider/powerdns.py +++ b/octodns/provider/powerdns.py @@ -289,7 +289,7 @@ class PowerDnsBaseProvider(BaseProvider): def _get_nameserver_record(self, existing): return None - def _extra_changes(self, existing, _, __): + def _extra_changes(self, existing, **kwargs): self.log.debug('_extra_changes: zone=%s', existing.name) ns = self._get_nameserver_record(existing) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 2d2ad59..46256c6 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -673,7 +673,7 @@ class Route53Provider(BaseProvider): self._gc_health_checks(change.existing, []) return self._gen_mods('DELETE', existing_records) - def _extra_changes(self, existing, desired, changes): + def _extra_changes(self, desired, changes, **kwargs): self.log.debug('_extra_changes: desired=%s', desired.name) zone_id = self._get_zone_id(desired.name) if not zone_id: diff --git a/tests/test_octodns_provider_base.py b/tests/test_octodns_provider_base.py index 23cffed..d5ac5b3 100644 --- a/tests/test_octodns_provider_base.py +++ b/tests/test_octodns_provider_base.py @@ -35,7 +35,7 @@ class HelperProvider(BaseProvider): return not self.include_change_callback or \ self.include_change_callback(change) - def _extra_changes(self, existing, desired, changes): + def _extra_changes(self, **kwargs): return self.__extra_changes def _apply(self, plan): diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index 068abd4..3412e34 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/tests/test_octodns_provider_dyn.py @@ -882,7 +882,8 @@ class TestDynProviderGeo(TestCase): 'value': '1.2.3.4', }) desired.add_record(record) - extra = provider._extra_changes(None, desired, [Create(record)]) + extra = provider._extra_changes(desired=desired, + changes=[Create(record)]) self.assertEquals(0, len(extra)) # in changes, noop @@ -896,11 +897,12 @@ class TestDynProviderGeo(TestCase): 'value': '1.2.3.4', }) desired.add_record(record) - extra = provider._extra_changes(None, desired, [Create(record)]) + extra = provider._extra_changes(desired=desired, + changes=[Create(record)]) self.assertEquals(0, len(extra)) # no diff, no extra - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(0, len(extra)) # monitors should have been fetched now @@ -923,7 +925,7 @@ class TestDynProviderGeo(TestCase): 'value': '1.2.3.4', }) desired.add_record(record) - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(1, len(extra)) extra = extra[0] self.assertIsInstance(extra, Update) @@ -940,7 +942,7 @@ class TestDynProviderGeo(TestCase): 'value': '1.2.3.4', }) desired.add_record(record) - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(1, len(extra)) extra = extra[0] self.assertIsInstance(extra, Update) diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 38dc424..86a6ad4 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -970,7 +970,7 @@ class TestRoute53Provider(TestCase): # empty is empty desired = Zone('unit.tests.', []) - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals([], extra) stubber.assert_no_pending_responses() @@ -982,13 +982,13 @@ class TestRoute53Provider(TestCase): 'value': '1.2.3.4', }) desired.add_record(record) - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals([], extra) stubber.assert_no_pending_responses() # short-circuit for unknown zone other = Zone('other.tests.', []) - extra = provider._extra_changes(None, other, []) + extra = provider._extra_changes(desired=other, changes=[]) self.assertEquals([], extra) stubber.assert_no_pending_responses() @@ -1036,7 +1036,7 @@ class TestRoute53Provider(TestCase): stubber.add_response('list_resource_record_sets', list_resource_record_sets_resp, {'HostedZoneId': 'z42'}) - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() @@ -1101,12 +1101,12 @@ class TestRoute53Provider(TestCase): 'MaxItems': '100', 'Marker': '', }) - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() for change in (Create(record), Update(record, record), Delete(record)): - extra = provider._extra_changes(None, desired, [change]) + extra = provider._extra_changes(desired=desired, changes=[change]) self.assertEquals(0, len(extra)) stubber.assert_no_pending_responses() @@ -1202,7 +1202,7 @@ class TestRoute53Provider(TestCase): 'MaxItems': '100', 'Marker': '', }) - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(0, len(extra)) stubber.assert_no_pending_responses() @@ -1210,7 +1210,7 @@ class TestRoute53Provider(TestCase): record._octodns['healthcheck'] = { 'path': '/_ready' } - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() @@ -1218,7 +1218,7 @@ class TestRoute53Provider(TestCase): record._octodns['healthcheck'] = { 'host': 'foo.bar.io' } - extra = provider._extra_changes(None, desired, []) + extra = provider._extra_changes(desired=desired, changes=[]) self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() From 084f1e7c5757a4e287ec914ff5c66e62f6c00438 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 31 Mar 2018 13:05:04 -0700 Subject: [PATCH 14/23] Consolidate Dyn monitor comparison logic --- octodns/provider/dyn.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index e9c7bca..09329c0 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -142,6 +142,10 @@ class _CachingDynZone(DynZone): self.flush_cache() +def _monitor_matches(monitor, host, path): + return monitor.host != host or monitor.path != path + + class DynProvider(BaseProvider): ''' Dynect Managed DNS provider @@ -474,8 +478,8 @@ class DynProvider(BaseProvider): 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: + if _monitor_matches(monitor, record.healthcheck_host, + record.healthcheck_path): self.log.info('_extra_changes: health-check mis-match for %s', label) extra.append(Update(record, record)) @@ -592,8 +596,8 @@ class DynProvider(BaseProvider): self.traffic_director_monitors[label] = \ self.traffic_director_monitors[fqdn] del self.traffic_director_monitors[fqdn] - if monitor.host != record.healthcheck_host or \ - monitor.path != record.healthcheck_path: + if _monitor_matches(monitor, record.healthcheck_host, + record.healthcheck_path): self.log.info('_traffic_director_monitor: updating monitor ' 'for %s', label) monitor.update(record.healthcheck_host, From 9752cb0a126fbed0c3d8951aec00682a8186b301 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 31 Mar 2018 13:10:06 -0700 Subject: [PATCH 15/23] Add protocol and port to octodns.healthcheck configurables --- octodns/record.py | 20 +++++++++++++++++--- tests/test_octodns_record.py | 7 +++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/octodns/record.py b/octodns/record.py index c60ecd6..795ad71 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -153,6 +153,13 @@ class Record(object): def included(self): return self._octodns.get('included', []) + @property + def healthcheck_host(self): + try: + return self._octodns['healthcheck']['host'] + except KeyError: + return self.fqdn[:-1] + @property def healthcheck_path(self): try: @@ -161,11 +168,18 @@ class Record(object): return '/_dns' @property - def healthcheck_host(self): + def healthcheck_protocol(self): try: - return self._octodns['healthcheck']['host'] + return self._octodns['healthcheck']['protocol'] except KeyError: - return self.fqdn[:-1] + return 'https' + + @property + def healthcheck_port(self): + try: + return self._octodns['healthcheck']['port'] + except KeyError: + return 443 def changes(self, other, target): # We're assuming we have the same name and type if we're being compared diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 45d790b..e23808b 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -755,11 +755,16 @@ class TestRecord(TestCase): 'healthcheck': { 'path': '/_ready', 'host': 'bleep.bloop', + 'protocol': 'http', + 'port': 8080, } } }) self.assertEquals('/_ready', new.healthcheck_path) self.assertEquals('bleep.bloop', new.healthcheck_host) + self.assertEquals('http', new.healthcheck_protocol) + self.assertEquals(8080, new.healthcheck_port) + new = Record.new(self.zone, 'a', { 'ttl': 44, 'type': 'A', @@ -767,6 +772,8 @@ class TestRecord(TestCase): }) self.assertEquals('/_dns', new.healthcheck_path) self.assertEquals('a.unit.tests', new.healthcheck_host) + self.assertEquals('https', new.healthcheck_protocol) + self.assertEquals(443, new.healthcheck_port) def test_inored(self): new = Record.new(self.zone, 'txt', { From 849a97f1619c21c70ace47cba7a537063b77f3f5 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 31 Mar 2018 13:22:28 -0700 Subject: [PATCH 16/23] Add healthcheck protocol validation, HTTP or HTTPS --- octodns/record.py | 10 ++++++++-- tests/test_octodns_record.py | 6 +++--- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/octodns/record.py b/octodns/record.py index 795ad71..201488c 100644 --- a/octodns/record.py +++ b/octodns/record.py @@ -115,6 +115,12 @@ class Record(object): reasons.append('invalid ttl') except KeyError: reasons.append('missing ttl') + try: + if data['octodns']['healthcheck']['protocol'] \ + not in ('HTTP', 'HTTPS'): + reasons.append('invalid healthcheck protocol') + except KeyError: + pass return reasons def __init__(self, zone, name, data, source=None): @@ -172,12 +178,12 @@ class Record(object): try: return self._octodns['healthcheck']['protocol'] except KeyError: - return 'https' + return 'HTTPS' @property def healthcheck_port(self): try: - return self._octodns['healthcheck']['port'] + return int(self._octodns['healthcheck']['port']) except KeyError: return 443 diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index e23808b..494a286 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -755,14 +755,14 @@ class TestRecord(TestCase): 'healthcheck': { 'path': '/_ready', 'host': 'bleep.bloop', - 'protocol': 'http', + 'protocol': 'HTTP', 'port': 8080, } } }) self.assertEquals('/_ready', new.healthcheck_path) self.assertEquals('bleep.bloop', new.healthcheck_host) - self.assertEquals('http', new.healthcheck_protocol) + self.assertEquals('HTTP', new.healthcheck_protocol) self.assertEquals(8080, new.healthcheck_port) new = Record.new(self.zone, 'a', { @@ -772,7 +772,7 @@ class TestRecord(TestCase): }) self.assertEquals('/_dns', new.healthcheck_path) self.assertEquals('a.unit.tests', new.healthcheck_host) - self.assertEquals('https', new.healthcheck_protocol) + self.assertEquals('HTTPS', new.healthcheck_protocol) self.assertEquals(443, new.healthcheck_port) def test_inored(self): From e6d86696113cce6a7bec7b5d4db8c6714bf879a7 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 31 Mar 2018 14:31:15 -0700 Subject: [PATCH 17/23] Implement healthcheck protocol and port for Dyn --- octodns/provider/dyn.py | 67 +++++++++++++++++++++++------- tests/test_octodns_provider_dyn.py | 46 ++++++++++++++++---- tests/test_octodns_record.py | 19 +++++++++ 3 files changed, 110 insertions(+), 22 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 09329c0..1eb013b 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -63,15 +63,46 @@ def _monitor_path_set(self, value): DSFMonitor.path = DSFMonitor.path.setter(_monitor_path_set) -def _monitor_update(self, host, path): +def _monitor_protocol_get(self): + return self._protocol + + +DSFMonitor.protocol = property(_monitor_protocol_get) + + +def _monitor_protocol_set(self, value): + self._protocol = value + + +DSFMonitor.protocol = DSFMonitor.protocol.setter(_monitor_protocol_set) + + +def _monitor_port_get(self): + return self._port or self._options['port'] + + +DSFMonitor.port = property(_monitor_port_get) + + +def _monitor_port_set(self, value): + if self._options is None: + self._options = {} + self._port = self._options['port'] = value + + +DSFMonitor.port = DSFMonitor.port.setter(_monitor_port_set) + + +def _monitor_update(self, host, path, protocol, port): # 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({ + 'protocol': protocol, 'options': { 'host': host, 'path': path, - 'port': DynProvider.MONITOR_PORT, + 'port': port, 'timeout': DynProvider.MONITOR_TIMEOUT, 'header': DynProvider.MONITOR_HEADER, } @@ -82,6 +113,11 @@ DSFMonitor.update = _monitor_update ############################################################################### +def _monitor_matches(monitor, host, path, protocol, port): + return monitor.host != host or monitor.path != path or \ + monitor.protocol != protocol or int(monitor.port) != port + + class _CachingDynZone(DynZone): log = getLogger('_CachingDynZone') @@ -142,10 +178,6 @@ class _CachingDynZone(DynZone): self.flush_cache() -def _monitor_matches(monitor, host, path): - return monitor.host != host or monitor.path != path - - class DynProvider(BaseProvider): ''' Dynect Managed DNS provider @@ -202,7 +234,6 @@ class DynProvider(BaseProvider): } MONITOR_HEADER = 'User-Agent: Dyn Monitor' - MONITOR_PORT = 443 MONITOR_TIMEOUT = 10 _sess_create_lock = Lock() @@ -479,7 +510,9 @@ class DynProvider(BaseProvider): # Heh, when pulled from the API host & path live under options, but # when you create with the constructor they're top-level :-( if _monitor_matches(monitor, record.healthcheck_host, - record.healthcheck_path): + record.healthcheck_path, + record.healthcheck_protocol, + record.healthcheck_port): self.log.info('_extra_changes: health-check mis-match for %s', label) extra.append(Update(record, record)) @@ -586,6 +619,8 @@ class DynProvider(BaseProvider): try: try: monitor = self.traffic_director_monitors[label] + self.log.debug('_traffic_director_monitor: existing for %s', + label) except KeyError: # UNTIL 1.0 We don't have one for the new label format, see if # we still have one for the old and update it @@ -597,19 +632,23 @@ class DynProvider(BaseProvider): self.traffic_director_monitors[fqdn] del self.traffic_director_monitors[fqdn] if _monitor_matches(monitor, record.healthcheck_host, - record.healthcheck_path): + record.healthcheck_path, + record.healthcheck_protocol, + record.healthcheck_port): self.log.info('_traffic_director_monitor: updating monitor ' 'for %s', label) monitor.update(record.healthcheck_host, - record.healthcheck_path) + record.healthcheck_path, + record.healthcheck_protocol, + record.healthcheck_port) return monitor except KeyError: self.log.info('_traffic_director_monitor: creating monitor ' 'for %s', label) - monitor = DSFMonitor(label, protocol='HTTPS', response_count=2, - probe_interval=60, retries=2, - port=self.MONITOR_PORT, active='Y', - host=record.healthcheck_host, + monitor = DSFMonitor(label, protocol=record.healthcheck_protocol, + response_count=2, probe_interval=60, + retries=2, port=record.healthcheck_port, + active='Y', host=record.healthcheck_host, timeout=self.MONITOR_TIMEOUT, header=self.MONITOR_HEADER, path=record.healthcheck_path) diff --git a/tests/test_octodns_provider_dyn.py b/tests/test_octodns_provider_dyn.py index 3412e34..ac56477 100644 --- a/tests/test_octodns_provider_dyn.py +++ b/tests/test_octodns_provider_dyn.py @@ -768,7 +768,9 @@ class TestDynProviderGeo(TestCase): 'octodns': { 'healthcheck': { 'host': 'bleep.bloop', - 'path': '/_nope' + 'path': '/_nope', + 'protocol': 'HTTP', + 'port': 8080, } }, 'ttl': 60, @@ -787,10 +789,10 @@ class TestDynProviderGeo(TestCase): 'header': 'User-Agent: Dyn Monitor', 'host': 'bleep.bloop', 'path': '/_nope', - 'port': '443', + 'port': '8080', 'timeout': '10', 'probe_interval': '60', - 'protocol': 'HTTPS', + 'protocol': 'HTTP', 'response_count': '2', 'retries': '2' }, @@ -806,11 +808,12 @@ class TestDynProviderGeo(TestCase): # should have resulted an update mock.assert_has_calls([ call('/DSFMonitor/42a/', 'PUT', { + 'protocol': 'HTTP', 'options': { 'path': '/_nope', 'host': 'bleep.bloop', 'header': 'User-Agent: Dyn Monitor', - 'port': 443, + 'port': 8080, 'timeout': 10 } }) @@ -821,6 +824,8 @@ class TestDynProviderGeo(TestCase): monitor = provider._traffic_director_monitors['unit.tests.:A'] self.assertEquals('bleep.bloop', monitor.host) self.assertEquals('/_nope', monitor.path) + self.assertEquals('HTTP', monitor.protocol) + self.assertEquals('8080', monitor.port) # test upgrading an old label record = Record.new(existing, 'old-label', { @@ -1510,15 +1515,20 @@ class TestDynProviderAlias(TestCase): # patching class DummyDSFMonitor(DSFMonitor): - def __init__(self, host=None, path=None, options_host=None, - options_path=None): + def __init__(self, host=None, path=None, protocol=None, port=None, + options_host=None, options_path=None, options_protocol=None, + options_port=None): # not calling super on purpose self._host = host self._path = path + self._protocol = protocol + self._port = port if options_host: self._options = { 'host': options_host, 'path': options_path, + 'protocol': options_protocol, + 'port': options_port, } else: self._options = None @@ -1527,12 +1537,16 @@ class DummyDSFMonitor(DSFMonitor): class TestDSFMonitorMonkeyPatching(TestCase): def test_host(self): - monitor = DummyDSFMonitor(host='host.com', path='/path') + monitor = DummyDSFMonitor(host='host.com', path='/path', + protocol='HTTP', port=8080) self.assertEquals('host.com', monitor.host) self.assertEquals('/path', monitor.path) + self.assertEquals('HTTP', monitor.protocol) + self.assertEquals(8080, monitor.port) monitor = DummyDSFMonitor(options_host='host.com', - options_path='/path') + options_path='/path', + options_protocol='HTTP', options_port=8080) self.assertEquals('host.com', monitor.host) self.assertEquals('/path', monitor.path) @@ -1540,6 +1554,10 @@ class TestDSFMonitorMonkeyPatching(TestCase): self.assertEquals('other.com', monitor.host) monitor.path = '/other-path' self.assertEquals('/other-path', monitor.path) + monitor.protocol = 'HTTPS' + self.assertEquals('HTTPS', monitor.protocol) + monitor.port = 8081 + self.assertEquals(8081, monitor.port) monitor = DummyDSFMonitor() monitor.host = 'other.com' @@ -1547,3 +1565,15 @@ class TestDSFMonitorMonkeyPatching(TestCase): monitor = DummyDSFMonitor() monitor.path = '/other-path' self.assertEquals('/other-path', monitor.path) + monitor.protocol = 'HTTP' + self.assertEquals('HTTP', monitor.protocol) + monitor.port = 8080 + self.assertEquals(8080, monitor.port) + + # Just to exercise the _options init + monitor = DummyDSFMonitor() + monitor.protocol = 'HTTP' + self.assertEquals('HTTP', monitor.protocol) + monitor = DummyDSFMonitor() + monitor.port = 8080 + self.assertEquals(8080, monitor.port) diff --git a/tests/test_octodns_record.py b/tests/test_octodns_record.py index 494a286..16da404 100644 --- a/tests/test_octodns_record.py +++ b/tests/test_octodns_record.py @@ -1020,6 +1020,25 @@ class TestRecordValidation(TestCase): 'invalid ip address "goodbye"' ], ctx.exception.reasons) + # invalid healthcheck protocol + with self.assertRaises(ValidationError) as ctx: + Record.new(self.zone, 'a', { + 'geo': { + 'NA': ['1.2.3.5'], + 'NA-US': ['1.2.3.5', '1.2.3.6'] + }, + 'type': 'A', + 'ttl': 600, + 'value': '1.2.3.4', + 'octodns': { + 'healthcheck': { + 'protocol': 'FTP', + } + } + }) + self.assertEquals(['invalid healthcheck protocol'], + ctx.exception.reasons) + def test_AAAA(self): # doesn't blow up Record.new(self.zone, '', { From b51a9148b81bde86c2bada082431092479ff9dff Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Sat, 31 Mar 2018 14:47:22 -0700 Subject: [PATCH 18/23] Route53 support for healthcheck protocol & port --- octodns/provider/route53.py | 23 ++++++++---- tests/test_octodns_provider_route53.py | 49 ++++++++++++++++++++++---- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 46256c6..aac8241 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -526,11 +526,12 @@ class Route53Provider(BaseProvider): # We've got a cached version use it return self._health_checks - def _health_check_equivilent(self, host, path, health_check, - first_value=None): + def _health_check_equivilent(self, host, path, protocol, port, + health_check, first_value=None): config = health_check['HealthCheckConfig'] return host == config['FullyQualifiedDomainName'] and \ - path == config['ResourcePath'] and \ + path == config['ResourcePath'] and protocol == config['Type'] \ + and port == config['Port'] and \ (first_value is None or first_value == config['IPAddress']) def get_health_check_id(self, record, ident, geo, create): @@ -546,6 +547,8 @@ class Route53Provider(BaseProvider): healthcheck_host = record.healthcheck_host healthcheck_path = record.healthcheck_path + healthcheck_protocol = record.healthcheck_protocol + healthcheck_port = record.healthcheck_port # we're looking for a healthcheck with the current version & our record # type, we'll ignore anything else @@ -556,7 +559,9 @@ class Route53Provider(BaseProvider): # not a version & type match, ignore continue if self._health_check_equivilent(healthcheck_host, - healthcheck_path, health_check, + healthcheck_path, + healthcheck_protocol, + healthcheck_port, health_check, first_value=first_value): # this is the health check we're looking for return id @@ -567,15 +572,15 @@ class Route53Provider(BaseProvider): # no existing matches, we need to create a new health check config = { - 'EnableSNI': True, + 'EnableSNI': healthcheck_protocol == 'HTTPS', 'FailureThreshold': 6, 'FullyQualifiedDomainName': healthcheck_host, 'IPAddress': first_value, 'MeasureLatency': True, - 'Port': 443, + 'Port': healthcheck_port, 'RequestInterval': 10, 'ResourcePath': healthcheck_path, - 'Type': 'HTTPS', + 'Type': healthcheck_protocol, } ref = '{}:{}:{}:{}'.format(self.HEALTH_CHECK_VERSION, record._type, record.name, uuid4().hex[:16]) @@ -699,6 +704,8 @@ class Route53Provider(BaseProvider): healthcheck_host = record.healthcheck_host healthcheck_path = record.healthcheck_path + healthcheck_protocol = record.healthcheck_protocol + healthcheck_port = record.healthcheck_port fqdn = record.fqdn # loop through all the r53 rrsets @@ -718,6 +725,8 @@ class Route53Provider(BaseProvider): if caller_ref.startswith(self.HEALTH_CHECK_VERSION): if self._health_check_equivilent(healthcheck_host, healthcheck_path, + healthcheck_protocol, + healthcheck_port, health_check): # it has the right health check continue diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 86a6ad4..d51e20b 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -102,6 +102,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }, { @@ -112,6 +114,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '5.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 42, }, { @@ -122,6 +126,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '5.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }, { @@ -132,6 +138,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '7.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }, { @@ -143,6 +151,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '7.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }] @@ -674,6 +684,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }, { @@ -684,6 +696,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '9.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }] @@ -704,6 +718,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '8.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }] @@ -749,6 +765,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }, { @@ -759,6 +777,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }] @@ -770,16 +790,15 @@ class TestRoute53Provider(TestCase): }) health_check_config = { - 'EnableSNI': True, + 'EnableSNI': False, 'FailureThreshold': 6, - 'FullyQualifiedDomainName': 'unit.tests', + 'FullyQualifiedDomainName': 'foo.bar.com', 'IPAddress': '4.2.3.4', - 'ResourcePath': '/_dns', 'MeasureLatency': True, - 'Port': 443, + 'Port': 8080, 'RequestInterval': 10, - 'ResourcePath': '/_dns', - 'Type': 'HTTPS' + 'ResourcePath': '/_status', + 'Type': 'HTTP' } stubber.add_response('create_health_check', { 'HealthCheck': { @@ -800,6 +819,14 @@ class TestRoute53Provider(TestCase): 'values': ['2.2.3.4', '3.2.3.4'], 'geo': { 'AF': ['4.2.3.4'], + }, + 'octodns': { + 'healthcheck': { + 'host': 'foo.bar.com', + 'path': '/_status', + 'port': 8080, + 'protocol': 'HTTP', + }, } }) @@ -900,6 +927,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }, { @@ -910,6 +939,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }, { @@ -920,6 +951,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'other.unit.tests', 'IPAddress': '4.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }] @@ -1094,6 +1127,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '2.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }], @@ -1195,6 +1230,8 @@ class TestRoute53Provider(TestCase): 'FullyQualifiedDomainName': 'a.unit.tests', 'IPAddress': '2.2.3.4', 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, }, 'HealthCheckVersion': 2, }], From db6b9d2ada2031d5d4f80522f4887c5b9f614682 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 2 Apr 2018 08:09:32 -0700 Subject: [PATCH 19/23] Remove stale comment about dyn property locations --- octodns/provider/dyn.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index 1eb013b..d8b535f 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -507,8 +507,6 @@ class DynProvider(BaseProvider): label) 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_matches(monitor, record.healthcheck_host, record.healthcheck_path, record.healthcheck_protocol, From 8330a3a16c7de4f4e6daee79f6f5eca01ddce0ee Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Mon, 2 Apr 2018 08:20:07 -0700 Subject: [PATCH 20/23] Update a now stale comment on Route53 provider about healthchecks --- octodns/provider/route53.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index aac8241..0682d5d 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -556,7 +556,7 @@ class Route53Provider(BaseProvider): record._type, record.name) for id, health_check in self.health_checks.items(): if not health_check['CallerReference'].startswith(expected_ref): - # not a version & type match, ignore + # not match, ignore continue if self._health_check_equivilent(healthcheck_host, healthcheck_path, From a5c560f203d88ec10292e6db0168ff3990339d3e Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 4 Apr 2018 07:33:57 -0700 Subject: [PATCH 21/23] Rename Dyn._monitor_matches to _monitor_doesnt_match --- octodns/provider/dyn.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/octodns/provider/dyn.py b/octodns/provider/dyn.py index d8b535f..de430db 100644 --- a/octodns/provider/dyn.py +++ b/octodns/provider/dyn.py @@ -113,7 +113,7 @@ DSFMonitor.update = _monitor_update ############################################################################### -def _monitor_matches(monitor, host, path, protocol, port): +def _monitor_doesnt_match(monitor, host, path, protocol, port): return monitor.host != host or monitor.path != path or \ monitor.protocol != protocol or int(monitor.port) != port @@ -507,10 +507,10 @@ class DynProvider(BaseProvider): label) extra.append(Update(record, record)) continue - if _monitor_matches(monitor, record.healthcheck_host, - record.healthcheck_path, - record.healthcheck_protocol, - record.healthcheck_port): + if _monitor_doesnt_match(monitor, record.healthcheck_host, + record.healthcheck_path, + record.healthcheck_protocol, + record.healthcheck_port): self.log.info('_extra_changes: health-check mis-match for %s', label) extra.append(Update(record, record)) @@ -629,10 +629,10 @@ class DynProvider(BaseProvider): self.traffic_director_monitors[label] = \ self.traffic_director_monitors[fqdn] del self.traffic_director_monitors[fqdn] - if _monitor_matches(monitor, record.healthcheck_host, - record.healthcheck_path, - record.healthcheck_protocol, - record.healthcheck_port): + if _monitor_doesnt_match(monitor, record.healthcheck_host, + record.healthcheck_path, + record.healthcheck_protocol, + record.healthcheck_port): self.log.info('_traffic_director_monitor: updating monitor ' 'for %s', label) monitor.update(record.healthcheck_host, From 19956f14bc040679fba13c22ec6f4a9fe2b91a52 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 4 Apr 2018 17:38:00 -0700 Subject: [PATCH 22/23] Include fqdn in Route53 health check refs, not name This will ensure unique refs for different zones. Without them the ref isn't enough to make sure we're looking at the right thing (notably when we're gc'ing old health checks.) This also adds a bit more debugging around health checks. --- octodns/provider/route53.py | 15 +++++++++------ tests/test_octodns_provider_route53.py | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 0682d5d..c99873a 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -553,7 +553,7 @@ class Route53Provider(BaseProvider): # we're looking for a healthcheck with the current version & our record # type, we'll ignore anything else expected_ref = '{}:{}:{}:'.format(self.HEALTH_CHECK_VERSION, - record._type, record.name) + record._type, record.fqdn) for id, health_check in self.health_checks.items(): if not health_check['CallerReference'].startswith(expected_ref): # not match, ignore @@ -564,10 +564,12 @@ class Route53Provider(BaseProvider): healthcheck_port, health_check, first_value=first_value): # this is the health check we're looking for + self.log.debug('get_health_check_id: found match id=%s', id) return id if not create: # no existing matches and not allowed to create, return none + self.log.debug('get_health_check_id: no matches, no create') return # no existing matches, we need to create a new health check @@ -583,7 +585,7 @@ class Route53Provider(BaseProvider): 'Type': healthcheck_protocol, } ref = '{}:{}:{}:{}'.format(self.HEALTH_CHECK_VERSION, record._type, - record.name, uuid4().hex[:16]) + record.fqdn, uuid4().hex[:12]) resp = self._conn.create_health_check(CallerReference=ref, HealthCheckConfig=config) health_check = resp['HealthCheck'] @@ -591,9 +593,10 @@ class Route53Provider(BaseProvider): # store the new health check so that we'll be able to find it in the # future self._health_checks[id] = health_check - self.log.info('get_health_check_id: created id=%s, host=%s, path=%s' - 'first_value=%s', id, healthcheck_host, healthcheck_path, - first_value) + self.log.info('get_health_check_id: created id=%s, host=%s, path=%s, ' + 'protocol=%s, port=%d, first_value=%s', id, + healthcheck_host, healthcheck_path, healthcheck_protocol, + healthcheck_port, first_value) return id def _gc_health_checks(self, record, new): @@ -611,7 +614,7 @@ class Route53Provider(BaseProvider): # that apply to this record, deleting any that do and are no longer in # use expected_re = re.compile(r'^\d\d\d\d:{}:{}:' - .format(record._type, record.name)) + .format(record._type, record.fqdn)) # UNITL 1.0: we'll clean out the previous version of Route53 health # checks as best as we can. expected_legacy_host = record.fqdn[:-1] diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index d51e20b..aec31cb 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -93,7 +93,8 @@ class TestRoute53Provider(TestCase): record = Record.new(expected, name, data) expected.add_record(record) - caller_ref = '{}:A::1324'.format(Route53Provider.HEALTH_CHECK_VERSION) + caller_ref = '{}:A:unit.tests.:1324' \ + .format(Route53Provider.HEALTH_CHECK_VERSION) health_checks = [{ 'Id': '42', 'CallerReference': caller_ref, From e4d75faf3ce18ad5ebb12c4037583edefa647aed Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Wed, 4 Apr 2018 17:49:11 -0700 Subject: [PATCH 23/23] Add a note to CHANGELOG about health check updates --- CHANGELOG.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11d984c..c3eaae6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,17 @@ +## v0.9.1 - UNRELEASED + +### NOTICE + +Using this version on existing records with `geo` will result in +recreating all health checks. This process has been tested pretty thoroughly to +try and ensure a seemless upgrade without any traffic shifting around. It's +probably best to take extra care when updating and to try and make sure that +all health checks are passing before the first sync with `--doit`. See +[#67](https://github.com/github/octodns/pull/67) for more information. + +* Major update to geo healthchecks to allow configuring host (header), path, + protocol, and port [#67](https://github.com/github/octodns/pull/67) + ## v0.9.0 - 2018-03-26 - Way too long since we last met * Way way way too much to list out here, shouldn't have waited so long