Automatic merge of gitlab-org/gitlab master

app/models/ci/runner.rb

@@ -152,11 +152,13 @@ module Ci
     }
 
     scope :owned_or_instance_wide, -> (project_id) do
+      project = project_id.respond_to?(:shared_runners) ? project_id : Project.find(project_id)
+
       from_union(
         [
           belonging_to_project(project_id),
           belonging_to_parent_group_of_project(project_id),
-          instance_type
+          project.shared_runners
         ],
         remove_duplicates: false
       )
@@ -173,7 +175,7 @@ module Ci
       from_union(
         [
           group_and_ancestor_runners,
-          instance_type
+          group.shared_runners
         ],
         remove_duplicates: false
       )

app/models/namespace.rb

@@ -492,6 +492,10 @@ class Namespace < ApplicationRecord
     end
   end
 
+  def shared_runners
+    @shared_runners ||= shared_runners_enabled ? Ci::Runner.instance_type : Ci::Runner.none
+  end
+
   def root?
     !has_parent?
   end

doc/api/runners.md

@@ -448,7 +448,7 @@ Example response:
 
 ## List project's runners
 
-List all runners available in the project, including from ancestor groups and any shared runners.
+List all runners available in the project, including from ancestor groups and [any allowed shared runners](../ci/runners/runners_scope.md#enable-shared-runners).
 
 ```plaintext
 GET /projects/:id/runners
@@ -566,7 +566,7 @@ curl --request DELETE --header "PRIVATE-TOKEN: <your_access_token>" "https://git
 
 ## List group's runners
 
-List all runners available in the group as well as its ancestor groups, including any shared runners.
+List all runners available in the group as well as its ancestor groups, including [any allowed shared runners](../ci/runners/runners_scope.md#enable-shared-runners).
 
 ```plaintext
 GET /groups/:id/runners

doc/development/service_ping/metrics_dictionary.md

@@ -39,7 +39,7 @@ Each metric is defined in a separate YAML file consisting of a number of fields:
 | `product_group`     | yes      | The [group](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/stages.yml) that owns the metric. |
 | `product_category`  | no       | The [product category](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/categories.yml) for the metric. |
 | `value_type`        | yes      | `string`; one of [`string`, `number`, `boolean`, `object`](https://json-schema.org/understanding-json-schema/reference/type.html).                                                     |
-| `status`            | yes      | `string`; [status](#metric-statuses) of the metric, may be set to `active`, `deprecated`, `removed`, `broken`. |
+| `status`            | yes      | `string`; [status](#metric-statuses) of the metric, may be set to `active`, `removed`, `broken`. |
 | `time_frame`        | yes      | `string`; may be set to a value like `7d`, `28d`, `all`, `none`. |
 | `data_source`       | yes      | `string`; may be set to a value like `database`, `redis`, `redis_hll`, `prometheus`, `system`. |
 | `data_category`     | yes      | `string`; [categories](#data-category) of the metric, may be set to `operational`, `optional`, `subscription`, `standard`. The default value is `optional`.|
@@ -60,7 +60,6 @@ Metric definitions can have one of the following statuses:
 
 - `active`: Metric is used and reports data.
 - `broken`: Metric reports broken data (for example, -1 fallback), or does not report data at all. A metric marked as `broken` must also have the `repair_issue_url` attribute.
-- `deprecated`: Metric is deprecated and possibly planned to be removed.
 - `removed`: Metric was removed, but it may appear in Service Ping payloads sent from instances running on older versions of GitLab.
 
 ### Metric value_type

ee/app/services/security/security_orchestration_policies/policy_commit_service.rb

@@ -8,6 +8,9 @@ module Security
 
         return error('Security Policy Project does not exist') unless policy_configuration.present?
 
+        validation_result = validate_policy_yaml
+        return error(validation_result[:message], :bad_request) if validation_result[:status] != :success
+
         process_policy_result = process_policy
         return process_policy_result if process_policy_result[:status] != :success
 
@@ -21,6 +24,12 @@ module Security
 
       private
 
+      def validate_policy_yaml
+        Security::SecurityOrchestrationPolicies::ValidatePolicyService
+          .new(project: project, params: { policy: policy })
+          .execute
+      end
+
       def process_policy
         ProcessPolicyService.new(
           policy_configuration: policy_configuration,

ee/app/services/security/security_orchestration_policies/process_policy_service.rb

@@ -16,7 +16,6 @@ module Security
         name = params[:name]
         operation = params[:operation]
 
-        return error("Invalid policy type", :bad_request) unless Security::OrchestrationPolicyConfiguration::AVAILABLE_POLICY_TYPES.include?(type)
         return error("Name should be same as the policy name", :bad_request) if name && operation != :replace && policy[:name] != name
 
         policy_hash = policy_configuration.policy_hash.dup || {}

ee/app/services/security/security_orchestration_policies/validate_policy_service.rb

+# frozen_string_literal: true
+
+module Security
+  module SecurityOrchestrationPolicies
+    class ValidatePolicyService < ::BaseProjectService
+      def execute
+        return success if policy_disabled?
+
+        return error(s_('SecurityOrchestration|Invalid policy type')) if invalid_policy_type?
+        return error(s_('SecurityOrchestration|Policy cannot be enabled without branch information')) if blank_branch_for_rule?
+        return error(s_('SecurityOrchestration|Policy cannot be enabled for non-existing branches (%{branches})') % { branches: missing_branch_names.join(', ') }) if missing_branch_for_rule?
+
+        success
+      end
+
+      private
+
+      def policy_disabled?
+        !policy&.[](:enabled)
+      end
+
+      def invalid_policy_type?
+        return true if policy[:type].blank?
+
+        !Security::OrchestrationPolicyConfiguration::AVAILABLE_POLICY_TYPES.include?(policy[:type].to_sym)
+      end
+
+      def blank_branch_for_rule?
+        policy[:rules].any? { |rule| rule[:clusters].blank? && rule[:branches].blank? }
+      end
+
+      def missing_branch_for_rule?
+        return false if project.blank?
+
+        missing_branch_names.present?
+      end
+
+      def missing_branch_names
+        strong_memoize(:missing_branch_names) do
+          policy[:rules]
+            .select { |rule| rule[:clusters].blank? }
+            .flat_map { |rule| rule[:branches] }
+            .compact
+            .uniq
+            .select { |pattern| RefMatcher.new(pattern).matching(branches_for_project).blank? }
+        end
+      end
+
+      def policy
+        @policy ||= params[:policy]
+      end
+
+      def branches_for_project
+        strong_memoize(:branches_for_project) do
+          repository.branch_names
+        end
+      end
+    end
+  end
+end

ee/spec/factories/security/policies.rb

@@ -17,11 +17,11 @@ FactoryBot.define do
     sequence(:name) { |n| "test-policy-#{n}" }
     description { 'This policy enforces to run DAST for every pipeline within the project' }
     enabled { true }
-    rules { [{ type: 'pipeline', branches: %w[production] }] }
+    rules { [{ type: 'pipeline', branches: %w[master] }] }
     actions { [{ scan: 'dast', site_profile: 'Site Profile', scanner_profile: 'Scanner Profile' }] }
 
     trait :with_schedule do
-      rules { [{ type: 'schedule', branches: %w[production], cadence: '*/15 * * * *' }] }
+      rules { [{ type: 'schedule', branches: %w[master], cadence: '*/15 * * * *' }] }
     end
   end
 

ee/spec/graphql/mutations/security_policy/commit_scan_execution_policy_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Mutations::SecurityPolicy::CommitScanExecutionPolicy do
 
   describe '#resolve' do
     let_it_be(:user) { create(:user) }
-    let_it_be(:project) { create(:project, namespace: user.namespace) }
+    let_it_be(:project) { create(:project, :repository, namespace: user.namespace) }
     let_it_be(:policy_management_project) { create(:project, :repository, namespace: user.namespace) }
     let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, security_policy_management_project: policy_management_project, project: project) }
     let_it_be(:operation_mode) { Types::MutationOperationModeEnum.enum[:append] }

ee/spec/lib/ee/gitlab/ci/config_spec.rb

@@ -40,7 +40,7 @@ RSpec.describe Gitlab::Ci::Config do
   describe 'with security orchestration policy' do
     let(:source) { 'push' }
 
-    let_it_be(:ref) { 'master' }
+    let(:ref) { 'master' }
     let_it_be_with_refind(:project) { create(:project, :repository) }
 
     let_it_be(:policies_repository) { create(:project, :repository) }
@@ -70,13 +70,15 @@ RSpec.describe Gitlab::Ci::Config do
       end
 
       context 'when policy is not applicable on branch from the pipeline' do
+        let(:ref) { 'another-branch' }
+
         it 'does not modify the config' do
           expect(config.to_hash).to eq(sample_job: { script: ["echo 'test'"] })
         end
       end
 
-      context 'when policy is not applicable on branch from the pipeline' do
-        let_it_be(:ref) { 'production' }
+      context 'when policy is applicable on branch from the pipeline' do
+        let(:ref) { 'master' }
 
         context 'when DAST profiles are not found' do
           it 'adds a job with error message' do

ee/spec/lib/gitlab/ci/config/security_orchestration_policies/processor_spec.rb

@@ -69,21 +69,23 @@ RSpec.describe Gitlab::Ci::Config::SecurityOrchestrationPolicies::Processor do
     end
 
     context 'when policy is not applicable on branch from the pipeline' do
+      let(:ref) { 'refs/head/another-branch' }
+
       it 'does not modify the config' do
         expect(subject).to eq(config)
       end
     end
 
     context 'when ref is a tag' do
-      let_it_be(:ref) { 'refs/tags/v1.1.0' }
+      let(:ref) { 'refs/tags/v1.1.0' }
 
       it 'does not modify the config' do
         expect(subject).to eq(config)
       end
     end
 
-    context 'when policy is not applicable on branch from the pipeline' do
-      let_it_be(:ref) { 'refs/heads/production' }
+    context 'when policy is applicable on branch from the pipeline' do
+      let(:ref) { 'refs/heads/master' }
 
       context 'when DAST profiles are not found' do
         it 'does not modify the config' do

ee/spec/models/security/orchestration_policy_configuration_spec.rb

@@ -226,7 +226,7 @@ RSpec.describe Security::OrchestrationPolicyConfiguration do
 
     let(:expected_active_policies) do
       [
-        build(:scan_execution_policy, name: 'Run DAST in every pipeline'),
+        build(:scan_execution_policy, name: 'Run DAST in every pipeline', rules: [{ type: 'pipeline', branches: %w[production] }]),
         build(:scan_execution_policy, name: 'Run DAST in every pipeline_v1', rules: [{ type: 'pipeline', branches: %w[master] }]),
         build(:scan_execution_policy, name: 'Run DAST in every pipeline_v3', rules: [{ type: 'pipeline', branches: %w[master] }]),
         build(:scan_execution_policy, name: 'Run DAST in every pipeline_v4', rules: [{ type: 'pipeline', branches: %w[master] }]),
@@ -291,7 +291,7 @@ RSpec.describe Security::OrchestrationPolicyConfiguration do
     end
 
     subject(:pipeline_scan_actions) do
-      security_orchestration_policy_configuration.pipeline_scan_actions('refs/heads/production')
+      security_orchestration_policy_configuration.pipeline_scan_actions('refs/heads/master')
     end
 
     it 'returns only actions for pipeline scans applicable for branch' do

ee/spec/requests/api/graphql/mutations/security_policy/commit_scan_execution_policy_spec.rb

@@ -48,7 +48,7 @@ RSpec.describe 'Create scan execution policy for a project' do
     end
 
     context 'when provided policy is invalid' do
-      let_it_be(:policy_yaml) { build(:scan_execution_policy, name: policy_name).merge(type: 'scan_execution_policy', rules: [{ type: 'invalid_type' }]).to_yaml }
+      let_it_be(:policy_yaml) { build(:scan_execution_policy, name: policy_name).merge(type: 'scan_execution_policy', rules: [{ type: 'invalid_type', branches: ['master'] }]).to_yaml }
 
       it 'returns error with detailed information' do
         post_graphql_mutation(mutation, current_user: current_user)

ee/spec/services/security/security_orchestration_policies/policy_commit_service_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
   include RepoHelpers
 
   describe '#execute' do
-    let_it_be(:project) { create(:project) }
+    let_it_be(:project) { create(:project, :repository) }
     let_it_be(:current_user) { project.first_owner }
     let_it_be(:policy_management_project) { create(:project, :repository, creator: current_user) }
     let_it_be(:policy_configuration) { create(:security_orchestration_policy_configuration, security_policy_management_project: policy_management_project, project: project) }
@@ -46,8 +46,21 @@ RSpec.describe Security::SecurityOrchestrationPolicies::PolicyCommitService do
       end
     end
 
+    context 'when defined branch is missing' do
+      let(:policy_hash) { build(:scan_execution_policy, name: 'Test Policy', rules: [{ type: 'pipeline' }]) }
+
+      let(:params) { { policy_yaml: input_policy_yaml, operation: operation } }
+
+      it 'returns error' do
+        response = service.execute
+
+        expect(response[:status]).to eq(:error)
+        expect(response[:message]).to eq('Policy cannot be enabled without branch information')
+      end
+    end
+
     context 'when security_orchestration_policies_configuration does not exist for project' do
-      let_it_be(:project) { create(:project) }
+      let_it_be(:project) { create(:project, :repository) }
 
       it 'does not create new project' do
         response = service.execute

ee/spec/services/security/security_orchestration_policies/process_policy_service_spec.rb

@@ -49,17 +49,6 @@ RSpec.describe Security::SecurityOrchestrationPolicies::ProcessPolicyService do
       end
     end
 
-    context 'when type is invalid' do
-      let(:type) { :invalid_type }
-
-      it 'returns error' do
-        result = service.execute
-
-        expect(result[:status]).to eq(:error)
-        expect(result[:message]).to eq('Invalid policy type')
-      end
-    end
-
     context 'append policy' do
       context 'when policy is present in repository' do
         before do

ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Security::SecurityOrchestrationPolicies::ValidatePolicyService do
+  describe '#execute' do
+    let(:service) { described_class.new(project: project, params: { policy: policy }) }
+    let(:enabled) { true }
+    let(:policy_type) { 'scan_execution_policy' }
+    let(:rule) { { clusters: { production: {} } } }
+    let(:policy) do
+      {
+        type: policy_type,
+        enabled: enabled,
+        rules: [rule]
+      }
+    end
+
+    subject(:result) { service.execute }
+
+    shared_examples 'checks only if policy is enabled' do
+      let(:enabled) { false }
+
+      it { expect(result[:status]).to eq(:success) }
+    end
+
+    shared_examples 'checks policy type' do
+      context 'when policy type is not provided' do
+        let(:policy_type) { nil }
+
+        it { expect(result[:status]).to eq(:error) }
+        it { expect(result[:message]).to eq('Invalid policy type') }
+      end
+
+      context 'when policy type is invalid' do
+        let(:policy_type) { 'invalid_policy_type' }
+
+        it { expect(result[:status]).to eq(:error) }
+        it { expect(result[:message]).to eq('Invalid policy type') }
+      end
+
+      context 'when policy type is valid' do
+        it { expect(result[:status]).to eq(:success) }
+      end
+    end
+
+    shared_examples 'checks if branches are provided in rule' do
+      context 'when rule has clusters defined' do
+        let(:rule) do
+          {
+            clusters: {
+              production: {}
+            },
+            branches: branches
+          }
+        end
+
+        context 'when branches are missing' do
+          let(:branches) { nil }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when branches are provided' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+      end
+
+      context 'when rule does not have clusters defined' do
+        let(:rule) do
+          {
+            branches: branches
+          }
+        end
+
+        context 'when branches are missing' do
+          let(:branches) { nil }
+
+          it { expect(result[:status]).to eq(:error) }
+          it { expect(result[:message]).to eq('Policy cannot be enabled without branch information') }
+
+          it_behaves_like 'checks only if policy is enabled'
+        end
+
+        context 'when branches are provided' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+      end
+    end
+
+    shared_examples 'checks if branches are defined in the project' do
+      context 'when rule has clusters defined' do
+        let(:rule) do
+          {
+            clusters: {
+              production: {}
+            },
+            branches: branches
+          }
+        end
+
+        context 'when branches are defined for project' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when branches are not defined for project' do
+          let(:branches) { ['non-exising-branch'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when pattern does not match any branch defined for project' do
+          let(:branches) { ['master', 'production-*', 'test-*'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+      end
+
+      context 'when rule does not have clusters defined' do
+        let(:rule) do
+          {
+            branches: branches
+          }
+        end
+
+        context 'when branches are defined for project' do
+          let(:branches) { ['master'] }
+
+          it { expect(result[:status]).to eq(:success) }
+        end
+
+        context 'when branches are not defined for project' do
+          let(:branches) { ['non-exising-branch'] }
+
+          it { expect(result[:status]).to eq(:error) }
+          it { expect(result[:message]).to eq('Policy cannot be enabled for non-existing branches (non-exising-branch)') }
+
+          it_behaves_like 'checks only if policy is enabled'
+        end
+
+        context 'when branches are defined as pattern' do
+          context 'when pattern matches at least one branch defined for project' do
+            let(:branches) { ['*'] }
+
+            it { expect(result[:status]).to eq(:success) }
+          end
+
+          context 'when pattern does not match any branch defined for project' do
+            let(:branches) { ['master', 'production-*', 'test-*'] }
+
+            it { expect(result[:status]).to eq(:error) }
+            it { expect(result[:message]).to eq('Policy cannot be enabled for non-existing branches (production-*, test-*)') }
+
+            it_behaves_like 'checks only if policy is enabled'
+          end
+        end
+      end
+    end
+
+    context 'when project is not provided' do
+      let_it_be(:project) { nil }
+
+      it_behaves_like 'checks policy type'
+      it_behaves_like 'checks if branches are provided in rule'
+    end
+
+    context 'when project is provided' do
+      let_it_be(:project) { create(:project, :repository) }
+
+      it_behaves_like 'checks policy type'
+      it_behaves_like 'checks if branches are provided in rule'
+      it_behaves_like 'checks if branches are defined in the project'
+    end
+  end
+end

locale/gitlab.pot

@@ -31863,6 +31863,9 @@ msgstr ""
 msgid "SecurityOrchestration|If you are using Auto DevOps, your %{monospacedStart}auto-deploy-values.yaml%{monospacedEnd} file will not be updated if you change a policy in this section. Auto DevOps users should make changes by following the %{linkStart}Container Network Policy documentation%{linkEnd}."
 msgstr ""
 
+msgid "SecurityOrchestration|Invalid policy type"
+msgstr ""
+
 msgid "SecurityOrchestration|Latest scan"
 msgstr ""
 
@@ -31881,6 +31884,12 @@ msgstr ""
 msgid "SecurityOrchestration|Policies"
 msgstr ""
 
+msgid "SecurityOrchestration|Policy cannot be enabled for non-existing branches (%{branches})"
+msgstr ""
+
+msgid "SecurityOrchestration|Policy cannot be enabled without branch information"
+msgstr ""
+
 msgid "SecurityOrchestration|Policy description"
 msgstr ""
 

spec/models/ci/runner_spec.rb

@@ -265,10 +265,10 @@ RSpec.describe Ci::Runner do
     it_behaves_like '.belonging_to_parent_group_of_project'
   end
 
-  context 'with existing system wide, group and project runners' do
+  context 'with instance runners sharing enabled' do
     # group specific
-    let_it_be(:group) { create(:group) }
-    let_it_be(:project) { create(:project, group: group) }
+    let_it_be(:group) { create(:group, shared_runners_enabled: true) }
+    let_it_be(:project) { create(:project, group: group, shared_runners_enabled: true) }
     let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) }
 
     # project specific
@@ -299,6 +299,40 @@ RSpec.describe Ci::Runner do
     end
   end
 
+  context 'with instance runners sharing disabled' do
+    # group specific
+    let_it_be(:group) { create(:group, shared_runners_enabled: false) }
+    let_it_be(:project) { create(:project, group: group, shared_runners_enabled: false) }
+    let_it_be(:group_runner) { create(:ci_runner, :group, groups: [group]) }
+
+    # project specific
+    let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project]) }
+
+    # globally shared
+    let_it_be(:shared_runner) { create(:ci_runner, :instance) }
+
+    describe '.owned_or_instance_wide' do
+      subject { described_class.owned_or_instance_wide(project.id) }
+
+      it 'returns a project specific and a group specific runner' do
+        is_expected.to contain_exactly(group_runner, project_runner)
+      end
+    end
+
+    describe '.group_or_instance_wide' do
+      subject { described_class.group_or_instance_wide(group) }
+
+      before do
+        # Ensure the project runner is instantiated
+        project_runner
+      end
+
+      it 'returns a group specific runner' do
+        is_expected.to contain_exactly(group_runner)
+      end
+    end
+  end
+
   describe '#display_name' do
     it 'returns the description if it has a value' do
       runner = build(:ci_runner, description: 'Linux/Ruby-1.9.3-p448')