Fix workflow:rules not accessing passed-upstream and trigger variables

This is the second attempt to fix this problem.

In "Seed" section of CreatePipelineService, we run the block that
is passed when calling the main service. This block is used for adding
the variables to the pipeline from upstream and trigger requests.

In this commit, we start to run this block before evaluating
the workflow rules.

This fix is behind a FF ci_seed_block_run_before_workflow_rules.

app/services/ci/create_pipeline_service.rb

@@ -14,6 +14,7 @@ module Ci
                 Gitlab::Ci::Pipeline::Chain::Config::Process,
                 Gitlab::Ci::Pipeline::Chain::RemoveUnwantedChatJobs,
                 Gitlab::Ci::Pipeline::Chain::Skip,
+                Gitlab::Ci::Pipeline::Chain::SeedBlock,
                 Gitlab::Ci::Pipeline::Chain::EvaluateWorkflowRules,
                 Gitlab::Ci::Pipeline::Chain::Seed,
                 Gitlab::Ci::Pipeline::Chain::Limit::Size,

config/feature_flags/development/ci_seed_block_run_before_workflow_rules.yml

+---
+name: ci_seed_block_run_before_workflow_rules
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45674
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/270439
+type: development
+group: group::pipeline authoring
+default_enabled: false

lib/gitlab/ci/features.rb

@@ -62,6 +62,10 @@ module Gitlab
       def self.manual_bridges_enabled?(project)
         ::Feature.enabled?(:ci_manual_bridges, project, default_enabled: true)
       end
+
+      def self.seed_block_run_before_workflow_rules_enabled?(project)
+        ::Feature.enabled?(:ci_seed_block_run_before_workflow_rules, project, default_enabled: false)
+      end
     end
   end
 end

lib/gitlab/ci/pipeline/chain/seed.rb

@@ -19,10 +19,12 @@ module Gitlab
             # Build to prevent erroring out on ambiguous refs.
             pipeline.protected = @command.protected_ref?
 
-            ##
-            # Populate pipeline with block argument of CreatePipelineService#execute.
-            #
-            @command.seeds_block&.call(pipeline)
+            unless ::Gitlab::Ci::Features.seed_block_run_before_workflow_rules_enabled?(project)
+              ##
+              # Populate pipeline with block argument of CreatePipelineService#execute.
+              #
+              @command.seeds_block&.call(pipeline)
+            end
 
             ##
             # Gather all runtime build/stage errors

lib/gitlab/ci/pipeline/chain/seed_block.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class SeedBlock < Chain::Base
+          include Chain::Helpers
+          include Gitlab::Utils::StrongMemoize
+
+          def perform!
+            return unless ::Gitlab::Ci::Features.seed_block_run_before_workflow_rules_enabled?(project)
+
+            ##
+            # Populate pipeline with block argument of CreatePipelineService#execute.
+            #
+            @command.seeds_block&.call(pipeline)
+
+            raise "Pipeline cannot be persisted by `seeds_block`" if pipeline.persisted?
+          end
+
+          def break?
+            return false unless ::Gitlab::Ci::Features.seed_block_run_before_workflow_rules_enabled?(project)
+
+            pipeline.errors.any?
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/pipeline/chain/populate_spec.rb

@@ -22,6 +22,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Populate do
     [
       Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
       Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command),
+      Gitlab::Ci::Pipeline::Chain::SeedBlock.new(pipeline, command),
       Gitlab::Ci::Pipeline::Chain::Seed.new(pipeline, command)
     ]
   end
@@ -180,23 +181,21 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Populate do
         ->(pipeline) { pipeline.variables.create!(key: 'VAR', value: '123') }
       end
 
-      it 'wastes pipeline iid' do
-        expect { run_chain }.to raise_error(ActiveRecord::RecordNotSaved)
-
-        last_iid = InternalId.ci_pipelines
-          .where(project_id: project.id)
-          .last.last_value
-
-        expect(last_iid).to be > 0
+      it 'raises error' do
+        expect { run_chain }.to raise_error(ActiveRecord::RecordNotSaved,
+                                            'You cannot call create unless the parent is saved')
       end
     end
   end
 
   context 'when pipeline gets persisted during the process' do
-    let(:pipeline) { create(:ci_pipeline, project: project) }
+    before do
+      dependencies.each(&:perform!)
+      pipeline.save!
+    end
 
     it 'raises error' do
-      expect { run_chain }.to raise_error(described_class::PopulateError)
+      expect { step.perform! }.to raise_error(described_class::PopulateError)
     end
   end
 

spec/lib/gitlab/ci/pipeline/chain/seed_block_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Pipeline::Chain::SeedBlock do
+  let(:project) { create(:project, :repository) }
+  let(:user) { create(:user, developer_projects: [project]) }
+  let(:seeds_block) { }
+
+  let(:command) do
+    Gitlab::Ci::Pipeline::Chain::Command.new(
+      project: project,
+      current_user: user,
+      origin_ref: 'master',
+      seeds_block: seeds_block)
+  end
+
+  let(:pipeline) { build(:ci_pipeline, project: project) }
+
+  describe '#perform!' do
+    before do
+      stub_ci_pipeline_yaml_file(YAML.dump(config))
+    end
+
+    subject(:run_chain) do
+      [
+        Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
+        Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command)
+      ].map(&:perform!)
+
+      described_class.new(pipeline, command).perform!
+    end
+
+    let(:config) do
+      { rspec: { script: 'rake' } }
+    end
+
+    context 'when there is not seeds_block' do
+      it 'does nothing' do
+        expect { run_chain }.not_to raise_error
+      end
+    end
+
+    context 'when there is seeds_block' do
+      let(:seeds_block) do
+        ->(pipeline) { pipeline.variables.build(key: 'VAR', value: '123') }
+      end
+
+      it 'executes the block' do
+        run_chain
+
+        expect(pipeline.variables.size).to eq(1)
+      end
+
+      context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+        before do
+          stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+        end
+
+        it 'does not execute the block' do
+          run_chain
+
+          expect(pipeline.variables.size).to eq(0)
+        end
+      end
+    end
+
+    context 'when the seeds_block tries to save the pipelie' do
+      let(:seeds_block) do
+        ->(pipeline) { pipeline.save! }
+      end
+
+      it 'raises error' do
+        expect { run_chain }.to raise_error('Pipeline cannot be persisted by `seeds_block`')
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/pipeline/chain/seed_spec.rb

@@ -5,22 +5,14 @@ require 'spec_helper'
 RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user, developer_projects: [project]) }
+  let(:seeds_block) { }
 
   let(:command) do
     Gitlab::Ci::Pipeline::Chain::Command.new(
       project: project,
       current_user: user,
       origin_ref: 'master',
-      seeds_block: nil)
-  end
-
-  def run_chain(pipeline, command)
-    [
-      Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
-      Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command)
-    ].map(&:perform!)
-
-    described_class.new(pipeline, command).perform!
+      seeds_block: seeds_block)
   end
 
   let(:pipeline) { build(:ci_pipeline, project: project) }
@@ -28,22 +20,36 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
   describe '#perform!' do
     before do
       stub_ci_pipeline_yaml_file(YAML.dump(config))
-      run_chain(pipeline, command)
     end
 
     let(:config) do
       { rspec: { script: 'rake' } }
     end
 
+    subject(:run_chain) do
+      [
+        Gitlab::Ci::Pipeline::Chain::Config::Content.new(pipeline, command),
+        Gitlab::Ci::Pipeline::Chain::Config::Process.new(pipeline, command)
+      ].map(&:perform!)
+
+      described_class.new(pipeline, command).perform!
+    end
+
     it 'allocates next IID' do
+      run_chain
+
       expect(pipeline.iid).to be_present
     end
 
     it 'ensures ci_ref' do
+      run_chain
+
       expect(pipeline.ci_ref).to be_present
     end
 
     it 'sets the seeds in the command object' do
+      run_chain
+
       expect(command.stage_seeds).to all(be_a Gitlab::Ci::Pipeline::Seed::Base)
       expect(command.stage_seeds.count).to eq 1
     end
@@ -58,6 +64,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'correctly fabricates a stage seeds object' do
+        run_chain
+
         seeds = command.stage_seeds
         expect(seeds.size).to eq 2
         expect(seeds.first.attributes[:name]).to eq 'test'
@@ -81,6 +89,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns stage seeds only assigned to master' do
+        run_chain
+
         seeds = command.stage_seeds
 
         expect(seeds.size).to eq 1
@@ -100,6 +110,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns stage seeds only assigned to schedules' do
+        run_chain
+
         seeds = command.stage_seeds
 
         expect(seeds.size).to eq 1
@@ -127,6 +139,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
           let(:pipeline) { build(:ci_pipeline, project: project) }
 
           it 'returns seeds for kubernetes dependent job' do
+            run_chain
+
             seeds = command.stage_seeds
 
             expect(seeds.size).to eq 2
@@ -138,6 +152,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
 
       context 'when kubernetes is not active' do
         it 'does not return seeds for kubernetes dependent job' do
+          run_chain
+
           seeds = command.stage_seeds
 
           expect(seeds.size).to eq 1
@@ -155,11 +171,39 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::Seed do
       end
 
       it 'returns stage seeds only when variables expression is truthy' do
+        run_chain
+
         seeds = command.stage_seeds
 
         expect(seeds.size).to eq 1
         expect(seeds.dig(0, 0, :name)).to eq 'unit'
       end
     end
+
+    context 'when there is seeds_block' do
+      let(:seeds_block) do
+        ->(pipeline) { pipeline.variables.build(key: 'VAR', value: '123') }
+      end
+
+      context 'when FF ci_seed_block_run_before_workflow_rules is enabled' do
+        it 'does not execute the block' do
+          run_chain
+
+          expect(pipeline.variables.size).to eq(0)
+        end
+      end
+
+      context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+        before do
+          stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+        end
+
+        it 'executes the block' do
+          run_chain
+
+          expect(pipeline.variables.size).to eq(1)
+        end
+      end
+    end
   end
 end

spec/services/ci/create_downstream_pipeline_service_spec.rb

@@ -581,5 +581,40 @@ RSpec.describe Ci::CreateDownstreamPipelineService, '#execute' do
         )
       end
     end
+
+    context 'when downstream pipeline has workflow rule' do
+      before do
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      let(:config) do
+        <<-EOY
+          workflow:
+            rules:
+              - if: $my_var
+
+          regular-job:
+            script: 'echo Hello, World!'
+        EOY
+      end
+
+      context 'when passing the required variable' do
+        before do
+          bridge.yaml_variables = [{ key: 'my_var', value: 'var', public: true }]
+        end
+
+        it 'creates the pipeline' do
+          expect { service.execute(bridge) }.to change(downstream_project.ci_pipelines, :count).by(1)
+
+          expect(bridge.reload).to be_success
+        end
+      end
+
+      context 'when not passing the required variable' do
+        it 'does not create the pipeline' do
+          expect { service.execute(bridge) }.not_to change(downstream_project.ci_pipelines, :count)
+        end
+      end
+    end
   end
 end

spec/services/ci/create_pipeline_service_spec.rb

@@ -41,7 +41,9 @@ RSpec.describe Ci::CreatePipelineService do
         save_on_errors: save_on_errors,
         trigger_request: trigger_request,
         merge_request: merge_request,
-        external_pull_request: external_pull_request)
+        external_pull_request: external_pull_request) do |pipeline|
+        yield(pipeline) if block_given?
+      end
     end
     # rubocop:enable Metrics/ParameterLists
 
@@ -2274,6 +2276,207 @@ RSpec.describe Ci::CreatePipelineService do
           end
         end
       end
+
+      context 'with workflow rules with persisted variables' do
+        let(:config) do
+          <<-EOY
+            workflow:
+              rules:
+                - if: $CI_COMMIT_REF_NAME == "master"
+
+            regular-job:
+              script: 'echo Hello, World!'
+          EOY
+        end
+
+        context 'with matches' do
+          it 'creates a pipeline' do
+            expect(pipeline).to be_persisted
+            expect(build_names).to contain_exactly('regular-job')
+          end
+        end
+
+        context 'with no matches' do
+          let(:ref_name) { 'refs/heads/feature' }
+
+          it 'does not create a pipeline' do
+            expect(pipeline).not_to be_persisted
+          end
+        end
+      end
+
+      context 'with workflow rules with pipeline variables' do
+        let(:pipeline) do
+          execute_service(variables_attributes: variables_attributes)
+        end
+
+        let(:config) do
+          <<-EOY
+            workflow:
+              rules:
+                - if: $SOME_VARIABLE
+
+            regular-job:
+              script: 'echo Hello, World!'
+          EOY
+        end
+
+        context 'with matches' do
+          let(:variables_attributes) do
+            [{ key: 'SOME_VARIABLE', secret_value: 'SOME_VAR' }]
+          end
+
+          it 'creates a pipeline' do
+            expect(pipeline).to be_persisted
+            expect(build_names).to contain_exactly('regular-job')
+          end
+        end
+
+        context 'with no matches' do
+          let(:variables_attributes) { {} }
+
+          it 'does not create a pipeline' do
+            expect(pipeline).not_to be_persisted
+          end
+        end
+      end
+
+      context 'with workflow rules with trigger variables' do
+        let(:pipeline) do
+          execute_service do |pipeline|
+            pipeline.variables.build(variables)
+          end
+        end
+
+        let(:config) do
+          <<-EOY
+            workflow:
+              rules:
+                - if: $SOME_VARIABLE
+
+            regular-job:
+              script: 'echo Hello, World!'
+          EOY
+        end
+
+        context 'with matches' do
+          let(:variables) do
+            [{ key: 'SOME_VARIABLE', secret_value: 'SOME_VAR' }]
+          end
+
+          it 'creates a pipeline' do
+            expect(pipeline).to be_persisted
+            expect(build_names).to contain_exactly('regular-job')
+          end
+
+          context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+            before do
+              stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+            end
+
+            it 'does not a pipeline' do
+              expect(pipeline).not_to be_persisted
+            end
+          end
+
+          context 'when a job requires the same variable' do
+            let(:config) do
+              <<-EOY
+                workflow:
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                build:
+                  stage: build
+                  script: 'echo build'
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                test1:
+                  stage: test
+                  script: 'echo test1'
+                  needs: [build]
+
+                test2:
+                  stage: test
+                  script: 'echo test2'
+              EOY
+            end
+
+            it 'creates a pipeline' do
+              expect(pipeline).to be_persisted
+              expect(build_names).to contain_exactly('build', 'test1', 'test2')
+            end
+
+            context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+              before do
+                stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+              end
+
+              it 'does not a pipeline' do
+                expect(pipeline).not_to be_persisted
+              end
+            end
+          end
+        end
+
+        context 'with no matches' do
+          let(:variables) { {} }
+
+          it 'does not create a pipeline' do
+            expect(pipeline).not_to be_persisted
+          end
+
+          context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+            before do
+              stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+            end
+
+            it 'does not create a pipeline' do
+              expect(pipeline).not_to be_persisted
+            end
+          end
+
+          context 'when a job requires the same variable' do
+            let(:config) do
+              <<-EOY
+                workflow:
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                build:
+                  stage: build
+                  script: 'echo build'
+                  rules:
+                    - if: $SOME_VARIABLE
+
+                test1:
+                  stage: test
+                  script: 'echo test1'
+                  needs: [build]
+
+                test2:
+                  stage: test
+                  script: 'echo test2'
+              EOY
+            end
+
+            it 'does not create a pipeline' do
+              expect(pipeline).not_to be_persisted
+            end
+
+            context 'when FF ci_seed_block_run_before_workflow_rules is disabled' do
+              before do
+                stub_feature_flags(ci_seed_block_run_before_workflow_rules: false)
+              end
+
+              it 'does not create a pipeline' do
+                expect(pipeline).not_to be_persisted
+              end
+            end
+          end
+        end
+      end
     end
   end