Merge branch 'rf-history-endpoint-timeout' into 'master'

Cache vulnerability history per project

See merge request gitlab-org/gitlab-ee!14619

ee/app/controllers/concerns/vulnerabilities_actions.rb

@@ -9,8 +9,6 @@
 module VulnerabilitiesActions
   extend ActiveSupport::Concern
 
-  HISTORY_RANGE = 3.months
-
   def index
     vulnerabilities = found_vulnerabilities(:with_sha).ordered.page(params[:page])
 
@@ -35,11 +33,11 @@ module VulnerabilitiesActions
   end
 
   def history
-    vulnerabilities_counter = found_vulnerabilities(:all).count_by_day_and_severity(HISTORY_RANGE)
+    history_count = Gitlab::Vulnerabilities::History.new(group, filter_params).vulnerabilities_counter
 
     respond_to do |format|
       format.json do
-        render json: Vulnerabilities::HistorySerializer.new.represent(vulnerabilities_counter)
+        render json: history_count
       end
     end
   end

ee/app/models/ee/group.rb

@@ -169,6 +169,10 @@ module EE
       projects.detect { |project| !project.empty_repo? }
     end
 
+    def project_ids_with_security_reports
+      all_projects.where('EXISTS (?)', ::Vulnerabilities::Occurrence.select(1).where('vulnerability_occurrences.project_id = projects.id')).pluck_primary_key
+    end
+
     def root_ancestor_ip_restriction
       return ip_restriction if parent_id.nil?
 

ee/app/services/security/store_reports_service.rb

@@ -15,11 +15,23 @@ module Security
         errors << result[:message] if result[:status] == :error
       end
 
+      cache_vulnerabilities
+
       if errors.any?
         error(errors.join(", "))
       else
         success
       end
     end
+
+    # Silently swallow errors if there are any problems caching vulnerabilities
+    def cache_vulnerabilities
+      project = @pipeline.project
+      if Feature.enabled?(:cache_vulnerability_history, project.group)
+        Gitlab::Vulnerabilities::HistoryCache.new(project.group, project.id).fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE, force: true)
+      end
+    rescue => err
+      error("Failed to cache vulnerabilities for pipeline #{@pipeline.id}: #{err}")
+    end
   end
 end

ee/changelogs/unreleased/rf-history-endpoint-timeout.yml

+---
+title: Cache vulnerability history per project
+merge_request: 14619
+author:
+type: performance

ee/lib/gitlab/vulnerabilities/history.rb

+# frozen_string_literal: true
+
+require 'vulnerabilities/history_serializer'
+
+module Gitlab
+  module Vulnerabilities
+    class History
+      attr_reader :group, :filters
+
+      HISTORY_RANGE = 3.months
+
+      def initialize(group, filters)
+        @group = group
+        @filters = filters
+      end
+
+      def vulnerabilities_counter
+        return cached_vulnerability_history if use_vulnerability_cache?
+
+        vulnerabilities = found_vulnerabilities.count_by_day_and_severity(HISTORY_RANGE)
+        ::Vulnerabilities::HistorySerializer.new.represent(vulnerabilities)
+      end
+
+      private
+
+      def found_vulnerabilities
+        ::Security::VulnerabilitiesFinder.new(group, params: filters).execute(:all)
+      end
+
+      def cached_vulnerability_history
+        history = { undefined: {}, info: {}, unknown: {}, low: {}, medium: {}, high: {}, critical: {}, total: {} }
+
+        project_ids_to_fetch.each do |project_id|
+          project_history = Gitlab::Vulnerabilities::HistoryCache.new(group, project_id).fetch(HISTORY_RANGE)
+          history_keys = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.map(&:to_sym)
+          history_keys << :total
+          history_keys.each do |key|
+            history[key].merge!(project_history[key]) { |k, aggregate, project_count| aggregate + project_count }
+          end
+        end
+
+        history[:total] = history[:total].sort_by { |date, count| date }.to_h
+        history
+      end
+
+      def use_vulnerability_cache?
+        Feature.enabled?(:cache_vulnerability_history, group) && !dynamic_filters_included?
+      end
+
+      def dynamic_filters_included?
+        dynamic_filters = [:report_type, :confidence, :severity]
+        filters.keys.any? { |key| dynamic_filters.include?(key.to_sym) }
+      end
+
+      def project_ids_to_fetch
+        return filters[:project_id] if filters.key?('project_id')
+
+        group.project_ids_with_security_reports
+      end
+    end
+  end
+end

ee/lib/gitlab/vulnerabilities/history_cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Vulnerabilities
+    class HistoryCache
+      attr_reader :group, :project_id
+
+      def initialize(group, project_id)
+        @group = group
+        @project_id = project_id
+      end
+
+      def fetch(range, force: false)
+        Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do
+          vulnerabilities = ::Security::VulnerabilitiesFinder
+            .new(group, params: { project_id: [project_id] })
+            .execute(:all)
+            .count_by_day_and_severity(range)
+          ::Vulnerabilities::HistorySerializer.new.represent(vulnerabilities)
+        end
+      end
+
+      private
+
+      def cache_key
+        ['projects', project_id, 'vulnerabilities']
+      end
+    end
+  end
+end

ee/spec/lib/gitlab/vulnerabilities/history_cache_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Vulnerabilities::HistoryCache do
+  let(:group) { create(:group) }
+  let(:project) { create(:project, :public, namespace: group) }
+  let(:project_cache_key) { described_class.new(group, project.id).send(:cache_key) }
+
+  before do
+    create_vulnerabilities(1, project)
+  end
+
+  describe '#fetch', :use_clean_rails_memory_store_caching do
+    it 'reads from cache when records are cached' do
+      history_cache = described_class.new(group, project.id)
+
+      expect(Rails.cache.fetch(project_cache_key, raw: true)).to be_nil
+
+      control_count = ActiveRecord::QueryRecorder.new { history_cache.fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE) }
+
+      expect { 2.times { history_cache.fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE) } }.not_to exceed_query_limit(control_count)
+    end
+
+    it 'returns the proper format for uncached history' do
+      Timecop.freeze do
+        fetched_history = described_class.new(group, project.id).fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE)
+
+        expect(fetched_history[:total]).to eq( Date.today => 1 )
+        expect(fetched_history[:high]).to eq( Date.today => 1 )
+      end
+    end
+
+    it 'returns the proper format for cached history' do
+      Timecop.freeze do
+        described_class.new(group, project.id).fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE)
+        fetched_history = described_class.new(group, project.id).fetch(Gitlab::Vulnerabilities::History::HISTORY_RANGE)
+
+        expect(fetched_history[:total]).to eq( Date.today => 1 )
+        expect(fetched_history[:high]).to eq( Date.today => 1 )
+      end
+    end
+
+    def create_vulnerabilities(count, project, options = {})
+      report_type = options[:report_type] || :sast
+      pipeline = create(:ci_pipeline, :success, project: project)
+      create_list(:vulnerabilities_occurrence, count, report_type: report_type, pipelines: [pipeline], project: project)
+    end
+  end
+end

ee/spec/lib/gitlab/vulnerabilities/history_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Vulnerabilities::History do
+  let(:group) { create(:group) }
+  let(:project1) { create(:project, :public, namespace: group) }
+  let(:project2) { create(:project, :public, namespace: group) }
+  let(:filters) { {} }
+
+  before do
+    create_vulnerabilities(1, project1, { severity: :medium, report_type: :sast })
+    create_vulnerabilities(2, project2, { severity: :high, report_type: :sast })
+  end
+
+  describe '#vulnerabilities_counter', :use_clean_rails_memory_store_caching do
+    subject(:counter) { described_class.new(group, filters).vulnerabilities_counter }
+
+    context 'feature disabled' do
+      before do
+        stub_feature_flags(cache_vulnerability_history: false)
+      end
+
+      it 'does not call Gitlab::Vulnerabilities::HistoryCache' do
+        expect(Gitlab::Vulnerabilities::HistoryCache).not_to receive(:new)
+
+        counter
+      end
+
+      it 'returns the proper format for the history' do
+        expect(counter[:total]).to eq({ Date.today => 3 })
+        expect(counter[:high]).to eq({ Date.today => 2 })
+      end
+    end
+
+    context 'feature enabled' do
+      before do
+        stub_feature_flags(cache_vulnerability_history: true)
+      end
+
+      context 'filters are passed' do
+        let(:filters) { { report_type: :sast } }
+
+        it 'does not call Gitlab::Vulnerabilities::HistoryCache' do
+          expect(Gitlab::Vulnerabilities::HistoryCache).not_to receive(:new)
+
+          counter
+        end
+      end
+
+      it 'calls Gitlab::Vulnerabilities::HistoryCache' do
+        expect(Gitlab::Vulnerabilities::HistoryCache).to receive(:new).twice.and_call_original
+
+        counter
+      end
+
+      it 'returns the proper format for the history' do
+        expect(counter[:total]).to eq({ Date.today => 3 })
+        expect(counter[:high]).to eq({ Date.today => 2 })
+      end
+    end
+
+    def create_vulnerabilities(count, project, options = {})
+      report_type = options[:report_type] || :sast
+      severity = options[:severity] || :high
+      pipeline = create(:ci_pipeline, :success, project: project)
+      create_list(:vulnerabilities_occurrence, count, report_type: report_type, severity: severity, pipelines: [pipeline], project: project)
+    end
+  end
+end

ee/spec/services/security/store_reports_service_spec.rb

@@ -2,51 +2,88 @@
 
 require 'spec_helper'
 
-describe Security::StoreReportsService, '#execute' do
-  let(:pipeline) { create(:ci_pipeline) }
+describe Security::StoreReportsService do
+  let(:group)   { create(:group) }
+  let(:project) { create(:project, :public, namespace: group) }
+  let(:pipeline) { create(:ci_pipeline, project: project) }
 
-  subject { described_class.new(pipeline).execute }
+  describe '#execute' do
+    subject { described_class.new(pipeline).execute }
 
-  context 'when there are reports' do
-    before do
-      stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true)
-      create(:ee_ci_build, :sast, pipeline: pipeline)
-      create(:ee_ci_build, :dependency_scanning, pipeline: pipeline)
-      create(:ee_ci_build, :container_scanning, pipeline: pipeline)
-    end
+    context 'when there are reports' do
+      before do
+        stub_licensed_features(sast: true, dependency_scanning: true, container_scanning: true)
+        create(:ee_ci_build, :sast, pipeline: pipeline)
+        create(:ee_ci_build, :dependency_scanning, pipeline: pipeline)
+        create(:ee_ci_build, :container_scanning, pipeline: pipeline)
+      end
 
-    it 'initializes and execute a StoreReportService for each report' do
-      expect(Security::StoreReportService).to receive(:new)
-        .exactly(3).times.with(pipeline, instance_of(::Gitlab::Ci::Reports::Security::Report))
-        .and_wrap_original do |method, *original_args|
-          method.call(*original_args).tap do |store_service|
-            expect(store_service).to receive(:execute).once.and_call_original
+      it 'initializes and execute a StoreReportService for each report' do
+        expect(Security::StoreReportService).to receive(:new)
+          .exactly(3).times.with(pipeline, instance_of(::Gitlab::Ci::Reports::Security::Report))
+          .and_wrap_original do |method, *original_args|
+            method.call(*original_args).tap do |store_service|
+              expect(store_service).to receive(:execute).once.and_call_original
+            end
           end
+
+        subject
+      end
+
+      context 'when StoreReportService returns an error for a report' do
+        let(:reports) { Gitlab::Ci::Reports::Security::Reports.new(pipeline.sha) }
+        let(:sast_report) { reports.get_report('sast') }
+        let(:dast_report) { reports.get_report('dast') }
+        let(:success) { { status: :success } }
+        let(:error) { { status: :error, message: "something went wrong" } }
+
+        before do
+          allow(pipeline).to receive(:security_reports).and_return(reports)
         end
 
-      subject
+        it 'returns the errors after having processed all reports' do
+          expect_next_instance_of(Security::StoreReportService, pipeline, sast_report) do |store_service|
+            expect(store_service).to receive(:execute).and_return(error)
+          end
+          expect_next_instance_of(Security::StoreReportService, pipeline, dast_report) do |store_service|
+            expect(store_service).to receive(:execute).and_return(success)
+          end
+
+          is_expected.to eq(error)
+        end
+      end
     end
 
-    context 'when StoreReportService returns an error for a report' do
-      let(:reports) { Gitlab::Ci::Reports::Security::Reports.new(pipeline.sha) }
-      let(:sast_report) { reports.get_report('sast') }
-      let(:dast_report) { reports.get_report('dast') }
-      let(:success) { { status: :success } }
-      let(:error) { { status: :error, message: "something went wrong" } }
+    context 'history caching' do
+      it 'swallows errors' do
+        allow( Gitlab::Vulnerabilities::HistoryCache).to receive(:new)
+          .and_raise("error")
 
-      before do
-        allow(pipeline).to receive(:security_reports).and_return(reports)
+        expect { subject }.not_to raise_error
       end
 
-      it 'returns the errors after having processed all reports' do
-        expect_next_instance_of(Security::StoreReportService, pipeline, sast_report) do |store_service|
-          expect(store_service).to receive(:execute).and_return(error)
+      context 'feature disabled' do
+        before do
+          stub_feature_flags(cache_vulnerability_history: false)
+        end
+
+        it 'does not cache vulnerability history' do
+          expect_any_instance_of(Gitlab::Vulnerabilities::HistoryCache).not_to receive(:fetch)
+
+          subject
         end
-        expect_next_instance_of(Security::StoreReportService, pipeline, dast_report) do |store_service|
-          expect(store_service).to receive(:execute).and_return(success)
+      end
+
+      context 'feature enabled' do
+        before do
+          stub_feature_flags(cache_vulnerability_history: true)
         end
 
-        is_expected.to eq(error)
+        it 'caches vulnerability history' do
+          expect_any_instance_of(Gitlab::Vulnerabilities::HistoryCache).to receive(:fetch)
+
+          subject
+        end
       end
     end
   end

ee/spec/workers/store_security_reports_worker_spec.rb

@@ -4,8 +4,9 @@ require 'spec_helper'
 
 describe StoreSecurityReportsWorker do
   describe '#perform' do
-    let(:pipeline) { create(:ci_pipeline, ref: 'master') }
-    let(:project) { pipeline.project }
+    let(:group)   { create(:group) }
+    let(:project) { create(:project, :public, namespace: group) }
+    let(:pipeline) { create(:ci_pipeline, ref: 'master', project: project) }
 
     before do
       allow(Ci::Pipeline).to receive(:find).with(pipeline.id) { pipeline }