From b02d5d0a2de8ff61271458da9d570283e325aff3 Mon Sep 17 00:00:00 2001 From: Tom Kaminski Date: Mon, 17 May 2021 19:29:10 -0500 Subject: [PATCH 1/2] Do not trigger change for health checks on cname dynamic records --- octodns/provider/route53.py | 11 +- tests/test_octodns_provider_route53.py | 157 +++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 1 deletion(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index 0d5bab9..a3b3a57 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -1253,7 +1253,16 @@ class Route53Provider(BaseProvider): return self._gen_mods('DELETE', existing_records, existing_rrsets) def _extra_changes_update_needed(self, record, rrset): - healthcheck_host = record.healthcheck_host + value = rrset['ResourceRecords'][0]['Value'] + + try: + ip_address(text_type(value)) + # We're working with an IP + healthcheck_host = record.healthcheck_host + except (AddressValueError, ValueError): + # This isn't an IP, host is the value + healthcheck_host = value + healthcheck_path = record.healthcheck_path healthcheck_protocol = record.healthcheck_protocol healthcheck_port = record.healthcheck_port diff --git a/tests/test_octodns_provider_route53.py b/tests/test_octodns_provider_route53.py index a2b61e7..1e7210d 100644 --- a/tests/test_octodns_provider_route53.py +++ b/tests/test_octodns_provider_route53.py @@ -1962,6 +1962,163 @@ class TestRoute53Provider(TestCase): self.assertEquals(1, len(extra)) stubber.assert_no_pending_responses() + def test_extra_change_dynamic_has_health_check_cname(self): + provider, stubber = self._get_stubbed_provider() + + list_hosted_zones_resp = { + 'HostedZones': [{ + 'Name': 'unit.tests.', + 'Id': 'z42', + 'CallerReference': 'abc', + }], + 'Marker': 'm', + 'IsTruncated': False, + 'MaxItems': '100', + } + stubber.add_response('list_hosted_zones', list_hosted_zones_resp, {}) + + # record with geo and no health check returns change + desired = Zone('unit.tests.', []) + record = Record.new(desired, 'cname', { + 'ttl': 30, + 'type': 'CNAME', + 'value': 'cname.unit.tests.', + 'dynamic': { + 'pools': { + 'one': { + 'values': [{ + 'value': 'one.cname.unit.tests.', + }], + }, + }, + 'rules': [{ + 'pool': 'one', + }], + }, + }) + desired.add_record(record) + list_resource_record_sets_resp = { + 'ResourceRecordSets': [{ + # Not dynamic value and other name + 'Name': 'unit.tests.', + 'Type': 'CNAME', + 'GeoLocation': { + 'CountryCode': '*', + }, + 'ResourceRecords': [{ + 'Value': 'cname.unit.tests.', + }], + 'TTL': 61, + # All the non-matches have a different Id so we'll fail if they + # match + 'HealthCheckId': '33', + }, { + # Not dynamic value, matching name, other type + 'Name': 'cname.unit.tests.', + 'Type': 'AAAA', + 'ResourceRecords': [{ + 'Value': '2001:0db8:3c4d:0015:0000:0000:1a2f:1a4b' + }], + 'TTL': 61, + 'HealthCheckId': '33', + }, { + # default value pool + 'Name': '_octodns-default-value.cname.unit.tests.', + 'Type': 'CNAME', + 'GeoLocation': { + 'CountryCode': '*', + }, + 'ResourceRecords': [{ + 'Value': 'cname.unit.tests.', + }], + 'TTL': 61, + 'HealthCheckId': '33', + }, { + # different record + 'Name': '_octodns-two-value.other.unit.tests.', + 'Type': 'CNAME', + 'GeoLocation': { + 'CountryCode': '*', + }, + 'ResourceRecords': [{ + 'Value': 'cname.unit.tests.', + }], + 'TTL': 61, + 'HealthCheckId': '33', + }, { + # same everything, but different type + 'Name': '_octodns-one-value.cname.unit.tests.', + 'Type': 'AAAA', + 'ResourceRecords': [{ + 'Value': '2001:0db8:3c4d:0015:0000:0000:1a2f:1a4b' + }], + 'TTL': 61, + 'HealthCheckId': '33', + }, { + # same everything, sub + 'Name': '_octodns-one-value.sub.cname.unit.tests.', + 'Type': 'CNAME', + 'ResourceRecords': [{ + 'Value': 'cname.unit.tests.', + }], + 'TTL': 61, + 'HealthCheckId': '33', + }, { + # match + 'Name': '_octodns-one-value.cname.unit.tests.', + 'Type': 'CNAME', + 'ResourceRecords': [{ + 'Value': 'one.cname.unit.tests.', + }], + 'TTL': 61, + 'HealthCheckId': '42', + }], + 'IsTruncated': False, + 'MaxItems': '100', + } + stubber.add_response('list_resource_record_sets', + list_resource_record_sets_resp, + {'HostedZoneId': 'z42'}) + + stubber.add_response('list_health_checks', { + 'HealthChecks': [{ + 'Id': '42', + 'CallerReference': self.caller_ref, + 'HealthCheckConfig': { + 'Type': 'HTTPS', + 'FullyQualifiedDomainName': 'one.cname.unit.tests.', + 'ResourcePath': '/_dns', + 'Type': 'HTTPS', + 'Port': 443, + 'MeasureLatency': True, + 'RequestInterval': 10, + }, + 'HealthCheckVersion': 2, + }], + 'IsTruncated': False, + 'MaxItems': '100', + 'Marker': '', + }) + extra = provider._extra_changes(desired=desired, changes=[]) + 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(desired=desired, changes=[]) + self.assertEquals(1, len(extra)) + stubber.assert_no_pending_responses() + + # no change b/c healthcheck host ignored for dynamic cname + record._octodns['healthcheck'] = { + 'host': 'foo.bar.io' + } + extra = provider._extra_changes(desired=desired, changes=[]) + self.assertEquals(0, len(extra)) + stubber.assert_no_pending_responses() + def _get_test_plan(self, max_changes): provider = Route53Provider('test', 'abc', '123', max_changes) From 62122e442931cf30f7b98cd91280419a3c58fac7 Mon Sep 17 00:00:00 2001 From: Tom Kaminski Date: Thu, 20 May 2021 15:24:34 -0500 Subject: [PATCH 2/2] More explicit check for CNAME records when checking for extra updates --- octodns/provider/route53.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/octodns/provider/route53.py b/octodns/provider/route53.py index a3b3a57..ad59eb0 100644 --- a/octodns/provider/route53.py +++ b/octodns/provider/route53.py @@ -1253,15 +1253,11 @@ class Route53Provider(BaseProvider): return self._gen_mods('DELETE', existing_records, existing_rrsets) def _extra_changes_update_needed(self, record, rrset): - value = rrset['ResourceRecords'][0]['Value'] - - try: - ip_address(text_type(value)) - # We're working with an IP + if record._type == 'CNAME': + # For CNAME, healthcheck host by default points to the CNAME value + healthcheck_host = rrset['ResourceRecords'][0]['Value'] + else: healthcheck_host = record.healthcheck_host - except (AddressValueError, ValueError): - # This isn't an IP, host is the value - healthcheck_host = value healthcheck_path = record.healthcheck_path healthcheck_protocol = record.healthcheck_protocol