diff options
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 8 | ||||
| -rw-r--r-- | app/models/ci/runner.rb | 7 | ||||
| -rw-r--r-- | spec/models/ci/runner_spec.rb | 14 | ||||
| -rw-r--r-- | spec/requests/api/ci/runner/runners_post_spec.rb | 50 | ||||
| -rw-r--r-- | spec/requests/api/ci/runners_spec.rb | 13 |
6 files changed, 86 insertions, 8 deletions
@@ -67,7 +67,7 @@ gem 'akismet', '~> 3.0' gem 'invisible_captcha', '~> 1.1.0' # Two-factor authentication -gem 'devise-two-factor', '~> 4.0.0' +gem 'devise-two-factor', '~> 4.0.2' gem 'rqrcode-rails3', '~> 0.1.7' gem 'attr_encrypted', '~> 3.1.0' gem 'u2f', '~> 0.2.1' diff --git a/Gemfile.lock b/Gemfile.lock index 2b0057d353d..f97c2bf67ec 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -270,11 +270,11 @@ GEM railties (>= 4.1.0) responders warden (~> 1.2.3) - devise-two-factor (4.0.0) - activesupport (< 6.2) + devise-two-factor (4.0.2) + activesupport (< 7.1) attr_encrypted (>= 1.3, < 4, != 2) devise (~> 4.0) - railties (< 6.2) + railties (< 7.1) rotp (~> 6.0) diff-lcs (1.4.4) diff_match_patch (0.1.0) @@ -1457,7 +1457,7 @@ DEPENDENCIES derailed_benchmarks device_detector devise (~> 4.7.2) - devise-two-factor (~> 4.0.0) + devise-two-factor (~> 4.0.2) diff_match_patch (~> 0.1.0) diffy (~> 3.3) discordrb-webhooks (~> 3.4) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 4228da279a4..b9ba9d8e1b0 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -65,6 +65,8 @@ module Ci FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze MINUTES_COST_FACTOR_FIELDS = %i[public_projects_minutes_cost_factor private_projects_minutes_cost_factor].freeze + TAG_LIST_MAX_LENGTH = 50 + has_many :builds has_many :runner_projects, inverse_of: :runner, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :runner_projects, disable_joins: true @@ -520,6 +522,11 @@ module Ci errors.add(:tags_list, 'can not be empty when runner is not allowed to pick untagged jobs') end + + if tag_list_changed? && tag_list.count > TAG_LIST_MAX_LENGTH + errors.add(:tags_list, + "Too many tags specified. Please limit the number of tags to #{TAG_LIST_MAX_LENGTH}") + end end def no_projects diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 0620bb1ffc5..42187c3ef99 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -41,15 +41,25 @@ RSpec.describe Ci::Runner do context 'when runner is not allowed to pick untagged jobs' do context 'when runner does not have tags' do + let(:runner) { build(:ci_runner, tag_list: [], run_untagged: false) } + + it 'is not valid' do + expect(runner).to be_invalid + end + end + + context 'when runner has too many tags' do + let(:runner) { build(:ci_runner, tag_list: (1..::Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" }, run_untagged: false) } + it 'is not valid' do - runner = build(:ci_runner, tag_list: [], run_untagged: false) expect(runner).to be_invalid end end context 'when runner has tags' do + let(:runner) { build(:ci_runner, tag_list: ['tag'], run_untagged: false) } + it 'is valid' do - runner = build(:ci_runner, tag_list: ['tag'], run_untagged: false) expect(runner).to be_valid end end diff --git a/spec/requests/api/ci/runner/runners_post_spec.rb b/spec/requests/api/ci/runner/runners_post_spec.rb index 1d553751eea..50ace7adccb 100644 --- a/spec/requests/api/ci/runner/runners_post_spec.rb +++ b/spec/requests/api/ci/runner/runners_post_spec.rb @@ -130,7 +130,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do } end - let_it_be(:new_runner) { create(:ci_runner) } + let_it_be(:new_runner) { build(:ci_runner) } it 'uses active value in registration' do expect_next_instance_of(::Ci::Runners::RegisterRunnerService) do |service| @@ -183,6 +183,54 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do expect(response).to have_gitlab_http_status(:created) expect(::Ci::Runner.last.ip_address).to eq('123.111.123.111') end + + context 'when tags parameter is provided' do + def request + post api('/runners'), params: { + token: registration_token, + tag_list: tag_list + } + end + + context 'with number of tags above limit' do + let(:tag_list) { (1..::Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } } + + it 'uses tag_list value in registration and returns error' do + expect_next_instance_of(::Ci::Runners::RegisterRunnerService) do |service| + expected_params = { tag_list: tag_list }.stringify_keys + + expect(service).to receive(:execute) + .once + .with(registration_token, a_hash_including(expected_params)) + .and_call_original + end + + request + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response.dig('message', 'tags_list')).to contain_exactly("Too many tags specified. Please limit the number of tags to #{::Ci::Runner::TAG_LIST_MAX_LENGTH}") + end + end + + context 'with number of tags below limit' do + let(:tag_list) { (1..20).map { |i| "tag#{i}" } } + + it 'uses tag_list value in registration and successfully creates runner' do + expect_next_instance_of(::Ci::Runners::RegisterRunnerService) do |service| + expected_params = { tag_list: tag_list }.stringify_keys + + expect(service).to receive(:execute) + .once + .with(registration_token, a_hash_including(expected_params)) + .and_call_original + end + + request + + expect(response).to have_gitlab_http_status(:created) + end + end + end end end end diff --git a/spec/requests/api/ci/runners_spec.rb b/spec/requests/api/ci/runners_spec.rb index a1fda68b77b..d6ebc197ab0 100644 --- a/spec/requests/api/ci/runners_spec.rb +++ b/spec/requests/api/ci/runners_spec.rb @@ -396,6 +396,19 @@ RSpec.describe API::Ci::Runners do expect(shared_runner.reload.tag_list).to include('ruby2.1', 'pgsql', 'mysql') end + it 'unrelated runner attribute on an existing runner with too many tags' do + # This test ensures that it is possible to update any attribute on a runner that currently fails the + # validation that ensures that there aren't too many tags associated with a runner + existing_invalid_shared_runner = build(:ci_runner, :instance, tag_list: (1..::Ci::Runner::TAG_LIST_MAX_LENGTH + 1).map { |i| "tag#{i}" } ) + existing_invalid_shared_runner.save!(validate: false) + + active = existing_invalid_shared_runner.active + update_runner(existing_invalid_shared_runner.id, admin, paused: active) + + expect(response).to have_gitlab_http_status(:ok) + expect(existing_invalid_shared_runner.reload.active).to eq(!active) + end + it 'runner untagged flag' do # Ensure tag list is non-empty before setting untagged to false. update_runner(shared_runner.id, admin, tag_list: ['ruby2.1', 'pgsql', 'mysql']) |