Only collect data about tracked CVEs - #45247
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
| // titles, unioned with all critical OS vulnerabilities. Used by the CVE | ||
| // collector to scope collection to only the CVEs the chart actually | ||
| // renders. See TODO in the mysql implementation. | ||
| TrackedCriticalCVEs(ctx context.Context) ([]string, error) |
There was a problem hiding this comment.
this method already exists, it's just being added to the Chart bounded-context's datastore interface.
WalkthroughThis PR scopes CVE vulnerability dataset collection to a curated set of critical CVEs to improve performance under large datasets. The 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves vulnerability dashboard performance by limiting CVE chart data collection to only the curated/“tracked” critical CVEs that the chart API already scopes to, instead of collecting host impact bitmaps for every CVE affecting any host.
Changes:
- Updated the chart datastore interfaces and MySQL implementation so
AffectedHostIDsByCVEaccepts an explicitcvesallowlist and short-circuits on an empty set. - Updated
CVEDatasetcollection to fetchTrackedCriticalCVEsand collect only those CVEs, while still writing empty snapshots to close stale rows. - Added/updated unit tests to verify forwarding of the tracked CVE set and correct behavior when the tracked set is empty.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| server/chart/internal/types/chart.go | Updates internal datastore interface for CVE host grouping to accept an explicit CVE allowlist. |
| server/chart/internal/service/service_test.go | Extends collector tests to ensure tracked CVEs are forwarded and empty tracked sets still write snapshots. |
| server/chart/internal/mysql/charts.go | Implements CVE allowlist filtering in the MySQL query and adds early return for empty lists. |
| server/chart/datasets.go | Scopes CVE dataset collection to TrackedCriticalCVEs and preserves snapshot semantics on empty input. |
| server/chart/api/chart.go | Extends DatasetStore with TrackedCriticalCVEs and updates CVE collection API contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sgress454
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45247 +/- ##
==========================================
+ Coverage 66.81% 66.84% +0.03%
==========================================
Files 2722 2723 +1
Lines 218994 219036 +42
Branches 10754 10754
==========================================
+ Hits 146311 146424 +113
+ Misses 59517 59445 -72
- Partials 13166 13167 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| var swArgs, osArgs []any | ||
| swWhere := []string{"sc.cve IN (?)"} | ||
| osWhere := []string{"osv.cve IN (?)"} |
There was a problem hiding this comment.
we don't have to worry about limiting these IN clauses right? I can't remember the max that we hit on mysql, but I think we will be safe here correct?
There was a problem hiding this comment.
I wondered the same. Our parameter limit for prepared statements is 65k. Based on the dataset we use in load test (which is based on customer data) there were 832 CVEs matching the criteria, so we've got tons of headroom there.
There was a problem hiding this comment.
Much smaller than I imagined. Seems like a non-issue.
Cherry-pick of #45247 into the RC branch.
Related issue: Resolves #45163
Details
Limits CVE data collection to only those CVEs which we report on in the chart. This is a performance optimization necessitated by the large amount of data that bigger fleets may generate. The plan is to implement a data compression strategy so that we can go back to collecting full CVE data soon.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
n/a, unreleased
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Testing
For unreleased bug fixes in a release candidate, one of:
should improve results!
Summary by CodeRabbit