Skip to content

Load only the requested report artifacts into memory

Mehmet Emin INAC requested to merge 238156_short_term_solution_to_load_pipeline_security_reports into master

What does this MR do?

The vulnerability_findings endpoint is currently trying to load all the security report artifacts into the memory regardless of the report_type filtering parameter. For this reason if there is a timeout error while loading the security reports for a pipeline, even if the user tries to load just one type of report, the timeout issue still exists. This probably(I didn't check the metrics) also causes memory bloat!

This MR fixes this behavior by just loading the requested report artifact into memory if so.

Related to #238156.

Note that, this does not fix the aforementioned issue but just makes the pipeline security tab partially available for problematic pipelines.

Database Review guideline

This MR does not introduce any new SQL query. There is just one query whose filtering parameter will be changed which shouldn't change the behavior of the query.

Original query

SELECT
    "ci_builds".*
FROM
    "ci_builds"
WHERE
    "ci_builds"."type" = 'Ci::Build'
    AND "ci_builds"."commit_id" = $commit_id
    AND ("ci_builds"."retried" IS FALSE
        OR "ci_builds"."retried" IS NULL)
    AND (EXISTS (
            SELECT
                1
            FROM
                "ci_job_artifacts"
            WHERE (ci_builds.id = ci_job_artifacts.job_id)
            AND "ci_job_artifacts"."file_type" IN (5, 6, 7, 8, 21, 23)))
 Nested Loop Semi Join  (cost=1.27..436.86 rows=1 width=1595) (actual time=27.977..27.978 rows=0 loops=1)
   Buffers: shared hit=13 read=8
   I/O Timings: read=27.860
   ->  Index Scan using index_ci_builds_on_commit_id_and_status_and_type on public.ci_builds  (cost=0.70..127.57 rows=113 width=1595) (actual time=21.810..27.929 rows=3 loops=1)
         Index Cond: ((ci_builds.commit_id = 174) AND ((ci_builds.type)::text = 'Ci::Build'::text))
         Filter: ((ci_builds.retried IS FALSE) OR (ci_builds.retried IS NULL))
         Rows Removed by Filter: 0
         Buffers: shared read=8
         I/O Timings: read=27.860
   ->  Index Only Scan using index_ci_job_artifacts_on_job_id_and_file_type on public.ci_job_artifacts  (cost=0.57..2.72 rows=1 width=4) (actual time=0.013..0.013 rows=0 loops=3)
         Index Cond: (ci_job_artifacts.job_id = ci_builds.id)
         Heap Fetches: 0
         Filter: (ci_job_artifacts.file_type = ANY ('{5,6,7,8,21,23}'::integer[]))
         Rows Removed by Filter: 1
         Buffers: shared hit=13

https://gitlab.slack.com/archives/CLJMDRD8C/p1597777377382500

An example new query

SELECT
    "ci_builds".*
FROM
    "ci_builds"
WHERE
    "ci_builds"."type" = 'Ci::Build'
    AND "ci_builds"."commit_id" = $commit_id
    AND ("ci_builds"."retried" IS FALSE
        OR "ci_builds"."retried" IS NULL)
    AND (EXISTS (
            SELECT
                1
            FROM
                "ci_job_artifacts"
            WHERE (ci_builds.id = ci_job_artifacts.job_id)
            AND "ci_job_artifacts"."file_type" IN (5, 6)))
 Nested Loop Semi Join  (cost=1.27..431.73 rows=1 width=1595) (actual time=0.042..0.042 rows=0 loops=1)
   Buffers: shared hit=21
   ->  Index Scan using index_ci_builds_on_commit_id_and_status_and_type on public.ci_builds  (cost=0.70..127.57 rows=113 width=1595) (actual time=0.020..0.026 rows=3 loops=1)
         Index Cond: ((ci_builds.commit_id = 174) AND ((ci_builds.type)::text = 'Ci::Build'::text))
         Filter: ((ci_builds.retried IS FALSE) OR (ci_builds.retried IS NULL))
         Rows Removed by Filter: 0
         Buffers: shared hit=8
   ->  Index Only Scan using index_ci_job_artifacts_on_job_id_and_file_type on public.ci_job_artifacts  (cost=0.57..2.67 rows=1 width=4) (actual time=0.004..0.004 rows=0 loops=3)
         Index Cond: (ci_job_artifacts.job_id = ci_builds.id)
         Heap Fetches: 0
         Filter: (ci_job_artifacts.file_type = ANY ('{5,6}'::integer[]))
         Rows Removed by Filter: 1
         Buffers: shared hit=13

https://gitlab.slack.com/archives/CLJMDRD8C/p1597777452385000

These two are same though.

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Mehmet Emin INAC

Merge request reports