diff options
Diffstat (limited to 'spec/services/ci/runners')
-rw-r--r-- | spec/services/ci/runners/bulk_delete_runners_service_spec.rb | 170 | ||||
-rw-r--r-- | spec/services/ci/runners/set_runner_associated_projects_service_spec.rb | 13 |
2 files changed, 149 insertions, 34 deletions
diff --git a/spec/services/ci/runners/bulk_delete_runners_service_spec.rb b/spec/services/ci/runners/bulk_delete_runners_service_spec.rb index 8e9fc4e3012..fa8af1100df 100644 --- a/spec/services/ci/runners/bulk_delete_runners_service_spec.rb +++ b/spec/services/ci/runners/bulk_delete_runners_service_spec.rb @@ -5,78 +5,180 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::BulkDeleteRunnersService, '#execute' do subject(:execute) { described_class.new(**service_args).execute } - let(:service_args) { { runners: runners_arg } } + let_it_be(:admin_user) { create(:user, :admin) } + let_it_be_with_refind(:owner_user) { create(:user) } # discard memoized ci_owned_runners + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + + let(:user) {} + let(:service_args) { { runners: runners_arg, current_user: user } } let(:runners_arg) {} context 'with runners specified' do let!(:instance_runner) { create(:ci_runner) } - let!(:group_runner) { create(:ci_runner, :group) } - let!(:project_runner) { create(:ci_runner, :project) } + let!(:group_runner) { create(:ci_runner, :group, groups: [group]) } + let!(:project_runner) { create(:ci_runner, :project, projects: [project]) } shared_examples 'a service deleting runners in bulk' do + let!(:expected_deleted_ids) { expected_deleted_runners.map(&:id) } + it 'destroys runners', :aggregate_failures do - expect { subject }.to change { Ci::Runner.count }.by(-2) + expect { execute }.to change { Ci::Runner.count }.by(-expected_deleted_ids.count) is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 2, deleted_ids: [instance_runner.id, project_runner.id] }) - expect(instance_runner[:errors]).to be_nil - expect(project_runner[:errors]).to be_nil + expect(execute.payload).to eq( + { + deleted_count: expected_deleted_ids.count, + deleted_ids: expected_deleted_ids, + errors: [] + }) expect { project_runner.runner_projects.first.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect { group_runner.reload }.not_to raise_error - expect { instance_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect { project_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) + expected_deleted_runners.each do |deleted_runner| + expect(deleted_runner[:errors]).to be_nil + expect { deleted_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) + end end - context 'with some runners already deleted' do + context 'with too many runners specified' do before do - instance_runner.destroy! + stub_const("#{described_class}::RUNNER_LIMIT", 1) end - let(:runners_arg) { [instance_runner.id, project_runner.id] } - - it 'destroys runners and returns only deleted runners', :aggregate_failures do - expect { subject }.to change { Ci::Runner.count }.by(-1) + it 'deletes only first RUNNER_LIMIT runners', :aggregate_failures do + expect { execute }.to change { Ci::Runner.count }.by(-1) is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 1, deleted_ids: [project_runner.id] }) - expect(instance_runner[:errors]).to be_nil - expect(project_runner[:errors]).to be_nil - expect { project_runner.reload }.to raise_error(ActiveRecord::RecordNotFound) + expect(execute.payload).to eq( + { + deleted_count: 1, + deleted_ids: expected_deleted_ids.take(1), + errors: ["Can only delete up to 1 runners per call. Ignored the remaining runner(s)."] + }) end end + end - context 'with too many runners specified' do + context 'when the user cannot delete runners' do + let(:runners_arg) { Ci::Runner.all } + + context 'when user is not group owner' do before do - stub_const("#{described_class}::RUNNER_LIMIT", 1) + group.add_developer(user) end - it 'deletes only first RUNNER_LIMIT runners' do - expect { subject }.to change { Ci::Runner.count }.by(-1) + let(:user) { create(:user) } - is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 1, deleted_ids: [instance_runner.id] }) + it 'does not delete any runner and returns error', :aggregate_failures do + expect { execute }.not_to change { Ci::Runner.count } + expect(execute[:errors]).to match_array("User does not have permission to delete any of the runners") end end - end - context 'with runners specified as relation' do - let(:runners_arg) { Ci::Runner.not_group_type } + context 'when user is not part of the group' do + let(:user) { create(:user) } - include_examples 'a service deleting runners in bulk' + it 'does not delete any runner and returns error', :aggregate_failures do + expect { execute }.not_to change { Ci::Runner.count } + expect(execute[:errors]).to match_array("User does not have permission to delete any of the runners") + end + end end - context 'with runners specified as array of IDs' do - let(:runners_arg) { Ci::Runner.not_group_type.ids } + context 'when the user can delete runners' do + context 'when user is an admin', :enable_admin_mode do + include_examples 'a service deleting runners in bulk' do + let(:runners_arg) { Ci::Runner.all } + let!(:expected_deleted_runners) { [instance_runner, group_runner, project_runner] } + let(:user) { admin_user } + end + + context 'with a runner already deleted' do + before do + group_runner.destroy! + end + + include_examples 'a service deleting runners in bulk' do + let(:runners_arg) { Ci::Runner.all } + let!(:expected_deleted_runners) { [instance_runner, project_runner] } + let(:user) { admin_user } + end + end + + context 'when deleting a single runner' do + let(:runners_arg) { Ci::Runner.all } + + it 'avoids N+1 cached queries', :use_sql_query_cache, :request_store do + # Run this once to establish a baseline + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) do + execute + end + + additional_runners = 1 + + create_list(:ci_runner, 1 + additional_runners, :instance) + create_list(:ci_runner, 1 + additional_runners, :group, groups: [group]) + create_list(:ci_runner, 1 + additional_runners, :project, projects: [project]) + + service = described_class.new(runners: runners_arg, current_user: user) + + # Base cost per runner is: + # - 1 `SELECT * FROM "taggings"` query + # - 1 `SAVEPOINT` query + # - 1 `DELETE FROM "ci_runners"` query + # - 1 `RELEASE SAVEPOINT` query + # Project runners have an additional query: + # - 1 `DELETE FROM "ci_runner_projects"` query, given the call to `destroy_all` + instance_runner_cost = 4 + group_runner_cost = 4 + project_runner_cost = 5 + expect { service.execute } + .not_to exceed_all_query_limit(control_count) + .with_threshold(additional_runners * (instance_runner_cost + group_runner_cost + project_runner_cost)) + end + end + end - include_examples 'a service deleting runners in bulk' + context 'when user is group owner' do + before do + group.add_owner(user) + end + + include_examples 'a service deleting runners in bulk' do + let(:runners_arg) { Ci::Runner.not_instance_type } + let!(:expected_deleted_runners) { [group_runner, project_runner] } + let(:user) { owner_user } + end + + context 'with a runner non-authorised to be deleted' do + let(:runners_arg) { Ci::Runner.all } + let!(:expected_deleted_runners) { [project_runner] } + let(:user) { owner_user } + + it 'destroys only authorised runners', :aggregate_failures do + allow(Ability).to receive(:allowed?).and_call_original + expect(Ability).to receive(:allowed?).with(user, :delete_runner, instance_runner).and_return(false) + + expect { execute }.to change { Ci::Runner.count }.by(-2) + + is_expected.to be_success + expect(execute.payload).to eq( + { + deleted_count: 2, + deleted_ids: [group_runner.id, project_runner.id], + errors: ["User does not have permission to delete runner(s) ##{instance_runner.id}"] + }) + end + end + end end context 'with no arguments specified' do let(:runners_arg) { nil } + let(:user) { owner_user } it 'returns 0 deleted runners' do is_expected.to be_success - expect(execute.payload).to eq({ deleted_count: 0, deleted_ids: [] }) + expect(execute.payload).to eq({ deleted_count: 0, deleted_ids: [], errors: [] }) end end end diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index 1f44612947b..e5cba80d567 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -67,6 +67,19 @@ RSpec.describe ::Ci::Runners::SetRunnerAssociatedProjectsService, '#execute' do expect(runner.projects.ids).to match_array([owner_project.id] + project_ids) end end + + context 'when disassociating all projects' do + let(:project_ids) { [] } + + it 'reassigns associated projects and returns success response' do + expect(execute).to be_success + + runner.reload + + expect(runner.owner_project).to eq(owner_project) + expect(runner.projects.ids).to contain_exactly(owner_project.id) + end + end end context 'with failing assign_to requests' do |