The following discussion from gitlab-org/security/gitlab!2863 should be addressed: - [ ] @fabiopitino started a [discussion](https://gitlab.com/gitlab-org/security/gitlab/-/merge_requests/2863#note_1146372917): (+3 comments) > `build.errors` could contain any other errors related to state machine transitions, not necessarily model validations. > > When wrong session URL is passed by the runner, we should see an exception being raised [here](https://gitlab.com/gitlab-org/gitlab/-/blob/7d04cb83fadf7fc12693a80215fc6f06029d4481/app/services/ci/register_job_service.rb#L200). We [assign the runner_session_attributes](https://gitlab.com/gitlab-org/gitlab/-/blob/7d04cb83fadf7fc12693a80215fc6f06029d4481/app/services/ci/register_job_service.rb#L247) and when we do [`build.run!`](https://gitlab.com/gitlab-org/gitlab/-/blob/7d04cb83fadf7fc12693a80215fc6f06029d4481/app/services/ci/register_job_service.rb#L258) it would raise a state machine error because when validating `Ci::Build` we would also validate the related `Ci::BuildRunnerSession`. > > This means that before we could have returned `409 Conflict` and now we could return a new status. :thinking: > > At the same time, if a user sets an invalid (or non https) URL, today they should still get `409 Conflict`. > > If I'm not wrong, this causes the runner to try again to pick other builds and always be unsuccessful. ## Proposal Apply the following patch: <details> <summary> Patch </summary> ```patch diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index f11577feb886..5a1b119b2c43 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -10,7 +10,9 @@ class RegisterJobService TEMPORARY_LOCK_TIMEOUT = 3.seconds - Result = Struct.new(:build, :build_json, :valid?) + Result = Struct.new(:build, :build_json, :valid?, :error) + + RunnerSessionError = Class.new(StandardError) ## # The queue depth limit number has been determined by observing 95 @@ -52,6 +54,7 @@ def execute(params = {}) def process_queue(params) valid = true + error = nil depth = 0 each_build(params) do |build| @@ -93,6 +96,7 @@ def process_queue(params) # The usage of valid: is described in # handling of ActiveRecord::StaleObjectError valid = false + error = result.error end end @@ -101,7 +105,7 @@ def process_queue(params) @metrics.observe_queue_depth(:not_found, depth) if valid @metrics.register_failure - Result.new(nil, nil, valid) + Result.new(nil, nil, valid, error) end # rubocop: disable CodeReuse/ActiveRecord @@ -184,6 +188,10 @@ def process_build(build, params) if assign_runner!(build, params) present_build!(build) end + rescue RunnerSessionError => ex + @metrics.increment_queue_operation(:runner_invalid_session) + + Result.new(nil, nil, false, ex.message) rescue ActiveRecord::StaleObjectError # We are looping to find another build that is not conflicting # It also indicates that this build can be picked and passed to runner. @@ -244,7 +252,11 @@ def log_build_dependencies_size(presented_build) def assign_runner!(build, params) build.runner_id = runner.id - build.runner_session_attributes = params[:session] if params[:session].present? + if params[:session].present? + build.runner_session_attributes = params[:session] + + check_build_runner_session_valid(build, runner) + end failure_reason, _ = pre_assign_runner_checks.find { |_, check| check.call(build, params) } @@ -293,6 +305,20 @@ def track_exception_for_build(ex, build) ) end + def check_build_runner_session_valid(build, runner) + return if build.runner_session.valid? + + error_messages = build.runner_session.errors.full_messages.join(', ') + + Gitlab::AppLogger.error( + event: 'runner_provided_invalid_session_data', + build_id: build.id, + runner_id: runner.id, + runner_session_errors: error_messages) + + raise RunnerSessionError, error_messages + end + def pre_assign_runner_checks { missing_dependency_failure: -> (build, _) { !build.has_valid_build_dependencies? }, diff --git a/ee/spec/requests/api/ci/runner_spec.rb b/ee/spec/requests/api/ci/runner_spec.rb index f1a9952267e8..346933670d7f 100644 --- a/ee/spec/requests/api/ci/runner_spec.rb +++ b/ee/spec/requests/api/ci/runner_spec.rb @@ -90,7 +90,7 @@ context 'job does not have secrets configured' do let(:secrets) { {} } - it 'doesn not return secrets configuration' do + it 'does not return secrets configuration' do request_job_with_secrets_supported expect(response).to have_gitlab_http_status(:created) @@ -115,10 +115,10 @@ end end end - end - def request_job_with_secrets_supported - request_job info: { features: { vault_secrets: true } } + def request_job_with_secrets_supported + request_job info: { features: { vault_secrets: true } } + end end def request_job(token = runner.token, **params) diff --git a/lib/api/ci/runner.rb b/lib/api/ci/runner.rb index df39ef1bafa1..b76d882fe503 100644 --- a/lib/api/ci/runner.rb +++ b/lib/api/ci/runner.rb @@ -101,7 +101,8 @@ class Runner < ::API::Base http_codes [[201, 'Job was scheduled'], [204, 'No job for Runner'], [403, 'Forbidden'], - [409, 'Conflict']] + [409, 'Conflict'], + [422, 'Unprocessable Entity']] end params do requires :token, type: String, desc: %q(Runner's authentication token) @@ -171,7 +172,12 @@ class Runner < ::API::Base else # We received build that is invalid due to concurrency conflict Gitlab::Metrics.add_event(:build_invalid) - conflict! + + if result.error + unprocessable_entity!(result.error) + else + conflict! + end end end diff --git a/lib/gitlab/ci/queue/metrics.rb b/lib/gitlab/ci/queue/metrics.rb index 5cee73238ca6..b0152b77ec08 100644 --- a/lib/gitlab/ci/queue/metrics.rb +++ b/lib/gitlab/ci/queue/metrics.rb @@ -31,6 +31,7 @@ class Metrics :queue_iteration, :queue_depth_limit, :queue_replication_lag, + :runner_invalid_session, :runner_pre_assign_checks_failed, :runner_pre_assign_checks_success, :runner_queue_tick, 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 baa16b052a07..0006e60ec756 100644 --- a/spec/requests/api/ci/runner/jobs_request_post_spec.rb +++ b/spec/requests/api/ci/runner/jobs_request_post_spec.rb @@ -977,8 +977,10 @@ it 'returns :unprocessable_entity status code', :aggregate_failures do request_job(**job_params) - expect(response).to have_gitlab_http_status(:conflict) - expect(response.body).to include('409 Conflict') + expect(response).to have_gitlab_http_status(:unprocessable_entity) + expect(response.body).to include( + 'Url is blocked: Requests to localhost are not allowed' + ) end end end ``` </details>