Self service categories - install all - #46865
Conversation
… id from software titles
- remove the self-service install-all title cap (no limit on queued titles)
- endpoint signature interface{} -> any
- datastore test for GetSoftwareTitlesForInstallAll (status/inventory/label/category/team matrix, alpha order)
- consolidate integration coverage into TestInstallAllSelfServiceSoftware: install statuses, existing/incoming + concurrent activities (incl. double-queue), category/label/team scoping, multiple hosts, with VPP exercised in every run
- add TestSelfServiceInstallDoubleEnqueue characterizing the pre-existing double-enqueue
Backend: - GetSoftwareTitlesForInstallAll returns BadRequestError for a nonexistent category_id (client-supplied bad param), not the wrapped not-found error. - Fix a misleading comment: the len(InstalledVersions) skip means the title is already installed. Tests: - Datastore: assert BadRequestError for both nonexistent and cross-fleet categories; integration: nonexistent category now expects 400. - Rename rollupName -> installAllActivityName and completeHead -> completeActivatedInstall. - Inline the single-use mdmEnroll helper into newMDMHost. - Drop the temporary TestSelfServiceInstallDoubleEnqueue. - Replace the AppConfig-cache time.Sleep with an EventuallyWithT poll of GET /config so we wait on the actual condition, never a fixed duration.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/39018-self-service-categories #46865 +/- ##
=====================================================================
Coverage ? 66.83%
=====================================================================
Files ? 2807
Lines ? 224009
Branches ? 11328
=====================================================================
Hits ? 149713
Misses ? 60696
Partials ? 13600
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:
|
|
|
![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.
|
@claude review once |
There was a problem hiding this comment.
I didn't find any bugs in the new install-all flow, but this PR adds a sizeable new self-service endpoint with VPP/in-house/package branches and label/category/team scoping — worth a human pass before merging into the feature branch.
Extended reasoning...
Overview
This PR adds a new "install all" self-service software flow on top of the in-progress self-service categories feature branch (feat/39018-self-service-categories). It introduces:
- A new device endpoint
POST /api/_version_/fleet/device/{token}/software/install_all(with optionalcategory_idquery param) - A new EE service method
SelfServiceInstallAllSoftwareTitles(~120 lines) that branches across VPP apps, in-house apps, and package installers, with per-title label scoping and pre-flight checks - A new datastore method
GetSoftwareTitlesForInstallAllthat filters self-service titles by status, inventory, category, and team scoping - A new
ActivityTypeInstalledAllSelfServiceSoftwareactivity and roll-up - Exposing the previously-unexported
InstallerAvailableForInstallForTeamAndTitleIDfor reuse - ~500 lines of integration test coverage and a focused datastore test
Security risks
The endpoint is authenticated via device token (same model as the existing single-title self-service install), and category/team scoping is validated in the datastore layer. Label scoping (include-any / exclude-any) is applied per title at queue time. The main attack surface is the "queue every available title" fan-out from a compromised device token, but that's already implicit in the per-title self-service endpoint that existed before. No new SQL injection / authz bypass / data exposure surface jumped out.
Level of scrutiny
Production-critical: this lands a new user-facing endpoint that queues MDM commands and software installs against real hosts, with multi-branch logic (VPP / in-house / package) and several scoping rules (category, team, label, MDM enrollment, inventory state) that all need to agree. The integration tests do cover the interesting cases (idempotency, queue coexistence, category/label/team scoping, the documented double-queue race for concurrent installs), but the combinatorics make a human review valuable.
Other factors
- Bug-hunting agents found no defects
- Patch coverage is 57.9% per Codecov; the EE service method is the largest gap (39%)
- Targets a feature branch, not main, so blast radius is limited until that branch merges
- One unrelated CI failure (
TestTranslateCPEToCVE/find_vulns_on_cpes), already acknowledged in-thread as fixed on main - Author explicitly requested a bot review
| return nil | ||
| }() | ||
| if queueErr != nil { | ||
| svc.logger.ErrorContext(ctx, "failed to enqueue software install", "title_id", title.ID, "err", queueErr) |
There was a problem hiding this comment.
It looks like we log here but the device gets a 202 no matter how many failed, is that right?
There was a problem hiding this comment.
Yeah I went with just logging any errors that happen within the loop for this, because we wouldn't want to fail the whole thing even if one installer failed right? Is there maybe something more nuanced we can do like fail if the count of actually queued installs doesn't match the count coming in? I'll leave that for later.
I think I can replace this whole part of the loop with SelfServiceInstallSoftwareTitle and log the errors from that rather than return, since it is queuing installs one by one anyway. Downside is it uses the slightly more expensive GetSoftwareInstallerMetadataByTeamAndTitleID function.
I think I'll give that a try.
There was a problem hiding this comment.
Oh, looks like we actually do want an error message here. My bad for missing that.
POST /device/{token}/software/install_all — when one or more apps in the list are currently installing, updating, or uninstalling, verify the endpoint returns an appropriate error with a human-readable message.
POST /device/{token}/software/install_all — verify that no apps are queued or double-queued when the request is rejected due to an in-progress operation.
Verify the error response body includes enough detail to identify which app(s) are blocking the request (or at minimum that the message is actionable for the end user).
cc @Brajim20 heads up that we missed this, in case you get to QAing before this is fixed.
| // queue each software install individually | ||
| var queuedCount uint | ||
| for _, title := range titles { | ||
| // log errors rather than return early |
There was a problem hiding this comment.
Well, we still return early for out of scope labels below (nit).
There was a problem hiding this comment.
Updated the comment to the one below. Now that it's using SelfServiceInstallSoftwareTitle this is actually correct because out of scope labels get treated as errors.
// Queue individual install activities for each title. If any errors occurred while
// queuing this title we log them and continue to the next software title.
cdcme
left a comment
cdcme
left a comment
There was a problem hiding this comment.
Couple of minor questions which could be dealt with later. Looks fantastic!
Undo exporting installerAvailableForInstallForTeamAndTitleID
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
d6b56d8
into
feat/39018-self-service-categories
3 participants
Related issue: Resolves #46393
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