diff --git a/config/feature_flags/development/ci_fix_max_includes.yml b/config/feature_flags/development/ci_fix_max_includes.yml new file mode 100644 index 0000000000000000000000000000000000000000..ef993f4f7eed2d02061162e7361af9c065d0001f --- /dev/null +++ b/config/feature_flags/development/ci_fix_max_includes.yml @@ -0,0 +1,8 @@ +--- +name: ci_fix_max_includes +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/112963 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/390909 +milestone: '15.10' +type: development +group: group::pipeline authoring +default_enabled: false diff --git a/ee/lib/gitlab/ci/project_config/compliance.rb b/ee/lib/gitlab/ci/project_config/compliance.rb index ad0594724f14a97b34d45e75d5330769a1310aa2..b2771f388810963c30f49fe35b13e5d159498cc7 100644 --- a/ee/lib/gitlab/ci/project_config/compliance.rb +++ b/ee/lib/gitlab/ci/project_config/compliance.rb @@ -16,7 +16,7 @@ def content end end - def contains_internal_include? + def internal_include_prepended? true end diff --git a/ee/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb b/ee/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb index 558dc6a8b5caf6a9bd10178b29a25e7f55f2afc8..d2453e95f7704b2325330949af669b74a91cfffa 100644 --- a/ee/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb +++ b/ee/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb @@ -36,7 +36,7 @@ expect(pipeline.config_source).to eq 'compliance_source' expect(pipeline.pipeline_config.content).to eq(content_result) expect(command.config_content).to eq(content_result) - expect(command.pipeline_config.contains_internal_include?).to eq(true) + expect(command.pipeline_config.internal_include_prepended?).to eq(true) end end diff --git a/lib/gitlab/ci/config/external/context.rb b/lib/gitlab/ci/config/external/context.rb index 881b131c25209f89bec5758d5a98b4e5373a16f7..156109a084d69bd9f0f575473536e2de1c084182 100644 --- a/lib/gitlab/ci/config/external/context.rb +++ b/lib/gitlab/ci/config/external/context.rb @@ -92,10 +92,11 @@ def includes expandset.map(&:metadata) end - # Some ProjectConfig sources inject an `include` into the config content. We use this - # method to exclude that `include` from the calculation of the total included files. - def contains_internal_include? - !!pipeline_config&.contains_internal_include? + # Some Ci::ProjectConfig sources prepend the config content with an "internal" `include`, which becomes + # the first included file. When running a pipeline, we pass pipeline_config into the context of the first + # included file, which we use in this method to determine if the file is an "internal" one. + def internal_include? + !!pipeline_config&.internal_include_prepended? end protected diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb index 6c2e52958da84774732a7434f1c91569b7f60fff..7284d2a7e01b023806a40aab7797fb95cbabaad0 100644 --- a/lib/gitlab/ci/config/external/mapper/verifier.rb +++ b/lib/gitlab/ci/config/external/mapper/verifier.rb @@ -9,12 +9,20 @@ class Mapper class Verifier < Base private + # rubocop: disable Metrics/CyclomaticComplexity def process_without_instrumentation(files) if ::Feature.disabled?(:ci_batch_project_includes_context, context.project) return legacy_process_without_instrumentation(files) end files.each do |file| + if YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) + # When running a pipeline, some Ci::ProjectConfig sources prepend the config content with an + # "internal" `include`. We use this condition to exclude that `include` from the included file set. + context.expandset << file unless context.internal_include? + verify_max_includes! + end + verify_execution_time! file.validate_location! @@ -31,19 +39,26 @@ def process_without_instrumentation(files) # We do not combine the loops because we need to load the content of all files via `BatchLoader`. files.each do |file| # rubocop:disable Style/CombinableLoops - # Checking the max includes will be changed with https://gitlab.com/gitlab-org/gitlab/-/issues/367150 - verify_max_includes! + verify_max_includes! unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) verify_execution_time! file.validate_content! if file.valid? file.load_and_validate_expanded_hash! if file.valid? - context.expandset << file + context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) end end + # rubocop: enable Metrics/CyclomaticComplexity def legacy_process_without_instrumentation(files) files.each do |file| + if YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) + # When running a pipeline, some Ci::ProjectConfig sources prepend the config content with an + # "internal" `include`. We use this condition to exclude that `include` from the included file set. + context.expandset << file unless context.internal_include? + verify_max_includes! + end + verify_execution_time! file.validate_location! @@ -54,19 +69,22 @@ def legacy_process_without_instrumentation(files) # We do not combine the loops because we need to load the content of all files before continuing # to call `BatchLoader` for all locations. files.each do |file| # rubocop:disable Style/CombinableLoops - # Checking the max includes will be changed with https://gitlab.com/gitlab-org/gitlab/-/issues/367150 - verify_max_includes! + verify_max_includes! unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) verify_execution_time! file.validate_content! if file.valid? file.load_and_validate_expanded_hash! if file.valid? - context.expandset << file + context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) end end def verify_max_includes! - return if context.expandset.count < context.max_includes + if YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) + return if context.expandset.count <= context.max_includes + else + return if context.expandset.count < context.max_includes # rubocop:disable Style/IfInsideElse + end raise Mapper::TooManyIncludesError, "Maximum of #{context.max_includes} nested includes are allowed!" end diff --git a/lib/gitlab/ci/project_config.rb b/lib/gitlab/ci/project_config.rb index e69efb85a937f63d443c7307e12cb29dc7c7a641..00b2ad58428e0f44e40232847a460e12a4b9f978 100644 --- a/lib/gitlab/ci/project_config.rb +++ b/lib/gitlab/ci/project_config.rb @@ -26,7 +26,7 @@ def initialize(project:, sha:, custom_content: nil, pipeline_source: nil, pipeli end delegate :content, :source, to: :@config, allow_nil: true - delegate :contains_internal_include?, to: :@config + delegate :internal_include_prepended?, to: :@config def exists? !!@config&.exists? diff --git a/lib/gitlab/ci/project_config/auto_devops.rb b/lib/gitlab/ci/project_config/auto_devops.rb index 70f8c7b9bb3f2385c6bf2dc13f3c8325e66ed6cc..c5f010ebaeac52ddf271f9b44636a8c41eea47c1 100644 --- a/lib/gitlab/ci/project_config/auto_devops.rb +++ b/lib/gitlab/ci/project_config/auto_devops.rb @@ -13,7 +13,7 @@ def content end end - def contains_internal_include? + def internal_include_prepended? true end diff --git a/lib/gitlab/ci/project_config/external_project.rb b/lib/gitlab/ci/project_config/external_project.rb index b29825514585cbd4e85ce36e84ce60c52a2000f9..0afdab23886d77ceb343956c8f2a42fc7ac4188f 100644 --- a/lib/gitlab/ci/project_config/external_project.rb +++ b/lib/gitlab/ci/project_config/external_project.rb @@ -17,7 +17,7 @@ def content end end - def contains_internal_include? + def internal_include_prepended? true end diff --git a/lib/gitlab/ci/project_config/remote.rb b/lib/gitlab/ci/project_config/remote.rb index 17d5a9ee68fd9618c5dd74c7d053a8f72cd2d67c..19cbf8e9c1ea74b8672a7e70afa37ea6c8e07579 100644 --- a/lib/gitlab/ci/project_config/remote.rb +++ b/lib/gitlab/ci/project_config/remote.rb @@ -12,7 +12,7 @@ def content end end - def contains_internal_include? + def internal_include_prepended? true end diff --git a/lib/gitlab/ci/project_config/repository.rb b/lib/gitlab/ci/project_config/repository.rb index c975023d35e27a1a7471f3c63a9030e9719330df..272425fd5465d854bdff8f0a9cfa1a8c9b5b5aff 100644 --- a/lib/gitlab/ci/project_config/repository.rb +++ b/lib/gitlab/ci/project_config/repository.rb @@ -12,7 +12,7 @@ def content end end - def contains_internal_include? + def internal_include_prepended? true end diff --git a/lib/gitlab/ci/project_config/source.rb b/lib/gitlab/ci/project_config/source.rb index 30edf1b552a5bb47ef58633c820c79ce2c35c8ac..9a4a6394fa1ff1ec66ab341cd37096d479510b82 100644 --- a/lib/gitlab/ci/project_config/source.rb +++ b/lib/gitlab/ci/project_config/source.rb @@ -24,8 +24,8 @@ def content raise NotImplementedError end - # Indicates if we are internally injecting an 'include' into the content - def contains_internal_include? + # Indicates if we are prepending the content with an "internal" `include` + def internal_include_prepended? false end diff --git a/spec/lib/gitlab/ci/config/external/context_spec.rb b/spec/lib/gitlab/ci/config/external/context_spec.rb index f1640822c6bce079ac00aca50cd173c497544831..fc68d3d62a25f584650e10e7e36472416723d15c 100644 --- a/spec/lib/gitlab/ci/config/external/context_spec.rb +++ b/spec/lib/gitlab/ci/config/external/context_spec.rb @@ -7,7 +7,16 @@ let(:user) { double('User') } let(:sha) { '12345' } let(:variables) { Gitlab::Ci::Variables::Collection.new([{ 'key' => 'a', 'value' => 'b' }]) } - let(:attributes) { { project: project, user: user, sha: sha, variables: variables } } + let(:pipeline_config) { instance_double(Gitlab::Ci::ProjectConfig) } + let(:attributes) do + { + project: project, + user: user, + sha: sha, + variables: variables, + pipeline_config: pipeline_config + } + end subject(:subject) { described_class.new(**attributes) } @@ -20,6 +29,7 @@ it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } it { expect(subject.variables_hash).to include('a' => 'b') } + it { expect(subject.pipeline_config).to eq(pipeline_config) } end context 'without values' do @@ -31,6 +41,7 @@ it { expect(subject.execution_deadline).to eq(0) } it { expect(subject.variables).to be_instance_of(Gitlab::Ci::Variables::Collection) } it { expect(subject.variables_hash).to be_instance_of(ActiveSupport::HashWithIndifferentAccess) } + it { expect(subject.pipeline_config).to be_nil } end end @@ -144,27 +155,24 @@ it { expect(subject.sentry_payload).to match(a_hash_including(:project, :user)) } end - describe '#contains_internal_include?' do + describe '#internal_include?' do context 'when pipeline_config is provided' do - let(:pipeline_config) { instance_double(Gitlab::Ci::ProjectConfig) } - let(:attributes) do - { project: project, user: user, sha: sha, variables: variables, pipeline_config: pipeline_config } - end - where(:value) { [true, false] } with_them do - it 'returns the value of .contains_internal_include?' do - allow(pipeline_config).to receive(:contains_internal_include?).and_return(value) + it 'returns the value of .internal_include_prepended?' do + allow(pipeline_config).to receive(:internal_include_prepended?).and_return(value) - expect(subject.contains_internal_include?).to eq(value) + expect(subject.internal_include?).to eq(value) end end end context 'when pipeline_config is not provided' do + let(:pipeline_config) { nil } + it 'returns false' do - expect(subject.contains_internal_include?).to eq(false) + expect(subject.internal_include?).to eq(false) end end end diff --git a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb index 478b26e97cdad1787e62c3dcf1b3544346eb80a1..056a06337afec84438365d3d27510d74efbad327 100644 --- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb +++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb @@ -209,7 +209,30 @@ end end - context 'when total file count exceeds max_includes' do + describe 'max includes detection' do + shared_examples 'verifies max includes' do + context 'when total file count is equal to max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count) + end + + it 'adds the expected number of files to expandset' do + expect { process }.not_to raise_error + expect(context.expandset.count).to eq(expected_total_file_count) + end + end + + context 'when total file count exceeds max_includes' do + before do + allow(context).to receive(:max_includes).and_return(expected_total_file_count - 1) + end + + it 'raises error' do + expect { process }.to raise_error(expected_error_class) + end + end + end + context 'when files are nested' do let(:files) do [ @@ -217,9 +240,21 @@ ] end - it 'raises Processor::IncludeError' do - allow(context).to receive(:max_includes).and_return(1) - expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + let(:expected_total_file_count) { 4 } # Includes nested_configs.yml + 3 nested files + let(:expected_error_class) { Gitlab::Ci::Config::External::Processor::IncludeError } + + it_behaves_like 'verifies max includes' + + context 'when duplicate files are included' do + let(:expected_total_file_count) { 8 } # 2 x (Includes nested_configs.yml + 3 nested files) + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) + ] + end + + it_behaves_like 'verifies max includes' end end @@ -231,24 +266,163 @@ ] end - it 'raises Mapper::TooManyIncludesError' do - allow(context).to receive(:max_includes).and_return(1) - expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + let(:expected_total_file_count) { files.count } + let(:expected_error_class) { Gitlab::Ci::Config::External::Mapper::TooManyIncludesError } + + it_behaves_like 'verifies max includes' + + context 'when duplicate files are included' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file2.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file2.yml' }, context) + ] + end + + let(:expected_total_file_count) { files.count } + + it_behaves_like 'verifies max includes' end end - context 'when files are duplicates' do + context 'when there is a circular include' do + let(:project_files) do + { + 'myfolder/file1.yml' => <<~YAML + include: myfolder/file1.yml + YAML + } + end + let(:files) do [ - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), - Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context) ] end + before do + allow(context).to receive(:max_includes).and_return(10) + end + it 'raises error' do - allow(context).to receive(:max_includes).and_return(2) - expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + end + end + + context 'when a file is an internal include' do + let(:project_files) do + { + 'myfolder/file1.yml' => <<~YAML, + my_build: + script: echo Hello World + YAML + '.internal-include.yml' => <<~YAML + include: + - local: myfolder/file1.yml + YAML + } + end + + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: '.internal-include.yml' }, context) + ] + end + + let(:total_file_count) { 2 } # Includes .internal-include.yml + myfolder/file1.yml + let(:pipeline_config) { instance_double(Gitlab::Ci::ProjectConfig) } + + let(:context) do + Gitlab::Ci::Config::External::Context.new( + project: project, + user: user, + sha: project.commit.id, + pipeline_config: pipeline_config + ) + end + + before do + allow(pipeline_config).to receive(:internal_include_prepended?).and_return(true) + allow(context).to receive(:max_includes).and_return(1) + end + + context 'when total file count excluding internal include is equal to max_includes' do + it 'does not add the internal include to expandset' do + expect { process }.not_to raise_error + expect(context.expandset.count).to eq(total_file_count - 1) + expect(context.expandset.first.location).to eq('myfolder/file1.yml') + end + end + + context 'when total file count excluding internal include exceeds max_includes' do + let(:project_files) do + { + 'myfolder/file1.yml' => <<~YAML, + my_build: + script: echo Hello World + YAML + '.internal-include.yml' => <<~YAML + include: + - local: myfolder/file1.yml + - local: myfolder/file1.yml + YAML + } + end + + it 'raises error' do + expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + end + end + end + end + + context 'when FF ci_fix_max_includes is disabled' do + before do + stub_feature_flags(ci_fix_max_includes: false) + end + + context 'when total file count exceeds max_includes' do + context 'when files are nested' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'nested_configs.yml' }, context) + ] + end + + it 'raises Processor::IncludeError' do + allow(context).to receive(:max_includes).and_return(1) + expect { process }.to raise_error(Gitlab::Ci::Config::External::Processor::IncludeError) + end + end + + context 'when files are not nested' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file2.yml' }, context) + ] + end + + it 'raises Mapper::TooManyIncludesError' do + allow(context).to receive(:max_includes).and_return(1) + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + end + end + + context 'when files are duplicates' do + let(:files) do + [ + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context), + Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context) + ] + end + + it 'raises error' do + allow(context).to receive(:max_includes).and_return(2) + expect { process }.to raise_error(Gitlab::Ci::Config::External::Mapper::TooManyIncludesError) + end end end end diff --git a/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb index 434acfb52742e11a705b2c0548f4edb6553fce48..a9a5297229425eca07104d0a379691ca272132ca 100644 --- a/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/chain/config/content_spec.rb @@ -26,7 +26,7 @@ expect(pipeline.config_source).to eq 'bridge_source' expect(command.config_content).to eq 'the-yaml' - expect(command.pipeline_config.contains_internal_include?).to eq(false) + expect(command.pipeline_config.internal_include_prepended?).to eq(false) end end @@ -53,7 +53,7 @@ expect(pipeline.config_source).to eq 'repository_source' expect(pipeline.pipeline_config.content).to eq(config_content_result) expect(command.config_content).to eq(config_content_result) - expect(command.pipeline_config.contains_internal_include?).to eq(true) + expect(command.pipeline_config.internal_include_prepended?).to eq(true) end end @@ -73,7 +73,7 @@ expect(pipeline.config_source).to eq 'remote_source' expect(pipeline.pipeline_config.content).to eq(config_content_result) expect(command.config_content).to eq(config_content_result) - expect(command.pipeline_config.contains_internal_include?).to eq(true) + expect(command.pipeline_config.internal_include_prepended?).to eq(true) end end @@ -94,7 +94,7 @@ expect(pipeline.config_source).to eq 'external_project_source' expect(pipeline.pipeline_config.content).to eq(config_content_result) expect(command.config_content).to eq(config_content_result) - expect(command.pipeline_config.contains_internal_include?).to eq(true) + expect(command.pipeline_config.internal_include_prepended?).to eq(true) end context 'when path specifies a refname' do @@ -115,7 +115,7 @@ expect(pipeline.config_source).to eq 'external_project_source' expect(pipeline.pipeline_config.content).to eq(config_content_result) expect(command.config_content).to eq(config_content_result) - expect(command.pipeline_config.contains_internal_include?).to eq(true) + expect(command.pipeline_config.internal_include_prepended?).to eq(true) end end end @@ -143,7 +143,7 @@ expect(pipeline.config_source).to eq 'repository_source' expect(pipeline.pipeline_config.content).to eq(config_content_result) expect(command.config_content).to eq(config_content_result) - expect(command.pipeline_config.contains_internal_include?).to eq(true) + expect(command.pipeline_config.internal_include_prepended?).to eq(true) end end @@ -167,7 +167,7 @@ expect(pipeline.config_source).to eq 'auto_devops_source' expect(pipeline.pipeline_config.content).to eq(config_content_result) expect(command.config_content).to eq(config_content_result) - expect(command.pipeline_config.contains_internal_include?).to eq(true) + expect(command.pipeline_config.internal_include_prepended?).to eq(true) end end @@ -188,7 +188,7 @@ expect(pipeline.config_source).to eq 'parameter_source' expect(pipeline.pipeline_config.content).to eq(content) expect(command.config_content).to eq(content) - expect(command.pipeline_config.contains_internal_include?).to eq(false) + expect(command.pipeline_config.internal_include_prepended?).to eq(false) end end diff --git a/spec/lib/gitlab/ci/project_config/repository_spec.rb b/spec/lib/gitlab/ci/project_config/repository_spec.rb index b31a909934879bf981053c4007d925141b1b68f8..e8a997a7e43dda0a4de3538f3ce80c7599c1ff38 100644 --- a/spec/lib/gitlab/ci/project_config/repository_spec.rb +++ b/spec/lib/gitlab/ci/project_config/repository_spec.rb @@ -45,8 +45,8 @@ it { is_expected.to eq(:repository_source) } end - describe '#contains_internal_include?' do - subject { config.contains_internal_include? } + describe '#internal_include_prepended?' do + subject { config.internal_include_prepended? } it { is_expected.to eq(true) } end diff --git a/spec/lib/gitlab/ci/project_config/source_spec.rb b/spec/lib/gitlab/ci/project_config/source_spec.rb index 5248cf080e8df019aa8f3b0f24d060268eda104c..eefabe1babbb9fd214c96bfdabf0b44342ad866a 100644 --- a/spec/lib/gitlab/ci/project_config/source_spec.rb +++ b/spec/lib/gitlab/ci/project_config/source_spec.rb @@ -21,9 +21,9 @@ it { expect { source }.to raise_error(NotImplementedError) } end - describe '#contains_internal_include?' do - subject(:contains_internal_include) { custom_config.contains_internal_include? } + describe '#internal_include_prepended?' do + subject(:internal_include_prepended) { custom_config.internal_include_prepended? } - it { expect(contains_internal_include).to eq(false) } + it { expect(internal_include_prepended).to eq(false) } end end