33494 optimize os versions response - #33691
Conversation
This reverts commit 62b5260.
|
@coderabbitai full review |
|
@getvictor: I'll perform a comprehensive review of all the changes in this PR, including the optimization for OS versions response and the seed tool updates. ✅ Actions performedFull review triggered. |
✅ Actions performedFull review triggered. |
WalkthroughImplements server-side sorting, counting, and pagination for OS versions and restricts vulnerability/kernel loading to the current page. Refactors the vulnerability seeder to use the datastore API (instead of raw SQL), adds CLI flags for platform cohorts, and updates README accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Service as Service (hosts.go)
participant DS as Datastore
Note over Client,Service: OS versions API request (with order/page params)
Client->>Service: GET /api/.../os_versions?order=hosts_count&page,per_page
Service->>DS: Query total count and OS versions (filtered)
DS-->>Service: total, OS versions (unpaged)
Note right of Service: sort by hosts_count and slice to requested page
Service->>DS: Fetch vulnerabilities/kernels for paged OS version IDs only
DS-->>Service: Vulnerabilities and kernels mapped by os_version_id
Service-->>Client: Paged OS versions + per-item vulns/kernels + pagination metadata
sequenceDiagram
autonumber
participant User
participant Seed as seed_vuln_data (CLI)
participant DS as Datastore
Note over User,Seed: Run seeder with platform flags
User->>Seed: go run ... --macos N --windows M --ubuntu K --linux-kernels X
loop per generated host
Seed->>DS: createOrGetHost(ctx, ds, identifier, base)
DS-->>Seed: Host
alt OS fields to set
Seed->>DS: UpdateHostOperatingSystem(host, os)
DS-->>Seed: OK
end
Seed->>Seed: Load platform CSV -> []fleet.Software
Seed->>DS: UpdateHostSoftware(host, softwareList)
DS-->>Seed: OK
end
opt add kernel packages for Ubuntu hosts
Seed->>DS: UpdateHostSoftware(ubuntuHosts, kernelPackages)
DS-->>Seed: OK
end
Seed-->>User: Done.
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/software/vulnerabilities/README.md (1)
16-16: Correct “Comma seperated” to “Comma separated”.The note should use the standard spelling “Comma separated values…”. Please fix the typo. (dictionary.com)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changes/33494-os-version-response-time(1 hunks)server/service/hosts.go(3 hunks)tools/software/vulnerabilities/README.md(2 hunks)tools/software/vulnerabilities/seed_vuln_data.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
tools/software/vulnerabilities/seed_vuln_data.goserver/service/hosts.go
| // Return only the page, but with total count | ||
| return &fleet.OSVersions{OSVersions: paged}, count, meta, nil |
There was a problem hiding this comment.
Preserve CountsUpdatedAt when returning the paged slice
By constructing a brand-new fleet.OSVersions here, you drop the CountsUpdatedAt field that came back from the datastore. The endpoint then emits a zero timestamp in counts_updated_at, regressing the API contract/UI display. Reuse the original struct (or copy the field) before returning.
- // Return only the page, but with total count
- return &fleet.OSVersions{OSVersions: paged}, count, meta, nil
+ // Return only the page while preserving metadata from the datastore
+ osVersions.OSVersions = paged
+ return osVersions, count, meta, nil🤖 Prompt for AI Agents
In server/service/hosts.go around lines 2245-2246, the code constructs a new
fleet.OSVersions for the paged results which drops the CountsUpdatedAt timestamp
returned from the datastore; copy the CountsUpdatedAt field from the original
datastore result (or return the original struct with OSVersions replaced by the
paged slice) so the returned object preserves CountsUpdatedAt before returning
the value, count, meta, nil.
There was a problem hiding this comment.
great catch, added tests to catch countUpdatedAt zero values
|
|
||
| ## Usage | ||
|
|
||
| 1. Ensure your local development enviornment is running using `docker-compose up` and `fleet serve` |
There was a problem hiding this comment.
Fix spelling of “environment”.
The instructions should read “Ensure your local development environment…”. Please correct the misspelling. (difference.wiki)
🤖 Prompt for AI Agents
In tools/software/vulnerabilities/README.md at line 8, the word "enviornment" is
misspelled; update the sentence to read "Ensure your local development
environment is running using `docker-compose up` and `fleet serve`" replacing
"enviornment" with "environment".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tools/software/vulnerabilities/README.md (1)
8-8: Fix spelling of "environment".The word "enviornment" is misspelled; it should be "environment".
server/service/hosts.go (1)
2245-2246: PreserveCountsUpdatedAtwhen returning the paged slice.Constructing a new
fleet.OSVersionshere drops theCountsUpdatedAtfield from the original datastore result. The endpoint will emit a zero timestamp incounts_updated_at, regressing the API contract. Reuse the original struct (or copy the field) before returning.Apply this diff to preserve the metadata:
- // Return only the page, but with total count - return &fleet.OSVersions{OSVersions: paged}, count, meta, nil + // Return only the page while preserving metadata from the datastore + osVersions.OSVersions = paged + return osVersions, count, meta, nil
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
changes/33494-os-version-response-time(1 hunks)server/service/hosts.go(3 hunks)tools/software/vulnerabilities/README.md(2 hunks)tools/software/vulnerabilities/seed_vuln_data.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/hosts.gotools/software/vulnerabilities/seed_vuln_data.go
🔇 Additional comments (21)
tools/software/vulnerabilities/seed_vuln_data.go (10)
33-60: LGTM!CSV reading logic is correct: properly handles file operations, defers close, skips header, and maps columns to
fleet.Softwarefields.
62-71: LGTM!The
createOrGetHosthelper correctly implements idempotent host creation: it retrieves existing hosts by identifier and only creates new ones if not found, aligning with the README's claim that the tool won't create duplicates.
73-82: LGTM!CLI flags are correctly declared with descriptive names and sensible defaults (0 = no action), matching the usage documented in the README.
83-96: LGTM!Datastore initialization correctly uses the Fleet datastore API and hardcoded local dev credentials (appropriate for a seed tool), replacing the old raw SQL approach.
100-125: LGTM!macOS host creation logic is correct: uses idempotent helper, handles errors gracefully (logs and continues), and sets appropriate platform/OS metadata.
127-152: LGTM!Windows host creation follows the same correct pattern as macOS: idempotent, error-tolerant, and sets appropriate metadata.
154-187: LGTM!Ubuntu host creation correctly varies the OS version per host (20.04.1, 20.04.2, etc.), calls
UpdateHostOperatingSystemto populate detailed OS metadata, and handles errors gracefully.
189-201: LGTM!macOS software loading correctly uses the datastore
UpdateHostSoftwareAPI, reads from the expected CSV path, and handles errors gracefully.
203-215: LGTM!Windows software loading mirrors the macOS approach and correctly uses the datastore API with the expected CSV path.
217-234: LGTM!Linux kernel package generation correctly creates kernel software entries with
IsKernel=true, varying the version per package, and uses the datastore API to insert them.changes/33494-os-version-response-time (1)
1-1: LGTM!Change file correctly documents the optimization for the os_versions API response time.
tools/software/vulnerabilities/README.md (3)
3-4: LGTM!Updated description accurately reflects the tool's purpose: seeding hosts and software without requiring real hosts or osquery-perf.
10-10: LGTM!Marking the CSV review step as "Optional" is appropriate and clarifies usage.
24-26: LGTM!The idempotent behavior note accurately describes the tool's functionality and clarifies that removed CSV entries are not deleted.
server/service/hosts.go (7)
2134-2142: LGTM!Function signature reformatted to multi-line for readability; no semantic changes to parameters or return types.
2181-2193: LGTM!Loading all OS versions unpaged is necessary for sorting and counting before pagination, which is the correct approach for this optimization.
2195-2204: LGTM!Sorting by
hosts_countwith default descending order preserves existing behavior while supporting explicit ascending sort when requested.
2206-2207: LGTM!Total count is correctly calculated before pagination, ensuring the API returns the full count of matching OS versions.
2209-2211: LGTM!Pagination is correctly applied before loading vulnerabilities, which is the core optimization that reduces response time by limiting vulnerability queries to the current page.
2213-2243: LGTM!Vulnerability loading is correctly restricted to the paginated slice, which is the critical optimization that improves response time from 52s to 3s (as noted in PR objectives). The implementation properly generates CPEs for Darwin, maps vulnerabilities, and initializes collections.
2249-2271: LGTM!The
paginateOSVersionshelper correctly implements slice-based pagination with proper edge-case handling (empty results, last page, no pagination).
|
|
||
| ```bash | ||
| go run ./tools/seed_data/seed_vuln_data.go | ||
| go run ./tools/seed_data/seed_vuln_data.go --ubuntu 1 --macos 1 --windows 1 --linux-kernels 1 |
There was a problem hiding this comment.
Incorrect file path in command example.
The command references ./tools/seed_data/seed_vuln_data.go, but based on the file structure, the correct path should be ./tools/software/vulnerabilities/seed_vuln_data.go.
Apply this diff to fix the path:
-go run ./tools/seed_data/seed_vuln_data.go --ubuntu 1 --macos 1 --windows 1 --linux-kernels 1
+go run ./tools/software/vulnerabilities/seed_vuln_data.go --ubuntu 1 --macos 1 --windows 1 --linux-kernels 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go run ./tools/seed_data/seed_vuln_data.go --ubuntu 1 --macos 1 --windows 1 --linux-kernels 1 | |
| go run ./tools/software/vulnerabilities/seed_vuln_data.go --ubuntu 1 --macos 1 --windows 1 --linux-kernels 1 |
🤖 Prompt for AI Agents
In tools/software/vulnerabilities/README.md around line 21, the example command
uses an incorrect path ./tools/seed_data/seed_vuln_data.go; update the command
to reference the correct file path
./tools/software/vulnerabilities/seed_vuln_data.go so the example runs against
the actual script in this directory.
| var ( | ||
| // MySQL config | ||
| mysqlAddr = "localhost:3306" | ||
| mysqlUser = "fleet" | ||
| mysqlPass = "insecure" | ||
| mysqlDB = "fleet" | ||
|
|
||
| type HostSoftware struct { | ||
| HostID int64 `db:"host_id"` | ||
| SoftwareID int64 `db:"software_id"` | ||
| } | ||
| // CSV paths | ||
| macCSVPath = "./tools/software/vulnerabilities/software-macos.csv" | ||
| winCSVPath = "./tools/software/vulnerabilities/software-win.csv" | ||
| ) |
There was a problem hiding this comment.
Remove unused global variables.
These package-level variables (mysqlAddr, mysqlUser, mysqlPass, mysqlDB, macCSVPath, winCSVPath) are declared but never referenced. The new implementation uses CLI flags and hardcoded paths within main(). Clean up by removing these unused globals.
🤖 Prompt for AI Agents
In tools/software/vulnerabilities/seed_vuln_data.go around lines 21 to 31 the
package declares unused global variables (mysqlAddr, mysqlUser, mysqlPass,
mysqlDB, macCSVPath, winCSVPath) that are not referenced because the program now
uses CLI flags and local paths in main; remove these unused globals from the
file to clean up dead code and ensure no unused-variable compile warnings
remain, leaving any necessary configuration handled via flags or local variables
in main.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good overall. I made a couple nit comments and I see Code Rabbit made some as well.
I did not do a detail review of the tool update.
Also, we should change the default for osquery-perf to randomize between at least 50 OS versions. (800 would be nice, but I don't how easy/maintainable that is) That way our load test would be closer to customer environment. But that can be done in a later PR.
| count = len(osVersions.OSVersions) | ||
|
|
||
| // Paginate first | ||
| var meta *fleet.PaginationMetadata |
There was a problem hiding this comment.
Nit. Can this line be removed since it is initialized below?
|
|
||
| // Pull vulnerabilities ONLY for the paginated slice, as the full list slows | ||
| // response times down significantly with many CVEs. | ||
| if len(paged) > 0 { |
There was a problem hiding this comment.
Nit. If len(paged) == 0, return right away. This is a general Go best practice to return early for special conditions. This will eliminate the indentation of the code below.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33691 +/- ##
=======================================
Coverage 64.00% 64.01%
=======================================
Files 2067 2067
Lines 207195 207214 +19
Branches 6725 6725
=======================================
+ Hits 132625 132642 +17
- Misses 64140 64143 +3
+ Partials 10430 10429 -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:
|
3 participants
Related issue: Resolves #33494
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests (note: skipped benchmark testing here, no functional changes)
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Performance
Documentation
Tools