From c35eec0615b42206e5eb2213152efb48aebe7c8a Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 17 Oct 2017 17:01:59 -0400 Subject: [PATCH] Add ability for triggers to be disabled Will be used in the followup commit to automatically disable broken triggers --- data/database.py | 6 ++ ...c_add_ability_for_build_triggers_to_be_.py | 56 ++++++++++++++ ...463dfb9fe_back_fill_build_expand_config.py | 6 +- data/model/build.py | 13 +++- endpoints/api/build.py | 7 +- endpoints/api/test/test_security.py | 22 +++++- endpoints/api/test/test_trigger.py | 33 +++++++++ endpoints/api/trigger.py | 44 ++++++++++- endpoints/building.py | 12 +++ endpoints/test/test_building.py | 14 +++- endpoints/test/test_webhooks.py | 24 ++++++ endpoints/webhooks.py | 10 ++- initdb.py | 5 +- .../repo-view/repo-panel-builds.css | 20 ++++- .../repo-view/repo-panel-builds.html | 73 ++++++++++++------- .../directives/repo-view/repo-panel-builds.js | 38 ++++++++++ static/js/directives/ui/logs-view.js | 10 +++ test/fixtures.py | 2 + 18 files changed, 358 insertions(+), 37 deletions(-) create mode 100644 data/migrations/versions/61cadbacb9fc_add_ability_for_build_triggers_to_be_.py create mode 100644 endpoints/test/test_webhooks.py diff --git a/data/database.py b/data/database.py index fa8fa88f8..6e41a9307 100644 --- a/data/database.py +++ b/data/database.py @@ -694,6 +694,10 @@ class BuildTriggerService(BaseModel): name = CharField(index=True, unique=True) +class DisableReason(BaseModel): + name = CharField(index=True, unique=True) + + class RepositoryBuildTrigger(BaseModel): uuid = CharField(default=uuid_generator) service = ForeignKeyField(BuildTriggerService) @@ -705,6 +709,8 @@ class RepositoryBuildTrigger(BaseModel): write_token = ForeignKeyField(AccessToken, null=True) pull_robot = QuayUserField(allows_robots=True, null=True, related_name='triggerpullrobot', robot_null_delete=True) + enabled = BooleanField(default=True) + disabled_reason = ForeignKeyField(DisableReason, null=True) class EmailConfirmation(BaseModel): diff --git a/data/migrations/versions/61cadbacb9fc_add_ability_for_build_triggers_to_be_.py b/data/migrations/versions/61cadbacb9fc_add_ability_for_build_triggers_to_be_.py new file mode 100644 index 000000000..0ff76ff08 --- /dev/null +++ b/data/migrations/versions/61cadbacb9fc_add_ability_for_build_triggers_to_be_.py @@ -0,0 +1,56 @@ +"""Add ability for build triggers to be disabled + +Revision ID: 61cadbacb9fc +Revises: d8989249f8f6 +Create Date: 2017-10-18 12:07:26.190901 + +""" + +# revision identifiers, used by Alembic. +revision = '61cadbacb9fc' +down_revision = 'd8989249f8f6' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import mysql + +def upgrade(tables): + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('disablereason', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('name', sa.String(length=255), nullable=False), + sa.PrimaryKeyConstraint('id', name=op.f('pk_disablereason')) + ) + op.create_index('disablereason_name', 'disablereason', ['name'], unique=True) + + op.bulk_insert( + tables.disablereason, + [ + {'id': 1, 'name': 'user_toggled'}, + ], + ) + + op.bulk_insert(tables.logentrykind, [ + {'name': 'toggle_repo_trigger'}, + ]) + + op.add_column(u'repositorybuildtrigger', sa.Column('disabled_reason_id', sa.Integer(), nullable=True)) + op.add_column(u'repositorybuildtrigger', sa.Column('enabled', sa.Boolean(), nullable=False)) + op.create_index('repositorybuildtrigger_disabled_reason_id', 'repositorybuildtrigger', ['disabled_reason_id'], unique=False) + op.create_foreign_key(op.f('fk_repositorybuildtrigger_disabled_reason_id_disablereason'), 'repositorybuildtrigger', 'disablereason', ['disabled_reason_id'], ['id']) + # ### end Alembic commands ### + + +def downgrade(tables): + # ### commands auto generated by Alembic - please adjust! ### + op.drop_constraint(op.f('fk_repositorybuildtrigger_disabled_reason_id_disablereason'), 'repositorybuildtrigger', type_='foreignkey') + op.drop_index('repositorybuildtrigger_disabled_reason_id', table_name='repositorybuildtrigger') + op.drop_column(u'repositorybuildtrigger', 'enabled') + op.drop_column(u'repositorybuildtrigger', 'disabled_reason_id') + op.drop_table('disablereason') + # ### end Alembic commands ### + + op.execute(tables + .logentrykind + .delete() + .where(tables.logentrykind.c.name == op.inline_literal('toggle_repo_trigger'))) diff --git a/data/migrations/versions/a6c463dfb9fe_back_fill_build_expand_config.py b/data/migrations/versions/a6c463dfb9fe_back_fill_build_expand_config.py index e682da932..425a20ad2 100644 --- a/data/migrations/versions/a6c463dfb9fe_back_fill_build_expand_config.py +++ b/data/migrations/versions/a6c463dfb9fe_back_fill_build_expand_config.py @@ -10,7 +10,8 @@ Create Date: 2017-03-17 10:00:19.739858 import json import os -from data.database import RepositoryBuildTrigger +from peewee import * +from data.database import BaseModel revision = 'a6c463dfb9fe' down_revision = 'b4df55dea4b3' @@ -18,6 +19,9 @@ down_revision = 'b4df55dea4b3' from alembic import op +class RepositoryBuildTrigger(BaseModel): + config = TextField(default='{}') + def upgrade(tables): repostioryBuildTriggers = RepositoryBuildTrigger.select() for repositoryBuildTrigger in repostioryBuildTriggers: diff --git a/data/model/build.py b/data/model/build.py index 73685aa1a..5d491389a 100644 --- a/data/model/build.py +++ b/data/model/build.py @@ -6,7 +6,8 @@ from peewee import JOIN_LEFT_OUTER import features from data.database import (BuildTriggerService, RepositoryBuildTrigger, Repository, Namespace, User, - RepositoryBuild, BUILD_PHASE, db_random_func, UseThenDisconnect) + RepositoryBuild, BUILD_PHASE, db_random_func, UseThenDisconnect, + DisableReason) from data.model import (InvalidBuildTriggerException, InvalidRepositoryBuildException, db_transaction, user as user_model, config) @@ -255,3 +256,13 @@ def mark_build_archived(build_uuid): .where(RepositoryBuild.uuid == build_uuid, RepositoryBuild.logs_archived == False) .execute()) > 0 + + +def toggle_build_trigger(trigger, enabled, reason='user_toggled'): + """ Toggles the enabled status of a build trigger. """ + trigger.enabled = enabled + + if not enabled: + trigger.disabled_reason = DisableReason.get(name=reason) + + trigger.save() diff --git a/endpoints/api/build.py b/endpoints/api/build.py index 5645236e6..86190fae8 100644 --- a/endpoints/api/build.py +++ b/endpoints/api/build.py @@ -22,7 +22,8 @@ from endpoints.api import (RepositoryParamResource, parse_args, query_param, nic require_repo_read, require_repo_write, validate_json_request, ApiResource, internal_only, format_date, api, path_param, require_repo_admin, abort, disallow_for_app_repositories) -from endpoints.building import start_build, PreparedBuild, MaximumBuildsQueuedException +from endpoints.building import (start_build, PreparedBuild, MaximumBuildsQueuedException, + BuildTriggerDisabledException) from endpoints.exception import Unauthorized, NotFound, InvalidRequest from util.names import parse_robot_username @@ -69,6 +70,8 @@ def trigger_view(trigger, can_read=False, can_admin=False, for_build=False): 'config': build_trigger.config if can_admin else {}, 'can_invoke': can_admin, + 'enabled': trigger.enabled, + 'disabled_reason': trigger.disabled_reason.name if trigger.disabled_reason else None, } if not for_build and can_admin and trigger.pull_robot: @@ -309,6 +312,8 @@ 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.') + except BuildTriggerDisabledException: + abort(400, message='Build trigger is disabled') resp = build_status_view(build_request) repo_string = '%s/%s' % (namespace, repository) diff --git a/endpoints/api/test/test_security.py b/endpoints/api/test/test_security.py index b701f7856..9d24b0367 100644 --- a/endpoints/api/test/test_security.py +++ b/endpoints/api/test/test_security.py @@ -13,7 +13,11 @@ from endpoints.api.signing import RepositorySignatures from endpoints.api.search import ConductRepositorySearch from endpoints.api.superuser import SuperUserRepositoryBuildLogs, SuperUserRepositoryBuildResource from endpoints.api.superuser import SuperUserRepositoryBuildStatus +<<<<<<< HEAD from endpoints.api.appspecifictokens import AppTokens, AppToken +======= +from endpoints.api.trigger import BuildTrigger +>>>>>>> Add ability for triggers to be disabled from endpoints.test.shared import client_with_identity, toggle_feature from test.fixtures import * @@ -24,6 +28,7 @@ REPO_PARAMS = {'repository': 'devtable/someapp'} SEARCH_PARAMS = {'query': ''} NOTIFICATION_PARAMS = {'namespace': 'devtable', 'repository': 'devtable/simple', 'uuid': 'some uuid'} TOKEN_PARAMS = {'token_uuid': 'someuuid'} +TRIGGER_PARAMS = {'repository': 'devtable/simple', 'trigger_uuid': 'someuuid'} @pytest.mark.parametrize('resource,method,params,body,identity,expected', [ (AppTokens, 'GET', {}, {}, None, 401), @@ -89,7 +94,22 @@ TOKEN_PARAMS = {'token_uuid': 'someuuid'} (RepositoryTrust, 'POST', REPO_PARAMS, {'trust_enabled': True}, 'freshuser', 403), (RepositoryTrust, 'POST', REPO_PARAMS, {'trust_enabled': True}, 'reader', 403), (RepositoryTrust, 'POST', REPO_PARAMS, {'trust_enabled': True}, 'devtable', 404), - + + (BuildTrigger, 'GET', TRIGGER_PARAMS, {}, None, 401), + (BuildTrigger, 'GET', TRIGGER_PARAMS, {}, 'freshuser', 403), + (BuildTrigger, 'GET', TRIGGER_PARAMS, {}, 'reader', 403), + (BuildTrigger, 'GET', TRIGGER_PARAMS, {}, 'devtable', 404), + + (BuildTrigger, 'DELETE', TRIGGER_PARAMS, {}, None, 403), + (BuildTrigger, 'DELETE', TRIGGER_PARAMS, {}, 'freshuser', 403), + (BuildTrigger, 'DELETE', TRIGGER_PARAMS, {}, 'reader', 403), + (BuildTrigger, 'DELETE', TRIGGER_PARAMS, {}, 'devtable', 404), + + (BuildTrigger, 'PUT', TRIGGER_PARAMS, {}, None, 403), + (BuildTrigger, 'PUT', TRIGGER_PARAMS, {}, 'freshuser', 403), + (BuildTrigger, 'PUT', TRIGGER_PARAMS, {}, 'reader', 403), + (BuildTrigger, 'PUT', TRIGGER_PARAMS, {}, 'devtable', 400), + (RepositoryUserTransitivePermission, 'GET', {'username': 'A2O9','repository': 'public/publicrepo'}, None, None, 401), (RepositoryUserTransitivePermission, 'GET', {'username': 'A2O9','repository': 'public/publicrepo'}, None, 'freshuser', 403), (RepositoryUserTransitivePermission, 'GET', {'username': 'A2O9','repository': 'public/publicrepo'}, None, 'reader', 403), diff --git a/endpoints/api/test/test_trigger.py b/endpoints/api/test/test_trigger.py index 32086ffbf..946b34431 100644 --- a/endpoints/api/test/test_trigger.py +++ b/endpoints/api/test/test_trigger.py @@ -1,6 +1,12 @@ import pytest +import json +from data import model from endpoints.api.trigger_analyzer import is_parent +from endpoints.api.trigger import BuildTrigger +from endpoints.api.test.shared import conduct_api_call +from endpoints.test.shared import client_with_identity +from test.fixtures import * @pytest.mark.parametrize('context,dockerfile_path,expected', [ @@ -20,3 +26,30 @@ from endpoints.api.trigger_analyzer import is_parent ]) def test_super_user_build_endpoints(context, dockerfile_path, expected): assert is_parent(context, dockerfile_path) == expected + + +def test_enabled_disabled_trigger(app, client): + trigger = model.build.list_build_triggers('devtable', 'building')[0] + trigger.config = json.dumps({'hook_id': 'someid'}) + trigger.save() + + params = { + 'repository': 'devtable/building', + 'trigger_uuid': trigger.uuid, + } + + body = { + 'enabled': False, + } + + with client_with_identity('devtable', client) as cl: + result = conduct_api_call(cl, BuildTrigger, 'PUT', params, body, 200).json + assert not result['enabled'] + + body = { + 'enabled': True, + } + + with client_with_identity('devtable', client) as cl: + result = conduct_api_call(cl, BuildTrigger, 'PUT', params, body, 200).json + assert result['enabled'] diff --git a/endpoints/api/trigger.py b/endpoints/api/trigger.py index 79c3cced9..8fee8505a 100644 --- a/endpoints/api/trigger.py +++ b/endpoints/api/trigger.py @@ -22,7 +22,8 @@ from endpoints.api import (RepositoryParamResource, nickname, resource, require_ disallow_for_app_repositories) from endpoints.api.build import build_status_view, trigger_view, RepositoryBuildStatus from endpoints.api.trigger_analyzer import TriggerAnalyzer -from endpoints.building import start_build, MaximumBuildsQueuedException +from endpoints.building import (start_build, MaximumBuildsQueuedException, + BuildTriggerDisabledException) from endpoints.exception import NotFound, Unauthorized, InvalidRequest from util.names import parse_robot_username @@ -62,6 +63,21 @@ class BuildTriggerList(RepositoryParamResource): @path_param('trigger_uuid', 'The UUID of the build trigger') class BuildTrigger(RepositoryParamResource): """ Resource for managing specific build triggers. """ + schemas = { + 'UpdateTrigger': { + 'type': 'object', + 'description': 'Options for updating a build trigger', + 'required': [ + 'enabled', + ], + 'properties': { + 'enabled': { + 'type': 'boolean', + 'description': 'Whether the build trigger is enabled', + }, + } + }, + } @require_repo_admin @disallow_for_app_repositories @@ -70,6 +86,27 @@ class BuildTrigger(RepositoryParamResource): """ Get information for the specified build trigger. """ return trigger_view(get_trigger(trigger_uuid), can_admin=True) + @require_repo_admin + @disallow_for_app_repositories + @nickname('updateBuildTrigger') + @validate_json_request('UpdateTrigger') + def put(self, namespace_name, repo_name, trigger_uuid): + """ Updates the specified build trigger. """ + trigger = get_trigger(trigger_uuid) + + handler = BuildTriggerHandler.get_handler(trigger) + if not handler.is_active(): + raise InvalidRequest('Cannot update an unactivated trigger') + + enable = request.get_json()['enabled'] + model.build.toggle_build_trigger(trigger, enable) + log_action('toggle_repo_trigger', namespace_name, + {'repo': repo_name, 'trigger_id': trigger_uuid, + 'service': trigger.service.name, 'enabled': enable}, + repo=model.repository.get_repository(namespace_name, repo_name)) + + return trigger_view(trigger) + @require_repo_admin @disallow_for_app_repositories @nickname('deleteBuildTrigger') @@ -340,6 +377,8 @@ class ActivateBuildTrigger(RepositoryParamResource): def post(self, namespace_name, repo_name, trigger_uuid): """ Manually start a build from the specified trigger. """ trigger = get_trigger(trigger_uuid) + if not trigger.enabled: + raise InvalidRequest('Trigger is not enabled.') handler = BuildTriggerHandler.get_handler(trigger) if not handler.is_active(): @@ -356,6 +395,8 @@ class ActivateBuildTrigger(RepositoryParamResource): raise InvalidRequest(tse.message) except MaximumBuildsQueuedException: abort(429, message='Maximum queued build rate exceeded.') + except BuildTriggerDisabledException: + abort(400, message='Build trigger is disabled') resp = build_status_view(build_request) repo_string = '%s/%s' % (namespace_name, repo_name) @@ -485,3 +526,4 @@ class BuildTriggerSourceNamespaces(RepositoryParamResource): raise InvalidRequest(rre.message) else: raise Unauthorized() + diff --git a/endpoints/building.py b/endpoints/building.py index 071f261a5..05dfd64f9 100644 --- a/endpoints/building.py +++ b/endpoints/building.py @@ -25,10 +25,22 @@ class MaximumBuildsQueuedException(Exception): pass +class BuildTriggerDisabledException(Exception): + """ + This exception is raised when a build is required, but the build trigger has been disabled. + """ + pass + + def start_build(repository, prepared_build, pull_robot_name=None): + # Ensure that builds are only run in image repositories. if repository.kind.name != 'image': raise Exception('Attempt to start a build for application repository %s' % repository.id) + # Ensure that disabled triggers are not run. + if prepared_build.trigger is not None and not prepared_build.trigger.enabled: + raise BuildTriggerDisabledException + if repository.namespace_user.maximum_queued_builds_count is not None: queue_item_canonical_name = [repository.namespace_user.username] alive_builds = dockerfile_build_queue.num_alive_jobs(queue_item_canonical_name) diff --git a/endpoints/test/test_building.py b/endpoints/test/test_building.py index 079bb8ca8..8058517ea 100644 --- a/endpoints/test/test_building.py +++ b/endpoints/test/test_building.py @@ -1,7 +1,8 @@ import pytest from data import model -from endpoints.building import start_build, PreparedBuild, MaximumBuildsQueuedException +from endpoints.building import (start_build, PreparedBuild, MaximumBuildsQueuedException, + BuildTriggerDisabledException) from test.fixtures import * @@ -29,3 +30,14 @@ def test_maximum_builds(app): # Try to queue a second build; should fail. with pytest.raises(MaximumBuildsQueuedException): start_build(repo, prepared_build) + + +def test_start_build_disabled_trigger(app): + trigger = model.build.list_build_triggers('devtable', 'building')[0] + trigger.enabled = False + trigger.save() + + build = PreparedBuild(trigger=trigger) + + with pytest.raises(BuildTriggerDisabledException): + start_build(trigger.repository, build) diff --git a/endpoints/test/test_webhooks.py b/endpoints/test/test_webhooks.py new file mode 100644 index 000000000..1061f106b --- /dev/null +++ b/endpoints/test/test_webhooks.py @@ -0,0 +1,24 @@ +import base64 +import pytest + +from flask import url_for + +from data import model +from endpoints.test.shared import conduct_call +from test.fixtures import * + +def test_start_build_disabled_trigger(app, client): + trigger = model.build.list_build_triggers('devtable', 'building')[0] + trigger.enabled = False + trigger.save() + + params = { + 'trigger_uuid': trigger.uuid, + } + + headers = { + 'Authorization': 'Basic ' + base64.b64encode('devtable:password'), + } + + conduct_call(client, 'webhooks.build_trigger_webhook', url_for, 'POST', params, None, 400, + headers=headers) diff --git a/endpoints/webhooks.py b/endpoints/webhooks.py index 8b65e6b69..48e5d3752 100644 --- a/endpoints/webhooks.py +++ b/endpoints/webhooks.py @@ -12,7 +12,8 @@ from util.http import abort from buildtrigger.basehandler import BuildTriggerHandler from buildtrigger.triggerutil import (ValidationRequestException, SkipRequestException, InvalidPayloadException) -from endpoints.building import start_build, MaximumBuildsQueuedException +from endpoints.building import (start_build, MaximumBuildsQueuedException, + BuildTriggerDisabledException) logger = logging.getLogger(__name__) @@ -91,9 +92,7 @@ def build_trigger_webhook(trigger_uuid, **kwargs): namespace = trigger.repository.namespace_user.username repository = trigger.repository.name - permission = ModifyRepositoryPermission(namespace, repository) - - if permission.can(): + if ModifyRepositoryPermission(namespace, repository).can(): handler = BuildTriggerHandler.get_handler(trigger) if trigger.repository.kind.name != 'image': @@ -121,6 +120,9 @@ def build_trigger_webhook(trigger_uuid, **kwargs): start_build(repo, prepared, pull_robot_name=pull_robot_name) except MaximumBuildsQueuedException: abort(429, message='Maximum queued build rate exceeded.') + except BuildTriggerDisabledException: + logger.debug('Build trigger %s is disabled', trigger_uuid) + abort(400, message='This build trigger is currently disabled. Please re-enable to continue.') return make_response('Okay') diff --git a/initdb.py b/initdb.py index aecdddfb7..e42d599e4 100644 --- a/initdb.py +++ b/initdb.py @@ -20,7 +20,7 @@ from data.database import (db, all_models, beta_classes, Role, TeamRole, Visibil ExternalNotificationEvent, ExternalNotificationMethod, NotificationKind, QuayRegion, QuayService, UserRegion, OAuthAuthorizationCode, ServiceKeyApprovalType, MediaType, LabelSourceType, UserPromptKind, - RepositoryKind, TagKind, BlobPlacementLocation, User, + RepositoryKind, TagKind, BlobPlacementLocation, User, DisableReason, DeletedNamespace) from data import model from data.queue import WorkQueue @@ -353,6 +353,7 @@ def initialize_database(): LogEntryKind.create(name='manifest_label_delete') LogEntryKind.create(name='change_tag_expiration') + LogEntryKind.create(name='toggle_repo_trigger') LogEntryKind.create(name='create_app_specific_token') LogEntryKind.create(name='revoke_app_specific_token') @@ -434,6 +435,8 @@ def initialize_database(): TagKind.create(name='release') TagKind.create(name='channel') + DisableReason.create(name='user_toggled') + def wipe_database(): logger.debug('Wiping all data from the DB.') diff --git a/static/css/directives/repo-view/repo-panel-builds.css b/static/css/directives/repo-view/repo-panel-builds.css index ec4c9dd3c..c50f5db45 100644 --- a/static/css/directives/repo-view/repo-panel-builds.css +++ b/static/css/directives/repo-view/repo-panel-builds.css @@ -13,4 +13,22 @@ .repo-panel-builds .heading-controls { white-space: nowrap; -} \ No newline at end of file +} + +.repo-panel-builds .trigger-disabled { + background-color: #fcfcfc; +} + +.repo-panel-builds .trigger-disabled td { + border-bottom: 0px; + color: #ccc; +} + +.repo-panel-builds .trigger-disabled-message { + font-size: 13px; +} + +.repo-panel-builds i.fa-exclamation-triangle { + color: #f5c77d; + margin-right: 4px; +} diff --git a/static/directives/repo-view/repo-panel-builds.html b/static/directives/repo-view/repo-panel-builds.html index 5653a92eb..1e7577ffc 100644 --- a/static/directives/repo-view/repo-panel-builds.html +++ b/static/directives/repo-view/repo-panel-builds.html @@ -137,31 +137,45 @@ Delete Trigger - - - - {{ trigger.config.dockerfile_path || '/Dockerfile' }} - {{ trigger.config.context || '/' }} - {{ trigger.config.branchtag_regex || 'All' }} - - - (None) - - - - - View Credentials + + + + + {{ trigger.config.dockerfile_path || '/Dockerfile' }} + {{ trigger.config.context || '/' }} + {{ trigger.config.branchtag_regex || 'All' }} + + + (None) + + + + + View Credentials + + + Run Trigger Now + + + + Disable Trigger + Enable Trigger + + + Delete Trigger + - - Run Trigger Now - - - Delete Trigger - - - - + + + + + + This build trigger is currently disabled and will not build: + Re-enable this trigger + + + @@ -174,7 +188,16 @@
- + +
+ Are you sure you want to disableenable this trigger? +
+ +