Optimize software title reconciliation in vulnerabilities job #34146
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #34146 +/- ##
==========================================
+ Coverage 63.81% 64.21% +0.40%
==========================================
Files 1876 2058 +182
Lines 185510 206819 +21309
Branches 6767 6767
==========================================
+ Hits 118383 132814 +14431
- Misses 57465 63577 +6112
- Partials 9662 10428 +766
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:
|
| INSERT INTO software_titles (name, source, extension_for, bundle_identifier) | ||
| SELECT | ||
| name, | ||
| source, | ||
| extension_for, | ||
| bundle_identifier | ||
| FROM ( | ||
| SELECT DISTINCT | ||
| name, | ||
| source, | ||
| extension_for, | ||
| bundle_identifier | ||
| FROM | ||
| software s | ||
| WHERE | ||
| NOT EXISTS ( | ||
| SELECT 1 FROM software_titles st | ||
| WHERE s.bundle_identifier = st.bundle_identifier AND | ||
| IF(s.source IN ('apps', 'ios_apps', 'ipados_apps'), s.source = st.source, 1) | ||
| ) | ||
| AND COALESCE(bundle_identifier, '') != '' | ||
| UNION ALL | ||
| SELECT DISTINCT | ||
| name, | ||
| source, | ||
| extension_for, | ||
| NULL as bundle_identifier | ||
| FROM | ||
| software s | ||
| WHERE | ||
| NOT EXISTS ( | ||
| SELECT 1 FROM software_titles st | ||
| WHERE (s.name, s.source, s.extension_for) = (st.name, st.source, st.extension_for) | ||
| ) | ||
| AND COALESCE(s.bundle_identifier, '') = '' | ||
| ) as combined_results | ||
| ON DUPLICATE KEY UPDATE | ||
| software_titles.name = software_titles.name, | ||
| software_titles.source = software_titles.source, | ||
| software_titles.extension_for = software_titles.extension_for, | ||
| software_titles.bundle_identifier = software_titles.bundle_identifier |
There was a problem hiding this comment.
This is not necessary with recent changes in software ingestion, which pre-inserts software_titles before inserting software (and makes sure all software entries have a title_id set).
| UPDATE software s | ||
| JOIN software_titles st | ||
| ON COALESCE(s.bundle_identifier, '') = '' AND s.name = st.name AND s.source = st.source AND s.extension_for = st.extension_for | ||
| SET s.title_id = st.id | ||
| WHERE (s.title_id IS NULL OR s.title_id != st.id) | ||
| AND COALESCE(s.bundle_identifier, '') = ''; | ||
| ` | ||
| func (ds *Datastore) cleanupOrphanedSoftwareTitles(ctx context.Context) error { | ||
| var n int64 | ||
| defer func(start time.Time) { | ||
| level.Debug(ds.logger).Log( | ||
| "msg", "cleanup orphaned software titles", | ||
| "rows_affected", n, | ||
| "took", start, | ||
| ) | ||
| }(time.Now()) | ||
|
|
||
| res, err = tx.ExecContext(ctx, updateSoftwareWithoutIdentifierStmt) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "update software title_id without bundle identifier") | ||
| } | ||
| n, _ = res.RowsAffected() | ||
| level.Debug(ds.logger).Log("msg", "update software title_id without bundle identifier", "rows_affected", n) | ||
|
|
||
| updateSoftwareWithIdentifierStmt := ` | ||
| UPDATE software s | ||
| JOIN software_titles st | ||
| ON s.bundle_identifier = st.bundle_identifier AND | ||
| IF(s.source IN ('apps', 'ios_apps', 'ipados_apps'), s.source = st.source, 1) | ||
| SET s.title_id = st.id | ||
| WHERE s.title_id IS NULL | ||
| OR s.title_id != st.id; |
There was a problem hiding this comment.
This is not necessary with recent changes in software ingestion, which pre-inserts software_titles before inserting software (and makes sure all software entries have a title_id set).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughRefactors software title reconciliation to a two-step process (cleanup orphaned titles, then update names), adjusts vulnerability cron function factory to not capture context, and updates the usage statistics schedule signature and its call site. Tests updated to reflect new reconciliation and fix a test name typo. Adds a change note. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Cron as Cron Scheduler
participant VF as getVulnFuncs
participant VJ as VulnFunc(ctx)
participant VP as cronVulnerabilities
participant DS as Datastore (MySQL)
Cron->>VF: Build vuln funcs (no ctx)
Cron->>VJ: Invoke with ctx
VJ->>VP: Run vulnerability processing
VP->>DS: ReconcileSoftwareTitles(ctx)
activate DS
DS->>DS: cleanupOrphanedSoftwareTitles(ctx)
DS->>DS: updateSoftwareTitleNames(ctx)
deactivate DS
VP-->>Cron: Complete
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 (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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
changes/34055-optimize-reconcile-software-titles(1 hunks)cmd/fleet/cron.go(2 hunks)cmd/fleet/serve.go(1 hunks)cmd/fleet/vuln_process.go(2 hunks)server/datastore/mysql/software.go(1 hunks)server/datastore/mysql/software_test.go(3 hunks)server/service/integration_mdm_test.go(0 hunks)
💤 Files with no reviewable changes (1)
- server/service/integration_mdm_test.go
🧰 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:
cmd/fleet/serve.gocmd/fleet/cron.gocmd/fleet/vuln_process.goserver/datastore/mysql/software.goserver/datastore/mysql/software_test.go
🔇 Additional comments (5)
cmd/fleet/serve.go (1)
982-982: LGTM! Call site correctly updated.The license parameter removal aligns with the updated signature in
cmd/fleet/cron.go(line 1146), where the license is now retrieved from the context instead of being passed as a parameter.cmd/fleet/vuln_process.go (2)
115-115: LGTM! Invocation updated to match new signature.The context parameter removal aligns with the factory signature change at line 162. The VulnFunc closures still receive context when invoked, preserving functionality.
162-203: LGTM! Clean refactoring to decouple factory from context.Removing the context parameter from the factory function signature is a good design choice. The VulnFunc closures (lines 166, 172, 178, 184, 190, 196) still receive context when invoked, so there's no loss of functionality. This decouples the factory from requiring an active context while preserving context usage inside the vulnerability functions.
cmd/fleet/cron.go (2)
68-68: LGTM! Invocation updated to match new signature.The context parameter removal aligns with the factory signature change in
cmd/fleet/vuln_process.go(line 162). All call sites have been consistently updated.
1146-1165: LGTM! Removed redundant license parameter.The license parameter removal is appropriate since the license is available in the context and retrieved where needed (line 1174 in
trySendStatistics). The call site incmd/fleet/serve.go(line 982) has been correctly updated to match this new signature.
…s job Back ported: #34146 to be compatible with the 4.75.0 release. Original ticket #34055 --------- Co-authored-by: Lucas Manuel Rodriguez <[email protected]>
2 participants
Resolves #34055
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Summary by CodeRabbit