Merge branch '46010-add-more-validations-for-runners-and-runner-type' into 'master'

Improve validations for Ci::Runner#runner_type

See merge request gitlab-org/gitlab-ce!18901

9 files changed, 59 insertions, 24 deletions

diff --git a/app/controllers/admin/runner_projects_controller.rb b/app/controllers/admin/runner_projects_controller.rb
index 7ed2de71028..7aba77d8129 100644
--- a/app/controllers/admin/runner_projects_controller.rb
+++ b/app/controllers/admin/runner_projects_controller.rb
@@ -4,9 +4,7 @@ class Admin::RunnerProjectsController < Admin::ApplicationController
   def create
     @runner = Ci::Runner.find(params[:runner_project][:runner_id])
 
-    runner_project = @runner.assign_to(@project, current_user)
-
-    if runner_project.persisted?
+    if @runner.assign_to(@project, current_user)
       redirect_to admin_runner_path(@runner)
     else
       redirect_to admin_runner_path(@runner), alert: 'Failed adding runner to project'
diff --git a/app/controllers/projects/runner_projects_controller.rb b/app/controllers/projects/runner_projects_controller.rb
index 0ec2490655f..a080724634b 100644
--- a/app/controllers/projects/runner_projects_controller.rb
+++ b/app/controllers/projects/runner_projects_controller.rb
@@ -9,9 +9,8 @@ class Projects::RunnerProjectsController < Projects::ApplicationController
     return head(403) unless can?(current_user, :assign_runner, @runner)
 
     path = project_runners_path(project)
-    runner_project = @runner.assign_to(project, current_user)
 
-    if runner_project.persisted?
+    if @runner.assign_to(project, current_user)
       redirect_to path
     else
       redirect_to path, alert: 'Failed adding runner to project'
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 530eacf4be0..57edd6a4956 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -12,9 +12,9 @@ module Ci
     FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze
 
     has_many :builds
-    has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
+    has_many :runner_projects, inverse_of: :runner, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
     has_many :projects, through: :runner_projects
-    has_many :runner_namespaces
+    has_many :runner_namespaces, inverse_of: :runner
     has_many :groups, through: :runner_namespaces
 
     has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build'
@@ -56,10 +56,15 @@ module Ci
     end
 
     validate :tag_constraints
-    validate :either_projects_or_group
     validates :access_level, presence: true
     validates :runner_type, presence: true
 
+    validate :no_projects, unless: :project_type?
+    validate :no_groups, unless: :group_type?
+    validate :any_project, if: :project_type?
+    validate :exactly_one_group, if: :group_type?
+    validate :validate_is_shared
+
     acts_as_taggable
 
     after_destroy :cleanup_runner_queue
@@ -115,8 +120,15 @@ module Ci
         raise ArgumentError, 'Transitioning a group runner to a project runner is not supported'
       end
 
-      self.save
-      project.runner_projects.create(runner_id: self.id)
+      begin
+        transaction do
+          self.projects << project
+          self.save!
+        end
+      rescue ActiveRecord::RecordInvalid => e
+        self.errors.add(:assign_to, e.message)
+        false
+      end
     end
 
     def display_name
@@ -253,13 +265,33 @@ module Ci
       self.class.owned_or_shared(project_id).where(id: self.id).any?
     end
 
-    def either_projects_or_group
-      if groups.many?
-        errors.add(:runner, 'can only be assigned to one group')
+    def no_projects
+      if projects.any?
+        errors.add(:runner, 'cannot have projects assigned')
+      end
+    end
+
+    def no_groups
+      if groups.any?
+        errors.add(:runner, 'cannot have groups assigned')
+      end
+    end
+
+    def any_project
+      unless projects.any?
+        errors.add(:runner, 'needs to be assigned to at least one project')
+      end
+    end
+
+    def exactly_one_group
+      unless groups.one?
+        errors.add(:runner, 'needs to be assigned to exactly one group')
       end
+    end
 
-      if assigned_to_group? && assigned_to_project?
-        errors.add(:runner, 'can only be assigned either to projects or to a group')
+    def validate_is_shared
+      unless is_shared? == instance_type?
+        errors.add(:is_shared, 'is not equal to instance_type?')
       end
     end
 
diff --git a/app/models/ci/runner_namespace.rb b/app/models/ci/runner_namespace.rb
index 3269f86e8ca..29508fdd326 100644
--- a/app/models/ci/runner_namespace.rb
+++ b/app/models/ci/runner_namespace.rb
@@ -2,8 +2,10 @@ module Ci
   class RunnerNamespace < ActiveRecord::Base
     extend Gitlab::Ci::Model
 
-    belongs_to :runner
-    belongs_to :namespace, class_name: '::Namespace'
+    belongs_to :runner, inverse_of: :runner_namespaces, validate: true
+    belongs_to :namespace, inverse_of: :runner_namespaces, class_name: '::Namespace'
     belongs_to :group, class_name: '::Group', foreign_key: :namespace_id
+
+    validates :runner_id, uniqueness: { scope: :namespace_id }
   end
 end
diff --git a/app/models/ci/runner_project.rb b/app/models/ci/runner_project.rb
index 505d178ba8e..52437047300 100644
--- a/app/models/ci/runner_project.rb
+++ b/app/models/ci/runner_project.rb
@@ -2,8 +2,8 @@ module Ci
   class RunnerProject < ActiveRecord::Base
     extend Gitlab::Ci::Model
 
-    belongs_to :runner
-    belongs_to :project
+    belongs_to :runner, inverse_of: :runner_projects
+    belongs_to :project, inverse_of: :runner_projects
 
     validates :runner_id, uniqueness: { scope: :project_id }
   end
diff --git a/app/models/clusters/applications/runner.rb b/app/models/clusters/applications/runner.rb
index b881b4eaf36..e6f795f3e0b 100644
--- a/app/models/clusters/applications/runner.rb
+++ b/app/models/clusters/applications/runner.rb
@@ -43,7 +43,7 @@ module Clusters
 
       def create_and_assign_runner
         transaction do
-          project.runners.create!(runner_create_params).tap do |runner|
+          Ci::Runner.create!(runner_create_params).tap do |runner|
             update!(runner_id: runner.id)
           end
         end
@@ -53,7 +53,8 @@ module Clusters
         {
           name: 'kubernetes-cluster',
           runner_type: :project_type,
-          tag_list: %w(kubernetes cluster)
+          tag_list: %w(kubernetes cluster),
+          projects: [project]
         }
       end
 
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 3dad4277713..52fe529c016 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -21,7 +21,7 @@ class Namespace < ActiveRecord::Base
   has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :project_statistics
 
-  has_many :runner_namespaces, class_name: 'Ci::RunnerNamespace'
+  has_many :runner_namespaces, inverse_of: :namespace, class_name: 'Ci::RunnerNamespace'
   has_many :runners, through: :runner_namespaces, source: :runner, class_name: 'Ci::Runner'
 
   # This should _not_ be `inverse_of: :namespace`, because that would also set
diff --git a/app/models/project.rb b/app/models/project.rb
index af9fca62dc3..32298fc7f5c 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -236,7 +236,7 @@ class Project < ActiveRecord::Base
   has_many :builds, class_name: 'Ci::Build', inverse_of: :project, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName'
   has_many :build_trace_chunks, class_name: 'Ci::BuildTraceChunk', through: :builds, source: :trace_chunks
-  has_many :runner_projects, class_name: 'Ci::RunnerProject'
+  has_many :runner_projects, class_name: 'Ci::RunnerProject', inverse_of: :project
   has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner'
   has_many :variables, class_name: 'Ci::Variable'
   has_many :triggers, class_name: 'Ci::Trigger'
diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb
index 4291631913a..317d1defbba 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -89,7 +89,10 @@ module Ci
     end
 
     def builds_for_group_runner
-      hierarchy_groups = Gitlab::GroupHierarchy.new(runner.groups).base_and_descendants
+      # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
+      groups = Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
+
+      hierarchy_groups = Gitlab::GroupHierarchy.new(groups).base_and_descendants
       projects = Project.where(namespace_id: hierarchy_groups)
         .with_group_runners_enabled
         .with_builds_enabled