Merge pull request #704 from viranch/ns1-dynamic-cname

Add support for dynamic CNAME records in NS1

octodns/provider/ns1.py

@@ -464,10 +464,10 @@ class Ns1Provider(BaseProvider):
             pass
         return pool_name
 
-    def _data_for_dynamic_A(self, _type, record):
+    def _data_for_dynamic(self, _type, record):
         # First make sure we have the expected filters config
         if not self._valid_filter_config(record['filters'], record['domain']):
-            self.log.error('_data_for_dynamic_A: %s %s has unsupported '
+            self.log.error('_data_for_dynamic: %s %s has unsupported '
                            'filters', record['domain'], _type)
             raise Ns1Exception('Unrecognized advanced record')
 
@@ -588,16 +588,22 @@ class Ns1Provider(BaseProvider):
         rules = list(rules.values())
         rules.sort(key=lambda r: (r['_order'], r['pool']))
 
-        return {
+        data = {
             'dynamic': {
                 'pools': pools,
                 'rules': rules,
             },
             'ttl': record['ttl'],
             'type': _type,
-            'values': sorted(default),
         }
 
+        if _type == 'CNAME':
+            data['value'] = default[0]
+        else:
+            data['values'] = default
+
+        return data
+
     def _data_for_A(self, _type, record):
         if record.get('tier', 1) > 1:
             # Advanced record, see if it's first answer has a note
@@ -607,7 +613,7 @@ class Ns1Provider(BaseProvider):
                 first_answer_note = ''
             # If that note includes a `from` (pool name) it's a dynamic record
             if 'from:' in first_answer_note:
-                return self._data_for_dynamic_A(_type, record)
+                return self._data_for_dynamic(_type, record)
             # If not it's an old geo record
             return self._data_for_geo_A(_type, record)
 
@@ -646,6 +652,10 @@ class Ns1Provider(BaseProvider):
         }
 
     def _data_for_CNAME(self, _type, record):
+        if record.get('tier', 1) > 1:
+            # Advanced dynamic record
+            return self._data_for_dynamic(_type, record)
+
         try:
             value = record['short_answers'][0]
         except IndexError:
@@ -822,6 +832,10 @@ class Ns1Provider(BaseProvider):
                     # This monitor does not belong to this record
                     config = monitor['config']
                     value = config['host']
+                    if record._type == 'CNAME':
+                        # Append a trailing dot for CNAME records so that
+                        # lookup by a CNAME answer works
+                        value = value + '.'
                     monitors[value] = monitor
 
         return monitors
@@ -872,6 +886,10 @@ class Ns1Provider(BaseProvider):
         host = record.fqdn[:-1]
         _type = record._type
 
+        if _type == 'CNAME':
+            # NS1 does not accept a host value with a trailing dot
+            value = value[:-1]
+
         ret = {
             'active': True,
             'config': {
@@ -1013,7 +1031,7 @@ class Ns1Provider(BaseProvider):
             }
             answers.append(answer)
 
-    def _params_for_dynamic_A(self, record):
+    def _params_for_dynamic(self, record):
         pools = record.dynamic.pools
 
         # Convert rules to regions
@@ -1114,10 +1132,14 @@ class Ns1Provider(BaseProvider):
                     'feed_id': feed_id,
                 })
 
+        if record._type == 'CNAME':
+            default_values = [record.value]
+        else:
+            default_values = record.values
         default_answers = [{
             'answer': [v],
             'weight': 1,
-        } for v in record.values]
+        } for v in default_values]
 
         # Build our list of answers
         # The regions dictionary built above already has the required pool
@@ -1146,7 +1168,7 @@ class Ns1Provider(BaseProvider):
 
     def _params_for_A(self, record):
         if getattr(record, 'dynamic', False):
-            return self._params_for_dynamic_A(record)
+            return self._params_for_dynamic(record)
         elif hasattr(record, 'geo'):
             return self._params_for_geo_A(record)
 
@@ -1171,8 +1193,10 @@ class Ns1Provider(BaseProvider):
         values = [(v.flags, v.tag, v.value) for v in record.values]
         return {'answers': values, 'ttl': record.ttl}, None
 
-    # TODO: dynamic CNAME support
     def _params_for_CNAME(self, record):
+        if getattr(record, 'dynamic', False):
+            return self._params_for_dynamic(record)
+
         return {'answers': [record.value], 'ttl': record.ttl}, None
 
     _params_for_ALIAS = _params_for_CNAME
@@ -1250,8 +1274,7 @@ class Ns1Provider(BaseProvider):
                     extra.append(Update(record, record))
                     continue
 
-            for have in self._monitors_for(record).values():
-                value = have['config']['host']
+            for value, have in self._monitors_for(record).items():
                 expected = self._monitor_gen(record, value)
                 # TODO: find values which have missing monitors
                 if not self._monitor_is_match(expected, have):

tests/test_octodns_provider_ns1.py

@@ -578,6 +578,34 @@ class TestNs1ProviderDynamic(TestCase):
             'meta': {},
         })
 
+    def cname_record(self):
+        return Record.new(self.zone, 'foo', {
+            'dynamic': {
+                'pools': {
+                    'iad': {
+                        'values': [{
+                            'value': 'iad.unit.tests.',
+                        }],
+                    },
+                },
+                'rules': [{
+                    'pool': 'iad',
+                }],
+            },
+            'octodns': {
+                'healthcheck': {
+                    'host': 'send.me',
+                    'path': '/_ping',
+                    'port': 80,
+                    'protocol': 'HTTP',
+                }
+            },
+            'ttl': 33,
+            'type': 'CNAME',
+            'value': 'value.unit.tests.',
+            'meta': {},
+        })
+
     def test_notes(self):
         provider = Ns1Provider('test', 'api-key')
 
@@ -609,6 +637,12 @@ class TestNs1ProviderDynamic(TestCase):
             },
             'notes': 'host:unit.tests type:A',
         }
+        monitor_five = {
+            'config': {
+                'host': 'iad.unit.tests',
+            },
+            'notes': 'host:foo.unit.tests type:CNAME',
+        }
         provider._client._monitors_cache = {
             'one': monitor_one,
             'two': {
@@ -624,6 +658,7 @@ class TestNs1ProviderDynamic(TestCase):
                 'notes': 'host:other.unit.tests type:A',
             },
             'four': monitor_four,
+            'five': monitor_five,
         }
 
         # Would match, but won't get there b/c it's not dynamic
@@ -641,6 +676,11 @@ class TestNs1ProviderDynamic(TestCase):
             '2.3.4.5': monitor_four,
         }, provider._monitors_for(self.record()))
 
+        # Check match for CNAME values
+        self.assertEquals({
+            'iad.unit.tests.': monitor_five,
+        }, provider._monitors_for(self.cname_record()))
+
     def test_uuid(self):
         # Just a smoke test/for coverage
         provider = Ns1Provider('test', 'api-key')
@@ -728,6 +768,14 @@ class TestNs1ProviderDynamic(TestCase):
         # No http response expected
         self.assertFalse('rules' in monitor)
 
+    def test_monitor_gen_CNAME(self):
+        provider = Ns1Provider('test', 'api-key')
+
+        value = 'iad.unit.tests.'
+        record = self.cname_record()
+        monitor = provider._monitor_gen(record, value)
+        self.assertEquals(value[:-1], monitor['config']['host'])
+
     def test_monitor_is_match(self):
         provider = Ns1Provider('test', 'api-key')
 
@@ -1242,7 +1290,7 @@ class TestNs1ProviderDynamic(TestCase):
             ('mid-2', 'fid-2'),
             ('mid-3', 'fid-3'),
         ]
-        # This indirectly calls into _params_for_dynamic_A and tests the
+        # This indirectly calls into _params_for_dynamic and tests the
         # handling to get there
         record = self.record()
         ret, _ = provider._params_for_A(record)
@@ -1270,7 +1318,39 @@ class TestNs1ProviderDynamic(TestCase):
         params, _ = provider._params_for_geo_A(record)
         self.assertEquals([], params['filters'])
 
-    def test_data_for_dynamic_A(self):
+    @patch('octodns.provider.ns1.Ns1Provider._monitor_sync')
+    @patch('octodns.provider.ns1.Ns1Provider._monitors_for')
+    def test_params_for_dynamic_CNAME(self, monitors_for_mock,
+                                      monitor_sync_mock):
+        provider = Ns1Provider('test', 'api-key')
+
+        # pre-fill caches to avoid extranious calls (things we're testing
+        # elsewhere)
+        provider._client._datasource_id = 'foo'
+        provider._client._feeds_for_monitors = {
+            'mon-id': 'feed-id',
+        }
+
+        # provider._params_for_A() calls provider._monitors_for() and
+        # provider._monitor_sync(). Mock their return values so that we don't
+        # make NS1 API calls during tests
+        monitors_for_mock.reset_mock()
+        monitor_sync_mock.reset_mock()
+        monitors_for_mock.side_effect = [{
+            'iad.unit.tests.': 'mid-1',
+        }]
+        monitor_sync_mock.side_effect = [
+            ('mid-1', 'fid-1'),
+        ]
+
+        record = self.cname_record()
+        ret, _ = provider._params_for_CNAME(record)
+
+        # Check if the default value was correctly read and populated
+        # All other dynamic record test cases are covered by dynamic_A tests
+        self.assertEquals(ret['answers'][-1]['answer'][0], 'value.unit.tests.')
+
+    def test_data_for_dynamic(self):
         provider = Ns1Provider('test', 'api-key')
 
         # Unexpected filters throws an error
@@ -1279,7 +1359,7 @@ class TestNs1ProviderDynamic(TestCase):
             'filters': [],
         }
         with self.assertRaises(Ns1Exception) as ctx:
-            provider._data_for_dynamic_A('A', ns1_record)
+            provider._data_for_dynamic('A', ns1_record)
         self.assertEquals('Unrecognized advanced record',
                           text_type(ctx.exception))
 
@@ -1291,7 +1371,7 @@ class TestNs1ProviderDynamic(TestCase):
             'regions': {},
             'ttl': 42,
         }
-        data = provider._data_for_dynamic_A('A', ns1_record)
+        data = provider._data_for_dynamic('A', ns1_record)
         self.assertEquals({
             'dynamic': {
                 'pools': {},
@@ -1396,7 +1476,7 @@ class TestNs1ProviderDynamic(TestCase):
             'tier': 3,
             'ttl': 42,
         }
-        data = provider._data_for_dynamic_A('A', ns1_record)
+        data = provider._data_for_dynamic('A', ns1_record)
         self.assertEquals({
             'dynamic': {
                 'pools': {
@@ -1440,7 +1520,7 @@ class TestNs1ProviderDynamic(TestCase):
         }, data)
 
         # Same answer if we go through _data_for_A which out sources the job to
-        # _data_for_dynamic_A
+        # _data_for_dynamic
         data2 = provider._data_for_A('A', ns1_record)
         self.assertEquals(data, data2)
 
@@ -1451,7 +1531,7 @@ class TestNs1ProviderDynamic(TestCase):
         ns1_record['regions'][old_style_catchall_pool_name] = \
             ns1_record['regions'][catchall_pool_name]
         del ns1_record['regions'][catchall_pool_name]
-        data3 = provider._data_for_dynamic_A('A', ns1_record)
+        data3 = provider._data_for_dynamic('A', ns1_record)
         self.assertEquals(data, data2)
 
         # Oceania test cases
@@ -1471,6 +1551,67 @@ class TestNs1ProviderDynamic(TestCase):
             self.assertTrue(
                 'OC-{}'.format(c) in data4['dynamic']['rules'][0]['geos'])
 
+    def test_data_for_dynamic_CNAME(self):
+        provider = Ns1Provider('test', 'api-key')
+
+        # Test out a small setup that just covers default value validation
+        # Everything else is same as dynamic A whose tests will cover all
+        # other options and test cases
+        # Not testing for geo/region specific cases
+        filters = provider._get_updated_filter_chain(False, False)
+        catchall_pool_name = 'iad__catchall'
+        ns1_record = {
+            'answers': [{
+                'answer': ['iad.unit.tests.'],
+                'meta': {
+                    'priority': 1,
+                    'weight': 12,
+                    'note': 'from:{}'.format(catchall_pool_name),
+                },
+                'region': catchall_pool_name,
+            }, {
+                'answer': ['value.unit.tests.'],
+                'meta': {
+                    'priority': 2,
+                    'note': 'from:--default--',
+                },
+                'region': catchall_pool_name,
+            }],
+            'domain': 'foo.unit.tests',
+            'filters': filters,
+            'regions': {
+                catchall_pool_name: {
+                    'meta': {
+                        'note': 'rule-order:1',
+                    },
+                }
+            },
+            'tier': 3,
+            'ttl': 43,
+            'type': 'CNAME',
+        }
+        data = provider._data_for_CNAME('CNAME', ns1_record)
+        self.assertEquals({
+            'dynamic': {
+                'pools': {
+                    'iad': {
+                        'fallback': None,
+                        'values': [{
+                            'value': 'iad.unit.tests.',
+                            'weight': 12,
+                        }],
+                    },
+                },
+                'rules': [{
+                    '_order': '1',
+                    'pool': 'iad',
+                }],
+            },
+            'ttl': 43,
+            'type': 'CNAME',
+            'value': 'value.unit.tests.',
+        }, data)
+
     @patch('ns1.rest.records.Records.retrieve')
     @patch('ns1.rest.zones.Zones.retrieve')
     @patch('octodns.provider.ns1.Ns1Provider._monitors_for')