Add unique validation for environment_scope in clusters. Add on_environment...

Add unique validation for environment_scope in clusters. Add on_environment scope for clusters. Add tests

app/models/ci/variable.rb

@@ -3,7 +3,7 @@ module Ci
     extend Gitlab::Ci::Model
     include HasVariable
     include Presentable
-    prepend Ci::HasEnvironmentScope
+    prepend HasEnvironmentScope
 
     belongs_to :project
 

app/models/clusters/cluster.rb

 module Clusters
   class Cluster < ActiveRecord::Base
     include Presentable
-    prepend Ci::HasEnvironmentScope
+    prepend HasEnvironmentScope
 
     self.table_name = 'clusters'
 
@@ -27,7 +27,7 @@ module Clusters
     accepts_nested_attributes_for :platform_kubernetes, update_only: true
 
     validates :name, cluster_name: true
-    validate :unique_environment_scope
+    validate :unique_environment_scope, if: :has_project?
     validate :restrict_modification, on: :update
 
     delegate :status, to: :provider, allow_nil: true
@@ -94,7 +94,7 @@ module Clusters
     private
 
     def unique_environment_scope
-      if project.clusters.where(environment_scope: environment_scope).exists?
+      if project.clusters.where(environment_scope: environment_scope).where.not(id: self.id).exists?
         errors.add(:base, "cannot add duplicated environment scope")
         return false
       end
@@ -110,5 +110,9 @@ module Clusters
 
       true
     end
+
+    def has_project?
+      projects.exists?
+    end
   end
 end

app/models/project.rb

@@ -905,7 +905,7 @@ class Project < ActiveRecord::Base
     @ci_service ||= ci_services.reorder(nil).find_by(active: true)
   end
 
-  def deployment_platform
+  def deployment_platform(environment: nil)
     @deployment_platform ||= clusters.find_by(enabled: true)&.platform_kubernetes
     @deployment_platform ||= services.where(category: :deployment).reorder(nil).find_by(active: true)
   end

ee/app/models/ci/has_environment_scope.rb

-module Ci
-  module HasEnvironmentScope
-    extend ActiveSupport::Concern
-
-    class << self
-      def filter_by(base_query, environment_name)
-        where = <<~SQL
-          environment_scope IN (:wildcard, :environment_name) OR
-            :environment_name LIKE
-              #{::Gitlab::SQL::Glob.to_like('environment_scope')}
-        SQL
-
-        order = <<~SQL
-          CASE environment_scope
-            WHEN %{wildcard} THEN 0
-            WHEN %{environment_name} THEN 2
-            ELSE 1
-          END
-        SQL
-
-        values = {
-          wildcard: '*',
-          environment_name: environment_name
-        }
-
-        quoted_values = values.transform_values do |value|
-          # Note that the connection could be
-          # Gitlab::Database::LoadBalancing::ConnectionProxy
-          # which supports `quote` via `method_missing`
-          self.class.connection.quote(value)
-        end
-
-        # The query is trying to find variables with scopes matching the
-        # current environment name. Suppose the environment name is
-        # 'review/app', and we have variables with environment scopes like:
-        # * variable A: review
-        # * variable B: review/app
-        # * variable C: review/*
-        # * variable D: *
-        # And the query should find variable B, C, and D, because it would
-        # try to convert the scope into a LIKE pattern for each variable:
-        # * A: review
-        # * B: review/app
-        # * C: review/%
-        # * D: %
-        # Note that we'll match % and _ literally therefore we'll escape them.
-        # In this case, B, C, and D would match. We also want to prioritize
-        # the exact matched name, and put * last, and everything else in the
-        # middle. So the order should be: D < C < B
-        query
-          .where(where, values)
-          .order(order % quoted_values) # `order` cannot escape for us!
-      end
-    end
-
-    prepended do
-      validates(
-        :environment_scope,
-        presence: true,
-        format: { with: ::Gitlab::Regex.environment_scope_regex,
-                  message: ::Gitlab::Regex.environment_scope_regex_message }
-      )
-    end
-
-    def environment_scope=(new_environment_scope)
-      super(new_environment_scope.to_s.strip)
-    end
-  end
-end

ee/app/models/concerns/has_environment_scope.rb

+module HasEnvironmentScope
+  extend ActiveSupport::Concern
+
+  prepended do
+    validates(
+      :environment_scope,
+      presence: true,
+      format: { with: ::Gitlab::Regex.environment_scope_regex,
+                message: ::Gitlab::Regex.environment_scope_regex_message }
+    )
+
+    scope :on_environment, -> (environment_name) do
+      where = <<~SQL
+        environment_scope IN (:wildcard, :environment_name) OR
+          :environment_name LIKE
+            #{::Gitlab::SQL::Glob.to_like('environment_scope')}
+      SQL
+
+      order = <<~SQL
+        CASE environment_scope
+          WHEN %{wildcard} THEN 0
+          WHEN %{environment_name} THEN 2
+          ELSE 1
+        END
+      SQL
+
+      values = {
+        wildcard: '*',
+        environment_name: environment_name
+      }
+
+      quoted_values = values.transform_values do |value|
+        # Note that the connection could be
+        # Gitlab::Database::LoadBalancing::ConnectionProxy
+        # which supports `quote` via `method_missing`
+        ActiveRecord::Base.connection.quote(value)
+      end
+
+      # The query is trying to find variables with scopes matching the
+      # current environment name. Suppose the environment name is
+      # 'review/app', and we have variables with environment scopes like:
+      # * variable A: review
+      # * variable B: review/app
+      # * variable C: review/*
+      # * variable D: *
+      # And the query should find variable B, C, and D, because it would
+      # try to convert the scope into a LIKE pattern for each variable:
+      # * A: review
+      # * B: review/app
+      # * C: review/%
+      # * D: %
+      # Note that we'll match % and _ literally therefore we'll escape them.
+      # In this case, B, C, and D would match. We also want to prioritize
+      # the exact matched name, and put * last, and everything else in the
+      # middle. So the order should be: D < C < B
+      where(where, values)
+        .order(order % quoted_values) # `order` cannot escape for us!
+    end
+  end
+
+  def environment_scope=(new_environment_scope)
+    super(new_environment_scope.to_s.strip)
+  end
+end

ee/app/models/ee/project.rb

@@ -257,9 +257,8 @@ module EE
     def deployment_platform(environment: nil)
       return super unless environment && feature_available?(:multiple_clusters)
 
-      @deployment_platform ||= Ci::HasEnvironmentScope
-        .filter_by(clusters.enabled, environment.name)
-        .last&.platform_kubernetes
+      @deployment_platform ||= clusters.enabled.on_environment(environment.name)
+                                               .last&.platform_kubernetes
 
       super # Wildcard or KubernetesService
     end
@@ -268,7 +267,7 @@ module EE
       return super.where(environment_scope: '*') unless
         environment && feature_available?(:variable_environment_scope)
 
-      Ci::HasEnvironmentScope.filter_by(super, environment.name)
+      super.on_environment(environment.name)
     end
 
     def execute_hooks(data, hooks_scope = :push_hooks)

spec/ee/spec/models/concerns/has_environment_scope_spec.rb

+require 'spec_helper'
+
+describe HasEnvironmentScope do
+  subject { build(:ci_variable) }
+
+  it { is_expected.to allow_value('*').for(:environment_scope) }
+  it { is_expected.to allow_value('review/*').for(:environment_scope) }
+  it { is_expected.not_to allow_value('').for(:environment_scope) }
+  it { is_expected.not_to allow_value('<>').for(:environment_scope) }
+
+  it do
+    is_expected.to validate_uniqueness_of(:key)
+      .scoped_to(:project_id, :environment_scope)
+  end
+
+  describe '.on_environment' do
+    let(:project) { create(:project) }
+    let!(:cluster1) { create(:cluster, projects: [project], environment_scope: '*') }
+    let!(:cluster2) { create(:cluster, projects: [project], environment_scope: 'product/*') }
+    let!(:cluster3) { create(:cluster, projects: [project], environment_scope: 'staging/*') }
+    let(:environment_name) { 'product/*' }
+
+    it 'returns scoped objects' do
+      expect(project.clusters.on_environment(environment_name)).to eq([cluster1, cluster2])
+    end
+  end
+
+  describe '#environment_scope=' do
+    context 'when the new environment_scope is nil' do
+      it 'strips leading and trailing whitespaces' do
+        subject.environment_scope = nil
+
+        expect(subject.environment_scope).to eq('')
+      end
+    end
+
+    context 'when the new environment_scope has leadind and trailing whitespaces' do
+      it 'strips leading and trailing whitespaces' do
+        subject.environment_scope = ' * '
+
+        expect(subject.environment_scope).to eq('*')
+      end
+    end
+  end
+end

spec/ee/spec/models/ee/ci/variable_spec.rb

@@ -3,30 +3,8 @@ require 'spec_helper'
 describe Ci::Variable do
   subject { build(:ci_variable) }
 
-  it { is_expected.to allow_value('*').for(:environment_scope) }
-  it { is_expected.to allow_value('review/*').for(:environment_scope) }
-  it { is_expected.not_to allow_value('').for(:environment_scope) }
-
   it do
     is_expected.to validate_uniqueness_of(:key)
       .scoped_to(:project_id, :environment_scope)
   end
-
-  describe '#environment_scope=' do
-    context 'when the new environment_scope is nil' do
-      it 'strips leading and trailing whitespaces' do
-        subject.environment_scope = nil
-
-        expect(subject.environment_scope).to eq('')
-      end
-    end
-
-    context 'when the new environment_scope has leadind and trailing whitespaces' do
-      it 'strips leading and trailing whitespaces' do
-        subject.environment_scope = ' * '
-
-        expect(subject.environment_scope).to eq('*')
-      end
-    end
-  end
 end

spec/ee/spec/models/ee/project_spec.rb

@@ -703,6 +703,145 @@ describe Project do
     end
   end
 
+  describe '#deployment_platform' do
+    let(:project) { create(:project) }
+
+    context 'when environment is specified' do
+      let(:environment) { create(:environment, name: 'review/name') }
+      let!(:default_cluster) { create(:cluster, :provided_by_user, projects: [project], environment_scope: '*') }
+      let!(:cluster) { create(:cluster, :provided_by_user, projects: [project]) }
+
+      subject { project.deployment_platform(environment: environment) }
+
+      shared_examples 'matching environment scope' do
+        context 'when multiple clusters is available' do
+          before do
+            stub_licensed_features(multiple_clusters: true)
+          end
+
+          it 'returns environment specific cluster' do
+            is_expected.to eq(cluster.platform_kubernetes)
+          end
+        end
+
+        context 'when multiple clusters is unavailable' do
+          before do
+            stub_licensed_features(multiple_clusters: false)
+          end
+
+          it 'returns a kubernetes platform' do
+            is_expected.to be_kind_of(Clusters::Platforms::Kubernetes)
+          end
+        end
+      end
+
+      shared_examples 'not matching environment scope' do
+        context 'when multiple clusters is available' do
+          before do
+            stub_licensed_features(multiple_clusters: true)
+          end
+
+          it 'returns default cluster' do
+            is_expected.to eq(default_cluster.platform_kubernetes)
+          end
+        end
+
+        context 'when multiple clusters is unavailable' do
+          before do
+            stub_licensed_features(multiple_clusters: false)
+          end
+
+          it 'returns a kubernetes platform' do
+            is_expected.to be_kind_of(Clusters::Platforms::Kubernetes)
+          end
+        end
+      end
+
+      context 'when environment scope is exactly matched' do
+        before do
+          cluster.update(environment_scope: 'review/name')
+        end
+
+        it_behaves_like 'matching environment scope'
+      end
+
+      context 'when environment scope is matched by wildcard' do
+        before do
+          cluster.update(environment_scope: 'review/*')
+        end
+
+        it_behaves_like 'matching environment scope'
+      end
+
+      context 'when environment scope does not match' do
+        before do
+          cluster.update(environment_scope: 'review/*/special')
+        end
+
+        it_behaves_like 'not matching environment scope'
+      end
+
+      context 'when environment scope has _' do
+        let!(:cluster) { create(:cluster, projects: [project]) }
+
+        before do
+          stub_licensed_features(multiple_clusters: true)
+        end
+
+        it 'does not treat it as wildcard' do
+          cluster.update(environment_scope: 'foo_bar/*')
+
+          is_expected.to eq(default_cluster.platform_kubernetes)
+        end
+
+        it 'matches literally for _' do
+          cluster.update(environment_scope: 'foo_bar/*')
+          environment.update(name: 'foo_bar/test')
+
+          is_expected.to eq(cluster.platform_kubernetes)
+        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!(:cluster) { create(:cluster, projects: [project]) }
+
+        before do
+          stub_licensed_features(multiple_clusters: true)
+        end
+
+        it 'does not treat it as wildcard' do
+          cluster.update_attribute(:environment_scope, '*%*')
+
+          is_expected.to eq(default_cluster.platform_kubernetes)
+        end
+
+        it 'matches literally for _' do
+          cluster.update(environment_scope: 'foo%bar/*')
+          environment.update_attribute(:name, 'foo%bar/test')
+
+          is_expected.to eq(cluster.platform_kubernetes)
+        end
+      end
+
+      context 'when variables with the same name have different environment scopes' do
+        let!(:partially_matched_cluster) { create(:cluster, :provided_by_user, projects: [project], environment_scope: 'review/*') }
+        let!(:perfectly_matched_cluster) { create(:cluster, :provided_by_user, projects: [project], environment_scope: 'review/name') }
+
+        before do
+          stub_licensed_features(multiple_clusters: true)
+        end
+
+        it 'puts variables matching environment scope more in the end' do
+          is_expected.to eq(perfectly_matched_cluster.platform_kubernetes)
+        end
+      end
+    end
+  end
+
   describe '#secret_variables_for' do
     let(:project) { create(:project) }
 

spec/models/ci/variable_spec.rb

@@ -9,6 +9,7 @@ describe Ci::Variable do
       stub_licensed_features(variable_environment_scope: true)
     end
 
+    it { is_expected.to include_module(HasEnvironmentScope) }
     it { is_expected.to include_module(HasVariable) }
     it { is_expected.to include_module(Presentable) }
     it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) }

spec/models/clusters/cluster_spec.rb

@@ -10,6 +10,7 @@ describe Clusters::Cluster do
   it { is_expected.to delegate_method(:status_name).to(:provider) }
   it { is_expected.to delegate_method(:on_creation?).to(:provider) }
   it { is_expected.to respond_to :project }
+  it { is_expected.to include_module(HasEnvironmentScope) }
 
   describe '.enabled' do
     subject { described_class.enabled }
@@ -127,6 +128,33 @@ describe Clusters::Cluster do
         it { expect(cluster.update(enabled: false)).to be_truthy }
       end
     end
+
+    context 'when validates unique_environment_scope' do
+      let(:project) { create(:project) }
+
+      before do
+        create(:cluster, projects: [project], environment_scope: 'product/*')
+      end
+
+      context 'when identical environment scope exists in project' do
+        let(:cluster) { create(:cluster, projects: [project], environment_scope: 'product/*') }
+
+        it { is_expected.to be_falsey }
+      end
+
+      context 'when identical environment scope does not exist in project' do
+        let(:cluster) { create(:cluster, projects: [project], environment_scope: '*') }
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when identical environment scope exists in different project' do
+        let(:project2) { create(:project) }
+        let(:cluster) { create(:cluster, projects: [project2], environment_scope: 'product/*') }
+
+        it { is_expected.to be_truthy }
+      end
+    end
   end
 
   describe '#provider' do