From 7877c6ab94d5bc14927b8f06b7763289ae608ab9 Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Mon, 5 Dec 2016 16:07:00 -0500 Subject: [PATCH 1/5] add rate limiting to build queues --- config.py | 4 ++++ data/queue.py | 4 ++++ endpoints/api/build.py | 9 ++++++--- endpoints/api/trigger.py | 6 ++++-- endpoints/building.py | 12 ++++++++++++ endpoints/webhooks.py | 9 ++++++--- 6 files changed, 36 insertions(+), 8 deletions(-) diff --git a/config.py b/config.py index 3b9eebfc8..ca86ec745 100644 --- a/config.py +++ b/config.py @@ -399,3 +399,7 @@ class DefaultConfig(object): # Location of the static marketing site. STATIC_SITE_BUCKET = None + + # Maximum number of builds allowed to be queued per repository before rejecting requests. + # Values less than zero allow queues of infinite size. + MAX_BUILD_QUEUE_SIZE = -1 diff --git a/data/queue.py b/data/queue.py index 421d7d6e7..059dd7d86 100644 --- a/data/queue.py +++ b/data/queue.py @@ -77,6 +77,10 @@ class WorkQueue(object): ._available_jobs(now, name_match_query) .where(~(QueueItem.queue_name << running_query))) + def num_available_jobs(self, prefix): + prefix = prefix.lstrip('/') + return self._available_jobs(datetime.utcnow(), self._name_match_query() + prefix).count() + def _name_match_query(self): return '%s%%' % self._canonical_name([self._queue_name] + self._canonical_name_match_list) diff --git a/endpoints/api/build.py b/endpoints/api/build.py index 7efedc993..36daf2bde 100644 --- a/endpoints/api/build.py +++ b/endpoints/api/build.py @@ -14,9 +14,9 @@ from buildtrigger.basehandler import BuildTriggerHandler from endpoints.api import (RepositoryParamResource, parse_args, query_param, nickname, resource, require_repo_read, require_repo_write, validate_json_request, ApiResource, internal_only, format_date, api, path_param, - require_repo_admin) + require_repo_admin, abort) from endpoints.exception import Unauthorized, NotFound, InvalidRequest -from endpoints.building import start_build, PreparedBuild +from endpoints.building import start_build, PreparedBuild, MaximumBuildsQueuedException from data import database from data import model from auth.permissions import (ReadRepositoryPermission, ModifyRepositoryPermission, @@ -287,7 +287,10 @@ class RepositoryBuildList(RepositoryParamResource): prepared.is_manual = True prepared.metadata = {} - build_request = start_build(repo, prepared, pull_robot_name=pull_robot_name) + try: + build_request = start_build(repo, prepared, pull_robot_name=pull_robot_name) + except MaximumBuildsQueuedException: + abort(429, message='Maximum queued build rate exceeded.') resp = build_status_view(build_request) repo_string = '%s/%s' % (namespace, repository) headers = { diff --git a/endpoints/api/trigger.py b/endpoints/api/trigger.py index da9c98087..c03e9fbd9 100644 --- a/endpoints/api/trigger.py +++ b/endpoints/api/trigger.py @@ -15,10 +15,10 @@ from buildtrigger.triggerutil import (TriggerDeactivationException, RepositoryReadException, TriggerStartException) from endpoints.api import (RepositoryParamResource, nickname, resource, require_repo_admin, log_action, request_error, query_param, parse_args, internal_only, - validate_json_request, api, path_param) + validate_json_request, api, path_param, abort) from endpoints.exception import NotFound, Unauthorized, InvalidRequest from endpoints.api.build import build_status_view, trigger_view, RepositoryBuildStatus -from endpoints.building import start_build +from endpoints.building import start_build, MaximumBuildsQueuedException from data import model from auth.permissions import (UserAdminPermission, AdministerOrganizationPermission, ReadRepositoryPermission, AdministerRepositoryPermission) @@ -436,6 +436,8 @@ class ActivateBuildTrigger(RepositoryParamResource): build_request = start_build(repo, prepared, pull_robot_name=pull_robot_name) except TriggerStartException as tse: raise InvalidRequest(tse.message) + except MaximumBuildsQueuedException: + abort(429, message='Maximum queued build rate exceeded.') resp = build_status_view(build_request) repo_string = '%s/%s' % (namespace_name, repo_name) diff --git a/endpoints/building.py b/endpoints/building.py index 977a964a3..c4aa98e6c 100644 --- a/endpoints/building.py +++ b/endpoints/building.py @@ -11,10 +11,22 @@ from endpoints.notificationhelper import spawn_notification from util.names import escape_tag from util.morecollections import AttrDict + logger = logging.getLogger(__name__) +MAX_BUILD_QUEUE_SIZE = app.config.get('MAX_BUILD_QUEUE_SIZE', -1) + + +class MaximumBuildsQueuedException(Exception): + pass + def start_build(repository, prepared_build, pull_robot_name=None): + queue_item_prefix = '%s/%s' % repository.namespace_user.username, repository.name + available_builds = dockerfile_build_queue.num_available_jobs(queue_item_prefix) + if available_builds >= MAX_BUILD_QUEUE_SIZE and MAX_BUILD_QUEUE_SIZE > -1: + raise MaximumBuildsQueuedException() + host = app.config['SERVER_HOSTNAME'] repo_path = '%s/%s/%s' % (host, repository.namespace_user.username, repository.name) diff --git a/endpoints/webhooks.py b/endpoints/webhooks.py index a65036705..c89d9b26e 100644 --- a/endpoints/webhooks.py +++ b/endpoints/webhooks.py @@ -10,9 +10,9 @@ from util.invoice import renderInvoiceToHtml from util.useremails import send_invoice_email, send_subscription_change, send_payment_failed from util.http import abort from buildtrigger.basehandler import BuildTriggerHandler -from buildtrigger.triggerutil import (ValidationRequestException, SkipRequestException, +from buildtrigger.triggerutil import (ValidationRequestException, SkipRequestException, InvalidPayloadException) -from endpoints.building import start_build +from endpoints.building import start_build, MaximumBuildsQueuedException logger = logging.getLogger(__name__) @@ -104,7 +104,10 @@ def build_trigger_webhook(trigger_uuid, **kwargs): pull_robot_name = model.build.get_pull_robot_name(trigger) repo = model.repository.get_repository(namespace, repository) - start_build(repo, prepared, pull_robot_name=pull_robot_name) + try: + start_build(repo, prepared, pull_robot_name=pull_robot_name) + except MaximumBuildsQueuedException: + abort(429) return make_response('Okay') From 57770493fa9321275aef08272017b31922dec619 Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Tue, 6 Dec 2016 13:59:47 -0500 Subject: [PATCH 2/5] build rate limiting: use a rate --- config.py | 8 +++++--- data/queue.py | 6 ++++-- endpoints/building.py | 13 +++++++++---- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/config.py b/config.py index ca86ec745..1e1eb5c9d 100644 --- a/config.py +++ b/config.py @@ -400,6 +400,8 @@ class DefaultConfig(object): # Location of the static marketing site. STATIC_SITE_BUCKET = None - # Maximum number of builds allowed to be queued per repository before rejecting requests. - # Values less than zero allow queues of infinite size. - MAX_BUILD_QUEUE_SIZE = -1 + # Count and duration used to produce a rate of builds allowed to be queued per repository before + # rejecting requests. Values less than zero disable rate limiting. + # Example: 10 builds per minute is accomplished by setting ITEMS = 10, SECS = 60 + MAX_BUILD_QUEUE_RATE_ITEMS = -1 + MAX_BUILD_QUEUE_RATE_SECS = -1 diff --git a/data/queue.py b/data/queue.py index 059dd7d86..5cd84726d 100644 --- a/data/queue.py +++ b/data/queue.py @@ -77,9 +77,11 @@ class WorkQueue(object): ._available_jobs(now, name_match_query) .where(~(QueueItem.queue_name << running_query))) - def num_available_jobs(self, prefix): + def num_available_jobs(self, available_min_time, prefix): prefix = prefix.lstrip('/') - return self._available_jobs(datetime.utcnow(), self._name_match_query() + prefix).count() + available = self._available_jobs(datetime.utcnow(), + self._name_match_query() + prefix) + return available.where(QueueItem.available_after >= available_min_time).count() def _name_match_query(self): return '%s%%' % self._canonical_name([self._queue_name] + self._canonical_name_match_list) diff --git a/endpoints/building.py b/endpoints/building.py index c4aa98e6c..a94154efa 100644 --- a/endpoints/building.py +++ b/endpoints/building.py @@ -1,6 +1,8 @@ import logging import json +from datetime import datetime, timedelta + from flask import request from app import app, dockerfile_build_queue @@ -14,7 +16,8 @@ from util.morecollections import AttrDict logger = logging.getLogger(__name__) -MAX_BUILD_QUEUE_SIZE = app.config.get('MAX_BUILD_QUEUE_SIZE', -1) +MAX_BUILD_QUEUE_ITEMS = app.config.get('MAX_BUILD_QUEUE_ITEMS', -1) +MAX_BUILD_QUEUE_SECS = app.config.get('MAX_BUILD_QUEUE_SECS', -1) class MaximumBuildsQueuedException(Exception): @@ -23,9 +26,11 @@ class MaximumBuildsQueuedException(Exception): def start_build(repository, prepared_build, pull_robot_name=None): queue_item_prefix = '%s/%s' % repository.namespace_user.username, repository.name - available_builds = dockerfile_build_queue.num_available_jobs(queue_item_prefix) - if available_builds >= MAX_BUILD_QUEUE_SIZE and MAX_BUILD_QUEUE_SIZE > -1: - raise MaximumBuildsQueuedException() + if MAX_BUILD_QUEUE_ITEMS > 0 and MAX_BUILD_QUEUE_SECS > 0: + available_min = datetime.utcnow() - timedelta(seconds=-MAX_BUILD_QUEUE_SECS) + available_builds = dockerfile_build_queue.num_available_jobs(available_min, queue_item_prefix) + if available_builds > MAX_BUILD_QUEUE_ITEMS: + raise MaximumBuildsQueuedException() host = app.config['SERVER_HOSTNAME'] repo_path = '%s/%s/%s' % (host, repository.namespace_user.username, repository.name) From eb69abff8b0b24d3269d49aa15b11010bf3a8442 Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Tue, 6 Dec 2016 14:47:02 -0500 Subject: [PATCH 3/5] build rate limiting: tests --- data/queue.py | 10 +++++++--- endpoints/building.py | 9 ++++++--- test/test_queue.py | 19 ++++++++++++++++++- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/data/queue.py b/data/queue.py index 5cd84726d..659a485b0 100644 --- a/data/queue.py +++ b/data/queue.py @@ -77,10 +77,14 @@ class WorkQueue(object): ._available_jobs(now, name_match_query) .where(~(QueueItem.queue_name << running_query))) - def num_available_jobs(self, available_min_time, prefix): + def num_available_jobs_between(self, available_min_time, available_max_time, prefix): + """ + Returns the number of available queue items with a given prefix between the two provided times. + """ prefix = prefix.lstrip('/') - available = self._available_jobs(datetime.utcnow(), - self._name_match_query() + prefix) + available = self._available_jobs(available_max_time, + '/'.join([self._queue_name, prefix]) + '%') + return available.where(QueueItem.available_after >= available_min_time).count() def _name_match_query(self): diff --git a/endpoints/building.py b/endpoints/building.py index a94154efa..d2684c8b8 100644 --- a/endpoints/building.py +++ b/endpoints/building.py @@ -25,10 +25,13 @@ class MaximumBuildsQueuedException(Exception): def start_build(repository, prepared_build, pull_robot_name=None): - queue_item_prefix = '%s/%s' % repository.namespace_user.username, repository.name + queue_item_prefix = '%s/%s' % (repository.namespace_user.username, repository.name) if MAX_BUILD_QUEUE_ITEMS > 0 and MAX_BUILD_QUEUE_SECS > 0: - available_min = datetime.utcnow() - timedelta(seconds=-MAX_BUILD_QUEUE_SECS) - available_builds = dockerfile_build_queue.num_available_jobs(available_min, queue_item_prefix) + now = datetime.utcnow() + available_min = now - timedelta(seconds=MAX_BUILD_QUEUE_SECS) + available_builds = dockerfile_build_queue.num_available_jobs_between(available_min, + now, + queue_item_prefix) if available_builds > MAX_BUILD_QUEUE_ITEMS: raise MaximumBuildsQueuedException() diff --git a/test/test_queue.py b/test/test_queue.py index d31268850..0ac6cb633 100644 --- a/test/test_queue.py +++ b/test/test_queue.py @@ -2,6 +2,7 @@ import unittest import json import time +from datetime import datetime, timedelta from functools import wraps from app import app @@ -182,6 +183,22 @@ class TestQueue(QueueTestCase): self.assertIn(msg, seen) + def test_num_available_between(self): + now = datetime.utcnow() + self.queue.put(['abc', 'def'], self.TEST_MESSAGE_1, available_after=-10) + self.queue.put(['abc', 'ghi'], self.TEST_MESSAGE_2, available_after=-5) + + # Partial results + count = self.queue.num_available_jobs_between(now-timedelta(seconds=8), now, 'abc') + self.assertEqual(1, count) + + # All results + count = self.queue.num_available_jobs_between(now-timedelta(seconds=20), now, 'abc') + self.assertEqual(2, count) + + # No results + count = self.queue.num_available_jobs_between(now, now, 'abc') + self.assertEqual(0, count) + if __name__ == '__main__': unittest.main() - From c41de8ded6e866166640a09ac31a27ff6fed0fb8 Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Tue, 6 Dec 2016 20:40:54 -0500 Subject: [PATCH 4/5] build queue rate limiting: address PR comments --- data/queue.py | 2 +- endpoints/api/build.py | 1 + endpoints/building.py | 14 +++++++++----- endpoints/webhooks.py | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/data/queue.py b/data/queue.py index 659a485b0..07135b746 100644 --- a/data/queue.py +++ b/data/queue.py @@ -79,7 +79,7 @@ class WorkQueue(object): def num_available_jobs_between(self, available_min_time, available_max_time, prefix): """ - Returns the number of available queue items with a given prefix between the two provided times. + Returns the number of available queue items with a given prefix, between the two provided times. """ prefix = prefix.lstrip('/') available = self._available_jobs(available_max_time, diff --git a/endpoints/api/build.py b/endpoints/api/build.py index 36daf2bde..0b2ef5464 100644 --- a/endpoints/api/build.py +++ b/endpoints/api/build.py @@ -291,6 +291,7 @@ class RepositoryBuildList(RepositoryParamResource): build_request = start_build(repo, prepared, pull_robot_name=pull_robot_name) except MaximumBuildsQueuedException: abort(429, message='Maximum queued build rate exceeded.') + resp = build_status_view(build_request) repo_string = '%s/%s' % (namespace, repository) headers = { diff --git a/endpoints/building.py b/endpoints/building.py index d2684c8b8..83fd9e028 100644 --- a/endpoints/building.py +++ b/endpoints/building.py @@ -16,23 +16,27 @@ from util.morecollections import AttrDict logger = logging.getLogger(__name__) -MAX_BUILD_QUEUE_ITEMS = app.config.get('MAX_BUILD_QUEUE_ITEMS', -1) -MAX_BUILD_QUEUE_SECS = app.config.get('MAX_BUILD_QUEUE_SECS', -1) +MAX_BUILD_QUEUE_RATE_ITEMS = app.config.get('MAX_BUILD_QUEUE_RATE_ITEMS', -1) +MAX_BUILD_QUEUE_RATE_SECS = app.config.get('MAX_BUILD_QUEUE_RATE_SECS', -1) class MaximumBuildsQueuedException(Exception): + """ + This exception is raised when a build is requested, but the incoming build + would exceed the configured maximum build rate. + """ pass def start_build(repository, prepared_build, pull_robot_name=None): queue_item_prefix = '%s/%s' % (repository.namespace_user.username, repository.name) - if MAX_BUILD_QUEUE_ITEMS > 0 and MAX_BUILD_QUEUE_SECS > 0: + if MAX_BUILD_QUEUE_RATE_ITEMS > 0 and MAX_BUILD_QUEUE_RATE_SECS > 0: now = datetime.utcnow() - available_min = now - timedelta(seconds=MAX_BUILD_QUEUE_SECS) + available_min = now - timedelta(seconds=MAX_BUILD_QUEUE_RATE_SECS) available_builds = dockerfile_build_queue.num_available_jobs_between(available_min, now, queue_item_prefix) - if available_builds > MAX_BUILD_QUEUE_ITEMS: + if available_builds >= MAX_BUILD_QUEUE_RATE_ITEMS: raise MaximumBuildsQueuedException() host = app.config['SERVER_HOSTNAME'] diff --git a/endpoints/webhooks.py b/endpoints/webhooks.py index c89d9b26e..79a3e58d1 100644 --- a/endpoints/webhooks.py +++ b/endpoints/webhooks.py @@ -107,7 +107,7 @@ def build_trigger_webhook(trigger_uuid, **kwargs): try: start_build(repo, prepared, pull_robot_name=pull_robot_name) except MaximumBuildsQueuedException: - abort(429) + abort(429, message='Maximum queued build rate exceeded.') return make_response('Okay') From ebbe58d3114c8bfb6847e494b8d81128093302cd Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Wed, 7 Dec 2016 12:55:22 -0500 Subject: [PATCH 5/5] replace prefix w/ canonical name list --- data/queue.py | 9 ++++++--- endpoints/building.py | 4 ++-- test/test_queue.py | 4 ++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/data/queue.py b/data/queue.py index 07135b746..2f9321c92 100644 --- a/data/queue.py +++ b/data/queue.py @@ -77,13 +77,16 @@ class WorkQueue(object): ._available_jobs(now, name_match_query) .where(~(QueueItem.queue_name << running_query))) - def num_available_jobs_between(self, available_min_time, available_max_time, prefix): + def num_available_jobs_between(self, available_min_time, available_max_time, canonical_name_list): """ Returns the number of available queue items with a given prefix, between the two provided times. """ - prefix = prefix.lstrip('/') + def strip_slash(name): + return name.lstrip('/') + canonical_name_list = map(strip_slash, canonical_name_list) + available = self._available_jobs(available_max_time, - '/'.join([self._queue_name, prefix]) + '%') + '/'.join([self._queue_name] + canonical_name_list) + '%') return available.where(QueueItem.available_after >= available_min_time).count() diff --git a/endpoints/building.py b/endpoints/building.py index 83fd9e028..9ad61f8a1 100644 --- a/endpoints/building.py +++ b/endpoints/building.py @@ -29,13 +29,13 @@ class MaximumBuildsQueuedException(Exception): def start_build(repository, prepared_build, pull_robot_name=None): - queue_item_prefix = '%s/%s' % (repository.namespace_user.username, repository.name) if MAX_BUILD_QUEUE_RATE_ITEMS > 0 and MAX_BUILD_QUEUE_RATE_SECS > 0: + queue_item_canonical_name = [repository.namespace_user.username, repository.name] now = datetime.utcnow() available_min = now - timedelta(seconds=MAX_BUILD_QUEUE_RATE_SECS) available_builds = dockerfile_build_queue.num_available_jobs_between(available_min, now, - queue_item_prefix) + queue_item_canonical_name) if available_builds >= MAX_BUILD_QUEUE_RATE_ITEMS: raise MaximumBuildsQueuedException() diff --git a/test/test_queue.py b/test/test_queue.py index 0ac6cb633..4fd320964 100644 --- a/test/test_queue.py +++ b/test/test_queue.py @@ -189,11 +189,11 @@ class TestQueue(QueueTestCase): self.queue.put(['abc', 'ghi'], self.TEST_MESSAGE_2, available_after=-5) # Partial results - count = self.queue.num_available_jobs_between(now-timedelta(seconds=8), now, 'abc') + count = self.queue.num_available_jobs_between(now-timedelta(seconds=8), now, ['abc']) self.assertEqual(1, count) # All results - count = self.queue.num_available_jobs_between(now-timedelta(seconds=20), now, 'abc') + count = self.queue.num_available_jobs_between(now-timedelta(seconds=20), now, ['/abc']) self.assertEqual(2, count) # No results