Add environment scope to group CI variables

Inspired by project level scoped variables, store an environment
scope for each group variable and use existing matching logic to
select only those that are relevant when passing variables to a
build.

Because the default scope is '*' (all environments) and there is
not yet a way to modify the scope, these changes alone don't
cause a change in behaviour.

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55256

app/models/ci/group_variable.rb

@@ -6,16 +6,18 @@ module Ci
     include Ci::HasVariable
     include Presentable
     include Ci::Maskable
+    prepend HasEnvironmentScope
 
     belongs_to :group, class_name: "::Group"
 
     alias_attribute :secret_value, :value
 
     validates :key, uniqueness: {
-      scope: :group_id,
+      scope: [:group_id, :environment_scope],
       message: "(%{value}) has already been taken"
     }
 
     scope :unprotected, -> { where(protected: false) }
+    scope :by_environment_scope, -> (environment_scope) { where(environment_scope: environment_scope) }
   end
 end

app/models/ci/variable.rb

@@ -18,7 +18,6 @@ module Ci
     }
 
     scope :unprotected, -> { where(protected: false) }
-    scope :by_key, -> (key) { where(key: key) }
     scope :by_environment_scope, -> (environment_scope) { where(environment_scope: environment_scope) }
   end
 end

app/models/concerns/ci/contextable.rb

@@ -20,7 +20,7 @@ module Ci
         variables.concat(user_variables)
         variables.concat(dependency_variables) if dependencies
         variables.concat(secret_instance_variables)
-        variables.concat(secret_group_variables)
+        variables.concat(secret_group_variables(environment: environment))
         variables.concat(secret_project_variables(environment: environment))
         variables.concat(trigger_request.user_variables) if trigger_request
         variables.concat(pipeline.variables)
@@ -85,13 +85,13 @@ module Ci
       project.ci_instance_variables_for(ref: git_ref)
     end
 
-    def secret_group_variables
+    def secret_group_variables(environment: expanded_environment_name)
       return [] unless project.group
 
-      project.group.ci_variables_for(git_ref, project)
+      project.group.ci_variables_for(git_ref, project, environment: environment)
     end
 
-    def secret_project_variables(environment: persisted_environment)
+    def secret_project_variables(environment: expanded_environment_name)
       project.ci_variables_for(ref: git_ref, environment: environment)
     end
 

app/models/concerns/ci/has_variable.rb

@@ -16,6 +16,7 @@ module Ci
         format: { with: /\A[a-zA-Z0-9_]+\z/,
                   message: "can contain only letters, digits and '_'." }
 
+      scope :by_key, -> (key) { where(key: key) }
       scope :order_key_asc, -> { reorder(key: :asc) }
 
       attr_encrypted :value,

app/models/group.rb

@@ -525,15 +525,11 @@ class Group < Namespace
     }
   end
 
-  def ci_variables_for(ref, project)
-    cache_key = "ci_variables_for:group:#{self&.id}:project:#{project&.id}:ref:#{ref}"
+  def ci_variables_for(ref, project, environment: nil)
+    cache_key = "ci_variables_for:group:#{self&.id}:project:#{project&.id}:ref:#{ref}:environment:#{environment}"
 
     ::Gitlab::SafeRequestStore.fetch(cache_key) do
-      list_of_ids = [self] + ancestors
-      variables = Ci::GroupVariable.where(group: list_of_ids)
-      variables = variables.unprotected unless project.protected_for?(ref)
-      variables = variables.group_by(&:group_id)
-      list_of_ids.reverse.flat_map { |group| variables[group.id] }.compact
+      uncached_ci_variables_for(ref, project, environment: environment)
     end
   end
 
@@ -775,6 +771,23 @@ class Group < Namespace
   def enable_shared_runners!
     update!(shared_runners_enabled: true)
   end
+
+  def uncached_ci_variables_for(ref, project, environment: nil)
+    list_of_ids = [self] + ancestors
+    variables = Ci::GroupVariable.where(group: list_of_ids)
+    variables = variables.unprotected unless project.protected_for?(ref)
+
+    if Feature.enabled?(:scoped_group_variables, self, default_enabled: :yaml)
+      variables = if environment
+                    variables.on_environment(environment)
+                  else
+                    variables.where(environment_scope: '*')
+                  end
+    end
+
+    variables = variables.group_by(&:group_id)
+    list_of_ids.reverse.flat_map { |group| variables[group.id] }.compact
+  end
 end
 
 Group.prepend_if_ee('EE::Group')

changelogs/unreleased/environment-scoped-group-variables.yml

+---
+title: Add environment_scope column to ci_group_variables
+merge_request: 55256
+author:
+type: other

config/feature_flags/development/scoped_group_variables.yml

+---
+name: scoped_group_variables
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55256
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323298
+milestone: '13.10'
+type: development
+group: group::configure
+default_enabled: false

db/migrate/20210218040814_add_environment_scope_to_group_variables.rb

+# frozen_string_literal: true
+
+class AddEnvironmentScopeToGroupVariables < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  OLD_INDEX = 'index_ci_group_variables_on_group_id_and_key'
+  NEW_INDEX = 'index_ci_group_variables_on_group_id_and_key_and_environment'
+
+  disable_ddl_transaction!
+
+  def up
+    unless column_exists?(:ci_group_variables, :environment_scope)
+      # rubocop:disable Migration/AddLimitToTextColumns
+      # Added in 20210305013509_add_text_limit_to_group_ci_variables_environment_scope
+      add_column :ci_group_variables, :environment_scope, :text, null: false, default: '*'
+      # rubocop:enable Migration/AddLimitToTextColumns
+    end
+
+    add_concurrent_index :ci_group_variables, [:group_id, :key, :environment_scope], unique: true, name: NEW_INDEX
+    remove_concurrent_index_by_name :ci_group_variables, OLD_INDEX
+  end
+
+  def down
+    remove_duplicates!
+
+    add_concurrent_index :ci_group_variables, [:group_id, :key], unique: true, name: OLD_INDEX
+    remove_concurrent_index_by_name :ci_group_variables, NEW_INDEX
+
+    remove_column :ci_group_variables, :environment_scope
+  end
+
+  private
+
+  def remove_duplicates!
+    execute <<-SQL
+      DELETE FROM ci_group_variables
+      WHERE id NOT IN (
+        SELECT MIN(id)
+        FROM ci_group_variables
+        GROUP BY group_id, key
+      )
+    SQL
+  end
+end

db/migrate/20210305013509_add_text_limit_to_group_ci_variables_environment_scope.rb

+# frozen_string_literal: true
+
+class AddTextLimitToGroupCiVariablesEnvironmentScope < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit :ci_group_variables, :environment_scope, 255
+  end
+
+  def down
+    remove_text_limit :ci_group_variables, :environment_scope
+  end
+end

db/schema_migrations/20210218040814

+6fe34be82f9aee6cbdb729a67d1d4ac0702c8d9774a038bfd4fd9d9cb28b1a2b
\ No newline at end of file

db/schema_migrations/20210305013509

+743344bb057d0e368c69cc3c90f72d560359d0753acf069e7423928c778a140a
\ No newline at end of file

db/structure.sql

@@ -10424,7 +10424,9 @@ CREATE TABLE ci_group_variables (
     created_at timestamp with time zone NOT NULL,
     updated_at timestamp with time zone NOT NULL,
     masked boolean DEFAULT false NOT NULL,
-    variable_type smallint DEFAULT 1 NOT NULL
+    variable_type smallint DEFAULT 1 NOT NULL,
+    environment_scope text DEFAULT '*'::text NOT NULL,
+    CONSTRAINT check_dfe009485a CHECK ((char_length(environment_scope) <= 255))
 );
 
 CREATE SEQUENCE ci_group_variables_id_seq
@@ -21807,7 +21809,7 @@ CREATE INDEX index_ci_deleted_objects_on_pick_up_at ON ci_deleted_objects USING
 
 CREATE INDEX index_ci_freeze_periods_on_project_id ON ci_freeze_periods USING btree (project_id);
 
-CREATE UNIQUE INDEX index_ci_group_variables_on_group_id_and_key ON ci_group_variables USING btree (group_id, key);
+CREATE UNIQUE INDEX index_ci_group_variables_on_group_id_and_key_and_environment ON ci_group_variables USING btree (group_id, key, environment_scope);
 
 CREATE UNIQUE INDEX index_ci_instance_variables_on_key ON ci_instance_variables USING btree (key);
 

spec/migrations/20210218040814_add_environment_scope_to_group_variables_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!('add_environment_scope_to_group_variables')
+
+RSpec.describe AddEnvironmentScopeToGroupVariables do
+  let(:migration) { described_class.new }
+  let(:ci_group_variables) { table(:ci_group_variables) }
+  let(:group) { table(:namespaces).create!(name: 'group', path: 'group') }
+
+  def create_variable!(group, key:, environment_scope: '*')
+    table(:ci_group_variables).create!(
+      group_id: group.id,
+      key: key,
+      environment_scope: environment_scope
+    )
+  end
+
+  describe '#down' do
+    context 'group has variables with duplicate keys' do
+      it 'deletes all but the first record' do
+        migration.up
+
+        remaining_variable = create_variable!(group, key: 'key')
+        create_variable!(group, key: 'key', environment_scope: 'staging')
+        create_variable!(group, key: 'key', environment_scope: 'production')
+
+        migration.down
+
+        expect(ci_group_variables.pluck(:id)).to eq [remaining_variable.id]
+      end
+    end
+
+    context 'group does not have variables with duplicate keys' do
+      it 'does not delete any records' do
+        migration.up
+
+        create_variable!(group, key: 'key')
+        create_variable!(group, key: 'staging')
+        create_variable!(group, key: 'production')
+
+        expect { migration.down }.not_to change { ci_group_variables.count }
+      end
+    end
+  end
+end

spec/models/ci/group_variable_spec.rb

@@ -9,7 +9,17 @@ RSpec.describe Ci::GroupVariable do
 
   it { is_expected.to include_module(Presentable) }
   it { is_expected.to include_module(Ci::Maskable) }
-  it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) }
+  it { is_expected.to include_module(HasEnvironmentScope) }
+  it { is_expected.to validate_uniqueness_of(:key).scoped_to([:group_id, :environment_scope]).with_message(/\(\w+\) has already been taken/) }
+
+  describe '.by_environment_scope' do
+    let!(:matching_variable) { create(:ci_group_variable, environment_scope: 'production ') }
+    let!(:non_matching_variable) { create(:ci_group_variable, environment_scope: 'staging') }
+
+    subject { Ci::GroupVariable.by_environment_scope('production') }
+
+    it { is_expected.to contain_exactly(matching_variable) }
+  end
 
   describe '.unprotected' do
     subject { described_class.unprotected }

spec/models/ci/variable_spec.rb

@@ -14,6 +14,15 @@ RSpec.describe Ci::Variable do
     it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) }
   end
 
+  describe '.by_environment_scope' do
+    let!(:matching_variable) { create(:ci_variable, environment_scope: 'production ') }
+    let!(:non_matching_variable) { create(:ci_variable, environment_scope: 'staging') }
+
+    subject { Ci::Variable.by_environment_scope('production') }
+
+    it { is_expected.to contain_exactly(matching_variable) }
+  end
+
   describe '.unprotected' do
     subject { described_class.unprotected }
 

spec/models/concerns/ci/has_variable_spec.rb

@@ -11,6 +11,17 @@ RSpec.describe Ci::HasVariable do
   it { is_expected.not_to allow_value('foo bar').for(:key) }
   it { is_expected.not_to allow_value('foo/bar').for(:key) }
 
+  describe 'scopes' do
+    describe '.by_key' do
+      let!(:matching_variable) { create(:ci_variable, key: 'example') }
+      let!(:non_matching_variable) { create(:ci_variable, key: 'other') }
+
+      subject { Ci::Variable.by_key('example') }
+
+      it { is_expected.to contain_exactly(matching_variable) }
+    end
+  end
+
   describe '#key=' do
     context 'when the new key is nil' do
       it 'strips leading and trailing whitespaces' do

spec/models/group_spec.rb

@@ -1310,9 +1310,10 @@ RSpec.describe Group do
 
   describe '#ci_variables_for' do
     let(:project) { create(:project, group: group) }
+    let(:environment_scope) { '*' }
 
     let!(:ci_variable) do
-      create(:ci_group_variable, value: 'secret', group: group)
+      create(:ci_group_variable, value: 'secret', group: group, environment_scope: environment_scope)
     end
 
     let!(:protected_variable) do
@@ -1321,13 +1322,16 @@ RSpec.describe Group do
 
     subject { group.ci_variables_for('ref', project) }
 
-    it 'memoizes the result by ref', :request_store do
+    it 'memoizes the result by ref and environment', :request_store do
+      scoped_variable = create(:ci_group_variable, value: 'secret', group: group, environment_scope: 'scoped')
+
       expect(project).to receive(:protected_for?).with('ref').once.and_return(true)
-      expect(project).to receive(:protected_for?).with('other').once.and_return(false)
+      expect(project).to receive(:protected_for?).with('other').twice.and_return(false)
 
       2.times do
-        expect(group.ci_variables_for('ref', project)).to contain_exactly(ci_variable, protected_variable)
+        expect(group.ci_variables_for('ref', project, environment: 'production')).to contain_exactly(ci_variable, protected_variable)
         expect(group.ci_variables_for('other', project)).to contain_exactly(ci_variable)
+        expect(group.ci_variables_for('other', project, environment: 'scoped')).to contain_exactly(ci_variable, scoped_variable)
       end
     end
 
@@ -1364,6 +1368,120 @@ RSpec.describe Group do
       it_behaves_like 'ref is protected'
     end
 
+    context 'when environment name is specified' do
+      let(:environment) { 'review/name' }
+
+      subject do
+        group.ci_variables_for('ref', project, environment: environment)
+      end
+
+      context 'when environment scope is exactly matched' do
+        let(:environment_scope) { 'review/name' }
+
+        it { is_expected.to contain_exactly(ci_variable) }
+      end
+
+      context 'when environment scope is matched by wildcard' do
+        let(:environment_scope) { 'review/*' }
+
+        it { is_expected.to contain_exactly(ci_variable) }
+      end
+
+      context 'when environment scope does not match' do
+        let(:environment_scope) { 'review/*/special' }
+
+        it { is_expected.not_to contain_exactly(ci_variable) }
+      end
+
+      context 'when environment scope has _' do
+        let(:environment_scope) { '*_*' }
+
+        it 'does not treat it as wildcard' do
+          is_expected.not_to contain_exactly(ci_variable)
+        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(ci_variable)
+          end
+        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
+        it 'does not treat it as wildcard' do
+          ci_variable.update_attribute(:environment_scope, '*%*')
+
+          is_expected.not_to contain_exactly(ci_variable)
+        end
+
+        context 'when environment name contains a percent' do
+          let(:environment) { 'foo%bar/test' }
+
+          it 'matches literally for %' do
+            ci_variable.update(environment_scope: 'foo%bar/*')
+
+            is_expected.to contain_exactly(ci_variable)
+          end
+        end
+      end
+
+      context 'when variables with the same name have different environment scopes' do
+        let!(:partially_matched_variable) do
+          create(:ci_group_variable,
+                 key: ci_variable.key,
+                 value: 'partial',
+                 environment_scope: 'review/*',
+                 group: group)
+        end
+
+        let!(:perfectly_matched_variable) do
+          create(:ci_group_variable,
+                 key: ci_variable.key,
+                 value: 'prefect',
+                 environment_scope: 'review/name',
+                 group: group)
+        end
+
+        it 'puts variables matching environment scope more in the end' do
+          is_expected.to eq(
+            [ci_variable,
+             partially_matched_variable,
+             perfectly_matched_variable])
+        end
+      end
+
+      context 'when :scoped_group_variables feature flag is disabled' do
+        before do
+          stub_feature_flags(scoped_group_variables: false)
+        end
+
+        context 'when environment scope is exactly matched' do
+          let(:environment_scope) { 'review/name' }
+
+          it { is_expected.to contain_exactly(ci_variable) }
+        end
+
+        context 'when environment scope is partially matched' do
+          let(:environment_scope) { 'review/*' }
+
+          it { is_expected.to contain_exactly(ci_variable) }
+        end
+
+        context 'when environment scope does not match' do
+          let(:environment_scope) { 'review/*/special' }
+
+          it { is_expected.to contain_exactly(ci_variable) }
+        end
+      end
+    end
+
     context 'when group has children' do
       let(:group_child)      { create(:group, parent: group) }
       let(:group_child_2)    { create(:group, parent: group_child) }