From 4b0465f20de1bf58326c7daf6876b63438f00d84 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 12 Dec 2017 15:46:05 -0800 Subject: Address review comments with playing pipeline scheduler --- .../projects/pipeline_schedules_controller.rb | 30 +++++++++++----------- lib/gitlab/action_rate_limiter.rb | 18 ++++++++++++- .../projects/pipeline_schedules_controller_spec.rb | 20 ++++++++++++--- spec/lib/gitlab/action_rate_limiter_spec.rb | 6 +++-- 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/app/controllers/projects/pipeline_schedules_controller.rb b/app/controllers/projects/pipeline_schedules_controller.rb index dd6593650e7..b478e7b5e05 100644 --- a/app/controllers/projects/pipeline_schedules_controller.rb +++ b/app/controllers/projects/pipeline_schedules_controller.rb @@ -1,6 +1,7 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController before_action :schedule, except: [:index, :new, :create] + before_action :play_rate_limit, only: [:play] before_action :authorize_play_pipeline_schedule!, only: [:play] before_action :authorize_read_pipeline_schedule! before_action :authorize_create_pipeline_schedule!, only: [:new, :create] @@ -42,21 +43,13 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController end def play - limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule) - - if limiter.throttled?(throttle_key, 1) - flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' - return redirect_to pipeline_schedules_path(@project) - end - job_id = RunPipelineScheduleWorker.perform_async(schedule.id, current_user.id) - flash[:notice] = - if job_id - "Successfully scheduled a pipeline to run. Go to the Pipelines page for details.".html_safe - else - 'Unable to schedule a pipeline to run immediately' - end + if job_id + flash[:notice] = "Successfully scheduled a pipeline to run. Go to the Pipelines page for details.".html_safe + else + flash[:alert] = 'Unable to schedule a pipeline to run immediately' + end redirect_to pipeline_schedules_path(@project) end @@ -81,8 +74,15 @@ class Projects::PipelineSchedulesController < Projects::ApplicationController private - def throttle_key - "user:#{current_user.id}:schedule:#{schedule.id}" + def play_rate_limit + return unless current_user + + limiter = ::Gitlab::ActionRateLimiter.new(action: :play_pipeline_schedule) + + return unless limiter.throttled?([current_user, schedule], 1) + + flash[:alert] = 'You cannot play this scheduled pipeline at the moment. Please wait a minute.' + redirect_to pipeline_schedules_path(@project) end def schedule diff --git a/lib/gitlab/action_rate_limiter.rb b/lib/gitlab/action_rate_limiter.rb index 53add503c60..4cd3bdefda3 100644 --- a/lib/gitlab/action_rate_limiter.rb +++ b/lib/gitlab/action_rate_limiter.rb @@ -12,11 +12,15 @@ module Gitlab @expiry_time = expiry_time end + # Increments the given cache key and increments the value by 1 with the + # given expiration time. Returns the incremented value. + # + # key - An array of ActiveRecord instances def increment(key) value = 0 Gitlab::Redis::Cache.with do |redis| - cache_key = "action_rate_limiter:#{action}:#{key}" + cache_key = action_key(key) value = redis.incr(cache_key) redis.expire(cache_key, expiry_time) if value == 1 end @@ -24,8 +28,20 @@ module Gitlab value end + # Increments the given key and returns true if the action should + # be throttled. + # + # key - An array of ActiveRecord instances + # threshold_value - The maximum number of times this action should occur in the given time interval def throttled?(key, threshold_value) self.increment(key) > threshold_value end + + private + + def action_key(key) + serialized = key.map { |obj| "#{obj.class.model_name.to_s.underscore}:#{obj.id}" }.join(":") + "action_rate_limiter:#{action}:#{serialized}" + end end end diff --git a/spec/controllers/projects/pipeline_schedules_controller_spec.rb b/spec/controllers/projects/pipeline_schedules_controller_spec.rb index ffc1259eb8f..966ffdf6996 100644 --- a/spec/controllers/projects/pipeline_schedules_controller_spec.rb +++ b/spec/controllers/projects/pipeline_schedules_controller_spec.rb @@ -370,13 +370,27 @@ describe Projects::PipelineSchedulesController do set(:user) { create(:user) } let(:ref) { 'master' } - context 'when a developer makes the request' do + before do + project.add_developer(user) + + sign_in(user) + end + + context 'when an anonymous user makes the request' do before do - project.add_developer(user) + sign_out(user) + end - sign_in(user) + it 'does not allow pipeline to be executed' do + expect(RunPipelineScheduleWorker).not_to receive(:perform_async) + + post :play, namespace_id: project.namespace.to_param, project_id: project, id: pipeline_schedule.id + + expect(response).to have_gitlab_http_status(404) end + end + context 'when a developer makes the request' do it 'executes a new pipeline' do expect(RunPipelineScheduleWorker).to receive(:perform_async).with(pipeline_schedule.id, user.id).and_return('job-123') diff --git a/spec/lib/gitlab/action_rate_limiter_spec.rb b/spec/lib/gitlab/action_rate_limiter_spec.rb index 84661605623..542fc03e555 100644 --- a/spec/lib/gitlab/action_rate_limiter_spec.rb +++ b/spec/lib/gitlab/action_rate_limiter_spec.rb @@ -2,8 +2,10 @@ require 'spec_helper' describe Gitlab::ActionRateLimiter do let(:redis) { double('redis') } - let(:key) { 'user:1' } - let(:cache_key) { "action_rate_limiter:test_action:#{key}" } + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:key) { [user, project] } + let(:cache_key) { "action_rate_limiter:test_action:user:#{user.id}:project:#{project.id}" } subject { described_class.new(action: :test_action, expiry_time: 100) } -- cgit v1.2.1