Commit 98393b46 authored by Marius Bobin's avatar Marius Bobin Committed by Kamil Trzciński

Add timeout mechanism for includes expansion

Define soft timeout for CI includes expansion in
`Gitlab::Ci::Config::External::File::Base::Context`
as `INCLUDES_TIMEOUT` with a default value of 10 seconds
parent cb068f91
---
title: Add timeout mechanism for CI config validation
merge_request: 16807
author:
type: fixed
......@@ -2289,6 +2289,10 @@ Nested includes allow you to compose a set of includes.
A total of 50 includes is allowed.
Duplicate includes are considered a configuration error.
> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/28212) in GitLab 12.4.
A hard limit of 30 seconds was set for resolving all files.
#### `include` examples
Here are a few more `include` examples.
......
......@@ -14,7 +14,7 @@ module EE
end
override :build_config
def build_config(config, project:, sha:, user:)
def build_config(config)
process_required_includes(super)
end
......
......@@ -7,6 +7,8 @@ module Gitlab
#
class Config
ConfigError = Class.new(StandardError)
TIMEOUT_SECONDS = 30.seconds
TIMEOUT_MESSAGE = 'Resolving config took longer than expected'
RESCUE_ERRORS = [
Gitlab::Config::Loader::FormatError,
......@@ -17,17 +19,17 @@ module Gitlab
attr_reader :root
def initialize(config, project: nil, sha: nil, user: nil)
@config = Config::Extendable
.new(build_config(config, project: project, sha: sha, user: user))
.to_hash
@context = build_context(project: project, sha: sha, user: user)
if Feature.enabled?(:ci_limit_yaml_expansion, project, default_enabled: true)
@context.set_deadline(TIMEOUT_SECONDS)
end
@config = expand_config(config)
@root = Entry::Root.new(@config)
@root.compose!
rescue Gitlab::Config::Loader::Yaml::DataTooLargeError => e
Gitlab::Sentry.track_exception(e, extra: { user: user.inspect, project: project.inspect })
raise Config::ConfigError, e.message
rescue *rescue_errors => e
raise Config::ConfigError, e.message
end
......@@ -61,18 +63,34 @@ module Gitlab
private
def build_config(config, project:, sha:, user:)
def expand_config(config)
build_config(config)
rescue Gitlab::Config::Loader::Yaml::DataTooLargeError => e
track_exception(e)
raise Config::ConfigError, e.message
rescue Gitlab::Ci::Config::External::Context::TimeoutError => e
track_exception(e)
raise Config::ConfigError, TIMEOUT_MESSAGE
end
def build_config(config)
initial_config = Gitlab::Config::Loader::Yaml.new(config).load!
initial_config = Config::External::Processor.new(initial_config, @context).perform
process_external_files(initial_config, project: project, sha: sha, user: user)
Config::Extendable.new(initial_config).to_hash
end
def process_external_files(config, project:, sha:, user:)
Config::External::Processor.new(config,
def build_context(project:, sha:, user:)
Config::External::Context.new(
project: project,
sha: sha || project&.repository&.root_ref_sha,
user: user,
expandset: Set.new).perform
user: user)
end
def track_exception(error)
Gitlab::Sentry.track_exception(error, extra: @context.sentry_payload)
end
# Overriden in EE
......
# frozen_string_literal: true
module Gitlab
module Ci
class Config
module External
class Context
TimeoutError = Class.new(StandardError)
attr_reader :project, :sha, :user
attr_reader :expandset, :execution_deadline
def initialize(project: nil, sha: nil, user: nil)
@project = project
@sha = sha
@user = user
@expandset = Set.new
@execution_deadline = 0
yield self if block_given?
end
def mutate(attrs = {})
self.class.new(**attrs) do |ctx|
ctx.expandset = expandset
ctx.execution_deadline = execution_deadline
end
end
def set_deadline(timeout_seconds)
@execution_deadline = current_monotonic_time + timeout_seconds.to_f
end
def check_execution_time!
raise TimeoutError if execution_expired?
end
def sentry_payload
{
user: user.inspect,
project: project.inspect
}
end
protected
attr_writer :expandset, :execution_deadline
private
def current_monotonic_time
Gitlab::Metrics::System.monotonic_time
end
def execution_expired?
return false if execution_deadline.zero?
current_monotonic_time > execution_deadline
end
end
end
end
end
end
......@@ -12,8 +12,6 @@ module Gitlab
YAML_WHITELIST_EXTENSION = /.+\.(yml|yaml)$/i.freeze
Context = Struct.new(:project, :sha, :user, :expandset)
def initialize(params, context)
@params = params
@context = context
......@@ -69,11 +67,16 @@ module Gitlab
end
def validate!
validate_execution_time!
validate_location!
validate_content! if errors.none?
validate_hash! if errors.none?
end
def validate_execution_time!
context.check_execution_time!
end
def validate_location!
if invalid_location_type?
errors.push("Included file `#{location}` needs to be a string")
......@@ -95,11 +98,11 @@ module Gitlab
end
def expand_includes(hash)
External::Processor.new(hash, **expand_context).perform
External::Processor.new(hash, context.mutate(expand_context_attrs)).perform
end
def expand_context
{ project: nil, sha: nil, user: nil, expandset: context.expandset }
def expand_context_attrs
{}
end
end
end
......
......@@ -6,6 +6,7 @@ module Gitlab
module External
module File
class Local < Base
extend ::Gitlab::Utils::Override
include Gitlab::Utils::StrongMemoize
def initialize(params, context)
......@@ -34,11 +35,13 @@ module Gitlab
context.project.repository.blob_data_at(context.sha, location)
end
def expand_context
super.merge(
override :expand_context_attrs
def expand_context_attrs
{
project: context.project,
sha: context.sha,
user: context.user)
user: context.user
}
end
end
end
......
......@@ -6,11 +6,12 @@ module Gitlab
module External
module File
class Project < Base
extend ::Gitlab::Utils::Override
include Gitlab::Utils::StrongMemoize
attr_reader :project_name, :ref_name
def initialize(params, context = {})
def initialize(params, context)
@location = params[:file]
@project_name = params[:project]
@ref_name = params[:ref] || 'HEAD'
......@@ -65,11 +66,13 @@ module Gitlab
end
end
def expand_context
super.merge(
override :expand_context_attrs
def expand_context_attrs
{
project: project,
sha: sha,
user: context.user)
user: context.user
}
end
end
end
......
......@@ -21,14 +21,9 @@ module Gitlab
DuplicateIncludesError = Class.new(Error)
TooManyIncludesError = Class.new(Error)
def initialize(values, project:, sha:, user:, expandset:)
raise Error, 'Expanded needs to be `Set`' unless expandset.is_a?(Set)
def initialize(values, context)
@locations = Array.wrap(values.fetch(:include, []))
@project = project
@sha = sha
@user = user
@expandset = expandset
@context = context
end
def process
......@@ -43,7 +38,9 @@ module Gitlab
private
attr_reader :locations, :project, :sha, :user, :expandset
attr_reader :locations, :context
delegate :expandset, to: :context
# convert location if String to canonical form
def normalize_location(location)
......@@ -68,11 +65,11 @@ module Gitlab
end
# We scope location to context, as this allows us to properly support
# relative incldues, and similarly looking relative in another project
# relative includes, and similarly looking relative in another project
# does not trigger duplicate error
scoped_location = location.merge(
context_project: project,
context_sha: sha)
context_project: context.project,
context_sha: context.sha)
unless expandset.add?(scoped_location)
raise DuplicateIncludesError, "Include `#{location.to_json}` was already included!"
......@@ -88,12 +85,6 @@ module Gitlab
matching.first
end
def context
strong_memoize(:context) do
External::File::Base::Context.new(project, sha, user, expandset)
end
end
end
end
end
......
......@@ -7,9 +7,9 @@ module Gitlab
class Processor
IncludeError = Class.new(StandardError)
def initialize(values, project:, sha:, user:, expandset:)
def initialize(values, context)
@values = values
@external_files = External::Mapper.new(values, project: project, sha: sha, user: user, expandset: expandset).process
@external_files = External::Mapper.new(values, context).process
@content = {}
rescue External::Mapper::Error,
OpenSSL::SSL::SSLError => e
......
# frozen_string_literal: true
require 'fast_spec_helper'
describe Gitlab::Ci::Config::External::Context do
let(:project) { double('Project') }
let(:user) { double('User') }
let(:sha) { '12345' }
let(:attributes) { { project: project, user: user, sha: sha } }
subject(:subject) { described_class.new(**attributes) }
describe 'attributes' do
context 'with values' do
it { is_expected.to have_attributes(**attributes) }
it { expect(subject.expandset).to eq(Set.new) }
it { expect(subject.execution_deadline).to eq(0) }
end
context 'without values' do
let(:attributes) { { project: nil, user: nil, sha: nil } }
it { is_expected.to have_attributes(**attributes) }
it { expect(subject.expandset).to eq(Set.new) }
it { expect(subject.execution_deadline).to eq(0) }
end
end
describe '#set_deadline' do
let(:stubbed_time) { 1 }
before do
allow(subject).to receive(:current_monotonic_time).and_return(stubbed_time)
end
context 'with a float value' do
let(:timeout_seconds) { 10.5.seconds }
it 'updates execution_deadline' do
expect { subject.set_deadline(timeout_seconds) }
.to change { subject.execution_deadline }
.to(timeout_seconds + stubbed_time)
end
end
context 'with nil as a value' do
let(:timeout_seconds) {}
it 'updates execution_deadline' do
expect { subject.set_deadline(timeout_seconds) }
.to change { subject.execution_deadline }
.to(stubbed_time)
end
end
end
describe '#check_execution_time!' do
before do
allow(subject).to receive(:current_monotonic_time).and_return(stubbed_time)
allow(subject).to receive(:execution_deadline).and_return(stubbed_deadline)
end
context 'when execution is expired' do
let(:stubbed_time) { 2 }
let(:stubbed_deadline) { 1 }
it 'raises an error' do
expect { subject.check_execution_time! }
.to raise_error(described_class::TimeoutError)
end
end
context 'when execution is not expired' do
let(:stubbed_time) { 1 }
let(:stubbed_deadline) { 2 }
it 'does not raises any errors' do
expect { subject.check_execution_time! }.not_to raise_error
end
end
context 'without setting a deadline' do
let(:stubbed_time) { 2 }
let(:stubbed_deadline) { 1 }
before do
allow(subject).to receive(:execution_deadline).and_call_original
end
it 'does not raises any errors' do
expect { subject.check_execution_time! }.not_to raise_error
end
end
end
describe '#mutate' do
shared_examples 'a mutated context' do
let(:mutated) { subject.mutate(new_attributes) }
before do
subject.expandset << :a_file
subject.set_deadline(15.seconds)
end
it { expect(mutated).not_to eq(subject) }
it { expect(mutated).to be_a(described_class) }
it { expect(mutated).to have_attributes(new_attributes) }
it { expect(mutated.expandset).to eq(subject.expandset) }
it { expect(mutated.execution_deadline).to eq(mutated.execution_deadline) }
end
context 'with attributes' do
let(:new_attributes) { { project: double, user: double, sha: '56789' } }
it_behaves_like 'a mutated context'
end
context 'without attributes' do
let(:new_attributes) { {} }
it_behaves_like 'a mutated context'
end
end
describe '#sentry_payload' do
it { expect(subject.sentry_payload).to match(a_hash_including(:project, :user)) }
end
end
# frozen_string_literal: true
require 'fast_spec_helper'
require 'spec_helper'
describe Gitlab::Ci::Config::External::File::Base do
let(:context) { described_class::Context.new(nil, 'HEAD', nil, Set.new) }
let(:context_params) { { sha: 'HEAD' } }
let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
let(:test_class) do
Class.new(described_class) do
def initialize(params, context = {})
def initialize(params, context)
@location = params
super
......@@ -20,6 +21,9 @@ describe Gitlab::Ci::Config::External::File::Base do
before do
allow_any_instance_of(test_class)
.to receive(:content).and_return('key: value')
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
describe '#matching?' do
......
......@@ -7,10 +7,17 @@ describe Gitlab::Ci::Config::External::File::Local do
set(:user) { create(:user) }
let(:sha) { '12345' }
let(:context) { described_class::Context.new(project, sha, user, Set.new) }
let(:context_params) { { project: project, sha: sha, user: user } }
let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
let(:params) { { local: location } }
let(:local_file) { described_class.new(params, context) }
before do
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
describe '#matching?' do
context 'when a local is specified' do
let(:params) { { local: 'file' } }
......@@ -109,7 +116,7 @@ describe Gitlab::Ci::Config::External::File::Local do
describe '#expand_context' do
let(:location) { 'location.yml' }
subject { local_file.send(:expand_context) }
subject { local_file.send(:expand_context_attrs) }
it 'inherits project, user and sha' do
is_expected.to include(user: user, project: project, sha: sha)
......
......@@ -8,11 +8,15 @@ describe Gitlab::Ci::Config::External::File::Project do
set(:user) { create(:user) }
let(:context_user) { user }
let(:context) { described_class::Context.new(context_project, '12345', context_user, Set.new) }
let(:context_params) { { project: context_project, sha: '12345', user: context_user } }
let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
let(:project_file) { described_class.new(params, context) }
before do
project.add_developer(user)
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
describe '#matching?' do
......@@ -145,7 +149,7 @@ describe Gitlab::Ci::Config::External::File::Project do
describe '#expand_context' do
let(:params) { { file: 'file.yml', project: project.full_path, ref: 'master' } }
subject { project_file.send(:expand_context) }
subject { project_file.send(:expand_context_attrs) }
it 'inherits user, and target project and sha' do
is_expected.to include(user: user, project: project, sha: project.commit('master').id)
......
......@@ -5,7 +5,8 @@ require 'spec_helper'
describe Gitlab::Ci::Config::External::File::Remote do
include StubRequests
let(:context) { described_class::Context.new(nil, '12345', nil, Set.new) }
let(:context_params) { { sha: '12345' } }
let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
let(:params) { { remote: location } }
let(:remote_file) { described_class.new(params, context) }
let(:location) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }
......@@ -19,6 +20,11 @@ describe Gitlab::Ci::Config::External::File::Remote do
HEREDOC
end
before do
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
describe '#matching?' do
context 'when a remote is specified' do
let(:params) { { remote: 'http://remote' } }
......@@ -187,10 +193,10 @@ describe Gitlab::Ci::Config::External::File::Remote do
describe '#expand_context' do
let(:params) { { remote: 'http://remote' } }
subject { remote_file.send(:expand_context) }
subject { remote_file.send(:expand_context_attrs) }
it 'drops all parameters' do
is_expected.to include(user: nil, project: nil, sha: nil)
is_expected.to be_empty
end
end
end
......@@ -6,12 +6,18 @@ describe Gitlab::Ci::Config::External::File::Template do
set(:project) { create(:project) }
set(:user) { create(:user) }
let(:context) { described_class::Context.new(project, '12345', user, Set.new) }
let(:context_params) { { project: project, sha: '12345', user: user } }
let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
let(:template) { 'Auto-DevOps.gitlab-ci.yml' }
let(:params) { { template: template } }
let(:template_file) { described_class.new(params, context) }
before do
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
describe '#matching?' do
context 'when a template is specified' do
let(:params) { { template: 'some-template' } }
......@@ -97,10 +103,10 @@ describe Gitlab::Ci::Config::External::File::Template do
describe '#expand_context' do
let(:location) { 'location.yml' }
subject { template_file.send(:expand_context) }
subject { template_file.send(:expand_context_attrs) }
it 'drops all parameters' do
is_expected.to include(user: nil, project: nil, sha: nil)
is_expected.to be_empty
end
end
end
......@@ -11,7 +11,8 @@ describe Gitlab::Ci::Config::External::Mapper do
let(:local_file) { '/lib/gitlab/ci/templates/non-existent-file.yml' }
let(:remote_url) { 'https://gitlab.com/gitlab-org/gitlab-foss/blob/1234/.gitlab-ci-1.yml' }
let(:template_file) { 'Auto-DevOps.gitlab-ci.yml' }
let(:expandset) { Set.new }
let(:context_params) { { project: project, sha: '123456', user: user } }
let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
let(:file_content) do
<<~HEREDOC
......@@ -21,10 +22,13 @@ describe Gitlab::Ci::Config::External::Mapper do
before do
stub_full_request(remote_url).to_return(body: file_content)
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
describe '#process' do
subject { described_class.new(values, project: project, sha: '123456', user: user, expandset: expandset).process }
subject { described_class.new(values, context).process }
context "when single 'include' keyword is defined" do
context 'when the string is a local file' do
......
......@@ -9,12 +9,16 @@ describe Gitlab::Ci::Config::External::Processor do
set(:another_project) { create(:project, :repository) }
set(:user) { create(:user) }
let(:expandset) { Set.new }
let(:sha) { '12345' }
let(:processor) { described_class.new(values, project: project, sha: '12345', user: user, expandset: expandset) }
let(:context_params) { { project: project, sha: sha, user: user } }
let(:context) { Gitlab::Ci::Config::External::Context.new(**context_params) }
let(:processor) { described_class.new(values, context) }
before do
project.add_developer(user)
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
describe "#perform" do
......
......@@ -7,6 +7,11 @@ describe Gitlab::Ci::Config do
set(:user) { create(:user) }
before do
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
end
let(:config) do
described_class.new(yml, project: nil, sha: nil, user: nil)
end
......@@ -303,6 +308,49 @@ describe Gitlab::Ci::Config do
end
end
context "when it takes too long to evaluate includes" do
before do
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
.and_call_original
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:set_deadline)
.with(described_class::TIMEOUT_SECONDS)
.and_call_original
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:execution_expired?)
.and_return(true)
end
it 'raises error TimeoutError' do
expect(Gitlab::Sentry).to receive(:track_exception)
expect { config }.to raise_error(
described_class::ConfigError,
'Resolving config took longer than expected'
)
end
end
context 'when context expansion timeout is disabled' do
before do
allow_any_instance_of(Gitlab::Ci::Config::External::Context)
.to receive(:check_execution_time!)
.and_call_original
allow(Feature)
.to receive(:enabled?)
.with(:ci_limit_yaml_expansion, project, default_enabled: true)
.and_return(false)
end
it 'does not raises errors' do
expect { config }.not_to raise_error
end
end
describe 'external file version' do
context 'when external local file SHA is defined' do
it 'is using a defined value' do
......
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