Follow-up from "Remove transaction in ProcessScanResultPolicyWorker"
ProcessScanResultPolicyService calls the SoftwareLicensePolicies::CreateService
to create SoftwareLicensePolicies
.
SoftwareLicensePolicies::CreateService
rescue some exception and return an error response in case of an exception. However, ProcessScanResultPolicyService
does not check the response status.
The following context illustrates that an error in SoftwareLicensePolicies::CreateService
could prevent SoftwareLicensePolicy
from being created.
diff --git a/ee/spec/workers/security/process_scan_result_policy_worker_spec.rb b/ee/spec/workers/security/process_scan_result_policy_worker_spec.rb
--- a/ee/spec/workers/security/process_scan_result_policy_worker_spec.rb (revision f444a61922b341af125ec013f1a4599826719511)
+++ b/ee/spec/workers/security/process_scan_result_policy_worker_spec.rb (date 1685377271182)
@@ -97,6 +97,57 @@
worker.perform(configuration.project_id, configuration.id)
end
+
+ context 'when an exception occurs in SoftwareLicensePolicies::CreateService' do
+ let_it_be(:project) { configuration.project }
+
+ let(:scan_result_policy_read) do
+ create(:scan_result_policy_read, security_orchestration_policy_configuration: configuration)
+ end
+
+ let!(:software_license_with_scan_result_policy) do
+ create(:software_license_policy, project: project,
+ scan_result_policy_read: scan_result_policy_read)
+ end
+
+ let(:active_policies) do
+ {
+ scan_execution_policy: [],
+ scan_result_policy:
+ [
+ {
+ name: 'CS critical policy',
+ description: 'This policy with CS for critical policy',
+ enabled: true,
+ rules: [
+ {
+ type: 'license_finding',
+ branches: %w[master],
+ match_on_inclusion: true,
+ license_types: %w[BSD MIT],
+ license_states: %w[newly_detected detected]
+ }
+ ],
+ actions: [
+ { type: 'require_approval', approvals_required: 1, user_approvers: %w[admin] }
+ ]
+ }
+ ]
+ }
+ end
+
+ let(:error_response) { { status: :error } }
+
+ before do
+ allow_next_instance_of(SoftwareLicensePolicies::CreateService) do |instance|
+ allow(instance).to receive(:execute).and_return(error_response)
+ end
+ end
+
+ it 'does not create any software_license_policies' do
+ expect { worker.perform(project.id, configuration.id) }.to change { SoftwareLicensePolicy.where(project_id: project.id).count }.from(1).to(0)
+ end
+ end
end
shared_context 'with scan_result_policy_reads' do
Implementation Plan
-
Log the error when an ActiveRecord::RecordInvalid
exception occurs.
===================================================================
diff --git a/ee/app/services/software_license_policies/create_service.rb b/ee/app/services/software_license_policies/create_service.rb
--- a/ee/app/services/software_license_policies/create_service.rb (revision f444a61922b341af125ec013f1a4599826719511)
+++ b/ee/app/services/software_license_policies/create_service.rb (date 1685378202257)
@@ -14,6 +14,7 @@
result = is_scan_result_policy ? create_for_scan_result_policy : create_software_license_policy
success(software_license_policy: result)
rescue ActiveRecord::RecordInvalid => exception
+ log_error(exception.record.errors.full_messages)
error(exception.record.errors.full_messages, 400)
rescue ArgumentError => exception
log_error(exception.message)
-
Refactor SoftwareLicensePolicies::CreateService
. This service is used only inProcessScanResultPolicyService
andis_scan_result_policy
argument is always true. We remove the dead code.
This issue was create to address the discussion from !118173 (merged):
-
@mc_rocha started a discussion: (+2 comments) Hi @sashi_kumar, since I'm not familiar with this job, I have some questions about this change:
The reason we had to do everything within a transaction was due to race condition while concurrent execution of the worker for a same project and configuration_id.
Was it the only reason to use a transaction? What would happen if one of the steps below fails without a transaction?
🤔 configuration.delete_scan_finding_rules_for_project(project_id)
configuration.delete_software_license_policies(project)
active_scan_result_policies.each_with_index do |policy, policy_index| Security::SecurityOrchestrationPolicies::ProcessScanResultPolicyService .new(project: project, policy_configuration: configuration, policy: policy, policy_index: policy_index) .execute
From this MR description we have the following statement about this worker:
it deletes and re-creates the resources, the end state remains the same even it run multiple times.
Will this assumption remain true without a transaction?
🤔 I was trying to identify if the behavior would change without the transaction, and I found something(not related to this MR) that can potentially create an inconsistent state even inside a transaction.
I captured this behavior in a context.
context 'when an exception occurs in SoftwareLicensePolicies::CreateService' do let_it_be(:project) { configuration.project } let(:scan_result_policy_read) do create(:scan_result_policy_read, security_orchestration_policy_configuration: configuration) end let!(:software_license_without_scan_result_policy) do create(:software_license_policy, project: project) end let!(:software_license_with_scan_result_policy) do create(:software_license_policy, project: project, scan_result_policy_read: scan_result_policy_read) end let(:active_policies) do { scan_execution_policy: [], scan_result_policy: [ { name: 'CS critical policy', description: 'This policy with CS for critical policy', enabled: true, rules: [ { type: 'license_finding', branches: %w[master], match_on_inclusion: true, license_types: %w[BSD MIT], license_states: %w[newly_detected detected] } ], actions: [ { type: 'require_approval', approvals_required: 1, user_approvers: %w[admin] } ] } ] } end let(:error_response) { { status: :error } } before do allow_next_instance_of(SoftwareLicensePolicies::CreateService) do |instance| allow(instance).to receive(:execute).and_return(error_response) end end it 'deletes software_license_policies associated to the project' do worker.perform(project.id, configuration.id) software_license_policies = SoftwareLicensePolicy.where(project_id: project.id) expect(software_license_policies).to match_array([software_license_without_scan_result_policy]) end it 'does not creat new software_license_policies' do expect { worker.perform(project.id, configuration.id) }.to change { SoftwareLicensePolicy.where(project_id: project.id).count }.from(2).to(1) end end
If you copy this context inside the context
with transaction
you created, the same behavior happens with or without a transaction since we ignore the return of::SoftwareLicensePolicies::CreateService
.