Merge branch 'timeout-ci-includes-expansion' into 'master'

Add timeout mechanism for includes expansion

See merge request gitlab-org/gitlab!16807

changelogs/unreleased/timeout-ci-includes-expansion.yml

+---
+title: Add timeout mechanism for CI config validation
+merge_request: 16807
+author:
+type: fixed

doc/ci/yaml/README.md

@@ -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.

ee/lib/ee/gitlab/ci/config_ee.rb

@@ -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
 

lib/gitlab/ci/config.rb

@@ -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

lib/gitlab/ci/config/external/context.rb

+# 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

lib/gitlab/ci/config/external/file/base.rb

@@ -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

lib/gitlab/ci/config/external/file/local.rb

@@ -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

lib/gitlab/ci/config/external/file/project.rb

@@ -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

lib/gitlab/ci/config/external/mapper.rb

@@ -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

lib/gitlab/ci/config/external/processor.rb

@@ -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

spec/lib/gitlab/ci/config/external/context_spec.rb

+# 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

spec/lib/gitlab/ci/config/external/file/base_spec.rb

 # 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

spec/lib/gitlab/ci/config/external/file/local_spec.rb

@@ -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)

spec/lib/gitlab/ci/config/external/file/project_spec.rb

@@ -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)

spec/lib/gitlab/ci/config/external/file/remote_spec.rb

@@ -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

spec/lib/gitlab/ci/config/external/file/template_spec.rb

@@ -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

spec/lib/gitlab/ci/config/external/mapper_spec.rb

@@ -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

spec/lib/gitlab/ci/config/external/processor_spec.rb

@@ -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

spec/lib/gitlab/ci/config_spec.rb

@@ -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