Skip to content

Improve MR approval policy handling of pipeline failure errors

Description

Today, if a pipeline failure occurs, it appears like this, with a bot comment that feels irrelevant/unuseful:

MR view Pipeline view
image image

The bot comment should likely either not appear or provide some more meaningful feedback regarding the policy error. Else it may be spam and distracting.

It's true that the policy would be a violation in this state since the pipeline didn't run at all, but we may want a more generic or useful message here.

Implementation plan

diff --git a/ee/app/models/security/scan_result_policy_violation.rb b/ee/app/models/security/scan_result_policy_violation.rb
index 8fcfb0e9ca24..051efcc2e3f0 100644
--- a/ee/app/models/security/scan_result_policy_violation.rb
+++ b/ee/app/models/security/scan_result_policy_violation.rb
@@ -38,7 +38,8 @@ class ScanResultPolicyViolation < ApplicationRecord
       scan_removed: 'SCAN_REMOVED',
       target_pipeline_missing: 'TARGET_PIPELINE_MISSING',
       artifacts_missing: 'ARTIFACTS_MISSING',
-      evaluation_skipped: 'EVALUATION_SKIPPED'
+      evaluation_skipped: 'EVALUATION_SKIPPED',
+      pipeline_failed: 'PIPELINE_FAILED'
     }.freeze
 
     MAX_VIOLATIONS = 10
diff --git a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb
index 5028c0d10c25..a82b4e3461ed 100644
--- a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb
+++ b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb
@@ -44,7 +44,7 @@ def update_for_report_type(merge_request, report_type, approval_rules)
       policy_evaluation = Security::SecurityOrchestrationPolicies::PolicyRuleEvaluationService.new(merge_request)
 
       applicable_rules.each do |rule|
-        policy_evaluation.error!(rule, :artifacts_missing, context: validation_context(report_type))
+        policy_evaluation.error!(rule, pipeline_error, context: validation_context(report_type))
       end
 
       policy_evaluation.save
@@ -70,6 +70,10 @@ def unblock_fail_open_rules(report_type)
         .execute
     end
 
+    def pipeline_error
+      pipeline.failed? ? :pipeline_failed : :artifacts_missing
+    end
+
     def validation_context(report_type)
       return if pipeline.nil?
 
diff --git a/ee/lib/security/scan_result_policies/policy_violation_details.rb b/ee/lib/security/scan_result_policies/policy_violation_details.rb
index f35808187078..0dd1155e6913 100644
--- a/ee/lib/security/scan_result_policies/policy_violation_details.rb
+++ b/ee/lib/security/scan_result_policies/policy_violation_details.rb
@@ -32,7 +32,9 @@ class PolicyViolationDetails
         },
         'EVALUATION_SKIPPED' => 'Policy `%{policy}` could not be evaluated within the specified timeframe ' \
           'and, as a result, approvals are required for the policy. ' \
-          'Ensure that scanners are present in the latest pipeline.'
+          'Ensure that scanners are present in the latest pipeline.',
+        'PIPELINE_FAILED' => 'Policy `%{policy}` could not be evaluated because the latest pipeline failed. ' \
+          'Ensure that the pipeline is configured properly and the scanners are present.'
       }.freeze
 
       # These messages correspond to the possible errors above and are shown when a policy is configured to fail open

Verification steps

  1. Create a project
  2. Configure a merge request approval policy
  3. Add an invalid .gitlab-ci.yml into the project so that the pipeline fails to run
    invalid
  4. Create MR (update README.md)
  5. Verify the message
  6. Update .gitlab-ci.yml so that pipeline starts, runs the scanners but fails afterwards
    include:
      - template: Jobs/Secret-Detection.gitlab-ci.yml
    
    project-job:
      stage: test
      script: sleep 10
    
    deploy-job:
      stage: deploy
      script: exit 1
  7. Create another MR
  8. Verify that no comment is created and there are no violations
Edited by Martin Cavoj