Merge branch 'mo-fix-ci-runner-projects-and-ci-runner-groups-doing-cross-joins' into 'master'

Fix Ci::Runner#projects and Ci::Runner#groups doing cross-joins

See merge request gitlab-org/gitlab!78372

app/controllers/admin/runners_controller.rb

@@ -85,7 +85,11 @@ class Admin::RunnersController < Admin::ApplicationController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def assign_builds_and_projects
-    @builds = runner.builds.order('id DESC').preload_project_and_pipeline_project.first(30)
+    @builds = runner
+      .builds
+      .order_id_desc
+      .preload_project_and_pipeline_project.first(30)
+
     @projects =
       if params[:search].present?
         ::Project.search(params[:search])
@@ -93,12 +97,10 @@ class Admin::RunnersController < Admin::ApplicationController
         Project.all
       end
 
-    ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659') do
-      @projects = @projects.where.not(id: runner.projects.select(:id)) if runner.projects.any?
-      @projects = @projects.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659')
-      @projects = @projects.inc_routes
-      @projects = @projects.page(params[:page]).per(30).without_count
-    end
+    runner_projects_ids = runner.runner_projects.pluck(:project_id)
+    @projects = @projects.where.not(id: runner_projects_ids) if runner_projects_ids.any?
+    @projects = @projects.inc_routes
+    @projects = @projects.page(params[:page]).per(30).without_count
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end

app/models/ci/runner.rb

@@ -67,7 +67,7 @@ module Ci
 
     has_many :builds
     has_many :runner_projects, inverse_of: :runner, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-    has_many :projects, through: :runner_projects
+    has_many :projects, through: :runner_projects, disable_joins: -> { ::Feature.enabled?(:ci_runner_projects_disable_joins, default_enabled: :yaml) }
     has_many :runner_namespaces, inverse_of: :runner, autosave: true
     has_many :groups, through: :runner_namespaces, disable_joins: true
 
@@ -378,9 +378,7 @@ module Ci
     end
 
     def only_for?(project)
-      ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659') do
-        projects == [project]
-      end
+      !runner_projects.where.not(project_id: project.id).exists?
     end
 
     def short_sha

config/feature_flags/development/ci_runner_projects_disable_joins.yml

+---
+name: ci_runner_projects_disable_joins
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78372
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/350538
+milestone: '14.8'
+type: development
+group: group::pipeline execution
+default_enabled: false

db/migrate/20220120085655_add_ci_runner_project_index_to_runner_id_and_project_id.rb

+# frozen_string_literal: true
+
+class AddCiRunnerProjectIndexToRunnerIdAndProjectId < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  OLD_INDEX_NAME = 'index_ci_runner_projects_on_runner_id'
+  NEW_INDEX_NAME = 'index_ci_runner_projects_on_runner_id_and_project_id'
+  TABLE_NAME = :ci_runner_projects
+
+  def up
+    add_concurrent_index(TABLE_NAME, [:runner_id, :project_id], name: NEW_INDEX_NAME)
+    remove_concurrent_index_by_name(TABLE_NAME, OLD_INDEX_NAME)
+  end
+
+  def down
+    add_concurrent_index(TABLE_NAME, :runner_id, name: OLD_INDEX_NAME)
+    remove_concurrent_index_by_name(TABLE_NAME, NEW_INDEX_NAME)
+  end
+end

db/schema_migrations/20220120085655

+966c42749e9d200f2b7295fdbb86e596c33510f0abbf431d40b09629e5e4a6aa
\ No newline at end of file

db/structure.sql

@@ -25707,7 +25707,7 @@ CREATE UNIQUE INDEX index_ci_runner_namespaces_on_runner_id_and_namespace_id ON
 
 CREATE INDEX index_ci_runner_projects_on_project_id ON ci_runner_projects USING btree (project_id);
 
-CREATE INDEX index_ci_runner_projects_on_runner_id ON ci_runner_projects USING btree (runner_id);
+CREATE INDEX index_ci_runner_projects_on_runner_id_and_project_id ON ci_runner_projects USING btree (runner_id, project_id);
 
 CREATE INDEX index_ci_runners_on_active ON ci_runners USING btree (active, id);
 

lib/api/entities/ci/runner_details.rb

@@ -15,18 +15,18 @@ module API
         # rubocop: disable CodeReuse/ActiveRecord
         expose :projects, with: Entities::BasicProjectDetails do |runner, options|
           if options[:current_user].admin? # rubocop: disable Cop/UserAdmin
-            runner.projects.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659')
+            runner.projects
           else
-            options[:current_user].authorized_projects.where(id: runner.projects).allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659')
+            options[:current_user].authorized_projects.where(id: runner.runner_projects.pluck(:project_id))
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord
         # rubocop: disable CodeReuse/ActiveRecord
         expose :groups, with: Entities::BasicGroupDetails do |runner, options|
           if options[:current_user].admin? # rubocop: disable Cop/UserAdmin
-            runner.groups.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659')
+            runner.groups
           else
-            options[:current_user].authorized_groups.where(id: runner.groups).allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338659')
+            options[:current_user].authorized_groups.where(id: runner.runner_namespaces.pluck(:namespace_id))
           end
         end
         # rubocop: enable CodeReuse/ActiveRecord

spec/models/ci/runner_spec.rb

@@ -23,6 +23,26 @@ RSpec.describe Ci::Runner do
     end
   end
 
+  describe 'projects association' do
+    let(:runner) { create(:ci_runner, :project) }
+
+    it 'does not create a cross-database query' do
+      with_cross_joins_prevented do
+        expect(runner.projects.count).to eq(1)
+      end
+    end
+
+    context 'when ci_runner_projects_disable_joins is disabled' do
+      before do
+        stub_feature_flags(ci_runner_projects_disable_joins: false)
+      end
+
+      it 'creates a cross-database query' do
+        expect { runner.projects.count }.to raise_error(Database::PreventCrossJoins::CrossJoinAcrossUnsupportedTablesError)
+      end
+    end
+  end
+
   describe 'validation' do
     it { is_expected.to validate_presence_of(:access_level) }
     it { is_expected.to validate_presence_of(:runner_type) }
@@ -291,6 +311,30 @@ RSpec.describe Ci::Runner do
     end
   end
 
+  describe '#only_for' do
+    let_it_be_with_reload(:runner) { create(:ci_runner, :project) }
+    let_it_be(:project) { runner.projects.first }
+
+    subject { runner.only_for?(project) }
+
+    context 'with matching project' do
+      it { is_expected.to be_truthy }
+    end
+
+    context 'without matching project' do
+      let_it_be(:project) { create(:project) }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'with runner having multiple projects' do
+      let_it_be(:other_project) { create(:project) }
+      let_it_be(:runner_project) { create(:ci_runner_project, project: other_project, runner: runner) }
+
+      it { is_expected.to be_falsey }
+    end
+  end
+
   describe '#assign_to' do
     let(:project) { create(:project) }