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

Refactor group secret variables

See merge request gitlab-org/gitlab!79936

app/models/ci/group_variable.rb

@@ -18,5 +18,6 @@ module Ci
 
     scope :unprotected, -> { where(protected: false) }
     scope :by_environment_scope, -> (environment_scope) { where(environment_scope: environment_scope) }
+    scope :for_groups, ->(group_ids) { where(group_id: group_ids) }
   end
 end

lib/gitlab/ci/variables/builder.rb

@@ -10,6 +10,7 @@ module Gitlab
           @pipeline = pipeline
           @instance_variables_builder = Builder::Instance.new
           @project_variables_builder = Builder::Project.new(project)
+          @group_variables_builder = Builder::Group.new(project.group)
         end
 
         def scoped_variables(job, environment:, dependencies:)
@@ -72,9 +73,13 @@ module Gitlab
         end
 
         def secret_group_variables(environment:, ref:)
-          return [] unless project.group
+          if memoize_secret_variables?
+            memoized_secret_group_variables(environment: environment)
+          else
+            return [] unless project.group
 
-          project.group.ci_variables_for(ref, project, environment: environment)
+            project.group.ci_variables_for(ref, project, environment: environment)
+          end
         end
 
         def secret_project_variables(environment:, ref:)
@@ -90,6 +95,7 @@ module Gitlab
         attr_reader :pipeline
         attr_reader :instance_variables_builder
         attr_reader :project_variables_builder
+        attr_reader :group_variables_builder
         delegate :project, to: :pipeline
 
         def predefined_variables(job)
@@ -119,6 +125,15 @@ module Gitlab
           end
         end
 
+        def memoized_secret_group_variables(environment:)
+          strong_memoize_with(:secret_group_variables, environment) do
+            group_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)

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Variables
+      class Builder
+        class Group
+          include Gitlab::Utils::StrongMemoize
+
+          def initialize(group)
+            @group = group
+          end
+
+          def secret_variables(environment:, protected_ref: false)
+            return [] unless group
+
+            variables = base_scope
+            variables = variables.unprotected unless protected_ref
+            variables = variables.for_environment(environment)
+            variables = variables.group_by(&:group_id)
+            variables = list_of_ids.reverse.flat_map { |group| variables[group.id] }.compact
+            Gitlab::Ci::Variables::Collection.new(variables)
+          end
+
+          private
+
+          attr_reader :group
+
+          def base_scope
+            strong_memoize(:base_scope) do
+              ::Ci::GroupVariable.for_groups(list_of_ids)
+            end
+          end
+
+          def list_of_ids
+            strong_memoize(:list_of_ids) do
+              if group.root_ancestor.use_traversal_ids?
+                [group] + group.ancestors(hierarchy_order: :asc)
+              else
+                [group] + group.ancestors
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Variables::Builder::Group do
+  let_it_be(:group) { create(:group) }
+
+  let(:builder) { described_class.new(group) }
+
+  describe '#secret_variables' do
+    let(:environment) { '*' }
+    let(:protected_ref) { false }
+
+    let_it_be(:variable) do
+      create(:ci_group_variable,
+        value: 'secret',
+        group: group)
+    end
+
+    let_it_be(:protected_variable) do
+      create(:ci_group_variable, :protected,
+        value: 'protected',
+        group: group)
+    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 not protected' do
+      let(:protected_ref) { false }
+
+      it 'contains only the CI variables' do
+        is_expected.to contain_exactly(variable_item)
+      end
+    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 environment name is specified' do
+      let(:environment) { 'review/name' }
+
+      before do
+        Ci::GroupVariable.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_group_variable,
+          key: variable.key,
+          value: 'partial',
+          environment_scope: 'review/*',
+          group: group)
+      end
+
+      let_it_be(:perfectly_matched_variable) do
+        create(:ci_group_variable,
+          key: variable.key,
+          value: 'prefect',
+          environment_scope: 'review/name',
+          group: group)
+      end
+
+      it 'orders the variables from least to most matched' 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
+
+    context 'when group has children' do
+      let(:protected_ref) { true }
+
+      let_it_be(:group_child_1) { create(:group, parent: group) }
+      let_it_be(:group_child_2) { create(:group, parent: group_child_1) }
+
+      let_it_be_with_reload(:group_child_3) do
+        create(:group, parent: group_child_2)
+      end
+
+      let_it_be(:variable_child_1) do
+        create(:ci_group_variable, group: group_child_1)
+      end
+
+      let_it_be(:variable_child_2) do
+        create(:ci_group_variable, group: group_child_2)
+      end
+
+      let_it_be(:variable_child_3) do
+        create(:ci_group_variable, group: group_child_3)
+      end
+
+      context 'traversal queries' do
+        shared_examples 'correct ancestor order' do
+          let(:builder) { described_class.new(group_child_3) }
+
+          it 'returns all variables belonging to the group and parent groups' do
+            expected_array1 = Gitlab::Ci::Variables::Collection.new(
+              [protected_variable_item, variable_item])
+            .to_runner_variables
+
+            expected_array2 = Gitlab::Ci::Variables::Collection.new(
+              [variable_child_1, variable_child_2, variable_child_3]
+            ).to_runner_variables
+
+            got_array = subject.to_runner_variables
+
+            expect(got_array.shift(2)).to contain_exactly(*expected_array1)
+            expect(got_array).to eq(expected_array2)
+          end
+        end
+
+        context 'recursive' do
+          before do
+            stub_feature_flags(use_traversal_ids: false)
+          end
+
+          include_examples 'correct ancestor order'
+        end
+
+        context 'linear' do
+          before do
+            stub_feature_flags(use_traversal_ids: true)
+          end
+
+          include_examples 'correct ancestor order'
+        end
+      end
+    end
+  end
+
+  def item(variable)
+    Gitlab::Ci::Variables::Collection::Item.fabricate(variable)
+  end
+end

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

@@ -342,10 +342,88 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
     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 }
+    context 'with ci_variables_builder_memoize_secret_variables disabled' do
+      before do
+        stub_feature_flags(ci_variables_builder_memoize_secret_variables: false)
+      end
 
-    include_examples "secret CI variables"
+      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_group_variable, group: group, environment_scope: 'scoped') }
+
+        let(:ref) { job.git_ref }
+        let(:environment) { job.expanded_environment_name }
+        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::Group) do |group_variables_builder|
+              expect(group_variables_builder)
+                .to receive(:secret_variables)
+                .with(environment: 'production', protected_ref: true)
+                .once
+                .and_call_original
+            end
+
+            2.times do
+              expect(builder.secret_group_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::Group) do |group_variables_builder|
+              expect(group_variables_builder)
+                .to receive(:secret_variables)
+                .with(environment: nil, protected_ref: false)
+                .once
+                .and_call_original
+
+              expect(group_variables_builder)
+                .to receive(:secret_variables)
+                .with(environment: 'scoped', protected_ref: false)
+                .once
+                .and_call_original
+            end
+
+            2.times do
+              expect(builder.secret_group_variables(ref: 'other', environment: nil))
+                .to contain_exactly(unprotected_variable_item)
+
+              expect(builder.secret_group_variables(ref: 'other', environment: 'scoped'))
+                .to contain_exactly(unprotected_variable_item, scoped_variable_item)
+            end
+          end
+        end
+      end
+    end
   end
 
   describe '#secret_project_variables' do

spec/models/ci/group_variable_spec.rb

@@ -43,6 +43,14 @@ RSpec.describe Ci::GroupVariable do
     end
   end
 
+  describe '.for_groups' do
+    let_it_be(:group) { create(:group) }
+    let_it_be(:group_variable) { create(:ci_group_variable, group: group) }
+    let_it_be(:other_variable) { create(:ci_group_variable) }
+
+    it { expect(described_class.for_groups([group.id])).to eq([group_variable]) }
+  end
+
   it_behaves_like 'cleanup by a loose foreign key' do
     let!(:model) { create(:ci_group_variable) }