Ingest, store, consider in unique_identifier, and serve upgrade_codes for Windows software
#34786
Conversation
jacobshandling
had a problem deploying
to
Docker Hub
GitHub Actions
Error
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #34786 +/- ##
==========================================
+ Coverage 66.21% 67.86% +1.64%
==========================================
Files 2073 1346 -727
Lines 174780 153326 -21454
Branches 7194 0 -7194
==========================================
- Hits 115738 104054 -11684
+ Misses 48409 38676 -9733
+ Partials 10633 10596 -37
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:
|
jacobshandling
force-pushed
the
33907-windows-sw-upgrade-code
branch
from
2a5b10b to
59844f2
Compare
jacobshandling
had a problem deploying
to
Docker Hub
GitHub Actions
Error
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
force-pushed
the
33907-windows-sw-upgrade-code
branch
from
4787500 to
6d0f14b
Compare
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
had a problem deploying
to
Docker Hub
GitHub Actions
Error
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR implements Windows UpgradeCode collection for the Fleet software inventory system. It adds the upgrade_code field to data models, database schema, and queries. A migration adds upgrade_code columns to software and software_titles tables, updates unique_identifier generation logic, and introduces validation for upgrade codes from Windows programs source. Changes
Sequence DiagramsequenceDiagram
participant osquery
participant osquery_utils
participant SoftwareFromOsqueryRow as SoftwareFromOsqueryRow<br/>(fleet)
participant Database as MySQL Database
participant API as API Response
osquery->>osquery_utils: Windows software query<br/>(includes upgrade_code)
osquery_utils->>osquery_utils: Extract row["upgrade_code"]
osquery_utils->>SoftwareFromOsqueryRow: SoftwareFromOsqueryRow(..., upgradeCode)
rect rgb(200, 220, 255)
Note over SoftwareFromOsqueryRow: Validation Logic
SoftwareFromOsqueryRow->>SoftwareFromOsqueryRow: if source == "programs"<br/>validate upgradeCode length
SoftwareFromOsqueryRow->>SoftwareFromOsqueryRow: Populate UpgradeCode field
SoftwareFromOsqueryRow->>SoftwareFromOsqueryRow: Update unique_identifier
end
SoftwareFromOsqueryRow->>Database: Insert/Update software<br/>with upgrade_code
Database->>Database: Generate unique_identifier<br/>via COALESCE logic
Database->>API: Return software record
API->>API: Include upgrade_code in response<br/>(programs source only)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/fleet/software.go (1)
153-171: Include UpgradeCode in ComputeRawChecksum() or remove it from ToUniqueStr().The checksum column has a UNIQUE constraint (
idx_software_checksumin the database schema), making this inconsistency a data integrity issue—not merely an optimization concern. Since UpgradeCode is included in ToUniqueStr() (lines 153–157) but not in ComputeRawChecksum() (line 171), two software entries could be treated as different by the application yet share the same checksum, violating the database constraint.Resolve by either:
- Adding UpgradeCode to the columns hashed in ComputeRawChecksum(), or
- Removing UpgradeCode from ToUniqueStr() if it shouldn't affect uniqueness.
The TODO at line 171 confirms this was previously flagged but unresolved.
🧹 Nitpick comments (11)
server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware.go (2)
14-31: Prefer VARCHAR(38) (or BINARY(16)) over CHAR(38).CHAR adds padding semantics and fixed storage even for empty-string rows set for 'programs'. VARCHAR(38) avoids padding; BINARY(16) is even denser if you ever normalize GUIDs to 16‑byte form. Functionally equivalent here; consider for future migrations.
-ALTER TABLE software_titles ADD COLUMN upgrade_code CHAR(38) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL +ALTER TABLE software_titles ADD COLUMN upgrade_code VARCHAR(38) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL ... -ALTER TABLE software ADD COLUMN upgrade_code CHAR(38) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL +ALTER TABLE software ADD COLUMN upgrade_code VARCHAR(38) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci DEFAULT NULL
14-21: Optional: consider indexing upgrade_code later if used in lookups.If APIs/datastore will filter by
upgrade_code, a secondary index can help. Not required for this PR but worth tracking.
Would you like a follow-up issue opened to benchmark queries onupgrade_codebefore adding an index?Also applies to: 28-31
server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware_test.go (1)
28-33: Add a small check for software table too.You set defaults on both tables; consider asserting
software.upgrade_codedefault '' mirrors titles.+var swUC *string +err = db.Get(&swUC, `SELECT upgrade_code FROM software WHERE source = 'programs' LIMIT 1`) +require.NoError(t, err) +require.Equal(t, "", *swUC)Also applies to: 63-68
docs/Contributing/product-groups/orchestration/understanding-host-vitals.md (1)
1066-1070: Replace hard tabs with spaces in code blocks.markdownlint MD010 flagged tabs; swap to spaces to keep checks green.
Also applies to: 1077-1081, 1088-1092, 1099-1103, 1110-1114
server/datastore/mysql/software_test.go (2)
3710-3713: Cover UpgradeCode in checksum tests (programs) once spec is finalized.If checksum/unique_identifier now incorporates Windows MSI UpgradeCode for source="programs", add explicit cases for:
- programs with a non-empty UpgradeCode
- programs with an empty UpgradeCode (treated as “no value”)
This will prevent regressions and prove checksum disambiguation works as intended. Example to enable later:
- // {Name: "foo", Version: "0.0.2", Source: "programs", UpgradeCode: ptr.String("{55ac7218-24cb-4b99-9449-f28d9c59cc7e}")}, - // {Name: "foo", Version: "0.0.2", Source: "programs", UpgradeCode: ptr.String("")}, + {Name: "foo", Version: "0.0.2", Source: "programs", UpgradeCode: ptr.String("{55ac7218-24cb-4b99-9449-f28d9c59cc7e}")}, + {Name: "foo", Version: "0.0.2", Source: "programs", UpgradeCode: ptr.String("")},Please confirm: Is UpgradeCode included in checksum for source="programs" only (and ignored for other sources)?
3727-3727: SELECT should project upgrade_code when validating checksum fetch.When you switch the test to include programs+UpgradeCode, also select upgrade_code so struct comparisons remain accurate:
- `SELECT name, version, source, bundle_identifier, ` + "`release`" + `, arch, vendor, extension_for, extension_id, application_id FROM software WHERE checksum = UNHEX(?)` + `SELECT name, version, source, bundle_identifier, ` + "`release`" + `, arch, vendor, extension_for, extension_id, application_id, upgrade_code FROM software WHERE checksum = UNHEX(?)`server/service/osquery_utils/queries.go (1)
1225-1226: Use COALESCE to ensure NULL upgrade_codes are returned as empty strings.Per the PR objectives, programs entries with no upgrade_code value should return an empty string. The current query may return NULL if the
upgrade_codecolumn is NULL in the database. While osquery might convert NULLs to empty strings when building the result map, it's better to be explicit.Apply this diff:
publisher AS vendor, install_location AS installed_path, - upgrade_code AS upgrade_code + COALESCE(upgrade_code, '') AS upgrade_code FROM programsserver/datastore/mysql/software.go (3)
1509-1509: Document GROUP BY clause decision for upgrade_code.The TODO questions whether
s.upgrade_codeshould be in the GROUP BY clause. Sinceupgrade_codeis included in other GROUP BY clauses (e.g., lines 4812, 4856, 4896), this inconsistency should be resolved or documented.
2478-2478: Clarify struct field naming convention.The TODO questions the naming
SoftwareUpgradeCodevsSoftwareUpgradeCodeList. Since the related field isSoftwareUpgradeCodeList(line 2515), this should be consistent throughout.
2528-2528: Consider including upgrade_code in installed software query.The TODO suggests
upgrade_codemight need to be included in this query for completeness. Evaluate whether this is needed for the Windows software identity verification feature.server/fleet/software.go (1)
572-574: Consider format validation for UpgradeCode GUID.The validation only checks length (38 characters), but the comment at lines 39-40 states UpgradeCode is "a GUID, only uses hexadecimal digits, hyphens, curly braces". Consider adding format validation to ensure the upgrade code matches the expected GUID pattern (e.g.,
{XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX}).This would catch malformed data earlier and provide better error messages.
Example validation:
import "regexp" var upgradeCodePattern = regexp.MustCompile(`^\{[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12}\}$`) if upgradeCode != "" { if len(upgradeCode) != UpgradeCodeExpectedLength { return nil, errors.New("host reported invalid upgrade code - unexpected length") } if !upgradeCodePattern.MatchString(upgradeCode) { return nil, errors.New("host reported invalid upgrade code - invalid GUID format") } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cmd/osquery-perf/windows_11-software.json.bz2is excluded by!**/*.bz2server/datastore/mysql/testdata/select_software_titles_sql_fixture.gzis excluded by!**/*.gz
📒 Files selected for processing (12)
cmd/osquery-perf/agent.go(3 hunks)docs/Contributing/product-groups/orchestration/understanding-host-vitals.md(5 hunks)server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware.go(1 hunks)server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware_test.go(1 hunks)server/datastore/mysql/schema.sql(3 hunks)server/datastore/mysql/software.go(34 hunks)server/datastore/mysql/software_test.go(3 hunks)server/datastore/mysql/software_titles.go(2 hunks)server/datastore/mysql/software_titles_test.go(1 hunks)server/fleet/software.go(9 hunks)server/fleet/software_installer.go(1 hunks)server/service/osquery_utils/queries.go(6 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/fleet/software_installer.goserver/service/osquery_utils/queries.goserver/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware.gocmd/osquery-perf/agent.goserver/datastore/mysql/software_titles.goserver/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware_test.goserver/datastore/mysql/software_titles_test.goserver/fleet/software.goserver/datastore/mysql/software.goserver/datastore/mysql/software_test.go
🧠 Learnings (5)
📚 Learning: 2025-07-08T16:13:39.114Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 30589
File: server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go:53-55
Timestamp: 2025-07-08T16:13:39.114Z
Learning: In the Fleet codebase, Down migration functions are intentionally left empty/no-op. The team does not implement rollback functionality for database migrations, so empty Down_* functions in migration files are correct and should not be flagged as issues.
Applied to files:
server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware.go
📚 Learning: 2025-09-18T21:55:10.359Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 33184
File: server/datastore/mysql/migrations/tables/20250918154557_AddKernelHostCountsIndexForVulnQueries.go:12-27
Timestamp: 2025-09-18T21:55:10.359Z
Learning: In Fleet's codebase, schema.sql is auto-generated from all existing migrations, so there's no risk of migrations creating duplicate database objects. Migrations don't need to be made idempotent to guard against conflicts with schema.sql since they work together as part of the same system.
Applied to files:
server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware.goserver/datastore/mysql/schema.sql
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
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/datastore/mysql/software_titles_test.goserver/datastore/mysql/software_test.go
📚 Learning: 2025-08-01T15:08:16.858Z
Learnt from: sgress454
Repo: fleetdm/fleet PR: 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
📚 Learning: 2025-09-12T13:04:23.777Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 32823
File: server/datastore/mysql/software.go:4457-4471
Timestamp: 2025-09-12T13:04:23.777Z
Learning: In the Fleet codebase, the `status` column in the `host_software_installs` table is defined as `GENERATED ALWAYS`, meaning it's automatically computed by the database based on other field values (like exit codes) and should not be explicitly included in INSERT statements.
Applied to files:
server/datastore/mysql/schema.sqldocs/Contributing/product-groups/orchestration/understanding-host-vitals.md
🪛 markdownlint-cli2 (0.18.1)
docs/Contributing/product-groups/orchestration/understanding-host-vitals.md
1069-1069: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1080-1080: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1091-1091: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1102-1102: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1113-1113: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (19)
server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware.go (2)
33-38: Unique identifier logic looks right.Including
NULLIF(upgrade_code, "")in the COALESCE safeguards empty-string sentinels. Good change.
43-45: No-op Down migration is correct for this repo.Empty Down is consistent with Fleet’s migration policy. (Based on learnings)
cmd/osquery-perf/agent.go (3)
122-128: Add field: OK.Accepts
upgrade_codefrom fixtures; backward-compatible for sources without the field.
2532-2536: Emit upgrade_code only for Windows programs.Matches spec (“only from programs”). Looks good.
138-142: Cannot verify fixture coverage—fixture file not accessible in repository.The code change at lines 138-142 is correct: it properly maps
s.UpgradeCodeto the"upgrade_code"field, and the review comment's note that "sources without values default to empty string" is accurate for Go's string type.However, the suggested sanity check cannot be completed because the fixture file
cmd/osquery-perf/windows_11-software.json.bz2is not accessible in the repository (it may be excluded, generated at build time, or located elsewhere). Without access to the actual fixture data, I cannot verify whether the Windows fixture contains the expectedupgrade_codeentries as the reviewer anticipated.server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware_test.go (2)
48-62: Good coverage of default values.Asserts '' for Windows and NULL for macOS after migration.
66-101: Solid COALESCE/NULLIF assertions.Tests for non-empty, empty, and nil upgrade_code paths are correct and readable.
server/datastore/mysql/software_titles.go (2)
395-406: ListSoftwareTitles now projects upgrade_code.Consistent with schema; no filtering/scope changes introduced.
51-58: No action required—struct field verified.The
SoftwareTitlestruct atserver/fleet/software.go:223correctly includesUpgradeCode *string \json:"upgrade_code,omitempty" db:"upgrade_code"``, ensuring proper sqlx StructScan mapping for the new column.docs/Contributing/product-groups/orchestration/understanding-host-vitals.md (1)
1056-1115: Windows query update matches spec.
upgrade_codereturned for programs; empty for non-programs sources. Looks good.server/datastore/mysql/software_titles_test.go (1)
2219-2221: Call-site correctly updated for SoftwareFromOsqueryRow signature change.Verification confirms all call sites in the repository have been updated to match the 12-parameter signature with
upgradeCodeas the final parameter. No stale call sites remain.server/fleet/software_installer.go (1)
636-637: UpgradeCode field implementation verified—no issues found.The three-case requirement is correctly supported:
- Database migration establishes the foundation: DEFAULT NULL for all rows, then sets empty string for "programs" source, leaving non-programs as NULL.
- SQL queries include
upgrade_codein SELECT clauses (e.g.,server/datastore/mysql/software.go:354).- Go's
database/sqlpackage automatically converts NULL to nil pointers and non-NULL values to pointers.- The struct definition with
*stringandomitemptyhandles all cases correctly: nil (omitted from JSON), empty string (included as""), or actual value (included).The datastore layer properly populates this field based on source, and the implementation is sound.
server/datastore/mysql/software_test.go (1)
9167-9170: LGTM: All call sites correctly updated to new arity.Verification confirms all four call sites to
SoftwareFromOsqueryRowuse the correct 12-parameter arity:
- Test call sites in
software_test.goandsoftware_titles_test.goproperly pass empty strings for new trailing parameters- Production call site in
queries.gopasses the corresponding row values for all 12 parametersNo missed call sites detected.
server/service/osquery_utils/queries.go (2)
1217-1275: LGTM! Clean implementation of upgrade_code for Windows software sources.The UNION query correctly adds the
upgrade_codecolumn to all sources with consistent column ordering. Non-programs sources appropriately return empty strings forupgrade_code, ensuring only the programs source can have actual upgrade_code values.
1969-1969: LGTM! Correct propagation of upgrade_code to the software ingestion path.The
upgrade_codeparameter is properly passed toSoftwareFromOsqueryRow, completing the data flow from the Windows software query to the software model.server/datastore/mysql/schema.sql (3)
2408-2408: UpgradeCode column definition looks good.The char(38) type correctly accommodates the Windows MSI UpgradeCode format (GUID with braces), and collation is consistent with schema standards.
2592-2593: GENERATED ALWAYS formula correctly handles upgrade_code logic.The updated
unique_identifiercolumn usesCOALESCE(bundle_identifier, application_id, NULLIF(upgrade_code, ''), name), which properly handles the requirement to treat empty upgrade_code values as NULL. This ensures upgrade_code is only used as a differentiator when non-empty.
1641-1641: Migration tracking updated correctly.The AUTO_INCREMENT for
migration_status_tablesis correctly incremented to 435, and the new migration entry (20251028163158) is properly included in the INSERT statement.Also applies to: 1643-1643
server/fleet/software.go (1)
571-577: No changes needed; UpgradeCode restriction to "programs" is correct.The code properly restricts
UpgradeCodeto the "programs" source only. This is intentional and correct because upgrade codes are exclusive to Windows MSI (Microsoft Installer) packages, which only appear in the "programs" source. Other Windows software sources in the codebase—such as "ie_extensions", "chrome_extensions", "firefox_addons", and "chocolatey_packages"—are explicitly queried with empty upgrade codes in the osquery software inventory query, confirming they do not support upgrade codes.
server/datastore/mysql/migrations/tables/20251028163158_AddWindowsUpgradeCodeToSoftware.go
Show resolved
Hide resolved
jacobshandling
force-pushed
the
33907-windows-sw-upgrade-code
branch
from
2449384 to
e39aa5b
Compare
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
jacobshandling
requested review from
a team,
sgress454 and
sharon-fdm
as code owners
jacobshandling
changed the title
upgrade_codes for Windows software
jacobshandling
temporarily deployed
to
Docker Hub
GitHub Actions
Inactive
Related issue: Resolves #33907
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Database migrations
- [ ] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.N/ACOLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
Release Notes
New Features
Chores