Restore host_mdm.server_url on Apple MDM re-enrollment - #50234
Draft
ScottifyShopson wants to merge 4 commits into
Draft
Restore host_mdm.server_url on Apple MDM re-enrollment#50234ScottifyShopson wants to merge 4 commits into
ScottifyShopson wants to merge 4 commits into
Conversation
MDMTurnOff clears server_url. upsertMDMAppleHostMDMInfoDB only rewrote enrolled and is_personal_enrollment on conflict, so iOS/iPadOS hosts that unenrolled and re-enrolled kept an empty server_url. Also update server_url on duplicate key. Fixes fleetdm#50187 🤖 Generated with xai/grok-4.5 via pi
Covers unenroll → re-enroll for iOS/iPadOS so an empty server_url after conflict upsert cannot regress silently. 🤖 Generated with xai/grok-4.5 via pi
4 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Apple MDM host enrollment upsert logic so that when an already-known host re-enrolls, host_mdm.server_url is refreshed instead of remaining blank after an unenroll.
Changes:
- Update
upsertMDMAppleHostMDMInfoDBto setserver_urlin theON DUPLICATE KEY UPDATEclause forhost_mdm.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, err = tx.ExecContext(ctx, fmt.Sprintf(` | ||
| INSERT INTO host_mdm (enrolled, server_url, installed_from_dep, mdm_id, is_server, host_id, is_personal_enrollment) VALUES %s | ||
| ON DUPLICATE KEY UPDATE enrolled = VALUES(enrolled), is_personal_enrollment = VALUES(is_personal_enrollment)`, strings.Join(parts, ",")), args...) | ||
| ON DUPLICATE KEY UPDATE enrolled = VALUES(enrolled), server_url = VALUES(server_url), is_personal_enrollment = VALUES(is_personal_enrollment)`, strings.Join(parts, ",")), args...) |
| _, err = tx.ExecContext(ctx, fmt.Sprintf(` | ||
| INSERT INTO host_mdm (enrolled, server_url, installed_from_dep, mdm_id, is_server, host_id, is_personal_enrollment) VALUES %s | ||
| ON DUPLICATE KEY UPDATE enrolled = VALUES(enrolled), is_personal_enrollment = VALUES(is_personal_enrollment)`, strings.Join(parts, ",")), args...) | ||
| ON DUPLICATE KEY UPDATE enrolled = VALUES(enrolled), server_url = VALUES(server_url), is_personal_enrollment = VALUES(is_personal_enrollment)`, strings.Join(parts, ",")), args...) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
server/datastore/mysql/apple_mdm.go:2076
- MDMTurnOff clears host_mdm.mdm_id (sets it to NULL), but this upsert still doesn’t restore mdm_id (or installed_from_dep) on the ON DUPLICATE KEY UPDATE path. That means a re-enrolled device can end up with server_url restored but mdm_id still NULL, which doesn’t match the expected “same as first-time enrollment” behavior described in #50187.
ON DUPLICATE KEY UPDATE enrolled = VALUES(enrolled), server_url = VALUES(server_url), is_personal_enrollment = VALUES(is_personal_enrollment)`, strings.Join(parts, ",")), args...)
server/datastore/mysql/apple_mdm_test.go:8213
- Add an assertion that mdm_id becomes NULL after MDMTurnOff (it is set to NULL in the UPDATE), so the test verifies the unenroll state before reenrolling.
afterTurnOff := readHostMDM(h.ID)
require.False(t, afterTurnOff.Enrolled)
require.Empty(t, afterTurnOff.ServerURL)
server/datastore/mysql/apple_mdm_test.go:8227
- Add an assertion that mdm_id is restored on re-enroll (ON DUPLICATE KEY UPDATE path), ideally matching the initial mdm_id.
afterReenroll := readHostMDM(h.ID)
require.True(t, afterReenroll.Enrolled, "platform %s", platform)
require.Equal(t, expectedServerURL, afterReenroll.ServerURL, "platform %s", platform)
server/datastore/mysql/apple_mdm_test.go:8206
- Add assertions that mdm_id is cleared on unenroll and restored on re-enroll; otherwise the test can pass even if reenroll still leaves mdm_id NULL.
initial := readHostMDM(h.ID)
require.True(t, initial.Enrolled)
require.Equal(t, expectedServerURL, initial.ServerURL)
server/datastore/mysql/apple_mdm_test.go:8181
- This regression test only verifies server_url. Since MDMTurnOff also clears host_mdm.mdm_id, it’s worth asserting mdm_id is NULL after unenroll and restored after re-enroll so the test fully covers the re-enrollment restoration behavior.
This issue also appears in the following locations of the same file:
- line 8204
- line 8211
- line 8225
type hostMDMRow struct {
Enrolled bool `db:"enrolled"`
ServerURL string `db:"server_url"`
}
server/datastore/mysql/apple_mdm_test.go:8187
- If you include mdm_id in hostMDMRow, update the SELECT to fetch it; otherwise the new assertion will never be populated.
`SELECT enrolled, server_url FROM host_mdm WHERE host_id = ?`, hostID)
})
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #50234 +/- ##
==========================================
+ Coverage 68.14% 68.16% +0.02%
==========================================
Files 3934 3935 +1
Lines 250914 250969 +55
Branches 13436 13436
==========================================
+ Hits 170983 171077 +94
+ Misses 64599 64561 -38
+ Partials 15332 15331 -1
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:
|
🤖 Generated with xai/grok-4.5 via pi
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
server/datastore/mysql/apple_mdm.go:2076
- MDMTurnOff clears both
host_mdm.server_urlandhost_mdm.mdm_id(seeMDMTurnOffUPDATE host_mdm), but this upsert still only restoresserver_urlon the duplicate-key path. After a device unenrolls and re-enrolls,mdm_idwill remain NULL, which can break joins/filters that rely onhmdm.mdm_id(e.g. host list filtering by MDM ID/name). This also doesn’t fully match the linked issue’s expected behavior (restore server_url and mdm_id).
_, err = tx.ExecContext(ctx, fmt.Sprintf(`
INSERT INTO host_mdm (enrolled, server_url, installed_from_dep, mdm_id, is_server, host_id, is_personal_enrollment) VALUES %s
ON DUPLICATE KEY UPDATE enrolled = VALUES(enrolled), server_url = VALUES(server_url), is_personal_enrollment = VALUES(is_personal_enrollment)`, strings.Join(parts, ",")), args...)
Comment on lines
+8178
to
+8187
| type hostMDMRow struct { | ||
| Enrolled bool `db:"enrolled"` | ||
| ServerURL string `db:"server_url"` | ||
| } | ||
| readHostMDM := func(hostID uint) hostMDMRow { | ||
| var row hostMDMRow | ||
| ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error { | ||
| return sqlx.GetContext(ctx, q, &row, | ||
| `SELECT enrolled, server_url FROM host_mdm WHERE host_id = ?`, hostID) | ||
| }) |
Comment on lines
+8204
to
+8213
| initial := readHostMDM(h.ID) | ||
| require.True(t, initial.Enrolled) | ||
| require.Equal(t, expectedServerURL, initial.ServerURL) | ||
|
|
||
| _, _, err = ds.MDMTurnOff(ctx, uuid) | ||
| require.NoError(t, err) | ||
|
|
||
| afterTurnOff := readHostMDM(h.ID) | ||
| require.False(t, afterTurnOff.Enrolled) | ||
| require.Empty(t, afterTurnOff.ServerURL) |
Comment on lines
+8225
to
+8227
| afterReenroll := readHostMDM(h.ID) | ||
| require.True(t, afterReenroll.Enrolled, "platform %s", platform) | ||
| require.Equal(t, expectedServerURL, afterReenroll.ServerURL, "platform %s", platform) |
🤖 Generated with openai/gpt-5.6-sol via pi
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
server/datastore/mysql/apple_mdm_test.go:8187
- Once hostMDMRow includes mdm_id, the SELECT should fetch it; otherwise mdm_id assertions will always use the zero value.
ExecAdhocSQL(t, ds, func(q sqlx.ExtContext) error {
return sqlx.GetContext(ctx, q, &row,
`SELECT enrolled, server_url FROM host_mdm WHERE host_id = ?`, hostID)
})
server/datastore/mysql/apple_mdm_test.go:8213
- After MDMTurnOff, mdm_id is expected to be NULL (and should be restored on re-enroll). Add an assertion so the test exercises that full unenroll/reenroll state transition.
afterTurnOff := readHostMDM(h.ID)
require.False(t, afterTurnOff.Enrolled)
require.Empty(t, afterTurnOff.ServerURL)
server/datastore/mysql/apple_mdm_test.go:8227
- To fully prevent regressions, assert that mdm_id is restored and non-zero after re-enrollment (the duplicate-key update path).
afterReenroll := readHostMDM(h.ID)
require.True(t, afterReenroll.Enrolled, "platform %s", platform)
require.Equal(t, expectedServerURL, afterReenroll.ServerURL, "platform %s", platform)
server/datastore/mysql/apple_mdm.go:2076
- MDMTurnOff clears host_mdm.mdm_id to NULL (apple_mdm.go:2207), but this upsert only restores server_url on the duplicate-key path. On re-enrollment the row will keep mdm_id=NULL, which can break invariants and does not match the expected behavior in #50187. Update the ON DUPLICATE KEY UPDATE clause to restore mdm_id as well.
_, err = tx.ExecContext(ctx, fmt.Sprintf(`
INSERT INTO host_mdm (enrolled, server_url, installed_from_dep, mdm_id, is_server, host_id, is_personal_enrollment) VALUES %s
ON DUPLICATE KEY UPDATE enrolled = VALUES(enrolled), server_url = VALUES(server_url), is_personal_enrollment = VALUES(is_personal_enrollment)`, strings.Join(parts, ",")), args...)
server/datastore/mysql/apple_mdm_test.go:8181
- This regression test only reads enrolled/server_url; it should also read mdm_id so it can assert that re-enrollment restores mdm_id after MDMTurnOff sets it to NULL.
This issue also appears in the following locations of the same file:
- line 8184
- line 8211
- line 8225
type hostMDMRow struct {
Enrolled bool `db:"enrolled"`
ServerURL string `db:"server_url"`
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Fixes #50187
Problem
MDMTurnOffclearshost_mdm.server_url. On re-enroll,upsertMDMAppleHostMDMInfoDBonly updatedenrolledandis_personal_enrollmenton conflict, so the cleared URL stayed empty.macOS often self-heals via osquery. iOS/iPadOS do not, so they keep
mdm.server_url: ""forever while still showingconnected_to_fleet: true.Fix
Also set
server_urlonON DUPLICATE KEY UPDATE.Minimal variant of #50188 (server_url only).
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
🤖 Generated with xai/grok-4.5 via pi