From 003754edc7f76f85d38e427cb863cd327b8188b3 Mon Sep 17 00:00:00 2001 From: rupa deadwyler Date: Thu, 5 Mar 2020 12:16:17 -0500 Subject: [PATCH 1/3] NS1 provider: support rate-limiting strategy Adds a "parallelism" argument to the NS1 Provider. If set, we analyze response headers and attempt to avoid 429 responses. --- octodns/provider/ns1.py | 43 +++++++++++++++++++++++++----- tests/test_octodns_provider_ns1.py | 18 +++++++++++++ 2 files changed, 55 insertions(+), 6 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 2a3ae07..0e2d271 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -27,11 +27,41 @@ class Ns1Exception(Exception): class Ns1Client(object): log = getLogger('NS1Client') - def __init__(self, api_key, retry_count=4): - self.log.debug('__init__: retry_count=%d', retry_count) + def __init__(self, api_key, parallelism=None, retry_count=4): + self.log.debug('__init__: parallelism=%s, retry_count=%d', parallelism, + retry_count) self.retry_count = retry_count client = NS1(apiKey=api_key) + + # NS1 rate limits via a "token bucket" scheme, and provides information + # about rate limiting in headers on responses. Token bucket can be + # thought of as an initially "full" bucket, where, if not full, tokens + # are added at some rate. This allows "bursting" requests until the + # bucket is empty, after which, you are limited to the rate of token + # replenishment. + # There are a couple of "strategies" built into the SDK to avoid 429s + # from rate limiting. Since octodns operates concurrently via + # `max_workers`, a concurrent strategy seems appropriate. + # This strategy does nothing until the remaining requests are equal to + # or less than our `parallelism`, after which, each process will sleep + # for the token replenishment interval times parallelism. + # For example, if we can make 10 requests in 60 seconds, a token is + # replenished every 6 seconds. If parallelism is 3, we will burst 7 + # requests, and subsequently each process will sleep for 18 seconds + # before making another request. + # In general, parallelism should match the number of workers. + if parallelism is not None: + client.config['rate_limit_strategy'] = 'concurrent' + client.config['parallelism'] = parallelism + + # The list of records for a zone is paginated at around ~2.5k records, + # this tells the client to handle any of that transparently and ensure + # we get the full list of records. + client.config['follow_pagination'] = True + + self._config = client.config + self._records = client.records() self._zones = client.zones() self._monitors = client.monitors() @@ -234,15 +264,16 @@ class Ns1Provider(BaseProvider): 'TK', 'TO', 'TV', 'WF', 'WS'}, } - def __init__(self, id, api_key, retry_count=4, monitor_regions=None, *args, - **kwargs): + def __init__(self, id, api_key, retry_count=4, monitor_regions=None, + parallelism=None, *args, **kwargs): self.log = getLogger('Ns1Provider[{}]'.format(id)) self.log.debug('__init__: id=%s, api_key=***, retry_count=%d, ' - 'monitor_regions=%s', id, retry_count, monitor_regions) + 'monitor_regions=%s, parallelism=%s', id, retry_count, + monitor_regions, parallelism) super(Ns1Provider, self).__init__(id, *args, **kwargs) self.monitor_regions = monitor_regions - self._client = Ns1Client(api_key, retry_count) + self._client = Ns1Client(api_key, parallelism, retry_count) def _encode_notes(self, data): return ' '.join(['{}:{}'.format(k, v) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index 8126c23..fb3bec0 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1392,6 +1392,24 @@ class TestNs1Client(TestCase): client.zones_retrieve('unit.tests') self.assertEquals('last', text_type(ctx.exception)) + def test_client_config(self): + with self.assertRaises(TypeError): + client = Ns1Client() + + client = Ns1Client('dummy-key') + self.assertEquals( + client._config.get('keys'), + {'default': {'key': u'dummy-key', 'desc': 'imported API key'}} + ) + self.assertEquals(client._config.get('rate_limit_strategy'), None) + self.assertEquals(client._config.get('parallelism'), None) + + client = Ns1Client('dummy-key', parallelism=11) + self.assertEquals( + client._config.get('rate_limit_strategy'), 'concurrent' + ) + self.assertEquals(client._config.get('parallelism'), 11) + @patch('ns1.rest.data.Source.list') @patch('ns1.rest.data.Source.create') def test_datasource_id(self, datasource_create_mock, datasource_list_mock): From 0f848e9b7650fae9e7bf9c2cf2936734051a6a53 Mon Sep 17 00:00:00 2001 From: rupa deadwyler Date: Thu, 5 Mar 2020 12:58:28 -0500 Subject: [PATCH 2/3] Add the parallelism arg to Ns1Provider docstring --- octodns/provider/ns1.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 0e2d271..2bfaec1 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -205,6 +205,9 @@ class Ns1Provider(BaseProvider): ns1: class: octodns.provider.ns1.Ns1Provider api_key: env/NS1_API_KEY + # Optional, to avoid 429s from rate-limiting. Try setting to the + # value of max_workers. + parallelism: 11 # Only required if using dynamic records monitor_regions: - lga From 0df33a51652b55b8745654c8e25bab8da0db4d80 Mon Sep 17 00:00:00 2001 From: rupa deadwyler Date: Fri, 6 Mar 2020 11:39:11 -0500 Subject: [PATCH 3/3] changes per review * Add a client_config option to Ns1Provider, for passing additional options or overrides to the SDK config. This should allow NS1 users some flexibility without bothering octodns so much. * Expose the actual SDK client object as `_client` on the Ns1Client wrapper * Do my best to clarify options and defaults in the Ns1Provider docstring --- octodns/provider/ns1.py | 46 +++++++++++++++++++++--------- tests/test_octodns_provider_ns1.py | 25 ++++++++++------ 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/octodns/provider/ns1.py b/octodns/provider/ns1.py index 2bfaec1..96b648d 100644 --- a/octodns/provider/ns1.py +++ b/octodns/provider/ns1.py @@ -7,7 +7,7 @@ from __future__ import absolute_import, division, print_function, \ from logging import getLogger from itertools import chain -from collections import OrderedDict, defaultdict +from collections import Mapping, OrderedDict, defaultdict from ns1 import NS1 from ns1.rest.errors import RateLimitException, ResourceException from pycountry_convert import country_alpha2_to_continent_code @@ -27,9 +27,11 @@ class Ns1Exception(Exception): class Ns1Client(object): log = getLogger('NS1Client') - def __init__(self, api_key, parallelism=None, retry_count=4): - self.log.debug('__init__: parallelism=%s, retry_count=%d', parallelism, - retry_count) + def __init__(self, api_key, parallelism=None, retry_count=4, + client_config=None): + self.log.debug('__init__: parallelism=%s, retry_count=%d, ' + 'client_config=%s', parallelism, retry_count, + client_config) self.retry_count = retry_count client = NS1(apiKey=api_key) @@ -60,7 +62,12 @@ class Ns1Client(object): # we get the full list of records. client.config['follow_pagination'] = True - self._config = client.config + # additional options or overrides + if isinstance(client_config, Mapping): + for k, v in client_config.items(): + client.config[k] = v + + self._client = client self._records = client.records() self._zones = client.zones() @@ -203,14 +210,26 @@ class Ns1Provider(BaseProvider): Ns1 provider ns1: + # Required class: octodns.provider.ns1.Ns1Provider api_key: env/NS1_API_KEY - # Optional, to avoid 429s from rate-limiting. Try setting to the - # value of max_workers. - parallelism: 11 # Only required if using dynamic records monitor_regions: - lga + # Optional. Default: None. If set, back off in advance to avoid 429s + # from rate-limiting. Generally this should be set to the number + # of processes or workers hitting the API, e.g. the value of + # `max_workers`. + parallelism: 11 + # Optional. Default: 4. Number of times to retry if a 429 response + # is received. + retry_count: 4 + # Optional. Default: None. Additional options or overrides passed to + # the NS1 SDK config, as key-value pairs. + client_config: + endpoint: my.nsone.endpoint # Default: api.nsone.net + ignore-ssl-errors: true # Default: false + follow_pagination: false # Default: true ''' SUPPORTS_GEO = True SUPPORTS_DYNAMIC = True @@ -268,15 +287,16 @@ class Ns1Provider(BaseProvider): } def __init__(self, id, api_key, retry_count=4, monitor_regions=None, - parallelism=None, *args, **kwargs): + parallelism=None, client_config=None, *args, **kwargs): self.log = getLogger('Ns1Provider[{}]'.format(id)) self.log.debug('__init__: id=%s, api_key=***, retry_count=%d, ' - 'monitor_regions=%s, parallelism=%s', id, retry_count, - monitor_regions, parallelism) + 'monitor_regions=%s, parallelism=%s, client_config=%s', + id, retry_count, monitor_regions, parallelism, + client_config) super(Ns1Provider, self).__init__(id, *args, **kwargs) self.monitor_regions = monitor_regions - - self._client = Ns1Client(api_key, parallelism, retry_count) + self._client = Ns1Client(api_key, parallelism, retry_count, + client_config) def _encode_notes(self, data): return ' '.join(['{}:{}'.format(k, v) diff --git a/tests/test_octodns_provider_ns1.py b/tests/test_octodns_provider_ns1.py index fb3bec0..f21dafd 100644 --- a/tests/test_octodns_provider_ns1.py +++ b/tests/test_octodns_provider_ns1.py @@ -1394,21 +1394,28 @@ class TestNs1Client(TestCase): def test_client_config(self): with self.assertRaises(TypeError): - client = Ns1Client() + Ns1Client() client = Ns1Client('dummy-key') self.assertEquals( - client._config.get('keys'), - {'default': {'key': u'dummy-key', 'desc': 'imported API key'}} - ) - self.assertEquals(client._config.get('rate_limit_strategy'), None) - self.assertEquals(client._config.get('parallelism'), None) + client._client.config.get('keys'), + {'default': {'key': u'dummy-key', 'desc': 'imported API key'}}) + self.assertEquals(client._client.config.get('follow_pagination'), True) + self.assertEquals( + client._client.config.get('rate_limit_strategy'), None) + self.assertEquals(client._client.config.get('parallelism'), None) client = Ns1Client('dummy-key', parallelism=11) self.assertEquals( - client._config.get('rate_limit_strategy'), 'concurrent' - ) - self.assertEquals(client._config.get('parallelism'), 11) + client._client.config.get('rate_limit_strategy'), 'concurrent') + self.assertEquals(client._client.config.get('parallelism'), 11) + + client = Ns1Client('dummy-key', client_config={ + 'endpoint': 'my.endpoint.com', 'follow_pagination': False}) + self.assertEquals( + client._client.config.get('endpoint'), 'my.endpoint.com') + self.assertEquals( + client._client.config.get('follow_pagination'), False) @patch('ns1.rest.data.Source.list') @patch('ns1.rest.data.Source.create')