From f00766a77909c216249b72dcb4396ced16909ac2 Mon Sep 17 00:00:00 2001 From: Stan Brinkerhoff Date: Wed, 26 May 2021 22:45:07 -0400 Subject: [PATCH 1/8] Add support for ALIAS type --- octodns/provider/ultra.py | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/octodns/provider/ultra.py b/octodns/provider/ultra.py index 03b70de..f65c519 100644 --- a/octodns/provider/ultra.py +++ b/octodns/provider/ultra.py @@ -36,12 +36,12 @@ class UltraProvider(BaseProvider): ''' Neustar UltraDNS provider - Documentation for Ultra REST API requires a login: - https://portal.ultradns.com/static/docs/REST-API_User_Guide.pdf - Implemented to the May 20, 2020 version of the document (dated on page ii) - Also described as Version 2.83.0 (title page) + Documentation for Ultra REST API: + https://ultra-portalstatic.ultradns.com/static/docs/REST-API_User_Guide.pdf + Implemented to the May 26, 2021 version of the document (dated on page ii) + Also described as Version 3.18.0 (title page) - Tested against 3.0.0-20200627220036.81047f5 + Tested against 3.20.1-20210521075351.36b9297 As determined by querying https://api.ultradns.com/version ultra: @@ -57,6 +57,7 @@ class UltraProvider(BaseProvider): RECORDS_TO_TYPE = { 'A (1)': 'A', 'AAAA (28)': 'AAAA', + 'APEXALIAS (65282)': 'ALIAS', 'CAA (257)': 'CAA', 'CNAME (5)': 'CNAME', 'MX (15)': 'MX', @@ -72,6 +73,7 @@ class UltraProvider(BaseProvider): SUPPORTS_GEO = False SUPPORTS_DYNAMIC = False TIMEOUT = 5 + ZONE_REQUEST_LIMIT = 1000 def _request(self, method, path, params=None, data=None, json=None, json_response=True): @@ -151,7 +153,7 @@ class UltraProvider(BaseProvider): def zones(self): if self._zones is None: offset = 0 - limit = 100 + limit = self.ZONE_REQUEST_LIMIT zones = [] paging = True while paging: @@ -401,8 +403,15 @@ class UltraProvider(BaseProvider): def _gen_data(self, record): zone_name = self._remove_prefix(record.fqdn, record.name + '.') + + # UltraDNS treats the `APEXALIAS` type as the octodns `ALIAS`. + if record._type == "ALIAS": + record_type = "APEXALIAS" + else: + record_type = record._type + path = '/v2/zones/{}/rrsets/{}/{}'.format(zone_name, - record._type, + record_type, record.fqdn) contents_for = getattr(self, '_contents_for_{}'.format(record._type)) return path, contents_for(record) @@ -444,7 +453,12 @@ class UltraProvider(BaseProvider): existing._type == self.RECORDS_TO_TYPE[record['rrtype']]: zone_name = self._remove_prefix(existing.fqdn, existing.name + '.') - path = '/v2/zones/{}/rrsets/{}/{}'.format(zone_name, + + # UltraDNS treats the `APEXALIAS` type as the octodns `ALIAS`. + if existing._type == "ALIAS": + existing._type = "APEXALIAS" + + path = '/v2/zones/{}/rrsets/{}/{ }'.format(zone_name, existing._type, existing.fqdn) self._delete(path, json_response=False) From 4b6fd8b4a1e19fe889943aab705ecf23b6c19495 Mon Sep 17 00:00:00 2001 From: sbrinkerhoff Date: Thu, 27 May 2021 17:59:10 -0400 Subject: [PATCH 2/8] Remove type mutation Co-authored-by: Ross McFarland --- octodns/provider/ultra.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/octodns/provider/ultra.py b/octodns/provider/ultra.py index f65c519..a2eae3c 100644 --- a/octodns/provider/ultra.py +++ b/octodns/provider/ultra.py @@ -455,10 +455,11 @@ class UltraProvider(BaseProvider): existing.name + '.') # UltraDNS treats the `APEXALIAS` type as the octodns `ALIAS`. - if existing._type == "ALIAS": - existing._type = "APEXALIAS" + existing_type = existing._type + if existing_type == "ALIAS": + existing_type = "APEXALIAS" path = '/v2/zones/{}/rrsets/{}/{ }'.format(zone_name, - existing._type, + existing_type, existing.fqdn) self._delete(path, json_response=False) From e816e8e49b6c467cdf6a911b9d0a27bd2c8424e6 Mon Sep 17 00:00:00 2001 From: Ross McFarland Date: Thu, 27 May 2021 15:19:33 -0700 Subject: [PATCH 3/8] Fixed extraneous whitespace in format in UltraDNS --- octodns/provider/ultra.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/octodns/provider/ultra.py b/octodns/provider/ultra.py index a2eae3c..55c2c34 100644 --- a/octodns/provider/ultra.py +++ b/octodns/provider/ultra.py @@ -459,7 +459,7 @@ class UltraProvider(BaseProvider): if existing_type == "ALIAS": existing_type = "APEXALIAS" - path = '/v2/zones/{}/rrsets/{}/{ }'.format(zone_name, + path = '/v2/zones/{}/rrsets/{}/{}'.format(zone_name, existing_type, existing.fqdn) self._delete(path, json_response=False) From 60f916cd2400a27f23fb365f90978e6968cc088a Mon Sep 17 00:00:00 2001 From: Stan Brinkerhoff Date: Fri, 28 May 2021 00:52:48 -0400 Subject: [PATCH 4/8] add contents for alias method, test for alias --- octodns/provider/ultra.py | 4 +++- tests/test_octodns_provider_ultra.py | 15 ++++++++++++--- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/octodns/provider/ultra.py b/octodns/provider/ultra.py index 55c2c34..bc855d4 100644 --- a/octodns/provider/ultra.py +++ b/octodns/provider/ultra.py @@ -73,7 +73,7 @@ class UltraProvider(BaseProvider): SUPPORTS_GEO = False SUPPORTS_DYNAMIC = False TIMEOUT = 5 - ZONE_REQUEST_LIMIT = 1000 + ZONE_REQUEST_LIMIT = 100 def _request(self, method, path, params=None, data=None, json=None, json_response=True): @@ -213,6 +213,7 @@ class UltraProvider(BaseProvider): _data_for_PTR = _data_for_single _data_for_CNAME = _data_for_single + _data_for_ALIAS = _data_for_single def _data_for_CAA(self, _type, records): return { @@ -376,6 +377,7 @@ class UltraProvider(BaseProvider): } _contents_for_PTR = _contents_for_CNAME + _contents_for_ALIAS = _contents_for_CNAME def _contents_for_SRV(self, record): return { diff --git a/tests/test_octodns_provider_ultra.py b/tests/test_octodns_provider_ultra.py index b6d1017..52e0307 100644 --- a/tests/test_octodns_provider_ultra.py +++ b/tests/test_octodns_provider_ultra.py @@ -274,7 +274,7 @@ class TestUltraProvider(TestCase): self.assertTrue(provider.populate(zone)) self.assertEquals('octodns1.test.', zone.name) - self.assertEquals(11, len(zone.records)) + self.assertEquals(12, len(zone.records)) self.assertEquals(4, mock.call_count) def test_apply(self): @@ -352,8 +352,8 @@ class TestUltraProvider(TestCase): })) plan = provider.plan(wanted) - self.assertEquals(10, len(plan.changes)) - self.assertEquals(10, provider.apply(plan)) + self.assertEquals(11, len(plan.changes)) + self.assertEquals(11, provider.apply(plan)) self.assertTrue(plan.exists) provider._request.assert_has_calls([ @@ -492,6 +492,15 @@ class TestUltraProvider(TestCase): Record.new(zone, 'txt', {'ttl': 60, 'type': 'TXT', 'values': ['abc', 'def']})), + + # ALIAS + ('', 'ALIAS', + '/v2/zones/unit.tests./rrsets/APEXALIAS/unit.tests.', + {'ttl': 60, 'rdata': ['target.unit.tests.']}, + Record.new(zone, '', + {'ttl': 60, 'type': 'ALIAS', + 'value': 'target.unit.tests.'})), + ): # Validate path and payload based on record meet expectations path, payload = provider._gen_data(expected_record) From 6e71f2df76d454578bac1a84bf8920058350743f Mon Sep 17 00:00:00 2001 From: Stan Brinkerhoff Date: Fri, 28 May 2021 00:54:57 -0400 Subject: [PATCH 5/8] update test record --- tests/fixtures/ultra-records-page-2.json | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/fixtures/ultra-records-page-2.json b/tests/fixtures/ultra-records-page-2.json index abdc44f..274d95e 100644 --- a/tests/fixtures/ultra-records-page-2.json +++ b/tests/fixtures/ultra-records-page-2.json @@ -32,7 +32,16 @@ "rdata": [ "www.octodns1.test." ] + }, + { + "ownerName": "host1.octodns1.test.", + "rrtype": "RRSET (70)", + "ttl": 3600, + "rdata": [ + "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855" + ] } + ], "resultInfo": { "totalCount": 13, From e97675fe3ddaa02ec4baa145fc7254f9b616fc28 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 28 May 2021 12:48:09 -0700 Subject: [PATCH 6/8] check dns config when comparing profiles for equality --- octodns/provider/azuredns.py | 10 +++++++++- tests/test_octodns_provider_azuredns.py | 20 +++++++++++++------- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/octodns/provider/azuredns.py b/octodns/provider/azuredns.py index 8eeba02..65d9967 100644 --- a/octodns/provider/azuredns.py +++ b/octodns/provider/azuredns.py @@ -284,10 +284,18 @@ def _profile_is_match(have, desired): # compare basic attributes if have.name != desired.name or \ have.traffic_routing_method != desired.traffic_routing_method or \ - have.dns_config.ttl != desired.dns_config.ttl or \ len(have.endpoints) != len(desired.endpoints): return False + # compare dns config + dns_have = have.dns_config + dns_desired = desired.dns_config + if dns_have.ttl != dns_desired.ttl or \ + dns_have.relative_name is None or \ + dns_desired.relative_name is None or \ + dns_have.relative_name != dns_desired.relative_name: + return False + # compare monitoring configuration monitor_have = have.monitor_config monitor_desired = desired.monitor_config diff --git a/tests/test_octodns_provider_azuredns.py b/tests/test_octodns_provider_azuredns.py index e8e5281..0a1c366 100644 --- a/tests/test_octodns_provider_azuredns.py +++ b/tests/test_octodns_provider_azuredns.py @@ -453,6 +453,7 @@ class Test_ProfileIsMatch(TestCase): name = 'foo-unit-tests', ttl = 60, method = 'Geographic', + dns_name = None, monitor_proto = 'HTTPS', monitor_port = 4443, monitor_path = '/_ping', @@ -465,7 +466,7 @@ class Test_ProfileIsMatch(TestCase): weight = 1, priority = 1, ): - dns = DnsConfig(ttl=ttl) + dns = DnsConfig(relative_name=(dns_name or name), ttl=ttl) return Profile( name=name, traffic_routing_method=method, dns_config=dns, monitor_config=MonitorConfig( @@ -488,6 +489,7 @@ class Test_ProfileIsMatch(TestCase): self.assertFalse(is_match(profile(), profile(name='two'))) self.assertFalse(is_match(profile(), profile(endpoints=2))) + self.assertFalse(is_match(profile(), profile(dns_name='two'))) self.assertFalse(is_match(profile(), profile(monitor_proto='HTTP'))) self.assertFalse(is_match(profile(), profile(endpoint_name='a'))) self.assertFalse(is_match(profile(), profile(endpoint_type='b'))) @@ -596,7 +598,6 @@ class TestAzureDnsProvider(TestCase): id_format = base_id + '{}--' + suffix name_format = '{}--' + suffix - dns = DnsConfig(ttl=60) header = MonitorConfigCustomHeadersItem(name='Host', value='foo.unit.tests') monitor = MonitorConfig(protocol='HTTPS', port=4443, path='/_ping', @@ -604,12 +605,12 @@ class TestAzureDnsProvider(TestCase): external = 'Microsoft.Network/trafficManagerProfiles/externalEndpoints' nested = 'Microsoft.Network/trafficManagerProfiles/nestedEndpoints' - return [ + profiles = [ Profile( id=id_format.format('pool-two'), name=name_format.format('pool-two'), traffic_routing_method='Weighted', - dns_config=dns, + dns_config=DnsConfig(ttl=60), monitor_config=monitor, endpoints=[ Endpoint( @@ -630,7 +631,7 @@ class TestAzureDnsProvider(TestCase): id=id_format.format('rule-one'), name=name_format.format('rule-one'), traffic_routing_method='Priority', - dns_config=dns, + dns_config=DnsConfig(ttl=60), monitor_config=monitor, endpoints=[ Endpoint( @@ -663,7 +664,7 @@ class TestAzureDnsProvider(TestCase): id=id_format.format('rule-two'), name=name_format.format('rule-two'), traffic_routing_method='Priority', - dns_config=dns, + dns_config=DnsConfig(ttl=60), monitor_config=monitor, endpoints=[ Endpoint( @@ -690,7 +691,7 @@ class TestAzureDnsProvider(TestCase): id=base_id + suffix, name=suffix, traffic_routing_method='Geographic', - dns_config=dns, + dns_config=DnsConfig(ttl=60), monitor_config=monitor, endpoints=[ Endpoint( @@ -709,6 +710,11 @@ class TestAzureDnsProvider(TestCase): ), ] + for profile in profiles: + profile.dns_config.relative_name = profile.name + + return profiles + def _get_dynamic_package(self): '''Convenience function to setup a sample dynamic record. ''' From 18c0e5675978507063178cf9a83622ae25fe054b Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 28 May 2021 12:59:27 -0700 Subject: [PATCH 7/8] log.debug the reason for profile mismatch --- octodns/provider/azuredns.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/octodns/provider/azuredns.py b/octodns/provider/azuredns.py index 65d9967..689088c 100644 --- a/octodns/provider/azuredns.py +++ b/octodns/provider/azuredns.py @@ -281,11 +281,20 @@ def _profile_is_match(have, desired): if have is None or desired is None: return False + log = logging.getLogger('azuredns._profile_is_match').debug + + def false(have, desired, name=None): + prefix = 'profile={}'.format(name) if name else '' + attr = have.__class__.__name__ + log('%s have.%s = %s', prefix, attr, have) + log('%s desired.%s = %s', prefix, attr, desired) + return False + # compare basic attributes if have.name != desired.name or \ have.traffic_routing_method != desired.traffic_routing_method or \ len(have.endpoints) != len(desired.endpoints): - return False + return false(have, desired) # compare dns config dns_have = have.dns_config @@ -294,7 +303,7 @@ def _profile_is_match(have, desired): dns_have.relative_name is None or \ dns_desired.relative_name is None or \ dns_have.relative_name != dns_desired.relative_name: - return False + return false(dns_have, dns_desired, have.name) # compare monitoring configuration monitor_have = have.monitor_config @@ -303,7 +312,7 @@ def _profile_is_match(have, desired): monitor_have.port != monitor_desired.port or \ monitor_have.path != monitor_desired.path or \ monitor_have.custom_headers != monitor_desired.custom_headers: - return False + return false(monitor_have, monitor_desired, have.name) # compare endpoints method = have.traffic_routing_method @@ -321,26 +330,26 @@ def _profile_is_match(have, desired): for have_endpoint, desired_endpoint in endpoints: if have_endpoint.name != desired_endpoint.name or \ have_endpoint.type != desired_endpoint.type: - return False + return false(have_endpoint, desired_endpoint, have.name) target_type = have_endpoint.type.split('/')[-1] if target_type == 'externalEndpoints': # compare value, weight, priority if have_endpoint.target != desired_endpoint.target: - return False + return false(have_endpoint, desired_endpoint, have.name) if method == 'Weighted' and \ have_endpoint.weight != desired_endpoint.weight: - return False + return false(have_endpoint, desired_endpoint, have.name) elif target_type == 'nestedEndpoints': # compare targets if have_endpoint.target_resource_id != \ desired_endpoint.target_resource_id: - return False + return false(have_endpoint, desired_endpoint, have.name) # compare geos if method == 'Geographic': have_geos = sorted(have_endpoint.geo_mapping) desired_geos = sorted(desired_endpoint.geo_mapping) if have_geos != desired_geos: - return False + return false(have_endpoint, desired_endpoint, have.name) else: # unexpected, give up return False From aaffdb1388a0569710ca5f0063236d977fdeb9f8 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 28 May 2021 13:23:35 -0700 Subject: [PATCH 8/8] minimize Azure Traffic Manager hops --- octodns/provider/azuredns.py | 253 ++++++---- tests/test_octodns_provider_azuredns.py | 585 ++++++++++++++++++------ 2 files changed, 622 insertions(+), 216 deletions(-) diff --git a/octodns/provider/azuredns.py b/octodns/provider/azuredns.py index 689088c..777e15b 100644 --- a/octodns/provider/azuredns.py +++ b/octodns/provider/azuredns.py @@ -646,14 +646,23 @@ class AzureProvider(BaseProvider): pools = defaultdict(lambda: {'fallback': None, 'values': []}) rules = [] - # top level geo profile - geo_profile = self._get_tm_profile_by_id(azrecord.target_resource.id) - for geo_ep in geo_profile.endpoints: + # top level profile + root_profile = self._get_tm_profile_by_id(azrecord.target_resource.id) + if root_profile.traffic_routing_method != 'Geographic': + # This record does not use geo fencing, so we skip the Geographic + # profile hop; let's pretend to be a geo-profile's only endpoint + geo_ep = Endpoint(target_resource_id=root_profile.id) + geo_ep.target_resource = root_profile + endpoints = [geo_ep] + else: + endpoints = root_profile.endpoints + + for geo_ep in endpoints: rule = {} # resolve list of regions - geo_map = list(geo_ep.geo_mapping) - if geo_map != ['WORLD']: + geo_map = list(geo_ep.geo_mapping or []) + if geo_map and geo_map != ['WORLD']: if 'GEO-ME' in geo_map: # Azure treats Middle East as a separate group, but # its part of Asia in octoDNS, so we need to remove GEO-ME @@ -662,7 +671,7 @@ class AzureProvider(BaseProvider): # profile was generated by octoDNS if 'GEO-AS' not in geo_map: msg = 'Profile={} for record {}: '.format( - geo_profile.name, azrecord.fqdn) + root_profile.name, azrecord.fqdn) msg += 'Middle East (GEO-ME) is not supported by ' + \ 'octoDNS. It needs to be either paired ' + \ 'with Asia (GEO-AS) or expanded into ' + \ @@ -690,18 +699,35 @@ class AzureProvider(BaseProvider): # country geos.append(GeoCodes.country_to_code(code)) - # second level priority profile + # build fallback chain from second level priority profile + if geo_ep.target_resource_id: + target = geo_ep.target_resource + if target.traffic_routing_method == 'Priority': + rule_endpoints = target.endpoints + rule_endpoints.sort(key=lambda e: e.priority) + else: + # Weighted + geo_ep.name = target.endpoints[0].name.split('--', 1)[0] + rule_endpoints = [geo_ep] + else: + # this geo directly points to the default, so we skip the + # Priority profile hop and directly use an external endpoint; + # let's pretend to be a Priority profile's only endpoint + rule_endpoints = [geo_ep] + pool = None - rule_endpoints = geo_ep.target_resource.endpoints - rule_endpoints = sorted(rule_endpoints, key=lambda e: e.priority) for rule_ep in rule_endpoints: pool_name = rule_ep.name # last/default pool - if pool_name == '--default--': + if pool_name.endswith('--default--'): default.add(rule_ep.target) - # this should be the last one, so let's break here - break + if pool_name == '--default--': + # this should be the last one, so let's break here + break + # last pool is a single value pool and its value is same + # as record's default value + pool_name = pool_name[:-len('--default--')] # set first priority endpoint as the rule's primary pool if 'pool' not in rule: @@ -711,32 +737,32 @@ class AzureProvider(BaseProvider): # set current pool as fallback of the previous pool pool['fallback'] = pool_name + if pool_name in pools: + # we've already populated the pool + continue + + # populate the pool from Weighted profile + # these should be leaf node entries with no further nesting pool = pools[pool_name] endpoints = [] - # these should be leaf node entries with no further nesting + if rule_ep.target_resource_id: # third (and last) level weighted RR profile endpoints = rule_ep.target_resource.endpoints else: - # single-value pool + # single-value pool, so we skip the Weighted profile hop + # and directly use an external endpoint; let's pretend to + # be a Weighted profile's only endpoint endpoints = [rule_ep] for pool_ep in endpoints: val = pool_ep.target - value_dict = { + pool['values'].append({ 'value': _check_endswith_dot(val), 'weight': pool_ep.weight or 1, - } - if value_dict not in pool['values']: - pool['values'].append(value_dict) - - if 'pool' not in rule or not default: - # this will happen if the priority profile does not have - # enough endpoints - msg = 'Expected at least 2 endpoints in {}, got {}'.format( - geo_ep.target_resource.name, len(rule_endpoints) - ) - raise AzureException(msg) + }) + if pool_ep.name.endswith('--default--'): + default.add(val) rules.append(rule) @@ -809,7 +835,7 @@ class AzureProvider(BaseProvider): elif ep.target: ep.type = endpoint_type_prefix + 'externalEndpoints' else: - msg = ('Invalid endpoint {} in profile {}, needs to have' + + msg = ('Invalid endpoint {} in profile {}, needs to have ' + 'either target or target_resource_id').format( ep.name, name) raise AzureException(msg) @@ -828,39 +854,83 @@ class AzureProvider(BaseProvider): location='global', ) + def _update_tm_name(self, profile, new_name): + profile.name = new_name + profile.id = self._profile_name_to_id(new_name) + profile.dns_config.relative_name = new_name + + return profile + def _generate_traffic_managers(self, record): traffic_managers = [] pools = record.dynamic.pools + default = record.value[:-1] tm_suffix = _traffic_manager_suffix(record) profile = self._generate_tm_profile geo_endpoints = [] + pool_profiles = {} for rule in record.dynamic.rules: + # Prepare the list of Traffic manager geos + rule_geos = rule.data.get('geos', []) + geos = [] + for geo in rule_geos: + if '-' in geo: + # country/state + geos.append(geo.split('-', 1)[-1]) + else: + # continent + if geo == 'AS': + # Middle East is part of Asia in octoDNS, but + # Azure treats it as a separate "group", so let's + # add it in the list of geo mappings. We will drop + # it when we later parse the list of regions. + geos.append('GEO-ME') + elif geo == 'OC': + # Azure uses Australia/Pacific (AP) instead of + # Oceania + geo = 'AP' + + geos.append('GEO-{}'.format(geo)) + if not geos: + geos.append('WORLD') + pool_name = rule.data['pool'] rule_endpoints = [] priority = 1 + default_seen = False while pool_name: # iterate until we reach end of fallback chain + default_seen = False pool = pools[pool_name].data - profile_name = 'pool-{}--{}'.format(pool_name, tm_suffix) if len(pool['values']) > 1: # create Weighted profile for multi-value pool - endpoints = [] - for val in pool['values']: - target = val['value'] - # strip trailing dot from CNAME value - target = target[:-1] - endpoints.append(Endpoint( - name=target, - target=target, - weight=val.get('weight', 1), - )) - pool_profile = profile(profile_name, 'Weighted', endpoints, - record) - traffic_managers.append(pool_profile) + pool_profile = pool_profiles.get(pool_name) + if pool_profile is None: + endpoints = [] + for val in pool['values']: + target = val['value'] + # strip trailing dot from CNAME value + target = target[:-1] + ep_name = '{}--{}'.format(pool_name, target) + if target == default: + # mark default + ep_name += '--default--' + default_seen = True + endpoints.append(Endpoint( + name=ep_name, + target=target, + weight=val.get('weight', 1), + )) + profile_name = 'pool-{}--{}'.format( + pool_name, tm_suffix) + pool_profile = profile(profile_name, 'Weighted', + endpoints, record) + traffic_managers.append(pool_profile) + pool_profiles[pool_name] = pool_profile # append pool to endpoint list of fallback rule profile rule_endpoints.append(Endpoint( @@ -869,8 +939,15 @@ class AzureProvider(BaseProvider): priority=priority, )) else: - # add single-value pool as an external endpoint + # Skip Weighted profile hop for single-value pool + # append its value as an external endpoint to fallback + # rule profile target = pool['values'][0]['value'][:-1] + ep_name = pool_name + if target == default: + # mark default + ep_name += '--default--' + default_seen = True rule_endpoints.append(Endpoint( name=pool_name, target=target, @@ -880,51 +957,61 @@ class AzureProvider(BaseProvider): priority += 1 pool_name = pool.get('fallback') - # append default profile to the end - rule_endpoints.append(Endpoint( - name='--default--', - target=record.value[:-1], - priority=priority, - )) - # create rule profile with fallback chain - rule_profile_name = 'rule-{}--{}'.format(rule.data['pool'], - tm_suffix) - rule_profile = profile(rule_profile_name, 'Priority', - rule_endpoints, record) - traffic_managers.append(rule_profile) + # append default endpoint unless it is already included in + # last pool of rule profile + if not default_seen: + rule_endpoints.append(Endpoint( + name='--default--', + target=default, + priority=priority, + )) - # append rule profile to top-level geo profile - rule_geos = rule.data.get('geos', []) - geos = [] - if len(rule_geos) > 0: - for geo in rule_geos: - if '-' in geo: - # country or state - geos.append(geo.split('-', 1)[-1]) - else: - # continent - if geo == 'AS': - # Middle East is part of Asia in octoDNS, but - # Azure treats it as a separate "group", so let's - # add it in the list of geo mappings. We will drop - # it when we later parse the list of regions. - geos.append('GEO-ME') - elif geo == 'OC': - # Azure uses Australia/Pacific (AP) instead of - # Oceania - geo = 'AP' + if len(rule_endpoints) > 1: + # create rule profile with fallback chain + rule_profile_name = 'rule-{}--{}'.format( + rule.data['pool'], tm_suffix) + rule_profile = profile(rule_profile_name, 'Priority', + rule_endpoints, record) + traffic_managers.append(rule_profile) - geos.append('GEO-{}'.format(geo)) + # append rule profile to top-level geo profile + geo_endpoints.append(Endpoint( + name='rule-{}'.format(rule.data['pool']), + target_resource_id=rule_profile.id, + geo_mapping=geos, + )) else: - geos.append('WORLD') - geo_endpoints.append(Endpoint( - name='rule-{}'.format(rule.data['pool']), - target_resource_id=rule_profile.id, - geo_mapping=geos, - )) + # Priority profile has only one endpoint; skip the hop and + # append its only endpoint to the top-level profile + rule_ep = rule_endpoints[0] + if rule_ep.target_resource_id: + # point directly to the Weighted pool profile + geo_endpoints.append(Endpoint( + name='rule-{}'.format(rule.data['pool']), + target_resource_id=rule_ep.target_resource_id, + geo_mapping=geos, + )) + else: + # just add the value of single-value pool + geo_endpoints.append(Endpoint( + name=rule_ep.name + '--default--', + target=rule_ep.target, + geo_mapping=geos, + )) - geo_profile = profile(tm_suffix, 'Geographic', geo_endpoints, record) - traffic_managers.append(geo_profile) + if len(geo_endpoints) == 1 and \ + geo_endpoints[0].geo_mapping == ['WORLD'] and \ + geo_endpoints[0].target_resource_id: + # Single WORLD rule does not require a Geographic profile, use + # the target profile as the root profile + target_profile_id = geo_endpoints[0].target_resource_id + profile_map = dict((tm.id, tm) for tm in traffic_managers) + target_profile = profile_map[target_profile_id] + self._update_tm_name(target_profile, tm_suffix) + else: + geo_profile = profile(tm_suffix, 'Geographic', geo_endpoints, + record) + traffic_managers.append(geo_profile) return traffic_managers diff --git a/tests/test_octodns_provider_azuredns.py b/tests/test_octodns_provider_azuredns.py index 0a1c366..2c998b6 100644 --- a/tests/test_octodns_provider_azuredns.py +++ b/tests/test_octodns_provider_azuredns.py @@ -614,13 +614,13 @@ class TestAzureDnsProvider(TestCase): monitor_config=monitor, endpoints=[ Endpoint( - name='two1.unit.tests', + name='two--two1.unit.tests', type=external, target='two1.unit.tests', weight=3, ), Endpoint( - name='two2.unit.tests', + name='two--two2.unit.tests', type=external, target='two2.unit.tests', weight=4, @@ -847,121 +847,6 @@ class TestAzureDnsProvider(TestCase): self.assertEquals(len(zone.records), 17) self.assertTrue(exists) - def test_populate_dynamic(self): - # Middle east without Asia raises exception - provider, zone, record = self._get_dynamic_package() - tm_suffix = _traffic_manager_suffix(record) - tm_id = provider._profile_name_to_id - tm_list = provider._tm_client.profiles.list_by_resource_group - rule_name = 'rule-one--{}'.format(tm_suffix) - nested = 'Microsoft.Network/trafficManagerProfiles/nestedEndpoints' - tm_list.return_value = [ - Profile( - id=tm_id(tm_suffix), - name=tm_suffix, - traffic_routing_method='Geographic', - endpoints=[ - Endpoint( - geo_mapping=['GEO-ME'], - ), - ], - ), - ] - azrecord = RecordSet( - ttl=60, - target_resource=SubResource(id=tm_id(tm_suffix)), - ) - azrecord.name = record.name or '@' - azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) - with self.assertRaises(AzureException) as ctx: - provider._populate_record(zone, azrecord) - self.assertTrue(text_type(ctx).startswith( - 'Middle East (GEO-ME) is not supported' - )) - - # empty priority profile raises exception - provider, zone, record = self._get_dynamic_package() - tm_list = provider._tm_client.profiles.list_by_resource_group - rule_name = 'rule-one--{}'.format(tm_suffix) - nested = 'Microsoft.Network/trafficManagerProfiles/nestedEndpoints' - tm_list.return_value = [ - Profile( - id=tm_id(rule_name), - name=rule_name, - traffic_routing_method='Priority', - endpoints=[], - ), - Profile( - id=tm_id(tm_suffix), - name=tm_suffix, - traffic_routing_method='Geographic', - endpoints=[ - Endpoint( - geo_mapping=['WORLD'], - name='rule-one', - type=nested, - target_resource_id=tm_id(rule_name), - ), - ], - ), - ] - with self.assertRaises(AzureException) as ctx: - provider._populate_record(zone, azrecord) - self.assertTrue(text_type(ctx).startswith( - 'Expected at least 2 endpoints' - )) - - # valid set of profiles produce expected dynamic record - provider, zone, record = self._get_dynamic_package() - root_profile_id = provider._profile_name_to_id( - _traffic_manager_suffix(record) - ) - azrecord = RecordSet( - ttl=60, - target_resource=SubResource(id=root_profile_id), - ) - azrecord.name = record.name or '@' - azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) - - record = provider._populate_record(zone, azrecord) - self.assertEqual(record.name, 'foo') - self.assertEqual(record.ttl, 60) - self.assertEqual(record.value, 'default.unit.tests.') - self.assertEqual(record.dynamic._data(), { - 'pools': { - 'one': { - 'values': [ - {'value': 'one.unit.tests.', 'weight': 1}, - ], - 'fallback': 'two', - }, - 'two': { - 'values': [ - {'value': 'two1.unit.tests.', 'weight': 3}, - {'value': 'two2.unit.tests.', 'weight': 4}, - ], - 'fallback': 'three', - }, - 'three': { - 'values': [ - {'value': 'three.unit.tests.', 'weight': 1}, - ], - 'fallback': None, - }, - }, - 'rules': [ - {'geos': ['AF', 'EU-DE', 'NA-US-CA', 'OC'], 'pool': 'one'}, - {'pool': 'two'}, - ], - }) - - # valid profiles with Middle East test case - geo_profile = provider._get_tm_for_dynamic_record(record) - geo_profile.endpoints[0].geo_mapping.extend(['GEO-ME', 'GEO-AS']) - record = provider._populate_record(zone, azrecord) - self.assertIn('AS', record.dynamic.rules[0].data['geos']) - self.assertNotIn('ME', record.dynamic.rules[0].data['geos']) - def test_populate_zone(self): provider = self._get_provider() @@ -1114,6 +999,7 @@ class TestAzureDnsProvider(TestCase): '/providers/Microsoft.Network/trafficManagerProfiles/' + name self.assertEqual(profile.id, expected_id) self.assertEqual(profile.name, name) + self.assertEqual(profile.name, profile.dns_config.relative_name) self.assertEqual(profile.traffic_routing_method, routing) self.assertEqual(profile.dns_config.ttl, record.ttl) self.assertEqual(len(profile.endpoints), len(endpoints)) @@ -1127,33 +1013,451 @@ class TestAzureDnsProvider(TestCase): 'Microsoft.Network/trafficManagerProfiles/nestedEndpoints' ) - def test_generate_traffic_managers(self): + def test_dynamic_record(self): provider, zone, record = self._get_dynamic_package() profiles = provider._generate_traffic_managers(record) - deduped = [] - seen = set() - for profile in profiles: - if profile.name not in seen: - deduped.append(profile) - seen.add(profile.name) # check that every profile is a match with what we expect expected_profiles = self._get_tm_profiles(provider) - self.assertEqual(len(expected_profiles), len(deduped)) - for have, expected in zip(deduped, expected_profiles): + self.assertEqual(len(expected_profiles), len(profiles)) + for have, expected in zip(profiles, expected_profiles): self.assertTrue(_profile_is_match(have, expected)) + # check that dynamic record is populated back from profiles + azrecord = RecordSet( + ttl=60, + target_resource=SubResource(id=profiles[-1].id), + ) + azrecord.name = record.name or '@' + azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) + record2 = provider._populate_record(zone, azrecord) + self.assertEqual(record2.dynamic._data(), record.dynamic._data()) + + def test_generate_traffic_managers_middle_east(self): # check Asia/Middle East test case + provider, zone, record = self._get_dynamic_package() record.dynamic._data()['rules'][0]['geos'].append('AS') profiles = provider._generate_traffic_managers(record) - geo_profile_name = _traffic_manager_suffix(record) - geo_profile = next( - profile - for profile in profiles - if profile.name == geo_profile_name + self.assertIn('GEO-ME', profiles[-1].endpoints[0].geo_mapping) + self.assertIn('GEO-AS', profiles[-1].endpoints[0].geo_mapping) + + def test_populate_dynamic_middle_east(self): + # Middle east without Asia raises exception + provider, zone, record = self._get_dynamic_package() + tm_suffix = _traffic_manager_suffix(record) + tm_id = provider._profile_name_to_id + tm_list = provider._tm_client.profiles.list_by_resource_group + tm_list.return_value = [ + Profile( + id=tm_id(tm_suffix), + name=tm_suffix, + traffic_routing_method='Geographic', + endpoints=[ + Endpoint( + geo_mapping=['GEO-ME'], + ), + ], + ), + ] + azrecord = RecordSet( + ttl=60, + target_resource=SubResource(id=tm_id(tm_suffix)), ) - self.assertIn('GEO-ME', geo_profile.endpoints[0].geo_mapping) - self.assertIn('GEO-AS', geo_profile.endpoints[0].geo_mapping) + azrecord.name = record.name or '@' + azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) + with self.assertRaises(AzureException) as ctx: + provider._populate_record(zone, azrecord) + self.assertTrue(text_type(ctx).startswith( + 'Middle East (GEO-ME) is not supported' + )) + + # valid profiles with Middle East test case + provider, zone, record = self._get_dynamic_package() + geo_profile = provider._get_tm_for_dynamic_record(record) + geo_profile.endpoints[0].geo_mapping.extend(['GEO-ME', 'GEO-AS']) + record = provider._populate_record(zone, azrecord) + self.assertIn('AS', record.dynamic.rules[0].data['geos']) + self.assertNotIn('ME', record.dynamic.rules[0].data['geos']) + + def test_dynamic_no_geo(self): + # test that traffic managers are generated as expected + provider, zone, record = self._get_dynamic_package() + external = 'Microsoft.Network/trafficManagerProfiles/externalEndpoints' + + record = Record.new(zone, 'foo', data={ + 'type': 'CNAME', + 'ttl': 60, + 'value': 'default.unit.tests.', + 'dynamic': { + 'pools': { + 'one': { + 'values': [ + {'value': 'one.unit.tests.'}, + ], + }, + }, + 'rules': [ + {'pool': 'one'}, + ], + } + }) + profiles = provider._generate_traffic_managers(record) + + self.assertEqual(len(profiles), 1) + self.assertTrue(_profile_is_match(profiles[0], Profile( + name='foo-unit-tests', + traffic_routing_method='Priority', + dns_config=DnsConfig( + relative_name='foo-unit-tests', ttl=60), + monitor_config=_get_monitor(record), + endpoints=[ + Endpoint( + name='one', + type=external, + target='one.unit.tests', + priority=1, + ), + Endpoint( + name='--default--', + type=external, + target='default.unit.tests', + priority=2, + ), + ], + ))) + + # test that same record gets populated back from traffic managers + tm_list = provider._tm_client.profiles.list_by_resource_group + tm_list.return_value = profiles + azrecord = RecordSet( + ttl=60, + target_resource=SubResource(id=profiles[0].id), + ) + azrecord.name = record.name or '@' + azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) + record2 = provider._populate_record(zone, azrecord) + self.assertEqual(record2.dynamic._data(), record.dynamic._data()) + + def test_dynamic_fallback_is_default(self): + # test that traffic managers are generated as expected + provider, zone, record = self._get_dynamic_package() + external = 'Microsoft.Network/trafficManagerProfiles/externalEndpoints' + + record = Record.new(zone, 'foo', data={ + 'type': 'CNAME', + 'ttl': 60, + 'value': 'default.unit.tests.', + 'dynamic': { + 'pools': { + 'def': { + 'values': [ + {'value': 'default.unit.tests.'}, + ], + }, + }, + 'rules': [ + {'geos': ['AF'], 'pool': 'def'}, + ], + } + }) + profiles = provider._generate_traffic_managers(record) + + self.assertEqual(len(profiles), 1) + self.assertTrue(_profile_is_match(profiles[0], Profile( + name='foo-unit-tests', + traffic_routing_method='Geographic', + dns_config=DnsConfig( + relative_name='foo-unit-tests', ttl=60), + monitor_config=_get_monitor(record), + endpoints=[ + Endpoint( + name='def--default--', + type=external, + target='default.unit.tests', + geo_mapping=['GEO-AF'], + ), + ], + ))) + + # test that same record gets populated back from traffic managers + tm_list = provider._tm_client.profiles.list_by_resource_group + tm_list.return_value = profiles + azrecord = RecordSet( + ttl=60, + target_resource=SubResource(id=profiles[0].id), + ) + azrecord.name = record.name or '@' + azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) + record2 = provider._populate_record(zone, azrecord) + self.assertEqual(record2.dynamic._data(), record.dynamic._data()) + + def test_dynamic_pool_contains_default(self): + # test that traffic managers are generated as expected + provider, zone, record = self._get_dynamic_package() + tm_id = provider._profile_name_to_id + external = 'Microsoft.Network/trafficManagerProfiles/externalEndpoints' + nested = 'Microsoft.Network/trafficManagerProfiles/nestedEndpoints' + + record = Record.new(zone, 'foo', data={ + 'type': 'CNAME', + 'ttl': 60, + 'value': 'default.unit.tests.', + 'dynamic': { + 'pools': { + 'rr': { + 'values': [ + {'value': 'one.unit.tests.'}, + {'value': 'two.unit.tests.'}, + {'value': 'default.unit.tests.'}, + {'value': 'final.unit.tests.'}, + ], + }, + }, + 'rules': [ + {'geos': ['AF'], 'pool': 'rr'}, + ], + } + }) + profiles = provider._generate_traffic_managers(record) + + self.assertEqual(len(profiles), 2) + self.assertTrue(_profile_is_match(profiles[0], Profile( + name='pool-rr--foo-unit-tests', + traffic_routing_method='Weighted', + dns_config=DnsConfig( + relative_name='pool-rr--foo-unit-tests', ttl=60), + monitor_config=_get_monitor(record), + endpoints=[ + Endpoint( + name='rr--one.unit.tests', + type=external, + target='one.unit.tests', + weight=1, + ), + Endpoint( + name='rr--two.unit.tests', + type=external, + target='two.unit.tests', + weight=1, + ), + Endpoint( + name='rr--default.unit.tests--default--', + type=external, + target='default.unit.tests', + weight=1, + ), + Endpoint( + name='rr--final.unit.tests', + type=external, + target='final.unit.tests', + weight=1, + ), + ], + ))) + self.assertTrue(_profile_is_match(profiles[1], Profile( + name='foo-unit-tests', + traffic_routing_method='Geographic', + dns_config=DnsConfig( + relative_name='foo-unit-tests', ttl=60), + monitor_config=_get_monitor(record), + endpoints=[ + Endpoint( + name='rule-rr', + type=nested, + target_resource_id=tm_id('pool-rr--foo-unit-tests'), + geo_mapping=['GEO-AF'], + ), + ], + ))) + + # test that same record gets populated back from traffic managers + tm_list = provider._tm_client.profiles.list_by_resource_group + tm_list.return_value = profiles + azrecord = RecordSet( + ttl=60, + target_resource=SubResource(id=profiles[1].id), + ) + azrecord.name = record.name or '@' + azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) + record2 = provider._populate_record(zone, azrecord) + self.assertEqual(record2.dynamic._data(), record.dynamic._data()) + + def test_dynamic_pool_contains_default_no_geo(self): + # test that traffic managers are generated as expected + provider, zone, record = self._get_dynamic_package() + external = 'Microsoft.Network/trafficManagerProfiles/externalEndpoints' + + record = Record.new(zone, 'foo', data={ + 'type': 'CNAME', + 'ttl': 60, + 'value': 'default.unit.tests.', + 'dynamic': { + 'pools': { + 'rr': { + 'values': [ + {'value': 'one.unit.tests.'}, + {'value': 'two.unit.tests.'}, + {'value': 'default.unit.tests.'}, + {'value': 'final.unit.tests.'}, + ], + }, + }, + 'rules': [ + {'pool': 'rr'}, + ], + } + }) + profiles = provider._generate_traffic_managers(record) + + self.assertEqual(len(profiles), 1) + self.assertTrue(_profile_is_match(profiles[0], Profile( + name='foo-unit-tests', + traffic_routing_method='Weighted', + dns_config=DnsConfig( + relative_name='foo-unit-tests', ttl=60), + monitor_config=_get_monitor(record), + endpoints=[ + Endpoint( + name='rr--one.unit.tests', + type=external, + target='one.unit.tests', + weight=1, + ), + Endpoint( + name='rr--two.unit.tests', + type=external, + target='two.unit.tests', + weight=1, + ), + Endpoint( + name='rr--default.unit.tests--default--', + type=external, + target='default.unit.tests', + weight=1, + ), + Endpoint( + name='rr--final.unit.tests', + type=external, + target='final.unit.tests', + weight=1, + ), + ], + ))) + + # test that same record gets populated back from traffic managers + tm_list = provider._tm_client.profiles.list_by_resource_group + tm_list.return_value = profiles + azrecord = RecordSet( + ttl=60, + target_resource=SubResource(id=profiles[0].id), + ) + azrecord.name = record.name or '@' + azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) + record2 = provider._populate_record(zone, azrecord) + self.assertEqual(record2.dynamic._data(), record.dynamic._data()) + + def test_dynamic_last_pool_contains_default_no_geo(self): + # test that traffic managers are generated as expected + provider, zone, record = self._get_dynamic_package() + tm_id = provider._profile_name_to_id + external = 'Microsoft.Network/trafficManagerProfiles/externalEndpoints' + nested = 'Microsoft.Network/trafficManagerProfiles/nestedEndpoints' + + record = Record.new(zone, 'foo', data={ + 'type': 'CNAME', + 'ttl': 60, + 'value': 'default.unit.tests.', + 'dynamic': { + 'pools': { + 'cloud': { + 'values': [ + {'value': 'cloud.unit.tests.'}, + ], + 'fallback': 'rr', + }, + 'rr': { + 'values': [ + {'value': 'one.unit.tests.'}, + {'value': 'two.unit.tests.'}, + {'value': 'default.unit.tests.'}, + {'value': 'final.unit.tests.'}, + ], + }, + }, + 'rules': [ + {'pool': 'cloud'}, + ], + } + }) + profiles = provider._generate_traffic_managers(record) + + self.assertEqual(len(profiles), 2) + self.assertTrue(_profile_is_match(profiles[0], Profile( + name='pool-rr--foo-unit-tests', + traffic_routing_method='Weighted', + dns_config=DnsConfig( + relative_name='pool-rr--foo-unit-tests', ttl=60), + monitor_config=_get_monitor(record), + endpoints=[ + Endpoint( + name='rr--one.unit.tests', + type=external, + target='one.unit.tests', + weight=1, + ), + Endpoint( + name='rr--two.unit.tests', + type=external, + target='two.unit.tests', + weight=1, + ), + Endpoint( + name='rr--default.unit.tests--default--', + type=external, + target='default.unit.tests', + weight=1, + ), + Endpoint( + name='rr--final.unit.tests', + type=external, + target='final.unit.tests', + weight=1, + ), + ], + ))) + self.assertTrue(_profile_is_match(profiles[1], Profile( + name='foo-unit-tests', + traffic_routing_method='Priority', + dns_config=DnsConfig( + relative_name='foo-unit-tests', ttl=60), + monitor_config=_get_monitor(record), + endpoints=[ + Endpoint( + name='cloud', + type=external, + target='cloud.unit.tests', + priority=1, + ), + Endpoint( + name='rr', + type=nested, + target_resource_id=tm_id('pool-rr--foo-unit-tests'), + priority=2, + ), + ], + ))) + + # test that same record gets populated back from traffic managers + tm_list = provider._tm_client.profiles.list_by_resource_group + tm_list.return_value = profiles + azrecord = RecordSet( + ttl=60, + target_resource=SubResource(id=profiles[1].id), + ) + azrecord.name = record.name or '@' + azrecord.type = 'Microsoft.Network/dnszones/{}'.format(record._type) + record2 = provider._populate_record(zone, azrecord) + self.assertEqual(record2.dynamic._data(), record.dynamic._data()) def test_sync_traffic_managers(self): provider, zone, record = self._get_dynamic_package() @@ -1194,6 +1498,21 @@ class TestAzureDnsProvider(TestCase): ) self.assertEqual(new_profile.endpoints[0].weight, 14) + @patch( + 'octodns.provider.azuredns.AzureProvider._generate_traffic_managers') + def test_sync_traffic_managers_duplicate(self, mock_gen_tms): + provider, zone, record = self._get_dynamic_package() + tm_sync = provider._tm_client.profiles.create_or_update + + # change and duplicate profiles + profile = self._get_tm_profiles(provider)[0] + profile.name = 'changing_this_to_trigger_sync' + mock_gen_tms.return_value = [profile, profile] + provider._sync_traffic_managers(record) + + # it should only be called once for duplicate profiles + tm_sync.assert_called_once() + def test_find_traffic_managers(self): provider, zone, record = self._get_dynamic_package()