Open
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an orphaned-vulnerability cleanup step to Fleet’s vulnerabilities processing so vulnerability counts don’t grow indefinitely after hosts are removed (e.g., host expiry removing hosts but leaving behind OVAL-related vuln rows).
Changes:
- Extend
fleet.Datastorewith two new cleanup methods for orphaned software and OS vulnerability rows. - Implement MySQL deletes for orphaned
software_cveandoperating_system_vulnerabilities, with new unit tests covering the behavior. - Invoke both cleanup methods during the vulnerabilities scan cron run.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cmd/fleet/cron.go |
Calls the new orphan-cleanup datastore methods during vulnerability scanning. |
server/fleet/datastore.go |
Adds DeleteOrphanedSoftwareVulnerabilities and DeleteOrphanedOSVulnerabilities to the datastore interface. |
server/datastore/mysql/software.go |
Implements orphan cleanup for software_cve via LEFT JOIN host_software. |
server/datastore/mysql/software_test.go |
Adds coverage verifying orphaned software_cve rows are removed after host deletion. |
server/datastore/mysql/operating_system_vulnerabilities.go |
Implements orphan cleanup for operating_system_vulnerabilities via LEFT JOIN host_operating_system. |
server/datastore/mysql/operating_system_vulnerabilities_test.go |
Adds coverage verifying orphaned OS vuln rows are removed after host deletion. |
server/mock/datastore_mock.go |
Extends mock datastore to support the new interface methods. |
changes/28091-vulnerabilities-cleanup |
Adds release note entry for the bug fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis pull request adds cleanup operations for orphaned vulnerability records in the database. Two new datastore methods are introduced: DeleteOrphanedSoftwareVulnerabilities and DeleteOrphanedOSVulnerabilities. These methods are called after vulnerability scanning in the cron job to remove vulnerability entries associated with hosts that have been deleted. The changes include implementations for MySQL, interface declarations, mock implementations, and test coverage for both cleanup operations. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/datastore/mysql/operating_system_vulnerabilities_test.go (1)
368-431: Add a same-os_idmulti-host case.This proves cleanup removes vulns once an OS loses all hosts, but it won't catch an implementation that accidentally deletes rows when one of several hosts sharing the same
os_idis removed. Please add a case with two hosts mapped to the same OS, delete one, and assert the vulnerability remains.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/operating_system_vulnerabilities_test.go` around lines 368 - 431, In testDeleteOrphanedOSVulnerabilities, add a second host that shares the same OS as hostWithOS to ensure DeleteOrphanedOSVulnerabilities doesn't remove vulnerabilities when at least one host still references the OS: create e.g. hostShared := test.NewHost(t, ds, "host_shared", "", "hsharedkey", "hshareduuid", time.Now()), insert a host_operating_system row mapping hostShared.ID to osWithHostID (use the same INSERT INTO host_operating_system call or an additional ExecContext), then after deleting only hostToRemove call ds.DeleteOrphanedOSVulnerabilities(ctx) and assert ListOSVulnerabilitiesByOS(ctx, uint(osWithHostID)) still returns the vulnerability (CVE-2024-100); optionally then delete hostShared and assert the orphaned OS vuln is removed to cover full lifecycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/fleet/cron.go`:
- Around line 202-208: The current vuln-cron path calls
ds.DeleteOrphanedSoftwareVulnerabilities and ds.DeleteOrphanedOSVulnerabilities
synchronously in cmd/fleet/cron.go which runs full-table DELETEs and causes long
locks; remove these calls from the hot vulnerability cron and instead either (A)
move invocation to the lower-frequency cleanups/aggregation schedule where other
heavy sweeps run, or (B) change the implementations of
DeleteOrphanedSoftwareVulnerabilities and DeleteOrphanedOSVulnerabilities to
perform batched deletes (looping delete LIMIT N in a transaction until no rows
affected) to cap lock time and return quickly so the vuln cron only triggers a
lightweight operation. Ensure references to
ds.DeleteOrphanedSoftwareVulnerabilities and ds.DeleteOrphanedOSVulnerabilities
are updated accordingly and add logging to indicate batching or the new schedule
trigger.
In `@server/datastore/mysql/operating_system_vulnerabilities.go`:
- Around line 392-394: The DELETE currently only compares operating_system_id
and can remove rows that should remain if another host still uses that OS;
update the join to include the vulnerability row’s host scope by joining
host_operating_system (hos) on both hos.host_id = osv.host_id AND hos.os_id =
osv.operating_system_id, then keep the WHERE hos.host_id IS NULL predicate so
only operating_system_vulnerabilities (osv) whose specific host no longer has
that OS mapping are deleted; reference the osv alias
(operating_system_vulnerabilities), hos alias (host_operating_system), and the
host_id and operating_system_id columns when making this change.
---
Nitpick comments:
In `@server/datastore/mysql/operating_system_vulnerabilities_test.go`:
- Around line 368-431: In testDeleteOrphanedOSVulnerabilities, add a second host
that shares the same OS as hostWithOS to ensure DeleteOrphanedOSVulnerabilities
doesn't remove vulnerabilities when at least one host still references the OS:
create e.g. hostShared := test.NewHost(t, ds, "host_shared", "", "hsharedkey",
"hshareduuid", time.Now()), insert a host_operating_system row mapping
hostShared.ID to osWithHostID (use the same INSERT INTO host_operating_system
call or an additional ExecContext), then after deleting only hostToRemove call
ds.DeleteOrphanedOSVulnerabilities(ctx) and assert
ListOSVulnerabilitiesByOS(ctx, uint(osWithHostID)) still returns the
vulnerability (CVE-2024-100); optionally then delete hostShared and assert the
orphaned OS vuln is removed to cover full lifecycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2be8315d-3048-4fb2-9eab-d0eb62c6a857
📒 Files selected for processing (8)
changes/28091-vulnerabilities-cleanupcmd/fleet/cron.goserver/datastore/mysql/operating_system_vulnerabilities.goserver/datastore/mysql/operating_system_vulnerabilities_test.goserver/datastore/mysql/software.goserver/datastore/mysql/software_test.goserver/fleet/datastore.goserver/mock/datastore_mock.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #41195 +/- ##
==========================================
- Coverage 66.35% 66.35% -0.01%
==========================================
Files 2475 2475
Lines 198385 198405 +20
Branches 8856 8856
==========================================
+ Hits 131639 131649 +10
- Misses 54859 54860 +1
- Partials 11887 11896 +9
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:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #28091
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
Bug Fixes