diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 52fb3d2..a910459 100644 --- a/octodns/provider/ns1.py +++ b/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') @@ -602,16 +602,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 @@ -621,7 +627,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) @@ -660,6 +666,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: @@ -836,6 +846,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 @@ -886,6 +900,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': { @@ -1030,7 +1048,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 @@ -1131,10 +1149,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 @@ -1163,7 +1185,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) @@ -1188,8 +1210,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 @@ -1267,8 +1291,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): diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index db713eb..f54c7bd 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/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') @@ -1249,7 +1297,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) @@ -1277,7 +1325,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 @@ -1286,7 +1366,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)) @@ -1298,7 +1378,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': {}, @@ -1403,7 +1483,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': { @@ -1447,7 +1527,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) @@ -1458,7 +1538,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 @@ -1549,7 +1629,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': { @@ -1587,6 +1667,67 @@ class TestNs1ProviderDynamic(TestCase): 'values': ['5.5.5.5'] }, data) + 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')