Merge branch '13707-summary-timeout' into 'master'

Group Security Dashboard `summary` endpoint is timing out

Closes #13707

See merge request gitlab-org/gitlab!21373

ee/app/controllers/concerns/project_vulnerability_findings_actions.rb

@@ -23,11 +23,11 @@ module ProjectVulnerabilityFindingsActions
   end
 
   def summary
-    vulnerabilities_summary = vulnerability_findings.counted_by_severity
+    summary = Gitlab::Vulnerabilities::Summary.new(vulnerable, filter_params).findings_counter
 
     respond_to do |format|
       format.json do
-        render json: VulnerabilitySummarySerializer.new.represent(vulnerabilities_summary)
+        render json: summary
       end
     end
   end

ee/changelogs/unreleased/13707-summary-timeout.yml

+---
+title: Cache vulnerability summary per project/group
+merge_request: 21373
+author:
+type: performance

ee/lib/gitlab/vulnerabilities/summary.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Vulnerabilities
+    class Summary
+      attr_reader :vulnerable, :filters
+
+      def initialize(vulnerable, params)
+        @filters = params
+        @vulnerable = vulnerable
+      end
+
+      def findings_counter
+        return cached_vulnerability_summary if use_vulnerability_cache?
+
+        vulnerabilities = found_vulnerabilities.counted_by_severity
+        VulnerabilitySummarySerializer.new.represent(vulnerabilities)
+      end
+
+      private
+
+      def cached_vulnerability_summary
+        summary = {
+          undefined: 0,
+          info:      0,
+          unknown:   0,
+          low:       0,
+          medium:    0,
+          high:      0,
+          critical:  0
+        }
+
+        summary_keys = ::Vulnerabilities::Occurrence::SEVERITY_LEVELS.keys.map(&:to_sym)
+
+        project_ids_to_fetch.each do |project_id|
+          project_summary = Gitlab::Vulnerabilities::SummaryCache
+            .new(vulnerable, project_id)
+            .fetch
+
+          summary_keys.each do |key|
+            summary[key] += project_summary[key] unless project_summary[key].nil?
+          end
+        end
+
+        summary
+      end
+
+      def found_vulnerabilities
+        ::Security::VulnerabilityFindingsFinder.new(vulnerable, params: filters).execute
+      end
+
+      def use_vulnerability_cache?
+        Feature.enabled?(:cache_vulnerability_summary, vulnerable) && !dynamic_filters_included?
+      end
+
+      def dynamic_filters_included?
+        dynamic_filters = [:report_type, :confidence, :severity]
+
+        filters.keys.any? { |k| dynamic_filters.include?(k.to_sym) }
+      end
+
+      def project_ids_to_fetch
+        project_ids = vulnerable.is_a?(Project) ? [vulnerable.id] : []
+
+        return filters[:project_id] + project_ids if filters.key?('project_id')
+
+        vulnerable.is_a?(Group) ? vulnerable.project_ids_with_security_reports : project_ids
+      end
+    end
+  end
+end

ee/lib/gitlab/vulnerabilities/summary_cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Vulnerabilities
+    class SummaryCache
+      attr_reader :vulnerable, :project_id
+
+      def initialize(vulnerable, project_id)
+        @vulnerable = vulnerable
+        @project_id = project_id
+      end
+
+      def fetch(force: false)
+        Rails.cache.fetch(cache_key, force: force, expires_in: 1.day) do
+          findings = ::Security::VulnerabilityFindingsFinder
+            .new(vulnerable, params: { project_id: [project_id] })
+            .execute(:all)
+            .counted_by_severity
+
+          VulnerabilitySummarySerializer.new.represent(findings)
+        end
+      end
+
+      private
+
+      def cache_key
+        ['projects', project_id, 'findings-summary']
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Vulnerabilities::SummaryCache 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
+      summary_cache = described_class.new(group, project.id)
+
+      expect(Rails.cache.fetch(project_cache_key, raw: true)).to be_nil
+
+      control_count = ActiveRecord::QueryRecorder.new { summary_cache.fetch }
+
+      expect { 2.times { summary_cache.fetch } }.not_to exceed_query_limit(control_count)
+    end
+
+    it 'returns the proper format for uncached summary' do
+      Timecop.freeze do
+        fetched_history = described_class.new(group, project.id).fetch
+
+        expect(fetched_history[:high]).to eq(1)
+      end
+    end
+
+    it 'returns the proper format for cached summary' do
+      Timecop.freeze do
+        described_class.new(group, project.id).fetch
+        fetched_history = described_class.new(group, project.id).fetch
+
+        expect(fetched_history[:high]).to eq(1)
+      end
+    end
+
+    def create_vulnerabilities(count, project, options = {})
+      pipeline = create(:ci_pipeline, :success, project: project)
+
+      create_list(
+        :vulnerabilities_occurrence,
+        count,
+        report_type: options[:report_type] || :sast,
+        pipelines:   [pipeline],
+        project:     project
+      )
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Vulnerabilities::Summary 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 '#findings_counter', :use_clean_rails_memory_store_caching do
+    subject(:counter) { described_class.new(group, filters).findings_counter }
+
+    context 'feature disabled' do
+      before do
+        stub_feature_flags(cache_vulnerability_summary: false)
+      end
+
+      it 'does not call Gitlab::Vulnerabilities::SummaryCache' do
+        expect(Gitlab::Vulnerabilities::SummaryCache).not_to receive(:new)
+
+        counter
+      end
+
+      it 'returns the proper format for the summary' do
+        Timecop.freeze do
+          expect(counter[:high]).to eq(2)
+        end
+      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::SummaryCache' do
+          expect(Gitlab::Vulnerabilities::SummaryCache).not_to receive(:new)
+
+          counter
+        end
+      end
+
+      it 'calls Gitlab::Vulnerabilities::SummaryCache' do
+        expect(Gitlab::Vulnerabilities::SummaryCache).to receive(:new).twice.and_call_original
+
+        counter
+      end
+
+      it 'returns the proper format for the summary' do
+        Timecop.freeze do
+          expect(counter[:high]).to eq(2)
+        end
+      end
+    end
+
+    def create_vulnerabilities(count, project, options = {})
+      pipeline = create(:ci_pipeline, :success, project: project)
+
+      create_list(
+        :vulnerabilities_occurrence,
+        count,
+        report_type: options[:report_type] || :sast,
+        severity:    options[:severity] || :high,
+        pipelines:   [pipeline],
+        project:     project,
+        created_at:  options[:created_at] || Date.today
+      )
+    end
+  end
+end

ee/spec/requests/security/vulnerability_findings_spec.rb

@@ -29,7 +29,7 @@ shared_examples 'instance security dashboard vulnerability findings endpoint' do
 
       expect(findings_fetcher).to receive(:new).with(
         anything,
-        params: hash_including('project_id' => [findings_project.id])
+        params: hash_including(project_id: [findings_project.id])
       ).and_return(fetcher_double)
 
       get findings_endpoint, headers: findings_headers
@@ -45,7 +45,7 @@ shared_examples 'instance security dashboard vulnerability findings endpoint' do
 
       expect(findings_fetcher).to receive(:new).with(
         anything,
-        params: hash_including('project_id' => [findings_project.id])
+        params: hash_including(project_id: [findings_project.id])
       ).and_return(fetcher_double)
 
       get findings_endpoint, headers: findings_headers, params: project_ids_param
@@ -77,7 +77,7 @@ shared_examples 'instance security dashboard vulnerability findings endpoint' do
 
         expect(findings_fetcher).to receive(:new).with(
           anything,
-          params: hash_including('project_id' => [findings_project.id])
+          params: hash_including(project_id: [findings_project.id])
         ).and_return(fetcher_double)
 
         get findings_endpoint, headers: findings_headers, params: project_ids_param
@@ -90,13 +90,14 @@ shared_examples 'instance security dashboard vulnerability findings endpoint' do
       let(:findings_user) { create(:auditor) }
 
       it 'fetches vulnerabilities for all given projects on their dashboard' do
+        stub_feature_flags(cache_vulnerability_summary: false)
         unmembered_project = create(:project)
         findings_user.security_dashboard_projects << unmembered_project
         project_ids_param = { project_id: [findings_project.id, unmembered_project.id] }
 
         expect(findings_fetcher).to receive(:new).with(
           anything,
-          params: hash_including('project_id' => array_including(findings_project.id, unmembered_project.id))
+          params: hash_including(project_id: array_including(findings_project.id, unmembered_project.id))
         ).and_return(fetcher_double)
 
         get findings_endpoint, headers: findings_headers, params: project_ids_param