Skip setup experience during AxM based migrations - #32822
Conversation
GitHub Actions
Error
|
@coderabbitai review |
GitHub Actions
Error
✅ Actions performedReview triggered.
|
GitHub Actions
Inactive
WalkthroughImplements per-host DEP migration deadline tracking end-to-end: schema migration adds columns, DEP sync ingests deadlines and upserts assignments, datastore and interfaces updated, TokenUpdate branches on migration state to skip setup items and mark completion, nanodep protocol/UA updated, structs/queries extended, and tests adjusted for new signatures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant ABM as Apple ABM (DEP)
participant Sync as DEP Sync
participant DS as Datastore
participant DB as MySQL
rect rgba(200,230,255,0.3)
note over ABM,Sync: DEP sync ingest with migration deadlines
ABM-->>Sync: List DEP devices (includes mdm_migration_deadline?)
Sync->>Sync: Map serial -> deadline
Sync->>DS: UpsertMDMAppleHostDEPAssignments(hosts, abmTokenID, deadlinesByHostID)
DS->>DB: INSERT ... (host_id, abm_token_id, mdm_migration_deadline)<br/>ON DUPLICATE UPDATE mdm_migration_deadline=VALUES(...)
DB-->>DS: OK
end
sequenceDiagram
autonumber
participant Device as macOS Device
participant Svc as Service TokenUpdate
participant DS as Datastore
participant DB as MySQL
Svc->>DS: GetHostMDMCheckinInfo(...)
DS->>DB: SELECT ..., migration_in_progress
DB-->>DS: host_id, platform, migration_in_progress
DS-->>Svc: HostMDMCheckinInfo
alt migration_in_progress == false
Svc->>Svc: EnqueueSetupExperienceItems
else migration_in_progress == true
note over Svc: Skip setup experience during migration
Svc->>DS: SetHostMDMMigrationCompleted(hostID)
DS->>DB: UPDATE host_dep_assignments<br/>SET mdm_migration_completed=mdm_migration_deadline<br/>WHERE host_id=?
DB-->>DS: OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
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: 6
🧹 Nitpick comments (17)
server/worker/apple_mdm_test.go (1)
98-99: Nit: pass nil instead of an empty map
nilmaps iterate fine in Go; considernilfor brevity where no deadlines are set.- err := ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h}, abmTokenID, make(map[uint]time.Time)) + err := ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h}, abmTokenID, nil)server/mdm/nanodep/godep/client.go (1)
18-21: User-Agent change LGTM; consider including Fleet version for diagnosticsThe new UA unblocks newer protocol versions. Consider appending the Fleet version/commit to help support triage (e.g.,
fleetdm/nanodep Fleet/<version>), wired via an injected/version package or build flag.server/fleet/hosts.go (1)
1367-1378: Add documentation for migration_in_progress logic
SQL atserver/datastore/mysql/hosts.go:4413correctly implementsdeadline IS NOT NULL AND (completed IS NULL OR deadline > completed), with equality clearing the flag; add a code comment here to document this invariant.server/worker/macos_setup_assistant_test.go (1)
56-57: Nit: pass nil instead of an empty mapFor readability, pass
nilwhere no deadlines exist.- err = ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h}, tokID, make(map[uint]time.Time)) + err = ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h}, tokID, nil)server/datastore/mysql/hosts_test.go (2)
8734-8740: Exercise and assert per-host DEP migration deadlines (not just assignment).Right now you pass an empty map, so the new behavior isn’t covered. Populate the deadline for the host and assert it persisted to host_dep_assignments. This guards against regressions in the migration-deadline path.
- err = ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*host}, abmToken.ID, make(map[uint]time.Time)) + deadline := time.Now().UTC().Add(24 * time.Hour).Truncate(time.Second) + err = ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*host}, abmToken.ID, map[uint]time.Time{host.ID: deadline}) require.NoError(t, err) + // verify deadline persisted + var gotDeadline sql.NullTime + ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { + return sqlx.GetContext(ctx, q, &gotDeadline, + `SELECT mdm_migration_deadline FROM host_dep_assignments WHERE host_id = ? AND deleted_at IS NULL`, + host.ID) + }) + require.True(t, gotDeadline.Valid) + require.WithinDuration(t, deadline, gotDeadline.Time.UTC(), time.Second)
11301-11303: Use nil map to exercise the nil-path; keep non-empty coverage in a dedicated test.Passing nil is simpler and ensures the function handles nil and empty maps equivalently. The non-empty case is covered by the earlier test.
-require.NoError(t, ds.UpsertMDMAppleHostDEPAssignments(ctx, hosts, abmTok.ID, make(map[uint]time.Time))) +require.NoError(t, ds.UpsertMDMAppleHostDEPAssignments(ctx, hosts, abmTok.ID, nil))server/service/integration_core_test.go (1)
10118-10118: Prefer nil over an empty map for clarity and to avoid accidental writesPassing an empty map works if the implementation only ranges keys, but if it checks map non-nil or writes default values, this could produce unintended rows. Using nil makes the “no deadlines provided” intent explicit.
- err = s.ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*hFleetMDM}, abmToken.ID, make(map[uint]time.Time)) + err = s.ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*hFleetMDM}, abmToken.ID, nil)server/fleet/apple_mdm.go (1)
531-538: Clarify MDMMigrationCompleted semantics (doc-only).Comment says “Not a timestamp but a marker” while the field is a TIMESTAMP. Clarify that it stores a copy of the deadline at completion time (marker), not an event time.
Apply this doc-only diff:
- // MDMMigrationCompleted is the value of MDMMigrationDeadline when the host completed its last - // Migration. Not a timestamp but a marker that the host completed the Migration for a given - // date. + // MDMMigrationCompleted stores a copy of MDMMigrationDeadline when the host completed its last + // migration. It’s used as a marker (i.e., equals the deadline value at completion), not an event + // timestamp of when completion happened.server/datastore/mysql/migrations/tables/20250910130115_AddMigrationDeadlineToHostDEPAssignments.go (1)
11-17: Consider adding indexes for common lookups.If you’ll frequently filter by “deadline set” and/or “completed is NULL”, add supporting indexes now to avoid future table scans.
Apply this migration tweak (safe to adjust before merging):
func Up_20250910130115(tx *sql.Tx) error { - stmt := `ALTER TABLE host_dep_assignments - ADD COLUMN mdm_migration_deadline TIMESTAMP(6) DEFAULT NULL, - ADD COLUMN mdm_migration_completed TIMESTAMP(6) DEFAULT NULL` + stmt := `ALTER TABLE host_dep_assignments + ADD COLUMN mdm_migration_deadline TIMESTAMP(6) DEFAULT NULL, + ADD COLUMN mdm_migration_completed TIMESTAMP(6) DEFAULT NULL, + ADD INDEX idx_dep_migration_deadline (mdm_migration_deadline), + ADD INDEX idx_dep_migration_completed (mdm_migration_completed)` _, err := tx.Exec(stmt) return err }server/datastore/mysql/apple_mdm_test.go (3)
6214-6214: Exercise the new addedAt path in DEP assignment updates.Current assertions validate DeletedAt toggling but not the new timestamp input. Suggest passing an explicit timestamp and asserting GetHostDEPAssignment().AddedAt equals it (and remains stable across delete/restore).
Minimal changes within this test:
- err = ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h}, abmToken.ID, make(map[uint]time.Time)) + wantAt := time.Now().Add(-time.Hour).UTC().Round(time.Second) + err = ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h}, abmToken.ID, map[uint]time.Time{h.ID: wantAt})Then after fetching
assignment:require.WithinDuration(t, wantAt, assignment.AddedAt, time.Second)Also applies to: 6230-6230
7415-7415: Nit: avoid repeated empty map allocations; prefer nil or a shared var.Same as above—replace make(map[uint]time.Time) with nil (if supported) or reuse a shared empty map variable.
Example:
- require.NoError(t, ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h1, *h4}, tok1.ID, make(map[uint]time.Time))) + require.NoError(t, ds.UpsertMDMAppleHostDEPAssignments(ctx, []fleet.Host{*h1, *h4}, tok1.ID, nil))Also applies to: 7417-7418
4244-4244: Use nil for empty deadlines and add a non‐nil deadline test
- In tests, replace
make(map[uint]time.Time)withnilwhen callingUpsertMDMAppleHostDEPAssignments(nil maps are safe and avoid repeated allocations).- Add a focused test that passes a map containing a non‐zero
time.Timefor a host ID and asserts themdm_migration_deadlinecolumn is set accordingly.server/fleet/datastore.go (1)
1251-1253: Clarify UTC and nil-map semantics for deadlines param.The new param is clear, but please document that:
- Times must be normalized to UTC.
- The map may be nil/empty (no deadlines known) and must be treated as no-ops by implementations.
Apply this doc tweak:
-// `host_dep_assignments` for all the provided hosts. mdmMigrationDeadlinesByHostID -// should include migration deadlines from the DEP API for any hosts that had one set +// `host_dep_assignments` for all the provided hosts. mdmMigrationDeadlinesByHostID +// should include migration deadlines from the DEP API for any hosts that had one set. +// Times MUST be normalized to UTC. Pass a nil/empty map if no deadlines are known.server/mdm/apple/apple_mdm.go (3)
619-635: Log deadline in RFC3339 and fix log key typo; avoid "nil" sentinel.Structured logs are easier to parse with RFC3339 and without string sentinels; also “push_push_time” looks like a typo.
- deadline := "nil" - if device.MDMMigrationDeadline != nil { - deadline = device.MDMMigrationDeadline.String() - } + var deadline string // empty means no deadline + if device.MDMMigrationDeadline != nil { + deadline = device.MDMMigrationDeadline.UTC().Format(time.RFC3339) + } ... - "push_push_time", device.ProfilePushTime, + "profile_push_time", device.ProfilePushTime, "profile_uuid", device.ProfileUUID, "mdm_migration_deadline", deadline,
783-783: Pre-allocate map capacity.Small win to avoid rehashing on large syncs.
- existingHostMigrationDeadlines := make(map[uint]time.Time) + existingHostMigrationDeadlines := make(map[uint]time.Time, len(existingSerials))
792-795: Normalize deadline to UTC before persisting/passing downstream.Keeps storage/logs consistent regardless of ABM timezone.
- if dd.MDMMigrationDeadline != nil { - existingHostMigrationDeadlines[existingHost.ID] = *dd.MDMMigrationDeadline - } + if dd.MDMMigrationDeadline != nil { + existingHostMigrationDeadlines[existingHost.ID] = dd.MDMMigrationDeadline.UTC() + }server/datastore/mysql/apple_mdm.go (1)
1721-1726: Normalize deadlines to UTC before storing.Minor, but makes comparisons, logging, and cross-DB consistency simpler.
- if device.MDMMigrationDeadline != nil { - migrationDeadlinesBySerial[device.SerialNumber] = *device.MDMMigrationDeadline - } + if device.MDMMigrationDeadline != nil { + dl := device.MDMMigrationDeadline.UTC() + migrationDeadlinesBySerial[device.SerialNumber] = dl + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
server/datastore/mysql/apple_mdm.go(5 hunks)server/datastore/mysql/apple_mdm_test.go(4 hunks)server/datastore/mysql/hosts.go(1 hunks)server/datastore/mysql/hosts_test.go(3 hunks)server/datastore/mysql/migrations/tables/20250910130115_AddMigrationDeadlineToHostDEPAssignments.go(1 hunks)server/datastore/mysql/migrations/tables/20250910130115_AddMigrationDeadlineToHostDEPAssignments_test.go(1 hunks)server/fleet/apple_mdm.go(1 hunks)server/fleet/datastore.go(1 hunks)server/fleet/hosts.go(2 hunks)server/mdm/apple/apple_mdm.go(4 hunks)server/mdm/nanodep/client/transport.go(1 hunks)server/mdm/nanodep/godep/client.go(1 hunks)server/mdm/nanodep/godep/device.go(1 hunks)server/mock/datastore_mock.go(4 hunks)server/service/apple_mdm.go(1 hunks)server/service/integration_core_test.go(1 hunks)server/worker/apple_mdm_test.go(1 hunks)server/worker/macos_setup_assistant_test.go(1 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/apple_mdm.goserver/mdm/nanodep/client/transport.goserver/datastore/mysql/migrations/tables/20250910130115_AddMigrationDeadlineToHostDEPAssignments_test.goserver/service/apple_mdm.goserver/mdm/nanodep/godep/client.goserver/fleet/hosts.goserver/datastore/mysql/migrations/tables/20250910130115_AddMigrationDeadlineToHostDEPAssignments.goserver/mock/datastore_mock.goserver/worker/macos_setup_assistant_test.goserver/mdm/nanodep/godep/device.goserver/datastore/mysql/hosts.goserver/worker/apple_mdm_test.goserver/mdm/apple/apple_mdm.goserver/service/integration_core_test.goserver/fleet/datastore.goserver/datastore/mysql/hosts_test.goserver/datastore/mysql/apple_mdm_test.goserver/datastore/mysql/apple_mdm.go
🔇 Additional comments (11)
server/mdm/nanodep/godep/device.go (1)
25-25: Vendor-specific field: confirm JSON key matches observed payload
Apple’s public DEP/Automated Device Enrollment schema does not includemdm_migration_deadline; this is an implementation-specific extension. The addition ofMDMMigrationDeadline *time.Time `json:"mdm_migration_deadline,omitempty"`is backward-compatible. If you’ve observed this key in your vendor’s DEP payload, verify the exact casing and ISO-8601 timestamp format against that source.
server/datastore/mysql/migrations/tables/20250910130115_AddMigrationDeadlineToHostDEPAssignments.go (1)
19-21: Down is a no-op — confirm this matches migration policy.If your project prefers irreversible migrations, fine. Otherwise, add a DROP COLUMNs (and DROP INDEXes) for local rollback.
server/fleet/datastore.go (1)
1259-1263: Drop equality-coupling suggestion Migration status is derived via the explicitmigration_in_progressflag (mdm_migration_deadline IS NOT NULL AND (mdm_migration_completed IS NULL OR mdm_migration_deadline > mdm_migration_completed)), andMDMMigrationCompletedis intentionally a marker—there are no direct==comparisons.Likely an incorrect or invalid review comment.
server/mock/datastore_mock.go (5)
2799-2801: LGTM: mock fields and invoked flag wired for SetHostMDMMigrationCompleted.Consistent with the file’s pattern.
6732-6737: LGTM: forwarder updated to pass deadlines map and mark invocation.No behavioral changes beyond plumbing; consistent with other methods.
6746-6751: LGTM: new forwarder for SetHostMDMMigrationCompleted.Matches the declared func type and struct fields.
882-882: LGTM: SetHostMDMMigrationCompleted implementation and usage verified. Found definitions in server/datastore/mysql/hosts.go (line 4386) and server/mock/datastore_mock.go (line 6746), and invocation in server/service/apple_mdm.go (line 3511).
878-878: Approve: signature and implementation validatedserver/datastore/mysql/apple_mdm.go (3)
2087-2087: LGTM: select includes new migration fields.Selecting mdm_migration_deadline and mdm_migration_completed looks correct given the struct changes.
2168-2172: Good cleanup on unassignment.Nulling mdm_migration_deadline and mdm_migration_completed prevents stale migration state after ABM unassignment.
1823-1829: **Bug: passing time.Time in SQL args will panic/return “unsupported type time.Time”.database/sql expects time.Time or sql.NullTime, not a pointer. This will fail whenever a deadline is present.
Apply this diff:
- var deadline *time.Time - if d, ok := migrationDeadlinesByHostID[host.ID]; ok { - deadline = &d - } - args = append(args, host.ID, abmTokenID, deadline) + var deadlineVal interface{} // nil or time.Time + if d, ok := migrationDeadlinesByHostID[host.ID]; ok { + deadlineVal = d + } + args = append(args, host.ID, abmTokenID, deadlineVal)Alternatively, use sql.NullTime:
+ var deadline sql.NullTime + if d, ok := migrationDeadlinesByHostID[host.ID]; ok { + deadline = sql.NullTime{Time: d, Valid: true} + } - args = append(args, host.ID, abmTokenID, deadline) + args = append(args, host.ID, abmTokenID, deadline)⛔ Skipped due to learnings
Learnt from: getvictor PR: fleetdm/fleet#32173 File: server/datastore/mysql/policies.go:360-365 Timestamp: 2025-08-22T01:14:05.454Z Learning: The sqlx library in Go (and the underlying database/sql package) can handle pointer types as query parameters correctly. Non-nil pointers are automatically dereferenced to get their values, and nil pointers are converted to SQL NULL values. This is standard, documented behavior - not a limitation or bug. Passing pointer parameters like `*uint`, `*string`, etc. to sqlx query methods is perfectly valid.
| func (ds *Datastore) SetHostMDMMigrationCompleted(ctx context.Context, hostID uint) error { | ||
| _, err := ds.writer(ctx).ExecContext(ctx, ` | ||
| UPDATE host_dep_assignments | ||
| SET mdm_migration_completed = mdm_migration_deadline | ||
| WHERE host_id = ?`, hostID, | ||
| ) | ||
| if err != nil { | ||
| return ctxerr.Wrapf(ctx, err, "set mdm migration completed for host_id %d", hostID) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limit the UPDATE to the active DEP assignment (avoid touching historical rows).
As written, this UPDATE will set mdm_migration_completed on all host_dep_assignments rows for the host, including historical ones with deleted_at set. That can corrupt history and conflict with future reassignments. Filter to the active (non-deleted) assignment; optionally cap with LIMIT 1 if there’s at most one active row by constraint.
Apply:
- UPDATE host_dep_assignments
- SET mdm_migration_completed = mdm_migration_deadline
- WHERE host_id = ?`, hostID,
+ UPDATE host_dep_assignments
+ SET mdm_migration_completed = mdm_migration_deadline
+ WHERE host_id = ? AND deleted_at IS NULL
+ LIMIT 1`, hostID,📝 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) SetHostMDMMigrationCompleted(ctx context.Context, hostID uint) error { | |
| _, err := ds.writer(ctx).ExecContext(ctx, ` | |
| UPDATE host_dep_assignments | |
| SET mdm_migration_completed = mdm_migration_deadline | |
| WHERE host_id = ?`, hostID, | |
| ) | |
| if err != nil { | |
| return ctxerr.Wrapf(ctx, err, "set mdm migration completed for host_id %d", hostID) | |
| } | |
| return nil | |
| } | |
| func (ds *Datastore) SetHostMDMMigrationCompleted(ctx context.Context, hostID uint) error { | |
| _, err := ds.writer(ctx).ExecContext(ctx, ` | |
| UPDATE host_dep_assignments | |
| SET mdm_migration_completed = mdm_migration_deadline | |
| WHERE host_id = ? AND deleted_at IS NULL | |
| LIMIT 1`, hostID, | |
| ) | |
| if err != nil { | |
| return ctxerr.Wrapf(ctx, err, "set mdm migration completed for host_id %d", hostID) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In server/datastore/mysql/hosts.go around lines 4386 to 4396, the UPDATE
currently modifies mdm_migration_completed for all host_dep_assignments for the
host, including historical (deleted) rows; change the WHERE clause to restrict
to the active assignment by adding a condition like "AND deleted_at IS NULL"
and, if the schema guarantees at most one active assignment, append "LIMIT 1" to
avoid touching historical rows and prevent corrupting history or future
reassignments.
There was a problem hiding this comment.
host_id is the pk for host_dep_assignments so this is not true
| h.id AS host_id, | ||
| h.hardware_serial, | ||
| COALESCE(hm.installed_from_dep, false) as installed_from_dep, | ||
| hd.display_name, | ||
| COALESCE(h.team_id, 0) as team_id, | ||
| hda.host_id IS NOT NULL AND hda.deleted_at IS NULL as dep_assigned_to_fleet, | ||
| h.node_key IS NOT NULL as osquery_enrolled, | ||
| EXISTS (SELECT 1 FROM nano_cert_auth_associations WHERE id = h.uuid AND renew_command_uuid IS NOT NULL) AS scep_renewal_in_progress, | ||
| h.platform | ||
| h.platform, | ||
| hda.mdm_migration_deadline IS NOT NULL AND | ||
| (hda.mdm_migration_completed IS NULL OR | ||
| (hda.mdm_migration_deadline > hda.mdm_migration_completed)) AS migration_in_progress | ||
| FROM |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Join host_dep_assignments on the active row to prevent nondeterministic results.
Without deleted_at IS NULL in the JOIN, multiple rows (historical + active) can match a single host. With LIMIT 1 and no ORDER BY, the row chosen is arbitrary — dep_assigned_to_fleet and the new migration_in_progress flag can be wrong.
Apply:
- LEFT JOIN
- host_dep_assignments hda
- ON h.id = hda.host_id
+ LEFT JOIN
+ host_dep_assignments hda
+ ON h.id = hda.host_id AND hda.deleted_at IS NULLNote: Adding host_id and platform to the SELECT looks good; the issue is only the JOIN scope driving migration_in_progress.
Also applies to: 4425-4427
🤖 Prompt for AI Agents
In server/datastore/mysql/hosts.go around lines 4404-4416 (and similarly at
4425-4427), the JOIN to host_dep_assignments is not scoped to the active row so
historical rows can match and produce nondeterministic values for
dep_assigned_to_fleet and migration_in_progress; update the JOIN condition to
include host_dep_assignments.deleted_at IS NULL (or equivalent active-row
predicate) so only the active assignment row is joined, ensuring
dep_assigned_to_fleet and mdm_migration_deadline/mdm_migration_completed are
evaluated against the current assignment.
| func TestUp_20250910130115(t *testing.T) { | ||
| db := applyUpToPrev(t) | ||
|
|
||
| // | ||
| // Insert data to test the migration | ||
| // | ||
| // ... | ||
|
|
||
| // Apply current migration. | ||
| applyNext(t, db) | ||
|
|
||
| // | ||
| // Check data, insert new entries, e.g. to verify migration is safe. | ||
| // | ||
| // ... | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fill in migration test assertions (ensure columns exist and are usable)
The scaffold doesn’t validate the migration. Add minimal checks that the new columns exist and are queryable.
package tables
-import "testing"
+import (
+ "testing"
+)
func TestUp_20250910130115(t *testing.T) {
db := applyUpToPrev(t)
- //
- // Insert data to test the migration
- //
- // ...
-
// Apply current migration.
applyNext(t, db)
- //
- // Check data, insert new entries, e.g. to verify migration is safe.
- //
- // ...
+ // Verify columns exist (query compiles) without requiring rows.
+ if _, err := db.Exec(`SELECT mdm_migration_deadline, mdm_migration_completed FROM host_dep_assignments LIMIT 0`); err != nil {
+ t.Fatalf("expected new columns to exist after migration: %v", err)
+ }
}I can extend this to insert a minimal row pre-migration and validate NULL defaults post-migration if you share required non-null columns for host_dep_assignments in the pre-migration schema.
📝 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 TestUp_20250910130115(t *testing.T) { | |
| db := applyUpToPrev(t) | |
| // | |
| // Insert data to test the migration | |
| // | |
| // ... | |
| // Apply current migration. | |
| applyNext(t, db) | |
| // | |
| // Check data, insert new entries, e.g. to verify migration is safe. | |
| // | |
| // ... | |
| } | |
| package tables | |
| import ( | |
| "testing" | |
| ) | |
| func TestUp_20250910130115(t *testing.T) { | |
| db := applyUpToPrev(t) | |
| // Apply current migration. | |
| applyNext(t, db) | |
| // Verify columns exist (query compiles) without requiring rows. | |
| if _, err := db.Exec(`SELECT mdm_migration_deadline, mdm_migration_completed FROM host_dep_assignments LIMIT 0`); err != nil { | |
| t.Fatalf("expected new columns to exist after migration: %v", err) | |
| } | |
| } |
| ServerProtocolVersion = "X-Server-Protocol-Version" | ||
|
|
||
| DefaultServerProtocolVersion = "3" | ||
| DefaultServerProtocolVersion = "8" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bump to protocol v8 — verify Apple’s latest and consider configurability
Good to default to the newest, but please:
- Confirm v8 is current/supported across all DEP endpoints you use.
- Consider making the default overridable (config/env or per-token), keeping the existing “only set if missing” behavior.
🌐 Web query:
What is the latest supported value for Apple DEP header `X-Server-Protocol-Version`, and is "8" valid as of September 2025?
💡 Result:
Short answer: The DEP/Automated Device Enrollment protocol currently supports X-Server-Protocol-Version values 1, 2, and 3 (with Apple servers running v3). "8" is not a valid/supported value as of September 10, 2025. Sources: [1][2][3]
Sources
- Mobile Device Management Protocol Reference (lists X-Server-Protocol-Version: 1, 2, or 3 and notes server now runs version 2/3). [1]
- nanoDEP (micromdm) operations guide — notes DEP proxy default/current version is "3". [2]
- Microsoft Intune Apple/DEP documentation (references keys valid in X-Server-Protocol-Version 2 and later). [3]
Use a supported DEP protocol version (max v3) and enable override
In server/mdm/nanodep/client/transport.go line 19, replace
DefaultServerProtocolVersion = "8"with a valid value (e.g. "3")—Apple’s DEP only supports versions 1–3 (github.com). Expose this via config/env or per-token, retaining the “only set if missing” logic.
🤖 Prompt for AI Agents
In server/mdm/nanodep/client/transport.go around line 19 the
DefaultServerProtocolVersion is set to an unsupported value ("8"); change the
default to a supported DEP protocol (e.g. "3") and wire it so it can be
overridden from configuration (env var) or per-token settings while keeping the
existing "only set if missing" behavior—i.e., read an optional override
(env/config or token field) first, fall back to the default constant only when
no version is provided, and validate/limit values to the supported range (1–3)
before using them.
There was a problem hiding this comment.
That Apple documentation is wrong. Their latest docs on apple.com cite v8 which we must use to get Migration deadlines
| if !info.MigrationInProgress { | ||
| // Enqueue setup experience items and mark the host as being in setup experience | ||
| hasSetupExpItems, err = svc.ds.EnqueueSetupExperienceItems(r.Context, info.Platform, r.ID, info.TeamID) | ||
| if err != nil { | ||
| return ctxerr.Wrap(r.Context, err, "queueing setup experience tasks") | ||
| } | ||
| } else { | ||
| svc.logger.Log("info", "skipping setup experience enqueueing because DEP migration is in progress", "host_uuid", r.ID) | ||
| err = svc.ds.SetHostMDMMigrationCompleted(r.Context, info.HostID) | ||
| if err != nil { | ||
| return ctxerr.Wrap(r.Context, err, "setting mdm migration completed") | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Skip-setup path looks right; verify idempotent completion and awaiting-config behavior.
- Ensure SetHostMDMMigrationCompleted is idempotent and only writes when mdm_migration_deadline is non-NULL (e.g., WHERE mdm_migration_completed IS NULL AND mdm_migration_deadline IS NOT NULL).
- Confirm product intent: Do we need to explicitly clear “awaiting configuration” or enqueue DeviceConfigured when skipping setup during migration? If not, all good.
Run this to confirm datastore semantics:
🏁 Script executed:
#!/bin/bash
# Find implementation and SQL for migration completion and checkin info flags
rg -nP 'SetHostMDMMigrationCompleted\s*\(' -C3
rg -nP 'UPDATE .*host_dep_assignments.*mdm_migration_completed' -C3 --type go
rg -nP 'type\s+HostMDMCheckinInfo\b' -C5
rg -nP '\bMigrationInProgress\b' -C3Length of output: 6579
Add idempotence guard to SetHostMDMMigrationCompleted
- Restrict the UPDATE in server/datastore/mysql/hosts.go to only apply when mdm_migration_completed IS NULL AND mdm_migration_deadline IS NOT NULL.
- Verify whether skipping setup during DEP migration should clear AwaitingConfiguration or enqueue a DeviceConfigured task.
There was a problem hiding this comment.
We have to wait for profile installation to clear the AwaitingConfiguration flag so I don't think we need to do this here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #32822 +/- ##
========================================
Coverage 63.81% 63.81%
========================================
Files 2040 2041 +1
Lines 200958 201241 +283
Branches 6681 6681
========================================
+ Hits 128237 128426 +189
- Misses 62588 62659 +71
- Partials 10133 10156 +23
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:
|
GitHub Actions
Inactive
GitHub Actions
Error
…s don't get marked as needing migration
GitHub Actions
Inactive
GitHub Actions
Inactive
JordanMontgomery
changed the title
Co-authored-by: Magnus Jensen <magnus@fleetdm.com>
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
Code looks good to me! good job on the quick turnover
Fixes #32096 The gist of the fix is that when syncing devices from DEP we save the migration deadline to our host_dep_assignments table. The next enrollment, which we assume should be the migration, looks at host_dep_assignments, sees that mdm_migration_deadline is non-Null and mdm_migration_completed is NULL, and uses that as the signal that a migration is in progress and skips enqueuing setup experience items. It then marks the migration as complete which sets mdm_migration_completed = mdm_migration_deadline. Once this is set setup experience will run as normal unless mdm_migration_completed gets set to NULL and/or mdm_migration_deadline gets set to a value in the future(which e.g. would happen if the customer assigned to another MDM server then assigned to migrate to fleet again) DB test failure is expected here because it won't like the migration timestamp but that is a necessary failure because this fix is going to be backported into 4.73 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [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) - [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 For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results - [x] Alerted the release DRI if additional load testing is needed ## Database migrations - [x] Checked table schema to confirm autoupdate - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * New Features * Tracks and stores Apple DEP MDM migration deadlines per device/host. * Detects “migration in progress” during DEP sync and check-in. * Automatically marks migration complete and skips Setup Assistant items while migration is in progress to prevent conflicts. * Bug Fixes * Improved DEP compatibility by updating the protocol version and User-Agent used for Apple’s APIs, reducing the chance of blocked or rejected requests. * Migrations * Adds fields to support migration deadlines and completion status (no action required). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Magnus Jensen <magnus@fleetdm.com>
Fixes #32096 The gist of the fix is that when syncing devices from DEP we save the migration deadline to our host_dep_assignments table. The next enrollment, which we assume should be the migration, looks at host_dep_assignments, sees that mdm_migration_deadline is non-Null and mdm_migration_completed is NULL, and uses that as the signal that a migration is in progress and skips enqueuing setup experience items. It then marks the migration as complete which sets mdm_migration_completed = mdm_migration_deadline. Once this is set setup experience will run as normal unless mdm_migration_completed gets set to NULL and/or mdm_migration_deadline gets set to a value in the future(which e.g. would happen if the customer assigned to another MDM server then assigned to migrate to fleet again) DB test failure is expected here because it won't like the migration timestamp but that is a necessary failure because this fix is going to be backported into 4.73 If some of the following don't apply, delete the relevant line. - [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) - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes - [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 For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results - [x] Alerted the release DRI if additional load testing is needed - [x] Checked table schema to confirm autoupdate - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * New Features * Tracks and stores Apple DEP MDM migration deadlines per device/host. * Detects “migration in progress” during DEP sync and check-in. * Automatically marks migration complete and skips Setup Assistant items while migration is in progress to prevent conflicts. * Bug Fixes * Improved DEP compatibility by updating the protocol version and User-Agent used for Apple’s APIs, reducing the chance of blocked or rejected requests. * Migrations * Adds fields to support migration deadlines and completion status (no action required). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Magnus Jensen <magnus@fleetdm.com>
Fixes #32096 The gist of the fix is that when syncing devices from DEP we save the migration deadline to our host_dep_assignments table. The next enrollment, which we assume should be the migration, looks at host_dep_assignments, sees that mdm_migration_deadline is non-Null and mdm_migration_completed is NULL, and uses that as the signal that a migration is in progress and skips enqueuing setup experience items. It then marks the migration as complete which sets mdm_migration_completed = mdm_migration_deadline. Once this is set setup experience will run as normal unless mdm_migration_completed gets set to NULL and/or mdm_migration_deadline gets set to a value in the future(which e.g. would happen if the customer assigned to another MDM server then assigned to migrate to fleet again) DB test failure is expected here because it won't like the migration timestamp but that is a necessary failure because this fix is going to be backported into 4.73 If some of the following don't apply, delete the relevant line. - [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) - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes - [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 For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results - [x] Alerted the release DRI if additional load testing is needed - [x] Checked table schema to confirm autoupdate - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * New Features * Tracks and stores Apple DEP MDM migration deadlines per device/host. * Detects “migration in progress” during DEP sync and check-in. * Automatically marks migration complete and skips Setup Assistant items while migration is in progress to prevent conflicts. * Bug Fixes * Improved DEP compatibility by updating the protocol version and User-Agent used for Apple’s APIs, reducing the chance of blocked or rejected requests. * Migrations * Adds fields to support migration deadlines and completion status (no action required). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Magnus Jensen <magnus@fleetdm.com>
2 participants
Fixes #32096
The gist of the fix is that when syncing devices from DEP we save the migration deadline to our host_dep_assignments table. The next enrollment, which we assume should be the migration, looks at host_dep_assignments, sees that mdm_migration_deadline is non-Null and mdm_migration_completed is NULL, and uses that as the signal that a migration is in progress and skips enqueuing setup experience items. It then marks the migration as complete which sets mdm_migration_completed = mdm_migration_deadline. Once this is set setup experience will run as normal unless mdm_migration_completed gets set to NULL and/or mdm_migration_deadline gets set to a value in the future(which e.g. would happen if the customer assigned to another MDM server then assigned to migrate to fleet again)
DB test failure is expected here because it won't like the migration timestamp but that is a necessary failure because this fix is going to be backported into 4.73
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)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
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
New Features
Bug Fixes
Migrations