Commit 6a6b5d8c authored by Stan Hu's avatar Stan Hu

Redact push options from error logs

When an error creating a pipeline occurs, we log all the pipeline
parameters, including the push options, in an error log and in
Sentry. However, since push options are flattened into unique keys
(e.g. `merge_request.label=gitlab-ci`, `ci.variable.MY_VARIABLE=test`),
it's easy to generate many unique keys, which may exceed the
Elasticsearch limit of 1000.

To avoid this from happening, this commit drops the `push_options` field
from the log. These generally shouldn't be needed to understand why
a pipeline failed to create.

Closes https://gitlab.com/gitlab-org/gitlab/issues/202129
parent 738e0e9e
......@@ -81,6 +81,7 @@ module Git
end
def pipeline_params
strong_memoize(:pipeline_params) do
{
before: oldrev,
after: newrev,
......@@ -91,6 +92,7 @@ module Git
project.repository, newrev, ref)
}
end
end
def ci_variables_from_push_options
strong_memoize(:ci_variables_from_push_options) do
......@@ -156,12 +158,16 @@ module Git
project_path: project.full_path,
message: "Error creating pipeline",
errors: exception.to_s,
pipeline_params: pipeline_params
pipeline_params: sanitized_pipeline_params
}
logger.warn(data)
end
def sanitized_pipeline_params
pipeline_params.except(:push_options)
end
def logger
if Gitlab::Runtime.sidekiq?
Sidekiq.logger
......
---
title: Redact push options from error logs
merge_request: 24540
author:
type: fixed
......@@ -12,6 +12,7 @@ describe Git::BranchPushService, services: true do
let(:newrev) { sample_commit.id }
let(:branch) { 'master' }
let(:ref) { "refs/heads/#{branch}" }
let(:push_options) { nil }
before do
project.add_maintainer(user)
......@@ -19,7 +20,7 @@ describe Git::BranchPushService, services: true do
describe 'Push branches' do
subject do
execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref)
execute_service(project, user, oldrev: oldrev, newrev: newrev, ref: ref, push_options: push_options)
end
context 'new branch' do
......@@ -113,6 +114,20 @@ describe Git::BranchPushService, services: true do
expect { subject }.not_to change { Ci::Pipeline.count }
end
context 'with push options' do
let(:push_options) { ['mr.create'] }
it 'sanitizes push options' do
allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
expect(Sidekiq.logger).to receive(:warn) do |args|
pipeline_params = args[:pipeline_params]
expect(pipeline_params.keys).to match_array(%i(before after ref variables_attributes checkout_sha))
end
expect { subject }.not_to change { Ci::Pipeline.count }
end
end
end
end
......@@ -637,8 +652,8 @@ describe Git::BranchPushService, services: true do
end
end
def execute_service(project, user, change)
service = described_class.new(project, user, change: change)
def execute_service(project, user, change, push_options = {})
service = described_class.new(project, user, change: change, push_options: push_options)
service.execute
service
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment