Merge branch 'sk/441500-remove-ff' into 'master'

Remove security_policies_sync_preexisting_state feature flag

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/144531



Merged-by: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
Approved-by: Hordur Freyr Yngvason <hfyngvason@gitlab.com>
Approved-by: Martin Čavoj <mcavoj@gitlab.com>
Reviewed-by: Martin Čavoj <mcavoj@gitlab.com>
Co-authored-by: Sashi Kumar <skumar@gitlab.com>

ee/app/services/ee/merge_requests/after_create_service.rb

@@ -35,8 +35,7 @@ def schedule_sync_for(merge_request)
           ::Security::ScanResultPolicies::SyncAnyMergeRequestApprovalRulesWorker.perform_async(merge_request.id)
         end
 
-        if ::Feature.enabled?(:security_policies_sync_preexisting_state, project, type: :gitlab_com_derisk) &&
-            merge_request.approval_rules.scan_finding.any?
+        if merge_request.approval_rules.scan_finding.any?
           ::Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesWorker.perform_async(merge_request.id)
         end
 

ee/app/services/ee/merge_requests/refresh_service.rb

@@ -74,8 +74,6 @@ def sync_any_merge_request_approval_rules
       end
 
       def sync_preexiting_states_approval_rules
-        return if ::Feature.disabled?(:security_policies_sync_preexisting_state, project, type: :gitlab_com_derisk)
-
         merge_requests_for_source_branch.each do |merge_request|
           if merge_request.approval_rules.scan_finding.any?
             ::Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesWorker.perform_async(merge_request.id)

ee/app/services/security/scan_result_policies/update_approvals_service.rb

@@ -21,20 +21,18 @@ def execute
 
         all_scan_finding_rules = merge_request.approval_rules.scan_finding
 
-        approval_rules = if security_policies_sync_preexisting_state_enabled?
-                           all_scan_finding_rules.select { |rule| include_newly_detected?(rule) }
-                         else
-                           all_scan_finding_rules
-                         end
+        approval_rules_with_newly_detected_states = all_scan_finding_rules.select do |rule|
+          include_newly_detected?(rule)
+        end
 
-        return if approval_rules.empty?
+        return if approval_rules_with_newly_detected_states.empty?
 
         log_update_approval_rule('Evaluating MR approval rules from scan result policies',
           pipeline_ids: related_pipeline_ids,
           target_pipeline_ids: related_target_pipeline_ids
         )
 
-        violated_rules, unviolated_rules = partition_rules(approval_rules)
+        violated_rules, unviolated_rules = partition_rules(approval_rules_with_newly_detected_states)
 
         update_required_approvals(violated_rules, unviolated_rules)
         violations.add(violated_rules.pluck(:scan_result_policy_id), unviolated_rules.pluck(:scan_result_policy_id)) # rubocop:disable CodeReuse/ActiveRecord
@@ -96,11 +94,7 @@ def log_update_approval_rule(message, **attributes)
 
       def violates_approval_rule?(approval_rule)
         target_pipeline_uuids = target_pipeline_findings_uuids(approval_rule)
-
-        return true if findings_count_violated?(approval_rule, target_pipeline_uuids)
-        return true if preexisting_findings_count_violated?(approval_rule, target_pipeline_uuids)
-
-        false
+        findings_count_violated?(approval_rule, target_pipeline_uuids)
       end
 
       def missing_scans(approval_rule)
@@ -153,15 +147,6 @@ def findings_count_violated?(approval_rule, target_pipeline_uuids)
         end
       end
 
-      def preexisting_findings_count_violated?(approval_rule, target_pipeline_uuids)
-        return false if security_policies_sync_preexisting_state_enabled?
-        return false if target_pipeline_uuids.empty? || include_newly_detected?(approval_rule)
-
-        vulnerabilities_count = vulnerabilities_count_for_uuids(target_pipeline_uuids, approval_rule)
-
-        vulnerabilities_count[:exceeded_allowed_count]
-      end
-
       def related_pipeline_sources
         Enums::Ci::Pipeline.ci_and_security_orchestration_sources.values
       end
@@ -220,10 +205,6 @@ def vulnerabilities_count_for_uuids(uuids, approval_rule)
           vulnerability_age: approval_rule.scan_result_policy_read&.vulnerability_age
         ).execute
       end
-
-      def security_policies_sync_preexisting_state_enabled?
-        Feature.enabled?(:security_policies_sync_preexisting_state, merge_request.project, type: :gitlab_com_derisk)
-      end
     end
   end
 end

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

@@ -36,9 +36,6 @@ def sync_any_merge_request_approval_rules(merge_request)
       end
 
       def sync_preexisting_state_approval_rules(merge_request)
-        return if ::Feature.disabled?(:security_policies_sync_preexisting_state, merge_request.project,
-          type: :gitlab_com_derisk)
-
         return unless merge_request.approval_rules.scan_finding.any?
 
         ::Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesWorker.perform_async(merge_request.id)

ee/app/workers/security/scan_result_policies/sync_preexisting_states_approval_rules_worker.rb

@@ -15,9 +15,6 @@ def perform(merge_request_id)
         merge_request = MergeRequest.find_by_id(merge_request_id)
         return unless merge_request
 
-        return if Feature.disabled?(:security_policies_sync_preexisting_state, merge_request.project,
-          type: :gitlab_com_derisk)
-
         Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesService.new(merge_request).execute
       end
     end

ee/config/feature_flags/gitlab_com_derisk/security_policies_sync_preexisting_state.yml

----
-name: security_policies_sync_preexisting_state
-feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/425482
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/141095
-rollout_issue_url: https://gitlab.com/gitlab-com/gl-infra/production/-/issues/17361
-milestone: '16.9'
-group: group::security policies
-type: gitlab_com_derisk
-default_enabled: false

ee/spec/services/ee/merge_requests/refresh_service_spec.rb

@@ -231,18 +231,6 @@
         subject
       end
 
-      context 'when security_policies_sync_preexisting_state is disabled' do
-        before do
-          stub_feature_flags(security_policies_sync_preexisting_state: false)
-        end
-
-        it 'does not enqueue SyncPreexistingStatesApprovalRulesWorker' do
-          expect(Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesWorker).not_to receive(:perform_async)
-
-          subject
-        end
-      end
-
       context 'without scan_finding rule' do
         let!(:scan_finding_rule) { nil }
 

ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb

@@ -463,15 +463,6 @@
           let(:vulnerability_states) { [] }
 
           it_behaves_like 'sets approvals_required to 0'
-
-          context 'when security_policies_sync_preexisting_state is disabled' do
-            before do
-              stub_feature_flags(security_policies_sync_preexisting_state: false)
-            end
-
-            it_behaves_like 'sets approvals_required to 0'
-            it_behaves_like 'triggers policy bot comment', :scan_finding, false
-          end
         end
 
         context 'when vulnerability_states has new_needs_triage' do
@@ -501,15 +492,6 @@
         context 'when vulnerabilities count exceeds the allowed limit' do
           it_behaves_like 'does not update approvals_required'
           it_behaves_like 'does not trigger policy bot comment'
-
-          context 'when security_policies_sync_preexisting_state is disabled' do
-            before do
-              stub_feature_flags(security_policies_sync_preexisting_state: false)
-            end
-
-            it_behaves_like 'does not update approvals_required'
-            it_behaves_like 'triggers policy bot comment', :scan_finding, true
-          end
         end
 
         context 'when vulnerabilities count does not exceed the allowed limit' do
@@ -517,15 +499,6 @@
 
           it_behaves_like 'does not update approvals_required'
           it_behaves_like 'does not trigger policy bot comment'
-
-          context 'when security_policies_sync_preexisting_state is disabled' do
-            before do
-              stub_feature_flags(security_policies_sync_preexisting_state: false)
-            end
-
-            it_behaves_like 'sets approvals_required to 0'
-            it_behaves_like 'triggers policy bot comment', :scan_finding, false
-          end
         end
       end
 
@@ -536,15 +509,6 @@
 
         it_behaves_like 'does not update approvals_required'
         it_behaves_like 'triggers policy bot comment', :scan_finding, true
-
-        context 'when security_policies_sync_preexisting_state is disabled' do
-          before do
-            stub_feature_flags(security_policies_sync_preexisting_state: false)
-          end
-
-          it_behaves_like 'does not update approvals_required'
-          it_behaves_like 'triggers policy bot comment', :scan_finding, true
-        end
       end
     end
 

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

@@ -123,20 +123,6 @@
         )
       end
 
-      context 'when security_policies_sync_preexisting_state is disabled' do
-        before do
-          stub_feature_flags(security_policies_sync_preexisting_state: false)
-        end
-
-        it 'does not enqueue SyncPreexistingStatesApprovalRulesWorker' do
-          expect(::Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesWorker).not_to(
-            receive(:perform_async)
-          )
-
-          subject
-        end
-      end
-
       it "enqueues SyncPreexistingStatesApprovalRulesWorker with opened merge requests" do
         expect(::Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesWorker).to(
           receive(:perform_async).with(opened_merge_request.id)

ee/spec/workers/security/scan_result_policies/sync_preexisting_states_approval_rules_worker_spec.rb

@@ -19,18 +19,6 @@
       run_worker
     end
 
-    context 'when security_policies_sync_preexisting_state is disabled' do
-      before do
-        stub_feature_flags(security_policies_sync_preexisting_state: false)
-      end
-
-      it 'does not call SyncPreexistingStatesApprovalRulesService' do
-        expect(Security::ScanResultPolicies::SyncPreexistingStatesApprovalRulesService).not_to receive(:new)
-
-        run_worker
-      end
-    end
-
     context 'when merge_request does not exist' do
       let(:merge_request_id) { non_existing_record_id }