Merge branch 'remove-additional-artifact-config-validation' into 'master'

Fix validation of dynamic child pipeline depenencies

See merge request gitlab-org/gitlab!28901

changelogs/unreleased/remove-additional-artifact-config-validation.yml

+---
+title: Validate dependency on job generating a CI config when using dynamic child pipelines
+merge_request: 28901
+author:
+type: added

doc/ci/parent_child_pipelines.md

@@ -136,12 +136,11 @@ your own script to generate a YAML file, which is then [used to trigger a child
 This technique can be very powerful in generating pipelines targeting content that changed or to
 build a matrix of targets and architectures.
 
+In GitLab 12.9, the child pipeline could fail to be created in certain cases, causing the parent pipeline to fail.
+This is [resolved in GitLab 12.10](https://gitlab.com/gitlab-org/gitlab/-/issues/209070).
+
 ## Limitations
 
 A parent pipeline can trigger many child pipelines, but a child pipeline cannot trigger
 further child pipelines. See the [related issue](https://gitlab.com/gitlab-org/gitlab/issues/29651)
 for discussion on possible future improvements.
-
-When triggering dynamic child pipelines, if the job containing the CI config artifact is not a predecessor of the
-trigger job, the child pipeline will fail to be created, causing also the parent pipeline to fail.
-In the future we want to validate the trigger job's dependencies [at the time the parent pipeline is created](https://gitlab.com/gitlab-org/gitlab/-/issues/209070) rather than when the child pipeline is created.

lib/gitlab/ci/config/entry/include.rb

@@ -15,6 +15,14 @@ module Gitlab
           validations do
             validates :config, hash_or_string: true
             validates :config, allowed_keys: ALLOWED_KEYS
+
+            validate do
+              next unless config.is_a?(Hash)
+
+              if config[:artifact] && config[:job].blank?
+                errors.add(:config, "must specify the job where to fetch the artifact from")
+              end
+            end
           end
         end
       end

lib/gitlab/ci/yaml_processor.rb

@@ -142,6 +142,7 @@ module Gitlab
           validate_job_stage!(name, job)
           validate_job_dependencies!(name, job)
           validate_job_needs!(name, job)
+          validate_dynamic_child_pipeline_dependencies!(name, job)
           validate_job_environment!(name, job)
         end
       end
@@ -163,37 +164,50 @@ module Gitlab
       def validate_job_dependencies!(name, job)
         return unless job[:dependencies]
 
-        stage_index = @stages.index(job[:stage])
-
         job[:dependencies].each do |dependency|
-          raise ValidationError, "#{name} job: undefined dependency: #{dependency}" unless @jobs[dependency.to_sym]
+          validate_job_dependency!(name, dependency)
+        end
+      end
 
-          dependency_stage_index = @stages.index(@jobs[dependency.to_sym][:stage])
+      def validate_dynamic_child_pipeline_dependencies!(name, job)
+        return unless includes = job.dig(:trigger, :include)
 
-          unless dependency_stage_index.present? && dependency_stage_index < stage_index
-            raise ValidationError, "#{name} job: dependency #{dependency} is not defined in prior stages"
-          end
+        Array(includes).each do |included|
+          next unless included.is_a?(Hash)
+          next unless dependency = included[:job]
+
+          validate_job_dependency!(name, dependency)
         end
       end
 
       def validate_job_needs!(name, job)
-        return unless job.dig(:needs, :job)
-
-        stage_index = @stages.index(job[:stage])
+        return unless needs = job.dig(:needs, :job)
 
-        job.dig(:needs, :job).each do |need|
-          need_job_name = need[:name]
+        needs.each do |need|
+          validate_job_dependency!(name, need[:name], 'need')
+        end
+      end
 
-          raise ValidationError, "#{name} job: undefined need: #{need_job_name}" unless @jobs[need_job_name.to_sym]
+      def validate_job_dependency!(name, dependency, dependency_type = 'dependency')
+        unless @jobs[dependency.to_sym]
+          raise ValidationError, "#{name} job: undefined #{dependency_type}: #{dependency}"
+        end
 
-          needs_stage_index = @stages.index(@jobs[need_job_name.to_sym][:stage])
+        job_stage_index = stage_index(name)
+        dependency_stage_index = stage_index(dependency)
 
-          unless needs_stage_index.present? && needs_stage_index < stage_index
-            raise ValidationError, "#{name} job: need #{need_job_name} is not defined in prior stages"
-          end
+        # A dependency might be defined later in the configuration
+        # with a stage that does not exist
+        unless dependency_stage_index.present? && dependency_stage_index < job_stage_index
+          raise ValidationError, "#{name} job: #{dependency_type} #{dependency} is not defined in prior stages"
         end
       end
 
+      def stage_index(name)
+        stage = @jobs.dig(name.to_sym, :stage)
+        @stages.index(stage)
+      end
+
       def validate_job_environment!(name, job)
         return unless job[:environment]
         return unless job[:environment].is_a?(Hash)

spec/lib/gitlab/ci/config/entry/include_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ::Gitlab::Ci::Config::Entry::Include do
+  subject(:include_entry) { described_class.new(config) }
+
+  describe 'validations' do
+    before do
+      include_entry.compose!
+    end
+
+    context 'when value is a string' do
+      let(:config) { 'test.yml' }
+
+      it { is_expected.to be_valid }
+    end
+
+    context 'when value is hash' do
+      context 'when using not allowed keys' do
+        let(:config) do
+          { not_allowed: 'key' }
+        end
+
+        it { is_expected.not_to be_valid }
+      end
+
+      context 'when using "local"' do
+        let(:config) { { local: 'test.yml' } }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'when using "file"' do
+        let(:config) { { file: 'test.yml' } }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'when using "template"' do
+        let(:config) { { template: 'test.yml' } }
+
+        it { is_expected.to be_valid }
+      end
+
+      context 'when using "artifact"' do
+        context 'and specifying "job"' do
+          let(:config) { { artifact: 'test.yml', job: 'generator' } }
+
+          it { is_expected.to be_valid }
+        end
+
+        context 'without "job"' do
+          let(:config) { { artifact: 'test.yml' } }
+
+          it { is_expected.not_to be_valid }
+
+          it 'has specific error' do
+            expect(include_entry.errors)
+              .to include('include config must specify the job where to fetch the artifact from')
+          end
+        end
+      end
+    end
+
+    context 'when value is something else' do
+      let(:config) { 123 }
+
+      it { is_expected.not_to be_valid }
+    end
+  end
+end

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -2052,6 +2052,54 @@ module Gitlab
         end
       end
 
+      describe 'with parent-child pipeline' do
+        context 'when artifact and job are specified' do
+          let(:config) do
+            YAML.dump({
+              build1: { stage: 'build', script: 'test' },
+              test1: { stage: 'test', trigger: {
+                include: [{ artifact: 'generated.yml', job: 'build1' }]
+              } }
+            })
+          end
+
+          it { expect { subject }.not_to raise_error }
+        end
+
+        context 'when job is not specified specified while artifact is' do
+          let(:config) do
+            YAML.dump({
+              build1: { stage: 'build', script: 'test' },
+              test1: { stage: 'test', trigger: {
+                include: [{ artifact: 'generated.yml' }]
+              } }
+            })
+          end
+
+          it do
+            expect { subject }.to raise_error(
+              described_class::ValidationError,
+              /include config must specify the job where to fetch the artifact from/)
+          end
+        end
+
+        context 'when include is a string' do
+          let(:config) do
+            YAML.dump({
+              build1: { stage: 'build', script: 'test' },
+              test1: {
+                stage: 'test',
+                trigger: {
+                  include: 'generated.yml'
+                }
+              }
+            })
+          end
+
+          it { expect { subject }.not_to raise_error }
+        end
+      end
+
       describe "Error handling" do
         it "fails to parse YAML" do
           expect do

spec/services/ci/create_pipeline_service/parent_child_pipeline_spec.rb

@@ -18,25 +18,10 @@ describe Ci::CreatePipelineService, '#execute' do
 
   before do
     project.add_developer(user)
-    stub_ci_pipeline_to_return_yaml_file
+    stub_ci_pipeline_yaml_file(config)
   end
 
-  describe 'child pipeline triggers' do
-    before do
-      stub_ci_pipeline_yaml_file <<~YAML
-        test:
-          script: rspec
-
-        deploy:
-          variables:
-            CROSS: downstream
-          stage: deploy
-          trigger:
-            include:
-              - local: path/to/child.yml
-      YAML
-    end
-
+  shared_examples 'successful creation' do
     it 'creates bridge jobs correctly' do
       pipeline = create_pipeline!
 
@@ -48,58 +33,140 @@ describe Ci::CreatePipelineService, '#execute' do
       expect(bridge).to be_a Ci::Bridge
       expect(bridge.stage).to eq 'deploy'
       expect(pipeline.statuses).to match_array [test, bridge]
-      expect(bridge.options).to eq(
-        'trigger' => { 'include' => [{ 'local' => 'path/to/child.yml' }] }
-      )
+      expect(bridge.options).to eq(expected_bridge_options)
       expect(bridge.yaml_variables)
         .to include(key: 'CROSS', value: 'downstream', public: true)
     end
   end
 
+  shared_examples 'creation failure' do
+    it 'returns errors' do
+      pipeline = create_pipeline!
+
+      expect(pipeline.errors.full_messages.first).to match(expected_error)
+      expect(pipeline.failure_reason).to eq 'config_error'
+      expect(pipeline).to be_persisted
+      expect(pipeline.status).to eq 'failed'
+    end
+  end
+
+  describe 'child pipeline triggers' do
+    let(:config) do
+      <<~YAML
+      test:
+        script: rspec
+      deploy:
+        variables:
+          CROSS: downstream
+        stage: deploy
+        trigger:
+          include:
+            - local: path/to/child.yml
+      YAML
+    end
+
+    it_behaves_like 'successful creation' do
+      let(:expected_bridge_options) do
+        {
+          'trigger' => {
+            'include' => [
+              { 'local' => 'path/to/child.yml' }
+            ]
+          }
+        }
+      end
+    end
+  end
+
   describe 'child pipeline triggers' do
     context 'when YAML is valid' do
-      before do
-        stub_ci_pipeline_yaml_file <<~YAML
+      let(:config) do
+        <<~YAML
+        test:
+          script: rspec
+        deploy:
+          variables:
+            CROSS: downstream
+          stage: deploy
+          trigger:
+            include:
+              - local: path/to/child.yml
+        YAML
+      end
+
+      it_behaves_like 'successful creation' do
+        let(:expected_bridge_options) do
+          {
+            'trigger' => {
+              'include' => [
+                { 'local' => 'path/to/child.yml' }
+              ]
+            }
+          }
+        end
+      end
+
+      context 'when trigger:include is specified as a string' do
+        let(:config) do
+          <<~YAML
           test:
             script: rspec
+          deploy:
+            variables:
+              CROSS: downstream
+            stage: deploy
+            trigger:
+              include: path/to/child.yml
+          YAML
+        end
+
+        it_behaves_like 'successful creation' do
+          let(:expected_bridge_options) do
+            {
+              'trigger' => {
+                'include' => 'path/to/child.yml'
+              }
+            }
+          end
+        end
+      end
 
+      context 'when trigger:include is specified as array of strings' do
+        let(:config) do
+          <<~YAML
+          test:
+            script: rspec
           deploy:
             variables:
               CROSS: downstream
             stage: deploy
             trigger:
               include:
-                - local: path/to/child.yml
-        YAML
-      end
+                - path/to/child.yml
+                - path/to/child2.yml
+          YAML
+        end
 
-      it 'creates bridge jobs correctly' do
-        pipeline = create_pipeline!
-
-        test = pipeline.statuses.find_by(name: 'test')
-        bridge = pipeline.statuses.find_by(name: 'deploy')
-
-        expect(pipeline).to be_persisted
-        expect(test).to be_a Ci::Build
-        expect(bridge).to be_a Ci::Bridge
-        expect(bridge.stage).to eq 'deploy'
-        expect(pipeline.statuses).to match_array [test, bridge]
-        expect(bridge.options).to eq(
-          'trigger' => { 'include' => [{ 'local' => 'path/to/child.yml' }] }
-        )
-        expect(bridge.yaml_variables)
-          .to include(key: 'CROSS', value: 'downstream', public: true)
+        it_behaves_like 'successful creation' do
+          let(:expected_bridge_options) do
+            {
+              'trigger' => {
+                'include' => ['path/to/child.yml', 'path/to/child2.yml']
+              }
+            }
+          end
+        end
       end
     end
 
-    context 'when YAML is invalid' do
+    context 'when limit of includes is reached' do
       let(:config) do
-        {
+        YAML.dump({
           test: { script: 'rspec' },
           deploy: {
             trigger: { include: included_files }
           }
-        }
+        })
       end
 
       let(:included_files) do
@@ -112,17 +179,46 @@ describe Ci::CreatePipelineService, '#execute' do
         Gitlab::Ci::Config::Entry::Trigger::ComplexTrigger::SameProjectTrigger::INCLUDE_MAX_SIZE
       end
 
-      before do
-        stub_ci_pipeline_yaml_file(YAML.dump(config))
+      it_behaves_like 'creation failure' do
+        let(:expected_error) { /trigger:include config is too long/ }
       end
+    end
 
-      it 'returns errors' do
-        pipeline = create_pipeline!
+    context 'when including configs from artifact' do
+      context 'when specified dependency is in the wrong order' do
+        let(:config) do
+          <<~YAML
+          test:
+            trigger:
+              include:
+                - job: generator
+                  artifact: 'generated.yml'
+          generator:
+            stage: 'deploy'
+            script: 'generator'
+          YAML
+        end
 
-        expect(pipeline.errors.full_messages.first).to match(/trigger:include config is too long/)
-        expect(pipeline.failure_reason).to eq 'config_error'
-        expect(pipeline).to be_persisted
-        expect(pipeline.status).to eq 'failed'
+        it_behaves_like 'creation failure' do
+          let(:expected_error) { /test job: dependency generator is not defined in prior stages/ }
+        end
+      end
+
+      context 'when specified dependency is missing :job key' do
+        let(:config) do
+          <<~YAML
+          test:
+            trigger:
+              include:
+                - artifact: 'generated.yml'
+          YAML
+        end
+
+        it_behaves_like 'creation failure' do
+          let(:expected_error) do
+            /include config must specify the job where to fetch the artifact from/
+          end
+        end
       end
     end
   end