Merge branch 'environment-scoped-group-variables' into 'master'

Add environment scope to group CI variables [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!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

@@ -10555,7 +10555,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
@@ -21956,7 +21958,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

@@ -1318,9 +1318,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
@@ -1329,13 +1330,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
 
@@ -1372,6 +1376,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) }