Extend update software installer API to support FMA version pinning - #47808
Conversation
…and drop stray TODO
… fallback unchanged
…ndpoint caret takes latest
Handle a version-only PATCH as its own switch case (flip active + record pin + cancel pending installs) rather than routing it through the default SaveInstallerUpdates path, which redundantly rewrote the now-inactive installer. Also return 400 instead of 500 when the pinned version is a bare caret "^".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/38504-auto-update-pin-rollback-fma #47808 +/- ##
=========================================================================
Coverage 67.23% 67.23%
=========================================================================
Files 3634 3634
Lines 229815 229944 +129
Branches 11967 11967
=========================================================================
+ Hits 154517 154608 +91
- Misses 61423 61443 +20
- Partials 13875 13893 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| Message: `Couldn't update. "version" can't be changed at the same time as other fields.`, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Unfortunately I couldn't extract much code out of softwareInstallerPayloadFromSlug since that controls whether a new FMA manifest should be downloaded, and in this code we want to compare existing cached versions.
The auto update cron would probably be able to reuse softwareInstallerPayloadFromSlug though
| WHERE software_installer_id IN ( | ||
| SELECT id FROM software_installers | ||
| WHERE global_or_team_id = ? AND title_id = ? AND fleet_maintained_app_id IS NULL | ||
| WHERE global_or_team_id = ? AND title_id = ? AND id != ? |
There was a problem hiding this comment.
This isn't strictly related to the subtask, but seemed like a bug. I assume we want to repoint any policies to the active installer for the title, and not just custom packages.
| } | ||
|
|
||
| if byVersion { | ||
| // sort by semantic version |
There was a problem hiding this comment.
Not strictly related, but sorting by the version string isn't sufficient for versions. For example if versions 1.2 and 1.12 were cached, it would return 1.12 as older than 1.2 before.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThis PR adds Fleet-maintained app (FMA) version pinning. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.43.0)server/service/integration_enterprise_test.goThanks 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/server/service/software_installers.go (1)
2538-2557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve caret pins against matching cached majors before falling back.
When the latest manifest major differs, this path always hydrates
versions[0]; if the requested major exists later in the semver-ordered cache, GitOps can pin^147but activate a newer major. Also, same-file logic notes some FMAs useVersion == "latest"until package extraction, so treat that as a mismatch instead of failing before cached-version fallback.Proposed fix
if usesCaret { - matches, err := versionMatchesMajor(app.Version, majorVersionString) - if err != nil { - return ctxerr.Wrap(ctx, err, "comparing Fleet-maintained app major version") + var matches bool + if app.Version != "latest" { + matches, err = versionMatchesMajor(app.Version, majorVersionString) + if err != nil { + return ctxerr.Wrap(ctx, err, "comparing Fleet-maintained app major version") + } } if !matches { // We cannot use the FMA we just got the manifest for since it is on a different major // version, so we try to find the latest cached version and use that instead. if app.TitleID == nil { return fleet.NewUserMessageError(errMajorVersionNotFound, http.StatusNotFound) } versions, err := svc.ds.GetFleetMaintainedVersionsByTitleID(ctx, teamID, *app.TitleID, true) if err != nil { return fleet.NewUserMessageError(errMajorVersionNotFound, http.StatusNotFound) } + if len(versions) == 0 { + return fleet.NewUserMessageError(errMajorVersionNotFound, http.StatusNotFound) + } + + cachedVersion := versions[0].Version + for _, v := range versions { + matches, err := versionMatchesMajor(v.Version, majorVersionString) + if err != nil { + return ctxerr.Wrap(ctx, err, "comparing Fleet-maintained app major version") + } + if matches { + cachedVersion = v.Version + break + } + } // This is a bit inefficient as we are duplicating strings for categories and install/uninstall scripts, // but it can be optimized in softwareBatchUpload if it accepted only passing category and script content IDs. - installer, err := svc.ds.GetCachedFMAInstallerMetadata(ctx, teamID, app.ID, versions[0].Version) + installer, err := svc.ds.GetCachedFMAInstallerMetadata(ctx, teamID, app.ID, cachedVersion)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ee/server/service/software_installers.go` around lines 2538 - 2557, The fallback logic when major version mismatch occurs currently always uses versions[0] without checking if a matching major version exists elsewhere in the cached versions slice. Modify the code after calling GetFleetMaintainedVersionsByTitleID to iterate through the returned versions slice and find the first version that matches the requested majorVersionString (using versionMatchesMajor for comparison). Additionally, handle the special case where app.Version equals "latest" by treating it as a mismatch before attempting the cached fallback. Once a matching major version is found in the cache, pass that version string to GetCachedFMAInstallerMetadata instead of hardcoding versions[0].Version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ee/server/service/software_installers.go`:
- Around line 722-733: The active installer flip and pin row modifications are
executed as separate datastore calls and can fail partially. Refactor the code
to wrap the three operations (SetFleetMaintainedAppActiveInstaller,
DeletePinnedVersion, and SetPinnedVersion) into a single atomic datastore
transaction. Create a new datastore method that accepts the necessary parameters
and performs all three operations within one transaction, ensuring that either
all operations succeed together or all are rolled back if any fails.
- Line 648: The error message in the Message field at the specified location in
software_installers.go for the non-FMA version validation does not match the API
contract standard, which can cause API client tests to fail. Locate the correct
standardized error message for non-FMA titles when version is specified (this
contract should be defined elsewhere in the codebase or API specifications) and
replace the current message string with the correct one to ensure consistency
with the API contract and prevent client assertion failures.
In `@server/datastore/mysql/software_installers.go`:
- Around line 4006-4015: The GetPinnedVersion function wraps all errors
unconditionally with ctxerr.Wrap, including sql.ErrNoRows, which prevents the
caller from detecting the not-found case using errors.Is. Check if the error
returned from sqlx.GetContext is sql.ErrNoRows immediately after the call, and
if so, return (nil, nil) to indicate no pinned version exists. Only wrap
non-ErrNoRows errors with ctxerr.Wrap to maintain the codebase pattern for
not-found detection.
---
Outside diff comments:
In `@ee/server/service/software_installers.go`:
- Around line 2538-2557: The fallback logic when major version mismatch occurs
currently always uses versions[0] without checking if a matching major version
exists elsewhere in the cached versions slice. Modify the code after calling
GetFleetMaintainedVersionsByTitleID to iterate through the returned versions
slice and find the first version that matches the requested majorVersionString
(using versionMatchesMajor for comparison). Additionally, handle the special
case where app.Version equals "latest" by treating it as a mismatch before
attempting the cached fallback. Once a matching major version is found in the
cache, pass that version string to GetCachedFMAInstallerMetadata instead of
hardcoding versions[0].Version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d80b03eb-efe5-49e5-af94-b4d4217432b4
📒 Files selected for processing (12)
ee/server/service/software_installers.goee/server/service/software_installers_test.goserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/datastore/mysql/software_titles.goserver/datastore/mysql/teams_test.goserver/fleet/datastore.goserver/fleet/software_installer.goserver/mock/datastore_mock.goserver/service/integration_enterprise_test.goserver/service/software_installers.goserver/service/software_titles.go
| if payload.PinnedVersion != nil { | ||
| if existingInstaller.FleetMaintainedAppID == nil { | ||
| return nil, &fleet.BadRequestError{ | ||
| Message: `Couldn't update. "version" can be only specified for a software title that has a Fleet-maintained app.`, |
There was a problem hiding this comment.
Match the non-FMA version error contract.
The requirement calls for a specific 400 response message for non-FMA titles; this wording currently differs and can break API tests/clients that assert the contract.
Proposed fix
- Message: `Couldn't update. "version" can be only specified for a software title that has a Fleet-maintained app.`,
+ Message: `Couldn't update. 'version' can be specified for a software title that have a Fleet-maintained app.`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/server/service/software_installers.go` at line 648, The error message in
the Message field at the specified location in software_installers.go for the
non-FMA version validation does not match the API contract standard, which can
cause API client tests to fail. Locate the correct standardized error message
for non-FMA titles when version is specified (this contract should be defined
elsewhere in the codebase or API specifications) and replace the current message
string with the correct one to ensure consistency with the API contract and
prevent client assertion failures.
There was a problem hiding this comment.
Just made a small change there on purpose, I can change it back if needed.
| case len(dirty) == 1 && dirty["PinnedVersion"]: // only the pinned version changed; flip the active installer rather than rewriting it | ||
| if err := svc.ds.SetFleetMaintainedAppActiveInstaller(ctx, payload.TeamID, payload.TitleID, *existingInstaller.FleetMaintainedAppID, activeInstallerID); err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "setting active Fleet-maintained app installer") | ||
| } | ||
|
|
||
| if *payload.PinnedVersion == "" { | ||
| if err := svc.ds.DeletePinnedVersion(ctx, payload.TeamID, payload.TitleID); err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "clearing Fleet-maintained app pin") | ||
| } | ||
| } else if err := svc.ds.SetPinnedVersion(ctx, payload.TeamID, payload.TitleID, *payload.PinnedVersion); err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "pinning Fleet-maintained app version") | ||
| } |
There was a problem hiding this comment.
Persist the active-installer flip and pin row atomically.
SetFleetMaintainedAppActiveInstaller can commit before SetPinnedVersion/DeletePinnedVersion fails, leaving policies and is_active pointing at the new installer while GET still reports the old/no pin. Move the active flip and pin-row write/delete into one datastore transaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ee/server/service/software_installers.go` around lines 722 - 733, The active
installer flip and pin row modifications are executed as separate datastore
calls and can fail partially. Refactor the code to wrap the three operations
(SetFleetMaintainedAppActiveInstaller, DeletePinnedVersion, and
SetPinnedVersion) into a single atomic datastore transaction. Create a new
datastore method that accepts the necessary parameters and performs all three
operations within one transaction, ensuring that either all operations succeed
together or all are rolled back if any fails.
| func (ds *Datastore) GetPinnedVersion(ctx context.Context, teamID *uint, titleID uint) (*string, error) { | ||
| var version string | ||
| err := sqlx.GetContext(ctx, ds.reader(ctx), &version, ` | ||
| SELECT pinned_version FROM software_title_team_pins WHERE team_id = ? AND title_id = ? | ||
| `, ptr.ValOrZero(teamID), titleID) | ||
| if err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "get pinned version") | ||
| } | ||
| return &version, nil | ||
| } |
There was a problem hiding this comment.
sql.ErrNoRows is wrapped, breaking caller's not-found detection.
The caller (SoftwareTitleByID per the summary) uses errors.Is(err, sql.ErrNoRows) to detect when no pin exists. However, ctxerr.Wrap wraps all errors unconditionally here. While errors.Is can unwrap error chains, the codebase pattern is to check for sql.ErrNoRows before wrapping to return a typed not-found error or (nil, nil).
Proposed fix
func (ds *Datastore) GetPinnedVersion(ctx context.Context, teamID *uint, titleID uint) (*string, error) {
var version string
err := sqlx.GetContext(ctx, ds.reader(ctx), &version, `
SELECT pinned_version FROM software_title_team_pins WHERE team_id = ? AND title_id = ?
`, ptr.ValOrZero(teamID), titleID)
if err != nil {
+ if errors.Is(err, sql.ErrNoRows) {
+ return nil, sql.ErrNoRows
+ }
return nil, ctxerr.Wrap(ctx, err, "get pinned version")
}
return &version, nil
}📝 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.
| func (ds *Datastore) GetPinnedVersion(ctx context.Context, teamID *uint, titleID uint) (*string, error) { | |
| var version string | |
| err := sqlx.GetContext(ctx, ds.reader(ctx), &version, ` | |
| SELECT pinned_version FROM software_title_team_pins WHERE team_id = ? AND title_id = ? | |
| `, ptr.ValOrZero(teamID), titleID) | |
| if err != nil { | |
| return nil, ctxerr.Wrap(ctx, err, "get pinned version") | |
| } | |
| return &version, nil | |
| } | |
| func (ds *Datastore) GetPinnedVersion(ctx context.Context, teamID *uint, titleID uint) (*string, error) { | |
| var version string | |
| err := sqlx.GetContext(ctx, ds.reader(ctx), &version, ` | |
| SELECT pinned_version FROM software_title_team_pins WHERE team_id = ? AND title_id = ? | |
| `, ptr.ValOrZero(teamID), titleID) | |
| if err != nil { | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return nil, sql.ErrNoRows | |
| } | |
| return nil, ctxerr.Wrap(ctx, err, "get pinned version") | |
| } | |
| return &version, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/software_installers.go` around lines 4006 - 4015, The
GetPinnedVersion function wraps all errors unconditionally with ctxerr.Wrap,
including sql.ErrNoRows, which prevents the caller from detecting the not-found
case using errors.Is. Check if the error returned from sqlx.GetContext is
sql.ErrNoRows immediately after the call, and if so, return (nil, nil) to
indicate no pinned version exists. Only wrap non-ErrNoRows errors with
ctxerr.Wrap to maintain the codebase pattern for not-found detection.
There was a problem hiding this comment.
Leaving as is because if the result is nil we want to return an error. Also seems like a common pattern to wrap it and have the caller check errors.Is(err, sql.ErrNoRows). I guess it would also be possible to do this, but I don't know which pattern is preferred:
if err == sql.ErrNoRows {
return nil, ctxerr.Wrap(ctx, notFound("SoftwareTitlePinnedVersion"), "get software title pinned version")
}
| return &version, nil | ||
| } | ||
|
|
||
| func (ds *Datastore) SetPinnedVersion(ctx context.Context, teamID *uint, titleID uint, version string) error { |
There was a problem hiding this comment.
Technically these set/delete functions are only used by test code now, but I'm leaving them here in case another subtask in this story would need them.
|
@jkatz01 It looks like the inactive Library rows have 'Added X ago', |
|
@cdcme I added the |
|
@jkatz01 that's right 👍 thanks! |
5c3eaca
into
feat/38504-auto-update-pin-rollback-fma
#48293) **Related issue:** Resolves #38504 **Constituent PRs (merged into this feature branch):** - #47682 — Fleet UI: APRF Software title details page Library/Inventory layout - #47808 — Extend update software installer API to support FMA version pinning - #47944 — Fleet UI: APRF library item accordion component - #48081 — Versions modal, multi-row Library, pinned state - #48098 — Add `pinned_version` to `edited_software` activity - #48123 — Auto-update FMA cron - #48144 — Download a newly-published FMA version when pinned to it # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## 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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Fleet-maintained app version pinning (Latest, exact, and major) via a new Versions modal. * Introduced premium auto-updates for maintained apps with pin-aware promotion and rollback-safe caching. * Added expandable library version rows and a Policies modal. * **Bug Fixes** * Improved pin handling, cache/manifest hydration, and safer update behavior on per-app failures and deduplication. * **UI/UX** * Refreshed the Software title details experience with new accordion/list patterns, redesigned details widget/tooltips, and updated installer presentation. * **Documentation** * Expanded Storybook component/page coverage and adjusted Storybook canvas padding. * **Tests** * Added/updated unit and integration tests for pinning, auto-update flows, and new modal/UI behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Related issue: Resolves #46726
Updates the
PATCH /api/v1/fleet/software/titles/:id/packageendpoint to be able to set the pinned version in thesoftware_title_team_pinstable and set the active installer if it changed.GET /software/titles/:idreturns the pinned version for a title from that table now.A few small fixes and updates that are related to this feature but not strictly in the scope of the subtask are also included (see individual comments).
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary 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
New Fleet configuration settings
This setting is already handled by gitops, this PR adds control of it with the update software installer api
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsSummary by CodeRabbit
New Features
Tests