ensure dynamic records map to unique ATM profiles

octodns/provider/azuredns.py

@@ -259,8 +259,21 @@ def _parse_azure_type(string):
     return string.split('/')[-1]
 
 
-def _traffic_manager_suffix(record):
-    return record.fqdn[:-1].replace('.', '-')
+def _root_traffic_manager_name(record):
+    # ATM names can only have letters, numbers and hyphens
+    # replace dots with double hyphens to ensure unique mapping,
+    # hoping that real life FQDNs won't have double hyphens
+    return record.fqdn[:-1].replace('.', '--')
+
+
+def _rule_traffic_manager_name(pool, record):
+    prefix = _root_traffic_manager_name(record)
+    return '{}-rule-{}'.format(prefix, pool)
+
+
+def _pool_traffic_manager_name(pool, record):
+    prefix = _root_traffic_manager_name(record)
+    return '{}-pool-{}'.format(prefix, pool)
 
 
 def _get_monitor(record):
@@ -533,7 +546,7 @@ class AzureProvider(BaseProvider):
         return self._get_tm_profile_by_id(profile_id)
 
     def _get_tm_for_dynamic_record(self, record):
-        name = _traffic_manager_suffix(record)
+        name = _root_traffic_manager_name(record)
         return self._get_tm_profile_by_name(name)
 
     def populate(self, zone, target=False, lenient=False):
@@ -799,6 +812,7 @@ class AzureProvider(BaseProvider):
                 raise AzureException(msg)
 
         log = self.log.info
+        seen_profiles = {}
         extra = []
         for record in desired.records:
             if not getattr(record, 'dynamic', False):
@@ -813,6 +827,16 @@ class AzureProvider(BaseProvider):
             profiles = self._generate_traffic_managers(record)
             for profile in profiles:
                 name = profile.name
+                if name in seen_profiles:
+                    # exit if a possible collision is detected, even though
+                    # we've tried to ensure unique mapping
+                    msg = 'Collision in Traffic Manager names detected'
+                    msg = '{}: {} and {} both want to use {}'.format(
+                        msg, seen_profiles[name], record.fqdn, name)
+                    raise AzureException(msg)
+                else:
+                    seen_profiles[name] = record.fqdn
+
                 active.add(name)
                 existing_profile = self._get_tm_profile_by_name(name)
                 if not _profile_is_match(existing_profile, profile):
@@ -831,7 +855,14 @@ class AzureProvider(BaseProvider):
 
         return extra
 
-    def _generate_tm_profile(self, name, routing, endpoints, record):
+    def _generate_tm_profile(self, routing, endpoints, record, label=None):
+        # figure out profile name and Traffic Manager FQDN
+        name = _root_traffic_manager_name(record)
+        if routing == 'Weighted':
+            name = _pool_traffic_manager_name(label, record)
+        elif routing == 'Priority':
+            name = _rule_traffic_manager_name(label, record)
+
         # set appropriate endpoint types
         endpoint_type_prefix = 'Microsoft.Network/trafficManagerProfiles/'
         for ep in endpoints:
@@ -859,10 +890,10 @@ 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
+    def _convert_tm_to_root(self, profile, record):
+        profile.name = _root_traffic_manager_name(record)
+        profile.id = self._profile_name_to_id(profile.name)
+        profile.dns_config.relative_name = profile.name
 
         return profile
 
@@ -871,7 +902,6 @@ class AzureProvider(BaseProvider):
         pools = record.dynamic.pools
 
         default = record.value[:-1]
-        tm_suffix = _traffic_manager_suffix(record)
         profile = self._generate_tm_profile
 
         geo_endpoints = []
@@ -930,10 +960,8 @@ class AzureProvider(BaseProvider):
                                 target=target,
                                 weight=val.get('weight', 1),
                             ))
-                        profile_name = 'pool-{}--{}'.format(
-                            pool_name, tm_suffix)
-                        pool_profile = profile(profile_name, 'Weighted',
-                                               endpoints, record)
+                        pool_profile = profile(
+                            'Weighted', endpoints, record, pool_name)
                         traffic_managers.append(pool_profile)
                         pool_profiles[pool_name] = pool_profile
 
@@ -973,10 +1001,8 @@ class AzureProvider(BaseProvider):
 
             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)
+                rule_profile = profile(
+                    'Priority', rule_endpoints, record, rule.data['pool'])
                 traffic_managers.append(rule_profile)
 
                 # append rule profile to top-level geo profile
@@ -1009,13 +1035,9 @@ class AzureProvider(BaseProvider):
            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)
+            self._convert_tm_to_root(traffic_managers[-1], record)
         else:
-            geo_profile = profile(tm_suffix, 'Geographic', geo_endpoints,
-                                  record)
+            geo_profile = profile('Geographic', geo_endpoints, record)
             traffic_managers.append(geo_profile)
 
         return traffic_managers
@@ -1047,14 +1069,15 @@ class AzureProvider(BaseProvider):
         return seen
 
     def _find_traffic_managers(self, record):
-        tm_suffix = _traffic_manager_suffix(record)
+        tm_prefix = _root_traffic_manager_name(record)
 
         profiles = set()
         for profile_id in self._traffic_managers:
-            # match existing profiles with record's suffix
+            # match existing profiles with record's prefix
             name = profile_id.split('/')[-1]
-            if name == tm_suffix or \
-               name.endswith('--{}'.format(tm_suffix)):
+            if name == tm_prefix or \
+               name.startswith('{}-pool-'.format(tm_prefix)) or \
+               name.startswith('{}-rule-'.format(tm_prefix)):
                 profiles.add(name)
 
         return profiles

tests/test_octodns_provider_azuredns.py

@@ -7,7 +7,7 @@ from __future__ import absolute_import, division, print_function, \
 
 from octodns.record import Create, Update, Delete, Record
 from octodns.provider.azuredns import _AzureRecord, AzureProvider, \
-    _check_endswith_dot, _parse_azure_type, _traffic_manager_suffix, \
+    _check_endswith_dot, _parse_azure_type, _root_traffic_manager_name, \
     _get_monitor, _profile_is_match, AzureException
 from octodns.zone import Zone
 from octodns.provider.base import Plan
@@ -402,12 +402,12 @@ class Test_CheckEndswithDot(TestCase):
             self.assertEquals(expected, _check_endswith_dot(test))
 
 
-class Test_TrafficManagerSuffix(TestCase):
-    def test_traffic_manager_suffix(self):
+class Test_RootTrafficManagerName(TestCase):
+    def test_root_traffic_manager_name(self):
         test = Record.new(zone, 'foo', data={
             'ttl': 60, 'type': 'CNAME', 'value': 'default.unit.tests.',
         })
-        self.assertEqual(_traffic_manager_suffix(test), 'foo-unit-tests')
+        self.assertEqual(_root_traffic_manager_name(test), 'foo--unit--tests')
 
 
 class Test_GetMonitor(TestCase):
@@ -594,9 +594,9 @@ class TestAzureDnsProvider(TestCase):
         base_id = '/subscriptions/' + sub + \
             '/resourceGroups/' + rg + \
             '/providers/Microsoft.Network/trafficManagerProfiles/'
-        suffix = 'foo-unit-tests'
-        id_format = base_id + '{}--' + suffix
-        name_format = '{}--' + suffix
+        prefix = 'foo--unit--tests'
+        name_format = prefix + '-{}'
+        id_format = base_id + name_format
 
         header = MonitorConfigCustomHeadersItem(name='Host',
                                                 value='foo.unit.tests')
@@ -688,8 +688,8 @@ class TestAzureDnsProvider(TestCase):
                 ],
             ),
             Profile(
-                id=base_id + suffix,
-                name=suffix,
+                id=base_id + prefix,
+                name=prefix,
                 traffic_routing_method='Geographic',
                 dns_config=DnsConfig(ttl=60),
                 monitor_config=monitor,
@@ -899,10 +899,10 @@ class TestAzureDnsProvider(TestCase):
         # test unused TM produces the extra change for clean up
         sample_profile = self._get_tm_profiles(provider)[0]
         tm_id = provider._profile_name_to_id
-        root_profile_name = _traffic_manager_suffix(record)
+        root_profile_name = _root_traffic_manager_name(record)
         extra_profile = Profile(
-            id=tm_id('random--{}'.format(root_profile_name)),
-            name='random--{}'.format(root_profile_name),
+            id=tm_id('{}-pool-random'.format(root_profile_name)),
+            name='{}-pool-random'.format(root_profile_name),
             traffic_routing_method='Weighted',
             dns_config=sample_profile.dns_config,
             monitor_config=sample_profile.monitor_config,
@@ -968,14 +968,40 @@ class TestAzureDnsProvider(TestCase):
         })
         desired.add_record(a_dynamic)
         changes.append(Create(a_dynamic))
-        with self.assertRaises(AzureException):
+        with self.assertRaises(AzureException) as ctx:
+            provider._extra_changes(existing, desired, changes)
+            self.assertTrue(text_type(ctx).endswith(
+                'must be of type CNAME'
+            ))
+        desired._remove_record(a_dynamic)
+
+        # test colliding ATM names throws exception
+        record1 = Record.new(desired, 'sub.www', data={
+            'type': record._type,
+            'ttl': record.ttl,
+            'value': record.value,
+            'dynamic': record.dynamic._data(),
+        })
+        record2 = Record.new(desired, 'sub--www', data={
+            'type': record._type,
+            'ttl': record.ttl,
+            'value': record.value,
+            'dynamic': record.dynamic._data(),
+        })
+        desired.add_record(record1)
+        desired.add_record(record2)
+        changes = [Create(record1), Create(record2)]
+        with self.assertRaises(AzureException) as ctx:
             provider._extra_changes(existing, desired, changes)
+            self.assertTrue(text_type(ctx).startswith(
+                'Collision in Traffic Manager'
+            ))
 
     def test_generate_tm_profile(self):
         provider, zone, record = self._get_dynamic_package()
         profile_gen = provider._generate_tm_profile
 
-        name = 'foobar'
+        label = 'foobar'
         routing = 'Priority'
         endpoints = [
             Endpoint(target='one.unit.tests'),
@@ -985,20 +1011,22 @@ class TestAzureDnsProvider(TestCase):
 
         # invalid endpoint raises exception
         with self.assertRaises(AzureException):
-            profile_gen(name, routing, endpoints, record)
+            profile_gen(routing, endpoints, record, label)
 
         # regular test
         endpoints.pop()
-        profile = profile_gen(name, routing, endpoints, record)
+        profile = profile_gen(routing, endpoints, record, label)
 
         # implicitly tests _profile_name_to_id
         sub = provider._dns_client_subscription_id
         rg = provider._resource_group
+        expected_name = 'foo--unit--tests-rule-foobar'
         expected_id = '/subscriptions/' + sub + \
             '/resourceGroups/' + rg + \
-            '/providers/Microsoft.Network/trafficManagerProfiles/' + name
+            '/providers/Microsoft.Network/trafficManagerProfiles/' + \
+            expected_name
         self.assertEqual(profile.id, expected_id)
-        self.assertEqual(profile.name, name)
+        self.assertEqual(profile.name, expected_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)
@@ -1044,7 +1072,7 @@ class TestAzureDnsProvider(TestCase):
     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_suffix = _root_traffic_manager_name(record)
         tm_id = provider._profile_name_to_id
         tm_list = provider._tm_client.profiles.list_by_resource_group
         tm_list.return_value = [
@@ -1105,10 +1133,10 @@ class TestAzureDnsProvider(TestCase):
 
         self.assertEqual(len(profiles), 1)
         self.assertTrue(_profile_is_match(profiles[0], Profile(
-            name='foo-unit-tests',
+            name='foo--unit--tests',
             traffic_routing_method='Priority',
             dns_config=DnsConfig(
-                relative_name='foo-unit-tests', ttl=60),
+                relative_name='foo--unit--tests', ttl=60),
             monitor_config=_get_monitor(record),
             endpoints=[
                 Endpoint(
@@ -1164,10 +1192,10 @@ class TestAzureDnsProvider(TestCase):
 
         self.assertEqual(len(profiles), 1)
         self.assertTrue(_profile_is_match(profiles[0], Profile(
-            name='foo-unit-tests',
+            name='foo--unit--tests',
             traffic_routing_method='Geographic',
             dns_config=DnsConfig(
-                relative_name='foo-unit-tests', ttl=60),
+                relative_name='foo--unit--tests', ttl=60),
             monitor_config=_get_monitor(record),
             endpoints=[
                 Endpoint(
@@ -1222,10 +1250,10 @@ class TestAzureDnsProvider(TestCase):
 
         self.assertEqual(len(profiles), 2)
         self.assertTrue(_profile_is_match(profiles[0], Profile(
-            name='pool-rr--foo-unit-tests',
+            name='foo--unit--tests-pool-rr',
             traffic_routing_method='Weighted',
             dns_config=DnsConfig(
-                relative_name='pool-rr--foo-unit-tests', ttl=60),
+                relative_name='foo--unit--tests-pool-rr', ttl=60),
             monitor_config=_get_monitor(record),
             endpoints=[
                 Endpoint(
@@ -1255,16 +1283,16 @@ class TestAzureDnsProvider(TestCase):
             ],
         )))
         self.assertTrue(_profile_is_match(profiles[1], Profile(
-            name='foo-unit-tests',
+            name='foo--unit--tests',
             traffic_routing_method='Geographic',
             dns_config=DnsConfig(
-                relative_name='foo-unit-tests', ttl=60),
+                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'),
+                    target_resource_id=tm_id(profiles[0].name),
                     geo_mapping=['GEO-AF'],
                 ),
             ],
@@ -1311,10 +1339,10 @@ class TestAzureDnsProvider(TestCase):
 
         self.assertEqual(len(profiles), 1)
         self.assertTrue(_profile_is_match(profiles[0], Profile(
-            name='foo-unit-tests',
+            name='foo--unit--tests',
             traffic_routing_method='Weighted',
             dns_config=DnsConfig(
-                relative_name='foo-unit-tests', ttl=60),
+                relative_name='foo--unit--tests', ttl=60),
             monitor_config=_get_monitor(record),
             endpoints=[
                 Endpoint(
@@ -1393,10 +1421,10 @@ class TestAzureDnsProvider(TestCase):
 
         self.assertEqual(len(profiles), 2)
         self.assertTrue(_profile_is_match(profiles[0], Profile(
-            name='pool-rr--foo-unit-tests',
+            name='foo--unit--tests-pool-rr',
             traffic_routing_method='Weighted',
             dns_config=DnsConfig(
-                relative_name='pool-rr--foo-unit-tests', ttl=60),
+                relative_name='foo--unit--tests-pool-rr', ttl=60),
             monitor_config=_get_monitor(record),
             endpoints=[
                 Endpoint(
@@ -1426,10 +1454,10 @@ class TestAzureDnsProvider(TestCase):
             ],
         )))
         self.assertTrue(_profile_is_match(profiles[1], Profile(
-            name='foo-unit-tests',
+            name='foo--unit--tests',
             traffic_routing_method='Priority',
             dns_config=DnsConfig(
-                relative_name='foo-unit-tests', ttl=60),
+                relative_name='foo--unit--tests', ttl=60),
             monitor_config=_get_monitor(record),
             endpoints=[
                 Endpoint(
@@ -1441,7 +1469,7 @@ class TestAzureDnsProvider(TestCase):
                 Endpoint(
                     name='rr',
                     type=nested,
-                    target_resource_id=tm_id('pool-rr--foo-unit-tests'),
+                    target_resource_id=tm_id(profiles[0].name),
                     priority=2,
                 ),
             ],
@@ -1459,16 +1487,37 @@ class TestAzureDnsProvider(TestCase):
         record2 = provider._populate_record(zone, azrecord)
         self.assertEqual(record2.dynamic._data(), record.dynamic._data())
 
+    def test_dynamic_unique_traffic_managers(self):
+        record = self._get_dynamic_record(zone)
+        data = {
+            'type': record._type,
+            'ttl': record.ttl,
+            'value': record.value,
+            'dynamic': record.dynamic._data()
+        }
+        record_names = [
+            'www.foo', 'www-foo'
+        ]
+        provider = self._get_provider()
+
+        seen = set()
+        for name in record_names:
+            record = Record.new(zone, name, data=data)
+            tms = provider._generate_traffic_managers(record)
+            for tm in tms:
+                self.assertNotIn(tm.name, seen)
+                seen.add(tm.name)
+
     def test_sync_traffic_managers(self):
         provider, zone, record = self._get_dynamic_package()
         provider._populate_traffic_managers()
 
         tm_sync = provider._tm_client.profiles.create_or_update
 
-        suffix = 'foo-unit-tests'
+        prefix = 'foo--unit--tests'
         expected_seen = {
-            suffix, 'pool-two--{}'.format(suffix),
-            'rule-one--{}'.format(suffix), 'rule-two--{}'.format(suffix),
+            prefix, '{}-pool-two'.format(prefix),
+            '{}-rule-one'.format(prefix), '{}-rule-two'.format(prefix),
         }
 
         # test no change
@@ -1494,7 +1543,7 @@ class TestAzureDnsProvider(TestCase):
 
         # test that new profile was successfully inserted in cache
         new_profile = provider._get_tm_profile_by_name(
-            'pool-two--{}'.format(suffix)
+            '{}-pool-two'.format(prefix)
         )
         self.assertEqual(new_profile.endpoints[0].weight, 14)
 
@@ -1524,11 +1573,11 @@ class TestAzureDnsProvider(TestCase):
             'ttl': record.ttl,
             'value': record.value,
         })
-        suffix2 = _traffic_manager_suffix(record2)
+        prefix2 = _root_traffic_manager_name(record2)
         tm_id = provider._profile_name_to_id
         extra_profile = Profile(
-            id=tm_id('random--{}'.format(suffix2)),
-            name='random--{}'.format(suffix2),
+            id=tm_id('{}-pool-random'.format(prefix2)),
+            name='{}-pool-random'.format(prefix2),
             traffic_routing_method='Weighted',
             dns_config=sample_profile.dns_config,
             monitor_config=sample_profile.monitor_config,
@@ -1539,10 +1588,10 @@ class TestAzureDnsProvider(TestCase):
         provider._populate_traffic_managers()
 
         # implicitly asserts that non-matching profile is not included
-        suffix = _traffic_manager_suffix(record)
+        prefix = _root_traffic_manager_name(record)
         self.assertEqual(provider._find_traffic_managers(record), {
-            suffix, 'pool-two--{}'.format(suffix),
-            'rule-one--{}'.format(suffix), 'rule-two--{}'.format(suffix),
+            prefix, '{}-pool-two'.format(prefix),
+            '{}-rule-one'.format(prefix), '{}-rule-two'.format(prefix),
         })
 
     def test_traffic_manager_gc(self):
@@ -1655,10 +1704,10 @@ class TestAzureDnsProvider(TestCase):
         provider, existing, dynamic_record = self._get_dynamic_package()
         sample_profile = self._get_tm_profiles(provider)[0]
         tm_id = provider._profile_name_to_id
-        root_profile_name = _traffic_manager_suffix(dynamic_record)
+        root_profile_name = _root_traffic_manager_name(dynamic_record)
         extra_profile = Profile(
-            id=tm_id('random--{}'.format(root_profile_name)),
-            name='random--{}'.format(root_profile_name),
+            id=tm_id('{}-pool-random'.format(root_profile_name)),
+            name='{}-pool-random'.format(root_profile_name),
             traffic_routing_method='Weighted',
             dns_config=sample_profile.dns_config,
             monitor_config=sample_profile.monitor_config,