Add scheduled updates functionality to iOS/iPadOS managed devices #37704
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37704 +/- ##
==========================================
- Coverage 65.88% 65.80% -0.08%
==========================================
Files 2361 2361
Lines 187298 187594 +296
Branches 7974 7873 -101
==========================================
+ Hits 123394 123449 +55
- Misses 52623 52858 +235
- Partials 11281 11287 +6
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:
|
lucasmrod
force-pushed
the
35455-schedule-updates
branch
from
efee8a3 to
785fbda
Compare
lucasmrod
force-pushed
the
35455-schedule-updates
branch
from
785fbda to
568b134
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR implements scheduled software updates for iOS/iPadOS managed devices. The changes add timezone tracking to hosts, integrate a VPP installer dependency into the MDM service, and introduce logic to determine when eligible software should be automatically installed within configured maintenance windows. Changes
Sequence Diagram(s)sequenceDiagram
participant Device as iOS/iPadOS Device
participant Fleet as Fleet MDM Service
participant DB as Datastore
participant VPP as VPP Installer
Device->>Fleet: Check-in (InstalledApplicationList)
activate Fleet
Fleet->>Device: Send DeviceInformation (with TimeZone query)
deactivate Fleet
Device->>Fleet: Device response + TimeZone + installed apps
activate Fleet
Fleet->>Fleet: Update host.TimeZone from response
Note over Fleet: Premium check: isPremium == true?
alt Premium License
Fleet->>DB: Get installed software for host
DB-->>Fleet: Installed apps list
Fleet->>DB: ListSoftwareAutoUpdateSchedules(teamID, source)
DB-->>Fleet: Auto-update schedules
loop For each schedule
Note over Fleet: Check eligibility:<br/>- Newer version available?<br/>- Host in labels?<br/>- App currently installed?
alt Eligible for update
Fleet->>Fleet: Check if current time<br/>in maintenance window<br/>(using timezone)
alt Time in window
Fleet->>VPP: InstallVPPAppPostValidation(host, app)
activate VPP
VPP-->>Fleet: Command UUID
deactivate VPP
end
end
end
end
Fleet->>DB: UpdateHost with new software state
deactivate Fleet
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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
🧹 Nitpick comments (5)
server/fleet/software_installer.go (1)
1009-1015: Clarify IsFleetInitiated comment now that ForScheduledUpdates is supportedThe implementation now classifies non‑self‑service scheduled updates as fleet‑initiated in addition to policy‑triggered installs. That behavior looks correct, but the comment above
IsFleetInitiatedstill only mentions policies; consider updating it to also call out scheduled updates so future readers don’t assume policies are the sole trigger.Also applies to: 1018-1022
server/service/apple_mdm_test.go (1)
6253-6258: Clarify test intent and consider adding a couple more casesThis smoke test is fine, but it only exercises a single happy-path window/timezone. To make the intent clearer and broaden coverage a bit, consider:
- Adding a short comment noting that
"00:00"–"23:59"is used so the result is deterministic regardless of current time.- Optionally expanding to a small table-driven test that also checks:
- A window that crosses midnight (e.g.
"23:00"–"01:00") to exercise that branch.- An invalid timezone or malformed window string to assert error behavior.
This would better lock in
isTimezoneInWindow’s expected semantics without much extra code.server/fleet/datastore.go (1)
593-599: Interface change for ListSoftwareAutoUpdateSchedules looks correctAdding the
source stringparameter aligns with the new per-source/per-platform scheduling tests and keeps the variadic filter semantics intact. Consider documenting (or typing) the expected source values ("ios_apps","ipados_apps", etc.) near this method to reduce the chance of call‑site typos, but the current shape is fine.server/datastore/mysql/software_titles_test.go (1)
2596-2791: testUpdateAutoUpdateConfig thoroughly covers per‑source schedules; minor robustness nitsThe test does a good job exercising:
- Validation of malformed time strings for
UpdateSoftwareTitleAutoUpdateConfig.- Enabling/disabling auto‑update and verifying persisted config via
SoftwareTitleByID.- Separation of schedules per
source("ipados_apps"vs"ios_apps") using the newListSoftwareAutoUpdateSchedules(ctx, teamID, source, filter...)API.- Filtering by
Enabledflag throughSoftwareAutoUpdateScheduleFilter.Two optional improvements to make the test less brittle:
- Instead of relying on
titles[0],titles[1],titles[2]to infer which title is which, derivetitleID/title2ID/title3IDbased ontitles[i].Source(and/orName). That will keep the test stable if the default ordering inListSoftwareTitlesever changes.- Similarly, if
ListSoftwareAutoUpdateSchedulesdoesn’t already guarantee ordering, consider asserting via a small map{TitleID: cfg}rather than positional indexes, to decouple the expectations from internalORDER BYchoices.These are test‑robustness tweaks only; the current behavior is logically sound.
cmd/fleet/serve.go (1)
1308-1316: LGTM — VPP installer integration follows established patterns.The type assertion and constructor call correctly wire up the VPP installer and premium flag for the MDM check-in service, enabling scheduled updates functionality. This matches the existing pattern at line 1032.
Optional: Consider defensive type assertion for better error diagnostics.
While the single-value type assertion matches the existing pattern at line 1032, using the two-value form would provide a more informative error message instead of a panic if the interface is not implemented:
🔎 Proposed defensive type assertion
- vppInstaller := svc.(fleet.AppleMDMVPPInstaller) + vppInstaller, ok := svc.(fleet.AppleMDMVPPInstaller) + if !ok { + initFatal(errors.New("service does not implement AppleMDMVPPInstaller interface"), "initialize MDM checkin service") + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
changes/35455-schedule-updatescmd/fleet/serve.gocmd/fleetctl/fleetctl/testdata/expectedHostDetailResponseJson.jsoncmd/fleetctl/fleetctl/testdata/expectedHostDetailResponseYaml.ymlcmd/fleetctl/fleetctl/testdata/expectedListHostsJson.jsoncmd/fleetctl/fleetctl/testdata/expectedListHostsMDM.jsoncmd/fleetctl/fleetctl/testdata/expectedListHostsYaml.ymlfrontend/pages/hosts/details/cards/Vitals/Vitals.tsxfrontend/utilities/constants.tsxserver/datastore/mysql/hosts.goserver/datastore/mysql/labels.goserver/datastore/mysql/software_titles.goserver/datastore/mysql/software_titles_test.goserver/fleet/apple_mdm.goserver/fleet/datastore.goserver/fleet/hosts.goserver/fleet/software_installer.goserver/mdm/apple/commander.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/service/apple_mdm_test.goserver/service/testing_utils.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:
server/datastore/mysql/labels.goserver/mdm/apple/commander.goserver/datastore/mysql/software_titles.goserver/fleet/apple_mdm.goserver/service/apple_mdm_test.goserver/fleet/hosts.goserver/fleet/datastore.goserver/service/testing_utils.gocmd/fleet/serve.goserver/fleet/software_installer.goserver/datastore/mysql/hosts.goserver/mock/datastore_mock.goserver/service/apple_mdm.goserver/datastore/mysql/software_titles_test.go
🧠 Learnings (2)
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
server/fleet/apple_mdm.goserver/service/apple_mdm_test.goserver/service/testing_utils.gocmd/fleet/serve.goserver/service/apple_mdm.go
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
cmd/fleet/serve.goserver/service/apple_mdm.go
🧬 Code graph analysis (7)
server/datastore/mysql/software_titles.go (1)
server/fleet/software.go (2)
SoftwareAutoUpdateScheduleFilter(273-275)SoftwareAutoUpdateSchedule(267-271)
server/fleet/datastore.go (1)
server/fleet/software.go (2)
SoftwareAutoUpdateScheduleFilter(273-275)SoftwareAutoUpdateSchedule(267-271)
cmd/fleet/serve.go (2)
server/fleet/apple_mdm.go (1)
AppleMDMVPPInstaller(1110-1117)server/service/apple_mdm.go (1)
NewMDMAppleCheckinAndCommandService(3340-3359)
frontend/pages/hosts/details/cards/Vitals/Vitals.tsx (1)
frontend/utilities/constants.tsx (1)
DEFAULT_EMPTY_CELL_VALUE(404-404)
server/mock/datastore_mock.go (1)
server/fleet/software.go (2)
SoftwareAutoUpdateScheduleFilter(273-275)SoftwareAutoUpdateSchedule(267-271)
server/service/apple_mdm.go (1)
server/fleet/software.go (4)
Software(49-115)Software(117-119)SoftwareAutoUpdateSchedule(267-271)SoftwareTitle(278-328)
server/datastore/mysql/software_titles_test.go (4)
server/fleet/vpp.go (3)
VPPApp(71-90)VPPAppTeam(21-58)VPPAppID(9-14)server/fleet/mdm.go (1)
IOSPlatform(1107-1107)server/fleet/software.go (2)
SoftwareAutoUpdateConfig(255-265)SoftwareAutoUpdateScheduleFilter(273-275)server/ptr/ptr.go (2)
String(10-12)Bool(25-27)
🔇 Additional comments (18)
server/mdm/apple/commander.go (1)
386-404: Adding TimeZone to DeviceInformation queries looks goodIncluding
TimeZonein the DeviceInformationQueriesarray cleanly supports populating the host timezone without impacting existing fields or the command structure.server/datastore/mysql/hosts.go (1)
692-745: Timezone wiring through host read/write paths is consistentSelecting
h.timezoneinHost,ListHosts,LoadHostByNodeKey, andHostByIdentifier, and persisting it via the newtimezone = ?column inUpdateHost, keeps the new field available across the main host read/write flows. The SQL still scopes by host id (no broader WHERE changes), so this is a straightforward schema extension rather than a behavioral change in host selection.Also applies to: 986-1037, 2663-2707, 3198-3241, 5275-5361
cmd/fleetctl/fleetctl/testdata/expectedHostDetailResponseYaml.yml (1)
102-102: Host detail YAML correctly includes timezoneThe added
timezone: nullfield aligns with the extended host schema and keeps the expected detail output in sync with backend changes.cmd/fleetctl/fleetctl/testdata/expectedListHostsYaml.yml (1)
68-68: List-hosts YAML fixtures updated to expose timezoneBoth host specs now include
timezone: null, matching the new host field while preserving the existing structure and ordering of the YAML.Also applies to: 127-127
changes/35455-schedule-updates (1)
1-1: Changelog entry matches the implemented featureThe note succinctly captures the new iOS/iPadOS scheduled updates functionality and fits the style of other
changes/entries.server/datastore/mysql/labels.go (1)
759-759: LGTM!The addition of
h.timezoneto the SELECT statement properly extends the host data returned by ListHostsInLabel. The query maintains appropriate filtering by label_id and team.cmd/fleetctl/fleetctl/testdata/expectedHostDetailResponseJson.json (1)
63-63: LGTM!The test data correctly reflects the new timezone field added to the host schema.
cmd/fleetctl/fleetctl/testdata/expectedListHostsMDM.json (1)
60-60: LGTM!The test data correctly reflects the new timezone field for both host entries.
Also applies to: 139-139
frontend/pages/hosts/details/cards/Vitals/Vitals.tsx (2)
243-257: LGTM!The renderTimezone function properly gates the timezone display to iOS/iPadOS hosts and checks for data presence before rendering.
526-526: LGTM!The timezone rendering is correctly integrated into the Vitals card layout.
cmd/fleetctl/fleetctl/testdata/expectedListHostsJson.json (1)
59-59: LGTM!The test data correctly reflects the new timezone field for both host entries.
Also applies to: 138-138
server/fleet/apple_mdm.go (1)
1115-1115: LGTM!The comment clarifies the return value of the InstallVPPAppPostValidation method, improving the interface documentation.
frontend/utilities/constants.tsx (1)
444-444: LGTM!The timezone field is correctly added to the HOST_VITALS_DATA constant, ensuring it's properly tracked as part of host vitals.
server/datastore/mysql/software_titles_test.go (1)
29-50: New UpdateAutoUpdateConfig test is properly wired into the main suiteIncluding
{"UpdateAutoUpdateConfig", testUpdateAutoUpdateConfig}in thecasesslice ensures the new auto‑update behavior is exercised alongside existing software title tests. No issues here.server/datastore/mysql/software_titles.go (1)
866-879: LGTM - Query correctly scoped by team and source.The changes properly filter schedules by both
team_idandsourceusing parameterized placeholders. The INNER JOIN withsoftware_titlesensures only schedules for valid titles matching the specified source (e.g.,ios_apps,ipados_apps) are returned, which aligns with the per-platform auto-update scheduling requirements.server/service/testing_utils.go (1)
477-478: LGTM - Test utility updated for VPP installer dependency.The type assertion and updated constructor call correctly wire the VPP installer and premium flag for MDM command processing in tests. While the unchecked type assertion could panic, this is acceptable in test code since it would immediately surface any interface mismatch during test execution.
server/mock/datastore_mock.go (2)
458-458: Signature update forListSoftwareAutoUpdateSchedulesFunclooks correctAdding the
source stringparameter here matches the evolved datastore interface and will let tests exercise source-specific auto-update behavior via the mock.
5762-5766: Mock method correctly forwards newsourceargumentThe
DataStore.ListSoftwareAutoUpdateSchedulesmethod now takessourceand passes it through toListSoftwareAutoUpdateSchedulesFuncwhile preserving the existing lock/invocation-flag pattern, which is consistent with the rest of this mock.
sgress454
left a comment
sgress454
left a comment
There was a problem hiding this comment.
Looks great! It'd be nice if we could just use InstallSoftwareTitle(). It'd need to be adjusted to accept the new ForScheduledUpdates install option, and I guess it'd do a lot of redundant work, but in the long run we probably don't want to be re-implementing the logic for "check if VPP app is right for host and if so, install it" multiple times. It's not something I'd touch in this pass at any rate.
| } | ||
|
|
||
| func (ds *Datastore) ListSoftwareAutoUpdateSchedules(ctx context.Context, teamID uint, optionalFilter ...fleet.SoftwareAutoUpdateScheduleFilter) ([]fleet.SoftwareAutoUpdateSchedule, error) { | ||
| func (ds *Datastore) ListSoftwareAutoUpdateSchedules(ctx context.Context, teamID uint, source string, optionalFilter ...fleet.SoftwareAutoUpdateScheduleFilter) ([]fleet.SoftwareAutoUpdateSchedule, error) { |
There was a problem hiding this comment.
We could also make source a filter in SoftwareAutoUpdateScheduleFilter. The idea there is that without any filters, you can list all the schedules for a team. Making it optional is a little more work though (we would need a token for the JOIN clause) so nbd either way.
| // 3. Filter out software that already has a pending installation (update). | ||
| adamIDsPendingInstallForHost, err := svc.ds.MapAdamIDsPendingInstall(ctx, host.ID) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "get Adam IDs pending install for host") | ||
| } | ||
| var softwaresWithinUpdateScheduleToInstall []*fleet.SoftwareTitle |
There was a problem hiding this comment.
Definitely out of scope for this story, but seems like InstallVPPAppPostValidation should either be idempotent by default or take something like ignorePending as an option, so that callers don't have to do this check themselves.
Resolves #35455
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)Testing
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.