Merge branch '336436-user-ci_owned_runners' into 'master'

Fix cross join in User#ci_owned_runners

See merge request gitlab-org/gitlab!78216

app/controllers/projects/settings/ci_cd_controller.rb

@@ -26,9 +26,13 @@ module Projects
           ).to_json
         end
 
-        # @assignable_runners is using ci_owned_runners
-        ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436') do
+        if current_user.ci_owned_runners_cross_joins_fix_enabled?
           render
+        else
+          # @assignable_runners is using ci_owned_runners
+          ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436') do
+            render
+          end
         end
       end
 

app/models/ci/namespace_mirror.rb

@@ -10,6 +10,10 @@ module Ci
       where('traversal_ids @> ARRAY[?]::int[]', id)
     end
 
+    scope :contains_any_of_namespaces, -> (ids) do
+      where('traversal_ids && ARRAY[?]::int[]', ids)
+    end
+
     scope :by_namespace_id, -> (namespace_id) { where(namespace_id: namespace_id) }
 
     class << self

app/models/user.rb

@@ -1605,23 +1605,32 @@ class User < ApplicationRecord
 
   def ci_owned_runners
     @ci_owned_runners ||= begin
-      project_runners = Ci::RunnerProject
-        .where(project: authorized_projects(Gitlab::Access::MAINTAINER))
-        .joins(:runner)
-        .select('ci_runners.*')
-
-      group_runners = Ci::RunnerNamespace
-        .where(namespace_id: owned_groups.self_and_descendant_ids)
-        .joins(:runner)
-        .select('ci_runners.*')
-
-      Ci::Runner.from_union([project_runners, group_runners]).allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436')
+      if ci_owned_runners_cross_joins_fix_enabled?
+        Ci::Runner
+          .from_union([ci_owned_project_runners_from_project_members,
+                       ci_owned_project_runners_from_group_members,
+                       ci_owned_group_runners])
+      else
+        Ci::Runner
+          .from_union([ci_legacy_owned_project_runners, ci_legacy_owned_group_runners])
+          .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436')
+      end
     end
   end
 
   def owns_runner?(runner)
-    ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436') do
+    if ci_owned_runners_cross_joins_fix_enabled?
       ci_owned_runners.exists?(runner.id)
+    else
+      ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436') do
+        ci_owned_runners.exists?(runner.id)
+      end
+    end
+  end
+
+  def ci_owned_runners_cross_joins_fix_enabled?
+    strong_memoize(:ci_owned_runners_cross_joins_fix_enabled) do
+      Feature.enabled?(:ci_owned_runners_cross_joins_fix, self, default_enabled: :yaml)
     end
   end
 
@@ -2199,6 +2208,50 @@ class User < ApplicationRecord
 
     ::Gitlab::Auth::Ldap::Access.allowed?(self)
   end
+
+  def ci_legacy_owned_project_runners
+    Ci::RunnerProject
+      .select('ci_runners.*')
+      .joins(:runner)
+      .where(project: authorized_projects(Gitlab::Access::MAINTAINER))
+  end
+
+  def ci_legacy_owned_group_runners
+    Ci::RunnerNamespace
+      .select('ci_runners.*')
+      .joins(:runner)
+      .where(namespace_id: owned_groups.self_and_descendant_ids)
+  end
+
+  def ci_owned_project_runners_from_project_members
+    Ci::RunnerProject
+      .select('ci_runners.*')
+      .joins(:runner)
+      .where(project: project_members.where('access_level >= ?', Gitlab::Access::MAINTAINER).pluck(:source_id))
+  end
+
+  def ci_owned_project_runners_from_group_members
+    Ci::RunnerProject
+      .select('ci_runners.*')
+      .joins(:runner)
+      .joins('JOIN ci_project_mirrors ON ci_project_mirrors.project_id = ci_runner_projects.project_id')
+      .joins('JOIN ci_namespace_mirrors ON ci_namespace_mirrors.namespace_id = ci_project_mirrors.namespace_id')
+      .merge(ci_namespace_mirrors_for_group_members(Gitlab::Access::MAINTAINER))
+  end
+
+  def ci_owned_group_runners
+    Ci::RunnerNamespace
+      .select('ci_runners.*')
+      .joins(:runner)
+      .joins('JOIN ci_namespace_mirrors ON ci_namespace_mirrors.namespace_id = ci_runner_namespaces.namespace_id')
+      .merge(ci_namespace_mirrors_for_group_members(Gitlab::Access::OWNER))
+  end
+
+  def ci_namespace_mirrors_for_group_members(level)
+    Ci::NamespaceMirror.contains_any_of_namespaces(
+      group_members.where('access_level >= ?', level).pluck(:source_id)
+    )
+  end
 end
 
 User.prepend_mod_with('User')

config/feature_flags/development/ci_owned_runners_cross_joins_fix.yml

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

spec/controllers/projects/settings/ci_cd_controller_spec.rb

@@ -25,6 +25,19 @@ RSpec.describe Projects::Settings::CiCdController do
       expect(response).to render_template(:show)
     end
 
+    context 'when the FF ci_owned_runners_cross_joins_fix is disabled' do
+      before do
+        stub_feature_flags(ci_owned_runners_cross_joins_fix: false)
+      end
+
+      it 'renders show with 200 status code' do
+        get :show, params: { namespace_id: project.namespace, project_id: project }
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to render_template(:show)
+      end
+    end
+
     context 'with CI/CD disabled' do
       before do
         project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED)

spec/graphql/mutations/ci/runner/delete_spec.rb

@@ -5,9 +5,9 @@ require 'spec_helper'
 RSpec.describe Mutations::Ci::Runner::Delete do
   include GraphqlHelpers
 
-  let_it_be(:user) { create(:user) }
   let_it_be(:runner) { create(:ci_runner) }
 
+  let(:user) { create(:user) }
   let(:current_ctx) { { current_user: user } }
 
   let(:mutation_params) do
@@ -46,10 +46,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do
     end
 
     context 'when user can delete owned runner' do
-      let_it_be(:project) { create(:project, creator_id: user.id) }
-      let_it_be(:project_runner, reload: true) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
+      let!(:project) { create(:project, creator_id: user.id) }
+      let!(:project_runner) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
 
-      before_all do
+      before do
         project.add_maintainer(user)
       end
 
@@ -63,10 +63,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do
       end
 
       context 'with more than one associated project' do
-        let_it_be(:project2) { create(:project, creator_id: user.id) }
-        let_it_be(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2]) }
+        let!(:project2) { create(:project, creator_id: user.id) }
+        let!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2]) }
 
-        before_all do
+        before do
           project2.add_maintainer(user)
         end
 

spec/models/ci/namespace_mirror_spec.rb

@@ -30,6 +30,20 @@ RSpec.describe Ci::NamespaceMirror do
       end
     end
 
+    describe '.contains_any_of_namespaces' do
+      let!(:other_group1) { create(:group) }
+      let!(:other_group2) { create(:group, parent: other_group1) }
+      let!(:other_group3) { create(:group, parent: other_group2) }
+
+      subject(:result) { described_class.contains_any_of_namespaces([group2.id, other_group2.id]) }
+
+      it 'returns groups having group2.id in traversal_ids' do
+        expect(result.pluck(:namespace_id)).to contain_exactly(
+          group2.id, group3.id, group4.id, other_group2.id, other_group3.id
+        )
+      end
+    end
+
     describe '.by_namespace_id' do
       subject(:result) { described_class.by_namespace_id(group2.id) }
 

spec/models/user_spec.rb

@@ -3967,7 +3967,7 @@ RSpec.describe User do
     end
   end
 
-  describe '#ci_owned_runners' do
+  shared_context '#ci_owned_runners' do
     let(:user) { create(:user) }
 
     shared_examples :nested_groups_owner do
@@ -4274,6 +4274,16 @@ RSpec.describe User do
     end
   end
 
+  it_behaves_like '#ci_owned_runners'
+
+  context 'when FF ci_owned_runners_cross_joins_fix is disabled' do
+    before do
+      stub_feature_flags(ci_owned_runners_cross_joins_fix: false)
+    end
+
+    it_behaves_like '#ci_owned_runners'
+  end
+
   describe '#projects_with_reporter_access_limited_to' do
     let(:project1) { create(:project) }
     let(:project2) { create(:project) }