Merge branch 'philipcunningham-fix-ci-sharding-issue-for-dast-343382' into 'master'

Fix dast_configuration keyword cross-database modification issue

See merge request gitlab-org/gitlab!73042

app/services/ci/create_pipeline_service.rb

@@ -25,6 +25,7 @@ module Ci
                 Gitlab::Ci::Pipeline::Chain::Populate,
                 Gitlab::Ci::Pipeline::Chain::StopDryRun,
                 Gitlab::Ci::Pipeline::Chain::Create,
+                Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations,
                 Gitlab::Ci::Pipeline::Chain::Limit::Activity,
                 Gitlab::Ci::Pipeline::Chain::Limit::JobActivity,
                 Gitlab::Ci::Pipeline::Chain::CancelPendingPipelines,

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

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Ci
+      module Pipeline
+        module Chain
+          module CreateCrossDatabaseAssociations
+            extend ::Gitlab::Utils::Override
+            include ::Gitlab::Ci::Pipeline::Chain::Helpers
+
+            override :perform!
+            def perform!
+              create_dast_configuration_associations
+            end
+
+            override :break?
+            def break?
+              pipeline.errors.any?
+            end
+
+            private
+
+            def create_dast_configuration_associations
+              pipeline.builds.each do |build|
+                response = find_dast_profiles(build)
+
+                error(response.errors.join(', '), config_error: true) if response.error?
+                next if response.error? || response.payload.blank?
+
+                build.dast_site_profile = response.payload[:dast_site_profile]
+                build.dast_scanner_profile = response.payload[:dast_scanner_profile]
+              end
+            rescue StandardError => e
+              ::Gitlab::ErrorTracking.track_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'
+
+              AppSec::Dast::Profiles::BuildConfigService.new(
+                project: pipeline.project,
+                current_user: pipeline.user,
+                params: {
+                  dast_site_profile: dast_configuration[:site_profile],
+                  dast_scanner_profile: dast_configuration[:scanner_profile]
+                }
+              ).execute
+            end
+          end
+        end
+      end
+    end
+  end
+end

ee/lib/ee/gitlab/ci/pipeline/seed/build.rb

-# frozen_string_literal: true
-
-module EE
-  module Gitlab
-    module Ci
-      module Pipeline
-        module Seed
-          module Build
-            extend ::Gitlab::Utils::Override
-
-            override :attributes
-            def initialize(context, attributes, stages_for_needs_lookup)
-              super
-
-              @dast_configuration = attributes.dig(:options, :dast_configuration)
-            end
-
-            override :attributes
-            def attributes
-              super.deep_merge(dast_configuration.payload)
-            end
-
-            override :errors
-            def errors
-              super.concat(dast_configuration.errors)
-            end
-
-            private
-
-            # rubocop:disable Gitlab/ModuleWithInstanceVariables
-            def dast_configuration
-              return ServiceResponse.success unless @dast_configuration && @seed_attributes[:stage] == 'dast'
-
-              strong_memoize(:dast_attributes) do
-                AppSec::Dast::Profiles::BuildConfigService.new(
-                  project: @pipeline.project,
-                  current_user: @pipeline.user,
-                  params: {
-                    dast_site_profile: @dast_configuration[:site_profile],
-                    dast_scanner_profile: @dast_configuration[:scanner_profile]
-                  }
-                ).execute
-              end
-            end
-            # rubocop:enable Gitlab/ModuleWithInstanceVariables
-          end
-        end
-      end
-    end
-  end
-end

ee/spec/lib/ee/gitlab/ci/pipeline/seed/build_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe Gitlab::Ci::Pipeline::Seed::Build do
-  let_it_be(:project) { create(:project, :repository) }
-  let_it_be(:user) { create(:user, developer_projects: [project]) }
-  let_it_be(:outsider) { create(:user) }
-
-  let(:pipeline) { build(:ci_empty_pipeline, project: project, user: user) }
-  let(:seed_context) { double(pipeline: pipeline, root_variables: []) }
-  let(:stage) { 'dast' }
-  let(:attributes) { { name: 'rspec', ref: 'master', scheduling_type: :stage, stage: stage, options: { dast_configuration: dast_configuration } } }
-
-  let(:seed_build) { described_class.new(seed_context, attributes, []) }
-
-  describe '#attributes' do
-    subject { seed_build.attributes }
-
-    context 'dast' do
-      let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
-      let_it_be(:dast_scanner_profile) { create(:dast_scanner_profile, project: project) }
-
-      let(:dast_site_profile_name) { dast_site_profile.name }
-      let(:dast_scanner_profile_name) { dast_scanner_profile.name }
-
-      let(:dast_configuration) { { site_profile: dast_site_profile_name, scanner_profile: dast_scanner_profile_name } }
-
-      shared_examples 'it does not change build attributes' do
-        it 'does not add dast_site_profile or dast_scanner_profile' do
-          expect(subject.keys).not_to include(:dast_site_profile, :dast_scanner_profile)
-        end
-      end
-
-      shared_examples 'an insufficient permissions error' do
-        it 'communicates failure' do
-          expect(seed_build.errors).to include('Insufficient permissions for dast_configuration keyword')
-        end
-      end
-
-      context 'when the feature is not licensed' do
-        it_behaves_like 'it does not change build attributes'
-
-        it 'communicates failure' do
-          expect(seed_build.errors).to contain_exactly('Insufficient permissions for dast_configuration keyword')
-        end
-      end
-
-      context 'when the feature is licensed' do
-        before do
-          stub_licensed_features(security_on_demand_scans: true)
-        end
-
-        context 'when the user cannot create dast scans' do
-          let_it_be(:user) { outsider }
-
-          it_behaves_like 'it does not change build attributes'
-          it_behaves_like 'an insufficient permissions error'
-        end
-
-        context 'dast configuration' do
-          shared_examples 'it looks up dast profiles in the database' do |dast_profile_name_key|
-            let(:profile_name) { public_send(dast_profile_name_key) }
-
-            context 'when the profile exists' do
-              it 'adds the profile to the build attributes' do
-                expect(subject).to include(profile.class.underscore.to_sym => profile)
-              end
-            end
-
-            shared_examples 'it has no effect' do
-              it 'does not add the profile to the build attributes' do
-                expect(subject).not_to include(profile.class.underscore.to_sym => profile)
-              end
-            end
-
-            context 'when the profile is not provided' do
-              let(dast_profile_name_key) { nil }
-
-              it_behaves_like 'it has no effect'
-            end
-
-            context 'when the stage is not dast' do
-              let(:stage) { 'test' }
-
-              it_behaves_like 'it has no effect'
-            end
-
-            context 'when the profile does not exist' do
-              let(dast_profile_name_key) { SecureRandom.hex }
-
-              it 'communicates failure' do
-                expect(seed_build.errors).to contain_exactly("DAST profile not found: #{profile_name}")
-              end
-            end
-
-            context 'when the profile cannot be read' do
-              let_it_be(:user) { outsider }
-
-              before do
-                allow_next_instance_of(AppSec::Dast::Profiles::BuildConfigService) do |service|
-                  allow(service).to receive(:can?).and_call_original
-                  allow(service).to receive(:can?).with(user, :create_on_demand_dast_scan, project).and_return(true)
-                end
-              end
-
-              it 'communicates failure' do
-                expect(seed_build.errors).to include("DAST profile not found: #{profile_name}")
-              end
-            end
-          end
-
-          context 'dast_site_profile' do
-            let(:profile) { dast_site_profile }
-
-            it_behaves_like 'it looks up dast profiles in the database', :dast_site_profile_name
-          end
-
-          context 'dast_scanner_profile' do
-            let(:profile) { dast_scanner_profile }
-
-            it_behaves_like 'it looks up dast profiles in the database', :dast_scanner_profile_name
-          end
-        end
-      end
-    end
-  end
-end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations do
+  let_it_be(:project) { create(:project) }
+  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) }
+
+  subject do
+    command = Gitlab::Ci::Pipeline::Chain::Command.new(project: project, current_user: user)
+
+    described_class.new(pipeline, command)
+  end
+
+  describe '#perform!' do
+    shared_examples 'it failed' do
+      it 'breaks the chain' do
+        expect(subject.break?).to be(true)
+      end
+
+      it 'attaches errors to the pipeline' do
+        expect(pipeline.errors.full_messages).to contain_exactly(*errors)
+      end
+    end
+
+    context 'dast' do
+      let_it_be(:dast_site_profile) { create(:dast_site_profile, project: project) }
+      let_it_be(:dast_scanner_profile) { create(:dast_scanner_profile, project: project) }
+
+      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] }
+
+      context 'when the feature is not licensed' do
+        before do
+          subject.perform!
+        end
+
+        it_behaves_like 'it failed' do
+          let(:errors) { ['Insufficient permissions for dast_configuration keyword'] }
+        end
+      end
+
+      context 'when the feature is licensed' do
+        before do
+          stub_licensed_features(security_on_demand_scans: true)
+          subject.perform!
+        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(:profile_name) { public_send(dast_profile_name_key) }
+
+          context 'when the profile exists' do
+            it 'assigns the association' do
+              expect(association).to eq(profile)
+            end
+          end
+
+          shared_examples 'it has no effect' do
+            it 'does not assign the association' do
+              expect(association).to be_nil
+            end
+          end
+
+          context 'when the profile is not provided' do
+            let(dast_profile_name_key) { nil }
+
+            it_behaves_like 'it has no effect'
+          end
+
+          context 'when the stage is not dast' do
+            let(:stage) { :test }
+
+            it_behaves_like 'it has no effect'
+          end
+
+          context 'when the profile does not exist' do
+            let(dast_profile_name_key) { SecureRandom.hex }
+
+            it_behaves_like 'it failed' do
+              let(:errors) { "DAST profile not found: #{profile_name}" }
+            end
+          end
+        end
+
+        context 'dast_site_profile' do
+          let(:profile) { dast_site_profile }
+
+          it_behaves_like 'it attempts to associate the profile', :dast_site_profile_name
+        end
+
+        context 'dast_scanner_profile' do
+          let(:profile) { dast_scanner_profile }
+
+          it_behaves_like 'it attempts to associate the profile', :dast_scanner_profile_name
+        end
+
+        context 'when the user cannot create dast scans' do
+          let_it_be(:user) { outsider }
+
+          it_behaves_like 'it failed' do
+            let(:errors) { ['Insufficient permissions for dast_configuration keyword'] }
+          end
+        end
+      end
+    end
+  end
+end

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

@@ -112,6 +112,25 @@ RSpec.describe Ci::CreatePipelineService do
 
         it_behaves_like 'a missing profile'
       end
+
+      context 'when there is an unexpected system error' do
+        let_it_be(:error_tracking) { Gitlab::ErrorTracking }
+        let_it_be(:exception) { ActiveRecord::ConnectionTimeoutError }
+
+        before do
+          allow(error_tracking).to receive(:track_exception).and_call_original
+
+          allow_next_instance_of(AppSec::Dast::Profiles::BuildConfigService) do |instance|
+            allow(instance).to receive(:execute).and_raise(exception)
+          end
+        end
+
+        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 })
+        end
+      end
     end
 
     context 'when the stage is not dast' do

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Pipeline
+      module Chain
+        class CreateCrossDatabaseAssociations < Chain::Base
+          def perform!
+            # to be overridden in EE
+          end
+
+          def break?
+            false # to be overridden in EE
+          end
+        end
+      end
+    end
+  end
+end
+
+Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations.prepend_mod_with('Gitlab::Ci::Pipeline::Chain::CreateCrossDatabaseAssociations')

lib/gitlab/ci/pipeline/seed/build.rb

@@ -244,5 +244,3 @@ module Gitlab
     end
   end
 end
-
-Gitlab::Ci::Pipeline::Seed::Build.prepend_mod_with('Gitlab::Ci::Pipeline::Seed::Build')

spec/support/database/cross-database-modification-allowlist.yml

@@ -20,7 +20,6 @@
 - "./ee/spec/services/app_sec/dast/profiles/update_service_spec.rb"
 - "./ee/spec/services/app_sec/dast/scans/create_service_spec.rb"
 - "./ee/spec/services/app_sec/dast/scans/run_service_spec.rb"
-- "./ee/spec/services/ci/create_pipeline_service/dast_configuration_spec.rb"
 - "./ee/spec/services/ci/destroy_pipeline_service_spec.rb"
 - "./ee/spec/services/ci/retry_build_service_spec.rb"
 - "./ee/spec/services/ci/subscribe_bridge_service_spec.rb"