From 40569945d2bc0fb89735fd97e3cdbbd00b1379c1 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 30 Apr 2021 16:53:37 -0700 Subject: [PATCH 1/4] Add support for dynamic CNAME records in NS1 --- octodns/provider/ns1.py | 24 +++++- tests/test_octodns_provider_ns1.py | 118 +++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 4 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 6cea185..4da0d33 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -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 @@ -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_A(_type, record) + try: value = record['short_answers'][0] except IndexError: @@ -1114,10 +1124,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 @@ -1171,8 +1185,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_A(record) + return {'answers': [record.value], 'ttl': record.ttl}, None _params_for_ALIAS = _params_for_CNAME diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 00b068b..c16a573 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1270,6 +1270,63 @@ class TestNs1ProviderDynamic(TestCase): params, _ = provider._params_for_geo_A(record) self.assertEquals([], params['filters']) + @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 = 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': 32, + 'type': 'CNAME', + 'value': 'value.unit.tests.', + 'meta': {}, + }) + 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_A(self): provider = Ns1Provider('test', 'api-key') @@ -1471,6 +1528,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': ['2.3.4.5.unit.tests.'], + 'meta': { + 'priority': 1, + 'weight': 12, + 'note': 'from:{}'.format(catchall_pool_name), + }, + 'region': catchall_pool_name, + }, { + 'answer': ['1.2.3.4.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': 42, + 'type': 'CNAME', + } + data = provider._data_for_CNAME('CNAME', ns1_record) + self.assertEquals({ + 'dynamic': { + 'pools': { + 'iad': { + 'fallback': None, + 'values': [{ + 'value': '2.3.4.5.unit.tests.', + 'weight': 12, + }], + }, + }, + 'rules': [{ + '_order': '1', + 'pool': 'iad', + }], + }, + 'ttl': 42, + 'type': 'CNAME', + 'value': '1.2.3.4.unit.tests.', + }, data) + @patch('ns1.rest.records.Records.retrieve') @patch('ns1.rest.zones.Zones.retrieve') @patch('octodns.provider.ns1.Ns1Provider._monitors_for') From 15eb23eeb672f743cb21724eddb2fa394c931aea Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 30 Apr 2021 20:26:11 -0700 Subject: [PATCH 2/4] Trim trailing dot from CNAME answers for NS1 monitors --- octodns/provider/ns1.py | 11 +++- tests/test_octodns_provider_ns1.py | 87 +++++++++++++++++++----------- 2 files changed, 64 insertions(+), 34 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 4da0d33..6ade3b9 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -832,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 @@ -882,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': { @@ -1266,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): diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index c16a573..a5c41a6 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') @@ -1295,32 +1343,7 @@ class TestNs1ProviderDynamic(TestCase): ('mid-1', 'fid-1'), ] - record = 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': 32, - 'type': 'CNAME', - 'value': 'value.unit.tests.', - 'meta': {}, - }) + record = self.cname_record() ret, _ = provider._params_for_CNAME(record) # Check if the default value was correctly read and populated @@ -1539,7 +1562,7 @@ class TestNs1ProviderDynamic(TestCase): catchall_pool_name = 'iad__catchall' ns1_record = { 'answers': [{ - 'answer': ['2.3.4.5.unit.tests.'], + 'answer': ['iad.unit.tests.'], 'meta': { 'priority': 1, 'weight': 12, @@ -1547,7 +1570,7 @@ class TestNs1ProviderDynamic(TestCase): }, 'region': catchall_pool_name, }, { - 'answer': ['1.2.3.4.unit.tests.'], + 'answer': ['value.unit.tests.'], 'meta': { 'priority': 2, 'note': 'from:--default--', @@ -1564,7 +1587,7 @@ class TestNs1ProviderDynamic(TestCase): } }, 'tier': 3, - 'ttl': 42, + 'ttl': 43, 'type': 'CNAME', } data = provider._data_for_CNAME('CNAME', ns1_record) @@ -1574,7 +1597,7 @@ class TestNs1ProviderDynamic(TestCase): 'iad': { 'fallback': None, 'values': [{ - 'value': '2.3.4.5.unit.tests.', + 'value': 'iad.unit.tests.', 'weight': 12, }], }, @@ -1584,9 +1607,9 @@ class TestNs1ProviderDynamic(TestCase): 'pool': 'iad', }], }, - 'ttl': 42, + 'ttl': 43, 'type': 'CNAME', - 'value': '1.2.3.4.unit.tests.', + 'value': 'value.unit.tests.', }, data) @patch('ns1.rest.records.Records.retrieve') From 3de5cd27404f5fae35feb14e7e4f01addace2528 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Fri, 30 Apr 2021 20:38:08 -0700 Subject: [PATCH 3/4] More future proof index lookup --- tests/test_octodns_provider_ns1.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index a5c41a6..c14173b 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1348,7 +1348,7 @@ class TestNs1ProviderDynamic(TestCase): # 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.') + self.assertEquals(ret['answers'][-1]['answer'][0], 'value.unit.tests.') def test_data_for_dynamic_A(self): provider = Ns1Provider('test', 'api-key') From 6d7cab43e881247201ea77cd5ef281fb925674a6 Mon Sep 17 00:00:00 2001 From: Viranch Mehta Date: Mon, 3 May 2021 10:59:12 -0700 Subject: [PATCH 4/4] Rename data/params for dynamic methods --- octodns/provider/ns1.py | 14 +++++++------- tests/test_octodns_provider_ns1.py | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 6ade3b9..38f9da3 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') @@ -613,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) @@ -654,7 +654,7 @@ class Ns1Provider(BaseProvider): def _data_for_CNAME(self, _type, record): if record.get('tier', 1) > 1: # Advanced dynamic record - return self._data_for_dynamic_A(_type, record) + return self._data_for_dynamic(_type, record) try: value = record['short_answers'][0] @@ -1031,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 @@ -1168,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) @@ -1195,7 +1195,7 @@ class Ns1Provider(BaseProvider): def _params_for_CNAME(self, record): if getattr(record, 'dynamic', False): - return self._params_for_dynamic_A(record) + return self._params_for_dynamic(record) return {'answers': [record.value], 'ttl': record.ttl}, None diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index c14173b..4d6f02a 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1290,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) @@ -1350,7 +1350,7 @@ class TestNs1ProviderDynamic(TestCase): # 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_A(self): + def test_data_for_dynamic(self): provider = Ns1Provider('test', 'api-key') # Unexpected filters throws an error @@ -1359,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)) @@ -1371,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': {}, @@ -1476,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': { @@ -1520,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) @@ -1531,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