Skip to content

Fix duplicate name validation in ProcessScanResultPolicyWorker

Sashi Kumar Kumaresan requested to merge sk/404415-fix-name-validation into master

What does this MR do and why?

Addresses #404415 (closed)

Follow-up MR to remove dead code: !117467 (merged)

Before

This MR fixes an exception in Security::ProcessScanResultPolicyWorker when the worker is executed for a same project with different security_orchestration_policy_configuration_id. The worker performs these steps in a transaction:

  1. Delete approval rules for project associated to security_orchestration_policy_configuration
  2. Create approval rules for project associated to security_orchestration_policy_configuration
  3. Syncs newly created project approval rules to mr approval rules through Security::SecurityOrchestrationPolicies::SyncOpenedMergeRequestsService

But step 3 performs the synchronisation at the whole project level which would lead to recreation of mr approval rules that already exist. This is also the reason why SyncOpenedMergeRequestsService is inside the transaction.

graph TD
    A[Security::SyncScanPoliciesWorker] -->|Project A, Config A| B(Security::ProcessScanResultPolicyWorker)
    G[Security::SyncScanPoliciesWorker] -->|Project A, Config A| H(Security::ProcessScanResultPolicyWorker)
    G[Security::SyncScanPoliciesWorker] -->|Project A, Config B| C(Security::ProcessScanResultPolicyWorker)
    B --> D[Delete approval rules for config A]
    B --> I[Deduplicated]
    H --> I[Deduplicated]
    C --> E[Delete approval rules for config B]
    D --> F[Sync project approval rules for project A]
    E --> F[Sync project approval rules for project A]  

Even though ProcessScanResultPolicyWorker has de-duplication enabled, for different configuration_id, it will not be deduplicated.

After

This MR fixes this by doing these things:

  • Update Security::SecurityOrchestrationPolicies::SyncOpenedMergeRequestsService to perform synchronisation at project and configuration level
  • Move Security::SecurityOrchestrationPolicies::SyncOpenedMergeRequestsService out of transaction
  • Move Security::SecurityOrchestrationPolicies::SyncOpenMergeRequestsHeadPipelineService to SyncOpenedMergeRequestsService as we don't have it inside the transaction, this also avoid iterating over the opened merge request twice.
graph TD
    A[Security::SyncScanPoliciesWorker] -->|Project A, Config A| B(Security::ProcessScanResultPolicyWorker)
    G[Security::SyncScanPoliciesWorker] -->|Project A, Config A| H(Security::ProcessScanResultPolicyWorker)
    G[Security::SyncScanPoliciesWorker] -->|Project A, Config B| C(Security::ProcessScanResultPolicyWorker)
    B --> D[Delete approval rules for config A]
    B --> I[Deduplicated]
    H --> I[Deduplicated]
    C --> E[Delete approval rules for config B]
    D --> F[Sync project approval rules for project A, Config A]
    E --> J[Sync project approval rules for project A, config B]

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Sashi Kumar Kumaresan

Merge request reports