Merge branch 'ci-cache-instance-level-variables' into 'master'

Better memoize instance level variables when creating pipelines

See merge request gitlab-org/gitlab!79435

app/models/ci/pipeline.rb

@@ -40,6 +40,10 @@ module Ci
     # https://gitlab.com/gitlab-org/gitlab/-/issues/259010
     attr_accessor :merged_yaml
 
+    # This is used to retain access to the method defined by `Ci::HasRef`
+    # before being overridden in this class.
+    alias_method :jobs_git_ref, :git_ref
+
     belongs_to :project, inverse_of: :all_pipelines
     belongs_to :user
     belongs_to :auto_canceled_by, class_name: 'Ci::Pipeline'

app/models/project.rb

@@ -2187,14 +2187,6 @@ class Project < ApplicationRecord
     end
   end
 
-  def ci_instance_variables_for(ref:)
-    if protected_for?(ref)
-      Ci::InstanceVariable.all_cached
-    else
-      Ci::InstanceVariable.unprotected_cached
-    end
-  end
-
   def protected_for?(ref)
     raise Repository::AmbiguousRefError if repository.ambiguous_ref?(ref)
 

lib/gitlab/ci/config.rb

@@ -158,7 +158,7 @@ module Gitlab
           # See more detail in the docs: https://docs.gitlab.com/ee/ci/variables/#cicd-variable-precedence
           variables.concat(project.predefined_variables)
           variables.concat(pipeline.predefined_variables) if pipeline
-          variables.concat(project.ci_instance_variables_for(ref: source_ref_path))
+          variables.concat(secret_variables(project: project, pipeline: pipeline))
           variables.concat(project.group.ci_variables_for(source_ref_path, project)) if project.group
           variables.concat(project.ci_variables_for(ref: source_ref_path))
           variables.concat(pipeline.variables) if pipeline
@@ -166,6 +166,14 @@ module Gitlab
         end
       end
 
+      def secret_variables(project:, pipeline:)
+        if pipeline
+          pipeline.variables_builder.secret_instance_variables
+        else
+          Gitlab::Ci::Variables::Builder::Instance.new.secret_variables
+        end
+      end
+
       def track_and_raise_for_dev_exception(error)
         Gitlab::ErrorTracking.track_and_raise_for_dev_exception(error, @context.sentry_payload)
       end

lib/gitlab/ci/variables/builder.rb

@@ -8,6 +8,7 @@ module Gitlab
 
         def initialize(pipeline)
           @pipeline = pipeline
+          @instance_variables_builder = Builder::Instance.new
         end
 
         def scoped_variables(job, environment:, dependencies:)
@@ -21,7 +22,7 @@ module Gitlab
             variables.concat(job.yaml_variables)
             variables.concat(user_variables(job.user))
             variables.concat(job.dependency_variables) if dependencies
-            variables.concat(secret_instance_variables(ref: job.git_ref))
+            variables.concat(secret_instance_variables)
             variables.concat(secret_group_variables(environment: environment, ref: job.git_ref))
             variables.concat(secret_project_variables(environment: environment, ref: job.git_ref))
             variables.concat(job.trigger_request.user_variables) if job.trigger_request
@@ -62,8 +63,11 @@ module Gitlab
           end
         end
 
-        def secret_instance_variables(ref:)
-          project.ci_instance_variables_for(ref: ref)
+        def secret_instance_variables
+          strong_memoize(:secret_instance_variables) do
+            instance_variables_builder
+              .secret_variables(protected_ref: protected_ref?)
+          end
         end
 
         def secret_group_variables(environment:, ref:)
@@ -79,6 +83,7 @@ module Gitlab
         private
 
         attr_reader :pipeline
+        attr_reader :instance_variables_builder
         delegate :project, to: :pipeline
 
         def predefined_variables(job)
@@ -104,6 +109,12 @@ module Gitlab
           parallel = parallel.dig(:total) if parallel.is_a?(Hash)
           parallel || 1
         end
+
+        def protected_ref?
+          strong_memoize(:protected_ref) do
+            project.protected_for?(pipeline.jobs_git_ref)
+          end
+        end
       end
     end
   end

lib/gitlab/ci/variables/builder/instance.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Variables
+      class Builder
+        class Instance
+          include Gitlab::Utils::StrongMemoize
+
+          def secret_variables(protected_ref: false)
+            variables = if protected_ref
+                          ::Ci::InstanceVariable.all_cached
+                        else
+                          ::Ci::InstanceVariable.unprotected_cached
+                        end
+
+            Gitlab::Ci::Variables::Collection.new(variables)
+          end
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/config_spec.rb

@@ -462,7 +462,7 @@ RSpec.describe Gitlab::Ci::Config do
           expect(project.repository).to receive(:blob_data_at)
             .with('eeff1122', local_location)
 
-          described_class.new(gitlab_ci_yml, project: project, sha: 'eeff1122', user: user)
+          described_class.new(gitlab_ci_yml, project: project, sha: 'eeff1122', user: user, pipeline: pipeline)
         end
       end
 
@@ -470,7 +470,7 @@ RSpec.describe Gitlab::Ci::Config do
         it 'is using latest SHA on the default branch' do
           expect(project.repository).to receive(:root_ref_sha)
 
-          described_class.new(gitlab_ci_yml, project: project, sha: nil, user: user)
+          described_class.new(gitlab_ci_yml, project: project, sha: nil, user: user, pipeline: pipeline)
         end
       end
     end

spec/lib/gitlab/ci/variables/builder/instance_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Variables::Builder::Instance do
+  let_it_be(:variable) { create(:ci_instance_variable, protected: false) }
+  let_it_be(:protected_variable) { create(:ci_instance_variable, protected: true) }
+
+  let(:builder) { described_class.new }
+
+  describe '#secret_variables' do
+    let(:variable_item) { item(variable) }
+    let(:protected_variable_item) { item(protected_variable) }
+
+    subject do
+      builder.secret_variables(protected_ref: protected_ref)
+    end
+
+    context 'when the ref is protected' do
+      let(:protected_ref) { true }
+
+      it 'contains all the variables' do
+        is_expected.to contain_exactly(variable_item, protected_variable_item)
+      end
+    end
+
+    context 'when the ref is not protected' do
+      let(:protected_ref) { false }
+
+      it 'contains only unprotected variables' do
+        is_expected.to contain_exactly(variable_item)
+      end
+    end
+  end
+
+  def item(variable)
+    Gitlab::Ci::Variables::Collection::Item.fabricate(variable)
+  end
+end

spec/lib/gitlab/ci/variables/builder_spec.rb

@@ -7,7 +7,7 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
   let_it_be(:project) { create(:project, :repository, namespace: group) }
   let_it_be_with_reload(:pipeline) { create(:ci_pipeline, project: project) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:job) do
+  let_it_be_with_reload(:job) do
     create(:ci_build,
       pipeline: pipeline,
       user: user,
@@ -276,27 +276,30 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
           create(:protected_branch, :developers_can_merge, name: job.ref, project: project)
         end
 
-        it { is_expected.to include(variable) }
+        it { is_expected.to contain_exactly(protected_variable_item, unprotected_variable_item) }
       end
 
       context 'when ref is not protected' do
-        it { is_expected.not_to include(variable) }
+        it { is_expected.to contain_exactly(unprotected_variable_item) }
       end
     end
 
     context 'when ref is tag' do
-      let_it_be(:job) { create(:ci_build, ref: 'v1.1.0', tag: true, pipeline: pipeline) }
+      before do
+        job.update!(ref: 'v1.1.0', tag: true)
+        pipeline.update!(ref: 'v1.1.0', tag: true)
+      end
 
       context 'when ref is protected' do
         before do
           create(:protected_tag, project: project, name: 'v*')
         end
 
-        it { is_expected.to include(variable) }
+        it { is_expected.to contain_exactly(protected_variable_item, unprotected_variable_item) }
       end
 
       context 'when ref is not protected' do
-        it { is_expected.not_to include(variable) }
+        it { is_expected.to contain_exactly(unprotected_variable_item) }
       end
     end
 
@@ -311,20 +314,24 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
         end
 
         it 'does not return protected variables as it is not supported for merge request pipelines' do
-          is_expected.not_to include(variable)
+          is_expected.to contain_exactly(unprotected_variable_item)
         end
       end
 
       context 'when ref is not protected' do
-        it { is_expected.not_to include(variable) }
+        it { is_expected.to contain_exactly(unprotected_variable_item) }
       end
     end
   end
 
   describe '#secret_instance_variables' do
-    subject { builder.secret_instance_variables(ref: job.git_ref) }
+    subject { builder.secret_instance_variables }
+
+    let_it_be(:protected_variable) { create(:ci_instance_variable, protected: true) }
+    let_it_be(:unprotected_variable) { create(:ci_instance_variable, protected: false) }
 
-    let_it_be(:variable) { create(:ci_instance_variable, protected: true) }
+    let(:protected_variable_item) { Gitlab::Ci::Variables::Collection::Item.fabricate(protected_variable) }
+    let(:unprotected_variable_item) { Gitlab::Ci::Variables::Collection::Item.fabricate(unprotected_variable) }
 
     include_examples "secret CI variables"
   end
@@ -332,7 +339,11 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
   describe '#secret_group_variables' do
     subject { builder.secret_group_variables(ref: job.git_ref, environment: job.expanded_environment_name) }
 
-    let_it_be(:variable) { create(:ci_group_variable, protected: true, group: group) }
+    let_it_be(:protected_variable) { create(:ci_group_variable, protected: true, group: group) }
+    let_it_be(:unprotected_variable) { create(:ci_group_variable, protected: false, group: group) }
+
+    let(:protected_variable_item) { protected_variable }
+    let(:unprotected_variable_item) { unprotected_variable }
 
     include_examples "secret CI variables"
   end
@@ -340,7 +351,11 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
   describe '#secret_project_variables' do
     subject { builder.secret_project_variables(ref: job.git_ref, environment: job.expanded_environment_name) }
 
-    let_it_be(:variable) { create(:ci_variable, protected: true, project: project) }
+    let_it_be(:protected_variable) { create(:ci_variable, protected: true, project: project) }
+    let_it_be(:unprotected_variable) { create(:ci_variable, protected: false, project: project) }
+
+    let(:protected_variable_item) { protected_variable }
+    let(:unprotected_variable_item) { unprotected_variable }
 
     include_examples "secret CI variables"
   end

spec/models/ci/pipeline_spec.rb

@@ -4738,4 +4738,41 @@ RSpec.describe Ci::Pipeline, :mailer, factory_default: :keep do
       let!(:model) { create(:ci_pipeline, project: parent) }
     end
   end
+
+  describe '#jobs_git_ref' do
+    subject { pipeline.jobs_git_ref }
+
+    context 'when tag is true' do
+      let(:pipeline) { build(:ci_pipeline, tag: true) }
+
+      it 'returns a tag ref' do
+        is_expected.to start_with(Gitlab::Git::TAG_REF_PREFIX)
+      end
+    end
+
+    context 'when tag is false' do
+      let(:pipeline) { build(:ci_pipeline, tag: false) }
+
+      it 'returns a branch ref' do
+        is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
+      end
+    end
+
+    context 'when tag is nil' do
+      let(:pipeline) { build(:ci_pipeline, tag: nil) }
+
+      it 'returns a branch ref' do
+        is_expected.to start_with(Gitlab::Git::BRANCH_REF_PREFIX)
+      end
+    end
+
+    context 'when it is triggered by a merge request' do
+      let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
+      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
+
+      it 'returns nil' do
+        is_expected.to be_nil
+      end
+    end
+  end
 end

spec/models/project_spec.rb

@@ -3871,45 +3871,6 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
-  describe '#ci_instance_variables_for' do
-    let(:project) { build_stubbed(:project) }
-
-    let!(:instance_variable) do
-      create(:ci_instance_variable, value: 'secret')
-    end
-
-    let!(:protected_instance_variable) do
-      create(:ci_instance_variable, :protected, value: 'protected')
-    end
-
-    subject { project.ci_instance_variables_for(ref: 'ref') }
-
-    before do
-      stub_application_setting(
-        default_branch_protection: Gitlab::Access::PROTECTION_NONE)
-    end
-
-    context 'when the ref is not protected' do
-      before do
-        allow(project).to receive(:protected_for?).with('ref').and_return(false)
-      end
-
-      it 'contains only the CI variables' do
-        is_expected.to contain_exactly(instance_variable)
-      end
-    end
-
-    context 'when the ref is protected' do
-      before do
-        allow(project).to receive(:protected_for?).with('ref').and_return(true)
-      end
-
-      it 'contains all the variables' do
-        is_expected.to contain_exactly(instance_variable, protected_instance_variable)
-      end
-    end
-  end
-
   describe '#any_lfs_file_locks?', :request_store do
     let_it_be(:project) { create(:project) }