diff --git a/lib/api/ci/helpers/runner.rb b/lib/api/ci/helpers/runner.rb
index 7dd3d80445043c9ef7baed4e7073914c9fc7cb86..429f6a4627d5748a5704d2daae8ca176cdf940d8 100644
--- a/lib/api/ci/helpers/runner.rb
+++ b/lib/api/ci/helpers/runner.rb
@@ -12,6 +12,9 @@ module Runner
JOB_TOKEN_PARAM = :token
LEGACY_SYSTEM_XID = '<legacy>'
+ # TODO: Remove once https://gitlab.com/gitlab-org/gitlab/-/issues/504277 is closed.
+ UnknownRunnerOwnerError = Class.new(StandardError)
+
def authenticate_runner!(ensure_runner_manager: true, update_contacted_at: true)
track_runner_authentication
forbidden! unless current_runner
@@ -53,6 +56,9 @@ def current_runner
end
def current_runner_manager
+ # NOTE: Avoid orphaned runners, since we're not allowed to created records with a nil sharding_key_id
+ raise UnknownRunnerOwnerError if !current_runner.instance_type? && current_runner.sharding_key_id.nil?
+
strong_memoize(:current_runner_manager) do
system_xid = params.fetch(:system_id, LEGACY_SYSTEM_XID)
current_runner&.ensure_manager(system_xid)
diff --git a/lib/api/ci/runner.rb b/lib/api/ci/runner.rb
index d578405fe3376bf40b8b6558f5e3795d8738b5cd..43c03fd01aaefde8c21b8ed6d2e5e1881036ddc3 100644
--- a/lib/api/ci/runner.rb
+++ b/lib/api/ci/runner.rb
@@ -113,7 +113,7 @@ class Runner < ::API::Base
desc 'Validate authentication credentials' do
summary "Verify authentication for a registered runner"
success Entities::Ci::RunnerRegistrationDetails
- http_codes [[200, 'Credentials are valid'], [403, 'Forbidden']]
+ http_codes [[200, 'Credentials are valid'], [403, 'Forbidden'], [422, 'Runner is orphaned']]
end
params do
requires :token, type: String, desc: "The runner's authentication token"
@@ -128,6 +128,8 @@ class Runner < ::API::Base
status 200
present current_runner, with: Entities::Ci::RunnerRegistrationDetails
+ rescue ::API::Ci::Helpers::Runner::UnknownRunnerOwnerError
+ unprocessable_entity!('Runner is orphaned')
end
desc 'Reset runner authentication token with current token' do
@@ -153,7 +155,8 @@ class Runner < ::API::Base
http_codes [[201, 'Job was scheduled'],
[204, 'No job for Runner'],
[403, 'Forbidden'],
- [409, 'Conflict']]
+ [409, 'Conflict'],
+ [422, 'Runner is orphaned']]
end
params do
requires :token, type: String, desc: "Runner's authentication token"
@@ -225,6 +228,8 @@ class Runner < ::API::Base
Gitlab::Metrics.add_event(:build_invalid)
conflict!
end
+ rescue ::API::Ci::Helpers::Runner::UnknownRunnerOwnerError
+ unprocessable_entity!('Runner is orphaned')
end
desc 'Update a job' do
diff --git a/spec/lib/api/ci/helpers/runner_spec.rb b/spec/lib/api/ci/helpers/runner_spec.rb
index cecafc04e7e082ab37cf5c4a4cd7a8a52680f1f2..c98781c5fde5a50628a57c736a0af6dae7308cfb 100644
--- a/spec/lib/api/ci/helpers/runner_spec.rb
+++ b/spec/lib/api/ci/helpers/runner_spec.rb
@@ -107,14 +107,20 @@
expect(current_runner_manager.contacted_at).to eq 1.hour.ago
end
- context 'when a runner manager with nil sharding_key_id already exists' do
- before do
+ # TODO Remove when https://gitlab.com/gitlab-org/gitlab/-/issues/503749 is merged
+ context 'with nil sharding_key_id' do
+ let!(:existing_runner_manager) do
Ci::ApplicationRecord.connection.execute <<~SQL
- ALTER TABLE group_type_ci_runner_machines_687967fa8a
- DROP CONSTRAINT IF EXISTS check_sharding_key_id_nullness
+ ALTER TABLE ci_runner_machines DISABLE TRIGGER ALL;
+
+ INSERT INTO ci_runner_machines
+ (created_at, updated_at, contacted_at, runner_id, runner_type, system_xid, sharding_key_id)
+ VALUES(NOW(), NOW(), '#{1.hour.ago}', #{runner.id}, 2, 'bar', NULL);
+
+ ALTER TABLE ci_runner_machines ENABLE TRIGGER ALL;
SQL
- existing_runner_manager.update_columns(sharding_key_id: nil)
+ Ci::RunnerManager.for_runner(runner).with_system_xid('bar').first
end
it 'reuses existing runner manager', :aggregate_failures do
@@ -161,6 +167,28 @@
expect(current_runner_manager.sharding_key_id).to eq(runner.sharding_key_id)
end
end
+
+ context 'when project runner is missing sharding_key_id' do
+ let(:runner) { Ci::Runner.project_type.last }
+ let(:connection) { Ci::Runner.connection }
+
+ before do
+ connection.execute(<<~SQL)
+ ALTER TABLE ci_runners DISABLE TRIGGER ALL;
+
+ INSERT INTO ci_runners(created_at, runner_type, token, sharding_key_id) VALUES(NOW(), 3, 'foo', NULL);
+
+ ALTER TABLE ci_runners ENABLE TRIGGER ALL;
+ SQL
+ end
+
+ it 'fails to create a new runner manager', :aggregate_failures do
+ allow(helper).to receive(:params).and_return(token: runner.token, system_id: 'new_system_id')
+ expect(helper.current_runner).to eq(runner)
+
+ expect { current_runner_manager }.to raise_error described_class::UnknownRunnerOwnerError
+ end
+ end
end
describe '#track_runner_authentication', :prometheus, feature_category: :runner do
diff --git a/spec/requests/api/ci/runner/jobs_request_post_spec.rb b/spec/requests/api/ci/runner/jobs_request_post_spec.rb
index 5d5dbda04fa79763c2d610bf197a4f71c590ce11..8b579fa3d3f825d97b54708b91d87ef0673ac79b 100644
--- a/spec/requests/api/ci/runner/jobs_request_post_spec.rb
+++ b/spec/requests/api/ci/runner/jobs_request_post_spec.rb
@@ -182,6 +182,24 @@
expect(non_partitioned_runner.contacted_at).to be_nil
end
+ context 'when project runner is missing sharding_key_id' do
+ let(:params) { { token: 'foo' } }
+ let(:runner) { Ci::Runner.project_type.last }
+ let(:non_partitioned_runner) do
+ connection.execute(<<~SQL)
+ INSERT INTO ci_runners(created_at, runner_type, token, sharding_key_id) VALUES(NOW(), 3, 'foo', NULL);
+ SQL
+
+ runner
+ end
+
+ it 'returns unprocessable entity status code', :aggregate_failures do
+ expect { request }.not_to change { Ci::RunnerManager.count }.from(0)
+ expect(response).to have_gitlab_http_status(:unprocessable_entity)
+ expect(response.body).to eq({ message: 'Runner is orphaned' }.to_json)
+ end
+ end
+
private
def partitioned_runner_exists?(runner)
diff --git a/spec/requests/api/ci/runner/runners_verify_post_spec.rb b/spec/requests/api/ci/runner/runners_verify_post_spec.rb
index 192e8d88a4dbe0d0259a170375b45ba5caddcde4..39323c8f988859afb3e55dc98a52e0cf4359fab6 100644
--- a/spec/requests/api/ci/runner/runners_verify_post_spec.rb
+++ b/spec/requests/api/ci/runner/runners_verify_post_spec.rb
@@ -159,6 +159,29 @@ def partitioned_runner_exists?(runner)
it 'creates a runner_manager' do
expect { verify }.to change { Ci::RunnerManager.count }.by(1)
end
+
+ # TODO: Remove once https://gitlab.com/gitlab-org/gitlab/-/issues/504277 is closed.
+ context 'when project runner is missing sharding_key_id' do
+ let(:runner) { Ci::Runner.project_type.last }
+ let(:params) { { token: 'foo' } }
+ let(:connection) { Ci::Runner.connection }
+
+ before do
+ connection.execute(<<~SQL)
+ ALTER TABLE ci_runners DISABLE TRIGGER ALL;
+
+ INSERT INTO ci_runners(created_at, runner_type, token, sharding_key_id) VALUES(NOW(), 3, 'foo', NULL);
+
+ ALTER TABLE ci_runners ENABLE TRIGGER ALL;
+ SQL
+ end
+
+ it 'returns unprocessable entity status code', :aggregate_failures do
+ expect { verify }.not_to change { Ci::RunnerManager.count }.from(0)
+ expect(response).to have_gitlab_http_status(:unprocessable_entity)
+ expect(response.body).to eq({ message: 'Runner is orphaned' }.to_json)
+ end
+ end
end
end