Stop deleting merged MRs approval rules (!141036) · Merge requests · GitLab.org / GitLab

Marcos Rocha requested to merge mc_rocha-stop-deleting-merged-mr-approval-rules-432769 into master Jan 04, 2024

What does this MR do and why?

This MR updates the approval rules update process, not to delete approval rules for merged MRs.

This MR accomplishes that by:

Filtering the unmerged MRs in ee/app/models/concerns/security/scan_result_policy.rb
Updating the foreign key in approval_merge_request_rules that references the scan_result_policies table. Otherwise the approval_merge_request_rules records would still be deleted when the scan_result_policies are recreated in process_scan_result_policy_worker as discussed here.

Migrations

db/migrate/20240103200822_replace_fk_on_approval_merge_request_rules_scan_result_policy_id.rb

main: == 20240103200822 ReplaceFkOnApprovalMergeRequestRulesScanResultPolicyId: migrating 
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- execute("ALTER TABLE approval_merge_request_rules ADD CONSTRAINT fk_approval_merge_request_rules_on_scan_result_policy_id FOREIGN KEY (scan_result_policy_id) REFERENCES scan_result_policies (id) ON DELETE SET NULL NOT VALID;")
main:    -> 0.0089s
main: == 20240103200822 ReplaceFkOnApprovalMergeRequestRulesScanResultPolicyId: migrated (0.1726s)

down

main: == 20240103200822 ReplaceFkOnApprovalMergeRequestRulesScanResultPolicyId: reverting 
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- remove_foreign_key(:approval_merge_request_rules, {:column=>:scan_result_policy_id, :on_delete=>:nullify, :name=>"fk_approval_merge_request_rules_on_scan_result_policy_id"})
main:    -> 0.0032s
main: == 20240103200822 ReplaceFkOnApprovalMergeRequestRulesScanResultPolicyId: reverted (0.0499s)

db/migrate/20240103202629_validate_fk_on_approval_merge_request_rules_scan_result_policy_id.rb

main: == 20240103202629 ValidateFkOnApprovalMergeRequestRulesScanResultPolicyId: migrating 
main: -- execute("SET statement_timeout TO 0")
main:    -> 0.0002s
main: -- execute("ALTER TABLE approval_merge_request_rules VALIDATE CONSTRAINT fk_approval_merge_request_rules_on_scan_result_policy_id;")
main:    -> 0.0051s
main: -- execute("RESET statement_timeout")
main:    -> 0.0002s
main: == 20240103202629 ValidateFkOnApprovalMergeRequestRulesScanResultPolicyId: migrated (0.1559s)

down

main: == 20240103202629 ValidateFkOnApprovalMergeRequestRulesScanResultPolicyId: reverting 
main: == 20240103202629 ValidateFkOnApprovalMergeRequestRulesScanResultPolicyId: reverted (0.0035s)

db/migrate/20240103203314_remove_old_fk_on_approval_merge_request_rules_scan_result_policy_id.rb

main: == 20240103203314 RemoveOldFkOnApprovalMergeRequestRulesScanResultPolicyId: migrating 
main: -- remove_foreign_key(:approval_merge_request_rules, {:column=>:scan_result_policy_id, :on_delete=>:cascade, :name=>"fk_f726c79756"})
main:    -> 0.0034s
main: == 20240103203314 RemoveOldFkOnApprovalMergeRequestRulesScanResultPolicyId: migrated (0.0207s)

down

main: == 20240103203314 RemoveOldFkOnApprovalMergeRequestRulesScanResultPolicyId: reverting 
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- transaction_open?(nil)
main:    -> 0.0000s
main: -- execute("ALTER TABLE approval_merge_request_rules ADD CONSTRAINT fk_f726c79756 FOREIGN KEY (scan_result_policy_id) REFERENCES scan_result_policies (id) ON DELETE CASCADE NOT VALID;")
main:    -> 0.0054s
main: == 20240103203314 RemoveOldFkOnApprovalMergeRequestRulesScanResultPolicyId: reverted (0.0993s)

Queries

delete_scan_finding_rules

Before

DELETE FROM "approval_merge_request_rules"
WHERE "approval_merge_request_rules"."security_orchestration_policy_configuration_id" = 344
    AND "approval_merge_request_rules"."id" >= 57

https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/25143/commands/79865

After

DELETE FROM "approval_merge_request_rules"
WHERE "approval_merge_request_rules"."id" IN (
        SELECT
            "approval_merge_request_rules"."id"
        FROM
            "approval_merge_request_rules"
            INNER JOIN "merge_requests" ON "merge_requests"."id" = "approval_merge_request_rules"."merge_request_id"
        WHERE
            "approval_merge_request_rules"."security_orchestration_policy_configuration_id" = 343
            AND "merge_requests"."state_id" != 3
            AND "approval_merge_request_rules"."id" >= 55)

https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/25143/commands/79866

delete_scan_result_policy_reads(project_id)

Before

DELETE FROM "approval_merge_request_rules"
WHERE "approval_merge_request_rules"."id" IN (
        SELECT
            "approval_merge_request_rules"."id"
        FROM
            "approval_merge_request_rules"
            INNER JOIN "merge_requests" ON "merge_requests"."id" = "approval_merge_request_rules"."merge_request_id"
        WHERE
            "approval_merge_request_rules"."security_orchestration_policy_configuration_id" = 347
            AND "merge_requests"."target_project_id" = 399
            AND "approval_merge_request_rules"."id" >= 65)

https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/25143/commands/79867

After

DELETE FROM "approval_merge_request_rules"
WHERE "approval_merge_request_rules"."id" IN (
        SELECT
            "approval_merge_request_rules"."id"
        FROM
            "approval_merge_request_rules"
            INNER JOIN "merge_requests" ON "merge_requests"."id" = "approval_merge_request_rules"."merge_request_id"
        WHERE
            "approval_merge_request_rules"."security_orchestration_policy_configuration_id" = 346
            AND "merge_requests"."state_id" != 3
            AND "merge_requests"."target_project_id" = 396
            AND "approval_merge_request_rules"."id" >= 62)

https://postgres.ai/console/gitlab/gitlab-production-tunnel-pg12/sessions/25143/commands/79868

Related to #432769 (closed)

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

Create a new project
Add a member with developer access
Add a .gitlab-ci.yml file with the content

include:
  - template: Jobs/Dependency-Scanning.gitlab-ci.yml

Add an empty Gemfile.lock file
Go to Secure > Policies.
Click on New Policy.
Select Scan result policy.
Change to yaml mode and copy the yaml content below, adjusting the user_approvers_ids as needed:

name: Deny new MIT licensed dependencies
description: ''
enabled: true
actions:
- type: require_approval
  approvals_required: 1
  user_approvers_ids:
  - 49
rules:
- type: license_finding
  match_on_inclusion: true
  license_types:
  - MIT
  license_states:
  - newly_detected
  branch_type: protected
approval_settings:
  block_protected_branch_modification:
    enabled: true

Click on Configure with a merge request
Merge the new MR to add the new policy.
Create a new MR
Check the approval rules using the rails console

ApprovalMergeRequestRule.last
=> #<ApprovalMergeRequestRule:0x0000000281925580
 id: 1061,
 created_at: Thu, 04 Jan 2024 22:08:23.858045000 UTC +00:00,
 updated_at: Thu, 04 Jan 2024 22:08:23.858045000 UTC +00:00,
 merge_request_id: 385,
 approvals_required: 0,
 name: "Deny new MIT licensed dependencies",
 rule_type: "report_approver",
 report_type: "license_scanning",
 section: nil,
 modified_from_project_rule: false,
 orchestration_policy_idx: 0,
 vulnerabilities_allowed: 0,
 scanners: [],
 severity_levels: [],
 vulnerability_states: ["newly_detected"],
 security_orchestration_policy_configuration_id: 100,
 scan_result_policy_id: 523,
 applicable_post_merge: nil>

Note the ApprovalMergeRequestRule is linked to a scan_result_policy.

Merge the MR
Go to Secure > Policies.
Update the policy description
Verify the approvals for the merged MR was not deleted

[2] pry(main)> ApprovalMergeRequestRule.find(1061)
  ApprovalMergeRequestRule Load (1.0ms)  SELECT "approval_merge_request_rules".* FROM "approval_merge_request_rules" WHERE "approval_merge_request_rules"."id" = 1061 LIMIT 1 
=> #<ApprovalMergeRequestRule:0x000000011c406768
 id: 1061,
 created_at: Thu, 04 Jan 2024 22:08:23.858045000 UTC +00:00,
 updated_at: Thu, 04 Jan 2024 22:08:23.858045000 UTC +00:00,
 merge_request_id: 385,
 approvals_required: 0,
 name: "Deny new MIT licensed dependencies",
 rule_type: "report_approver",
 report_type: "license_scanning",
 section: nil,
 modified_from_project_rule: false,
 orchestration_policy_idx: 0,
 vulnerabilities_allowed: 0,
 scanners: [],
 severity_levels: [],
 vulnerability_states: ["newly_detected"],
 security_orchestration_policy_configuration_id: 100,
 scan_result_policy_id: nil,
 applicable_post_merge: nil>

Verify the previous linked scan_result_policy was deleted.

[5] pry(main)> Security::ScanResultPolicyRead.find(523)
  Security::ScanResultPolicyRead Load (1.4ms)  SELECT "scan_result_policies".* FROM "scan_result_policies" WHERE "scan_result_policies"."id" = 523 LIMIT 1 
ActiveRecord::RecordNotFound: Couldn't find Security::ScanResultPolicyRead with 'id'=523

Edited Jan 08, 2024 by Marcos Rocha

Stop deleting merged MRs approval rules

What does this MR do and why?

Migrations

Queries

MR acceptance checklist

How to set up and validate locally

Merge request reports