Merge branch 'pedropombeiro/337838/1-add-ascendants-membership-enum-value' into 'master'

GraphQL: Add ALL_AVAILABLE membership value

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/96770



Merged-by: Alexandru Croitor <acroitor@gitlab.com>
Approved-by: Corinna Gogolok <cgogolok@gitlab.com>
Approved-by: Alexandru Croitor <acroitor@gitlab.com>
Approved-by: Dominic Couture <dcouture@gitlab.com>
Co-authored-by: Pedro Pombeiro <noreply@pedro.pombei.ro>

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

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

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.

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

config/feature_flags/development/runners_finder_all_available.yml

+---
+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

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. |

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

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

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 }