Merge branch 'mb-ci-memoize-group-project-secret-variables' into 'master'

Refactor project secret variables

See merge request gitlab-org/gitlab!79850

app/models/concerns/has_environment_scope.rb

@@ -70,6 +70,14 @@ module HasEnvironmentScope
 
       relation
     end
+
+    scope :for_environment, ->(environment) do
+      if environment
+        on_environment(environment)
+      else
+        where(environment_scope: '*')
+      end
+    end
   end
 
   def environment_scope=(new_environment_scope)

config/feature_flags/development/ci_variables_builder_memoize_secret_variables.yml

+---
+name: ci_variables_builder_memoize_secret_variables
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79850
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/351995
+milestone: '14.8'
+type: development
+group: group::pipeline execution
+default_enabled: false

lib/gitlab/ci/variables/builder.rb

@@ -9,6 +9,7 @@ module Gitlab
         def initialize(pipeline)
           @pipeline = pipeline
           @instance_variables_builder = Builder::Instance.new
+          @project_variables_builder = Builder::Project.new(project)
         end
 
         def scoped_variables(job, environment:, dependencies:)
@@ -77,13 +78,18 @@ module Gitlab
         end
 
         def secret_project_variables(environment:, ref:)
+          if memoize_secret_variables?
+            memoized_secret_project_variables(environment: environment)
+          else
             project.ci_variables_for(ref: ref, environment: environment)
           end
+        end
 
         private
 
         attr_reader :pipeline
         attr_reader :instance_variables_builder
+        attr_reader :project_variables_builder
         delegate :project, to: :pipeline
 
         def predefined_variables(job)
@@ -104,6 +110,15 @@ module Gitlab
           end
         end
 
+        def memoized_secret_project_variables(environment:)
+          strong_memoize_with(:secret_project_variables, environment) do
+            project_variables_builder
+              .secret_variables(
+                environment: environment,
+                protected_ref: protected_ref?)
+          end
+        end
+
         def ci_node_total_value(job)
           parallel = job.options&.dig(:parallel)
           parallel = parallel.dig(:total) if parallel.is_a?(Hash)
@@ -115,6 +130,24 @@ module Gitlab
             project.protected_for?(pipeline.jobs_git_ref)
           end
         end
+
+        def memoize_secret_variables?
+          strong_memoize(:memoize_secret_variables) do
+            ::Feature.enabled?(:ci_variables_builder_memoize_secret_variables,
+              project,
+              default_enabled: :yaml)
+          end
+        end
+
+        def strong_memoize_with(name, *args)
+          container = strong_memoize(name) { {} }
+
+          if container.key?(args)
+            container[args]
+          else
+            container[args] = yield
+          end
+        end
       end
     end
   end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Variables
+      class Builder
+        class Project
+          include Gitlab::Utils::StrongMemoize
+
+          def initialize(project)
+            @project = project
+          end
+
+          def secret_variables(environment:, protected_ref: false)
+            variables = @project.variables
+            variables = variables.unprotected unless protected_ref
+            variables = variables.for_environment(environment)
+
+            Gitlab::Ci::Variables::Collection.new(variables)
+          end
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Variables::Builder::Project do
+  let_it_be(:project) { create(:project, :repository) }
+
+  let(:builder) { described_class.new(project) }
+
+  describe '#secret_variables' do
+    let(:environment) { '*' }
+    let(:protected_ref) { false }
+
+    let_it_be(:variable) do
+      create(:ci_variable,
+        value: 'secret',
+        project: project)
+    end
+
+    let_it_be(:protected_variable) do
+      create(:ci_variable, :protected,
+        value: 'protected',
+        project: project)
+    end
+
+    let(:variable_item) { item(variable) }
+    let(:protected_variable_item) { item(protected_variable) }
+
+    subject do
+      builder.secret_variables(
+        environment: environment,
+        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 the unprotected variables' do
+        is_expected.to contain_exactly(variable_item)
+      end
+    end
+
+    context 'when environment name is specified' do
+      let(:environment) { 'review/name' }
+
+      before do
+        Ci::Variable.update_all(environment_scope: environment_scope)
+      end
+
+      context 'when environment scope is exactly matched' do
+        let(:environment_scope) { 'review/name' }
+
+        it { is_expected.to contain_exactly(variable_item) }
+      end
+
+      context 'when environment scope is matched by wildcard' do
+        let(:environment_scope) { 'review/*' }
+
+        it { is_expected.to contain_exactly(variable_item) }
+      end
+
+      context 'when environment scope does not match' do
+        let(:environment_scope) { 'review/*/special' }
+
+        it { is_expected.not_to contain_exactly(variable_item) }
+      end
+
+      context 'when environment scope has _' do
+        let(:environment_scope) { '*_*' }
+
+        it 'does not treat it as wildcard' do
+          is_expected.not_to contain_exactly(variable_item)
+        end
+      end
+
+      context 'when environment name contains underscore' do
+        let(:environment) { 'foo_bar/test' }
+        let(:environment_scope) { 'foo_bar/*' }
+
+        it 'matches literally for _' do
+          is_expected.to contain_exactly(variable_item)
+        end
+      end
+
+      # The environment name and scope cannot have % at the moment,
+      # but we're considering relaxing it and we should also make sure
+      # it doesn't break in case some data sneaked in somehow as we're
+      # not checking this integrity in database level.
+      context 'when environment scope has %' do
+        let(:environment_scope) { '*%*' }
+
+        it 'does not treat it as wildcard' do
+          is_expected.not_to contain_exactly(variable_item)
+        end
+      end
+
+      context 'when environment name contains a percent' do
+        let(:environment) { 'foo%bar/test' }
+        let(:environment_scope) { 'foo%bar/*' }
+
+        it 'matches literally for _' do
+          is_expected.to contain_exactly(variable_item)
+        end
+      end
+    end
+
+    context 'when variables with the same name have different environment scopes' do
+      let(:environment) { 'review/name' }
+
+      let_it_be(:partially_matched_variable) do
+        create(:ci_variable,
+          key: variable.key,
+          value: 'partial',
+          environment_scope: 'review/*',
+          project: project)
+      end
+
+      let_it_be(:perfectly_matched_variable) do
+        create(:ci_variable,
+          key: variable.key,
+          value: 'prefect',
+          environment_scope: 'review/name',
+          project: project)
+      end
+
+      it 'puts variables matching environment scope more in the end' do
+        variables_collection = Gitlab::Ci::Variables::Collection.new([
+          variable,
+          partially_matched_variable,
+          perfectly_matched_variable
+        ]).to_runner_variables
+
+        expect(subject.to_runner_variables).to eq(variables_collection)
+      end
+    end
+  end
+
+  def item(variable)
+    Gitlab::Ci::Variables::Collection::Item.fabricate(variable)
+  end
+end

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

@@ -349,14 +349,93 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
   end
 
   describe '#secret_project_variables' do
-    subject { builder.secret_project_variables(ref: job.git_ref, environment: job.expanded_environment_name) }
-
     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(:ref) { job.git_ref }
+    let(:environment) { job.expanded_environment_name }
+
+    subject { builder.secret_project_variables(ref: ref, environment: environment) }
+
+    context 'with ci_variables_builder_memoize_secret_variables disabled' do
+      before do
+        stub_feature_flags(ci_variables_builder_memoize_secret_variables: false)
+      end
+
       let(:protected_variable_item) { protected_variable }
       let(:unprotected_variable_item) { unprotected_variable }
 
       include_examples "secret CI variables"
     end
+
+    context 'with ci_variables_builder_memoize_secret_variables enabled' do
+      before do
+        stub_feature_flags(ci_variables_builder_memoize_secret_variables: true)
+      end
+
+      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"
+
+      context 'variables memoization' do
+        let_it_be(:scoped_variable) { create(:ci_variable, project: project, environment_scope: 'scoped') }
+
+        let(:scoped_variable_item) { Gitlab::Ci::Variables::Collection::Item.fabricate(scoped_variable) }
+
+        context 'with protected environments' do
+          it 'memoizes the result by environment' do
+            expect(pipeline.project)
+              .to receive(:protected_for?)
+              .with(pipeline.jobs_git_ref)
+              .once.and_return(true)
+
+            expect_next_instance_of(described_class::Project) do |project_variables_builder|
+              expect(project_variables_builder)
+                .to receive(:secret_variables)
+                .with(environment: 'production', protected_ref: true)
+                .once
+                .and_call_original
+            end
+
+            2.times do
+              expect(builder.secret_project_variables(ref: ref, environment: 'production'))
+                .to contain_exactly(unprotected_variable_item, protected_variable_item)
+            end
+          end
+        end
+
+        context 'with unprotected environments' do
+          it 'memoizes the result by environment' do
+            expect(pipeline.project)
+              .to receive(:protected_for?)
+              .with(pipeline.jobs_git_ref)
+              .once.and_return(false)
+
+            expect_next_instance_of(described_class::Project) do |project_variables_builder|
+              expect(project_variables_builder)
+                .to receive(:secret_variables)
+                .with(environment: nil, protected_ref: false)
+                .once
+                .and_call_original
+
+              expect(project_variables_builder)
+                .to receive(:secret_variables)
+                .with(environment: 'scoped', protected_ref: false)
+                .once
+                .and_call_original
+            end
+
+            2.times do
+              expect(builder.secret_project_variables(ref: 'other', environment: nil))
+                .to contain_exactly(unprotected_variable_item)
+
+              expect(builder.secret_project_variables(ref: 'other', environment: 'scoped'))
+                .to contain_exactly(unprotected_variable_item, scoped_variable_item)
+            end
+          end
+        end
+      end
+    end
+  end
 end

spec/models/concerns/has_environment_scope_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe HasEnvironmentScope do
+  let_it_be(:project) { create(:project) }
+
   subject { build(:ci_variable) }
 
   it { is_expected.to allow_value('*').for(:environment_scope) }
@@ -17,8 +19,6 @@ RSpec.describe HasEnvironmentScope do
   end
 
   describe '.on_environment' do
-    let(:project) { create(:project) }
-
     it 'returns scoped objects' do
       variable1 = create(:ci_variable, project: project, environment_scope: '*')
       variable2 = create(:ci_variable, project: project, environment_scope: 'product/*')
@@ -63,4 +63,32 @@ RSpec.describe HasEnvironmentScope do
       end
     end
   end
+
+  describe '.for_environment' do
+    subject { project.variables.for_environment(environment) }
+
+    let_it_be(:variable1) do
+      create(:ci_variable, project: project, environment_scope: '*')
+    end
+
+    let_it_be(:variable2) do
+      create(:ci_variable, project: project, environment_scope: 'production/*')
+    end
+
+    let_it_be(:variable3) do
+      create(:ci_variable, project: project, environment_scope: 'staging/*')
+    end
+
+    context 'when the environment is present' do
+      let(:environment) { 'production/canary-1' }
+
+      it { is_expected.to eq([variable1, variable2]) }
+    end
+
+    context 'when the environment is nil' do
+      let(:environment) {}
+
+      it { is_expected.to eq([variable1]) }
+    end
+  end
 end