From 6a3e17cb9eae2216c2be2d7c257cced6adb1d30f Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Tue, 7 Mar 2017 12:33:36 +0100 Subject: [PATCH 01/31] Fix some typos in README.md #346 --- README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index a9df9fe..c597812 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ Steps to deploy a new Data Services API version : - install server and client extensions ``` - # in data-services repo root path: + # in dataservices-api repo root path: cd client && sudo make install cd server/extension && sudo make install ``` @@ -50,7 +50,7 @@ Steps to deploy a new Data Services API version : - install python library ``` - # in data-services repo root path: + # in dataservices-api repo root path: cd server/lib/python/cartodb_services && sudo pip install . --upgrade ``` @@ -77,13 +77,13 @@ Steps to deploy a new Data Services API version : SELECT CDB_Conf_SetConf('redis_metrics_config', '{"redis_host": "localhost", "redis_port": 6379, "sentinel_master_id": "", "timeout": 0.1, "redis_db": 5}'); SELECT CDB_Conf_SetConf('heremaps_conf', '{"geocoder": {"app_id": "here_geocoder_app_id", "app_code": "here_geocoder_app_code", "geocoder_cost_per_hit": "1"}, "isolines" : {"app_id": "here_isolines_app_id", "app_code": "here_geocoder_app_code"}}'); - SELECT CDB_Conf_SetConf('user_config', '{"is_organization": false, "entity_name": ""}') + SELECT CDB_Conf_SetConf('user_config', '{"is_organization": false, "entity_name": ""}'); SELECT CDB_Conf_SetConf('mapzen_conf', '{"routing": {"api_key": "valhalla_app_key", "monthly_quota": 999999}, "geocoder": {"api_key": "search_app_key", "monthly_quota": 999999}, "matrix": {"api_key": "[your_matrix_key]", "monthly_quota": 1500000}}'); SELECT CDB_Conf_SetConf('logger_conf', '{"geocoder_log_path": "/tmp/geocodings.log", [ "min_log_level": "[debug|info|warning|error]", "rollbar_api_key": "SERVER_SIDE_API_KEY", "log_file_path": "LOG_FILE_PATH"]}'); SELECT CDB_Conf_SetConf('data_observatory_conf', '{"connection": {"whitelist": [], "production": "host=localhost port=5432 dbname=dataservices_db user=geocoder_api", "staging": "host=localhost port=5432 dbname=dataservices_db user=geocoder_api"}}'); # Environment to decide: rollbar message, which servers for third party use, etc. If not setted uses production by default (current behavior) - SELECT CDB_Conf_SetConf('server_conf', '{"environment": "[development|staging|production]"}') + SELECT CDB_Conf_SetConf('server_conf', '{"environment": "[development|staging|production]"}'); ``` - configure the user DB: From 25ba9866aea3cec96accd64c5594c9c73d90ed4e Mon Sep 17 00:00:00 2001 From: Rafa de la Torre Date: Tue, 7 Mar 2017 13:31:06 +0100 Subject: [PATCH 02/31] Small PoC with rratelimit #346 --- server/extension/sql/20_geocode_street.sql | 10 ++++++++++ server/lib/python/cartodb_services/requirements.txt | 3 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index b4fbcd1..91b87db 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -169,6 +169,16 @@ RETURNS Geometry AS $$ redis_metrics_connection = RedisMetricsConnectionFactory(environment, server_config_backend).get() + #-- e.g: RateLimit(service='geocoder', user=username, max_requests=2, period=60) + #-- rate_limiter = RateLimitBuilder(service='geocoder', user=username) + #-- How to pass the redis config along? + #-- rate_limiter_geocoder_config = RateLimiterUserConfigFactory(redis_metrics_connection, service='geocoder', user=username) + from rratelimit import Limiter + rate_limiter = Limiter(redis_metrics_connection, action='geocode', limit=2, period=60) + if not rate_limiter.checked_insert(username): + raise Exception('Rate limit exceeded') + + quota_service = QuotaService(mapzen_geocoder_config, redis_metrics_connection) if not quota_service.check_user_quota(): raise Exception('You have reached the limit of your quota') diff --git a/server/lib/python/cartodb_services/requirements.txt b/server/lib/python/cartodb_services/requirements.txt index 1b8b873..822d980 100644 --- a/server/lib/python/cartodb_services/requirements.txt +++ b/server/lib/python/cartodb_services/requirements.txt @@ -4,7 +4,8 @@ python-dateutil==2.2 googlemaps==2.4.2 rollbar==0.13.2 # Dependency for googlemaps package -requests<=2.9.1 +requests==2.9.1 +rratelimit==0.0.4 # Test mock==1.3.0 From 250d384d061593c0e707883f72c295b206d4cfc9 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 9 Mar 2017 18:42:48 +0100 Subject: [PATCH 03/31] Geocoder, per user, cofigurable rate limits WIP, specially the GeocoderConfig part is flaky and ugly --- server/extension/sql/20_geocode_street.sql | 26 +++++--- .../cartodb_services/metrics/config.py | 27 +++++++- .../refactor/config/rate_limits.py | 63 +++++++++++++++++++ .../cartodb_services/tools/rate_limiter.py | 17 +++++ 4 files changed, 123 insertions(+), 10 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py create mode 100644 server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index 91b87db..521eb9f 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -75,6 +75,8 @@ RETURNS Geometry AS $$ from cartodb_services.here import HereMapsGeocoder from cartodb_services.metrics import QuotaService from cartodb_services.tools import Logger,LoggerConfig + from cartodb_services.tools import RateLimiter + from cartodb_services.refactor.config.rate_limits import RateLimitsConfig redis_conn = GD["redis_connection_{0}".format(username)]['redis_metrics_connection'] user_geocoder_config = GD["user_geocoder_config_{0}".format(username)] @@ -82,6 +84,15 @@ RETURNS Geometry AS $$ plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") logger_config = GD["logger_config"] logger = Logger(logger_config) + + rate_limits_config = RateLimitsConfig('geocoder', + username, + user_geocoder_config.rate_limit.get('limit'), + user_geocoder_config.rate_limit.get('period')) + rate_limiter = RateLimiter(rate_limits_config, redis_conn) + if not rate_limiter.check(): + raise Exception('Rate limit exceeded') + # -- Check the quota quota_service = QuotaService(user_geocoder_config, redis_conn) if not quota_service.check_user_quota(): @@ -115,7 +126,7 @@ RETURNS Geometry AS $$ redis_conn = GD["redis_connection_{0}".format(username)]['redis_metrics_connection'] user_geocoder_config = GD["user_geocoder_config_{0}".format(username)] - + plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") logger_config = GD["logger_config"] logger = Logger(logger_config) @@ -149,6 +160,7 @@ RETURNS Geometry AS $$ from cartodb_services.mapzen.types import country_to_iso3 from cartodb_services.metrics import QuotaService from cartodb_services.tools import Logger + from cartodb_services.tools import RateLimiter from cartodb_services.refactor.tools.logger import LoggerConfigBuilder from cartodb_services.refactor.service.mapzen_geocoder_config import MapzenGeocoderConfigBuilder from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder @@ -156,6 +168,7 @@ RETURNS Geometry AS $$ from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory + from cartodb_services.refactor.config.rate_limits import RateLimitsConfigBuilder server_config_backend = ServerConfigBackendFactory().get() environment = ServerEnvironmentBuilder(server_config_backend).get() @@ -166,19 +179,14 @@ RETURNS Geometry AS $$ logger = Logger(logger_config) mapzen_geocoder_config = MapzenGeocoderConfigBuilder(server_config_backend, user_config_backend, org_config_backend, username, orgname).get() + rate_limit_config = RateLimiterConfigBuilder(server_config_backend, user_config_backend, org_config_backend, service='geocoder' user=username, org=orgname).get() redis_metrics_connection = RedisMetricsConnectionFactory(environment, server_config_backend).get() - #-- e.g: RateLimit(service='geocoder', user=username, max_requests=2, period=60) - #-- rate_limiter = RateLimitBuilder(service='geocoder', user=username) - #-- How to pass the redis config along? - #-- rate_limiter_geocoder_config = RateLimiterUserConfigFactory(redis_metrics_connection, service='geocoder', user=username) - from rratelimit import Limiter - rate_limiter = Limiter(redis_metrics_connection, action='geocode', limit=2, period=60) - if not rate_limiter.checked_insert(username): + rate_limiter = RateLimiter(rate_limit_config, redis_metrics_connection) + if not rate_limiter.check(): raise Exception('Rate limit exceeded') - quota_service = QuotaService(mapzen_geocoder_config, redis_metrics_connection) if not quota_service.check_user_quota(): raise Exception('You have reached the limit of your quota') diff --git a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py index 14638f3..a3ff3bb 100644 --- a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py +++ b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py @@ -56,6 +56,13 @@ class ServiceConfig(object): else: return None + def _get_rate_limit(self, service): + rate_limit_key = "{0}_rate_limit".format(service) + rate_limit_json = self._redis_config.get(rate_limit_key, None) + if (rate_limit_json): + return rate_limit_json and json.loads(rate_limit_json) + else: + return self._db_config.rate_limits.get(service, {}) class DataObservatoryConfig(ServiceConfig): @@ -376,6 +383,8 @@ class GeocoderConfig(ServiceConfig): self._mapzen_api_key = db_config.mapzen_geocoder_api_key self._cost_per_hit = 0 + self._rate_limit = self._get_rate_limit('geocoder') + @property def service_type(self): if self._geocoder_provider == self.GOOGLE_GEOCODER: @@ -444,6 +453,9 @@ class GeocoderConfig(ServiceConfig): def provider(self): return self._geocoder_provider + @property + def rate_limit(self): + return self._rate_limit class ServicesDBConfig: @@ -458,6 +470,7 @@ class ServicesDBConfig: self._get_here_config() self._get_mapzen_config() self._get_data_observatory_config() + self._get_rate_limits_config() def _get_server_config(self): server_config_json = self._get_conf('server_conf') @@ -509,13 +522,17 @@ class ServicesDBConfig: else: self._data_observatory_connection_str = do_conf['connection']['production'] + def _get_rate_limits_config(self): + self._rate_limits = self._get_conf('rate_limits', default={}) - def _get_conf(self, key): + def _get_conf(self, key, default='raise'): try: sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(key) conf = self._db_conn.execute(sql, 1) return conf[0]['conf'] except Exception as e: + if (default != 'raise'): + return default raise ConfigException("Error trying to get config for {0}: {1}".format(key, e)) @property @@ -570,6 +587,10 @@ class ServicesDBConfig: def data_observatory_connection_str(self): return self._data_observatory_connection_str + @property + def rate_limits(self): + return self._rate_limits + @property def logger_config(self): logger_conf_json = self._get_conf('logger_conf') @@ -592,6 +613,7 @@ class ServicesRedisConfig: GEOCODER_PROVIDER_KEY = 'geocoder_provider' ISOLINES_PROVIDER_KEY = 'isolines_provider' ROUTING_PROVIDER_KEY = 'routing_provider' + GEOCODING_RATE_LIMIT_KEY = 'geocoder_rate_limit' def __init__(self, redis_conn): self._redis_connection = redis_conn @@ -646,3 +668,6 @@ class ServicesRedisConfig: user_config[self.ISOLINES_PROVIDER_KEY] = org_config[self.ISOLINES_PROVIDER_KEY] if self.ROUTING_PROVIDER_KEY in org_config: user_config[self.ROUTING_PROVIDER_KEY] = org_config[self.ROUTING_PROVIDER_KEY] + # for rate limit parameters, user config has precedence over organization + if self.GEOCODING_RATE_LIMIT_KEY in org_config and not self.GEOCODING_RATE_LIMIT_KEY in user_config: + user_config[self.GEOCODING_RATE_LIMIT_KEY] = org_config[self.GEOCODING_RATE_LIMIT_KEY] diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py new file mode 100644 index 0000000..9a2b6de --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py @@ -0,0 +1,63 @@ +class RateLimitsConfig(object): + """ + Value object that represents the configuration needed to rate-limit services + """ + + def __init__(self, + service, + username, + limit, + period): + self._service = service + self._username = username + self._limit = limit and int(limit) + self._period = period and int(period) + + # service this limit applies to + @property + def service(self): + return self._service + + # user this limit applies to + @property + def username(self): + return self._username + + # rate period in seconds + @property + def period(self): + return self._period + + # rate limit in seconds + @property + def limit(self): + return self._limit + + def is_limited(self): + return self._limit and self._limit > 0 and self._period and self._period > 0 + + +class RateLimitsConfigBuilder(object): + + def __init__(self, server_conf, user_conf, org_conf, service, user, org): + self._server_conf = server_conf + self._user_conf = user_conf + self._org_conf = org_conf + self._service = service + self._username = user + self._orgname = org + + def get(self): + # Order of precedence is user_conf, org_conf, server_conf + + rate_limit_key = "{0}_rate_limit".format(service) + rate_limit_json = self._user_conf.get(rate_limit_key, None) or self._org_conf.get(rate_limit_key, None) + if (rate_limit_json): + rate_limit = rate_limit_json and json.loads(rate_limit_json) + else: + rate_limit = self._server_conf.get('rate_limits', {}).get(service, {}) + + return RateLimitsConfig(self._service, + self._username, + self._limit, + self._period) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py b/server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py new file mode 100644 index 0000000..16cf3f2 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py @@ -0,0 +1,17 @@ +from rratelimit import Limiter + +class RateLimiter: + + def __init__(self, rate_limits_config, redis_connection): + self._config = rate_limits_config + if (self._config.is_limited()): + self._limiter = Limiter(rdis_connection, + action=self._config.service, + limit=self._config.limit, + period=self._config.period) + + def check(): + ok = True + if (self._limiter): + ok = self._limiter.checked_insert(self._config.username) + return ok From 6c5ca97468fc3c08fb0a946e813239d72ccf93fc Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 14 Mar 2017 18:51:18 +0100 Subject: [PATCH 04/31] Complete rate-limiting for Mapzen & Here gecoding ServiceManager class has been introduced to handle service configuration at SQL level (with a LegacyServiceManager alternative for non-refactored services). These new classes take the responsibility of rate limits and quota checking. Tests have been added for ServiceManager and rate limits, but currently they check only the limits configuration since Lua support would be needed to use rratelimit with MockRedis. --- server/extension/sql/20_geocode_street.sql | 96 +++----- .../cartodb_services/metrics/config.py | 6 +- .../refactor/config/rate_limits.py | 9 +- .../refactor/core/interfaces.py | 2 +- .../refactor/storage/mem_config.py | 4 +- .../refactor/storage/null_config.py | 4 +- .../refactor/storage/redis_config.py | 8 +- .../refactor/storage/server_config.py | 15 +- .../cartodb_services/tools/__init__.py | 3 + .../cartodb_services/tools/exceptions.py | 3 + .../cartodb_services/tools/rate_limiter.py | 5 +- .../cartodb_services/tools/service_manager.py | 70 ++++++ .../test/test_servicemanager.py | 218 ++++++++++++++++++ 13 files changed, 359 insertions(+), 84 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py create mode 100644 server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py create mode 100644 server/lib/python/cartodb_services/test/test_servicemanager.py diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index 521eb9f..fd85d6d 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -72,34 +72,20 @@ $$ LANGUAGE plpythonu; CREATE OR REPLACE FUNCTION cdb_dataservices_server._cdb_here_geocode_street_point(username TEXT, orgname TEXT, searchtext TEXT, city TEXT DEFAULT NULL, state_province TEXT DEFAULT NULL, country TEXT DEFAULT NULL) RETURNS Geometry AS $$ + from cartodb_services.tools import ServiceManager from cartodb_services.here import HereMapsGeocoder - from cartodb_services.metrics import QuotaService - from cartodb_services.tools import Logger,LoggerConfig - from cartodb_services.tools import RateLimiter - from cartodb_services.refactor.config.rate_limits import RateLimitsConfig - redis_conn = GD["redis_connection_{0}".format(username)]['redis_metrics_connection'] - user_geocoder_config = GD["user_geocoder_config_{0}".format(username)] + # from cartodb_services.metrics import QuotaService + # from cartodb_services.tools import Logger,LoggerConfig + # from cartodb_services.tools import RateLimiter + # from cartodb_services.refactor.config.rate_limits import RateLimitsConfig plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") - logger_config = GD["logger_config"] - logger = Logger(logger_config) - - rate_limits_config = RateLimitsConfig('geocoder', - username, - user_geocoder_config.rate_limit.get('limit'), - user_geocoder_config.rate_limit.get('period')) - rate_limiter = RateLimiter(rate_limits_config, redis_conn) - if not rate_limiter.check(): - raise Exception('Rate limit exceeded') - - # -- Check the quota - quota_service = QuotaService(user_geocoder_config, redis_conn) - if not quota_service.check_user_quota(): - raise Exception('You have reached the limit of your quota') + service_manager = LegacyServiceManager('geocoder', username, orgname, GD) + service_manager.check() try: - geocoder = HereMapsGeocoder(user_geocoder_config.heremaps_app_id, user_geocoder_config.heremaps_app_code, logger) + geocoder = HereMapsGeocoder(service_manager.config.heremaps_app_id, service_manager.config.heremaps_app_code, logger) coordinates = geocoder.geocode(searchtext=searchtext, city=city, state=state_province, country=country) if coordinates: quota_service.increment_success_service_use() @@ -107,15 +93,15 @@ RETURNS Geometry AS $$ point = plpy.execute(plan, [coordinates[0], coordinates[1]], 1)[0] return point['st_setsrid'] else: - quota_service.increment_empty_service_use() + service_manager.quota_service.increment_empty_service_use() return None except BaseException as e: import sys - quota_service.increment_failed_service_use() + service_manager.quota_service.increment_failed_service_use() logger.error('Error trying to geocode street point using here maps', sys.exc_info(), data={"username": username, "orgname": orgname}) raise Exception('Error trying to geocode street point using here maps') finally: - quota_service.increment_total_service_use() + service_manager.quota_service.increment_total_service_use() $$ LANGUAGE plpythonu; CREATE OR REPLACE FUNCTION cdb_dataservices_server._cdb_google_geocode_street_point(username TEXT, orgname TEXT, searchtext TEXT, city TEXT DEFAULT NULL, state_province TEXT DEFAULT NULL, country TEXT DEFAULT NULL) @@ -154,64 +140,48 @@ $$ LANGUAGE plpythonu; CREATE OR REPLACE FUNCTION cdb_dataservices_server._cdb_mapzen_geocode_street_point(username TEXT, orgname TEXT, searchtext TEXT, city TEXT DEFAULT NULL, state_province TEXT DEFAULT NULL, country TEXT DEFAULT NULL) RETURNS Geometry AS $$ - import cartodb_services - cartodb_services.init(plpy, GD) + from cartodb_services.tools import ServiceManager from cartodb_services.mapzen import MapzenGeocoder from cartodb_services.mapzen.types import country_to_iso3 - from cartodb_services.metrics import QuotaService - from cartodb_services.tools import Logger - from cartodb_services.tools import RateLimiter - from cartodb_services.refactor.tools.logger import LoggerConfigBuilder from cartodb_services.refactor.service.mapzen_geocoder_config import MapzenGeocoderConfigBuilder - from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder - from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory - from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory - from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory - from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory - from cartodb_services.refactor.config.rate_limits import RateLimitsConfigBuilder + # from cartodb_services.metrics import QuotaService + # from cartodb_services.tools import Logger + # from cartodb_services.tools import RateLimiter + # from cartodb_services.refactor.tools.logger import LoggerConfigBuilder + # from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder + # from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory + # from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory + # from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory + # from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory + # from cartodb_services.refactor.config.rate_limits import RateLimitsConfigBuilder - server_config_backend = ServerConfigBackendFactory().get() - environment = ServerEnvironmentBuilder(server_config_backend).get() - user_config_backend = UserConfigBackendFactory(username, environment, server_config_backend).get() - org_config_backend = OrgConfigBackendFactory(orgname, environment, server_config_backend).get() + import cartodb_services + cartodb_services.init(plpy, GD) - logger_config = LoggerConfigBuilder(environment, server_config_backend).get() - logger = Logger(logger_config) - - mapzen_geocoder_config = MapzenGeocoderConfigBuilder(server_config_backend, user_config_backend, org_config_backend, username, orgname).get() - rate_limit_config = RateLimiterConfigBuilder(server_config_backend, user_config_backend, org_config_backend, service='geocoder' user=username, org=orgname).get() - - redis_metrics_connection = RedisMetricsConnectionFactory(environment, server_config_backend).get() - - rate_limiter = RateLimiter(rate_limit_config, redis_metrics_connection) - if not rate_limiter.check(): - raise Exception('Rate limit exceeded') - - quota_service = QuotaService(mapzen_geocoder_config, redis_metrics_connection) - if not quota_service.check_user_quota(): - raise Exception('You have reached the limit of your quota') + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, username, orgname) + service_manager.check() try: - geocoder = MapzenGeocoder(mapzen_geocoder_config.mapzen_api_key, logger) + geocoder = MapzenGeocoder(service_manager.config.mapzen_api_key, logger) country_iso3 = None if country: - country_iso3 = country_to_iso3(country) + untry_iso3 = country_to_iso3(country) coordinates = geocoder.geocode(searchtext=searchtext, city=city, state_province=state_province, country=country_iso3, search_type='address') if coordinates: - quota_service.increment_success_service_use() + service_manager.quota_service.quota_service.increment_success_service_use() plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326); ", ["double precision", "double precision"]) point = plpy.execute(plan, [coordinates[0], coordinates[1]], 1)[0] return point['st_setsrid'] else: - quota_service.increment_empty_service_use() + service_manager.quota_service.increment_empty_service_use() return None except BaseException as e: import sys - quota_service.increment_failed_service_use() - logger.error('Error trying to geocode street point using mapzen', sys.exc_info(), data={"username": username, "orgname": orgname}) + service_manager.quota_service.increment_failed_service_use() + service_manager.logger.error('Error trying to geocode street point using mapzen', sys.exc_info(), data={"username": username, "orgname": orgname}) raise Exception('Error trying to geocode street point using mapzen') finally: - quota_service.increment_total_service_use() + service_manager.quota_service.increment_total_service_use() $$ LANGUAGE plpythonu; diff --git a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py index a3ff3bb..d4328a1 100644 --- a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py +++ b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py @@ -523,15 +523,15 @@ class ServicesDBConfig: self._data_observatory_connection_str = do_conf['connection']['production'] def _get_rate_limits_config(self): - self._rate_limits = self._get_conf('rate_limits', default={}) + self._rate_limits = json.loads(self._get_conf('rate_limits', default='{}')) - def _get_conf(self, key, default='raise'): + def _get_conf(self, key, default=KeyError): try: sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(key) conf = self._db_conn.execute(sql, 1) return conf[0]['conf'] except Exception as e: - if (default != 'raise'): + if (default != KeyError): return default raise ConfigException("Error trying to get config for {0}: {1}".format(key, e)) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py index 9a2b6de..80d92a0 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py @@ -50,14 +50,15 @@ class RateLimitsConfigBuilder(object): def get(self): # Order of precedence is user_conf, org_conf, server_conf - rate_limit_key = "{0}_rate_limit".format(service) + rate_limit_key = "{0}_rate_limit".format(self._service) rate_limit_json = self._user_conf.get(rate_limit_key, None) or self._org_conf.get(rate_limit_key, None) if (rate_limit_json): rate_limit = rate_limit_json and json.loads(rate_limit_json) else: - rate_limit = self._server_conf.get('rate_limits', {}).get(service, {}) + rate_limit = self._server_conf.get('rate_limits', {}).get(self._service, {}) return RateLimitsConfig(self._service, self._username, - self._limit, - self._period) + rate_limit.get('limit', None), + rate_limit.get('period', None)) + diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/core/interfaces.py b/server/lib/python/cartodb_services/cartodb_services/refactor/core/interfaces.py index 46dcd42..cb69b6f 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/core/interfaces.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/core/interfaces.py @@ -6,6 +6,6 @@ class ConfigBackendInterface(object): __metaclass__ = abc.ABCMeta @abc.abstractmethod - def get(self, key): + def get(self, key, default=None): """Return a value based on the key supplied from some storage""" pass diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/mem_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/mem_config.py index 0661626..148b970 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/mem_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/mem_config.py @@ -5,8 +5,8 @@ class InMemoryConfigStorage(ConfigBackendInterface): def __init__(self, config_hash={}): self._config_hash = config_hash - def get(self, key): + def get(self, key, default=None): try: return self._config_hash[key] except KeyError: - return None + return default diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/null_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/null_config.py index b9c11e6..c080e13 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/null_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/null_config.py @@ -2,5 +2,5 @@ from ..core.interfaces import ConfigBackendInterface class NullConfigStorage(ConfigBackendInterface): - def get(self, key): - return None + def get(self, key, default=None): + return default diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py index cdb2e57..1e1f33c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py @@ -9,11 +9,13 @@ class RedisConfigStorage(ConfigBackendInterface): self._config_key = config_key self._data = None - def get(self, key): + def get(self, key, default=KeyError): if not self._data: self._data = self._connection.hgetall(self._config_key) - return self._data[key] - + if (default == KeyError): + return self._data[key] + else: + return self._data.get(key, default) class RedisUserConfigStorageBuilder(object): def __init__(self, redis_connection, username): diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py index 50137ca..1d3603c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py @@ -4,11 +4,18 @@ from ..core.interfaces import ConfigBackendInterface class InDbServerConfigStorage(ConfigBackendInterface): - def get(self, key): + def get(self, key, default=None): sql = "SELECT cdb_dataservices_server.cdb_conf_getconf('{0}') as conf".format(key) rows = cartodb_services.plpy.execute(sql, 1) - json_output = rows[0]['conf'] - if json_output: + json_output = None + try: + json_output = rows[0]['conf'] + except IndexError: + pass + if (json_output): return json.loads(json_output) else: - return None + if (default == KeyError): + raise KeyError + else: + return default diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py b/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py index 7bea3e2..0658841 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py @@ -2,3 +2,6 @@ from redis_tools import RedisConnection, RedisDBConfig from coordinates import Coordinate from polyline import PolyLine from log import Logger, LoggerConfig +from rate_limiter import RateLimiter +from exceptions import RateLimitExceeded +from service_manager import ServiceManager, LegacyServiceManager diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py b/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py new file mode 100644 index 0000000..f8b79c1 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py @@ -0,0 +1,3 @@ +class RateLimitExceeded(Exception): + def __str__(self): + return repr('Rate limit exceeded') diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py b/server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py index 16cf3f2..3410519 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/rate_limiter.py @@ -4,13 +4,14 @@ class RateLimiter: def __init__(self, rate_limits_config, redis_connection): self._config = rate_limits_config + self._limiter = None if (self._config.is_limited()): - self._limiter = Limiter(rdis_connection, + self._limiter = Limiter(redis_connection, action=self._config.service, limit=self._config.limit, period=self._config.period) - def check(): + def check(self): ok = True if (self._limiter): ok = self._limiter.checked_insert(self._config.username) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py new file mode 100644 index 0000000..03c8fb4 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py @@ -0,0 +1,70 @@ +from exceptions import RateLimitExceeded + +class ServiceManagerBase: + def check(self): + if not self.rate_limiter.check(): + raise RateLimitExceeded() + if not self.quota_service.check_user_quota(): + raise Exception('You have reached the limit of your quota') + + @property + def config(self): + return self.config + + @property + def quota_service(self): + return self.quota_service + + @property + def logger(self): + return self.logger + +from cartodb_services.metrics import QuotaService +from cartodb_services.tools import Logger,LoggerConfig +from cartodb_services.tools import RateLimiter +from cartodb_services.refactor.config.rate_limits import RateLimitsConfig + +class LegacyServiceManager(ServiceManagerBase): + + def __init__(self, service, username, orgname, gd): + redis_conn = gd["redis_connection_{0}".format(username)]['redis_metrics_connection'] + self.config = gd["user_{0}_config_{1}".format(service, username)] + logger_config = gd["logger_config"] + self.logger = Logger(logger_config) + + rate_limits_config = RateLimitsConfig(service, + username, + self.config.rate_limit.get('limit'), + self.config.rate_limit.get('period')) + self.rate_limiter = RateLimiter(rate_limits_config, redis_conn) + self.quota_service = QuotaService(self.config, redis_conn) + +from cartodb_services.metrics import QuotaService +from cartodb_services.tools import Logger +from cartodb_services.tools import RateLimiter +from cartodb_services.refactor.tools.logger import LoggerConfigBuilder +from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder +from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory +from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory +from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory +from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory +from cartodb_services.refactor.config.rate_limits import RateLimitsConfigBuilder + +class ServiceManager(ServiceManagerBase): + + def __init__(self, service, config_builder, username, orgname): + server_config_backend = ServerConfigBackendFactory().get() + environment = ServerEnvironmentBuilder(server_config_backend).get() + user_config_backend = UserConfigBackendFactory(username, environment, server_config_backend).get() + org_config_backend = OrgConfigBackendFactory(orgname, environment, server_config_backend).get() + + logger_config = LoggerConfigBuilder(environment, server_config_backend).get() + self.logger = Logger(logger_config) + + self.config = config_builder(server_config_backend, user_config_backend, org_config_backend, username, orgname).get() + rate_limit_config = RateLimitsConfigBuilder(server_config_backend, user_config_backend, org_config_backend, service=service, user=username, org=orgname).get() + + redis_metrics_connection = RedisMetricsConnectionFactory(environment, server_config_backend).get() + + self.rate_limiter = RateLimiter(rate_limit_config, redis_metrics_connection) + self.quota_service = QuotaService(self.config, redis_metrics_connection) diff --git a/server/lib/python/cartodb_services/test/test_servicemanager.py b/server/lib/python/cartodb_services/test/test_servicemanager.py new file mode 100644 index 0000000..b7874ab --- /dev/null +++ b/server/lib/python/cartodb_services/test/test_servicemanager.py @@ -0,0 +1,218 @@ +from test_helper import * +from unittest import TestCase +from mock import Mock, MagicMock, patch +from nose.tools import assert_raises, assert_not_equal, assert_equal + +from datetime import datetime, date +from cartodb_services.tools import ServiceManager, LegacyServiceManager +from mockredis import MockRedis +import cartodb_services +from cartodb_services.metrics import GeocoderConfig +from cartodb_services.refactor.service.mapzen_geocoder_config import MapzenGeocoderConfigBuilder +from cartodb_services.refactor.backend.redis_metrics_connection import RedisConnectionBuilder +from cartodb_services.tools import RateLimitExceeded + +LUA_AVAILABLE_FOR_MOCKREDIS = False + +class MockRedisWithVersionInfo(MockRedis): + def info(self): + return {'redis_version': '3.0.2'} + +class TestServiceManager(TestCase): + + def setUp(self): + plpy_mock_config() + cartodb_services.init(plpy_mock, _GD={}) + self.username = 'test_user' + self.orgname = 'test_org' + self.redis_conn = MockRedisWithVersionInfo() + build_redis_user_config(self.redis_conn, self.username, 'geocoding') + build_redis_org_config(self.redis_conn, self.orgname, 'geocoding', provider='mapzen') + self.environment = 'production' + plpy_mock._define_result("CDB_Conf_GetConf\('server_conf'\)", [{'conf': '{"environment": "production"}'}]) + plpy_mock._define_result("CDB_Conf_GetConf\('redis_metadata_config'\)", [{'conf': '{"redis_host":"localhost","redis_port":"6379"}'}]) + plpy_mock._define_result("CDB_Conf_GetConf\('redis_metrics_config'\)", [{'conf': '{"redis_host":"localhost","redis_port":"6379"}'}]) + + def check_rate_limit(self, service_manager, n, active=True): + if LUA_AVAILABLE_FOR_MOCKREDIS: + for _ in xrange(n): + service_manager.check() + if active: + with assert_raises(RateLimitExceeded): + service_manager.check() + else: + service_manager.check() + else: + # rratelimit doesn't work with MockRedis because it needs Lua support + # so, we'll simply perform some sanity check on the configuration of the rate limiter + if active: + assert_equal(service_manager.rate_limiter._config.is_limited(), True) + assert_equal(service_manager.rate_limiter._config.limit, n) + else: + assert not service_manager.rate_limiter._config.is_limited() + + def test_legacy_service_manager(self): + config = GeocoderConfig(self.redis_conn, plpy_mock, self.username, self.orgname, 'mapzen') + config_cache = { + 'redis_connection_test_user' : { 'redis_metrics_connection': self.redis_conn }, + 'user_geocoder_config_test_user' : config, + 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) + } + service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) + service_manager.check() + assert_equal(service_manager.config.service_type, 'geocoder_mapzen') + + def test_service_manager(self): + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) + service_manager.check() + assert_equal(service_manager.config.service_type, 'geocoder_mapzen') + + def test_no_rate_limit_by_default(self): + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) + self.check_rate_limit(service_manager, 3, False) + + def test_no_legacy_rate_limit_by_default(self): + config = GeocoderConfig(self.redis_conn, plpy_mock, self.username, self.orgname, 'mapzen') + config_cache = { + 'redis_connection_test_user' : { 'redis_metrics_connection': self.redis_conn }, + 'user_geocoder_config_test_user' : config, + 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) + } + service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) + self.check_rate_limit(service_manager, 3, False) + + def test_legacy_server_rate_limit(self): + rate_limits = '{"geocoder":{"limit":"3","period":3600}}' + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", [{'conf': rate_limits}]) + config = GeocoderConfig(self.redis_conn, plpy_mock, self.username, self.orgname, 'mapzen') + config_cache = { + 'redis_connection_test_user' : { 'redis_metrics_connection': self.redis_conn }, + 'user_geocoder_config_test_user' : config, + 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) + } + service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) + self.check_rate_limit(service_manager, 3) + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) + + def test_server_rate_limit(self): + rate_limits = '{"geocoder":{"limit":"3","period":3600}}' + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", [{'conf': rate_limits}]) + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) + self.check_rate_limit(service_manager, 3) + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) + + def test_user_rate_limit(self): + user_redis_name = "rails:users:{0}".format(self.username) + rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(user_redis_name, 'geocoder_rate_limit', rate_limits) + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(user_redis_name, 'geocoder_rate_limit') + + def test_legacy_user_rate_limit(self): + user_redis_name = "rails:users:{0}".format(self.username) + rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(user_redis_name, 'geocoder_rate_limit', rate_limits) + config = GeocoderConfig(self.redis_conn, plpy_mock, self.username, self.orgname, 'mapzen') + config_cache = { + 'redis_connection_test_user' : { 'redis_metrics_connection': self.redis_conn }, + 'user_geocoder_config_test_user' : config, + 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) + } + service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(user_redis_name, 'geocoder_rate_limit') + + def test_org_rate_limit(self): + org_redis_name = "rails:orgs:{0}".format(self.orgname) + rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(org_redis_name, 'geocoder_rate_limit', rate_limits) + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(org_redis_name, 'geocoder_rate_limit') + + def test_legacy_org_rate_limit(self): + org_redis_name = "rails:orgs:{0}".format(self.orgname) + rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(org_redis_name, 'geocoder_rate_limit', rate_limits) + config = GeocoderConfig(self.redis_conn, plpy_mock, self.username, self.orgname, 'mapzen') + config_cache = { + 'redis_connection_test_user' : { 'redis_metrics_connection': self.redis_conn }, + 'user_geocoder_config_test_user' : config, + 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) + } + service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(org_redis_name, 'geocoder_rate_limit') + + def test_user_rate_limit_precedence_over_org(self): + org_redis_name = "rails:orgs:{0}".format(self.orgname) + org_rate_limits = '{"limit":"1000","period":3600}' + self.redis_conn.hset(org_redis_name, 'geocoder_rate_limit', org_rate_limits) + user_redis_name = "rails:users:{0}".format(self.username) + user_rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(user_redis_name, 'geocoder_rate_limit', user_rate_limits) + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(org_redis_name, 'geocoder_rate_limit') + self.redis_conn.hdel(user_redis_name, 'geocoder_rate_limit') + + def test_org_rate_limit_precedence_over_server(self): + server_rate_limits = '{"geocoder":{"limit":"1000","period":3600}}' + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", [{'conf': server_rate_limits}]) + org_redis_name = "rails:orgs:{0}".format(self.orgname) + org_rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(org_redis_name, 'geocoder_rate_limit', org_rate_limits) + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(org_redis_name, 'geocoder_rate_limit') + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) + + def test_legacy_user_rate_limit_precedence_over_org(self): + org_redis_name = "rails:orgs:{0}".format(self.orgname) + org_rate_limits = '{"limit":"1000","period":3600}' + self.redis_conn.hset(org_redis_name, 'geocoder_rate_limit', org_rate_limits) + user_redis_name = "rails:users:{0}".format(self.username) + user_rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(user_redis_name, 'geocoder_rate_limit', user_rate_limits) + config = GeocoderConfig(self.redis_conn, plpy_mock, self.username, self.orgname, 'mapzen') + config_cache = { + 'redis_connection_test_user' : { 'redis_metrics_connection': self.redis_conn }, + 'user_geocoder_config_test_user' : config, + 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) + } + service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(org_redis_name, 'geocoder_rate_limit') + self.redis_conn.hdel(user_redis_name, 'geocoder_rate_limit') + + def test_legacy_org_rate_limit_precedence_over_server(self): + server_rate_limits = '{"geocoder":{"limit":"1000","period":3600}}' + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", [{'conf': server_rate_limits}]) + org_redis_name = "rails:orgs:{0}".format(self.orgname) + org_rate_limits = '{"limit":"3","period":3600}' + self.redis_conn.hset(org_redis_name, 'geocoder_rate_limit', org_rate_limits) + config = GeocoderConfig(self.redis_conn, plpy_mock, self.username, self.orgname, 'mapzen') + config_cache = { + 'redis_connection_test_user' : { 'redis_metrics_connection': self.redis_conn }, + 'user_geocoder_config_test_user' : config, + 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) + } + service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) + self.check_rate_limit(service_manager, 3) + self.redis_conn.hdel(org_redis_name, 'geocoder_rate_limit') + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) From 77b5ff0b9e6368aa96a063f11b212df37d049260 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 15 Mar 2017 12:46:25 +0100 Subject: [PATCH 05/31] Fix problem with db config default arguments --- .../cartodb_services/metrics/config.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py index e87608d..7b44d97 100644 --- a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py +++ b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py @@ -563,17 +563,21 @@ class ServicesDBConfig: self._data_observatory_connection_str = do_conf['connection']['production'] def _get_rate_limits_config(self): - self._rate_limits = json.loads(self._get_conf('rate_limits', default='{}')) + # self._rate_limits = json.loads(self._get_conf('rate_limits', default='{}')) + rl = self._get_conf('rate_limits', default='{}') + self._rate_limits = json.loads(rl) def _get_conf(self, key, default=KeyError): try: sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(key) - conf = self._db_conn.execute(sql, 1) - return conf[0]['conf'] + conf = self._db_conn.execute(sql, 1)[0]['conf'] + if default != KeyError: + conf = conf or default + return conf except Exception as e: - if (default != KeyError): - return default - raise ConfigException("Error trying to get config for {0}: {1}".format(key, e)) + if (default == KeyError): + raise ConfigException("Error trying to get config for {0}: {1}".format(key, e)) + return default @property def server_environment(self): From 548d6b08dbcb3c107f0c3d3cc90afd6d836a0a50 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 15 Mar 2017 12:47:11 +0100 Subject: [PATCH 06/31] Debug SQL geocoding interface which uses ServiceManager --- server/extension/sql/20_geocode_street.sql | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index eaf7dc1..39dffbb 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -75,17 +75,12 @@ RETURNS Geometry AS $$ from cartodb_services.tools import ServiceManager from cartodb_services.here import HereMapsGeocoder - # from cartodb_services.metrics import QuotaService - # from cartodb_services.tools import Logger,LoggerConfig - # from cartodb_services.tools import RateLimiter - # from cartodb_services.refactor.config.rate_limits import RateLimitsConfig - plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") service_manager = LegacyServiceManager('geocoder', username, orgname, GD) service_manager.check() try: - geocoder = HereMapsGeocoder(user_geocoder_config.heremaps_app_id, user_geocoder_config.heremaps_app_code, logger, user_geocoder_config.heremaps_service_params) + geocoder = HereMapsGeocoder(service_manager.config.heremaps_app_id, service_manager.config.heremaps_app_code, service_manager.logger, service_manager.config.heremaps_service_params) coordinates = geocoder.geocode(searchtext=searchtext, city=city, state=state_province, country=country) if coordinates: quota_service.increment_success_service_use() @@ -144,16 +139,6 @@ RETURNS Geometry AS $$ from cartodb_services.mapzen import MapzenGeocoder from cartodb_services.mapzen.types import country_to_iso3 from cartodb_services.refactor.service.mapzen_geocoder_config import MapzenGeocoderConfigBuilder - # from cartodb_services.metrics import QuotaService - # from cartodb_services.tools import Logger - # from cartodb_services.tools import RateLimiter - # from cartodb_services.refactor.tools.logger import LoggerConfigBuilder - # from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder - # from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory - # from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory - # from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory - # from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory - # from cartodb_services.refactor.config.rate_limits import RateLimitsConfigBuilder import cartodb_services cartodb_services.init(plpy, GD) @@ -162,7 +147,7 @@ RETURNS Geometry AS $$ service_manager.check() try: - geocoder = MapzenGeocoder(mapzen_geocoder_config.mapzen_api_key, logger, mapzen_geocoder_config.service_params) + geocoder = MapzenGeocoder(service_manager.config.mapzen_api_key, service_manager.logger, service_manager.config.service_params) country_iso3 = None if country: untry_iso3 = country_to_iso3(country) @@ -170,7 +155,7 @@ RETURNS Geometry AS $$ state_province=state_province, country=country_iso3, search_type='address') if coordinates: - service_manager.quota_service.quota_service.increment_success_service_use() + service_manager.quota_service.increment_success_service_use() plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326); ", ["double precision", "double precision"]) point = plpy.execute(plan, [coordinates[0], coordinates[1]], 1)[0] return point['st_setsrid'] From 680206a437db3877f6748e1f6f04718e3989e850 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 15 Mar 2017 12:51:56 +0100 Subject: [PATCH 07/31] Missing import --- .../cartodb_services/refactor/config/rate_limits.py | 1 + 1 file changed, 1 insertion(+) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py index 80d92a0..70bde8b 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py @@ -1,3 +1,4 @@ +import json class RateLimitsConfig(object): """ Value object that represents the configuration needed to rate-limit services From 9d94f99b415e2a5a101a4c6fc2a7ce06de8a322b Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 15 Mar 2017 13:04:14 +0100 Subject: [PATCH 08/31] Remove incorrect README changes A mistake was introduced when resolving merge conflicts --- README.md | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/README.md b/README.md index 9c6588d..377b9ca 100644 --- a/README.md +++ b/README.md @@ -64,30 +64,6 @@ Steps to deploy a new Data Services API version : create extension cdb_dataservices_client; ``` -- add configuration for different services in server database - - - ``` - # If sentinel is used: - SELECT CDB_Conf_SetConf('redis_metadata_config', '{"sentinel_host": "localhost", "sentinel_port": 26379, "sentinel_master_id": "mymaster", "timeout": 0.1, "redis_db": 5}'); - SELECT CDB_Conf_SetConf('redis_metrics_config', '{"sentinel_host": "localhost", "sentinel_port": 26379, "sentinel_master_id": "mymaster", "timeout": 0.1, "redis_db": 5}'); - - # If sentinel is not used - SELECT CDB_Conf_SetConf('redis_metadata_config', '{"redis_host": "localhost", "redis_port": 6379, "sentinel_master_id": "", "timeout": 0.1, "redis_db": 5}'); - SELECT CDB_Conf_SetConf('redis_metrics_config', '{"redis_host": "localhost", "redis_port": 6379, "sentinel_master_id": "", "timeout": 0.1, "redis_db": 5}'); - - SELECT CDB_Conf_SetConf('heremaps_conf', '{"geocoder": {"app_id": "here_geocoder_app_id", "app_code": "here_geocoder_app_code", "geocoder_cost_per_hit": "1"}, "isolines" : {"app_id": "here_isolines_app_id", "app_code": "here_geocoder_app_code"}}'); - SELECT CDB_Conf_SetConf('user_config', '{"is_organization": false, "entity_name": ""}'); - SELECT CDB_Conf_SetConf('mapzen_conf', '{"routing": {"api_key": "valhalla_app_key", "monthly_quota": 999999}, "geocoder": {"api_key": "search_app_key", "monthly_quota": 999999}, "matrix": {"api_key": "[your_matrix_key]", "monthly_quota": 1500000}}'); - SELECT CDB_Conf_SetConf('logger_conf', '{"geocoder_log_path": "/tmp/geocodings.log", [ "min_log_level": "[debug|info|warning|error]", "rollbar_api_key": "SERVER_SIDE_API_KEY", "log_file_path": "LOG_FILE_PATH"]}'); - SELECT CDB_Conf_SetConf('data_observatory_conf', '{"connection": {"whitelist": [], "production": "host=localhost port=5432 dbname=dataservices_db user=geocoder_api", "staging": "host=localhost port=5432 dbname=dataservices_db user=geocoder_api"}}'); - - # Environment to decide: rollbar message, which servers for third party use, etc. If not setted uses production by default (current behavior) - SELECT CDB_Conf_SetConf('server_conf', '{"environment": "[development|staging|production]"}'); - ``` - -- configure the user DB: - ### Server configuration Configuration for the different services must be stored in the server database using `CDB_Conf_SetConf()`. From 696bdb40b093db57e3acc712bb88df160a5b7029 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 15 Mar 2017 17:49:29 +0100 Subject: [PATCH 09/31] Fix incorrect import --- server/extension/sql/20_geocode_street.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index 39dffbb..6342a53 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -72,7 +72,7 @@ $$ LANGUAGE plpythonu; CREATE OR REPLACE FUNCTION cdb_dataservices_server._cdb_here_geocode_street_point(username TEXT, orgname TEXT, searchtext TEXT, city TEXT DEFAULT NULL, state_province TEXT DEFAULT NULL, country TEXT DEFAULT NULL) RETURNS Geometry AS $$ - from cartodb_services.tools import ServiceManager + from cartodb_services.tools import LegacyServiceManager from cartodb_services.here import HereMapsGeocoder plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") From cebd7d21411a99c0bd14a9fd5de685005985dc7d Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 15 Mar 2017 17:58:14 +0100 Subject: [PATCH 10/31] Apply ServiceManager to Google Geocoder Note that the Goolge geocoder uses QuotaService to account for used quota, but it doesn't have a quota limit. --- server/extension/sql/20_geocode_street.sql | 21 +++++++------------ .../cartodb_services/tools/service_manager.py | 6 +++--- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index 6342a53..30675c4 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -101,33 +101,28 @@ $$ LANGUAGE plpythonu; CREATE OR REPLACE FUNCTION cdb_dataservices_server._cdb_google_geocode_street_point(username TEXT, orgname TEXT, searchtext TEXT, city TEXT DEFAULT NULL, state_province TEXT DEFAULT NULL, country TEXT DEFAULT NULL) RETURNS Geometry AS $$ + from cartodb_services.tools import LegacyServiceManager from cartodb_services.google import GoogleMapsGeocoder - from cartodb_services.metrics import QuotaService - from cartodb_services.tools import Logger,LoggerConfig - - redis_conn = GD["redis_connection_{0}".format(username)]['redis_metrics_connection'] - user_geocoder_config = GD["user_geocoder_config_{0}".format(username)] plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") - logger_config = GD["logger_config"] - logger = Logger(logger_config) - quota_service = QuotaService(user_geocoder_config, redis_conn) + service_manager = LegacyServiceManager('geocoder', username, orgname, GD) + service_manager.check(quota=False) try: - geocoder = GoogleMapsGeocoder(user_geocoder_config.google_client_id, user_geocoder_config.google_api_key, logger) + geocoder = GoogleMapsGeocoder(service_manager.config.google_client_id, service_manager.config.google_api_key, service_manager.logger) coordinates = geocoder.geocode(searchtext=searchtext, city=city, state=state_province, country=country) if coordinates: - quota_service.increment_success_service_use() + service_manager.quota_service.increment_success_service_use() plan = plpy.prepare("SELECT ST_SetSRID(ST_MakePoint($1, $2), 4326); ", ["double precision", "double precision"]) point = plpy.execute(plan, [coordinates[0], coordinates[1]], 1)[0] return point['st_setsrid'] else: - quota_service.increment_empty_service_use() + service_manager.quota_service.increment_empty_service_use() return None except BaseException as e: import sys - quota_service.increment_failed_service_use() - logger.error('Error trying to geocode street point using google maps', sys.exc_info(), data={"username": username, "orgname": orgname}) + service_manager.quota_service.increment_failed_service_use() + service_manager.logger.error('Error trying to geocode street point using google maps', sys.exc_info(), data={"username": username, "orgname": orgname}) raise Exception('Error trying to geocode street point using google maps') finally: quota_service.increment_total_service_use() diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py index 03c8fb4..4ac76cb 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py @@ -1,10 +1,10 @@ from exceptions import RateLimitExceeded class ServiceManagerBase: - def check(self): - if not self.rate_limiter.check(): + def check(self, quota=True, rate=True): + if rate and not self.rate_limiter.check(): raise RateLimitExceeded() - if not self.quota_service.check_user_quota(): + if quota and not self.quota_service.check_user_quota(): raise Exception('You have reached the limit of your quota') @property From 9a04ad06a0ef38bf1e5ed9964e4f94ac1c4c9def Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 15 Mar 2017 19:32:50 +0100 Subject: [PATCH 11/31] Move rate limits out of Config The (legacy) Config object rate limit-related modifications are reverted. For the legacy case, configuration is handled in a specific RateLimits builder class. --- .../cartodb_services/metrics/config.py | 38 ++-------------- .../refactor/config/rate_limits.py | 45 +++++++++++++++++++ .../cartodb_services/tools/service_manager.py | 40 ++++++++--------- 3 files changed, 69 insertions(+), 54 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py index 7b44d97..f3e93d0 100644 --- a/server/lib/python/cartodb_services/cartodb_services/metrics/config.py +++ b/server/lib/python/cartodb_services/cartodb_services/metrics/config.py @@ -56,13 +56,6 @@ class ServiceConfig(object): else: return None - def _get_rate_limit(self, service): - rate_limit_key = "{0}_rate_limit".format(service) - rate_limit_json = self._redis_config.get(rate_limit_key, None) - if (rate_limit_json): - return rate_limit_json and json.loads(rate_limit_json) - else: - return self._db_config.rate_limits.get(service, {}) class DataObservatoryConfig(ServiceConfig): @@ -405,8 +398,6 @@ class GeocoderConfig(ServiceConfig): self._cost_per_hit = 0 self._mapzen_service_params = db_config.mapzen_geocoder_service_params - self._rate_limit = self._get_rate_limit('geocoder') - @property def service_type(self): if self._geocoder_provider == self.GOOGLE_GEOCODER: @@ -483,10 +474,6 @@ class GeocoderConfig(ServiceConfig): def provider(self): return self._geocoder_provider - @property - def rate_limit(self): - return self._rate_limit - @property def service(self): return self._service @@ -504,7 +491,6 @@ class ServicesDBConfig: self._get_here_config() self._get_mapzen_config() self._get_data_observatory_config() - self._get_rate_limits_config() def _get_server_config(self): server_config_json = self._get_conf('server_conf') @@ -562,22 +548,14 @@ class ServicesDBConfig: else: self._data_observatory_connection_str = do_conf['connection']['production'] - def _get_rate_limits_config(self): - # self._rate_limits = json.loads(self._get_conf('rate_limits', default='{}')) - rl = self._get_conf('rate_limits', default='{}') - self._rate_limits = json.loads(rl) - def _get_conf(self, key, default=KeyError): + def _get_conf(self, key): try: sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(key) - conf = self._db_conn.execute(sql, 1)[0]['conf'] - if default != KeyError: - conf = conf or default - return conf + conf = self._db_conn.execute(sql, 1) + return conf[0]['conf'] except Exception as e: - if (default == KeyError): - raise ConfigException("Error trying to get config for {0}: {1}".format(key, e)) - return default + raise ConfigException("Error trying to get config for {0}: {1}".format(key, e)) @property def server_environment(self): @@ -655,10 +633,6 @@ class ServicesDBConfig: def data_observatory_connection_str(self): return self._data_observatory_connection_str - @property - def rate_limits(self): - return self._rate_limits - @property def logger_config(self): logger_conf_json = self._get_conf('logger_conf') @@ -681,7 +655,6 @@ class ServicesRedisConfig: GEOCODER_PROVIDER_KEY = 'geocoder_provider' ISOLINES_PROVIDER_KEY = 'isolines_provider' ROUTING_PROVIDER_KEY = 'routing_provider' - GEOCODING_RATE_LIMIT_KEY = 'geocoder_rate_limit' def __init__(self, redis_conn): self._redis_connection = redis_conn @@ -736,6 +709,3 @@ class ServicesRedisConfig: user_config[self.ISOLINES_PROVIDER_KEY] = org_config[self.ISOLINES_PROVIDER_KEY] if self.ROUTING_PROVIDER_KEY in org_config: user_config[self.ROUTING_PROVIDER_KEY] = org_config[self.ROUTING_PROVIDER_KEY] - # for rate limit parameters, user config has precedence over organization - if self.GEOCODING_RATE_LIMIT_KEY in org_config and not self.GEOCODING_RATE_LIMIT_KEY in user_config: - user_config[self.GEOCODING_RATE_LIMIT_KEY] = org_config[self.GEOCODING_RATE_LIMIT_KEY] diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py index 70bde8b..6a465f5 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py @@ -63,3 +63,48 @@ class RateLimitsConfigBuilder(object): rate_limit.get('limit', None), rate_limit.get('period', None)) +class RateLimitsConfigLegacyBuilder(object): + """ + Build a RateLimitsConfig object using the legacy configuration + classes ... + instead of the refactored ... + """ + + def __init__(self, redis_connection, db_conn, service, user, org): + self._service = service + self._username = user + self._orgname = org + self._redis_connection = redis_connection + self._db_conn = db_conn + + def get(self): + rate_limit = self.__get_rate_limit() + return RateLimitsConfig(self._service, + self._username, + rate_limit.get('limit', None), + rate_limit.get('period', None)) + + def __get_rate_limit(self): + rate_limit = {} + rate_limit_key = "{0}_rate_limit".format(self._service) + user_key = "rails:users:{0}".format(self._username) + rate_limit_json = self.__get_redis_config(user_key, rate_limit_key) + if not rate_limit_json and self._orgname: + org_key = "rails:orgs:{0}".format(self._orgname) + rate_limit_json = self.__get_redis_config(org_key, rate_limit_key) + if rate_limit_json: + rate_limit = json.loads(rate_limit_json) + else: + conf_key = 'rate_limits' + sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(conf_key) + try: + conf = self._db_conn.execute(sql, 1)[0]['conf'] + except Exception: + conf = None + if conf: + rate_limit = json.loads(conf).get(self._service) + return rate_limit or {} + + def __get_redis_config(self, basekey, param): + config = self._redis_connection.hgetall(basekey) + return config and config.get(param) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py index 4ac76cb..5f140a5 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py @@ -19,26 +19,6 @@ class ServiceManagerBase: def logger(self): return self.logger -from cartodb_services.metrics import QuotaService -from cartodb_services.tools import Logger,LoggerConfig -from cartodb_services.tools import RateLimiter -from cartodb_services.refactor.config.rate_limits import RateLimitsConfig - -class LegacyServiceManager(ServiceManagerBase): - - def __init__(self, service, username, orgname, gd): - redis_conn = gd["redis_connection_{0}".format(username)]['redis_metrics_connection'] - self.config = gd["user_{0}_config_{1}".format(service, username)] - logger_config = gd["logger_config"] - self.logger = Logger(logger_config) - - rate_limits_config = RateLimitsConfig(service, - username, - self.config.rate_limit.get('limit'), - self.config.rate_limit.get('period')) - self.rate_limiter = RateLimiter(rate_limits_config, redis_conn) - self.quota_service = QuotaService(self.config, redis_conn) - from cartodb_services.metrics import QuotaService from cartodb_services.tools import Logger from cartodb_services.tools import RateLimiter @@ -68,3 +48,23 @@ class ServiceManager(ServiceManagerBase): self.rate_limiter = RateLimiter(rate_limit_config, redis_metrics_connection) self.quota_service = QuotaService(self.config, redis_metrics_connection) + +from cartodb_services.metrics import QuotaService +from cartodb_services.tools import Logger,LoggerConfig +from cartodb_services.tools import RateLimiter +from cartodb_services.refactor.config.rate_limits import RateLimitsConfigLegacyBuilder +import plpy + +class LegacyServiceManager(ServiceManagerBase): + + def __init__(self, service, username, orgname, gd): + redis_conn = gd["redis_connection_{0}".format(username)]['redis_metrics_connection'] + self.config = gd["user_{0}_config_{1}".format(service, username)] + logger_config = gd["logger_config"] + self.logger = Logger(logger_config) + + self.quota_service = QuotaService(self.config, redis_conn) + + rate_limit_config = RateLimitsConfigLegacyBuilder(redis_conn, plpy, service=service, user=username, org=orgname).get() + self.rate_limiter = RateLimiter(rate_limit_config, redis_conn) + From 87e37e1a26b7331eecd913aa8df0befbc47863d8 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 16 Mar 2017 17:55:27 +0100 Subject: [PATCH 12/31] Fix typo --- server/extension/sql/20_geocode_street.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index 30675c4..5fec700 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -145,7 +145,7 @@ RETURNS Geometry AS $$ geocoder = MapzenGeocoder(service_manager.config.mapzen_api_key, service_manager.logger, service_manager.config.service_params) country_iso3 = None if country: - untry_iso3 = country_to_iso3(country) + country_iso3 = country_to_iso3(country) coordinates = geocoder.geocode(searchtext=searchtext, city=city, state_province=state_province, country=country_iso3, search_type='address') From 7f6c19b2929b9ce0556175a6b5f1efb7912750f6 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 16 Mar 2017 17:59:22 +0100 Subject: [PATCH 13/31] Minor changes for clarity * ServiceManager check method renamed as assert_within_limits * Legacy classes moved to separate files --- server/extension/sql/20_geocode_street.sql | 6 +- .../refactor/config/__init__.py | 2 + .../refactor/config/legacy_rate_limits.py | 46 ++++++++++++ .../refactor/config/rate_limits.py | 51 +------------ .../cartodb_services/tools/__init__.py | 4 +- .../cartodb_services/tools/exceptions.py | 3 - .../tools/legacy_service_manager.py | 23 ++++++ .../cartodb_services/tools/service_manager.py | 74 ++++++++++--------- .../test/test_servicemanager.py | 10 +-- 9 files changed, 126 insertions(+), 93 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py create mode 100644 server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index 5fec700..b3daec9 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -77,7 +77,7 @@ RETURNS Geometry AS $$ plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") service_manager = LegacyServiceManager('geocoder', username, orgname, GD) - service_manager.check() + service_manager.assert_within_limits() try: geocoder = HereMapsGeocoder(service_manager.config.heremaps_app_id, service_manager.config.heremaps_app_code, service_manager.logger, service_manager.config.heremaps_service_params) @@ -106,7 +106,7 @@ RETURNS Geometry AS $$ plpy.execute("SELECT cdb_dataservices_server._get_logger_config()") service_manager = LegacyServiceManager('geocoder', username, orgname, GD) - service_manager.check(quota=False) + service_manager.assert_within_limits(quota=False) try: geocoder = GoogleMapsGeocoder(service_manager.config.google_client_id, service_manager.config.google_api_key, service_manager.logger) @@ -139,7 +139,7 @@ RETURNS Geometry AS $$ cartodb_services.init(plpy, GD) service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, username, orgname) - service_manager.check() + service_manager.assert_within_limits() try: geocoder = MapzenGeocoder(service_manager.config.mapzen_api_key, service_manager.logger, service_manager.config.service_params) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py index e69de29..b9559c6 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py @@ -0,0 +1,2 @@ +from rate_limits import RateLimitsConfig, RateLimitsConfigBuilder +from legacy_rate_limits import RateLimitsConfigLegacyBuilder diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py new file mode 100644 index 0000000..d4208ec --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py @@ -0,0 +1,46 @@ +import json +from rate_limits import RateLimitsConfig + +class RateLimitsConfigLegacyBuilder(object): + """ + Build a RateLimitsConfig object using the *legacy* configuration classes + """ + + def __init__(self, redis_connection, db_conn, service, user, org): + self._service = service + self._username = user + self._orgname = org + self._redis_connection = redis_connection + self._db_conn = db_conn + + def get(self): + rate_limit = self.__get_rate_limit() + return RateLimitsConfig(self._service, + self._username, + rate_limit.get('limit', None), + rate_limit.get('period', None)) + + def __get_rate_limit(self): + rate_limit = {} + rate_limit_key = "{0}_rate_limit".format(self._service) + user_key = "rails:users:{0}".format(self._username) + rate_limit_json = self.__get_redis_config(user_key, rate_limit_key) + if not rate_limit_json and self._orgname: + org_key = "rails:orgs:{0}".format(self._orgname) + rate_limit_json = self.__get_redis_config(org_key, rate_limit_key) + if rate_limit_json: + rate_limit = json.loads(rate_limit_json) + else: + conf_key = 'rate_limits' + sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(conf_key) + try: + conf = self._db_conn.execute(sql, 1)[0]['conf'] + except Exception: + conf = None + if conf: + rate_limit = json.loads(conf).get(self._service) + return rate_limit or {} + + def __get_redis_config(self, basekey, param): + config = self._redis_connection.hgetall(basekey) + return config and config.get(param) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py index 6a465f5..6f6d0df 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py @@ -37,8 +37,11 @@ class RateLimitsConfig(object): def is_limited(self): return self._limit and self._limit > 0 and self._period and self._period > 0 - class RateLimitsConfigBuilder(object): + """ + Build a rate limits configuration obtaining the parameters + from the user/org/server configuration. + """ def __init__(self, server_conf, user_conf, org_conf, service, user, org): self._server_conf = server_conf @@ -62,49 +65,3 @@ class RateLimitsConfigBuilder(object): self._username, rate_limit.get('limit', None), rate_limit.get('period', None)) - -class RateLimitsConfigLegacyBuilder(object): - """ - Build a RateLimitsConfig object using the legacy configuration - classes ... - instead of the refactored ... - """ - - def __init__(self, redis_connection, db_conn, service, user, org): - self._service = service - self._username = user - self._orgname = org - self._redis_connection = redis_connection - self._db_conn = db_conn - - def get(self): - rate_limit = self.__get_rate_limit() - return RateLimitsConfig(self._service, - self._username, - rate_limit.get('limit', None), - rate_limit.get('period', None)) - - def __get_rate_limit(self): - rate_limit = {} - rate_limit_key = "{0}_rate_limit".format(self._service) - user_key = "rails:users:{0}".format(self._username) - rate_limit_json = self.__get_redis_config(user_key, rate_limit_key) - if not rate_limit_json and self._orgname: - org_key = "rails:orgs:{0}".format(self._orgname) - rate_limit_json = self.__get_redis_config(org_key, rate_limit_key) - if rate_limit_json: - rate_limit = json.loads(rate_limit_json) - else: - conf_key = 'rate_limits' - sql = "SELECT cartodb.CDB_Conf_GetConf('{0}') as conf".format(conf_key) - try: - conf = self._db_conn.execute(sql, 1)[0]['conf'] - except Exception: - conf = None - if conf: - rate_limit = json.loads(conf).get(self._service) - return rate_limit or {} - - def __get_redis_config(self, basekey, param): - config = self._redis_connection.hgetall(basekey) - return config and config.get(param) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py b/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py index 0658841..16072f8 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/__init__.py @@ -3,5 +3,5 @@ from coordinates import Coordinate from polyline import PolyLine from log import Logger, LoggerConfig from rate_limiter import RateLimiter -from exceptions import RateLimitExceeded -from service_manager import ServiceManager, LegacyServiceManager +from service_manager import ServiceManager, RateLimitExceeded +from legacy_service_manager import LegacyServiceManager diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py b/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py index f8b79c1..e69de29 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/exceptions.py @@ -1,3 +0,0 @@ -class RateLimitExceeded(Exception): - def __str__(self): - return repr('Rate limit exceeded') diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py new file mode 100644 index 0000000..fca642f --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py @@ -0,0 +1,23 @@ +from cartodb_services.metrics import QuotaService +from cartodb_services.tools import Logger,LoggerConfig +from cartodb_services.tools import RateLimiter +from cartodb_services.refactor.config import RateLimitsConfigLegacyBuilder +from cartodb_services.tools.service_manager import ServiceManagerBase +import plpy + +class LegacyServiceManager(ServiceManagerBase): + """ + This service manager relies on cached configuration (in gd) stored in *legacy* configuration objects + It's intended for use by the *legacy* configuration objects (in use prior to the configuration refactor). + """ + + def __init__(self, service, username, orgname, gd): + redis_conn = gd["redis_connection_{0}".format(username)]['redis_metrics_connection'] + self.config = gd["user_{0}_config_{1}".format(service, username)] + logger_config = gd["logger_config"] + self.logger = Logger(logger_config) + + self.quota_service = QuotaService(self.config, redis_conn) + + rate_limit_config = RateLimitsConfigLegacyBuilder(redis_conn, plpy, service=service, user=username, org=orgname).get() + self.rate_limiter = RateLimiter(rate_limit_config, redis_conn) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py index 5f140a5..06005d7 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py @@ -1,7 +1,41 @@ -from exceptions import RateLimitExceeded +from cartodb_services.metrics import QuotaService +from cartodb_services.tools import Logger +from cartodb_services.tools import RateLimiter +from cartodb_services.refactor.tools.logger import LoggerConfigBuilder +from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder +from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory +from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory +from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory +from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory +from cartodb_services.refactor.config import RateLimitsConfigBuilder + +class RateLimitExceeded(Exception): + def __str__(self): + return repr('Rate limit exceeded') class ServiceManagerBase: - def check(self, quota=True, rate=True): + """ + A Service manager collects the configuration needed to use a service, + including thir-party services parameters. + + This abstract class serves as the base for concrete service manager classes; + derived class must provide and initialize attributes for ``config``, + ``quota_service``, ``logger`` and ``rate_limiter`` (which can be None + for no limits). + + It provides an `assert_within_limits` method to check quota and rate limits + which raises exceptions when limits are exceeded. + + It exposes properties containing: + + * ``config`` : a configuration object containing the configuration parameters for + a given service and provider. + * ``quota_service`` a QuotaService object to for quota accounting + * ``logger`` + + """ + + def assert_within_limits(self, quota=True, rate=True): if rate and not self.rate_limiter.check(): raise RateLimitExceeded() if quota and not self.quota_service.check_user_quota(): @@ -19,18 +53,12 @@ class ServiceManagerBase: def logger(self): return self.logger -from cartodb_services.metrics import QuotaService -from cartodb_services.tools import Logger -from cartodb_services.tools import RateLimiter -from cartodb_services.refactor.tools.logger import LoggerConfigBuilder -from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder -from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory -from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory -from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory -from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory -from cartodb_services.refactor.config.rate_limits import RateLimitsConfigBuilder - class ServiceManager(ServiceManagerBase): + """ + This service manager delegates the configuration parameter details, + and the policies about configuration precedence to a configuration-builder class. + It uses the refactored configuration classes. + """ def __init__(self, service, config_builder, username, orgname): server_config_backend = ServerConfigBackendFactory().get() @@ -48,23 +76,3 @@ class ServiceManager(ServiceManagerBase): self.rate_limiter = RateLimiter(rate_limit_config, redis_metrics_connection) self.quota_service = QuotaService(self.config, redis_metrics_connection) - -from cartodb_services.metrics import QuotaService -from cartodb_services.tools import Logger,LoggerConfig -from cartodb_services.tools import RateLimiter -from cartodb_services.refactor.config.rate_limits import RateLimitsConfigLegacyBuilder -import plpy - -class LegacyServiceManager(ServiceManagerBase): - - def __init__(self, service, username, orgname, gd): - redis_conn = gd["redis_connection_{0}".format(username)]['redis_metrics_connection'] - self.config = gd["user_{0}_config_{1}".format(service, username)] - logger_config = gd["logger_config"] - self.logger = Logger(logger_config) - - self.quota_service = QuotaService(self.config, redis_conn) - - rate_limit_config = RateLimitsConfigLegacyBuilder(redis_conn, plpy, service=service, user=username, org=orgname).get() - self.rate_limiter = RateLimiter(rate_limit_config, redis_conn) - diff --git a/server/lib/python/cartodb_services/test/test_servicemanager.py b/server/lib/python/cartodb_services/test/test_servicemanager.py index b7874ab..2131815 100644 --- a/server/lib/python/cartodb_services/test/test_servicemanager.py +++ b/server/lib/python/cartodb_services/test/test_servicemanager.py @@ -36,12 +36,12 @@ class TestServiceManager(TestCase): def check_rate_limit(self, service_manager, n, active=True): if LUA_AVAILABLE_FOR_MOCKREDIS: for _ in xrange(n): - service_manager.check() + service_manager.assert_within_limits() if active: with assert_raises(RateLimitExceeded): - service_manager.check() + service_manager.assert_within_limits() else: - service_manager.check() + service_manager.assert_within_limits() else: # rratelimit doesn't work with MockRedis because it needs Lua support # so, we'll simply perform some sanity check on the configuration of the rate limiter @@ -59,14 +59,14 @@ class TestServiceManager(TestCase): 'logger_config' : Mock(min_log_level='debug', log_file_path=None, rollbar_api_key=None, environment=self.environment) } service_manager = LegacyServiceManager('geocoder', self.username, self.orgname, config_cache) - service_manager.check() + service_manager.assert_within_limits() assert_equal(service_manager.config.service_type, 'geocoder_mapzen') def test_service_manager(self): with patch.object(RedisConnectionBuilder,'get') as get_fn: get_fn.return_value = self.redis_conn service_manager = ServiceManager('geocoder', MapzenGeocoderConfigBuilder, self.username, self.orgname) - service_manager.check() + service_manager.assert_within_limits() assert_equal(service_manager.config.service_type, 'geocoder_mapzen') def test_no_rate_limit_by_default(self): From 945c6cd685cba64658d065991406619b0e98fc20 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 16 Mar 2017 19:12:39 +0100 Subject: [PATCH 14/31] WIP: support for storing rate limits configuration --- server/extension/sql/15_config_helper.sql | 18 +++++++++ .../refactor/config/rate_limits.py | 37 +++++++++++++++++++ .../refactor/storage/redis_config.py | 6 +++ .../refactor/storage/server_config.py | 10 +++++ .../cartodb_services/tools/service_manager.py | 37 +++++++++++++++---- 5 files changed, 100 insertions(+), 8 deletions(-) diff --git a/server/extension/sql/15_config_helper.sql b/server/extension/sql/15_config_helper.sql index 9d83ebd..95eccf8 100644 --- a/server/extension/sql/15_config_helper.sql +++ b/server/extension/sql/15_config_helper.sql @@ -16,6 +16,24 @@ RETURNS JSON AS $$ SELECT VALUE FROM cartodb.cdb_conf WHERE key = input_key; $$ LANGUAGE SQL STABLE SECURITY DEFINER; +CREATE OR REPLACE +FUNCTION cartodb.CDB_Conf_SetConf(key text, value JSON) + RETURNS void AS $$ +BEGIN + PERFORM cartodb.CDB_Conf_RemoveConf(key); + EXECUTE 'INSERT INTO cartodb.CDB_CONF (KEY, VALUE) VALUES ($1, $2);' USING key, value; +END +$$ LANGUAGE PLPGSQL VOLATILE; + +CREATE OR REPLACE +FUNCTION cartodb.CDB_Conf_RemoveConf(key text) + RETURNS void AS $$ +BEGIN + EXECUTE 'DELETE FROM cartodb.CDB_CONF WHERE KEY = $1;' USING key; +END +$$ LANGUAGE PLPGSQL VOLATILE; + + CREATE OR REPLACE FUNCTION cdb_dataservices_server._get_geocoder_config(username text, orgname text, provider text DEFAULT NULL) RETURNS boolean AS $$ cache_key = "user_geocoder_config_{0}".format(username) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py index 6f6d0df..4079d0d 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py @@ -65,3 +65,40 @@ class RateLimitsConfigBuilder(object): self._username, rate_limit.get('limit', None), rate_limit.get('period', None)) + + +class RateLimitsConfigSetter(object): + + def __init__(self, service, username, orgname=None): + self._service = service + self._service_config = ServerConfiguration(service, username, orgname) + + def set_user_rate_limits(self, rate_limits_config): + # Note we allow copying a config from another user/service, so we + # ignore rate_limits:config.service and rate_limits:config.username + rate_limit_key = "{0}_rate_limit".format(service) + if rate_limits_config.is_limited(): + rate_limit = {'limit': rate_limits_config.limit, 'period': rate_limits_config.period} + self.service_config.user.set(rate_limit_key, rate_limit) + else + self.service_config.user.remove(rate_limit_key) + + def set_org_rate_limits(self, rate_limits_config): + rate_limit_key = "{0}_rate_limit".format(service) + if rate_limits_config.is_limited(): + rate_limit = {'limit': rate_limits_config.limit, 'period': rate_limits_config.period} + self.service_config.org.set(rate_limit_key, rate_limit) + else + self.service_config.org.remove(rate_limit_key) + + def set_server_rate_limits(self, rate_limits_config): + rate_limits = self.service_config.server.get('rate_limits', {}) + if rate_limits_config.is_limited(): + rate_limits[self._service] = {'limit': rate_limits_config.limit, 'period': rate_limits_config.period} + else + rate_limits.pop(self._service, None) + if rate_limits: + self.service_config.server.set('rate_limits', rate_limits) + else + self.service_config.server.remove('rate_limits') + diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py index 1e1f33c..cc35c24 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py @@ -17,6 +17,12 @@ class RedisConfigStorage(ConfigBackendInterface): else: return self._data.get(key, default) + def set(self, key, value): + self._connection.hset(self._config_key, key, value) + + def remove(self, key): + self._connection.hdel(self._config_key, key) + class RedisUserConfigStorageBuilder(object): def __init__(self, redis_connection, username): self._redis_connection = redis_connection diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py index 1d3603c..7ee3fe5 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py @@ -19,3 +19,13 @@ class InDbServerConfigStorage(ConfigBackendInterface): raise KeyError else: return default + + def set(self, key, config): + json_config = json.dumps(config) + quoted_config = cartodb_services.plpy.quote_nullable(json_config) + sql = "SELECT cdb_dataservices_server.cdb_conf_setconf('{0}', {0})".format(key, quoted_config) + cartodb_services.plpy.execute(sql) + + def remove(self, key): + sql = "SELECT cdb_dataservices_server.cdb_conf_removeconf('{0}')".format(key) + cartodb_services.plpy.execute(sql) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py index 06005d7..a6d40ac 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py @@ -53,6 +53,30 @@ class ServiceManagerBase: def logger(self): return self.logger + +class ServiceConfiguration: + def __init__(self, service, username, orgname): + self._server_config_backend = ServerConfigBackendFactory().get() + self._environment = ServerEnvironmentBuilder(server_config_backend).get() + self._user_config_backend = UserConfigBackendFactory(username, environment, server_config_backend).get() + self._org_config_backend = OrgConfigBackendFactory(orgname, environment, server_config_backend).get() + + @property + def environment(self): + return self._environment + + @property + def server(self): + return self._server_config_backend + + @property + def user(self): + return self._user_config_backend + + @property + def org(self): + return self._org_config_backend + class ServiceManager(ServiceManagerBase): """ This service manager delegates the configuration parameter details, @@ -61,18 +85,15 @@ class ServiceManager(ServiceManagerBase): """ def __init__(self, service, config_builder, username, orgname): - server_config_backend = ServerConfigBackendFactory().get() - environment = ServerEnvironmentBuilder(server_config_backend).get() - user_config_backend = UserConfigBackendFactory(username, environment, server_config_backend).get() - org_config_backend = OrgConfigBackendFactory(orgname, environment, server_config_backend).get() + service_config = ServerConfiguration(service, username, orgname) - logger_config = LoggerConfigBuilder(environment, server_config_backend).get() + logger_config = LoggerConfigBuilder(service_config.environment, service_config.server).get() self.logger = Logger(logger_config) - self.config = config_builder(server_config_backend, user_config_backend, org_config_backend, username, orgname).get() - rate_limit_config = RateLimitsConfigBuilder(server_config_backend, user_config_backend, org_config_backend, service=service, user=username, org=orgname).get() + self.config = config_builder(service_config.server, service_config.user, service_config.org, username, orgname).get() + rate_limit_config = RateLimitsConfigBuilder(service_config.server, service_config.user, service_config.org, service=service, user=username, org=orgname).get() - redis_metrics_connection = RedisMetricsConnectionFactory(environment, server_config_backend).get() + redis_metrics_connection = RedisMetricsConnectionFactory(service_config.environment, service_config.server).get() self.rate_limiter = RateLimiter(rate_limit_config, redis_metrics_connection) self.quota_service = QuotaService(self.config, redis_metrics_connection) From 73f97128ed0ed49461d76ec0f93920e2b1f93d3d Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 21 Mar 2017 15:42:53 +0100 Subject: [PATCH 15/31] Reorganization of configuration modules, fix circular dependencies The new module cartodb_services/config is intended for services configuration objects Some legacy configuration objects remain under cartodb_services/metrics. The refactored configuration backends are also not moved here --- .../cartodb_services/config/__init__.py | 3 ++ .../config/legacy_rate_limits.py | 0 .../{refactor => }/config/rate_limits.py | 38 ++++++++++++------- .../config/service_configuration.py | 36 ++++++++++++++++++ .../refactor/config/__init__.py | 2 - .../tools/legacy_service_manager.py | 2 +- .../cartodb_services/tools/service_manager.py | 33 +--------------- 7 files changed, 66 insertions(+), 48 deletions(-) create mode 100644 server/lib/python/cartodb_services/cartodb_services/config/__init__.py rename server/lib/python/cartodb_services/cartodb_services/{refactor => }/config/legacy_rate_limits.py (100%) rename server/lib/python/cartodb_services/cartodb_services/{refactor => }/config/rate_limits.py (74%) create mode 100644 server/lib/python/cartodb_services/cartodb_services/config/service_configuration.py diff --git a/server/lib/python/cartodb_services/cartodb_services/config/__init__.py b/server/lib/python/cartodb_services/cartodb_services/config/__init__.py new file mode 100644 index 0000000..6a98689 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/config/__init__.py @@ -0,0 +1,3 @@ +from service_configuration import ServiceConfiguration +from rate_limits import RateLimitsConfig, RateLimitsConfigBuilder, RateLimitsConfigSetter +from legacy_rate_limits import RateLimitsConfigLegacyBuilder diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/config/legacy_rate_limits.py similarity index 100% rename from server/lib/python/cartodb_services/cartodb_services/refactor/config/legacy_rate_limits.py rename to server/lib/python/cartodb_services/cartodb_services/config/legacy_rate_limits.py diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/config/rate_limits.py similarity index 74% rename from server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py rename to server/lib/python/cartodb_services/cartodb_services/config/rate_limits.py index 4079d0d..814a1f3 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/config/rate_limits.py @@ -1,4 +1,7 @@ import json + +from service_configuration import ServiceConfiguration + class RateLimitsConfig(object): """ Value object that represents the configuration needed to rate-limit services @@ -14,6 +17,9 @@ class RateLimitsConfig(object): self._limit = limit and int(limit) self._period = period and int(period) + def __eq__(self, other): + return self.__dict__ == other.__dict__ + # service this limit applies to @property def service(self): @@ -43,6 +49,7 @@ class RateLimitsConfigBuilder(object): from the user/org/server configuration. """ + # TODO: user->username, org->orgname def __init__(self, server_conf, user_conf, org_conf, service, user, org): self._server_conf = server_conf self._user_conf = user_conf @@ -55,6 +62,7 @@ class RateLimitsConfigBuilder(object): # Order of precedence is user_conf, org_conf, server_conf rate_limit_key = "{0}_rate_limit".format(self._service) + rate_limit_json = self._user_conf.get(rate_limit_key, None) or self._org_conf.get(rate_limit_key, None) if (rate_limit_json): rate_limit = rate_limit_json and json.loads(rate_limit_json) @@ -71,34 +79,36 @@ class RateLimitsConfigSetter(object): def __init__(self, service, username, orgname=None): self._service = service - self._service_config = ServerConfiguration(service, username, orgname) + self._service_config = ServiceConfiguration(service, username, orgname) def set_user_rate_limits(self, rate_limits_config): # Note we allow copying a config from another user/service, so we # ignore rate_limits:config.service and rate_limits:config.username - rate_limit_key = "{0}_rate_limit".format(service) + rate_limit_key = "{0}_rate_limit".format(self._service) if rate_limits_config.is_limited(): rate_limit = {'limit': rate_limits_config.limit, 'period': rate_limits_config.period} - self.service_config.user.set(rate_limit_key, rate_limit) - else - self.service_config.user.remove(rate_limit_key) + rate_limit_json = json.dumps(rate_limit) + self._service_config.user.set(rate_limit_key, rate_limit_json) + else: + self._service_config.user.remove(rate_limit_key) def set_org_rate_limits(self, rate_limits_config): - rate_limit_key = "{0}_rate_limit".format(service) + rate_limit_key = "{0}_rate_limit".format(self._service) if rate_limits_config.is_limited(): rate_limit = {'limit': rate_limits_config.limit, 'period': rate_limits_config.period} - self.service_config.org.set(rate_limit_key, rate_limit) - else - self.service_config.org.remove(rate_limit_key) + rate_limit_json = json.dumps(rate_limit) + self._service_config.org.set(rate_limit_key, rate_limit_json) + else: + self._service_config.org.remove(rate_limit_key) def set_server_rate_limits(self, rate_limits_config): - rate_limits = self.service_config.server.get('rate_limits', {}) + rate_limits = self._service_config.server.get('rate_limits', {}) if rate_limits_config.is_limited(): rate_limits[self._service] = {'limit': rate_limits_config.limit, 'period': rate_limits_config.period} - else + else: rate_limits.pop(self._service, None) if rate_limits: - self.service_config.server.set('rate_limits', rate_limits) - else - self.service_config.server.remove('rate_limits') + self._service_config.server.set('rate_limits', rate_limits) + else: + self._service_config.server.remove('rate_limits') diff --git a/server/lib/python/cartodb_services/cartodb_services/config/service_configuration.py b/server/lib/python/cartodb_services/cartodb_services/config/service_configuration.py new file mode 100644 index 0000000..468a915 --- /dev/null +++ b/server/lib/python/cartodb_services/cartodb_services/config/service_configuration.py @@ -0,0 +1,36 @@ +from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder +from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory +from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory +from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory + +class ServiceConfiguration(object): + """ + This class instantiates configuration backend objects for all the configuration levels of a service: + * environment + * server + * organization + * user + The configuration backends allow retrieval and modification of configuration parameters. + """ + + def __init__(self, service, username, orgname): + self._server_config_backend = ServerConfigBackendFactory().get() + self._environment = ServerEnvironmentBuilder(self._server_config_backend ).get() + self._user_config_backend = UserConfigBackendFactory(username, self._environment, self._server_config_backend ).get() + self._org_config_backend = OrgConfigBackendFactory(orgname, self._environment, self._server_config_backend ).get() + + @property + def environment(self): + return self._environment + + @property + def server(self): + return self._server_config_backend + + @property + def user(self): + return self._user_config_backend + + @property + def org(self): + return self._org_config_backend \ No newline at end of file diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py b/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py index b9559c6..e69de29 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/config/__init__.py @@ -1,2 +0,0 @@ -from rate_limits import RateLimitsConfig, RateLimitsConfigBuilder -from legacy_rate_limits import RateLimitsConfigLegacyBuilder diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py index fca642f..63bb90f 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py @@ -1,7 +1,7 @@ from cartodb_services.metrics import QuotaService from cartodb_services.tools import Logger,LoggerConfig from cartodb_services.tools import RateLimiter -from cartodb_services.refactor.config import RateLimitsConfigLegacyBuilder +from cartodb_services.config import RateLimitsConfigLegacyBuilder from cartodb_services.tools.service_manager import ServiceManagerBase import plpy diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py index a6d40ac..22d1677 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py @@ -2,12 +2,8 @@ from cartodb_services.metrics import QuotaService from cartodb_services.tools import Logger from cartodb_services.tools import RateLimiter from cartodb_services.refactor.tools.logger import LoggerConfigBuilder -from cartodb_services.refactor.core.environment import ServerEnvironmentBuilder -from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory -from cartodb_services.refactor.backend.user_config import UserConfigBackendFactory -from cartodb_services.refactor.backend.org_config import OrgConfigBackendFactory from cartodb_services.refactor.backend.redis_metrics_connection import RedisMetricsConnectionFactory -from cartodb_services.refactor.config import RateLimitsConfigBuilder +from cartodb_services.config import ServiceConfiguration, RateLimitsConfigBuilder class RateLimitExceeded(Exception): def __str__(self): @@ -52,31 +48,6 @@ class ServiceManagerBase: @property def logger(self): return self.logger - - -class ServiceConfiguration: - def __init__(self, service, username, orgname): - self._server_config_backend = ServerConfigBackendFactory().get() - self._environment = ServerEnvironmentBuilder(server_config_backend).get() - self._user_config_backend = UserConfigBackendFactory(username, environment, server_config_backend).get() - self._org_config_backend = OrgConfigBackendFactory(orgname, environment, server_config_backend).get() - - @property - def environment(self): - return self._environment - - @property - def server(self): - return self._server_config_backend - - @property - def user(self): - return self._user_config_backend - - @property - def org(self): - return self._org_config_backend - class ServiceManager(ServiceManagerBase): """ This service manager delegates the configuration parameter details, @@ -85,7 +56,7 @@ class ServiceManager(ServiceManagerBase): """ def __init__(self, service, config_builder, username, orgname): - service_config = ServerConfiguration(service, username, orgname) + service_config = ServiceConfiguration(service, username, orgname) logger_config = LoggerConfigBuilder(service_config.environment, service_config.server).get() self.logger = Logger(logger_config) From 208469f53472b4c579e5814d07f4727349007044 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 21 Mar 2017 15:43:33 +0100 Subject: [PATCH 16/31] Add rate limits configuration writing tests --- .../python/cartodb_services/test/mock_plpy.py | 29 +++- .../test/test_ratelimitsconfig.py | 124 ++++++++++++++++++ 2 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 server/lib/python/cartodb_services/test/test_ratelimitsconfig.py diff --git a/server/lib/python/cartodb_services/test/mock_plpy.py b/server/lib/python/cartodb_services/test/mock_plpy.py index 5298a98..0fe067f 100644 --- a/server/lib/python/cartodb_services/test/mock_plpy.py +++ b/server/lib/python/cartodb_services/test/mock_plpy.py @@ -16,7 +16,7 @@ class MockPlPy: def __init__(self): self._reset() - def _reset(self): + def _reset(self, log_executed_queries=False): self.infos = [] self.notices = [] self.debugs = [] @@ -28,11 +28,30 @@ class MockPlPy: self.results = [] self.prepares = [] self.results = {} + self._log_executed_queries = log_executed_queries + self._logged_queries = [] def _define_result(self, query, result): pattern = re.compile(query, re.IGNORECASE | re.MULTILINE) self.results[pattern] = result + def _executed_queries(self): + if self._log_executed_queries: + return self._logged_queries + else: + raise Exception('Executed queries logging is not active') + + def _has_executed_query(self, query): + pattern = re.compile(re.escape(query)) + for executed_query in self._executed_queries(): + if pattern.search(executed_query): + return True + return False + + def _start_logging_executed_queries(self): + self._logged_queries = [] + self._log_executed_queries = True + def notice(self, msg): self.notices.append(msg) @@ -47,7 +66,15 @@ class MockPlPy: return MockCursor(data) def execute(self, query, rows=1): + if self._log_executed_queries: + self._logged_queries.append(query) for pattern, result in self.results.iteritems(): if pattern.search(query): return result return [] + + def quote_nullable(self, value): + if value is None: + return 'NULL' + else: + return "'{0}'".format(value) diff --git a/server/lib/python/cartodb_services/test/test_ratelimitsconfig.py b/server/lib/python/cartodb_services/test/test_ratelimitsconfig.py new file mode 100644 index 0000000..976b0c9 --- /dev/null +++ b/server/lib/python/cartodb_services/test/test_ratelimitsconfig.py @@ -0,0 +1,124 @@ +from test_helper import * +from unittest import TestCase +from mock import Mock, MagicMock, patch +from nose.tools import assert_raises, assert_not_equal, assert_equal + +from datetime import datetime, date +from mockredis import MockRedis +import cartodb_services + +from cartodb_services.tools import ServiceManager, LegacyServiceManager + +from cartodb_services.metrics import GeocoderConfig +from cartodb_services.refactor.service.mapzen_geocoder_config import MapzenGeocoderConfigBuilder +from cartodb_services.refactor.backend.redis_metrics_connection import RedisConnectionBuilder +from cartodb_services.tools import RateLimitExceeded + +from cartodb_services.refactor.storage.redis_config import * +from cartodb_services.refactor.storage.mem_config import InMemoryConfigStorage +from cartodb_services.refactor.backend.server_config import ServerConfigBackendFactory +from cartodb_services.config import RateLimitsConfig, RateLimitsConfigBuilder, RateLimitsConfigSetter + +class TestRateLimitsConfig(TestCase): + + def setUp(self): + plpy_mock_config() + cartodb_services.init(plpy_mock, _GD={}) + self.username = 'test_user' + self.orgname = 'test_org' + self.redis_conn = MockRedis() + build_redis_user_config(self.redis_conn, self.username, 'geocoding') + build_redis_org_config(self.redis_conn, self.orgname, 'geocoding', provider='mapzen') + self.environment = 'production' + plpy_mock._define_result("CDB_Conf_GetConf\('server_conf'\)", [{'conf': '{"environment": "production"}'}]) + plpy_mock._define_result("CDB_Conf_GetConf\('redis_metadata_config'\)", [{'conf': '{"redis_host":"localhost","redis_port":"6379"}'}]) + plpy_mock._define_result("CDB_Conf_GetConf\('redis_metrics_config'\)", [{'conf': '{"redis_host":"localhost","redis_port":"6379"}'}]) + basic_server_conf = {"server_conf": {"environment": "testing"}, + "mapzen_conf": + {"geocoder": + {"api_key": "search-xxxxxxx", "monthly_quota": 1500000, "service":{"base_url":"http://base"}} + }, "logger_conf": {}} + self.empty_server_config = InMemoryConfigStorage(basic_server_conf) + self.empty_redis_config = InMemoryConfigStorage({}) + self.user_config = RedisUserConfigStorageBuilder(self.redis_conn, self.username).get() + self.org_config = RedisOrgConfigStorageBuilder(self.redis_conn, self.orgname).get() + self.server_config = ServerConfigBackendFactory().get() + + def test_server_config(self): + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + + # Write server level configuration + config = RateLimitsConfig(service='geocoder', username=self.username, limit=1234, period=86400) + config_setter = RateLimitsConfigSetter(service='geocoder', username=self.username, orgname=self.orgname) + plpy_mock._start_logging_executed_queries() + config_setter.set_server_rate_limits(config) + assert plpy_mock._has_executed_query('cdb_conf_setconf(\'rate_limits\', \'{"geocoder": {"limit": 1234, "period": 86400}}\')') + + # Re-read configuration + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", [{'conf': '{"geocoder": {"limit": 1234, "period": 86400}}'}]) + read_config = RateLimitsConfigBuilder( + server_conf=self.server_config, + user_conf=self.empty_redis_config, + org_conf=self.empty_redis_config, + service='geocoder', + user=self.username, + org=self.orgname + ).get() + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) + assert_equal(read_config, config) + + def test_server_org_config(self): + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + + server_config = RateLimitsConfig(service='geocoder', username=self.username, limit=1234, period=86400) + org_config = RateLimitsConfig(service='geocoder', username=self.username, limit=1235, period=86400) + config_setter = RateLimitsConfigSetter(service='geocoder', username=self.username, orgname=self.orgname) + + # Write server level configuration + config_setter.set_server_rate_limits(server_config) + # Override with org level configuration + config_setter.set_org_rate_limits(org_config) + + # Re-read configuration + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", [{'conf': '{"geocoder": {"limit": 1234, "period": 86400}}'}]) + read_config = RateLimitsConfigBuilder( + server_conf=self.server_config, + user_conf=self.empty_redis_config, + org_conf=self.org_config, + service='geocoder', + user=self.username, + org=self.orgname + ).get() + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) + assert_equal(read_config, org_config) + + def test_server_org_user_config(self): + with patch.object(RedisConnectionBuilder,'get') as get_fn: + get_fn.return_value = self.redis_conn + + server_config = RateLimitsConfig(service='geocoder', username=self.username, limit=1234, period=86400) + org_config = RateLimitsConfig(service='geocoder', username=self.username, limit=1235, period=86400) + user_config = RateLimitsConfig(service='geocoder', username=self.username, limit=1236, period=86400) + config_setter = RateLimitsConfigSetter(service='geocoder', username=self.username, orgname=self.orgname) + + # Write server level configuration + config_setter.set_server_rate_limits(server_config) + # Override with org level configuration + config_setter.set_org_rate_limits(org_config) + # Override with user level configuration + config_setter.set_user_rate_limits(user_config) + + # Re-read configuration + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", [{'conf': '{"geocoder": {"limit": 1234, "period": 86400}}'}]) + read_config = RateLimitsConfigBuilder( + server_conf=self.server_config, + user_conf=self.user_config, + org_conf=self.org_config, + service='geocoder', + user=self.username, + org=self.orgname + ).get() + plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) + assert_equal(read_config, user_config) From 942c6ac63d8565eb69656e4650708de16700cc21 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 21 Mar 2017 15:44:50 +0100 Subject: [PATCH 17/31] Fix indentation --- .../cartodb_services/refactor/storage/redis_config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py index cc35c24..10dd21e 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/redis_config.py @@ -17,10 +17,10 @@ class RedisConfigStorage(ConfigBackendInterface): else: return self._data.get(key, default) - def set(self, key, value): + def set(self, key, value): self._connection.hset(self._config_key, key, value) - def remove(self, key): + def remove(self, key): self._connection.hdel(self._config_key, key) class RedisUserConfigStorageBuilder(object): From 63c03894cc7daa16129794e27fb3224a299ca7e1 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 21 Mar 2017 15:45:05 +0100 Subject: [PATCH 18/31] Fix bugs --- .../cartodb_services/refactor/storage/server_config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py index 7ee3fe5..67b6328 100644 --- a/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py +++ b/server/lib/python/cartodb_services/cartodb_services/refactor/storage/server_config.py @@ -10,7 +10,7 @@ class InDbServerConfigStorage(ConfigBackendInterface): json_output = None try: json_output = rows[0]['conf'] - except IndexError: + except (IndexError, KeyError): pass if (json_output): return json.loads(json_output) @@ -23,7 +23,7 @@ class InDbServerConfigStorage(ConfigBackendInterface): def set(self, key, config): json_config = json.dumps(config) quoted_config = cartodb_services.plpy.quote_nullable(json_config) - sql = "SELECT cdb_dataservices_server.cdb_conf_setconf('{0}', {0})".format(key, quoted_config) + sql = "SELECT cdb_dataservices_server.cdb_conf_setconf('{0}', {1})".format(key, quoted_config) cartodb_services.plpy.execute(sql) def remove(self, key): From cf7460c27d00630317f8ee9ea4b3451e709ea1e5 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 21 Mar 2017 16:17:39 +0100 Subject: [PATCH 19/31] Fix functions schema --- server/extension/sql/15_config_helper.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/extension/sql/15_config_helper.sql b/server/extension/sql/15_config_helper.sql index 95eccf8..2c5acf7 100644 --- a/server/extension/sql/15_config_helper.sql +++ b/server/extension/sql/15_config_helper.sql @@ -17,7 +17,7 @@ RETURNS JSON AS $$ $$ LANGUAGE SQL STABLE SECURITY DEFINER; CREATE OR REPLACE -FUNCTION cartodb.CDB_Conf_SetConf(key text, value JSON) +FUNCTION cdb_dataservices_server.CDB_Conf_SetConf(key text, value JSON) RETURNS void AS $$ BEGIN PERFORM cartodb.CDB_Conf_RemoveConf(key); @@ -26,7 +26,7 @@ END $$ LANGUAGE PLPGSQL VOLATILE; CREATE OR REPLACE -FUNCTION cartodb.CDB_Conf_RemoveConf(key text) +FUNCTION cdb_dataservices_server.CDB_Conf_RemoveConf(key text) RETURNS void AS $$ BEGIN EXECUTE 'DELETE FROM cartodb.CDB_CONF WHERE KEY = $1;' USING key; From ef0984052572ddf73ef49cb0d1b11fc40c1a9911 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 21 Mar 2017 17:58:33 +0100 Subject: [PATCH 20/31] Implement server-side rate limits configuration interface --- server/extension/sql/210_rates.sql | 91 +++++++++++++++++++ .../test/expected/210_rates_test.out | 44 +++++++++ server/extension/test/sql/210_rates_test.sql | 27 ++++++ 3 files changed, 162 insertions(+) create mode 100644 server/extension/sql/210_rates.sql create mode 100644 server/extension/test/expected/210_rates_test.out create mode 100644 server/extension/test/sql/210_rates_test.sql diff --git a/server/extension/sql/210_rates.sql b/server/extension/sql/210_rates.sql new file mode 100644 index 0000000..63e8d4e --- /dev/null +++ b/server/extension/sql/210_rates.sql @@ -0,0 +1,91 @@ + +CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_service_get_rate_limit( + username TEXT, + orgname TEXT, + service TEXT) +RETURNS JSON AS $$ + import json + from cartodb_services.config import ServiceConfiguration, RateLimitsConfigBuilder + + import cartodb_services + cartodb_services.init(plpy, GD) + + service_config = ServiceConfiguration(service, username, orgname) + rate_limit_config = RateLimitsConfigBuilder(service_config.server, service_config.user, service_config.org, service=service, user=username, org=orgname).get() + if rate_limit_config.is_limited(): + return json.dumps({'limit': rate_limit_config.limit, 'period': rate_limit_config.period}) + else: + return None +$$ LANGUAGE plpythonu; + +CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_service_set_user_rate_limit( + username TEXT, + orgname TEXT, + service TEXT, + rate_limit_json JSON) +RETURNS VOID AS $$ + import json + from cartodb_services.config import RateLimitsConfig, RateLimitsConfigSetter + + import cartodb_services + cartodb_services.init(plpy, GD) + + config_setter = RateLimitsConfigSetter(service=service, username=username, orgname=orgname) + if rate_limit_json: + rate_limit = json.loads(rate_limit_json) + limit = rate_limit.get('limit', None) + period = rate_limit.get('period', None) + else: + limit = None + period = None + config = RateLimitsConfig(service=service, username=username, limit=limit, period=period) + config_setter.set_user_rate_limits(config) +$$ LANGUAGE plpythonu; + +CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_service_set_org_rate_limit( + username TEXT, + orgname TEXT, + service TEXT, + rate_limit_json JSON) +RETURNS VOID AS $$ + import json + from cartodb_services.config import RateLimitsConfig, RateLimitsConfigSetter + + import cartodb_services + cartodb_services.init(plpy, GD) + + config_setter = RateLimitsConfigSetter(service=service, username=username, orgname=orgname) + if rate_limit_json: + rate_limit = json.loads(rate_limit_json) + limit = rate_limit.get('limit', None) + period = rate_limit.get('period', None) + else: + limit = None + period = None + config = RateLimitsConfig(service=service, username=username, limit=limit, period=period) + config_setter.set_org_rate_limits(config) +$$ LANGUAGE plpythonu; + +CREATE OR REPLACE FUNCTION cdb_dataservices_server.cdb_service_set_server_rate_limit( + username TEXT, + orgname TEXT, + service TEXT, + rate_limit_json JSON) +RETURNS VOID AS $$ + import json + from cartodb_services.config import RateLimitsConfig, RateLimitsConfigSetter + + import cartodb_services + cartodb_services.init(plpy, GD) + + config_setter = RateLimitsConfigSetter(service=service, username=username, orgname=orgname) + if rate_limit_json: + rate_limit = json.loads(rate_limit_json) + limit = rate_limit.get('limit', None) + period = rate_limit.get('period', None) + else: + limit = None + period = None + config = RateLimitsConfig(service=service, username=username, limit=limit, period=period) + config_setter.set_server_rate_limits(config) +$$ LANGUAGE plpythonu; diff --git a/server/extension/test/expected/210_rates_test.out b/server/extension/test/expected/210_rates_test.out new file mode 100644 index 0000000..f79cb33 --- /dev/null +++ b/server/extension/test/expected/210_rates_test.out @@ -0,0 +1,44 @@ +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_get_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text'); + exists +-------- + t +(1 row) + +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_set_user_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text, json'); + exists +-------- + t +(1 row) + +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_set_org_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text, json'); + exists +-------- + t +(1 row) + +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_set_server_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text, json'); + exists +-------- + t +(1 row) + diff --git a/server/extension/test/sql/210_rates_test.sql b/server/extension/test/sql/210_rates_test.sql new file mode 100644 index 0000000..752cdf4 --- /dev/null +++ b/server/extension/test/sql/210_rates_test.sql @@ -0,0 +1,27 @@ +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_get_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text'); + +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_set_user_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text, json'); + +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_set_org_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text, json'); + +SELECT exists(SELECT * + FROM pg_proc p + INNER JOIN pg_namespace ns ON (p.pronamespace = ns.oid) + WHERE ns.nspname = 'cdb_dataservices_server' + AND proname = 'cdb_service_set_server_rate_limit' + AND oidvectortypes(p.proargtypes) = 'text, text, text, json'); From 7101c8d8e8bcc4b5550eb53ec27dd7b24c3594c5 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 22 Mar 2017 16:31:45 +0100 Subject: [PATCH 21/31] Expose client-side rate limit configuration interface The client functions to make configuration changes are not publicly available (require a super user) and they have username, orgname parameters like the server-sixe functions --- client/renderer/interface.yaml | 26 +++++++ client/renderer/sql-template-renderer | 70 +++++++++++++++++-- .../templates/20_public_functions.erb | 17 ++--- .../25_exception_safe_private_functions.erb | 44 +++--------- .../templates/30_plproxy_functions.erb | 6 +- .../renderer/templates/90_grant_execute.erb | 4 +- 6 files changed, 108 insertions(+), 59 deletions(-) diff --git a/client/renderer/interface.yaml b/client/renderer/interface.yaml index fa7c257..f13f6eb 100644 --- a/client/renderer/interface.yaml +++ b/client/renderer/interface.yaml @@ -421,3 +421,29 @@ params: - { name: service, type: TEXT } - { name: input_size, type: NUMERIC } + +- name: cdb_service_get_rate_limit + return_type: json + params: + - { name: service, type: "text" } + +- name: cdb_service_set_user_rate_limit + private: true + return_type: void + params: + - { name: service, type: "text" } + - { name: rate_limit, type: json } + +- name: cdb_service_set_org_rate_limit + private: true + return_type: void + params: + - { name: service, type: "text" } + - { name: rate_limit, type: json } + +- name: cdb_service_set_server_rate_limit + private: true + return_type: void + params: + - { name: service, type: "text" } + - { name: rate_limit, type: json } diff --git a/client/renderer/sql-template-renderer b/client/renderer/sql-template-renderer index 2516a09..3019730 100755 --- a/client/renderer/sql-template-renderer +++ b/client/renderer/sql-template-renderer @@ -17,7 +17,7 @@ class SqlTemplateRenderer end def render - ERB.new(@template).result(binding) + ERB.new(@template, nil, '-').result(binding) end def name @@ -44,16 +44,29 @@ class SqlTemplateRenderer @function_signature['geocoder_config_key'] end - def params - @function_signature['params'].reject(&:empty?).map { |p| "#{p['name']}"} + def parameters_info(with_credentials) + parameters = [] + if with_credentials + parameters << { 'name' => 'username', 'type' => 'text' } + parameters << { 'name' => 'orgname', 'type' => 'text' } + end + parameters + @function_signature['params'].reject(&:empty?) end - def params_with_type - @function_signature['params'].reject(&:empty?).map { |p| "#{p['name']} #{p['type']}" } + def credentials_declaration() + "username text;\n orgname text;" if public_function? end - def params_with_type_and_default - parameters = @function_signature['params'].reject(&:empty?).map do |p| + def params(with_credentials = !public_function?) + parameters_info(with_credentials).map { |p| p['name'].to_s } + end + + def params_with_type(with_credentials = !public_function?) + parameters_info(with_credentials).map { |p| "#{p['name']} #{p['type']}" } + end + + def params_with_type_and_default(with_credentials = !public_function?) + parameters = parameters_info(with_credentials).map do |p| if not p['default'].nil? "#{p['name']} #{p['type']} DEFAULT #{p['default']}" else @@ -62,6 +75,49 @@ class SqlTemplateRenderer end return parameters end + + def public_function? + !@function_signature['private'] + end + + def void_return_type? + return_type.downcase == 'void' + end + + def return_declaration + "ret #{return_type};" unless void_return_type? || multi_row + end + + def return_statement(&block) + if block + erbout = block.binding.eval('_erbout') + + if multi_row + erbout << 'RETURN QUERY SELECT * FROM ' + elsif multi_field + erbout << 'SELECT * FROM ' + elsif void_return_type? + erbout << 'PERFORM ' + else + erbout << 'SELECT ' + end + yield + if multi_row || void_return_type? + erbout << ';' + else + erbout << ' INTO ret;' + end + if !multi_row && !void_return_type? + erbout << ' RETURN ret;' + end + else + if !multi_row && !void_return_type? + ' RETURN ret;' + end + end + end + + end diff --git a/client/renderer/templates/20_public_functions.erb b/client/renderer/templates/20_public_functions.erb index 88db53d..8e65ed5 100644 --- a/client/renderer/templates/20_public_functions.erb +++ b/client/renderer/templates/20_public_functions.erb @@ -7,9 +7,8 @@ CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>.<%= name %> (<%= params_with_type_and_default.join(' ,') %>) RETURNS <%= return_type %> AS $$ DECLARE - <% if not multi_row %>ret <%= return_type %>;<% end %> - username text; - orgname text; + <%= return_declaration if not multi_row %> + <%= credentials_declaration %> BEGIN IF session_user = 'publicuser' OR session_user ~ 'cartodb_publicuser_*' THEN RAISE EXCEPTION 'The api_key must be provided'; @@ -19,15 +18,7 @@ BEGIN IF username IS NULL OR username = '' OR username = '""' THEN RAISE EXCEPTION 'Username is a mandatory argument, check it out'; END IF; - <% if multi_row %> - RETURN QUERY - SELECT * FROM <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= ['username', 'orgname'].concat(params).join(', ') %>); - <% elsif multi_field %> - SELECT * FROM <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= ['username', 'orgname'].concat(params).join(', ') %>) INTO ret; - RETURN ret; - <% else %> - SELECT <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= ['username', 'orgname'].concat(params).join(', ') %>) INTO ret; - RETURN ret; - <% end %> + + <% return_statement do %><%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= params(true).join(', ') %>)<% end %> END; $$ LANGUAGE 'plpgsql' SECURITY DEFINER; diff --git a/client/renderer/templates/25_exception_safe_private_functions.erb b/client/renderer/templates/25_exception_safe_private_functions.erb index 2dde980..4b19b67 100644 --- a/client/renderer/templates/25_exception_safe_private_functions.erb +++ b/client/renderer/templates/25_exception_safe_private_functions.erb @@ -5,9 +5,8 @@ CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>_exception_safe (<%= params_with_type_and_default.join(' ,') %>) RETURNS <%= return_type %> AS $$ DECLARE - <% if not multi_row %>ret <%= return_type %>;<% end %> - username text; - orgname text; + <%= return_declaration %> + <%= credentials_declaration %> _returned_sqlstate TEXT; _message_text TEXT; _pg_exception_context TEXT; @@ -21,41 +20,16 @@ BEGIN RAISE EXCEPTION 'Username is a mandatory argument, check it out'; END IF; - <% if multi_row %> - BEGIN - RETURN QUERY - SELECT * FROM <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= ['username', 'orgname'].concat(params).join(', ') %>); - EXCEPTION - WHEN OTHERS THEN + + BEGIN + <% return_statement do %><%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= params(true).join(', ') %>)<% end %> + EXCEPTION + WHEN OTHERS THEN GET STACKED DIAGNOSTICS _returned_sqlstate = RETURNED_SQLSTATE, _message_text = MESSAGE_TEXT, _pg_exception_context = PG_EXCEPTION_CONTEXT; RAISE WARNING USING ERRCODE = _returned_sqlstate, MESSAGE = _message_text, DETAIL = _pg_exception_context; - END; - <% elsif multi_field %> - BEGIN - SELECT * FROM <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= ['username', 'orgname'].concat(params).join(', ') %>) INTO ret; - RETURN ret; - EXCEPTION - WHEN OTHERS THEN - GET STACKED DIAGNOSTICS _returned_sqlstate = RETURNED_SQLSTATE, - _message_text = MESSAGE_TEXT, - _pg_exception_context = PG_EXCEPTION_CONTEXT; - RAISE WARNING USING ERRCODE = _returned_sqlstate, MESSAGE = _message_text, DETAIL = _pg_exception_context; - RETURN ret; - END; - <% else %> - BEGIN - SELECT <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= ['username', 'orgname'].concat(params).join(', ') %>) INTO ret; - RETURN ret; - EXCEPTION - WHEN OTHERS THEN - GET STACKED DIAGNOSTICS _returned_sqlstate = RETURNED_SQLSTATE, - _message_text = MESSAGE_TEXT, - _pg_exception_context = PG_EXCEPTION_CONTEXT; - RAISE WARNING USING ERRCODE = _returned_sqlstate, MESSAGE = _message_text, DETAIL = _pg_exception_context; - RETURN ret; - END; - <% end %> + <%= return_statement %> + END; END; $$ LANGUAGE 'plpgsql' SECURITY DEFINER; diff --git a/client/renderer/templates/30_plproxy_functions.erb b/client/renderer/templates/30_plproxy_functions.erb index 2d5dfe3..92d2469 100644 --- a/client/renderer/templates/30_plproxy_functions.erb +++ b/client/renderer/templates/30_plproxy_functions.erb @@ -1,9 +1,9 @@ -CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %> (<%= ['username text', 'organization_name text'].concat(params_with_type_and_default).join(', ') %>) +CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %> (<%= params_with_type_and_default(true).join(', ') %>) RETURNS <%= return_type %> AS $$ CONNECT <%= DATASERVICES_CLIENT_SCHEMA %>._server_conn_str(); <% if multi_field %> - SELECT * FROM <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= ['username', 'organization_name'].concat(params).join(', ') %>); + SELECT * FROM <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= params(true).join(', ') %>); <% else %> - SELECT <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= ['username', 'organization_name'].concat(params).join(', ') %>); + SELECT <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= params(true).join(', ') %>); <% end %> $$ LANGUAGE plproxy; diff --git a/client/renderer/templates/90_grant_execute.erb b/client/renderer/templates/90_grant_execute.erb index b6c797e..924f495 100644 --- a/client/renderer/templates/90_grant_execute.erb +++ b/client/renderer/templates/90_grant_execute.erb @@ -1,2 +1,4 @@ +<% if public_function? %> GRANT EXECUTE ON FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>.<%= name %>(<%= params_with_type.join(', ') %>) TO publicuser; -GRANT EXECUTE ON FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>_exception_safe(<%= params_with_type.join(', ') %>) TO publicuser; +GRANT EXECUTE ON FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>_exception_safe(<%= params_with_type.join(', ') %> ) TO publicuser; +<% end %> \ No newline at end of file From e850d5c72e96ea50dfc127bd3cf2650d6aed0f1f Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 22 Mar 2017 16:57:15 +0100 Subject: [PATCH 22/31] Add rate limits documentation --- doc/rate_limits.md | 110 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 doc/rate_limits.md diff --git a/doc/rate_limits.md b/doc/rate_limits.md new file mode 100644 index 0000000..7255e77 --- /dev/null +++ b/doc/rate_limits.md @@ -0,0 +1,110 @@ +# Rate limits + +Services can be rate-limited. (currently only gecoding with Mapzen or Here providers is limited) + +The rate limits configuration can be established at server, organization or user levels, the latter having precedence over the earlier. + +The default configuration (a null or empty configuration) doesn't impose any limits. + +The configuration consist of a JSON object with two attributes: + +* `period`: the rate-limiting period, in seconds. +* `limit`: the maximum number of request in the established period. + +If a service request exceeds the configured rate limits +(i.e. if more than `limit` calls are performe in a fixed interval of +duration `period` seconds) the call will fail with an "Rate limit exceeded" error. + +## Server-side interface + +There's a server-side SQL interface to query or change the configuration. + +### cdb_dataservices_server.cdb_service_get_rate_limit(username, orgname, service) + +This function returns the rate limit configuration for a given user and service. + +#### Returns + +The result is a JSON object with the configuration (`period` and `limit` attributes as explained above). + +### cdb_dataservices_server.cdb_service_set_user_rate_limit(username, orgname, service, rate_limit) + +This function sets the rate limit configuration for the user. This overrides any other configuration. + +The configuration is provided as a JSON literal. To remove the user-level configuration `NULL` should be passed as the `rate_limit`. + +#### Returns + +This functions doesn't return any value. + +### cdb_dataservices_server.cdb_service_set_org_rate_limit(username, orgname, service, rate_limit) + +This function sets the rate limit configuration for the organization. +This overrides server level configuration and is overriden by user configuration if present. + +The configuration is provided as a JSON literal. To remove the organization-level configuration `NULL` should be passed as the `rate_limit`. + +#### Returns + +This functions doesn't return any value. + +### cdb_dataservices_server.cdb_service_set_server_rate_limit(username, orgname, service, rate_limit) + +This function sets the default rate limit configuration for all users accesing the dataservices server. This is overriden by organization of user configuration. + +The configuration is provided as a JSON literal. To remove the organization-level configuration `NULL` should be passed as the `rate_limit`. + +#### Returns + +This functions doesn't return any value. + +## Client-side interface + +For convenience there's also a client-side interface (in the client dataservices-api extension), consisting +of public functions to get the current configuration and privileged functions to change it. + +### Public functions + +### cdb_dataservices_client.cdb_service_get_rate_limit(service) + +This function returns the rate limit configuration for the specified service +and the user corresponding to the role which makes the calls. + +#### Returns + +The result is a JSON object with the configuration (`period` and `limit` attributes as explained above). + +### Privileged function + +Thes functions are not accessible by regular user roles, and the user and organization names must be provided as parameters. + +### cdb_dataservices_server.cdb_service_set_user_rate_limit(username, orgname, service, rate_limit) + +This function sets the rate limit configuration for the user. This overrides any other configuration. + +The configuration is provided as a JSON literal. To remove the user-level configuration `NULL` should be passed as the `rate_limit`. + +#### Returns + +This functions doesn't return any value. + +### cdb_dataservices_server.cdb_service_set_org_rate_limit(username, orgname, service, rate_limit) + +This function sets the rate limit configuration for the organization. +This overrides server level configuration and is overriden by user configuration if present. + +The configuration is provided as a JSON literal. To remove the organization-level configuration `NULL` should be passed as the `rate_limit`. + +#### Returns + +This functions doesn't return any value. + +### cdb_dataservices_server.cdb_service_set_server_rate_limit(username, orgname, service, rate_limit) + +This function sets the default rate limit configuration for all users accesing the dataservices server. This is overriden by organization of user configuration. + +The configuration is provided as a JSON literal. To remove the organization-level configuration `NULL` should be passed as the `rate_limit`. + +#### Returns + +This functions doesn't return any value. From 41c5271de1dbc3b8278a4e4228ae6f139be242f4 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Wed, 22 Mar 2017 17:01:03 +0100 Subject: [PATCH 23/31] Fix documentation --- doc/rate_limits.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/rate_limits.md b/doc/rate_limits.md index 7255e77..84db54a 100644 --- a/doc/rate_limits.md +++ b/doc/rate_limits.md @@ -78,7 +78,7 @@ The result is a JSON object with the configuration (`period` and `limit` attribu Thes functions are not accessible by regular user roles, and the user and organization names must be provided as parameters. -### cdb_dataservices_server.cdb_service_set_user_rate_limit(username, orgname, service, rate_limit) +### cdb_dataservices_client.cdb_service_set_user_rate_limit(username, orgname, service, rate_limit) This function sets the rate limit configuration for the user. This overrides any other configuration. @@ -88,7 +88,7 @@ The configuration is provided as a JSON literal. To remove the user-level config This functions doesn't return any value. -### cdb_dataservices_server.cdb_service_set_org_rate_limit(username, orgname, service, rate_limit) +### cdb_dataservices_client.cdb_service_set_org_rate_limit(username, orgname, service, rate_limit) This function sets the rate limit configuration for the organization. This overrides server level configuration and is overriden by user configuration if present. @@ -99,7 +99,7 @@ The configuration is provided as a JSON literal. To remove the organization-leve This functions doesn't return any value. -### cdb_dataservices_server.cdb_service_set_server_rate_limit(username, orgname, service, rate_limit) +### cdb_dataservices_client.cdb_service_set_server_rate_limit(username, orgname, service, rate_limit) This function sets the default rate limit configuration for all users accesing the dataservices server. This is overriden by organization of user configuration. From c379593d89de3ea7d7248ca927f47e1baac478a7 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Thu, 23 Mar 2017 15:59:13 +0100 Subject: [PATCH 24/31] Fix rate limits documentation --- doc/rate_limits.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/rate_limits.md b/doc/rate_limits.md index 84db54a..25b2af3 100644 --- a/doc/rate_limits.md +++ b/doc/rate_limits.md @@ -1,6 +1,6 @@ # Rate limits -Services can be rate-limited. (currently only gecoding with Mapzen or Here providers is limited) +Services can be rate-limited. (currently only gecoding is limited) The rate limits configuration can be established at server, organization or user levels, the latter having precedence over the earlier. From b1e765c63946a0a88e7df471a6ada917b724fa21 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 28 Mar 2017 09:54:00 +0200 Subject: [PATCH 25/31] Clarify what configuration is retrieved by cdb_service_get_rate_limit --- doc/rate_limits.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/rate_limits.md b/doc/rate_limits.md index 25b2af3..71fcb55 100644 --- a/doc/rate_limits.md +++ b/doc/rate_limits.md @@ -67,8 +67,10 @@ of public functions to get the current configuration and privileged functions to ### cdb_dataservices_client.cdb_service_get_rate_limit(service) -This function returns the rate limit configuration for the specified service -and the user corresponding to the role which makes the calls. +This function returns the rate limit configuration in effect for the specified service +and the user corresponding to the role which makes the calls. The effective configuration +may come from any of the configuration levels (server/organization/user); only the +existing configuration with most precedence is returned. #### Returns From 2d6b73cb9d08ba7d2acda71231d7b00ac414afc4 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 28 Mar 2017 10:19:58 +0200 Subject: [PATCH 26/31] Add some examples to the documentation --- doc/rate_limits.md | 85 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/doc/rate_limits.md b/doc/rate_limits.md index 71fcb55..333a8f3 100644 --- a/doc/rate_limits.md +++ b/doc/rate_limits.md @@ -65,6 +65,10 @@ of public functions to get the current configuration and privileged functions to ### Public functions +These functions are accesible to non-privileged roles, and should only be executed +using the role corresponding to a CARTO user, since that will determine the +user and organization to which the rate limits configuration applies. + ### cdb_dataservices_client.cdb_service_get_rate_limit(service) This function returns the rate limit configuration in effect for the specified service @@ -76,7 +80,19 @@ existing configuration with most precedence is returned. The result is a JSON object with the configuration (`period` and `limit` attributes as explained above). -### Privileged function +#### Example: + +``` +SELECT cdb_dataservices_client.cdb_service_get_rate_limit('geocoding'); + + cdb_service_get_rate_limit +--------------------------------- + {"limit": 1000, "period": 86400} +(1 row) +``` + + +### Privileged (superuser) functions Thes functions are not accessible by regular user roles, and the user and organization names must be provided as parameters. @@ -90,6 +106,27 @@ The configuration is provided as a JSON literal. To remove the user-level config This functions doesn't return any value. +#### Example + +This will configure the geocoding service rate limit for user `myusername`, a non-organization user. +The limit will be set at 1000 requests per day. Since the user doesn't belong to any organization, +`NULL` will be passed to the organization argument; otherwise the name of the user's organization should +be provided. + +``` +SELECT cdb_dataservices_client.cdb_service_set_user_rate_limit( + 'myusername', + NULL, + 'geocoding', + '{"limit":1000,"period":86400}' +); + + cdb_service_set_user_rate_limit +--------------------------------- + +(1 row) +``` + ### cdb_dataservices_client.cdb_service_set_org_rate_limit(username, orgname, service, rate_limit) This function sets the rate limit configuration for the organization. @@ -101,6 +138,28 @@ The configuration is provided as a JSON literal. To remove the organization-leve This functions doesn't return any value. +#### Example + +This will configure the geocoding service rate limit for the `myorg` organization. +The limit will be set at 100 requests per hour. +Note that even we're setting the default configuration for the whole organization, +the name of a user of the organization must be provided for technical reasons. + +``` +SELECT cdb_dataservices_client.cdb_service_set_org_rate_limit( + 'myorgadmin', + 'myorg', + 'geocoding', + '{"limit":100,"period":3600}' +); + + + cdb_service_set_org_rate_limit +--------------------------------- + +(1 row) +``` + ### cdb_dataservices_client.cdb_service_set_server_rate_limit(username, orgname, service, rate_limit) This function sets the default rate limit configuration for all users accesing the dataservices server. This is overriden by organization of user configuration. @@ -110,3 +169,27 @@ The configuration is provided as a JSON literal. To remove the organization-leve #### Returns This functions doesn't return any value. + +#### Example + +This will configure the default geocoding service rate limit for all users +accesing the data-services server. +The limit will be set at 10000 requests per month. +Note that even we're setting the default configuration for the server, +the name of a user and the name of the corresponding organization (or NULL) +must be provided for technical reasons. + +``` +SELECT cdb_dataservices_client.cdb_service_set_server_rate_limit( + 'myorgadmin', + 'myorg', + 'geocoding', + '{"limit":10000,"period":108000}' +); + + + cdb_service_set_server_rate_limit +--------------------------------- + +(1 row) +``` From 39878ef5428ec51fee561eca5dbc035ce7ef3489 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 28 Mar 2017 10:37:21 +0200 Subject: [PATCH 27/31] Rename some template functions internal terms * credentials => user_org * private => superuser --- client/renderer/interface.yaml | 6 ++--- client/renderer/sql-template-renderer | 24 +++++++++---------- .../templates/20_public_functions.erb | 4 ++-- .../25_exception_safe_private_functions.erb | 4 ++-- .../templates/30_plproxy_functions.erb | 6 ++--- .../renderer/templates/90_grant_execute.erb | 2 +- 6 files changed, 23 insertions(+), 23 deletions(-) diff --git a/client/renderer/interface.yaml b/client/renderer/interface.yaml index f13f6eb..7211802 100644 --- a/client/renderer/interface.yaml +++ b/client/renderer/interface.yaml @@ -428,21 +428,21 @@ - { name: service, type: "text" } - name: cdb_service_set_user_rate_limit - private: true + superuser: true return_type: void params: - { name: service, type: "text" } - { name: rate_limit, type: json } - name: cdb_service_set_org_rate_limit - private: true + superuser: true return_type: void params: - { name: service, type: "text" } - { name: rate_limit, type: json } - name: cdb_service_set_server_rate_limit - private: true + superuser: true return_type: void params: - { name: service, type: "text" } diff --git a/client/renderer/sql-template-renderer b/client/renderer/sql-template-renderer index 3019730..31d52f5 100755 --- a/client/renderer/sql-template-renderer +++ b/client/renderer/sql-template-renderer @@ -44,29 +44,29 @@ class SqlTemplateRenderer @function_signature['geocoder_config_key'] end - def parameters_info(with_credentials) + def parameters_info(with_user_org) parameters = [] - if with_credentials + if with_user_org parameters << { 'name' => 'username', 'type' => 'text' } parameters << { 'name' => 'orgname', 'type' => 'text' } end parameters + @function_signature['params'].reject(&:empty?) end - def credentials_declaration() - "username text;\n orgname text;" if public_function? + def user_org_declaration() + "username text;\n orgname text;" unless superuser_function? end - def params(with_credentials = !public_function?) - parameters_info(with_credentials).map { |p| p['name'].to_s } + def params(with_user_org = superuser_function?) + parameters_info(with_user_org).map { |p| p['name'].to_s } end - def params_with_type(with_credentials = !public_function?) - parameters_info(with_credentials).map { |p| "#{p['name']} #{p['type']}" } + def params_with_type(with_user_org = superuser_function?) + parameters_info(with_user_org).map { |p| "#{p['name']} #{p['type']}" } end - def params_with_type_and_default(with_credentials = !public_function?) - parameters = parameters_info(with_credentials).map do |p| + def params_with_type_and_default(with_user_org = superuser_function?) + parameters = parameters_info(with_user_org).map do |p| if not p['default'].nil? "#{p['name']} #{p['type']} DEFAULT #{p['default']}" else @@ -76,8 +76,8 @@ class SqlTemplateRenderer return parameters end - def public_function? - !@function_signature['private'] + def superuser_function? + !!@function_signature['superuser'] end def void_return_type? diff --git a/client/renderer/templates/20_public_functions.erb b/client/renderer/templates/20_public_functions.erb index 8e65ed5..abd6d05 100644 --- a/client/renderer/templates/20_public_functions.erb +++ b/client/renderer/templates/20_public_functions.erb @@ -8,7 +8,7 @@ CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>.<%= name %> (<%= pa RETURNS <%= return_type %> AS $$ DECLARE <%= return_declaration if not multi_row %> - <%= credentials_declaration %> + <%= user_org_declaration %> BEGIN IF session_user = 'publicuser' OR session_user ~ 'cartodb_publicuser_*' THEN RAISE EXCEPTION 'The api_key must be provided'; @@ -19,6 +19,6 @@ BEGIN RAISE EXCEPTION 'Username is a mandatory argument, check it out'; END IF; - <% return_statement do %><%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= params(true).join(', ') %>)<% end %> + <% return_statement do %><%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= params(_with_user_org=true).join(', ') %>)<% end %> END; $$ LANGUAGE 'plpgsql' SECURITY DEFINER; diff --git a/client/renderer/templates/25_exception_safe_private_functions.erb b/client/renderer/templates/25_exception_safe_private_functions.erb index 4b19b67..b0d2921 100644 --- a/client/renderer/templates/25_exception_safe_private_functions.erb +++ b/client/renderer/templates/25_exception_safe_private_functions.erb @@ -6,7 +6,7 @@ CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>_except RETURNS <%= return_type %> AS $$ DECLARE <%= return_declaration %> - <%= credentials_declaration %> + <%= user_org_declaration %> _returned_sqlstate TEXT; _message_text TEXT; _pg_exception_context TEXT; @@ -22,7 +22,7 @@ BEGIN BEGIN - <% return_statement do %><%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= params(true).join(', ') %>)<% end %> + <% return_statement do %><%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>(<%= params(_with_user_org=true).join(', ') %>)<% end %> EXCEPTION WHEN OTHERS THEN GET STACKED DIAGNOSTICS _returned_sqlstate = RETURNED_SQLSTATE, diff --git a/client/renderer/templates/30_plproxy_functions.erb b/client/renderer/templates/30_plproxy_functions.erb index 92d2469..7a7a230 100644 --- a/client/renderer/templates/30_plproxy_functions.erb +++ b/client/renderer/templates/30_plproxy_functions.erb @@ -1,9 +1,9 @@ -CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %> (<%= params_with_type_and_default(true).join(', ') %>) +CREATE OR REPLACE FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %> (<%= params_with_type_and_default(_with_user_org=true).join(', ') %>) RETURNS <%= return_type %> AS $$ CONNECT <%= DATASERVICES_CLIENT_SCHEMA %>._server_conn_str(); <% if multi_field %> - SELECT * FROM <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= params(true).join(', ') %>); + SELECT * FROM <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= params(_with_user_org=true).join(', ') %>); <% else %> - SELECT <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= params(true).join(', ') %>); + SELECT <%= DATASERVICES_SERVER_SCHEMA %>.<%= name %> (<%= params(_with_user_org=true).join(', ') %>); <% end %> $$ LANGUAGE plproxy; diff --git a/client/renderer/templates/90_grant_execute.erb b/client/renderer/templates/90_grant_execute.erb index 924f495..1b8d7c8 100644 --- a/client/renderer/templates/90_grant_execute.erb +++ b/client/renderer/templates/90_grant_execute.erb @@ -1,4 +1,4 @@ -<% if public_function? %> +<% unless superuser_function? %> GRANT EXECUTE ON FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>.<%= name %>(<%= params_with_type.join(', ') %>) TO publicuser; GRANT EXECUTE ON FUNCTION <%= DATASERVICES_CLIENT_SCHEMA %>._<%= name %>_exception_safe(<%= params_with_type.join(', ') %> ) TO publicuser; <% end %> \ No newline at end of file From 970d8287685a1490f18280ebb0714f0337163a15 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 28 Mar 2017 10:39:23 +0200 Subject: [PATCH 28/31] Remove unneeded ERB options --- client/renderer/sql-template-renderer | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/renderer/sql-template-renderer b/client/renderer/sql-template-renderer index 31d52f5..c286d13 100755 --- a/client/renderer/sql-template-renderer +++ b/client/renderer/sql-template-renderer @@ -17,7 +17,7 @@ class SqlTemplateRenderer end def render - ERB.new(@template, nil, '-').result(binding) + ERB.new(@template).result(binding) end def name From 4b18e1f60105fe26b60f1c8398bbaf33899f7cb2 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 28 Mar 2017 10:42:03 +0200 Subject: [PATCH 29/31] Rename variable --- client/renderer/sql-template-renderer | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/client/renderer/sql-template-renderer b/client/renderer/sql-template-renderer index c286d13..4ad22d0 100755 --- a/client/renderer/sql-template-renderer +++ b/client/renderer/sql-template-renderer @@ -90,25 +90,25 @@ class SqlTemplateRenderer def return_statement(&block) if block - erbout = block.binding.eval('_erbout') + erb_out = block.binding.eval('_erbout') if multi_row - erbout << 'RETURN QUERY SELECT * FROM ' + erb_out << 'RETURN QUERY SELECT * FROM ' elsif multi_field - erbout << 'SELECT * FROM ' + erb_out << 'SELECT * FROM ' elsif void_return_type? - erbout << 'PERFORM ' + erb_out << 'PERFORM ' else - erbout << 'SELECT ' + erb_out << 'SELECT ' end yield if multi_row || void_return_type? - erbout << ';' + erb_out << ';' else - erbout << ' INTO ret;' + erb_out << ' INTO ret;' end if !multi_row && !void_return_type? - erbout << ' RETURN ret;' + erb_out << ' RETURN ret;' end else if !multi_row && !void_return_type? From 6b86acfaa3206dd8e3558d9810ab897e6f095d23 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 28 Mar 2017 10:44:25 +0200 Subject: [PATCH 30/31] Fix indentation --- server/extension/sql/20_geocode_street.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/extension/sql/20_geocode_street.sql b/server/extension/sql/20_geocode_street.sql index b3daec9..ffed77c 100644 --- a/server/extension/sql/20_geocode_street.sql +++ b/server/extension/sql/20_geocode_street.sql @@ -145,7 +145,7 @@ RETURNS Geometry AS $$ geocoder = MapzenGeocoder(service_manager.config.mapzen_api_key, service_manager.logger, service_manager.config.service_params) country_iso3 = None if country: - country_iso3 = country_to_iso3(country) + country_iso3 = country_to_iso3(country) coordinates = geocoder.geocode(searchtext=searchtext, city=city, state_province=state_province, country=country_iso3, search_type='address') From 2c15110255540e1d5ce5bbacc3ccd15f9a4fa135 Mon Sep 17 00:00:00 2001 From: Javier Goizueta Date: Tue, 28 Mar 2017 10:53:16 +0200 Subject: [PATCH 31/31] Rename constructor arguments for consistency --- server/extension/sql/210_rates.sql | 2 +- .../cartodb_services/config/legacy_rate_limits.py | 6 +++--- .../cartodb_services/config/rate_limits.py | 7 +++---- .../cartodb_services/tools/legacy_service_manager.py | 2 +- .../cartodb_services/tools/service_manager.py | 2 +- .../cartodb_services/test/test_ratelimitsconfig.py | 12 ++++++------ 6 files changed, 15 insertions(+), 16 deletions(-) diff --git a/server/extension/sql/210_rates.sql b/server/extension/sql/210_rates.sql index 63e8d4e..058c766 100644 --- a/server/extension/sql/210_rates.sql +++ b/server/extension/sql/210_rates.sql @@ -11,7 +11,7 @@ RETURNS JSON AS $$ cartodb_services.init(plpy, GD) service_config = ServiceConfiguration(service, username, orgname) - rate_limit_config = RateLimitsConfigBuilder(service_config.server, service_config.user, service_config.org, service=service, user=username, org=orgname).get() + rate_limit_config = RateLimitsConfigBuilder(service_config.server, service_config.user, service_config.org, service=service, username=username, orgname=orgname).get() if rate_limit_config.is_limited(): return json.dumps({'limit': rate_limit_config.limit, 'period': rate_limit_config.period}) else: diff --git a/server/lib/python/cartodb_services/cartodb_services/config/legacy_rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/config/legacy_rate_limits.py index d4208ec..e50da06 100644 --- a/server/lib/python/cartodb_services/cartodb_services/config/legacy_rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/config/legacy_rate_limits.py @@ -6,10 +6,10 @@ class RateLimitsConfigLegacyBuilder(object): Build a RateLimitsConfig object using the *legacy* configuration classes """ - def __init__(self, redis_connection, db_conn, service, user, org): + def __init__(self, redis_connection, db_conn, service, username, orgname): self._service = service - self._username = user - self._orgname = org + self._username = username + self._orgname = orgname self._redis_connection = redis_connection self._db_conn = db_conn diff --git a/server/lib/python/cartodb_services/cartodb_services/config/rate_limits.py b/server/lib/python/cartodb_services/cartodb_services/config/rate_limits.py index 814a1f3..98f6a47 100644 --- a/server/lib/python/cartodb_services/cartodb_services/config/rate_limits.py +++ b/server/lib/python/cartodb_services/cartodb_services/config/rate_limits.py @@ -49,14 +49,13 @@ class RateLimitsConfigBuilder(object): from the user/org/server configuration. """ - # TODO: user->username, org->orgname - def __init__(self, server_conf, user_conf, org_conf, service, user, org): + def __init__(self, server_conf, user_conf, org_conf, service, username, orgname): self._server_conf = server_conf self._user_conf = user_conf self._org_conf = org_conf self._service = service - self._username = user - self._orgname = org + self._username = username + self._orgname = orgname def get(self): # Order of precedence is user_conf, org_conf, server_conf diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py index 63bb90f..2ae700c 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/legacy_service_manager.py @@ -19,5 +19,5 @@ class LegacyServiceManager(ServiceManagerBase): self.quota_service = QuotaService(self.config, redis_conn) - rate_limit_config = RateLimitsConfigLegacyBuilder(redis_conn, plpy, service=service, user=username, org=orgname).get() + rate_limit_config = RateLimitsConfigLegacyBuilder(redis_conn, plpy, service=service, username=username, orgname=orgname).get() self.rate_limiter = RateLimiter(rate_limit_config, redis_conn) diff --git a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py index 22d1677..d493e40 100644 --- a/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py +++ b/server/lib/python/cartodb_services/cartodb_services/tools/service_manager.py @@ -62,7 +62,7 @@ class ServiceManager(ServiceManagerBase): self.logger = Logger(logger_config) self.config = config_builder(service_config.server, service_config.user, service_config.org, username, orgname).get() - rate_limit_config = RateLimitsConfigBuilder(service_config.server, service_config.user, service_config.org, service=service, user=username, org=orgname).get() + rate_limit_config = RateLimitsConfigBuilder(service_config.server, service_config.user, service_config.org, service=service, username=username, orgname=orgname).get() redis_metrics_connection = RedisMetricsConnectionFactory(service_config.environment, service_config.server).get() diff --git a/server/lib/python/cartodb_services/test/test_ratelimitsconfig.py b/server/lib/python/cartodb_services/test/test_ratelimitsconfig.py index 976b0c9..5cb967c 100644 --- a/server/lib/python/cartodb_services/test/test_ratelimitsconfig.py +++ b/server/lib/python/cartodb_services/test/test_ratelimitsconfig.py @@ -62,8 +62,8 @@ class TestRateLimitsConfig(TestCase): user_conf=self.empty_redis_config, org_conf=self.empty_redis_config, service='geocoder', - user=self.username, - org=self.orgname + username=self.username, + orgname=self.orgname ).get() plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) assert_equal(read_config, config) @@ -88,8 +88,8 @@ class TestRateLimitsConfig(TestCase): user_conf=self.empty_redis_config, org_conf=self.org_config, service='geocoder', - user=self.username, - org=self.orgname + username=self.username, + orgname=self.orgname ).get() plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) assert_equal(read_config, org_config) @@ -117,8 +117,8 @@ class TestRateLimitsConfig(TestCase): user_conf=self.user_config, org_conf=self.org_config, service='geocoder', - user=self.username, - org=self.orgname + username=self.username, + orgname=self.orgname ).get() plpy_mock._define_result("CDB_Conf_GetConf\('rate_limits'\)", []) assert_equal(read_config, user_config)