Refactor Security::StoreReportService
Summary
When working on !46483 (merged) me and @minac noticed that this particular service is very hard to reason about and a simple change such as calculating UUIDv5 instead of UUIDv4 (!43881 (merged)) didn't work at first (#212322 (comment 434346683)) and required additional investigation (!45899 (merged)) and was finally fixed with !46483 (merged).
We should refine this issue in order to come up with a rough plan on how to simplify this or break it up in smaller parts so it's easier to reason about.
StoreReportService Responsibilities
- Store finding
- Store vulnerability
- Update update_vulnerability_scanners
- Reset remediations
- Update finding_signatures
- Update vulnerability_identifier
- Update vulnerability_links
- Associate findings with pipelines
- Update "resolved on default branch"
As we can see above, this service class has so many responsibilities.
Known issues
We should address all the issues listed below;
-
Not transactional -
If an error happens, an uncomplete finding will be created -
If an error happens the job terminates and does not save the rest of the findings
-
-
If an error happens, there is no indication to the end-user -
Has performance issues -
Dry-run option is not available
Implementation plan
-
backend Fix the known issues aforementioned by following; -
The improvements should be under a feature flag to control the risk of introducing a regression -
Responsibilities should be set clearly and handled by different modules/classes -
Implementation should be robust which means, does not propagate the exception to the caller -
Any kind of exception has to be logged -
We should put meaningful log messages for each operation to debug the system easily -
Any error that happened during the ingestion process should be added to the related Security::Scan
record -
The StoreReportService
should run after we ingest the findings for the pipeline because the validation runs on this process and we shouldn't try ingesting the invalid reports. -
Model layer validations should be applied -
We should also update the vulnerability statistics after ingestion(branch: update_vulnerability_statistics_after_ingestion)
-
Pseudo implementation
class StoreReportService_V2
BATCH_SIZE = 1_000
def initialize(pipeline, report)
@pipeline = pipeline
@report = report
end
def execute
update_scanners
update_findings
update_resolved_findings
end
private
def update_scanners
# ...
end
def update_findings
@vuln_ids ||= report.findings.each_slice(BATCH_SIZE).map do |batch|
FindingsProcessor.process(batch) # This must be transactional
end
end
def update_resolved_findings
# mark vulnerabilities as resolved where id != vuln_ids
end
end
Edited by Mehmet Emin INAC