From 4f0d8443d8424a993145c6926aaf4104aacb0927 Mon Sep 17 00:00:00 2001
From: Pedro Pombeiro <noreply@pedro.pombei.ro>
Date: Fri, 23 Sep 2022 17:58:23 +0000
Subject: [PATCH] GraphQL: Add ALL_AVAILABLE membership value
Returns all runners available to group
Changelog: added
---
app/finders/ci/runners_finder.rb | 6 ++
.../types/ci/runner_membership_filter_enum.rb | 8 ++
app/models/ci/runner.rb | 11 +++
app/policies/group_policy.rb | 8 ++
.../runners_finder_all_available.yml | 8 ++
doc/api/graphql/reference/index.md | 1 +
spec/finders/ci/runners_finder_spec.rb | 59 ++++++++++++-
spec/models/ci/runner_spec.rb | 59 ++++++++++---
spec/policies/group_policy_spec.rb | 84 +++++++++++++++++++
9 files changed, 230 insertions(+), 14 deletions(-)
create mode 100644 config/feature_flags/development/runners_finder_all_available.yml
diff --git a/app/finders/ci/runners_finder.rb b/app/finders/ci/runners_finder.rb
index 774947a35b7d1..d0d98a5967784 100644
--- a/app/finders/ci/runners_finder.rb
+++ b/app/finders/ci/runners_finder.rb
@@ -55,6 +55,12 @@ def group_runners
Ci::Runner.belonging_to_group(@group.id)
when :descendants, nil
Ci::Runner.belonging_to_group_or_project_descendants(@group.id)
+ when :all_available
+ unless can?(@current_user, :read_group_all_available_runners, @group)
+ raise Gitlab::Access::AccessDeniedError
+ end
+
+ Ci::Runner.usable_from_scope(@group)
else
raise ArgumentError, 'Invalid membership filter'
end
diff --git a/app/graphql/types/ci/runner_membership_filter_enum.rb b/app/graphql/types/ci/runner_membership_filter_enum.rb
index 4fd7e0749b0e0..5c895ebcb48e8 100644
--- a/app/graphql/types/ci/runner_membership_filter_enum.rb
+++ b/app/graphql/types/ci/runner_membership_filter_enum.rb
@@ -15,6 +15,14 @@ class RunnerMembershipFilterEnum < BaseEnum
description: "Include runners that have either a direct or inherited relationship. " \
"These runners can be specific to a project or a group.",
value: :descendants
+
+ value 'ALL_AVAILABLE',
+ description:
+ "Include all runners. This list includes runners for all projects in the group " \
+ "and subgroups, as well as for the parent groups and instance. " \
+ "Will not return runners if `runners_finder_all_available` feature flag is disabled.",
+ value: :all_available,
+ deprecated: { milestone: '15.5', reason: :alpha }
end
end
end
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index dbb68c54bcdf2..fcb99d0ec2d4a 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -152,6 +152,17 @@ class Runner < Ci::ApplicationRecord
)
end
+ scope :usable_from_scope, -> (group) do
+ from_union(
+ [
+ belonging_to_group(group.ancestor_ids),
+ belonging_to_group_or_project_descendants(group.id),
+ group.shared_runners
+ ],
+ remove_duplicates: false
+ )
+ end
+
scope :assignable_for, ->(project) do
# FIXME: That `to_sql` is needed to workaround a weird Rails bug.
# Without that, placeholders would miss one and couldn't match.
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 746730a8fb2e1..a2dfce6748fd0 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -87,6 +87,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
Feature.disabled?(:runner_registration_control) || Gitlab::CurrentSettings.valid_runner_registrars.include?('group')
end
+ condition(:runners_finder_all_available, scope: :subject) do
+ Feature.enabled?(:runners_finder_all_available, group)
+ end
+
rule { can?(:read_group) & design_management_enabled }.policy do
enable :read_design_activity
end
@@ -150,8 +154,12 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
enable :admin_crm_organization
enable :admin_crm_contact
enable :read_cluster
+
+ enable :read_group_all_available_runners
end
+ rule { ~runners_finder_all_available }.prevent :read_group_all_available_runners
+
rule { reporter }.policy do
enable :reporter_access
enable :read_container_image
diff --git a/config/feature_flags/development/runners_finder_all_available.yml b/config/feature_flags/development/runners_finder_all_available.yml
new file mode 100644
index 0000000000000..fbcf28c4cde0a
--- /dev/null
+++ b/config/feature_flags/development/runners_finder_all_available.yml
@@ -0,0 +1,8 @@
+---
+name: runners_finder_all_available
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96770
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/374525
+milestone: '15.5'
+type: development
+group: group::runner
+default_enabled: false
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index 4946c21d5a84f..d8fd02e177950 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -19853,6 +19853,7 @@ Values for filtering runners in namespaces. The previous type name `RunnerMember
| Value | Description |
| ----- | ----------- |
+| <a id="cirunnermembershipfilterall_available"></a>`ALL_AVAILABLE` **{warning-solid}** | **Introduced** in 15.5. This feature is in Alpha. It can be changed or removed at any time. Include all runners. This list includes runners for all projects in the group and subgroups, as well as for the parent groups and instance. Will not return runners if `runners_finder_all_available` feature flag is disabled. |
| <a id="cirunnermembershipfilterdescendants"></a>`DESCENDANTS` | Include runners that have either a direct or inherited relationship. These runners can be specific to a project or a group. |
| <a id="cirunnermembershipfilterdirect"></a>`DIRECT` | Include runners that have a direct relationship. |
diff --git a/spec/finders/ci/runners_finder_spec.rb b/spec/finders/ci/runners_finder_spec.rb
index 8d3c375385ae0..62663cd5c272d 100644
--- a/spec/finders/ci/runners_finder_spec.rb
+++ b/spec/finders/ci/runners_finder_spec.rb
@@ -319,6 +319,33 @@ def execute
end
end
+ context 'with :all_available membership' do
+ let(:membership) { :all_available }
+
+ context 'with runners_finder_all_available FF disabled' do
+ before do
+ stub_feature_flags(runners_finder_all_available: false)
+ end
+
+ it 'returns no runners' do
+ expect(subject).to be_empty
+ end
+ end
+
+ context 'with runners_finder_all_available FF enabled' do
+ before do
+ stub_feature_flags(runners_finder_all_available: [target_group])
+ end
+
+ it 'returns runners available to group' do
+ expect(subject).to match_array([runner_project_7, runner_project_6, runner_project_5,
+ runner_project_4, runner_project_3, runner_project_2,
+ runner_project_1, runner_sub_group_4, runner_sub_group_3,
+ runner_sub_group_2, runner_sub_group_1, runner_group, runner_instance])
+ end
+ end
+ end
+
context 'with unknown membership' do
let(:membership) { :unsupported }
@@ -400,11 +427,37 @@ def execute
with_them do
before do
- create(:group_member, user_permission, group: group, user: user)
+ create(:group_member, user_permission, group: sub_group_1, user: user)
end
- it 'returns no runners' do
- expect(subject).to be_empty
+ context 'with :sub_group_1 as target group' do
+ let(:target_group) { sub_group_1 }
+
+ it 'returns no runners' do
+ is_expected.to be_empty
+ end
+ end
+
+ context 'with :group as target group' do
+ let(:target_group) { group }
+
+ it 'returns no runners' do
+ is_expected.to be_empty
+ end
+
+ context 'with :all_available membership' do
+ let(:membership) { :all_available }
+
+ context 'with runners_finder_all_available FF enabled' do
+ before do
+ stub_feature_flags(runners_finder_all_available: [target_group])
+ end
+
+ it 'returns no runners' do
+ expect(subject).to be_empty
+ end
+ end
+ end
end
end
end
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index b4a5774ea3337..13eb7086586aa 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -1595,33 +1595,59 @@ def does_db_update
end
describe '.belonging_to_group_and_ancestors' do
- subject(:relation) { described_class.belonging_to_group_and_ancestors(scope.id) }
+ subject(:relation) { described_class.belonging_to_group_and_ancestors(child_group.id) }
+
+ it 'returns the group runners from the group and parent group' do
+ is_expected.to contain_exactly(child_group_runner, top_level_group_runner)
+ end
+ end
+
+ describe '.belonging_to_group_or_project_descendants' do
+ subject(:relation) { described_class.belonging_to_group_or_project_descendants(scope.id) }
context 'with scope set to top_level_group' do
let(:scope) { top_level_group }
- it 'returns the group runners from the group' do
- is_expected.to contain_exactly(top_level_group_runner)
+ it 'returns the expected group and project runners without duplicates', :aggregate_failures do
+ expect(relation).to contain_exactly(
+ top_level_group_runner,
+ top_level_group_project_runner,
+ child_group_runner,
+ child_group_project_runner,
+ child_group2_runner,
+ shared_top_level_group_project_runner
+ )
+
+ # Ensure no duplicates are returned
+ expect(relation.distinct).to match_array(relation)
end
end
context 'with scope set to child_group' do
let(:scope) { child_group }
- it 'returns the group runners from the group and parent group' do
- is_expected.to contain_exactly(child_group_runner, top_level_group_runner)
+ it 'returns the expected group and project runners without duplicates', :aggregate_failures do
+ expect(relation).to contain_exactly(
+ child_group_runner,
+ child_group_project_runner,
+ shared_top_level_group_project_runner
+ )
+
+ # Ensure no duplicates are returned
+ expect(relation.distinct).to match_array(relation)
end
end
end
- describe '.belonging_to_group_or_project_descendants' do
- subject(:relation) { described_class.belonging_to_group_or_project_descendants(scope.id) }
+ describe '.usable_from_scope' do
+ subject(:relation) { described_class.usable_from_scope(scope) }
context 'with scope set to top_level_group' do
let(:scope) { top_level_group }
- it 'returns the expected group and project runners without duplicates', :aggregate_failures do
+ it 'returns all runners usable from top_level_group without duplicates' do
expect(relation).to contain_exactly(
+ instance_runner,
top_level_group_runner,
top_level_group_project_runner,
child_group_runner,
@@ -1638,15 +1664,26 @@ def does_db_update
context 'with scope set to child_group' do
let(:scope) { child_group }
- it 'returns the expected group and project runners without duplicates', :aggregate_failures do
+ it 'returns all runners usable from child_group' do
expect(relation).to contain_exactly(
+ instance_runner,
+ top_level_group_runner,
child_group_runner,
child_group_project_runner,
shared_top_level_group_project_runner
)
+ end
+ end
- # Ensure no duplicates are returned
- expect(relation.distinct).to match_array(relation)
+ context 'with scope set to other_top_level_group' do
+ let(:scope) { other_top_level_group }
+
+ it 'returns all runners usable from other_top_level_group' do
+ expect(relation).to contain_exactly(
+ instance_runner,
+ other_top_level_group_runner,
+ other_top_level_group_project_runner
+ )
end
end
end
diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index da0270c15b990..5f8e582b1aaf7 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -1266,6 +1266,90 @@
end
end
+ describe 'read_group_all_available_runners' do
+ context 'admin' do
+ let(:current_user) { admin }
+
+ context 'when admin mode is enabled', :enable_admin_mode do
+ context 'with runners_finder_all_available FF disabled' do
+ before do
+ stub_feature_flags(runners_finder_all_available: false)
+ end
+
+ specify { is_expected.to be_disallowed(:read_group_all_available_runners) }
+ end
+
+ context 'with runners_finder_all_available FF enabled' do
+ before do
+ stub_feature_flags(runners_finder_all_available: [group])
+ end
+
+ specify { is_expected.to be_allowed(:read_group_all_available_runners) }
+ end
+ end
+
+ context 'when admin mode is disabled' do
+ specify { is_expected.to be_disallowed(:read_group_all_available_runners) }
+ end
+ end
+
+ context 'with owner' do
+ let(:current_user) { owner }
+
+ context 'with runners_finder_all_available FF disabled' do
+ before do
+ stub_feature_flags(runners_finder_all_available: false)
+ end
+
+ specify { is_expected.to be_disallowed(:read_group_all_available_runners) }
+ end
+
+ context 'with runners_finder_all_available FF enabled' do
+ before do
+ stub_feature_flags(runners_finder_all_available: [group])
+ end
+
+ specify { is_expected.to be_allowed(:read_group_all_available_runners) }
+ end
+ end
+
+ context 'with maintainer' do
+ let(:current_user) { maintainer }
+
+ specify { is_expected.to be_allowed(:read_group_all_available_runners) }
+ end
+
+ context 'with developer' do
+ let(:current_user) { developer }
+
+ specify { is_expected.to be_allowed(:read_group_all_available_runners) }
+ end
+
+ context 'with reporter' do
+ let(:current_user) { reporter }
+
+ specify { is_expected.to be_disallowed(:read_group_all_available_runners) }
+ end
+
+ context 'with guest' do
+ let(:current_user) { guest }
+
+ specify { is_expected.to be_disallowed(:read_group_all_available_runners) }
+ end
+
+ context 'with non member' do
+ let(:current_user) { create(:user) }
+
+ specify { is_expected.to be_disallowed(:read_group_all_available_runners) }
+ end
+
+ context 'with anonymous' do
+ let(:current_user) { nil }
+
+ specify { is_expected.to be_disallowed(:read_group_all_available_runners) }
+ end
+ end
+
describe 'change_prevent_sharing_groups_outside_hierarchy' do
context 'with owner' do
let(:current_user) { owner }
--
GitLab