Merge branch 'philipcunningham-fix-n-plus-1-issue-with-dast-sharding' into 'master'

Avoid N+1 issue linking DAST profiles and builds

See merge request gitlab-org/gitlab!74967

ee/app/services/app_sec/dast/scan_configs/build_service.rb

@@ -4,6 +4,8 @@ module AppSec
   module Dast
     module ScanConfigs
       class BuildService < BaseContainerService
+        STAGE_NAME = 'dast'
+
         include Gitlab::Utils::StrongMemoize
 
         def execute
@@ -41,7 +43,7 @@ module AppSec
 
         def ci_configuration
           {
-            'stages' => ['dast'],
+            'stages' => [STAGE_NAME],
             'include' => [{ 'template' => 'Security/DAST-On-Demand-Scan.gitlab-ci.yml' }],
             'dast' => {
               'dast_configuration' => { 'site_profile' => dast_site_profile.name, 'scanner_profile' => dast_scanner_profile&.name }.compact

ee/lib/ee/gitlab/ci/pipeline/chain/create_cross_database_associations.rb

@@ -11,7 +11,7 @@ module EE
 
             override :perform!
             def perform!
-              create_dast_configuration_associations
+              create_dast_associations
             end
 
             override :break?
@@ -21,30 +21,43 @@ module EE
 
             private
 
-            def create_dast_configuration_associations
-              pipeline.builds.each do |build|
-                response = find_dast_profiles(build)
+            def create_dast_associations
+              # we don't use pipeline.stages.by_name because it introduces an extra sql query
+              dast_stage = pipeline.stages.find { |stage| stage.name == ::AppSec::Dast::ScanConfigs::BuildService::STAGE_NAME }
+              return unless dast_stage
 
-                error(response.errors.join(', '), config_error: true) if response.error?
-                next if response.error? || response.payload.blank?
+              # we use dast_stage.statuses to avoid extra sql queries
+              dast_stage.statuses.each do |status|
+                next unless status.is_a?(::Ci::Build)
 
-                build.dast_site_profile = response.payload[:dast_site_profile]
-                build.dast_scanner_profile = response.payload[:dast_scanner_profile]
+                associate_dast_profiles(dast_stage, status)
               end
-            rescue StandardError => e
-              ::Gitlab::ErrorTracking.track_exception(e, extra: { pipeline_id: pipeline.id })
+            end
+
+            def associate_dast_profiles(stage, build)
+              response = find_dast_profiles(build)
+
+              error(response.errors.join(', '), config_error: true) if response.error?
+              return if response.error? || response.payload.blank?
+
+              dast_site_profile = response.payload[:dast_site_profile]
+              Dast::SiteProfilesBuild.create!(ci_build: build, dast_site_profile: dast_site_profile) if dast_site_profile
+
+              dast_scanner_profile = response.payload[:dast_scanner_profile]
+              Dast::ScannerProfilesBuild.create!(ci_build: build, dast_scanner_profile: dast_scanner_profile) if dast_scanner_profile
+            rescue ActiveRecord::ActiveRecordError => e
+              ::Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, extra: { pipeline_id: pipeline.id })
 
               error('Failed to associate DAST profiles')
             end
 
             def find_dast_profiles(build)
               dast_configuration = build.options[:dast_configuration]
-
-              return ServiceResponse.success unless dast_configuration && build.stage == 'dast'
+              return ServiceResponse.success unless dast_configuration
 
               AppSec::Dast::Profiles::BuildConfigService.new(
-                project: pipeline.project,
-                current_user: pipeline.user,
+                project: build.project,
+                current_user: build.user,
                 params: {
                   dast_site_profile: dast_configuration[:site_profile],
                   dast_scanner_profile: dast_configuration[:scanner_profile]

ee/spec/lib/gitlab/ci/pipeline/chain/create_cross_database_associations_spec.rb

@@ -7,8 +7,8 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations do
   let_it_be(:user) { create(:user, developer_projects: [project]) }
   let_it_be(:outsider) { create(:user) }
 
-  let(:builds) { [] }
-  let(:pipeline) { create(:ci_pipeline, project: project, user: user, builds: builds) }
+  let!(:pipeline) { create(:ci_pipeline, project: project, user: user) }
+  let!(:stage) { create(:ci_stage_entity, project: project, pipeline: pipeline, name: :dast) }
 
   subject do
     command = Gitlab::Ci::Pipeline::Chain::Command.new(project: project, current_user: user)
@@ -34,9 +34,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations do
       let(:dast_site_profile_name) { dast_site_profile.name }
       let(:dast_scanner_profile_name) { dast_scanner_profile.name }
 
-      let(:stage) { :dast }
-      let(:dast_build) { create(:ci_build, project: project, user: user, stage: stage, options: { dast_configuration: { site_profile: dast_site_profile_name, scanner_profile: dast_scanner_profile_name } }) }
-      let(:builds) { [dast_build] }
+      let!(:dast_build) { create(:ci_build, project: project, user: user, pipeline: pipeline, stage_id: stage.id, options: { dast_configuration: { site_profile: dast_site_profile_name, scanner_profile: dast_scanner_profile_name } }) }
 
       context 'when the feature is not licensed' do
         before do
@@ -55,7 +53,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations do
         end
 
         shared_examples 'it attempts to associate the profile' do |dast_profile_name_key|
-          let(:association) { dast_build.association(profile.class.underscore.to_sym).target }
+          let(:association) { dast_build.public_send(profile.class.underscore.to_sym) }
           let(:profile_name) { public_send(dast_profile_name_key) }
 
           context 'when the profile exists' do
@@ -77,7 +75,7 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations do
           end
 
           context 'when the stage is not dast' do
-            let(:stage) { :test }
+            let!(:stage) { create(:ci_stage_entity, project: project, pipeline: pipeline, name: :test) }
 
             it_behaves_like 'it has no effect'
           end

ee/spec/services/ci/create_pipeline_service/dast_configuration_spec.rb

@@ -50,7 +50,9 @@ RSpec.describe Ci::CreatePipelineService do
       .to_runner_variables
   end
 
-  subject { described_class.new(project, user, ref: 'refs/heads/master').execute(:push).payload }
+  let(:service) { described_class.new(project, user, ref: 'refs/heads/master') }
+
+  subject { service.execute(:push).payload }
 
   before do
     stub_ci_pipeline_yaml_file(config)
@@ -118,7 +120,7 @@ RSpec.describe Ci::CreatePipelineService do
         let_it_be(:exception) { ActiveRecord::ConnectionTimeoutError }
 
         before do
-          allow(error_tracking).to receive(:track_exception).and_call_original
+          allow(error_tracking).to receive(:track_and_raise_for_dev_exception)
 
           allow_next_instance_of(AppSec::Dast::Profiles::BuildConfigService) do |instance|
             allow(instance).to receive(:execute).and_raise(exception)
@@ -128,7 +130,7 @@ RSpec.describe Ci::CreatePipelineService do
         it 'handles the error', :aggregate_failures do
           expect(subject.errors.full_messages).to include('Failed to associate DAST profiles')
 
-          expect(error_tracking).to have_received(:track_exception).with(exception, extra: { pipeline_id: subject.id })
+          expect(error_tracking).to have_received(:track_and_raise_for_dev_exception).with(exception, extra: { pipeline_id: subject.id })
         end
       end
     end
@@ -136,5 +138,49 @@ RSpec.describe Ci::CreatePipelineService do
     context 'when the stage is not dast' do
       it_behaves_like 'it does not expand the dast variables'
     end
+
+    it_behaves_like 'pipelines are created without N+1 SQL queries' do
+      let_it_be(:config1) do
+        <<~YAML
+        include:
+          - template: Security/DAST.gitlab-ci.yml
+        stages:
+          - dast
+        dast:
+          dast_configuration:
+            site_profile: #{dast_site_profile.name}
+            scanner_profile: #{dast_scanner_profile.name}
+        YAML
+      end
+
+      let_it_be(:config2) do
+        <<~YAML
+        include:
+          - template: Security/DAST.gitlab-ci.yml
+        stages:
+          - dast
+        dast:
+          dast_configuration:
+            site_profile: #{dast_site_profile.name}
+            scanner_profile: #{dast_scanner_profile.name}
+        dast2:
+          stage: dast
+          script:
+            - exit 0
+        YAML
+      end
+
+      let(:accepted_n_plus_ones) do
+        1 + # SELECT "ci_instance_variables"
+        1 + # SELECT "ci_builds".* FROM "ci_builds"
+        1 + # INSERT INTO "ci_builds_metadata"
+        1 + # SELECT "taggings".* FROM "taggings"
+        1   # SELECT "ci_pipelines"."id" FROM
+      end
+
+      def execute_service
+        service.execute(:push)
+      end
+    end
   end
 end

spec/services/ci/create_pipeline_service_spec.rb

@@ -46,6 +46,47 @@ RSpec.describe Ci::CreatePipelineService do
     end
     # rubocop:enable Metrics/ParameterLists
 
+    context 'performance' do
+      it_behaves_like 'pipelines are created without N+1 SQL queries' do
+        let(:config1) do
+          <<~YAML
+          job1:
+            stage: build
+            script: exit 0
+
+          job2:
+            stage: test
+            script: exit 0
+          YAML
+        end
+
+        let(:config2) do
+          <<~YAML
+          job1:
+            stage: build
+            script: exit 0
+
+          job2:
+            stage: test
+            script: exit 0
+
+          job3:
+            stage: deploy
+            script: exit 0
+          YAML
+        end
+
+        let(:accepted_n_plus_ones) do
+          1 + # SELECT "ci_instance_variables"
+          1 + # INSERT INTO "ci_stages"
+          1 + # SELECT "ci_builds".* FROM "ci_builds"
+          1 + # INSERT INTO "ci_builds"
+          1 + # INSERT INTO "ci_builds_metadata"
+          1   # SELECT "taggings".* FROM "taggings"
+        end
+      end
+    end
+
     context 'valid params' do
       let(:pipeline) { execute_service.payload }
 

spec/support/shared_examples/ci/create_pipeline_service_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'pipelines are created without N+1 SQL queries' do
+  before do
+    # warm up
+    stub_ci_pipeline_yaml_file(config1)
+    execute_service
+  end
+
+  it 'avoids N+1 queries', :aggregate_failures, :request_store, :use_sql_query_cache do
+    control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+      stub_ci_pipeline_yaml_file(config1)
+
+      pipeline = execute_service.payload
+
+      expect(pipeline).to be_created_successfully
+    end
+
+    expect do
+      stub_ci_pipeline_yaml_file(config2)
+
+      pipeline = execute_service.payload
+
+      expect(pipeline).to be_created_successfully
+    end.not_to exceed_all_query_limit(control).with_threshold(accepted_n_plus_ones)
+  end
+end