From d2d0df9b082605e0ac0d6f29e964ac6cb0d4c4a9 Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Mon, 23 Sep 2019 15:29:37 +1000 Subject: [PATCH 01/10] Identify secure jobs in a pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Secure jobs can be identified when an artifact report has been configured with the appropriate secure job name. This commit deals with the possibility that the ‘ci_build_metadata_config’ feature may be enabled or disabled. --- app/models/ci/build.rb | 6 + ee/app/finders/security/jobs_finder.rb | 69 ++++++++ ee/spec/finders/security/jobs_finder_spec.rb | 172 +++++++++++++++++++ spec/factories/ci/builds.rb | 32 ++++ 4 files changed, 279 insertions(+) create mode 100644 ee/app/finders/security/jobs_finder.rb create mode 100644 ee/spec/finders/security/jobs_finder_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 5cca18024c10bd..c91bf4ed45bdcf 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -128,6 +128,12 @@ def persisted_environment scope :with_stale_live_trace, -> { with_live_trace.finished_before(12.hours.ago) } scope :finished_before, -> (date) { finished.where('finished_at < ?', date) } + scope :with_secure_report_legacy, -> (job_type) { where('options like :job_type', job_type: "%#{job_type}%") } + + scope :with_secure_reports, -> (job_types) do + joins(:metadata).where("ci_builds_metadata.config_options -> 'artifacts' -> 'reports' ?| array[:job_types]", job_types: job_types) + end + scope :matches_tag_ids, -> (tag_ids) do matcher = ::ActsAsTaggableOn::Tagging .where(taggable_type: CommitStatus.name) diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb new file mode 100644 index 00000000000000..8f3bdd33e45721 --- /dev/null +++ b/ee/app/finders/security/jobs_finder.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +# Security::JobsFinder +# +# Used to find jobs (builds) that are for Secure products, SAST, DAST, Dependency Scanning and Container Scanning. +# +# Arguments: +# pipeline: only jobs for the specified pipeline will be found +# params: +# all: boolean, include jobs for all secure job types +# sast: boolean, include jobs for SAST +# dast: boolean, include jobs for DAST +# container_scanning: boolean, include jobs for Container Scanning +# dependency_scanning: boolean, include jobs for Dependency Scanning + +module Security + class JobsFinder + attr_reader :pipeline + + def initialize(pipeline, params = { all: true }) + @pipeline = pipeline + @params = params + end + + def execute + job_types = all_secure_jobs + job_types = by_specific_job_type(job_types, :sast) + job_types = by_specific_job_type(job_types, :dast) + job_types = by_specific_job_type(job_types, :dependency_scanning) + job_types = by_specific_job_type(job_types, :container_scanning) + + return [] if job_types.empty? + + Feature.enabled?(:ci_build_metadata_config) ? find_jobs(job_types) : find_jobs_legacy(job_types) + end + + def all_secure_jobs + if @params[:all] + return [:sast, :dast, :dependency_scanning, :container_scanning] + end + + [] + end + + def by_specific_job_type(job_types, job_type) + if @params[job_type] + return job_types + [job_type] + end + + job_types + end + + def find_jobs(job_types) + @pipeline.builds.with_secure_reports(job_types) + end + + def find_jobs_legacy(job_types) + query = @pipeline.builds.with_secure_report_legacy(job_types.first) + + jobs = job_types.drop(1).reduce(query) do |qry, job_type| + qry.or(@pipeline.builds.with_secure_report_legacy(job_type)) + end + + jobs.select do |job| + job_types.find { |job_type| job.options.dig(:artifacts, :reports, job_type) } + end + end + end +end diff --git a/ee/spec/finders/security/jobs_finder_spec.rb b/ee/spec/finders/security/jobs_finder_spec.rb new file mode 100644 index 00000000000000..18f51f05ee0d5f --- /dev/null +++ b/ee/spec/finders/security/jobs_finder_spec.rb @@ -0,0 +1,172 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Security::JobsFinder do + let(:pipeline) { create(:ci_pipeline) } + let(:finder) { described_class.new(pipeline) } + + describe '#execute' do + subject { finder.execute } + + describe 'legacy options stored' do + before do + stub_feature_flags(ci_build_metadata_config: false) + end + + context 'with no jobs' do + it { is_expected.to be_empty } + end + + context 'with non secure jobs' do + let!(:build) { create(:ci_build, pipeline: pipeline) } + + it { is_expected.to be_empty } + end + + context 'with jobs having report artifacts' do + let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) } + + it { is_expected.to be_empty } + end + + context 'with jobs having non secure report artifacts' do + let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) } + + it { is_expected.to be_empty } + end + + context 'with jobs having almost secure like report artifacts' do + let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'sast.file' } } }) } + + it { is_expected.to be_empty } + end + + context 'with dast jobs' do + let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with sast jobs' do + let!(:build) { create(:ci_build, :sast, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with container scanning jobs' do + let!(:build) { create(:ci_build, :container_scanning, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with dependency scanning jobs' do + let!(:build) { create(:ci_build, :dependency_scanning, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with many secure pipelines' do + let!(:another_pipeline) { create(:ci_pipeline) } + let!(:another_build) { create(:ci_build, :dast, pipeline: another_pipeline) } + + let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } + + it 'returns jobs associated with provided pipeline' do + is_expected.to eq([build]) + end + end + + context 'with specific secure job types' do + let!(:build_a) { create(:ci_build, :sast, pipeline: pipeline) } + let!(:build_b) { create(:ci_build, :container_scanning, pipeline: pipeline) } + let!(:build_c) { create(:ci_build, :dast, pipeline: pipeline) } + + let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) } + + it 'returns only those requested' do + is_expected.to include(build_a) + is_expected.to include(build_b) + is_expected.not_to include(build_c) + end + end + end + + describe 'config options stored' do + before do + stub_feature_flags(ci_build_metadata_config: true) + end + + context 'with no jobs' do + it { is_expected.to be_empty } + end + + context 'with non secure jobs' do + let!(:build) { create(:ci_build, pipeline: pipeline) } + + it { is_expected.to be_empty } + end + + context 'with jobs having report artifacts' do + let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) } + + it { is_expected.to be_empty } + end + + context 'with jobs having non secure report artifacts' do + let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) } + + it { is_expected.to be_empty } + end + + context 'with dast jobs' do + let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with sast jobs' do + let!(:build) { create(:ci_build, :sast, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with container scanning jobs' do + let!(:build) { create(:ci_build, :container_scanning, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with dependency scanning jobs' do + let!(:build) { create(:ci_build, :dependency_scanning, pipeline: pipeline) } + + it { is_expected.to eq([build]) } + end + + context 'with many secure pipelines' do + let!(:another_pipeline) { create(:ci_pipeline) } + let!(:another_build) { create(:ci_build, :dast, pipeline: another_pipeline) } + + let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } + + it 'returns jobs associated with provided pipeline' do + is_expected.to eq([build]) + end + end + + context 'with specific secure job types' do + let!(:build_a) { create(:ci_build, :sast, pipeline: pipeline) } + let!(:build_b) { create(:ci_build, :container_scanning, pipeline: pipeline) } + let!(:build_c) { create(:ci_build, :dast, pipeline: pipeline) } + + let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) } + + it 'returns only those requested' do + is_expected.to include(build_a) + is_expected.to include(build_b) + is_expected.not_to include(build_c) + end + end + end + end +end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 6725cde08f285b..c0f7948f96392a 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -330,6 +330,38 @@ options { {} } end + trait :dast do + options do + { + artifacts: { reports: { dast: 'gl-dast-report.json' } } + } + end + end + + trait :sast do + options do + { + artifacts: { reports: { sast: 'gl-sast-report.json' } } + } + end + end + + trait :dependency_scanning do + options do + { + artifacts: { reports: { dependency_scanning: 'gl-dependency-scanning-report.json' } } + } + end + end + + trait :container_scanning do + options do + { + artifacts: { reports: { container_scanning: 'gl-container-scanning-report.json' } } + } + end + end + trait :non_playable do status { 'created' } self.when { 'manual' } -- GitLab From b904cebf9b2299d8e80e72fdca92381c55b34804 Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Wed, 25 Sep 2019 15:34:05 +1000 Subject: [PATCH 02/10] Add index to build options for fast searching --- app/models/ci/build.rb | 2 +- ...190925054412_add_index_to_build_options.rb | 25 +++++++++++++++++++ db/schema.rb | 1 + 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20190925054412_add_index_to_build_options.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index c91bf4ed45bdcf..6593a672fb7b0b 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -128,7 +128,7 @@ def persisted_environment scope :with_stale_live_trace, -> { with_live_trace.finished_before(12.hours.ago) } scope :finished_before, -> (date) { finished.where('finished_at < ?', date) } - scope :with_secure_report_legacy, -> (job_type) { where('options like :job_type', job_type: "%#{job_type}%") } + scope :with_secure_report_legacy, -> (job_type) { where('options like :job_type', job_type: "%:artifacts:%:reports:%:#{job_type}:%") } scope :with_secure_reports, -> (job_types) do joins(:metadata).where("ci_builds_metadata.config_options -> 'artifacts' -> 'reports' ?| array[:job_types]", job_types: job_types) diff --git a/db/migrate/20190925054412_add_index_to_build_options.rb b/db/migrate/20190925054412_add_index_to_build_options.rb new file mode 100644 index 00000000000000..9d253a811cce76 --- /dev/null +++ b/db/migrate/20190925054412_add_index_to_build_options.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class AddIndexToBuildOptions < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'index_build_on_options' + + disable_ddl_transaction! + + def up + add_concurrent_index :ci_builds, :options, + name: INDEX_NAME, + using: :gist, + opclass: :gist_trgm_ops, + where: "options like '%:artifacts:%:reports:%:sast:%'" \ + " or options like '%:artifacts:%:reports:%:dast:%'" \ + " or options like '%:artifacts:%:reports:%:dependency_scanning:%'" \ + " or options like '%:artifacts:%:reports:%:container_scanning:%'" + end + + def down + remove_concurrent_index_by_name :ci_builds, INDEX_NAME + end +end diff --git a/db/schema.rb b/db/schema.rb index 352023ac40d601..dbcb22d50aacd2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -644,6 +644,7 @@ t.index ["commit_id", "type", "name", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_name_and_ref" t.index ["commit_id", "type", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_ref" t.index ["name"], name: "index_ci_builds_on_name_for_security_products_values", where: "((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('sast'::character varying)::text]))" + t.index ["options"], name: "index_build_on_options", opclass: :gist_trgm_ops, where: "((options ~~ '%:artifacts:%:reports:%:sast:%'::text) OR (options ~~ '%:artifacts:%:reports:%:dast:%'::text) OR (options ~~ '%:artifacts:%:reports:%:dependency_scanning:%'::text) OR (options ~~ '%:artifacts:%:reports:%:container_scanning:%'::text))", using: :gist t.index ["project_id", "id"], name: "index_ci_builds_on_project_id_and_id" t.index ["project_id", "status"], name: "index_ci_builds_project_id_and_status_for_live_jobs_partial2", where: "(((type)::text = 'Ci::Build'::text) AND ((status)::text = ANY (ARRAY[('running'::character varying)::text, ('pending'::character varying)::text, ('created'::character varying)::text])))" t.index ["project_id"], name: "index_ci_builds_on_project_id_for_successfull_pages_deploy", where: "(((type)::text = 'GenericCommitStatus'::text) AND ((stage)::text = 'deploy'::text) AND ((name)::text = 'pages:deploy'::text) AND ((status)::text = 'success'::text))" -- GitLab From 2d8ba98d0c6d34eba28dba827370d9c529478e83 Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Fri, 27 Sep 2019 12:48:26 +1000 Subject: [PATCH 03/10] Implement suggestions based on reviewer feedback --- ee/app/finders/security/jobs_finder.rb | 43 +++++++------ ee/spec/finders/security/jobs_finder_spec.rb | 64 ++++++++++++-------- 2 files changed, 63 insertions(+), 44 deletions(-) diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb index 8f3bdd33e45721..f3d03f1521dde9 100644 --- a/ee/app/finders/security/jobs_finder.rb +++ b/ee/app/finders/security/jobs_finder.rb @@ -15,39 +15,39 @@ module Security class JobsFinder + include Gitlab::Utils::StrongMemoize + attr_reader :pipeline + JOB_TYPES = [:sast, :dast, :dependency_scanning, :container_scanning].freeze + def initialize(pipeline, params = { all: true }) @pipeline = pipeline @params = params end def execute - job_types = all_secure_jobs - job_types = by_specific_job_type(job_types, :sast) - job_types = by_specific_job_type(job_types, :dast) - job_types = by_specific_job_type(job_types, :dependency_scanning) - job_types = by_specific_job_type(job_types, :container_scanning) - - return [] if job_types.empty? - - Feature.enabled?(:ci_build_metadata_config) ? find_jobs(job_types) : find_jobs_legacy(job_types) - end + return [] if job_types_for_processing.empty? - def all_secure_jobs - if @params[:all] - return [:sast, :dast, :dependency_scanning, :container_scanning] + if Feature.enabled?(:ci_build_metadata_config) + find_jobs(job_types_for_processing) + else + find_jobs_legacy(job_types_for_processing) end - - [] end - def by_specific_job_type(job_types, job_type) - if @params[job_type] - return job_types + [job_type] - end + private - job_types + def job_types_for_processing + strong_memoize(:job_types_for_processing) do + if @params[:all] + JOB_TYPES + else + JOB_TYPES.each_with_object([]) do |job_type, job_types| + job_types << job_type if @params[job_type] + end + end + end end def find_jobs(job_types) @@ -55,12 +55,15 @@ def find_jobs(job_types) end def find_jobs_legacy(job_types) + # first job type is added as a WHERE statement query = @pipeline.builds.with_secure_report_legacy(job_types.first) + # following job types are added as OR statements jobs = job_types.drop(1).reduce(query) do |qry, job_type| qry.or(@pipeline.builds.with_secure_report_legacy(job_type)) end + # the query doesn't guarantee accuracy, so we verify it here jobs.select do |job| job_types.find { |job_type| job.options.dig(:artifacts, :reports, job_type) } end diff --git a/ee/spec/finders/security/jobs_finder_spec.rb b/ee/spec/finders/security/jobs_finder_spec.rb index 18f51f05ee0d5f..83c0477b0428cd 100644 --- a/ee/spec/finders/security/jobs_finder_spec.rb +++ b/ee/spec/finders/security/jobs_finder_spec.rb @@ -19,25 +19,33 @@ end context 'with non secure jobs' do - let!(:build) { create(:ci_build, pipeline: pipeline) } + before do + create(:ci_build, pipeline: pipeline) + end it { is_expected.to be_empty } end context 'with jobs having report artifacts' do - let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) } + before do + create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) + end it { is_expected.to be_empty } end context 'with jobs having non secure report artifacts' do - let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) } + before do + create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) + end it { is_expected.to be_empty } end - context 'with jobs having almost secure like report artifacts' do - let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'sast.file' } } }) } + context 'with jobs having report artifacts that are similar to secure artifacts' do + before do + create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'sast.file' } } }) + end it { is_expected.to be_empty } end @@ -67,8 +75,9 @@ end context 'with many secure pipelines' do - let!(:another_pipeline) { create(:ci_pipeline) } - let!(:another_build) { create(:ci_build, :dast, pipeline: another_pipeline) } + before do + create(:ci_build, :dast, pipeline: create(:ci_pipeline)) + end let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } @@ -78,16 +87,16 @@ end context 'with specific secure job types' do - let!(:build_a) { create(:ci_build, :sast, pipeline: pipeline) } - let!(:build_b) { create(:ci_build, :container_scanning, pipeline: pipeline) } - let!(:build_c) { create(:ci_build, :dast, pipeline: pipeline) } + let!(:sast_build) { create(:ci_build, :sast, pipeline: pipeline) } + let!(:container_scanning_build) { create(:ci_build, :container_scanning, pipeline: pipeline) } + let!(:dast_build) { create(:ci_build, :dast, pipeline: pipeline) } let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) } it 'returns only those requested' do - is_expected.to include(build_a) - is_expected.to include(build_b) - is_expected.not_to include(build_c) + is_expected.to include(sast_build) + is_expected.to include(container_scanning_build) + is_expected.not_to include(dast_build) end end end @@ -102,19 +111,25 @@ end context 'with non secure jobs' do - let!(:build) { create(:ci_build, pipeline: pipeline) } + before do + create(:ci_build, pipeline: pipeline) + end it { is_expected.to be_empty } end context 'with jobs having report artifacts' do - let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) } + before do + create(:ci_build, pipeline: pipeline, options: { artifacts: { file: 'test.file' } }) + end it { is_expected.to be_empty } end context 'with jobs having non secure report artifacts' do - let!(:build) { create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) } + before do + create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'test.file' } } }) + end it { is_expected.to be_empty } end @@ -144,8 +159,9 @@ end context 'with many secure pipelines' do - let!(:another_pipeline) { create(:ci_pipeline) } - let!(:another_build) { create(:ci_build, :dast, pipeline: another_pipeline) } + before do + create(:ci_build, :dast, pipeline: create(:ci_pipeline)) + end let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } @@ -155,16 +171,16 @@ end context 'with specific secure job types' do - let!(:build_a) { create(:ci_build, :sast, pipeline: pipeline) } - let!(:build_b) { create(:ci_build, :container_scanning, pipeline: pipeline) } - let!(:build_c) { create(:ci_build, :dast, pipeline: pipeline) } + let!(:sast_build) { create(:ci_build, :sast, pipeline: pipeline) } + let!(:container_scanning_build) { create(:ci_build, :container_scanning, pipeline: pipeline) } + let!(:dast_build) { create(:ci_build, :dast, pipeline: pipeline) } let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) } it 'returns only those requested' do - is_expected.to include(build_a) - is_expected.to include(build_b) - is_expected.not_to include(build_c) + is_expected.to include(sast_build) + is_expected.to include(container_scanning_build) + is_expected.not_to include(dast_build) end end end -- GitLab From 9fac26f683fc75538d94c577ca9931529967f2cf Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Mon, 30 Sep 2019 11:06:29 +1000 Subject: [PATCH 04/10] Renamed scopes to better describe what they do --- app/models/ci/build.rb | 4 ++-- ee/app/finders/security/jobs_finder.rb | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 6593a672fb7b0b..fba14f0100cd15 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -128,9 +128,9 @@ def persisted_environment scope :with_stale_live_trace, -> { with_live_trace.finished_before(12.hours.ago) } scope :finished_before, -> (date) { finished.where('finished_at < ?', date) } - scope :with_secure_report_legacy, -> (job_type) { where('options like :job_type', job_type: "%:artifacts:%:reports:%:#{job_type}:%") } + scope :with_secure_reports_from_options, -> (job_type) { where('options like :job_type', job_type: "%:artifacts:%:reports:%:#{job_type}:%") } - scope :with_secure_reports, -> (job_types) do + scope :with_secure_reports_from_config_options, -> (job_types) do joins(:metadata).where("ci_builds_metadata.config_options -> 'artifacts' -> 'reports' ?| array[:job_types]", job_types: job_types) end diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb index f3d03f1521dde9..d4a8b9ec48631d 100644 --- a/ee/app/finders/security/jobs_finder.rb +++ b/ee/app/finders/security/jobs_finder.rb @@ -51,16 +51,16 @@ def job_types_for_processing end def find_jobs(job_types) - @pipeline.builds.with_secure_reports(job_types) + @pipeline.builds.with_secure_reports_from_config_options(job_types) end def find_jobs_legacy(job_types) # first job type is added as a WHERE statement - query = @pipeline.builds.with_secure_report_legacy(job_types.first) + query = @pipeline.builds.with_secure_reports_from_options(job_types.first) # following job types are added as OR statements jobs = job_types.drop(1).reduce(query) do |qry, job_type| - qry.or(@pipeline.builds.with_secure_report_legacy(job_type)) + qry.or(@pipeline.builds.with_secure_reports_from_options(job_type)) end # the query doesn't guarantee accuracy, so we verify it here -- GitLab From 0a3672788b9fb4edf88829442de782eeeb67203d Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Mon, 30 Sep 2019 11:12:50 +1000 Subject: [PATCH 05/10] Removed prematurely optimized memoization of small array --- ee/app/finders/security/jobs_finder.rb | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb index d4a8b9ec48631d..4855f91c625894 100644 --- a/ee/app/finders/security/jobs_finder.rb +++ b/ee/app/finders/security/jobs_finder.rb @@ -15,8 +15,6 @@ module Security class JobsFinder - include Gitlab::Utils::StrongMemoize - attr_reader :pipeline JOB_TYPES = [:sast, :dast, :dependency_scanning, :container_scanning].freeze @@ -39,15 +37,9 @@ def execute private def job_types_for_processing - strong_memoize(:job_types_for_processing) do - if @params[:all] - JOB_TYPES - else - JOB_TYPES.each_with_object([]) do |job_type, job_types| - job_types << job_type if @params[job_type] - end - end - end + return JOB_TYPES if @params[:all] + + JOB_TYPES.select { |job_type| @params[job_type] } end def find_jobs(job_types) -- GitLab From 3fa8fa9e455605edf1e720b7f8b706eaf8695bde Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Tue, 1 Oct 2019 14:20:35 +1000 Subject: [PATCH 06/10] Remove index as it will no longer be used --- ...190925054412_add_index_to_build_options.rb | 25 ------------------- db/schema.rb | 1 - 2 files changed, 26 deletions(-) delete mode 100644 db/migrate/20190925054412_add_index_to_build_options.rb diff --git a/db/migrate/20190925054412_add_index_to_build_options.rb b/db/migrate/20190925054412_add_index_to_build_options.rb deleted file mode 100644 index 9d253a811cce76..00000000000000 --- a/db/migrate/20190925054412_add_index_to_build_options.rb +++ /dev/null @@ -1,25 +0,0 @@ -# frozen_string_literal: true - -class AddIndexToBuildOptions < ActiveRecord::Migration[5.2] - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - INDEX_NAME = 'index_build_on_options' - - disable_ddl_transaction! - - def up - add_concurrent_index :ci_builds, :options, - name: INDEX_NAME, - using: :gist, - opclass: :gist_trgm_ops, - where: "options like '%:artifacts:%:reports:%:sast:%'" \ - " or options like '%:artifacts:%:reports:%:dast:%'" \ - " or options like '%:artifacts:%:reports:%:dependency_scanning:%'" \ - " or options like '%:artifacts:%:reports:%:container_scanning:%'" - end - - def down - remove_concurrent_index_by_name :ci_builds, INDEX_NAME - end -end diff --git a/db/schema.rb b/db/schema.rb index dbcb22d50aacd2..352023ac40d601 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -644,7 +644,6 @@ t.index ["commit_id", "type", "name", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_name_and_ref" t.index ["commit_id", "type", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_ref" t.index ["name"], name: "index_ci_builds_on_name_for_security_products_values", where: "((name)::text = ANY (ARRAY[('container_scanning'::character varying)::text, ('dast'::character varying)::text, ('dependency_scanning'::character varying)::text, ('license_management'::character varying)::text, ('sast'::character varying)::text]))" - t.index ["options"], name: "index_build_on_options", opclass: :gist_trgm_ops, where: "((options ~~ '%:artifacts:%:reports:%:sast:%'::text) OR (options ~~ '%:artifacts:%:reports:%:dast:%'::text) OR (options ~~ '%:artifacts:%:reports:%:dependency_scanning:%'::text) OR (options ~~ '%:artifacts:%:reports:%:container_scanning:%'::text))", using: :gist t.index ["project_id", "id"], name: "index_ci_builds_on_project_id_and_id" t.index ["project_id", "status"], name: "index_ci_builds_project_id_and_status_for_live_jobs_partial2", where: "(((type)::text = 'Ci::Build'::text) AND ((status)::text = ANY (ARRAY[('running'::character varying)::text, ('pending'::character varying)::text, ('created'::character varying)::text])))" t.index ["project_id"], name: "index_ci_builds_on_project_id_for_successfull_pages_deploy", where: "(((type)::text = 'GenericCommitStatus'::text) AND ((stage)::text = 'deploy'::text) AND ((name)::text = 'pages:deploy'::text) AND ((status)::text = 'success'::text))" -- GitLab From 84126b0729c806ad8f92dfdc0c0e34d97fd9506b Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Tue, 8 Oct 2019 16:18:10 +1100 Subject: [PATCH 07/10] Added to description precedence of options --- ee/app/finders/security/jobs_finder.rb | 2 +- ee/spec/finders/security/jobs_finder_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb index 4855f91c625894..73effab9b8a0ef 100644 --- a/ee/app/finders/security/jobs_finder.rb +++ b/ee/app/finders/security/jobs_finder.rb @@ -7,7 +7,7 @@ # Arguments: # pipeline: only jobs for the specified pipeline will be found # params: -# all: boolean, include jobs for all secure job types +# all: boolean, include jobs for all secure job types. Takes precedence over other options. # sast: boolean, include jobs for SAST # dast: boolean, include jobs for DAST # container_scanning: boolean, include jobs for Container Scanning diff --git a/ee/spec/finders/security/jobs_finder_spec.rb b/ee/spec/finders/security/jobs_finder_spec.rb index 83c0477b0428cd..756d4381ba741f 100644 --- a/ee/spec/finders/security/jobs_finder_spec.rb +++ b/ee/spec/finders/security/jobs_finder_spec.rb @@ -50,6 +50,14 @@ it { is_expected.to be_empty } end + context 'searching for all types takes precedence over excluding specific types' do + let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } + + let(:finder) { described_class.new(pipeline, all: true, dast: false) } + + it { is_expected.to eq([build]) } + end + context 'with dast jobs' do let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } -- GitLab From abecec10c7a609a10000e8e546526dae27bcdf16 Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Wed, 9 Oct 2019 12:30:23 +1100 Subject: [PATCH 08/10] Refactored jobs finder to simplify code --- ee/app/finders/security/jobs_finder.rb | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb index 73effab9b8a0ef..192777a319bd28 100644 --- a/ee/app/finders/security/jobs_finder.rb +++ b/ee/app/finders/security/jobs_finder.rb @@ -47,18 +47,16 @@ def find_jobs(job_types) end def find_jobs_legacy(job_types) - # first job type is added as a WHERE statement - query = @pipeline.builds.with_secure_reports_from_options(job_types.first) - - # following job types are added as OR statements - jobs = job_types.drop(1).reduce(query) do |qry, job_type| - qry.or(@pipeline.builds.with_secure_reports_from_options(job_type)) - end - # the query doesn't guarantee accuracy, so we verify it here - jobs.select do |job| + legacy_jobs_query(job_types) do |job| job_types.find { |job_type| job.options.dig(:artifacts, :reports, job_type) } end end + + def legacy_jobs_query(job_types) + job_types.map do |job_type| + @pipeline.builds.with_secure_reports_from_options(job_type) + end.reduce(&:or) + end end end -- GitLab From d8d3a8b1391bd408bc96bbc9f9bd5774648de846 Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Wed, 9 Oct 2019 12:54:24 +1100 Subject: [PATCH 09/10] The finder interface uses a job types array for clarity --- ee/app/finders/security/jobs_finder.rb | 38 ++++++++------------ ee/spec/finders/security/jobs_finder_spec.rb | 8 ++--- 2 files changed, 18 insertions(+), 28 deletions(-) diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb index 192777a319bd28..4943360fcdea82 100644 --- a/ee/app/finders/security/jobs_finder.rb +++ b/ee/app/finders/security/jobs_finder.rb @@ -5,13 +5,9 @@ # Used to find jobs (builds) that are for Secure products, SAST, DAST, Dependency Scanning and Container Scanning. # # Arguments: -# pipeline: only jobs for the specified pipeline will be found # params: -# all: boolean, include jobs for all secure job types. Takes precedence over other options. -# sast: boolean, include jobs for SAST -# dast: boolean, include jobs for DAST -# container_scanning: boolean, include jobs for Container Scanning -# dependency_scanning: boolean, include jobs for Dependency Scanning +# pipeline: required, only jobs for the specified pipeline will be found +# job_types: required, array of job types that should be returned, defaults to all job types module Security class JobsFinder @@ -19,42 +15,36 @@ class JobsFinder JOB_TYPES = [:sast, :dast, :dependency_scanning, :container_scanning].freeze - def initialize(pipeline, params = { all: true }) + def initialize(pipeline:, job_types: JOB_TYPES) @pipeline = pipeline - @params = params + @job_types = job_types end def execute - return [] if job_types_for_processing.empty? + return [] if @job_types.empty? if Feature.enabled?(:ci_build_metadata_config) - find_jobs(job_types_for_processing) + find_jobs else - find_jobs_legacy(job_types_for_processing) + find_jobs_legacy end end private - def job_types_for_processing - return JOB_TYPES if @params[:all] - - JOB_TYPES.select { |job_type| @params[job_type] } - end - - def find_jobs(job_types) - @pipeline.builds.with_secure_reports_from_config_options(job_types) + def find_jobs + @pipeline.builds.with_secure_reports_from_config_options(@job_types) end - def find_jobs_legacy(job_types) + def find_jobs_legacy # the query doesn't guarantee accuracy, so we verify it here - legacy_jobs_query(job_types) do |job| - job_types.find { |job_type| job.options.dig(:artifacts, :reports, job_type) } + legacy_jobs_query do |job| + @job_types.find { |job_type| job.options.dig(:artifacts, :reports, job_type) } end end - def legacy_jobs_query(job_types) - job_types.map do |job_type| + def legacy_jobs_query + @job_types.map do |job_type| @pipeline.builds.with_secure_reports_from_options(job_type) end.reduce(&:or) end diff --git a/ee/spec/finders/security/jobs_finder_spec.rb b/ee/spec/finders/security/jobs_finder_spec.rb index 756d4381ba741f..d428783fa4c152 100644 --- a/ee/spec/finders/security/jobs_finder_spec.rb +++ b/ee/spec/finders/security/jobs_finder_spec.rb @@ -4,7 +4,7 @@ describe Security::JobsFinder do let(:pipeline) { create(:ci_pipeline) } - let(:finder) { described_class.new(pipeline) } + let(:finder) { described_class.new(pipeline: pipeline, job_types: ::Security::JobsFinder::JOB_TYPES) } describe '#execute' do subject { finder.execute } @@ -53,7 +53,7 @@ context 'searching for all types takes precedence over excluding specific types' do let!(:build) { create(:ci_build, :dast, pipeline: pipeline) } - let(:finder) { described_class.new(pipeline, all: true, dast: false) } + let(:finder) { described_class.new(pipeline: pipeline, job_types: [:dast]) } it { is_expected.to eq([build]) } end @@ -99,7 +99,7 @@ let!(:container_scanning_build) { create(:ci_build, :container_scanning, pipeline: pipeline) } let!(:dast_build) { create(:ci_build, :dast, pipeline: pipeline) } - let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) } + let(:finder) { described_class.new(pipeline: pipeline, job_types: [:sast, :container_scanning]) } it 'returns only those requested' do is_expected.to include(sast_build) @@ -183,7 +183,7 @@ let!(:container_scanning_build) { create(:ci_build, :container_scanning, pipeline: pipeline) } let!(:dast_build) { create(:ci_build, :dast, pipeline: pipeline) } - let(:finder) { described_class.new(pipeline, { sast: true, container_scanning: true }) } + let(:finder) { described_class.new(pipeline: pipeline, job_types: [:sast, :container_scanning]) } it 'returns only those requested' do is_expected.to include(sast_build) -- GitLab From a22c802a9d2b1e6727856ffd580396d29816e743 Mon Sep 17 00:00:00 2001 From: Cameron Swords Date: Thu, 10 Oct 2019 14:41:31 -0500 Subject: [PATCH 10/10] Added a missing select and fixed a broken test --- ee/app/finders/security/jobs_finder.rb | 2 +- ee/spec/finders/security/jobs_finder_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/finders/security/jobs_finder.rb b/ee/app/finders/security/jobs_finder.rb index 4943360fcdea82..22087879f7ed7a 100644 --- a/ee/app/finders/security/jobs_finder.rb +++ b/ee/app/finders/security/jobs_finder.rb @@ -38,7 +38,7 @@ def find_jobs def find_jobs_legacy # the query doesn't guarantee accuracy, so we verify it here - legacy_jobs_query do |job| + legacy_jobs_query.select do |job| @job_types.find { |job_type| job.options.dig(:artifacts, :reports, job_type) } end end diff --git a/ee/spec/finders/security/jobs_finder_spec.rb b/ee/spec/finders/security/jobs_finder_spec.rb index d428783fa4c152..b7fe1b753dc0a5 100644 --- a/ee/spec/finders/security/jobs_finder_spec.rb +++ b/ee/spec/finders/security/jobs_finder_spec.rb @@ -44,7 +44,7 @@ context 'with jobs having report artifacts that are similar to secure artifacts' do before do - create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'sast.file' } } }) + create(:ci_build, pipeline: pipeline, options: { artifacts: { reports: { file: 'report:sast:result.file' } } }) end it { is_expected.to be_empty } -- GitLab