From a8e0351950ac0ecf0ab64601f1c86042b275cec6 Mon Sep 17 00:00:00 2001
From: lma-git <lma@gitlab.com>
Date: Fri, 24 Feb 2023 14:39:29 -0800
Subject: [PATCH 1/5] Fix to prevent unlimited nested includes

This MR fixes a bug that allows unlimited nested includes.
The Verifier operations were rearranged so that all relevant
files are added to the structure we use for counting the
includes (expandset).
---
 .../development/ci_fix_max_includes.yml       |   8 +
 .../ci/config/external/mapper/verifier.rb     |  17 +-
 .../gitlab/ci/config/external/context_spec.rb |  20 +-
 .../config/external/mapper/verifier_spec.rb   | 197 ++++++++++++++++--
 4 files changed, 220 insertions(+), 22 deletions(-)
 create mode 100644 config/feature_flags/development/ci_fix_max_includes.yml

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 00000000000000..ef993f4f7eed2d
--- /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/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb
index 6c2e52958da847..0c323b9a07a555 100644
--- a/lib/gitlab/ci/config/external/mapper/verifier.rb
+++ b/lib/gitlab/ci/config/external/mapper/verifier.rb
@@ -44,6 +44,12 @@ def process_without_instrumentation(files)
 
             def legacy_process_without_instrumentation(files)
               files.each do |file|
+                if ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
+                  # Do not add the internal include (if present, it would be the only file at the top level)
+                  context.expandset << file unless context.contains_internal_include?
+                  verify_max_includes!
+                end
+
                 verify_execution_time!
 
                 file.validate_location!
@@ -54,19 +60,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 ::Gitlab::Ci::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 ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
               end
             end
 
             def verify_max_includes!
-              return if context.expandset.count < context.max_includes
+              if ::Gitlab::Ci::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/spec/lib/gitlab/ci/config/external/context_spec.rb b/spec/lib/gitlab/ci/config/external/context_spec.rb
index f1640822c6bce0..917db25c3b4c4d 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
 
@@ -146,11 +157,6 @@
 
   describe '#contains_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
@@ -163,6 +169,8 @@
     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)
       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 478b26e97cdad1..5fafcbbead0def 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 }
+          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,162 @@
           ]
         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(:expected_total_file_count) { 3 }
+          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
+
+          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 config contains 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(:contains_internal_include?).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
-- 
GitLab


From 0d6484610a8eb0300a5cc9e6845961513e32e90c Mon Sep 17 00:00:00 2001
From: lma-git <lma@gitlab.com>
Date: Wed, 1 Mar 2023 09:47:52 -0800
Subject: [PATCH 2/5] Rename method in context to is_internal_include

---
 lib/gitlab/ci/config/external/context.rb        |  4 +++-
 .../ci/config/external/mapper/verifier.rb       | 17 ++++++++++++-----
 .../gitlab/ci/config/external/context_spec.rb   |  6 +++---
 .../ci/config/external/mapper/verifier_spec.rb  |  7 ++++---
 4 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/lib/gitlab/ci/config/external/context.rb b/lib/gitlab/ci/config/external/context.rb
index 881b131c25209f..1d645c34478436 100644
--- a/lib/gitlab/ci/config/external/context.rb
+++ b/lib/gitlab/ci/config/external/context.rb
@@ -94,9 +94,11 @@ def includes
 
           # 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?
+          # rubocop: disable Naming/PredicateName
+          def is_internal_include?
             !!pipeline_config&.contains_internal_include?
           end
+          # rubocop: enable Naming/PredicateName
 
           protected
 
diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb
index 0c323b9a07a555..ba1179753205a8 100644
--- a/lib/gitlab/ci/config/external/mapper/verifier.rb
+++ b/lib/gitlab/ci/config/external/mapper/verifier.rb
@@ -9,12 +9,19 @@ 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 ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
+                  # We do not add internal include into expandset because we use it to calculate total file count
+                  context.expandset << file unless context.is_internal_include?
+                  verify_max_includes!
+                end
+
                 verify_execution_time!
 
                 file.validate_location!
@@ -31,22 +38,22 @@ 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 ::Gitlab::Ci::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 ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
               end
             end
+            # rubocop: enable Metrics/CyclomaticComplexity
 
             def legacy_process_without_instrumentation(files)
               files.each do |file|
                 if ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
-                  # Do not add the internal include (if present, it would be the only file at the top level)
-                  context.expandset << file unless context.contains_internal_include?
+                  # We do not add internal include into expandset because we use it to calculate total file count
+                  context.expandset << file unless context.is_internal_include?
                   verify_max_includes!
                 end
 
diff --git a/spec/lib/gitlab/ci/config/external/context_spec.rb b/spec/lib/gitlab/ci/config/external/context_spec.rb
index 917db25c3b4c4d..7f3eb00bdcbaab 100644
--- a/spec/lib/gitlab/ci/config/external/context_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/context_spec.rb
@@ -155,7 +155,7 @@
     it { expect(subject.sentry_payload).to match(a_hash_including(:project, :user)) }
   end
 
-  describe '#contains_internal_include?' do
+  describe '#is_internal_include?' do
     context 'when pipeline_config is provided' do
       where(:value) { [true, false] }
 
@@ -163,7 +163,7 @@
         it 'returns the value of .contains_internal_include?' do
           allow(pipeline_config).to receive(:contains_internal_include?).and_return(value)
 
-          expect(subject.contains_internal_include?).to eq(value)
+          expect(subject.is_internal_include?).to eq(value)
         end
       end
     end
@@ -172,7 +172,7 @@
       let(:pipeline_config) { nil }
 
       it 'returns false' do
-        expect(subject.contains_internal_include?).to eq(false)
+        expect(subject.is_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 5fafcbbead0def..9860b1f07446a3 100644
--- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb
@@ -246,7 +246,7 @@
         it_behaves_like 'verifies max includes'
 
         context 'when duplicate files are included' do
-          let(:expected_total_file_count) { 8 }
+          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),
@@ -272,7 +272,6 @@
         it_behaves_like 'verifies max includes'
 
         context 'when duplicate files are included' do
-          let(:expected_total_file_count) { 3 }
           let(:files) do
             [
               Gitlab::Ci::Config::External::File::Local.new({ local: 'myfolder/file1.yml' }, context),
@@ -281,6 +280,8 @@
             ]
           end
 
+          let(:expected_total_file_count) { files.count }
+
           it_behaves_like 'verifies max includes'
         end
       end
@@ -309,7 +310,7 @@
         end
       end
 
-      context 'when config contains internal include' do
+      context 'when a file is an internal include' do
         let(:project_files) do
           {
             'myfolder/file1.yml' => <<~YAML,
-- 
GitLab


From c583ff7bf169c30eefb74aec0dc56186fc96f313 Mon Sep 17 00:00:00 2001
From: lma-git <lma@gitlab.com>
Date: Wed, 1 Mar 2023 10:11:13 -0800
Subject: [PATCH 3/5] Workaround to pass FF usage check

---
 lib/gitlab/ci/config/external/mapper/verifier.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb
index ba1179753205a8..669710cc5bef92 100644
--- a/lib/gitlab/ci/config/external/mapper/verifier.rb
+++ b/lib/gitlab/ci/config/external/mapper/verifier.rb
@@ -78,7 +78,8 @@ def legacy_process_without_instrumentation(files)
             end
 
             def verify_max_includes!
-              if ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
+              if ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) ||
+                  (::Feature.enabled?(:ci_fix_max_includes) && false) # rubocop: disable Lint/LiteralAsCondition # Temp workaround pass FF usage test
                 return if context.expandset.count <= context.max_includes
               else
                 return if context.expandset.count < context.max_includes # rubocop:disable Style/IfInsideElse
-- 
GitLab


From dd806dba7e6950f618d33f7460e933b2e1ad81e3 Mon Sep 17 00:00:00 2001
From: lma-git <lma@gitlab.com>
Date: Thu, 2 Mar 2023 09:32:48 -0800
Subject: [PATCH 4/5] Change class name to pass FF usage test

---
 lib/gitlab/ci/config/external/mapper/verifier.rb | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb
index 669710cc5bef92..dd87449dc9f197 100644
--- a/lib/gitlab/ci/config/external/mapper/verifier.rb
+++ b/lib/gitlab/ci/config/external/mapper/verifier.rb
@@ -16,7 +16,7 @@ def process_without_instrumentation(files)
               end
 
               files.each do |file|
-                if ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
+                if YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
                   # We do not add internal include into expandset because we use it to calculate total file count
                   context.expandset << file unless context.is_internal_include?
                   verify_max_includes!
@@ -38,20 +38,20 @@ 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
-                verify_max_includes! unless ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_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 unless ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
+                context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
               end
             end
             # rubocop: enable Metrics/CyclomaticComplexity
 
             def legacy_process_without_instrumentation(files)
               files.each do |file|
-                if ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
+                if YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
                   # We do not add internal include into expandset because we use it to calculate total file count
                   context.expandset << file unless context.is_internal_include?
                   verify_max_includes!
@@ -67,19 +67,18 @@ 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
-                verify_max_includes! unless ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_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 unless ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
+                context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
               end
             end
 
             def verify_max_includes!
-              if ::Gitlab::Ci::YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) ||
-                  (::Feature.enabled?(:ci_fix_max_includes) && false) # rubocop: disable Lint/LiteralAsCondition # Temp workaround pass FF usage test
+              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
-- 
GitLab


From 5cbb9212e97856b586692062ebb231db36fa6d1c Mon Sep 17 00:00:00 2001
From: lma-git <lma@gitlab.com>
Date: Thu, 2 Mar 2023 10:22:24 -0800
Subject: [PATCH 5/5] Rename contains_internal_include to
 internal_include_prepended

Renaming the internal_include method to better clarify
what's happening. Also updating the comments to be
more descriptive.
---
 ee/lib/gitlab/ci/project_config/compliance.rb    |  2 +-
 .../ci/pipeline/chain/config/content_spec.rb     |  2 +-
 lib/gitlab/ci/config/external/context.rb         | 11 +++++------
 lib/gitlab/ci/config/external/mapper/verifier.rb | 14 ++++++++------
 lib/gitlab/ci/project_config.rb                  |  2 +-
 lib/gitlab/ci/project_config/auto_devops.rb      |  2 +-
 lib/gitlab/ci/project_config/external_project.rb |  2 +-
 lib/gitlab/ci/project_config/remote.rb           |  2 +-
 lib/gitlab/ci/project_config/repository.rb       |  2 +-
 lib/gitlab/ci/project_config/source.rb           |  4 ++--
 .../gitlab/ci/config/external/context_spec.rb    | 10 +++++-----
 .../ci/config/external/mapper/verifier_spec.rb   |  2 +-
 .../ci/pipeline/chain/config/content_spec.rb     | 16 ++++++++--------
 .../gitlab/ci/project_config/repository_spec.rb  |  4 ++--
 spec/lib/gitlab/ci/project_config/source_spec.rb |  6 +++---
 15 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/ee/lib/gitlab/ci/project_config/compliance.rb b/ee/lib/gitlab/ci/project_config/compliance.rb
index ad0594724f14a9..b2771f38881096 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 558dc6a8b5caf6..d2453e95f7704b 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 1d645c34478436..156109a084d69b 100644
--- a/lib/gitlab/ci/config/external/context.rb
+++ b/lib/gitlab/ci/config/external/context.rb
@@ -92,13 +92,12 @@ 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.
-          # rubocop: disable Naming/PredicateName
-          def is_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
-          # rubocop: enable Naming/PredicateName
 
           protected
 
diff --git a/lib/gitlab/ci/config/external/mapper/verifier.rb b/lib/gitlab/ci/config/external/mapper/verifier.rb
index dd87449dc9f197..7284d2a7e01b02 100644
--- a/lib/gitlab/ci/config/external/mapper/verifier.rb
+++ b/lib/gitlab/ci/config/external/mapper/verifier.rb
@@ -17,8 +17,9 @@ def process_without_instrumentation(files)
 
               files.each do |file|
                 if YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
-                  # We do not add internal include into expandset because we use it to calculate total file count
-                  context.expandset << file unless context.is_internal_include?
+                  # 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
 
@@ -44,7 +45,7 @@ def process_without_instrumentation(files)
                 file.validate_content! if file.valid?
                 file.load_and_validate_expanded_hash! if file.valid?
 
-                context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
+                context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
               end
             end
             # rubocop: enable Metrics/CyclomaticComplexity
@@ -52,8 +53,9 @@ def process_without_instrumentation(files)
             def legacy_process_without_instrumentation(files)
               files.each do |file|
                 if YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
-                  # We do not add internal include into expandset because we use it to calculate total file count
-                  context.expandset << file unless context.is_internal_include?
+                  # 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
 
@@ -73,7 +75,7 @@ def legacy_process_without_instrumentation(files)
                 file.validate_content! if file.valid?
                 file.load_and_validate_expanded_hash! if file.valid?
 
-                context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes) # rubocop:disable Layout/LineLength
+                context.expandset << file unless YamlProcessor::FeatureFlags.enabled?(:ci_fix_max_includes)
               end
             end
 
diff --git a/lib/gitlab/ci/project_config.rb b/lib/gitlab/ci/project_config.rb
index e69efb85a937f6..00b2ad58428e0f 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 70f8c7b9bb3f23..c5f010ebaeac52 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 b29825514585cb..0afdab23886d77 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 17d5a9ee68fd96..19cbf8e9c1ea74 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 c975023d35e27a..272425fd5465d8 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 30edf1b552a5bb..9a4a6394fa1ff1 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 7f3eb00bdcbaab..fc68d3d62a25f5 100644
--- a/spec/lib/gitlab/ci/config/external/context_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/context_spec.rb
@@ -155,15 +155,15 @@
     it { expect(subject.sentry_payload).to match(a_hash_including(:project, :user)) }
   end
 
-  describe '#is_internal_include?' do
+  describe '#internal_include?' do
     context 'when pipeline_config is provided' do
       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.is_internal_include?).to eq(value)
+          expect(subject.internal_include?).to eq(value)
         end
       end
     end
@@ -172,7 +172,7 @@
       let(:pipeline_config) { nil }
 
       it 'returns false' do
-        expect(subject.is_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 9860b1f07446a3..056a06337afec8 100644
--- a/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb
+++ b/spec/lib/gitlab/ci/config/external/mapper/verifier_spec.rb
@@ -343,7 +343,7 @@
         end
 
         before do
-          allow(pipeline_config).to receive(:contains_internal_include?).and_return(true)
+          allow(pipeline_config).to receive(:internal_include_prepended?).and_return(true)
           allow(context).to receive(:max_includes).and_return(1)
         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 434acfb52742e1..a9a5297229425e 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 b31a909934879b..e8a997a7e43dda 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 5248cf080e8df0..eefabe1babbb9f 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
-- 
GitLab