Merge branch 'fix/gb/fix-pipeline-build-chain-tag-evaluation' into 'master'

Fix invalid pipeline build chain tag evaluation

Closes #40944

See merge request gitlab-org/gitlab-ce!15805

app/services/ci/create_pipeline_service.rb

@@ -12,18 +12,19 @@ module Ci
     def execute(source, ignore_skip_ci: false, save_on_errors: true, trigger_request: nil, schedule: nil, &block)
       @pipeline = Ci::Pipeline.new
 
-      command = OpenStruct.new(source: source,
-                               origin_ref: params[:ref],
-                               checkout_sha: params[:checkout_sha],
-                               after_sha: params[:after],
-                               before_sha: params[:before],
-                               trigger_request: trigger_request,
-                               schedule: schedule,
-                               ignore_skip_ci: ignore_skip_ci,
-                               save_incompleted: save_on_errors,
-                               seeds_block: block,
-                               project: project,
-                               current_user: current_user)
+      command = Gitlab::Ci::Pipeline::Chain::Command.new(
+        source: source,
+        origin_ref: params[:ref],
+        checkout_sha: params[:checkout_sha],
+        after_sha: params[:after],
+        before_sha: params[:before],
+        trigger_request: trigger_request,
+        schedule: schedule,
+        ignore_skip_ci: ignore_skip_ci,
+        save_incompleted: save_on_errors,
+        seeds_block: block,
+        project: project,
+        current_user: current_user)
 
       sequence = Gitlab::Ci::Pipeline::Chain::Sequence
         .new(pipeline, command, SEQUENCE)

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

@@ -3,14 +3,13 @@ module Gitlab
     module Pipeline
       module Chain
         class Base
-          attr_reader :pipeline, :project, :current_user
+          attr_reader :pipeline, :command
+
+          delegate :project, :current_user, to: :command
 
           def initialize(pipeline, command)
             @pipeline = pipeline
             @command = command
-
-            @project = command.project
-            @current_user = command.current_user
           end
 
           def perform!

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

@@ -3,20 +3,18 @@ module Gitlab
     module Pipeline
       module Chain
         class Build < Chain::Base
-          include Chain::Helpers
-
           def perform!
             @pipeline.assign_attributes(
               source: @command.source,
-              project: @project,
-              ref: ref,
-              sha: sha,
-              before_sha: before_sha,
-              tag: tag_exists?,
+              project: @command.project,
+              ref: @command.ref,
+              sha: @command.sha,
+              before_sha: @command.before_sha,
+              tag: @command.tag_exists?,
               trigger_requests: Array(@command.trigger_request),
-              user: @current_user,
+              user: @command.current_user,
               pipeline_schedule: @command.schedule,
-              protected: protected_ref?
+              protected: @command.protected_ref?
             )
 
             @pipeline.set_config_source
@@ -25,32 +23,6 @@ module Gitlab
           def break?
             false
           end
-
-          private
-
-          def ref
-            @ref ||= Gitlab::Git.ref_name(origin_ref)
-          end
-
-          def sha
-            @project.commit(origin_sha || origin_ref).try(:id)
-          end
-
-          def origin_ref
-            @command.origin_ref
-          end
-
-          def origin_sha
-            @command.checkout_sha || @command.after_sha
-          end
-
-          def before_sha
-            @command.checkout_sha || @command.before_sha || Gitlab::Git::BLANK_SHA
-          end
-
-          def protected_ref?
-            @project.protected_for?(ref)
-          end
         end
       end
     end

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

+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        Command = Struct.new(
+          :source, :project, :current_user,
+          :origin_ref, :checkout_sha, :after_sha, :before_sha,
+          :trigger_request, :schedule,
+          :ignore_skip_ci, :save_incompleted,
+          :seeds_block
+        ) do
+          include Gitlab::Utils::StrongMemoize
+
+          def initialize(**params)
+            params.each do |key, value|
+              self[key] = value
+            end
+          end
+
+          def branch_exists?
+            strong_memoize(:is_branch) do
+              project.repository.branch_exists?(ref)
+            end
+          end
+
+          def tag_exists?
+            strong_memoize(:is_tag) do
+              project.repository.tag_exists?(ref)
+            end
+          end
+
+          def ref
+            strong_memoize(:ref) do
+              Gitlab::Git.ref_name(origin_ref)
+            end
+          end
+
+          def sha
+            strong_memoize(:sha) do
+              project.commit(origin_sha || origin_ref).try(:id)
+            end
+          end
+
+          def origin_sha
+            checkout_sha || after_sha
+          end
+
+          def before_sha
+            self[:before_sha] || checkout_sha || Gitlab::Git::BLANK_SHA
+          end
+
+          def protected_ref?
+            strong_memoize(:protected_ref) do
+              project.protected_for?(ref)
+            end
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -3,18 +3,6 @@ module Gitlab
     module Pipeline
       module Chain
         module Helpers
-          def branch_exists?
-            return @is_branch if defined?(@is_branch)
-
-            @is_branch = project.repository.branch_exists?(pipeline.ref)
-          end
-
-          def tag_exists?
-            return @is_tag if defined?(@is_tag)
-
-            @is_tag = project.repository.tag_exists?(pipeline.ref)
-          end
-
           def error(message)
             pipeline.errors.add(:base, message)
           end

lib/gitlab/ci/pipeline/chain/validate/abilities.rb

@@ -14,7 +14,7 @@ module Gitlab
 
               unless allowed_to_trigger_pipeline?
                 if can?(current_user, :create_pipeline, project)
-                  return error("Insufficient permissions for protected ref '#{pipeline.ref}'")
+                  return error("Insufficient permissions for protected ref '#{command.ref}'")
                 else
                   return error('Insufficient permissions to create a new pipeline')
                 end
@@ -29,7 +29,7 @@ module Gitlab
               if current_user
                 allowed_to_create?
               else # legacy triggers don't have a corresponding user
-                !project.protected_for?(@pipeline.ref)
+                !@command.protected_ref?
               end
             end
 
@@ -38,10 +38,10 @@ module Gitlab
 
               access = Gitlab::UserAccess.new(current_user, project: project)
 
-              if branch_exists?
-                access.can_update_branch?(@pipeline.ref)
-              elsif tag_exists?
-                access.can_create_tag?(@pipeline.ref)
+              if @command.branch_exists?
+                access.can_update_branch?(@command.ref)
+              elsif @command.tag_exists?
+                access.can_create_tag?(@command.ref)
               else
                 true # Allow it for now and we'll reject when we check ref existence
               end

lib/gitlab/ci/pipeline/chain/validate/repository.rb

@@ -7,14 +7,11 @@ module Gitlab
             include Chain::Helpers
 
             def perform!
-              unless branch_exists? || tag_exists?
+              unless @command.branch_exists? || @command.tag_exists?
                 return error('Reference not found')
               end
 
-              ## TODO, we check commit in the service, that is why
-              # there is no repository access here.
-              #
-              unless pipeline.sha
+              unless @command.sha
                 return error('Commit not found')
               end
             end

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

@@ -6,46 +6,81 @@ describe Gitlab::Ci::Pipeline::Chain::Build do
   let(:pipeline) { Ci::Pipeline.new }
 
   let(:command) do
-    double('command', source: :push,
-                      origin_ref: 'master',
-                      checkout_sha: project.commit.id,
-                      after_sha: nil,
-                      before_sha: nil,
-                      trigger_request: nil,
-                      schedule: nil,
-                      project: project,
-                      current_user: user)
+    Gitlab::Ci::Pipeline::Chain::Command.new(
+      source: :push,
+      origin_ref: 'master',
+      checkout_sha: project.commit.id,
+      after_sha: nil,
+      before_sha: nil,
+      trigger_request: nil,
+      schedule: nil,
+      project: project,
+      current_user: user)
   end
 
   let(:step) { described_class.new(pipeline, command) }
 
   before do
     stub_repository_ci_yaml_file(sha: anything)
-
-    step.perform!
   end
 
   it 'never breaks the chain' do
+    step.perform!
+
     expect(step.break?).to be false
   end
 
   it 'fills pipeline object with data' do
+    step.perform!
+
     expect(pipeline.sha).not_to be_empty
     expect(pipeline.sha).to eq project.commit.id
     expect(pipeline.ref).to eq 'master'
+    expect(pipeline.tag).to be false
     expect(pipeline.user).to eq user
     expect(pipeline.project).to eq project
   end
 
   it 'sets a valid config source' do
+    step.perform!
+
     expect(pipeline.repository_source?).to be true
   end
 
   it 'returns a valid pipeline' do
+    step.perform!
+
     expect(pipeline).to be_valid
   end
 
   it 'does not persist a pipeline' do
+    step.perform!
+
     expect(pipeline).not_to be_persisted
   end
+
+  context 'when pipeline is running for a tag' do
+    let(:command) do
+      Gitlab::Ci::Pipeline::Chain::Command.new(
+        source: :push,
+        origin_ref: 'mytag',
+        checkout_sha: project.commit.id,
+        after_sha: nil,
+        before_sha: nil,
+        trigger_request: nil,
+        schedule: nil,
+        project: project,
+        current_user: user)
+    end
+
+    before do
+      allow_any_instance_of(Repository).to receive(:tag_exists?).with('mytag').and_return(true)
+
+      step.perform!
+    end
+
+    it 'correctly indicated that this is a tagged pipeline' do
+      expect(pipeline).to be_tag
+    end
+  end
 end

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

+require 'spec_helper'
+
+describe Gitlab::Ci::Pipeline::Chain::Command do
+  set(:project) { create(:project, :repository) }
+
+  describe '#initialize' do
+    subject do
+      described_class.new(origin_ref: 'master')
+    end
+
+    it 'properly initialises object from hash' do
+      expect(subject.origin_ref).to eq('master')
+    end
+  end
+
+  context 'handling of origin_ref' do
+    let(:command) { described_class.new(project: project, origin_ref: origin_ref) }
+
+    describe '#branch_exists?' do
+      subject { command.branch_exists? }
+
+      context 'for existing branch' do
+        let(:origin_ref) { 'master' }
+
+        it { is_expected.to eq(true) }
+      end
+
+      context 'for invalid branch' do
+        let(:origin_ref) { 'something' }
+
+        it { is_expected.to eq(false) }
+      end
+    end
+
+    describe '#tag_exists?' do
+      subject { command.tag_exists? }
+
+      context 'for existing ref' do
+        let(:origin_ref) { 'v1.0.0' }
+
+        it { is_expected.to eq(true) }
+      end
+
+      context 'for invalid ref' do
+        let(:origin_ref) { 'something' }
+
+        it { is_expected.to eq(false) }
+      end
+    end
+
+    describe '#ref' do
+      subject { command.ref }
+
+      context 'for regular ref' do
+        let(:origin_ref) { 'master' }
+
+        it { is_expected.to eq('master') }
+      end
+
+      context 'for branch ref' do
+        let(:origin_ref) { 'refs/heads/master' }
+
+        it { is_expected.to eq('master') }
+      end
+
+      context 'for tag ref' do
+        let(:origin_ref) { 'refs/tags/1.0.0' }
+
+        it { is_expected.to eq('1.0.0') }
+      end
+
+      context 'for other refs' do
+        let(:origin_ref) { 'refs/merge-requests/11/head' }
+
+        it { is_expected.to eq('refs/merge-requests/11/head') }
+      end
+    end
+  end
+
+  describe '#sha' do
+    subject { command.sha }
+
+    context 'when invalid checkout_sha is specified' do
+      let(:command) { described_class.new(project: project, checkout_sha: 'aaa') }
+
+      it 'returns empty value' do
+        is_expected.to be_nil
+      end
+    end
+
+    context 'when a valid checkout_sha is specified' do
+      let(:command) { described_class.new(project: project, checkout_sha: project.commit.id) }
+
+      it 'returns checkout_sha' do
+        is_expected.to eq(project.commit.id)
+      end
+    end
+
+    context 'when a valid after_sha is specified' do
+      let(:command) { described_class.new(project: project, after_sha: project.commit.id) }
+
+      it 'returns after_sha' do
+        is_expected.to eq(project.commit.id)
+      end
+    end
+
+    context 'when a valid origin_ref is specified' do
+      let(:command) { described_class.new(project: project, origin_ref: 'HEAD') }
+
+      it 'returns SHA for given ref' do
+        is_expected.to eq(project.commit.id)
+      end
+    end
+  end
+
+  describe '#origin_sha' do
+    subject { command.origin_sha }
+
+    context 'when using checkout_sha and after_sha' do
+      let(:command) { described_class.new(project: project, checkout_sha: 'aaa', after_sha: 'bbb') }
+
+      it 'uses checkout_sha' do
+        is_expected.to eq('aaa')
+      end
+    end
+
+    context 'when using after_sha only' do
+      let(:command) { described_class.new(project: project, after_sha: 'bbb') }
+
+      it 'uses after_sha' do
+        is_expected.to eq('bbb')
+      end
+    end
+  end
+
+  describe '#before_sha' do
+    subject { command.before_sha }
+
+    context 'when using checkout_sha and before_sha' do
+      let(:command) { described_class.new(project: project, checkout_sha: 'aaa', before_sha: 'bbb') }
+
+      it 'uses before_sha' do
+        is_expected.to eq('bbb')
+      end
+    end
+
+    context 'when using checkout_sha only' do
+      let(:command) { described_class.new(project: project, checkout_sha: 'aaa') }
+
+      it 'uses checkout_sha' do
+        is_expected.to eq('aaa')
+      end
+    end
+
+    context 'when checkout_sha and before_sha are empty' do
+      let(:command) { described_class.new(project: project) }
+
+      it 'uses BLANK_SHA' do
+        is_expected.to eq(Gitlab::Git::BLANK_SHA)
+      end
+    end
+  end
+
+  describe '#protected_ref?' do
+    let(:command) { described_class.new(project: project, origin_ref: 'my-branch') }
+
+    subject { command.protected_ref? }
+
+    context 'when a ref is protected' do
+      before do
+        expect_any_instance_of(Project).to receive(:protected_for?).with('my-branch').and_return(true)
+      end
+
+      it { is_expected.to eq(true) }
+    end
+
+    context 'when a ref is unprotected' do
+      before do
+        expect_any_instance_of(Project).to receive(:protected_for?).with('my-branch').and_return(false)
+      end
+
+      it { is_expected.to eq(false) }
+    end
+  end
+end

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

@@ -10,9 +10,9 @@ describe Gitlab::Ci::Pipeline::Chain::Create do
   end
 
   let(:command) do
-    double('command', project: project,
-                      current_user: user,
-                      seeds_block: nil)
+    Gitlab::Ci::Pipeline::Chain::Command.new(
+      project: project,
+      current_user: user, seeds_block: nil)
   end
 
   let(:step) { described_class.new(pipeline, command) }

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

@@ -5,7 +5,7 @@ describe Gitlab::Ci::Pipeline::Chain::Sequence do
   set(:user) { create(:user) }
 
   let(:pipeline) { build_stubbed(:ci_pipeline) }
-  let(:command) { double('command' ) }
+  let(:command) { Gitlab::Ci::Pipeline::Chain::Command.new }
   let(:first_step) { spy('first step') }
   let(:second_step) { spy('second step') }
   let(:sequence) { [first_step, second_step] }

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

@@ -6,10 +6,11 @@ describe Gitlab::Ci::Pipeline::Chain::Skip do
   set(:pipeline) { create(:ci_pipeline, project: project) }
 
   let(:command) do
-    double('command', project: project,
-                      current_user: user,
-                      ignore_skip_ci: false,
-                      save_incompleted: true)
+    Gitlab::Ci::Pipeline::Chain::Command.new(
+      project: project,
+      current_user: user,
+      ignore_skip_ci: false,
+      save_incompleted: true)
   end
 
   let(:step) { described_class.new(pipeline, command) }

spec/lib/gitlab/ci/pipeline/chain/validate/abilities_spec.rb

@@ -5,11 +5,12 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Abilities do
   set(:user) { create(:user) }
 
   let(:pipeline) do
-    build_stubbed(:ci_pipeline, ref: ref, project: project)
+    build_stubbed(:ci_pipeline, project: project)
   end
 
   let(:command) do
-    double('command', project: project, current_user: user)
+    Gitlab::Ci::Pipeline::Chain::Command.new(
+      project: project, current_user: user, origin_ref: ref)
   end
 
   let(:step) { described_class.new(pipeline, command) }

spec/lib/gitlab/ci/pipeline/chain/validate/config_spec.rb

@@ -5,9 +5,10 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Config do
   set(:user) { create(:user) }
 
   let(:command) do
-    double('command', project: project,
-                      current_user: user,
-                      save_incompleted: true)
+    Gitlab::Ci::Pipeline::Chain::Command.new(
+      project: project,
+      current_user: user,
+      save_incompleted: true)
   end
 
   let!(:step) { described_class.new(pipeline, command) }

spec/lib/gitlab/ci/pipeline/chain/validate/repository_spec.rb

@@ -3,10 +3,7 @@ require 'spec_helper'
 describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do
   set(:project) { create(:project, :repository) }
   set(:user) { create(:user) }
-
-  let(:command) do
-    double('command', project: project, current_user: user)
-  end
+  let(:pipeline) { build_stubbed(:ci_pipeline) }
 
   let!(:step) { described_class.new(pipeline, command) }
 
@@ -14,9 +11,10 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do
     step.perform!
   end
 
-  context 'when pipeline ref and sha exists' do
-    let(:pipeline) do
-      build_stubbed(:ci_pipeline, ref: 'master', sha: '123', project: project)
+  context 'when ref and sha exists' do
+    let(:command) do
+      Gitlab::Ci::Pipeline::Chain::Command.new(
+        project: project, current_user: user, origin_ref: 'master', checkout_sha: project.commit.id)
     end
 
     it 'does not break the chain' do
@@ -28,9 +26,10 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do
     end
   end
 
-  context 'when pipeline ref does not exist' do
-    let(:pipeline) do
-      build_stubbed(:ci_pipeline, ref: 'something', project: project)
+  context 'when ref does not exist' do
+    let(:command) do
+      Gitlab::Ci::Pipeline::Chain::Command.new(
+        project: project, current_user: user, origin_ref: 'something')
     end
 
     it 'breaks the chain' do
@@ -43,9 +42,10 @@ describe Gitlab::Ci::Pipeline::Chain::Validate::Repository do
     end
   end
 
-  context 'when pipeline does not have SHA set' do
-    let(:pipeline) do
-      build_stubbed(:ci_pipeline, ref: 'master', sha: nil, project: project)
+  context 'when does not have existing SHA set' do
+    let(:command) do
+      Gitlab::Ci::Pipeline::Chain::Command.new(
+        project: project, current_user: user, origin_ref: 'master', checkout_sha: 'something')
     end
 
     it 'breaks the chain' do

spec/services/ci/create_pipeline_service_spec.rb

@@ -518,5 +518,20 @@ describe Ci::CreatePipelineService do
         end
       end
     end
+
+    context 'when pipeline is running for a tag' do
+      before do
+        config = YAML.dump(test: { script: 'test', only: ['branches'] },
+                           deploy: { script: 'deploy', only: ['tags'] })
+
+        stub_ci_pipeline_yaml_file(config)
+      end
+
+      it 'creates a tagged pipeline' do
+        pipeline = execute_service(ref: 'v1.0.0')
+
+        expect(pipeline.tag?).to be true
+      end
+    end
   end
 end