From 7a32e4c912e4578864e2ad23592de467543f2f1f Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 9 Dec 2021 10:24:15 -0800 Subject: [PATCH 1/5] Add value status support to Route53 dynamic --- octodns/provider/route53.py | 57 +++++++++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index a6a3105..67fab21 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -80,9 +80,11 @@ class _Route53Record(EqualityTupleMixin): # be served out according to their weights for i, value in enumerate(pool.data['values']): weight = value['weight'] + status = value['status'] value = value['value'] ret.add(_Route53DynamicValue(provider, record, pool_name, - value, weight, i, creating)) + value, weight, status, i, + creating)) # Rules for i, rule in enumerate(record.dynamic.rules): @@ -345,19 +347,21 @@ class _Route53DynamicRule(_Route53Record): class _Route53DynamicValue(_Route53Record): - def __init__(self, provider, record, pool_name, value, weight, index, - creating): + def __init__(self, provider, record, pool_name, value, weight, status, + index, creating): fqdn_override = f'_octodns-{pool_name}-value.{record.fqdn}' super(_Route53DynamicValue, self).__init__(provider, record, creating, fqdn_override=fqdn_override) self.pool_name = pool_name + self.status = status self.index = index value_convert = getattr(self, f'_value_convert_{record._type}') self.value = value_convert(value, record) self.weight = weight self.health_check_id = provider.get_health_check_id(record, self.value, + self.status, creating) @property @@ -433,7 +437,7 @@ class _Route53GeoRecord(_Route53Record): value = geo.values[0] self.health_check_id = provider.get_health_check_id(record, value, - creating) + 'obey', creating) def mod(self, action, existing_rrsets): geo = self.geo @@ -599,6 +603,7 @@ class Route53Provider(BaseProvider): ''' SUPPORTS_GEO = True SUPPORTS_DYNAMIC = True + SUPPORTS_POOL_VALUE_STATUS = True SUPPORTS = set(('A', 'AAAA', 'CAA', 'CNAME', 'MX', 'NAPTR', 'NS', 'PTR', 'SPF', 'SRV', 'TXT')) @@ -914,7 +919,25 @@ class Route53Provider(BaseProvider): # it's ignored only used to make sure the value is unique pool_name = rrset['SetIdentifier'][:-4] value = rrset['ResourceRecords'][0]['Value'] + try: + health_check_id = rrset.get('HealthCheckId', None) + health_check = self.health_checks[health_check_id] + health_check_config = health_check['HealthCheckConfig'] + if health_check_config['Disabled']: + if health_check_config['Inverted']: + # disabled and inverted means down + status = 'down' + else: + # disabled means always up + status = 'up' + else: + # otherwise obey + status = 'obey' + except KeyError: + # No healthcheck implies status is always up + status = 'up' pools[pool_name]['values'].append({ + 'status': status, 'value': value, 'weight': rrset['Weight'], }) @@ -1092,7 +1115,8 @@ class Route53Provider(BaseProvider): def _health_check_equivalent(self, host, path, protocol, port, measure_latency, request_interval, - health_check, value=None): + health_check, value=None, disabled=None, + inverted=None): config = health_check['HealthCheckConfig'] # So interestingly Route53 normalizes IPv6 addresses to a funky, but @@ -1115,16 +1139,18 @@ class Route53Provider(BaseProvider): port == config['Port'] and \ measure_latency == config['MeasureLatency'] and \ request_interval == config['RequestInterval'] and \ + (disabled is None or disabled == config['Disabled']) and \ + (inverted is None or inverted == config['Inverted']) and \ value == config_ip_address - def get_health_check_id(self, record, value, create): + def get_health_check_id(self, record, value, status, 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 # we're going to assume that ips are interchangeable to avoid # health-checking each one independently fqdn = record.fqdn - self.log.debug('get_health_check_id: fqdn=%s, type=%s, value=%s', - fqdn, record._type, value) + self.log.debug('get_health_check_id: fqdn=%s, type=%s, value=%s, ' + 'status=%s', fqdn, record._type, value, status) try: ip_address(str(value)) @@ -1140,6 +1166,15 @@ class Route53Provider(BaseProvider): healthcheck_port = record.healthcheck_port healthcheck_latency = self._healthcheck_measure_latency(record) healthcheck_interval = self._healthcheck_request_interval(record) + if status == 'up': + healthcheck_disabled = True + healthcheck_inverted = False + elif status == 'down': + healthcheck_disabled = True + healthcheck_inverted = True + else: + healthcheck_disabled = False + healthcheck_inverted = False # we're looking for a healthcheck with the current version & our record # type, we'll ignore anything else @@ -1156,7 +1191,9 @@ class Route53Provider(BaseProvider): healthcheck_latency, healthcheck_interval, health_check, - value=value): + value=value, + disabled=healthcheck_disabled, + inverted=healthcheck_inverted): # this is the health check we're looking for self.log.debug('get_health_check_id: found match id=%s', id) return id @@ -1168,6 +1205,8 @@ class Route53Provider(BaseProvider): # no existing matches, we need to create a new health check config = { + 'Disabled': healthcheck_disabled, + 'Inverted': healthcheck_inverted, 'EnableSNI': healthcheck_protocol == 'HTTPS', 'FailureThreshold': 6, 'MeasureLatency': healthcheck_latency, From 81ef5f5b8902fa8a7543209f8d41475a95af28a8 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 9 Dec 2021 12:24:07 -0800 Subject: [PATCH 2/5] Update tests to handle new status related params/data --- tests/test_octodns_provider_route53.py | 105 ++++++++++++++++++++----- 1 file changed, 85 insertions(+), 20 deletions(-) diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 56362a0..be9b714 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -199,24 +199,24 @@ dynamic_record_data = { 'ap-southeast-1': { 'fallback': 'us-east-1', 'values': [{ - 'weight': 2, 'value': '1.4.1.1' + 'weight': 2, 'value': '1.4.1.1', 'status': 'up', }, { - 'weight': 2, 'value': '1.4.1.2' + 'weight': 2, 'value': '1.4.1.2', 'status': 'up', }] }, 'eu-central-1': { 'fallback': 'us-east-1', 'values': [{ - 'weight': 1, 'value': '1.3.1.1' + 'weight': 1, 'value': '1.3.1.1', 'status': 'up', }, { - 'weight': 1, 'value': '1.3.1.2' + 'weight': 1, 'value': '1.3.1.2', 'status': 'up', }], }, 'us-east-1': { 'values': [{ - 'weight': 1, 'value': '1.5.1.1' + 'weight': 1, 'value': '1.5.1.1', 'status': 'up', }, { - 'weight': 1, 'value': '1.5.1.2' + 'weight': 1, 'value': '1.5.1.2', 'status': 'up', }], } }, @@ -296,6 +296,9 @@ class TestRoute53Provider(TestCase): 'Id': '42', 'CallerReference': caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', @@ -310,6 +313,9 @@ class TestRoute53Provider(TestCase): 'Id': 'ignored-also', 'CallerReference': 'something-else', 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '5.2.3.4', @@ -324,6 +330,9 @@ class TestRoute53Provider(TestCase): 'Id': '43', 'CallerReference': caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '5.2.3.4', @@ -338,6 +347,9 @@ class TestRoute53Provider(TestCase): 'Id': '44', 'CallerReference': caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '7.2.3.4', @@ -353,6 +365,9 @@ class TestRoute53Provider(TestCase): # won't match anything based on type 'CallerReference': caller_ref.replace(':A:', ':AAAA:'), 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '7.2.3.4', @@ -1161,6 +1176,9 @@ class TestRoute53Provider(TestCase): 'Id': '42', 'CallerReference': self.caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', @@ -1175,6 +1193,9 @@ class TestRoute53Provider(TestCase): 'Id': '43', 'CallerReference': 'abc123', 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '9.2.3.4', @@ -1199,6 +1220,9 @@ class TestRoute53Provider(TestCase): 'Id': '44', 'CallerReference': self.caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '8.2.3.4', @@ -1248,6 +1272,9 @@ class TestRoute53Provider(TestCase): # No match based on version 'CallerReference': '9999:A:foo1234', 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', @@ -1262,6 +1289,9 @@ class TestRoute53Provider(TestCase): 'Id': '43', 'CallerReference': caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', @@ -1281,7 +1311,9 @@ class TestRoute53Provider(TestCase): }) health_check_config = { + 'Disabled': False, 'EnableSNI': False, + 'Inverted': False, 'FailureThreshold': 6, 'FullyQualifiedDomainName': 'foo.bar.com', 'IPAddress': '4.2.3.4', @@ -1306,7 +1338,9 @@ class TestRoute53Provider(TestCase): stubber.add_response('change_tags_for_resource', {}) health_check_config = { + 'Disabled': False, 'EnableSNI': False, + 'Inverted': False, 'FailureThreshold': 6, 'FullyQualifiedDomainName': '4.2.3.4', 'IPAddress': '4.2.3.4', @@ -1349,23 +1383,25 @@ class TestRoute53Provider(TestCase): # if not allowed to create returns none value = record.geo['AF'].values[0] - id = provider.get_health_check_id(record, value, False) + id = provider.get_health_check_id(record, value, 'obey', False) self.assertFalse(id) # when allowed to create we do - id = provider.get_health_check_id(record, value, True) + id = provider.get_health_check_id(record, value, 'obey', True) self.assertEquals('42', id) # when allowed to create and when host is None record._octodns['healthcheck']['host'] = None - id = provider.get_health_check_id(record, value, True) + id = provider.get_health_check_id(record, value, 'obey', True) self.assertEquals('43', id) stubber.assert_no_pending_responses() # A CNAME style healthcheck, without a value health_check_config = { + 'Disabled': False, 'EnableSNI': False, + 'Inverted': False, 'FailureThreshold': 6, 'FullyQualifiedDomainName': 'target-1.unit.tests.', 'MeasureLatency': True, @@ -1388,14 +1424,17 @@ class TestRoute53Provider(TestCase): }) stubber.add_response('change_tags_for_resource', {}) - id = provider.get_health_check_id(record, 'target-1.unit.tests.', True) + id = provider.get_health_check_id(record, 'target-1.unit.tests.', + 'obey', True) self.assertEquals('42', id) stubber.assert_no_pending_responses() # TCP health check health_check_config = { + 'Disabled': False, 'EnableSNI': False, + 'Inverted': False, 'FailureThreshold': 6, 'MeasureLatency': True, 'Port': 8080, @@ -1417,7 +1456,8 @@ class TestRoute53Provider(TestCase): stubber.add_response('change_tags_for_resource', {}) record._octodns['healthcheck']['protocol'] = 'TCP' - id = provider.get_health_check_id(record, 'target-1.unit.tests.', True) + id = provider.get_health_check_id(record, 'target-1.unit.tests.', + 'obey', True) self.assertEquals('42', id) stubber.assert_no_pending_responses() @@ -1494,7 +1534,9 @@ class TestRoute53Provider(TestCase): provider, stubber = self._get_stubbed_provider() health_check_config = { + 'Disabled': False, 'EnableSNI': True, + 'Inverted': False, 'FailureThreshold': 6, 'FullyQualifiedDomainName': 'a.unit.tests', 'IPAddress': '1.2.3.4', @@ -1547,7 +1589,7 @@ class TestRoute53Provider(TestCase): }) value = record.geo['AF'].values[0] - id = provider.get_health_check_id(record, value, True) + id = provider.get_health_check_id(record, value, 'obey', True) ml = provider.health_checks[id]['HealthCheckConfig']['MeasureLatency'] ri = provider.health_checks[id]['HealthCheckConfig']['RequestInterval'] self.assertFalse(ml) @@ -1636,6 +1678,9 @@ class TestRoute53Provider(TestCase): 'Id': '42', 'CallerReference': self.caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', @@ -1650,6 +1695,9 @@ class TestRoute53Provider(TestCase): 'Id': '43', 'CallerReference': old_caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '4.2.3.4', @@ -1664,6 +1712,9 @@ class TestRoute53Provider(TestCase): 'Id': '44', 'CallerReference': old_caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'other.unit.tests', 'IPAddress': '4.2.3.4', @@ -2105,6 +2156,9 @@ class TestRoute53Provider(TestCase): 'Id': '42', 'CallerReference': 'foo', 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'unit.tests', 'IPAddress': '2.2.3.4', @@ -2210,6 +2264,9 @@ class TestRoute53Provider(TestCase): 'Id': '42', 'CallerReference': self.caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'a.unit.tests', 'IPAddress': '2.2.3.4', @@ -2359,6 +2416,9 @@ class TestRoute53Provider(TestCase): 'Id': '42', 'CallerReference': self.caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'a.unit.tests', 'IPAddress': '2.2.3.4', @@ -2517,6 +2577,9 @@ class TestRoute53Provider(TestCase): 'Id': '42', 'CallerReference': self.caller_ref, 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, 'Type': 'HTTPS', 'FullyQualifiedDomainName': 'one.cname.unit.tests.', 'ResourcePath': '/_dns', @@ -2695,6 +2758,7 @@ class TestRoute53Provider(TestCase): def test_data_for_dynamic(self): provider = Route53Provider('test', 'abc', '123') + provider._health_checks = {} data = provider._data_for_dynamic('', 'A', dynamic_rrsets) self.assertEquals(dynamic_record_data, data) @@ -2703,6 +2767,7 @@ class TestRoute53Provider(TestCase): @patch('octodns.provider.route53.Route53Provider._load_records') def test_dynamic_populate(self, load_records_mock, get_zone_id_mock): provider = Route53Provider('test', 'abc', '123') + provider._health_checks = {} get_zone_id_mock.side_effect = ['z44'] load_records_mock.side_effect = [dynamic_rrsets] @@ -2724,25 +2789,25 @@ class TestRoute53Provider(TestCase): 'ap-southeast-1': { 'fallback': 'us-east-1', 'values': [{ - 'weight': 2, 'value': '1.4.1.1', 'status': 'obey', + 'weight': 2, 'value': '1.4.1.1', 'status': 'up', }, { - 'weight': 2, 'value': '1.4.1.2', 'status': 'obey', + 'weight': 2, 'value': '1.4.1.2', 'status': 'up', }] }, 'eu-central-1': { 'fallback': 'us-east-1', 'values': [{ - 'weight': 1, 'value': '1.3.1.1', 'status': 'obey', + 'weight': 1, 'value': '1.3.1.1', 'status': 'up', }, { - 'weight': 1, 'value': '1.3.1.2', 'status': 'obey', + 'weight': 1, 'value': '1.3.1.2', 'status': 'up', }], }, 'us-east-1': { 'fallback': None, 'values': [{ - 'weight': 1, 'value': '1.5.1.1', 'status': 'obey', + 'weight': 1, 'value': '1.5.1.1', 'status': 'up', }, { - 'weight': 1, 'value': '1.5.1.2', 'status': 'obey', + 'weight': 1, 'value': '1.5.1.2', 'status': 'up', }], } }, {k: v.data for k, v in record.dynamic.pools.items()}) @@ -2905,7 +2970,7 @@ class TestRoute53Records(TestCase): def test_dynamic_value_delete(self): provider = DummyProvider() geo = _Route53DynamicValue(provider, self.record_a, 'iad', '2.2.2.2', - 1, 0, False) + 1, 'obey', 0, False) rrset = { 'HealthCheckId': 'x12346z', @@ -3008,7 +3073,7 @@ class TestRoute53Records(TestCase): # thoroughly tested elsewhere provider._health_checks = {} # When asked for a healthcheck return dummy info - provider.get_health_check_id = lambda r, v, c: 'hc42' + provider.get_health_check_id = lambda r, v, s, c: 'hc42' zone = Zone('unit.tests.', []) record = Record.new(zone, '', dynamic_record_data) From 606ffeaed17efa30da3eebdd19aeeafe33b0009d Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 9 Dec 2021 12:55:37 -0800 Subject: [PATCH 3/5] Route53 dynamic value status support test coverage --- tests/test_octodns_provider_route53.py | 128 ++++++++++++++++++++++++- 1 file changed, 124 insertions(+), 4 deletions(-) diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index be9b714..cf4e64e 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -192,6 +192,26 @@ dynamic_rrsets = [{ 'SetIdentifier': '3-us-east-1-None', 'Type': 'A', }] +dynamic_health_checks = { + '76': { + 'HealthCheckConfig': { + 'Disabled': False, + 'Inverted': False, + } + }, + '09': { + 'HealthCheckConfig': { + 'Disabled': True, + 'Inverted': False, + } + }, + 'ab': { + 'HealthCheckConfig': { + 'Disabled': True, + 'Inverted': True, + } + }, +} dynamic_record_data = { 'dynamic': { @@ -199,7 +219,7 @@ dynamic_record_data = { 'ap-southeast-1': { 'fallback': 'us-east-1', 'values': [{ - 'weight': 2, 'value': '1.4.1.1', 'status': 'up', + 'weight': 2, 'value': '1.4.1.1', 'status': 'obey', }, { 'weight': 2, 'value': '1.4.1.2', 'status': 'up', }] @@ -207,7 +227,7 @@ dynamic_record_data = { 'eu-central-1': { 'fallback': 'us-east-1', 'values': [{ - 'weight': 1, 'value': '1.3.1.1', 'status': 'up', + 'weight': 1, 'value': '1.3.1.1', 'status': 'down', }, { 'weight': 1, 'value': '1.3.1.2', 'status': 'up', }], @@ -1259,9 +1279,109 @@ class TestRoute53Provider(TestCase): } }) value = record.geo['AF'].values[0] - id = provider.get_health_check_id(record, value, True) + id = provider.get_health_check_id(record, value, 'obey', True) self.assertEquals('42', id) + def test_health_check_status_support(self): + provider, stubber = self._get_stubbed_provider() + + health_checks = [{ + 'Id': '42', + 'CallerReference': self.caller_ref, + 'HealthCheckConfig': { + 'Disabled': False, + 'EnableSNI': True, + 'Inverted': False, + 'Type': 'HTTPS', + 'FullyQualifiedDomainName': 'unit.tests', + 'IPAddress': '1.1.1.1', + 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, + 'MeasureLatency': True, + 'RequestInterval': 10, + }, + 'HealthCheckVersion': 2, + }, { + 'Id': '43', + 'CallerReference': self.caller_ref, + 'HealthCheckConfig': { + 'Disabled': True, + 'EnableSNI': True, + 'Inverted': False, + 'Type': 'HTTPS', + 'FullyQualifiedDomainName': 'unit.tests', + 'IPAddress': '2.2.2.2', + 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, + 'MeasureLatency': True, + 'RequestInterval': 10, + }, + 'HealthCheckVersion': 2, + }, { + 'Id': '44', + 'CallerReference': self.caller_ref, + 'HealthCheckConfig': { + 'Disabled': True, + 'EnableSNI': True, + 'Inverted': True, + 'Type': 'HTTPS', + 'FullyQualifiedDomainName': 'unit.tests', + 'IPAddress': '3.3.3.3', + 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, + 'MeasureLatency': True, + 'RequestInterval': 10, + }, + 'HealthCheckVersion': 2, + }] + stubber.add_response('list_health_checks', + { + 'HealthChecks': health_checks, + 'IsTruncated': False, + 'MaxItems': '20', + 'Marker': '', + }) + + health_checks = provider.health_checks + + # get without create + record = Record.new(self.expected, '', { + 'ttl': 61, + 'type': 'A', + 'value': '5.5.5.5', + 'dynamic': { + 'pools': { + 'main': { + 'values': [{ + 'value': '6.6.6.6', + }] + } + }, + 'rules': [{ + 'pool': 'main', + }] + } + }) + self.assertEquals('42', + provider.get_health_check_id(record, '1.1.1.1', + 'obey', False)) + self.assertEquals('43', + provider.get_health_check_id(record, '2.2.2.2', + 'up', False)) + self.assertEquals('44', + provider.get_health_check_id(record, '3.3.3.3', + 'down', False)) + + # If we're not allowed to create we won't find a health check for + # 1.1.1.1 with status up or down + self.assertFalse(provider.get_health_check_id(record, '1.1.1.1', + 'up', False)) + self.assertFalse(provider.get_health_check_id(record, '1.1.1.1', + 'down', False)) + def test_health_check_create(self): provider, stubber = self._get_stubbed_provider() @@ -2758,7 +2878,7 @@ class TestRoute53Provider(TestCase): def test_data_for_dynamic(self): provider = Route53Provider('test', 'abc', '123') - provider._health_checks = {} + provider._health_checks = dynamic_health_checks data = provider._data_for_dynamic('', 'A', dynamic_rrsets) self.assertEquals(dynamic_record_data, data) From e517023208a8cfcc279c38d7b286dcd672d9afdd Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 14 Dec 2021 10:28:20 -0800 Subject: [PATCH 4/5] Route53 status: up means no health check created --- octodns/provider/route53.py | 33 ++++++++++++++------------ tests/test_octodns_provider_route53.py | 11 ++------- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 67fab21..3ac9874 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -383,10 +383,9 @@ class _Route53DynamicValue(_Route53Record): 'ResourceRecordSet': existing, } - return { + ret = { 'Action': action, 'ResourceRecordSet': { - 'HealthCheckId': self.health_check_id, 'Name': self.fqdn, 'ResourceRecords': [{'Value': self.value}], 'SetIdentifier': self.identifer, @@ -396,6 +395,11 @@ class _Route53DynamicValue(_Route53Record): } } + if self.health_check_id: + ret['ResourceRecordSet']['HealthCheckId'] = self.health_check_id + + return ret + def __hash__(self): return f'{self.fqdn}:{self._type}:{self.identifer}'.__hash__() @@ -923,18 +927,15 @@ class Route53Provider(BaseProvider): health_check_id = rrset.get('HealthCheckId', None) health_check = self.health_checks[health_check_id] health_check_config = health_check['HealthCheckConfig'] - if health_check_config['Disabled']: - if health_check_config['Inverted']: - # disabled and inverted means down - status = 'down' - else: - # disabled means always up - status = 'up' + if health_check_config['Disabled'] and \ + health_check_config['Inverted']: + # disabled and inverted means down + status = 'down' else: # otherwise obey status = 'obey' except KeyError: - # No healthcheck implies status is always up + # No healthcheck means status is up status = 'up' pools[pool_name]['values'].append({ 'status': status, @@ -1152,6 +1153,11 @@ class Route53Provider(BaseProvider): self.log.debug('get_health_check_id: fqdn=%s, type=%s, value=%s, ' 'status=%s', fqdn, record._type, value, status) + if status == 'up': + # status up means no health check + self.log.debug('get_health_check_id: status up, no health check') + return None + try: ip_address(str(value)) # We're working with an IP, host is the Host header @@ -1166,13 +1172,10 @@ class Route53Provider(BaseProvider): healthcheck_port = record.healthcheck_port healthcheck_latency = self._healthcheck_measure_latency(record) healthcheck_interval = self._healthcheck_request_interval(record) - if status == 'up': - healthcheck_disabled = True - healthcheck_inverted = False - elif status == 'down': + if status == 'down': healthcheck_disabled = True healthcheck_inverted = True - else: + else: # obey healthcheck_disabled = False healthcheck_inverted = False diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index cf4e64e..01cdce3 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -58,7 +58,6 @@ dynamic_rrsets = [{ 'Type': 'A', 'Weight': 2 }, { - 'HealthCheckId': '09', 'Name': '_octodns-ap-southeast-1-value.unit.tests.', 'ResourceRecords': [{'Value': '1.4.1.2'}], 'SetIdentifier': 'ap-southeast-1-001', @@ -199,12 +198,6 @@ dynamic_health_checks = { 'Inverted': False, } }, - '09': { - 'HealthCheckConfig': { - 'Disabled': True, - 'Inverted': False, - } - }, 'ab': { 'HealthCheckConfig': { 'Disabled': True, @@ -1368,7 +1361,7 @@ class TestRoute53Provider(TestCase): self.assertEquals('42', provider.get_health_check_id(record, '1.1.1.1', 'obey', False)) - self.assertEquals('43', + self.assertEquals(None, provider.get_health_check_id(record, '2.2.2.2', 'up', False)) self.assertEquals('44', @@ -3129,7 +3122,7 @@ class TestRoute53Records(TestCase): # If we don't provide the candidate rrsets we get back exactly what we # put in minus the healthcheck - rrset['HealthCheckId'] = None + del rrset['HealthCheckId'] mod = geo.mod('DELETE', []) self.assertEquals(rrset, mod['ResourceRecordSet']) From ad17e4cbe8c1bb5fcae25cc7967e5419fe0a6e32 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Tue, 14 Dec 2021 13:11:44 -0800 Subject: [PATCH 5/5] Update Route53 extra change to handle status=up case --- octodns/provider/route53.py | 24 +++++++++++++-- tests/test_octodns_provider_route53.py | 42 ++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 3ac9874..3359f3e 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -1338,10 +1338,11 @@ class Route53Provider(BaseProvider): self._gc_health_checks(change.existing, []) return self._gen_mods('DELETE', existing_records, existing_rrsets) - def _extra_changes_update_needed(self, record, rrset): + def _extra_changes_update_needed(self, record, rrset, statuses={}): + value = rrset['ResourceRecords'][0]['Value'] if record._type == 'CNAME': # For CNAME, healthcheck host by default points to the CNAME value - healthcheck_host = rrset['ResourceRecords'][0]['Value'] + healthcheck_host = value else: healthcheck_host = record.healthcheck_host() @@ -1351,6 +1352,17 @@ class Route53Provider(BaseProvider): healthcheck_latency = self._healthcheck_measure_latency(record) healthcheck_interval = self._healthcheck_request_interval(record) + status = statuses.get(value, 'obey') + if status == 'up': + if 'HealthCheckId' in rrset: + self.log.info('_extra_changes_update_needed: health-check ' + 'found for status="up", causing update of %s:%s', + record.fqdn, record._type) + return True + else: + # No health check needed + return False + try: health_check_id = rrset['HealthCheckId'] health_check = self.health_checks[health_check_id] @@ -1406,6 +1418,12 @@ class Route53Provider(BaseProvider): fqdn = record.fqdn _type = record._type + # map values to statuses + statuses = {} + for pool in record.dynamic.pools.values(): + for value in pool.data['values']: + statuses[value['value']] = value.get('status', 'obey') + # loop through all the r53 rrsets for rrset in self._load_records(zone_id): name = rrset['Name'] @@ -1424,7 +1442,7 @@ class Route53Provider(BaseProvider): # rrset isn't for the current record continue - if self._extra_changes_update_needed(record, rrset): + if self._extra_changes_update_needed(record, rrset, statuses): # no good, doesn't have the right health check, needs an update self.log.info('_extra_changes_dynamic_needs_update: ' 'health-check caused update of %s:%s', diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index 01cdce3..680c9ce 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -2567,6 +2567,48 @@ class TestRoute53Provider(TestCase): self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() + def test_extra_change_dyamic_status_up(self): + provider, stubber = self._get_stubbed_provider() + + zone = Zone('unit.tests.', []) + record = Record.new(zone, 'a', { + 'ttl': 30, + 'type': 'A', + 'value': '1.1.1.1', + 'dynamic': { + 'pools': { + 'one': { + 'values': [{ + 'status': 'up', + 'value': '1.2.3.4', + }], + }, + }, + 'rules': [{ + 'pool': 'one', + }], + }, + }) + + # status up and no health check so we're good + rrset = { + 'ResourceRecords': [{'Value': '1.2.3.4'}], + } + statuses = {'1.2.3.4': 'up'} + self.assertFalse( + provider._extra_changes_update_needed(record, rrset, statuses) + ) + + # status up and has a health check so update needed + rrset = { + 'ResourceRecords': [{'Value': '1.2.3.4'}], + 'HealthCheckId': 'foo', + } + statuses = {'1.2.3.4': 'up'} + self.assertTrue( + provider._extra_changes_update_needed(record, rrset, statuses) + ) + def test_extra_change_dynamic_has_health_check_cname(self): provider, stubber = self._get_stubbed_provider()