Commit f8fe01e2 authored by Furkan Ayhan's avatar Furkan Ayhan Committed by Marius Bobin

Fix cross join in User#ci_owned_runners

This is behind a FF ci_owned_runners_cross_joins_fix
parent aa6fe0e3
...@@ -26,11 +26,15 @@ module Projects ...@@ -26,11 +26,15 @@ module Projects
).to_json ).to_json
end end
if current_user.ci_owned_runners_cross_joins_fix_enabled?
render
else
# @assignable_runners is using ci_owned_runners # @assignable_runners is using ci_owned_runners
::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436') do ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436') do
render render
end end
end end
end
def update def update
Projects::UpdateService.new(project, current_user, update_params).tap do |service| Projects::UpdateService.new(project, current_user, update_params).tap do |service|
......
...@@ -10,6 +10,10 @@ module Ci ...@@ -10,6 +10,10 @@ module Ci
where('traversal_ids @> ARRAY[?]::int[]', id) where('traversal_ids @> ARRAY[?]::int[]', id)
end 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) } scope :by_namespace_id, -> (namespace_id) { where(namespace_id: namespace_id) }
class << self class << self
......
...@@ -1605,25 +1605,34 @@ class User < ApplicationRecord ...@@ -1605,25 +1605,34 @@ class User < ApplicationRecord
def ci_owned_runners def ci_owned_runners
@ci_owned_runners ||= begin @ci_owned_runners ||= begin
project_runners = Ci::RunnerProject if ci_owned_runners_cross_joins_fix_enabled?
.where(project: authorized_projects(Gitlab::Access::MAINTAINER)) Ci::Runner
.joins(:runner) .from_union([ci_owned_project_runners_from_project_members,
.select('ci_runners.*') ci_owned_project_runners_from_group_members,
ci_owned_group_runners])
group_runners = Ci::RunnerNamespace else
.where(namespace_id: owned_groups.self_and_descendant_ids) Ci::Runner
.joins(:runner) .from_union([ci_legacy_owned_project_runners, ci_legacy_owned_group_runners])
.select('ci_runners.*') .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436')
end
Ci::Runner.from_union([project_runners, group_runners]).allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436')
end end
end end
def owns_runner?(runner) def owns_runner?(runner)
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 ::Gitlab::Database.allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/336436') do
ci_owned_runners.exists?(runner.id) ci_owned_runners.exists?(runner.id)
end end
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
def notification_email_for(notification_group) def notification_email_for(notification_group)
# Return group-specific email address if present, otherwise return global notification email address # Return group-specific email address if present, otherwise return global notification email address
...@@ -2199,6 +2208,50 @@ class User < ApplicationRecord ...@@ -2199,6 +2208,50 @@ class User < ApplicationRecord
::Gitlab::Auth::Ldap::Access.allowed?(self) ::Gitlab::Auth::Ldap::Access.allowed?(self)
end 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 end
User.prepend_mod_with('User') User.prepend_mod_with('User')
---
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
...@@ -25,6 +25,19 @@ RSpec.describe Projects::Settings::CiCdController do ...@@ -25,6 +25,19 @@ RSpec.describe Projects::Settings::CiCdController do
expect(response).to render_template(:show) expect(response).to render_template(:show)
end 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 context 'with CI/CD disabled' do
before do before do
project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED) project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED)
......
...@@ -5,9 +5,9 @@ require 'spec_helper' ...@@ -5,9 +5,9 @@ require 'spec_helper'
RSpec.describe Mutations::Ci::Runner::Delete do RSpec.describe Mutations::Ci::Runner::Delete do
include GraphqlHelpers include GraphqlHelpers
let_it_be(:user) { create(:user) }
let_it_be(:runner) { create(:ci_runner) } let_it_be(:runner) { create(:ci_runner) }
let(:user) { create(:user) }
let(:current_ctx) { { current_user: user } } let(:current_ctx) { { current_user: user } }
let(:mutation_params) do let(:mutation_params) do
...@@ -46,10 +46,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do ...@@ -46,10 +46,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do
end end
context 'when user can delete owned runner' do context 'when user can delete owned runner' do
let_it_be(:project) { create(:project, creator_id: user.id) } let!(: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_runner) { create(:ci_runner, :project, description: 'Project runner', projects: [project]) }
before_all do before do
project.add_maintainer(user) project.add_maintainer(user)
end end
...@@ -63,10 +63,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do ...@@ -63,10 +63,10 @@ RSpec.describe Mutations::Ci::Runner::Delete do
end end
context 'with more than one associated project' do context 'with more than one associated project' do
let_it_be(:project2) { create(:project, creator_id: user.id) } let!(: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!(:two_projects_runner) { create(:ci_runner, :project, description: 'Two projects runner', projects: [project, project2]) }
before_all do before do
project2.add_maintainer(user) project2.add_maintainer(user)
end end
......
...@@ -30,6 +30,20 @@ RSpec.describe Ci::NamespaceMirror do ...@@ -30,6 +30,20 @@ RSpec.describe Ci::NamespaceMirror do
end end
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 describe '.by_namespace_id' do
subject(:result) { described_class.by_namespace_id(group2.id) } subject(:result) { described_class.by_namespace_id(group2.id) }
......
...@@ -3967,7 +3967,7 @@ RSpec.describe User do ...@@ -3967,7 +3967,7 @@ RSpec.describe User do
end end
end end
describe '#ci_owned_runners' do shared_context '#ci_owned_runners' do
let(:user) { create(:user) } let(:user) { create(:user) }
shared_examples :nested_groups_owner do shared_examples :nested_groups_owner do
...@@ -4274,6 +4274,16 @@ RSpec.describe User do ...@@ -4274,6 +4274,16 @@ RSpec.describe User do
end end
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 describe '#projects_with_reporter_access_limited_to' do
let(:project1) { create(:project) } let(:project1) { create(:project) }
let(:project2) { create(:project) } let(:project2) { create(:project) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment