31214 linux vulns optimization - #31722
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 30738-linux-vulns #31722 +/- ##
====================================================
Coverage ? 63.76%
====================================================
Files ? 1963
Lines ? 191662
Branches ? 6311
====================================================
Hits ? 122218
Misses ? 59888
Partials ? 9556
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:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Command as vuln_process command
participant DS as Datastore
participant DB as Database
Command->>DS: InsertKernelSoftwareMapping(ctx)
DS->>DB: INSERT INTO kernels SELECT ... (mapping kernel software to OS versions)
DB-->>DS: Result (success/error)
DS-->>Command: error (if any)
sequenceDiagram
participant Service as Service Layer
participant DS as Datastore
participant DB as Database
Service->>DS: ListVulnsByOsNameAndVersion(...)
DS->>DB: SELECT ... FROM kernels JOIN ...
DB-->>DS: Vulnerability rows
DS-->>Service: Vulnerability list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
server/fleet/datastore.go (2)
1107-1108: Add brief GoDoc to clarify semantics (idempotency, scope, when to run).A short comment here helps future maintainers understand what the mapping does, whether it’s safe to run repeatedly, and if it wraps a transaction.
Example:
// InsertKernelSoftwareMapping populates/refreshes the kernels mapping table by // upserting kernel↔OS associations discovered from software/OS data. // It is intended to be idempotent and safe to run multiple times.
1107-1108: Consider plural naming and returning rows affected for consistency with existing insert/upsert APIs.Other datastore methods that insert many rows typically use plural naming and return a count (e.g., InsertOSVulnerabilities(ctx, ...) (int64, error)). Returning a count here would improve observability, tests, and logging.
Proposed interface change:
- InsertKernelSoftwareMapping(ctx context.Context) error + // InsertKernelSoftwareMappings upserts kernel↔OS mapping rows and returns the number of affected rows. + InsertKernelSoftwareMappings(ctx context.Context) (int64, error)If you prefer to keep the existing name/signature in this PR, consider at least returning/logging counts from the implementation and we can revisit the signature in a follow-up.
server/service/integration_enterprise_vulns_test.go (1)
129-132: Replace debugging dump with an assertion.DumpTable adds noisy logs and no guarantees. Replace it with a targeted assertion to validate mappings exist for this OS version.
Apply this diff:
- mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { - mysql.DumpTable(t, q, "kernels") - return nil - }) + mysql.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error { + var cnt int + if err := sqlx.GetContext(ctx, q, &cnt, `SELECT COUNT(*) FROM kernels WHERE os_version_id = ?`, osinfo.OSVersionID); err != nil { + return err + } + require.Equal(t, len(tt.software), cnt, "unexpected kernels mapping count for os_version_id=%d", osinfo.OSVersionID) + return nil + })server/datastore/mysql/schema.sql (1)
1035-1035: Clarify os_version_id naming (it maps to operating_systems.id)The name
os_version_idis misleading given the referenced table isoperating_systems. Consider renaming tooperating_system_idfor consistency, or at minimum keep the explicit FK (proposed above) so intent is clear.If you choose to rename, remember to update:
- the migration creating this table,
- datastore methods (e.g., InsertKernelSoftwareMapping),
- any SQL that reads/writes this column.
server/datastore/mysql/operating_system_vulnerabilities_test.go (2)
503-506: Prepare an expected count to enforce exact-match assertionsYou build an expected set but never compute its size. Capture count for consistent checks below.
-expectedSet := make(map[string]struct{}) +expectedSet := make(map[string]struct{}) for _, v := range tt.vulns { expectedSet[v.CVE] = struct{}{} } +expectedCount := len(expectedSet)
521-525: Use deduped expected count for the withMeta=true caseAlign the length assertion with the deduped set to avoid accidental failures if tt.vulns ever contains duplicates.
-cves, err = ds.ListVulnsByOsNameAndVersion(ctx, os.Name, os.Version, true) +cves, err = ds.ListVulnsByOsNameAndVersion(ctx, os.Name, os.Version, true) require.NoError(t, err) -require.Len(t, cves, len(tt.vulns)) +require.Len(t, cves, expectedCount) for _, g := range cves { _, ok := expectedSet[g.CVE] assert.True(t, ok) }cmd/fleet/vuln_process.go (1)
192-197: Optional naming consistencyMost entries use a “cron_” prefix; consider renaming to cron_insert_kernel_software_mapping for consistency.
server/mock/datastore_mock.go (2)
796-797: Add brief GoDoc for exported mock func typeTo satisfy linters and improve discoverability, add a short comment for this exported type.
+// InsertKernelSoftwareMappingFunc allows tests to stub Datastore.InsertKernelSoftwareMapping behavior. type InsertKernelSoftwareMappingFunc func(ctx context.Context) error
6265-6270: Avoid potential race by copying func under the lock before callingMinor concurrency polish: copy the function pointer while holding the lock, then call the local variable after unlocking. This prevents a rare race if another goroutine mutates the mock func between unlock and call.
-func (s *DataStore) InsertKernelSoftwareMapping(ctx context.Context) error { - s.mu.Lock() - s.InsertKernelSoftwareMappingFuncInvoked = true - s.mu.Unlock() - return s.InsertKernelSoftwareMappingFunc(ctx) -} +func (s *DataStore) InsertKernelSoftwareMapping(ctx context.Context) error { + s.mu.Lock() + s.InsertKernelSoftwareMappingFuncInvoked = true + fn := s.InsertKernelSoftwareMappingFunc + s.mu.Unlock() + return fn(ctx) +}If the repo prefers explicit guardrails, consider a nil check on fn with a clear panic message to align with other mocks’ style.
server/datastore/mysql/operating_system_vulnerabilities.go (1)
274-293: Consider indexing & batching for InsertKernelSoftwareMapping
INSERT IGNORE … SELECT DISTINCT …over five joined tables will lock/scan large datasets on every run. On fleets with >100 k hosts it quickly becomes a hotspot.Recommendations:
- Add a covering composite index on
(software_title_id, software_id, os_version_id)inkernels.- Run the insertion in smaller batches (e.g. by host-software chunk or time window) or behind a
INSERT … ON DUPLICATE KEYwith primary key = the same three columns to avoid full-table scans.Not urgent for correctness but worth addressing before this code hits prod.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
cmd/osquery-perf/ubuntu_2204-software.json.bz2is excluded by!**/*.bz2
📒 Files selected for processing (8)
cmd/fleet/vuln_process.go(2 hunks)server/datastore/mysql/migrations/tables/20250807141218_SoftwareIsKernelColumn.go(1 hunks)server/datastore/mysql/operating_system_vulnerabilities.go(5 hunks)server/datastore/mysql/operating_system_vulnerabilities_test.go(3 hunks)server/datastore/mysql/schema.sql(1 hunks)server/fleet/datastore.go(1 hunks)server/mock/datastore_mock.go(3 hunks)server/service/integration_enterprise_vulns_test.go(1 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/datastore/mysql/migrations/tables/20250807141218_SoftwareIsKernelColumn.goserver/service/integration_enterprise_vulns_test.gocmd/fleet/vuln_process.goserver/fleet/datastore.goserver/datastore/mysql/operating_system_vulnerabilities_test.goserver/datastore/mysql/operating_system_vulnerabilities.goserver/mock/datastore_mock.go
🧠 Learnings (2)
📚 Learning: 2025-08-08T07:40:05.274Z
Learnt from: getvictor
PR: fleetdm/fleet#31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.274Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/service/integration_enterprise_vulns_test.go
📚 Learning: 2025-08-01T15:08:16.858Z
Learnt from: sgress454
PR: fleetdm/fleet#31508
File: server/datastore/mysql/schema.sql:102-116
Timestamp: 2025-08-01T15:08:16.858Z
Learning: The schema.sql file in server/datastore/mysql/ is auto-generated from migrations for use with tests, so it cannot be manually edited. Any changes must be made through migrations.
Applied to files:
server/datastore/mysql/schema.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: test-packaging (ubuntu-latest, local)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: publish
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: lint (windows-latest)
- GitHub Check: lint (macos-latest)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fast, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: build-binaries
🔇 Additional comments (4)
server/service/integration_enterprise_vulns_test.go (1)
127-127: Good placement; ensure idempotency and observable effect.Calling InsertKernelSoftwareMapping after SyncHostsSoftwareTitles is the right spot. Please confirm the method is idempotent (no duplicate rows on repeated runs) and consider asserting expected mappings instead of relying on logs.
Do you want me to add an assertion that verifies the mapping count per os_version_id equals len(tt.software)?
server/datastore/mysql/schema.sql (1)
1031-1038: Update thekernelsmigration and regenerateschema.sqlThe
kernelstable is defined in the Up_20250807141218 migration (not by hand in schema.sql). Please update that migration to:• Use unsigned types that match the referenced tables:
id→int unsignedsoftware_title_id→int unsignedsoftware_id→bigint unsignedos_version_id→int unsigned NOT NULL• Add indexes on the three ID columns for join performance:
KEY idx_kernels_software_title_id (software_title_id)KEY idx_kernels_software_id (software_id)KEY idx_kernels_os_version_id (os_version_id)• Add foreign-key constraints to enforce referential integrity:
FOREIGN KEY (software_title_id)→software_titles(id)ON DELETE SET NULLFOREIGN KEY (software_id)→software(id)ON DELETE SET NULLFOREIGN KEY (os_version_id)→operating_systems(id)ON DELETE CASCADE• Enforce that exactly one of
software_title_idorsoftware_idis set:CONSTRAINT ck_kernels_one_parent CHECK ( (software_title_id IS NOT NULL AND software_id IS NULL) OR (software_title_id IS NULL AND software_id IS NOT NULL) )After applying those changes in
server/datastore/mysql/migrations/tables/20250807141218_SoftwareIsKernelColumn.go, re-run the migration generator to updateserver/datastore/mysql/schema.sql.— Example diff inside
Up_20250807141218’stx.Exec(...)SQL block:CREATE TABLE kernels ( - id int NOT NULL AUTO_INCREMENT, - software_title_id int DEFAULT NULL, - software_id int DEFAULT NULL, - os_version_id int DEFAULT NULL, + id int unsigned NOT NULL AUTO_INCREMENT, + software_title_id int unsigned DEFAULT NULL, + software_id bigint unsigned DEFAULT NULL, + os_version_id int unsigned NOT NULL, PRIMARY KEY (id), + KEY idx_kernels_software_title_id (software_title_id), + KEY idx_kernels_software_id (software_id), + KEY idx_kernels_os_version_id (os_version_id), + CONSTRAINT fk_kernels_software_title_id FOREIGN KEY (software_title_id) REFERENCES software_titles (id) ON DELETE SET NULL, + CONSTRAINT fk_kernels_software_id FOREIGN KEY (software_id) REFERENCES software (id) ON DELETE SET NULL, + CONSTRAINT fk_kernels_os_version_id FOREIGN KEY (os_version_id) REFERENCES operating_systems (id) ON DELETE CASCADE, + CONSTRAINT ck_kernels_one_parent CHECK ( + (software_title_id IS NOT NULL AND software_id IS NULL) + OR (software_title_id IS NULL AND software_id IS NOT NULL) + ) ) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_cifix_required
cmd/fleet/vuln_process.go (1)
32-36: Docstring whitespace cleanupNo functional changes. OK.
server/mock/datastore_mock.go (1)
2598-2600: Struct additions look consistent; ensure reset helpers include the new flagNaming and pattern match existing fields (e.g., ListKernelsByOSFuncInvoked). If this mock has a reset/cleanup helper, include InsertKernelSoftwareMappingFuncInvoked in it to avoid cross-test leakage.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
Flushing pending comments again as the only file I have left to review is tests.
| } | ||
|
|
||
| statsStmt := ` | ||
| INSERT IGNORE INTO kernel_host_counts (software_title_id, software_id, os_version_id, hosts_count, team_id) |
There was a problem hiding this comment.
This needs to be an ON DUPLICATE KEY UPDATE (and we'll want to add a test that fails if it isn't).
Also, we'll need to loadtest this since it's shaped completely differently than the query we had before.
iansltx
left a comment
iansltx
left a comment
There was a problem hiding this comment.
I think we're good here.
Still needs to be data volume tested IIRC but that can be done on the feature branch.
- **linux vulns API changes (#31490)** - **31214 linux vulns optimization (#31722)** # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually ## Database migrations - [x] Checked table schema to confirm autoupdate - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`).
- **linux vulns API changes (#31490)** - **31214 linux vulns optimization (#31722)** # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements) ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually ## Database migrations - [x] Checked table schema to confirm autoupdate - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`).
2 participants
Checklist for submitter
If some of the following don't apply, delete the relevant line.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Database migrations
Summary by CodeRabbit